statesync: assert app version matches (backport #7856) (#7886)

internal/statesync/reactor_test.go

@@ -202,7 +202,7 @@ func TestReactor_Sync(t *testing.T) {
 
 	// app query returns valid state app hash
 	rts.connQuery.On("InfoSync", ctx, proxy.RequestInfo).Return(&abci.ResponseInfo{
-		AppVersion:       9,
+		AppVersion:       0,
 		LastBlockHeight:  snapshotHeight,
 		LastBlockAppHash: chain[snapshotHeight+1].AppHash,
 	}, nil)

internal/statesync/syncer.go

@@ -365,12 +365,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(snapshot)
-	if err != nil {
+	// Verify app hash and 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,
@@ -562,19 +560,27 @@ func (s *syncer) requestChunk(snapshot *snapshot, chunk uint32) {
 	}
 }
 
-// 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(snapshot *snapshot) (uint64, error) {
-	resp, err := s.connQuery.InfoSync(context.Background(), proxy.RequestInfo)
+// 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.InfoSync(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 {
@@ -583,9 +589,9 @@ func (s *syncer) verifyApp(snapshot *snapshot) (uint64, error) {
 			"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
 }

internal/statesync/syncer_test.go

@@ -24,13 +24,15 @@ import (
 
 var ctx = context.Background()
 
+const testAppVersion = 9
+
 func TestSyncer_SyncAny(t *testing.T) {
 	state := sm.State{
 		ChainID: "chain",
 		Version: sm.Version{
 			Consensus: version.Consensus{
 				Block: version.BlockProtocol,
-				App:   0,
+				App:   testAppVersion,
 			},
 			Software: version.TMVersion,
 		},
@@ -178,7 +180,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("InfoSync", ctx, proxy.RequestInfo).Return(&abci.ResponseInfo{
-		AppVersion:       9,
+		AppVersion:       testAppVersion,
 		LastBlockHeight:  1,
 		LastBlockAppHash: []byte("app_hash"),
 	}, nil)
@@ -192,10 +194,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)
 
@@ -669,45 +668,47 @@ 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 {
 		response  *abci.ResponseInfo
-		err       error
 		expectErr error
 	}{
 		"verified": {&abci.ResponseInfo{
 			LastBlockHeight:  3,
 			LastBlockAppHash: []byte("app_hash"),
-			AppVersion:       9,
-		}, nil, nil},
+			AppVersion:       appVersion,
+		}, nil},
+		"invalid app version": {&abci.ResponseInfo{
+			LastBlockHeight:  3,
+			LastBlockAppHash: []byte("app_hash"),
+			AppVersion:       2,
+		}, appVersionMismatchErr},
 		"invalid height": {&abci.ResponseInfo{
 			LastBlockHeight:  5,
 			LastBlockAppHash: []byte("app_hash"),
-			AppVersion:       9,
-		}, nil, errVerifyFailed},
+			AppVersion:       appVersion,
+		}, errVerifyFailed},
 		"invalid hash": {&abci.ResponseInfo{
 			LastBlockHeight:  3,
 			LastBlockAppHash: []byte("xxx"),
-			AppVersion:       9,
-		}, nil, errVerifyFailed},
-		"error": {nil, boom, boom},
+			AppVersion:       appVersion,
+		}, errVerifyFailed},
+		"error": {nil, boom},
 	}
 	for name, tc := range testcases {
 		tc := tc
 		t.Run(name, func(t *testing.T) {
 			rts := setup(t, nil, nil, nil, 2)
 
-			rts.connQuery.On("InfoSync", ctx, proxy.RequestInfo).Return(tc.response, tc.err)
-			version, err := rts.syncer.verifyApp(s)
-			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)
+			rts.connQuery.On("InfoSync", mock.Anything, proxy.RequestInfo).Return(tc.response, tc.expectErr)
+			err := rts.syncer.verifyApp(ctx, s, appVersion)
+			if tc.expectErr != nil {
+				require.ErrorIs(t, err, tc.expectErr)
+			} else {
+				require.NoError(t, err)
 			}
 		})
 	}

rpc/client/mocks/client.go

@@ -600,13 +600,13 @@ func (_m *Client) RemoveTx(_a0 context.Context, _a1 types.TxKey) error {
 	return r0
 }
 
-// Start provides a mock function with given fields: _a0
-func (_m *Client) Start(_a0 context.Context) error {
-	ret := _m.Called(_a0)
+// Start provides a mock function with given fields:
+func (_m *Client) Start() error {
+	ret := _m.Called()
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(context.Context) error); ok {
-		r0 = rf(_a0)
+	if rf, ok := ret.Get(0).(func() error); ok {
+		r0 = rf()
 	} else {
 		r0 = ret.Error(0)
 	}