cmd: add integration test for rollback functionality (backport #7315) (#7368)

* cmd: add integration test and fix bug in rollback command (#7315)

(cherry picked from commit bca2080c01)

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

cmd/tendermint/commands/rollback.go

@@ -43,6 +43,10 @@ func RollbackState(config *cfg.Config) (int64, []byte, error) {
 	if err != nil {
 		return -1, nil, err
 	}
+	defer func() {
+		_ = blockStore.Close()
+		_ = stateStore.Close()
+	}()
 
 	// rollback the last state
 	return state.Rollback(blockStore, stateStore)

evidence/pool.go

@@ -256,6 +256,10 @@ func (evpool *Pool) State() sm.State {
 	return evpool.state
 }
 
+func (evpool *Pool) Close() error {
+	return evpool.evidenceStore.Close()
+}
+
 // IsExpired checks whether evidence or a polc is expired by checking whether a height and time is older
 // than set by the evidence consensus parameters
 func (evpool *Pool) isExpired(height int64, time time.Time) bool {

node/node.go

@@ -1015,6 +1015,16 @@ func (n *Node) OnStop() {
 			n.Logger.Error("Prometheus HTTP server Shutdown", "err", err)
 		}
 	}
+	if n.blockStore != nil {
+		if err := n.blockStore.Close(); err != nil {
+			n.Logger.Error("problem closing blockstore", "err", err)
+		}
+	}
+	if n.stateStore != nil {
+		if err := n.stateStore.Close(); err != nil {
+			n.Logger.Error("problem closing statestore", "err", err)
+		}
+	}
 }
 
 // ConfigureRPC makes sure RPC has all the objects it needs to operate.

state/mocks/store.go

@@ -33,6 +33,20 @@ func (_m *Store) Bootstrap(_a0 state.State) error {
 	return r0
 }
 
+// Close provides a mock function with given fields:
+func (_m *Store) Close() error {
+	ret := _m.Called()
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func() error); ok {
+		r0 = rf()
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
 // Load provides a mock function with given fields:
 func (_m *Store) Load() (state.State, error) {
 	ret := _m.Called()

state/rollback.go

@@ -21,18 +21,35 @@ func Rollback(bs BlockStore, ss Store) (int64, []byte, error) {
 		return -1, nil, errors.New("no state found")
 	}
 
-	rollbackHeight := invalidState.LastBlockHeight
+	height := bs.Height()
+
+	// 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. In this situation
+	// we don't need to rollback any state and can just return early
+	if height == invalidState.LastBlockHeight+1 {
+		return invalidState.LastBlockHeight, invalidState.AppHash, nil
+	}
+
+	// If the state store isn't one below nor equal to the blockstore height than this violates the
+	// invariant
+	if height != invalidState.LastBlockHeight {
+		return -1, nil, fmt.Errorf("statestore height (%d) is not one below or equal to blockstore height (%d)",
+			invalidState.LastBlockHeight, height)
+	}
+
+	// state store height is equal to blockstore height. We're good to proceed with rolling back state
+	rollbackHeight := invalidState.LastBlockHeight - 1
 	rollbackBlock := bs.LoadBlockMeta(rollbackHeight)
 	if rollbackBlock == nil {
 		return -1, nil, fmt.Errorf("block at height %d not found", rollbackHeight)
 	}
 
-	previousValidatorSet, err := ss.LoadValidators(rollbackHeight - 1)
+	previousLastValidatorSet, err := ss.LoadValidators(rollbackHeight)
 	if err != nil {
 		return -1, nil, err
 	}
 
-	previousParams, err := ss.LoadConsensusParams(rollbackHeight)
+	previousParams, err := ss.LoadConsensusParams(rollbackHeight + 1)
 	if err != nil {
 		return -1, nil, err
 	}
@@ -40,13 +57,13 @@ func Rollback(bs BlockStore, ss Store) (int64, []byte, error) {
 	valChangeHeight := invalidState.LastHeightValidatorsChanged
 	// this can only happen if the validator set changed since the last block
 	if valChangeHeight > rollbackHeight {
-		valChangeHeight = rollbackHeight
+		valChangeHeight = rollbackHeight + 1
 	}
 
 	paramsChangeHeight := invalidState.LastHeightConsensusParamsChanged
 	// this can only happen if params changed from the last block
 	if paramsChangeHeight > rollbackHeight {
-		paramsChangeHeight = rollbackHeight
+		paramsChangeHeight = rollbackHeight + 1
 	}
 
 	// build the new state from the old state and the prior block
@@ -62,13 +79,13 @@ func Rollback(bs BlockStore, ss Store) (int64, []byte, error) {
 		ChainID:       invalidState.ChainID,
 		InitialHeight: invalidState.InitialHeight,
 
-		LastBlockHeight: invalidState.LastBlockHeight - 1,
-		LastBlockID:     rollbackBlock.Header.LastBlockID,
+		LastBlockHeight: rollbackBlock.Header.Height,
+		LastBlockID:     rollbackBlock.BlockID,
 		LastBlockTime:   rollbackBlock.Header.Time,
 
 		NextValidators:              invalidState.Validators,
 		Validators:                  invalidState.LastValidators,
-		LastValidators:              previousValidatorSet,
+		LastValidators:              previousLastValidatorSet,
 		LastHeightValidatorsChanged: valChangeHeight,
 
 		ConsensusParams:                  previousParams,

state/rollback_test.go

@@ -17,19 +17,97 @@ import (
 )
 
 func TestRollback(t *testing.T) {
+	var (
+		height     int64 = 100
+		nextHeight int64 = 101
+	)
+	blockStore := &mocks.BlockStore{}
+	stateStore := setupStateStore(t, height)
+	initialState, err := stateStore.Load()
+	require.NoError(t, err)
+
+	// perform the rollback over a version bump
+	newParams := types.DefaultConsensusParams()
+	newParams.Version.AppVersion = 11
+	newParams.Block.MaxBytes = 1000
+	nextState := initialState.Copy()
+	nextState.LastBlockHeight = nextHeight
+	nextState.Version.Consensus.App = 11
+	nextState.LastBlockID = makeBlockIDRandom()
+	nextState.AppHash = tmhash.Sum([]byte("app_hash"))
+	nextState.LastValidators = initialState.Validators
+	nextState.Validators = initialState.NextValidators
+	nextState.NextValidators = initialState.NextValidators.CopyIncrementProposerPriority(1)
+	nextState.ConsensusParams = *newParams
+	nextState.LastHeightConsensusParamsChanged = nextHeight + 1
+	nextState.LastHeightValidatorsChanged = nextHeight + 1
+
+	// update the state
+	require.NoError(t, stateStore.Save(nextState))
+
+	block := &types.BlockMeta{
+		BlockID: initialState.LastBlockID,
+		Header: types.Header{
+			Height:          initialState.LastBlockHeight,
+			AppHash:         initialState.AppHash,
+			LastBlockID:     makeBlockIDRandom(),
+			LastResultsHash: initialState.LastResultsHash,
+		},
+	}
+	blockStore.On("LoadBlockMeta", initialState.LastBlockHeight).Return(block)
+	blockStore.On("Height").Return(nextHeight)
+
+	// rollback the state
+	rollbackHeight, rollbackHash, err := state.Rollback(blockStore, stateStore)
+	require.NoError(t, err)
+	require.EqualValues(t, height, rollbackHeight)
+	require.EqualValues(t, initialState.AppHash, rollbackHash)
+	blockStore.AssertExpectations(t)
+
+	// assert that we've recovered the prior state
+	loadedState, err := stateStore.Load()
+	require.NoError(t, err)
+	require.EqualValues(t, initialState, loadedState)
+}
+
+func TestRollbackNoState(t *testing.T) {
 	stateStore := state.NewStore(dbm.NewMemDB())
 	blockStore := &mocks.BlockStore{}
-	var (
-		height     int64  = 100
-		appVersion uint64 = 10
-	)
 
+	_, _, err := state.Rollback(blockStore, stateStore)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "no state found")
+}
+
+func TestRollbackNoBlocks(t *testing.T) {
+	const height = int64(100)
+	stateStore := setupStateStore(t, height)
+	blockStore := &mocks.BlockStore{}
+	blockStore.On("Height").Return(height)
+	blockStore.On("LoadBlockMeta", height-1).Return(nil)
+
+	_, _, err := state.Rollback(blockStore, stateStore)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "block at height 99 not found")
+}
+
+func TestRollbackDifferentStateHeight(t *testing.T) {
+	const height = int64(100)
+	stateStore := setupStateStore(t, height)
+	blockStore := &mocks.BlockStore{}
+	blockStore.On("Height").Return(height + 2)
+
+	_, _, err := state.Rollback(blockStore, stateStore)
+	require.Error(t, err)
+	require.Equal(t, err.Error(), "statestore height (100) is not one below or equal to blockstore height (102)")
+}
+
+func setupStateStore(t *testing.T, height int64) state.Store {
+	stateStore := state.NewStore(dbm.NewMemDB())
 	valSet, _ := types.RandValidatorSet(5, 10)
 
 	params := types.DefaultConsensusParams()
-	params.Version.AppVersion = appVersion
-	newParams := types.DefaultConsensusParams()
-	newParams.Block.MaxBytes = 10000
+	params.Version.AppVersion = 10
 
 	initialState := state.State{
 		Version: tmstate.Version{
@@ -53,99 +131,7 @@ func TestRollback(t *testing.T) {
 		LastHeightConsensusParamsChanged: height + 1,
 	}
 	require.NoError(t, stateStore.Bootstrap(initialState))
-
-	height++
-	block := &types.BlockMeta{
-		Header: types.Header{
-			Height:          height,
-			AppHash:         initialState.AppHash,
-			LastBlockID:     initialState.LastBlockID,
-			LastResultsHash: initialState.LastResultsHash,
-		},
-	}
-	blockStore.On("LoadBlockMeta", height).Return(block)
-
-	appVersion++
-	newParams.Version.AppVersion = appVersion
-	nextState := initialState.Copy()
-	nextState.LastBlockHeight = height
-	nextState.Version.Consensus.App = appVersion
-	nextState.LastBlockID = makeBlockIDRandom()
-	nextState.AppHash = tmhash.Sum([]byte("next_app_hash"))
-	nextState.LastValidators = initialState.Validators
-	nextState.Validators = initialState.NextValidators
-	nextState.NextValidators = initialState.NextValidators.CopyIncrementProposerPriority(1)
-	nextState.ConsensusParams = *newParams
-	nextState.LastHeightConsensusParamsChanged = height + 1
-	nextState.LastHeightValidatorsChanged = height + 1
-
-	// update the state
-	require.NoError(t, stateStore.Save(nextState))
-
-	// rollback the state
-	rollbackHeight, rollbackHash, err := state.Rollback(blockStore, stateStore)
-	require.NoError(t, err)
-	require.EqualValues(t, int64(100), rollbackHeight)
-	require.EqualValues(t, initialState.AppHash, rollbackHash)
-	blockStore.AssertExpectations(t)
-
-	// assert that we've recovered the prior state
-	loadedState, err := stateStore.Load()
-	require.NoError(t, err)
-	require.EqualValues(t, initialState, loadedState)
-}
-
-func TestRollbackNoState(t *testing.T) {
-	stateStore := state.NewStore(dbm.NewMemDB())
-	blockStore := &mocks.BlockStore{}
-
-	_, _, err := state.Rollback(blockStore, stateStore)
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "no state found")
-}
-
-func TestRollbackNoBlocks(t *testing.T) {
-	stateStore := state.NewStore(dbm.NewMemDB())
-	blockStore := &mocks.BlockStore{}
-	var (
-		height     int64  = 100
-		appVersion uint64 = 10
-	)
-
-	valSet, _ := types.RandValidatorSet(5, 10)
-
-	params := types.DefaultConsensusParams()
-	params.Version.AppVersion = appVersion
-	newParams := types.DefaultConsensusParams()
-	newParams.Block.MaxBytes = 10000
-
-	initialState := state.State{
-		Version: tmstate.Version{
-			Consensus: tmversion.Consensus{
-				Block: version.BlockProtocol,
-				App:   10,
-			},
-			Software: version.TMCoreSemVer,
-		},
-		ChainID:                          "test-chain",
-		InitialHeight:                    10,
-		LastBlockID:                      makeBlockIDRandom(),
-		AppHash:                          tmhash.Sum([]byte("app_hash")),
-		LastResultsHash:                  tmhash.Sum([]byte("last_results_hash")),
-		LastBlockHeight:                  height,
-		LastValidators:                   valSet,
-		Validators:                       valSet.CopyIncrementProposerPriority(1),
-		NextValidators:                   valSet.CopyIncrementProposerPriority(2),
-		LastHeightValidatorsChanged:      height + 1,
-		ConsensusParams:                  *params,
-		LastHeightConsensusParamsChanged: height + 1,
-	}
-	require.NoError(t, stateStore.Save(initialState))
-	blockStore.On("LoadBlockMeta", height).Return(nil)
-
-	_, _, err := state.Rollback(blockStore, stateStore)
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "block at height 100 not found")
+	return stateStore
 }
 
 func makeBlockIDRandom() types.BlockID {

state/store.go

@@ -68,6 +68,8 @@ type Store interface {
 	Bootstrap(State) error
 	// PruneStates takes the height from which to start prning and which height stop at
 	PruneStates(int64, int64) error
+	// Close closes the connection with the database
+	Close() error
 }
 
 // dbStore wraps a db (github.com/tendermint/tm-db)
@@ -593,3 +595,7 @@ func (store dbStore) saveConsensusParamsInfo(nextHeight, changeHeight int64, par
 
 	return nil
 }
+
+func (store dbStore) Close() error {
+	return store.db.Close()
+}

store/store.go

@@ -424,6 +424,10 @@ func (bs *BlockStore) SaveSeenCommit(height int64, seenCommit *types.Commit) err
 	return bs.db.Set(calcSeenCommitKey(height), seenCommitBytes)
 }
 
+func (bs *BlockStore) Close() error {
+	return bs.db.Close()
+}
+
 //-----------------------------------------------------------------------------
 
 func calcBlockMetaKey(height int64) []byte {

test/e2e/app/app.go

@@ -79,7 +79,7 @@ func DefaultConfig(dir string) *Config {
 
 // NewApplication creates the application.
 func NewApplication(cfg *Config) (*Application, error) {
-	state, err := NewState(filepath.Join(cfg.Dir, "state.json"), cfg.PersistInterval)
+	state, err := NewState(cfg.Dir, cfg.PersistInterval)
 	if err != nil {
 		return nil, err
 	}
@@ -254,6 +254,10 @@ func (app *Application) ApplySnapshotChunk(req abci.RequestApplySnapshotChunk) a
 	return abci.ResponseApplySnapshotChunk{Result: abci.ResponseApplySnapshotChunk_ACCEPT}
 }
 
+func (app *Application) Rollback() error {
+	return app.state.Rollback()
+}
+
 // validatorUpdates generates a validator set update.
 func (app *Application) validatorUpdates(height uint64) (abci.ValidatorUpdates, error) {
 	updates := app.cfg.ValidatorUpdates[fmt.Sprintf("%v", height)]

test/e2e/app/state.go

@@ -8,10 +8,14 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"sort"
 	"sync"
 )
 
+const stateFileName = "app_state.json"
+const prevStateFileName = "prev_app_state.json"
+
 // State is the application state.
 type State struct {
 	sync.RWMutex
@@ -20,16 +24,19 @@ type State struct {
 	Hash   []byte
 
 	// private fields aren't marshaled to disk.
-	file            string
+	currentFile string
+	// app saves current and previous state for rollback functionality
+	previousFile    string
 	persistInterval uint64
 	initialHeight   uint64
 }
 
 // NewState creates a new state.
-func NewState(file string, persistInterval uint64) (*State, error) {
+func NewState(dir string, persistInterval uint64) (*State, error) {
 	state := &State{
 		Values:          make(map[string]string),
-		file:            file,
+		currentFile:     filepath.Join(dir, stateFileName),
+		previousFile:    filepath.Join(dir, prevStateFileName),
 		persistInterval: persistInterval,
 	}
 	state.Hash = hashItems(state.Values)
@@ -45,13 +52,22 @@ func NewState(file string, persistInterval uint64) (*State, error) {
 // load loads state from disk. It does not take out a lock, since it is called
 // during construction.
 func (s *State) load() error {
-	bz, err := ioutil.ReadFile(s.file)
+	bz, err := ioutil.ReadFile(s.currentFile)
 	if err != nil {
-		return fmt.Errorf("failed to read state from %q: %w", s.file, err)
+		// if the current state doesn't exist then we try recover from the previous state
+		if errors.Is(err, os.ErrNotExist) {
+			bz, err = ioutil.ReadFile(s.previousFile)
+			if err != nil {
+				return fmt.Errorf("failed to read both current and previous state (%q): %w",
+					s.previousFile, err)
+			}
+		} else {
+			return fmt.Errorf("failed to read state from %q: %w", s.currentFile, err)
+		}
 	}
 	err = json.Unmarshal(bz, s)
 	if err != nil {
-		return fmt.Errorf("invalid state data in %q: %w", s.file, err)
+		return fmt.Errorf("invalid state data in %q: %w", s.currentFile, err)
 	}
 	return nil
 }
@@ -65,12 +81,19 @@ func (s *State) save() error {
 	}
 	// We write the state to a separate file and move it to the destination, to
 	// make it atomic.
-	newFile := fmt.Sprintf("%v.new", s.file)
+	newFile := fmt.Sprintf("%v.new", s.currentFile)
 	err = ioutil.WriteFile(newFile, bz, 0644)
 	if err != nil {
-		return fmt.Errorf("failed to write state to %q: %w", s.file, err)
+		return fmt.Errorf("failed to write state to %q: %w", s.currentFile, err)
 	}
-	return os.Rename(newFile, s.file)
+	// We take the current state and move it to the previous state, replacing it
+	if _, err := os.Stat(s.currentFile); err == nil {
+		if err := os.Rename(s.currentFile, s.previousFile); err != nil {
+			return fmt.Errorf("failed to replace previous state: %w", err)
+		}
+	}
+	// Finally, we take the new state and replace the current state.
+	return os.Rename(newFile, s.currentFile)
 }
 
 // Export exports key/value pairs as JSON, used for state sync snapshots.
@@ -136,6 +159,18 @@ func (s *State) Commit() (uint64, []byte, error) {
 	return s.Height, s.Hash, nil
 }
 
+func (s *State) Rollback() error {
+	bz, err := ioutil.ReadFile(s.previousFile)
+	if err != nil {
+		return fmt.Errorf("failed to read state from %q: %w", s.previousFile, err)
+	}
+	err = json.Unmarshal(bz, s)
+	if err != nil {
+		return fmt.Errorf("invalid state data in %q: %w", s.previousFile, err)
+	}
+	return nil
+}
+
 // hashItems hashes a set of key/value items.
 func hashItems(items map[string]string) []byte {
 	keys := make([]string, 0, len(items))