From 4425e62e9e894ce3ea4d2ae59919c0a33b91fa34 Mon Sep 17 00:00:00 2001 From: Callum Waters Date: Sat, 19 Feb 2022 17:13:16 +0100 Subject: [PATCH] statesync: assert app version matches (#7856) Reinstates: #7463 which was overridden by the following commit --- internal/statesync/syncer.go | 28 +++++++++++++++++----------- internal/statesync/syncer_test.go | 26 +++++++++++++------------- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/internal/statesync/syncer.go b/internal/statesync/syncer.go index c22a59c38..e2e41586c 100644 --- a/internal/statesync/syncer.go +++ b/internal/statesync/syncer.go @@ -348,12 +348,10 @@ func (s *syncer) Sync(ctx context.Context, snapshot *snapshot, chunks *chunkQueu return sm.State{}, nil, err } - // Verify app and update app version - appVersion, err := s.verifyApp(ctx, snapshot) - if err != nil { + // Verify app and app version + if err := s.verifyApp(ctx, snapshot, state.Version.Consensus.App); err != nil { return sm.State{}, nil, err } - state.Version.Consensus.App = appVersion // Done! 🎉 s.logger.Info("Snapshot restored", "height", snapshot.Height, "format", snapshot.Format, @@ -547,19 +545,27 @@ func (s *syncer) requestChunk(ctx context.Context, snapshot *snapshot, chunk uin return nil } -// verifyApp verifies the sync, checking the app hash and last block height. It returns the -// app version, which should be returned as part of the initial state. -func (s *syncer) verifyApp(ctx context.Context, snapshot *snapshot) (uint64, error) { +// verifyApp verifies the sync, checking the app hash, last block height and app version +func (s *syncer) verifyApp(ctx context.Context, snapshot *snapshot, appVersion uint64) error { resp, err := s.connQuery.Info(ctx, proxy.RequestInfo) if err != nil { - return 0, fmt.Errorf("failed to query ABCI app for appHash: %w", err) + return fmt.Errorf("failed to query ABCI app for appHash: %w", err) + } + + // sanity check that the app version in the block matches the application's own record + // of its version + if resp.AppVersion != appVersion { + // An error here most likely means that the app hasn't inplemented state sync + // or the Info call correctly + return fmt.Errorf("app version mismatch. Expected: %d, got: %d", + appVersion, resp.AppVersion) } if !bytes.Equal(snapshot.trustedAppHash, resp.LastBlockAppHash) { s.logger.Error("appHash verification failed", "expected", snapshot.trustedAppHash, "actual", resp.LastBlockAppHash) - return 0, errVerifyFailed + return errVerifyFailed } if uint64(resp.LastBlockHeight) != snapshot.Height { @@ -568,9 +574,9 @@ func (s *syncer) verifyApp(ctx context.Context, snapshot *snapshot) (uint64, err "expected", snapshot.Height, "actual", resp.LastBlockHeight, ) - return 0, errVerifyFailed + return errVerifyFailed } s.logger.Info("Verified ABCI app", "height", snapshot.Height, "appHash", snapshot.trustedAppHash) - return resp.AppVersion, nil + return nil } diff --git a/internal/statesync/syncer_test.go b/internal/statesync/syncer_test.go index be0c5b63d..7619a718e 100644 --- a/internal/statesync/syncer_test.go +++ b/internal/statesync/syncer_test.go @@ -30,7 +30,7 @@ func TestSyncer_SyncAny(t *testing.T) { Version: sm.Version{ Consensus: version.Consensus{ Block: version.BlockProtocol, - App: 0, + App: testAppVersion, }, Software: version.TMVersion, }, @@ -189,7 +189,7 @@ func TestSyncer_SyncAny(t *testing.T) { Index: 2, Chunk: []byte{1, 1, 2}, }).Once().Return(&abci.ResponseApplySnapshotChunk{Result: abci.ResponseApplySnapshotChunk_ACCEPT}, nil) connQuery.On("Info", mock.Anything, proxy.RequestInfo).Return(&abci.ResponseInfo{ - AppVersion: 9, + AppVersion: testAppVersion, LastBlockHeight: 1, LastBlockAppHash: []byte("app_hash"), }, nil) @@ -203,10 +203,7 @@ func TestSyncer_SyncAny(t *testing.T) { require.Equal(t, map[uint32]int{0: 1, 1: 2, 2: 1}, chunkRequests) chunkRequestsMtx.Unlock() - // The syncer should have updated the state app version from the ABCI info response. expectState := state - expectState.Version.Consensus.App = 9 - require.Equal(t, expectState, newState) require.Equal(t, commit, lastCommit) @@ -724,6 +721,8 @@ func TestSyncer_applyChunks_RejectSenders(t *testing.T) { func TestSyncer_verifyApp(t *testing.T) { boom := errors.New("boom") + const appVersion = 9 + appVersionMismatchErr := errors.New("app version mismatch. Expected: 9, got: 2") s := &snapshot{Height: 3, Format: 1, Chunks: 5, Hash: []byte{1, 2, 3}, trustedAppHash: []byte("app_hash")} testcases := map[string]struct { @@ -734,17 +733,22 @@ func TestSyncer_verifyApp(t *testing.T) { "verified": {&abci.ResponseInfo{ LastBlockHeight: 3, LastBlockAppHash: []byte("app_hash"), - AppVersion: 9, + AppVersion: appVersion, }, nil, nil}, + "invalid app version": {&abci.ResponseInfo{ + LastBlockHeight: 3, + LastBlockAppHash: []byte("app_hash"), + AppVersion: 2, + }, nil, appVersionMismatchErr}, "invalid height": {&abci.ResponseInfo{ LastBlockHeight: 5, LastBlockAppHash: []byte("app_hash"), - AppVersion: 9, + AppVersion: appVersion, }, nil, errVerifyFailed}, "invalid hash": {&abci.ResponseInfo{ LastBlockHeight: 3, LastBlockAppHash: []byte("xxx"), - AppVersion: 9, + AppVersion: appVersion, }, nil, errVerifyFailed}, "error": {nil, boom, boom}, } @@ -760,16 +764,12 @@ func TestSyncer_verifyApp(t *testing.T) { rts := setup(ctx, t, nil, nil, nil, 2) rts.connQuery.On("Info", mock.Anything, proxy.RequestInfo).Return(tc.response, tc.err) - version, err := rts.syncer.verifyApp(ctx, s) + err := rts.syncer.verifyApp(ctx, s, appVersion) unwrapped := errors.Unwrap(err) if unwrapped != nil { err = unwrapped } - require.Equal(t, tc.expectErr, err) - if err == nil { - require.Equal(t, tc.response.AppVersion, version) - } }) } }