Rollback should not cause missing validatorset (#250)

* Add logs

* Fix

* Add logs

* Fix

* Fix

* Log

* log

* log

* Log

* Rollback should not cause missing validatorset issue

* Fix lint

* Fix comment

* Fix unit test

cmd/tendermint/commands/rollback.go

@@ -48,7 +48,6 @@ application.
 func RollbackState(config *config.Config, removeBlock bool) (int64, []byte, error) {
 	// use the parsed config to load the block and state store
 	blockStore, stateStore, err := loadStateAndBlockStore(config)
-	fmt.Printf("Current blockStore height=%d hash=%X\n", blockStore.Height(), blockStore.LoadSeenCommit().Hash())
 	if err != nil {
 		return -1, nil, err
 	}

internal/state/rollback.go

@@ -28,35 +28,34 @@ func resetPrivValidatorConfig(privValidatorConfig config.PrivValidatorConfig) er
 // recent previous state (height n - 1).
 // Note that this function does not affect application state.
 func Rollback(bs BlockStore, ss Store, removeBlock bool, privValidatorConfig *config.PrivValidatorConfig) (int64, []byte, error) {
-	// Only the latest state is stored
 	latestState, err := ss.Load()
-	fmt.Printf("Initial tendermint state height=%d, appHash=%X, lastResultHash=%X\n", latestState.LastBlockHeight, latestState.AppHash, latestState.LastResultsHash)
 	if err != nil {
 		return -1, nil, err
 	}
 	if latestState.IsEmpty() {
 		return -1, nil, errors.New("no state found")
 	}
 
-	height := bs.Height()
+	latestBlockHeight := bs.Height()
+	latestStateHeight := latestState.LastBlockHeight
+	fmt.Printf("Current blockStore height=%d tendermint state height=%d appHash=%X lastResultHash=%X\n", latestBlockHeight, latestStateHeight, latestState.AppHash, latestState.LastResultsHash)
+
 	// NOTE: persistence of state and blocks don't happen atomically. Therefore it is possible that
 	// when the user stopped the node the state wasn't updated but the blockstore was. Discard the
 	// pending block before continuing.
-	if height == latestState.LastBlockHeight+1 {
-		fmt.Printf("Invalid state in the latest block height=%d, removing it first \n", height)
-		if removeBlock {
-			if err := bs.DeleteLatestBlock(); err != nil {
-				return -1, nil, fmt.Errorf("failed to remove final block from blockstore: %w", err)
-			}
+	if latestBlockHeight == latestStateHeight+1 {
+		fmt.Printf("Invalid state in the latest block height=%d, removing it first \n", latestBlockHeight)
+		if err := bs.DeleteLatestBlock(); err != nil {
+			return -1, nil, fmt.Errorf("failed to remove final block from blockstore: %w", err)
 		}
 		return latestState.LastBlockHeight, latestState.AppHash, nil
 	}
 
 	// If the state store isn't one below nor equal to the blockstore height than this violates the
 	// invariant
-	if height != latestState.LastBlockHeight {
+	if latestBlockHeight != latestState.LastBlockHeight {
 		return -1, nil, fmt.Errorf("statestore height (%d) is not one below or equal to blockstore height (%d)",
-			latestState.LastBlockHeight, height)
+			latestState.LastBlockHeight, latestBlockHeight)
 	}
 
 	// state store height is equal to blockstore height. We're good to proceed with rolling back state
@@ -72,20 +71,23 @@ func Rollback(bs BlockStore, ss Store, removeBlock bool, privValidatorConfig *co
 		return -1, nil, fmt.Errorf("block at height %d not found", latestState.LastBlockHeight)
 	}
 
-	previousLastValidatorSet, err := ss.LoadValidators(rollbackHeight)
-	if err != nil {
-		return -1, nil, err
-	}
-
 	previousParams, err := ss.LoadConsensusParams(rollbackHeight + 1)
 	if err != nil {
 		return -1, nil, err
 	}
 
 	valChangeHeight := latestState.LastHeightValidatorsChanged
-	// this can only happen if the validator set changed since the last block
 	if valChangeHeight > rollbackHeight {
-		valChangeHeight = rollbackHeight + 1
+		valInfo, err := ss.(dbStore).LoadValidatorsInfo(rollbackHeight)
+		if err != nil {
+			return -1, nil, err
+		}
+		valChangeHeight = valInfo.LastHeightChanged
+	}
+
+	previousLastValidatorSet, err := ss.LoadValidators(rollbackHeight)
+	if err != nil {
+		return -1, nil, err
 	}
 
 	paramsChangeHeight := latestState.LastHeightConsensusParamsChanged
@@ -94,7 +96,7 @@ func Rollback(bs BlockStore, ss Store, removeBlock bool, privValidatorConfig *co
 		paramsChangeHeight = rollbackHeight + 1
 	}
 
-	lastBlockHeight := rollbackBlock.Header.Height
+	rolledBackHeight := rollbackBlock.Header.Height
 	rolledBackAppHash := latestBlock.Header.AppHash
 	rolledBackLastResultHash := latestBlock.Header.LastResultsHash
 
@@ -114,7 +116,7 @@ func Rollback(bs BlockStore, ss Store, removeBlock bool, privValidatorConfig *co
 		ChainID:       latestState.ChainID,
 		InitialHeight: latestState.InitialHeight,
 
-		LastBlockHeight: lastBlockHeight,
+		LastBlockHeight: rolledBackHeight,
 		LastBlockID:     rollbackBlock.BlockID,
 		LastBlockTime:   rollbackBlock.Header.Time,
 
@@ -150,6 +152,6 @@ func Rollback(bs BlockStore, ss Store, removeBlock bool, privValidatorConfig *co
 		}
 	}
 
-	fmt.Printf("Saved tendermint state height=%d, appHash=%X, lastResultHash=%X\n", lastBlockHeight, rolledBackState.AppHash, rolledBackState.LastResultsHash)
-	return lastBlockHeight, rolledBackState.AppHash, nil
+	fmt.Printf("Saved tendermint state height=%d, appHash=%X, lastResultHash=%X\n", rolledBackHeight, rolledBackState.AppHash, rolledBackState.LastResultsHash)
+	return rolledBackHeight, rolledBackState.AppHash, nil
 }

internal/state/rollback_test.go

@@ -149,7 +149,7 @@ func setupStateStore(t *testing.T, height int64) state.Store {
 		LastValidators:                   valSet,
 		Validators:                       valSet.CopyIncrementProposerPriority(1),
 		NextValidators:                   valSet.CopyIncrementProposerPriority(2),
-		LastHeightValidatorsChanged:      height + 1,
+		LastHeightValidatorsChanged:      height,
 		ConsensusParams:                  *params,
 		LastHeightConsensusParamsChanged: height + 1,
 	}

internal/state/store.go

@@ -507,7 +507,6 @@ func (store dbStore) SaveValidatorSets(lowerHeight, upperHeight int64, vals *typ
 // LoadValidators loads the ValidatorSet for a given height.
 // Returns ErrNoValSetForHeight if the validator set can't be found for this height.
 func (store dbStore) LoadValidators(height int64) (*types.ValidatorSet, error) {
-
 	valInfo, err := loadValidatorsInfo(store.db, height)
 	if err != nil {
 		return nil, ErrNoValSetForHeight{Height: height, Err: err}
@@ -556,6 +555,14 @@ func lastStoredHeightFor(height, lastHeightChanged int64) int64 {
 	return tmmath.MaxInt64(checkpointHeight, lastHeightChanged)
 }
 
+func (store dbStore) LoadValidatorsInfo(height int64) (*tmstate.ValidatorsInfo, error) {
+	valInfo, err := loadValidatorsInfo(store.db, height)
+	if err != nil {
+		return nil, ErrNoValSetForHeight{Height: height, Err: err}
+	}
+	return valInfo, nil
+}
+
 // CONTRACT: Returned ValidatorsInfo can be mutated.
 func loadValidatorsInfo(db dbm.DB, height int64) (*tmstate.ValidatorsInfo, error) {
 	buf, err := db.Get(validatorsKey(height))