state: Use last height changed if validator set is empty (#3560)

What happened:

New code was supposed to fall back to last height changed when/if it
failed to find validators at checkpoint height (to make release
non-breaking).

But because we did not check if validator set is empty, the fall back
logic was never executed => resulting in LoadValidators returning an
empty validator set for cases where `lastStoredHeight` is checkpoint
height (i.e. almost all heights if the application does not change
validator set often).

How it was found:

one of our users - @sunboshan reported a bug here
https://github.com/tendermint/tendermint/pull/3537#issuecomment-482711833

* use last height changed in validator set is empty
* add a changelog entry

CHANGELOG_PENDING.md

@@ -19,3 +19,4 @@
 ### IMPROVEMENTS:
 
 ### BUG FIXES:
+- [state] [\#3537](https://github.com/tendermint/tendermint/pull/3537#issuecomment-482711833) LoadValidators: do not return an empty validator set

state/store.go

@@ -193,7 +193,7 @@ func LoadValidators(db dbm.DB, height int64) (*types.ValidatorSet, error) {
 	if valInfo.ValidatorSet == nil {
 		lastStoredHeight := lastStoredHeightFor(height, valInfo.LastHeightChanged)
 		valInfo2 := loadValidatorsInfo(db, lastStoredHeight)
-		if valInfo2 == nil {
+		if valInfo2 == nil || valInfo2.ValidatorSet == nil {
 			// TODO (melekes): remove the below if condition in the 0.33 major
 			// release and just panic. Old chains might panic otherwise if they
 			// haven't saved validators at intermediate (%valSetCheckpointInterval)
@@ -201,7 +201,7 @@ func LoadValidators(db dbm.DB, height int64) (*types.ValidatorSet, error) {
 			// https://github.com/tendermint/tendermint/issues/3543
 			valInfo2 = loadValidatorsInfo(db, valInfo.LastHeightChanged)
 			lastStoredHeight = valInfo.LastHeightChanged
-			if valInfo2 == nil {
+			if valInfo2 == nil || valInfo2.ValidatorSet == nil {
 				panic(
 					fmt.Sprintf("Couldn't find validators at height %d (height %d was originally requested)",
 						lastStoredHeight,

state/store_test.go

@@ -6,34 +6,50 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	cfg "github.com/tendermint/tendermint/config"
 	dbm "github.com/tendermint/tendermint/libs/db"
 	"github.com/tendermint/tendermint/types"
 )
 
-func TestSaveValidatorsInfo(t *testing.T) {
+func TestStoreLoadValidators(t *testing.T) {
-	// test we persist validators every valSetCheckpointInterval blocks
 	stateDB := dbm.NewMemDB()
 	val, _ := types.RandValidator(true, 10)
 	vals := types.NewValidatorSet([]*types.Validator{val})
 
-	// TODO(melekes): remove in 0.33 release
+	// 1) LoadValidators loads validators using a height where they were last changed
-	// https://github.com/tendermint/tendermint/issues/3543
 	saveValidatorsInfo(stateDB, 1, 1, vals)
 	saveValidatorsInfo(stateDB, 2, 1, vals)
+	loadedVals, err := LoadValidators(stateDB, 2)
+	require.NoError(t, err)
+	assert.NotZero(t, loadedVals.Size())
+
+	// 2) LoadValidators loads validators using a checkpoint height
+
+	// TODO(melekes): REMOVE in 0.33 release
+	// https://github.com/tendermint/tendermint/issues/3543
+	// for releases prior to v0.31.4, it uses last height changed
+	valInfo := &ValidatorsInfo{
+		LastHeightChanged: valSetCheckpointInterval,
+	}
+	stateDB.Set(calcValidatorsKey(valSetCheckpointInterval), valInfo.Bytes())
 	assert.NotPanics(t, func() {
-		_, err := LoadValidators(stateDB, 2)
+		saveValidatorsInfo(stateDB, valSetCheckpointInterval+1, 1, vals)
+		loadedVals, err := LoadValidators(stateDB, valSetCheckpointInterval+1)
 		if err != nil {
-			panic(err)
+			t.Fatal(err)
+		}
+		if loadedVals.Size() == 0 {
+			t.Fatal("Expected validators to be non-empty")
 		}
 	})
-	//ENDREMOVE
+	// ENDREMOVE
 
 	saveValidatorsInfo(stateDB, valSetCheckpointInterval, 1, vals)
 
-	loadedVals, err := LoadValidators(stateDB, valSetCheckpointInterval)
+	loadedVals, err = LoadValidators(stateDB, valSetCheckpointInterval)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.NotZero(t, loadedVals.Size())
 }