fix: invalid txs_results returned for legacy ABCI responses (backport

#3031) (#3434)

close: #3002 

This PR fixes the issue reported above.

This is not a storage issue in particular, the results are still in
storage after an upgrade, but not returned properly by the RPC endpoint.
The fix is to make the `/block_results` endpoint in `v0.38` to return
properly a legacy ABCI response created with `v0.37`.

Once this fix is merged on `v0.38` and a patch release is cut, any node
on `v0.38` (e.g. an archive node) that applies the patch release, should
have the results returned properly by the RPC `/block_results` endpoint.

---

#### PR checklist

- [X] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] ~~Updated relevant documentation (`docs/` or `spec/`) and code
comments~~
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3031 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Andy Nogueira <[email protected]>

.changelog/unreleased/bug-fixes/3002-invalid-txs-results.md

@@ -0,0 +1,3 @@
+- `[rpc]` Fix an issue where a legacy ABCI response, created on `v0.37` or before, is not returned properly in `v0.38` and up
+on the `/block_results` RPC endpoint.
+  ([\#3002](https://github.com/cometbft/cometbft/issues/3002))

rpc/core/blocks.go

@@ -195,6 +195,7 @@ func (env *Environment) BlockResults(_ *rpctypes.Context, heightPtr *int64) (*ct
 		FinalizeBlockEvents:   results.Events,
 		ValidatorUpdates:      results.ValidatorUpdates,
 		ConsensusParamUpdates: results.ConsensusParamUpdates,
+		AppHash:               results.AppHash,
 	}, nil
 }
 

rpc/core/blocks_test.go

@@ -73,6 +73,7 @@ func TestBlockResults(t *testing.T) {
 			{Code: 0, Data: []byte{0x02}, Log: "ok"},
 			{Code: 1, Log: "not ok"},
 		},
+		AppHash: make([]byte, 1),
 	}
 
 	env := &Environment{}
@@ -100,6 +101,7 @@ func TestBlockResults(t *testing.T) {
 			FinalizeBlockEvents:   results.Events,
 			ValidatorUpdates:      results.ValidatorUpdates,
 			ConsensusParamUpdates: results.ConsensusParamUpdates,
+			AppHash:               make([]byte, 1),
 		}},
 	}
 

state/errors.go

@@ -5,6 +5,12 @@ import (
 	"fmt"
 )
 
+var (
+	ErrFinalizeBlockResponsesNotPersisted = errors.New("node is not persisting finalize block responses")
+	ErrPrunerCannotLowerRetainHeight      = errors.New("cannot set a height lower than previously requested - heights might have already been pruned")
+	ErrInvalidRetainHeight                = errors.New("retain height cannot be less or equal than 0")
+)
+
 type (
 	ErrInvalidBlock error
 	ErrProxyAppConn error
@@ -74,6 +80,15 @@ type (
 	ErrCannotLoadState struct {
 		Err error
 	}
+
+	ErrABCIResponseResponseUnmarshalForHeight struct {
+		Height int64
+	}
+
+	ErrABCIResponseCorruptedOrSpecChangeForHeight struct {
+		Err    error
+		Height int64
+	}
 )
 
 func (e ErrUnknownBlock) Error() string {
@@ -126,6 +141,18 @@ func (e ErrNoABCIResponsesForHeight) Error() string {
 	return fmt.Sprintf("could not find results for height #%d", e.Height)
 }
 
+func (e ErrABCIResponseResponseUnmarshalForHeight) Error() string {
+	return fmt.Sprintf("could not decode results for height %d", e.Height)
+}
+
+func (e ErrABCIResponseCorruptedOrSpecChangeForHeight) Error() string {
+	return fmt.Sprintf("failed to unmarshall FinalizeBlockResponse (also tried as legacy ABCI response) for height %d", e.Height)
+}
+
+func (e ErrABCIResponseCorruptedOrSpecChangeForHeight) Unwrap() error {
+	return e.Err
+}
+
 func (e ErrPrunerFailedToGetRetainHeight) Error() string {
 	return fmt.Sprintf("pruner failed to get existing %s retain height: %s", e.Which, e.Err.Error())
 }
@@ -158,12 +185,6 @@ func (e ErrFailedToPruneStates) Unwrap() error {
 	return e.Err
 }
 
-var (
-	ErrFinalizeBlockResponsesNotPersisted = errors.New("node is not persisting finalize block responses")
-	ErrPrunerCannotLowerRetainHeight      = errors.New("cannot set a height lower than previously requested - heights might have already been pruned")
-	ErrInvalidRetainHeight                = errors.New("retain height cannot be less or equal than 0")
-)
-
 func (e ErrCannotLoadState) Error() string {
 	return fmt.Sprintf("cannot load state: %v", e.Err)
 }

state/state_test.go

@@ -23,6 +23,13 @@ import (
 
 // setupTestCase does setup common to all test cases.
 func setupTestCase(t *testing.T) (func(t *testing.T), dbm.DB, sm.State) {
+	t.Helper()
+	tearDown, stateDB, state, _ := setupTestCaseWithStore(t)
+	return tearDown, stateDB, state
+}
+
+// setupTestCase does setup common to all test cases.
+func setupTestCaseWithStore(t *testing.T) (func(t *testing.T), dbm.DB, sm.State, sm.Store) {
 	t.Helper()
 	config := test.ResetTestRoot("state_")
 	dbType := dbm.BackendType(config.DBBackend)
@@ -41,7 +48,7 @@ func setupTestCase(t *testing.T) (func(t *testing.T), dbm.DB, sm.State) {
 		os.RemoveAll(config.RootDir)
 	}
 
-	return tearDown, stateDB, state
+	return tearDown, stateDB, state, stateStore
 }
 
 // TestStateCopy tests the correct copying behavior of State.
@@ -121,6 +128,8 @@ func TestFinalizeBlockResponsesSaveLoad1(t *testing.T) {
 		abci.NewValidatorUpdate(ed25519.GenPrivKey().PubKey(), 10),
 	}
 
+	abciResponses.AppHash = make([]byte, 1)
+
 	err := stateStore.SaveFinalizeBlockResponse(block.Height, abciResponses)
 	require.NoError(t, err)
 	loadedABCIResponses, err := stateStore.LoadFinalizeBlockResponse(block.Height)
@@ -524,7 +533,7 @@ func TestProposerPriorityDoesNotGetResetToZero(t *testing.T) {
 	_, updatedVal2 := updatedState3.NextValidators.GetByAddress(val2PubKey.Address())
 
 	// 2. Scale
-	// old prios: v1(10):-38, v2(1):39
+	// old prios: cryptov1(10):-38, v2(1):39
 	wantVal1Prio = prevVal1.ProposerPriority
 	wantVal2Prio = prevVal2.ProposerPriority
 	// scale to diffMax = 22 = 2 * tvp, diff=39-(-38)=77
@@ -533,14 +542,14 @@ func TestProposerPriorityDoesNotGetResetToZero(t *testing.T) {
 	dist := wantVal2Prio - wantVal1Prio
 	// ratio := (dist + 2*totalPower - 1) / 2*totalPower = 98/22 = 4
 	ratio := (dist + 2*totalPower - 1) / (2 * totalPower)
-	// v1(10):-38/4, v2(1):39/4
+	// cryptov1(10):-38/4, v2(1):39/4
 	wantVal1Prio /= ratio // -9
 	wantVal2Prio /= ratio // 9
 
 	// 3. Center - noop
 	// 4. IncrementProposerPriority() ->
-	// v1(10):-9+10, v2(1):9+1 -> v2 proposer so subsract tvp(11)
-	// v1(10):1, v2(1):-1
+	// cryptov1(10):-9+10, v2(1):9+1 -> v2 proposer so subsract tvp(11)
+	// cryptov1(10):1, v2(1):-1
 	wantVal2Prio += updatedVal2.VotingPower // 10 -> prop
 	wantVal1Prio += updatedVal1.VotingPower // 1
 	wantVal2Prio -= totalPower              // -1

state/store.go

@@ -122,7 +122,7 @@ type Store interface {
 	LoadValidators(height int64) (*types.ValidatorSet, error)
 	// LoadFinalizeBlockResponse loads the abciResponse for a given height
 	LoadFinalizeBlockResponse(height int64) (*abci.FinalizeBlockResponse, error)
-	// LoadLastABCIResponse loads the last abciResponse for a given height
+	// LoadLastFinalizeBlockResponse loads the last abciResponse for a given height
 	LoadLastFinalizeBlockResponse(height int64) (*abci.FinalizeBlockResponse, error)
 	// LoadConsensusParams loads the consensus params for a given height
 	LoadConsensusParams(height int64) (types.ConsensusParams, error)
@@ -652,14 +652,22 @@ func (store dbStore) LoadFinalizeBlockResponse(height int64) (*abci.FinalizeBloc
 
 	resp := new(abci.FinalizeBlockResponse)
 	err = resp.Unmarshal(buf)
-	if err != nil {
+	// Check for an error or if the resp.AppHash is nil if so
+	// this means the unmarshalling should be a LegacyABCIResponses
+	// Depending on a source message content (serialized as ABCIResponses)
+	// there are instances where it can be deserialized as a FinalizeBlockResponse
+	// without causing an error. But the values will not be deserialized properly
+	// and, it will contain zero values, and one of them is an AppHash == nil
+	// This can be verified in the /state/compatibility_test.go file
+	if err != nil || resp.AppHash == nil {
 		// The data might be of the legacy ABCI response type, so
 		// we try to unmarshal that
 		legacyResp := new(cmtstate.LegacyABCIResponses)
-		rerr := legacyResp.Unmarshal(buf)
-		if rerr != nil {
-			cmtos.Exit(fmt.Sprintf(`LoadFinalizeBlockResponse: Data has been corrupted or its spec has
-					changed: %v\n`, err))
+		if err := legacyResp.Unmarshal(buf); err != nil {
+			// only return an error, this method is only invoked through the `/block_results` not for state logic and
+			// some tests, so no need to exit cometbft if there's an error, just return it.
+			store.Logger.Error("failed in LoadFinalizeBlockResponse", "error", ErrABCIResponseCorruptedOrSpecChangeForHeight{Height: height, Err: err})
+			return nil, ErrABCIResponseCorruptedOrSpecChangeForHeight{Height: height, Err: err}
 		}
 		// The state store contains the old format. Migrate to
 		// the new FinalizeBlockResponse format. Note that the
@@ -669,10 +677,11 @@ func (store dbStore) LoadFinalizeBlockResponse(height int64) (*abci.FinalizeBloc
 
 	// TODO: ensure that buf is completely read.
 
+	// Otherwise return the FinalizeBlockResponse
 	return resp, nil
 }
 
-// LoadLastFinalizeBlockResponses loads the FinalizeBlockResponses from the most recent height.
+// LoadLastFinalizeBlockResponse loads the FinalizeBlockResponses from the most recent height.
 // The height parameter is used to ensure that the response corresponds to the latest height.
 // If not, an error is returned.
 //
@@ -1077,14 +1086,52 @@ func min(a int64, b int64) int64 {
 // responseFinalizeBlockFromLegacy is a convenience function that takes the old abci responses and morphs
 // it to the finalize block response. Note that the app hash is missing.
 func responseFinalizeBlockFromLegacy(legacyResp *cmtstate.LegacyABCIResponses) *abci.FinalizeBlockResponse {
-	return &abci.FinalizeBlockResponse{
-		TxResults:             legacyResp.DeliverTxs,
-		ValidatorUpdates:      legacyResp.EndBlock.ValidatorUpdates,
-		ConsensusParamUpdates: legacyResp.EndBlock.ConsensusParamUpdates,
-		Events:                append(legacyResp.BeginBlock.Events, legacyResp.EndBlock.Events...),
-		// NOTE: AppHash is missing in the response but will
-		// be caught and filled in consensus/replay.go
+	var response abci.FinalizeBlockResponse
+	events := make([]abci.Event, 0)
+
+	if legacyResp.DeliverTxs != nil {
+		response.TxResults = legacyResp.DeliverTxs
+	}
+
+	// Check for begin block and end block and only append events or assign values if they are not nil
+	if legacyResp.BeginBlock != nil {
+		if legacyResp.BeginBlock.Events != nil {
+			// Add BeginBlock attribute to BeginBlock events
+			for idx := range legacyResp.BeginBlock.Events {
+				legacyResp.BeginBlock.Events[idx].Attributes = append(legacyResp.BeginBlock.Events[idx].Attributes, abci.EventAttribute{
+					Key:   "mode",
+					Value: "BeginBlock",
+					Index: false,
+				})
+			}
+			events = append(events, legacyResp.BeginBlock.Events...)
+		}
+	}
+	if legacyResp.EndBlock != nil {
+		if legacyResp.EndBlock.ValidatorUpdates != nil {
+			response.ValidatorUpdates = legacyResp.EndBlock.ValidatorUpdates
+		}
+		if legacyResp.EndBlock.ConsensusParamUpdates != nil {
+			response.ConsensusParamUpdates = legacyResp.EndBlock.ConsensusParamUpdates
+		}
+		if legacyResp.EndBlock.Events != nil {
+			// Add EndBlock attribute to BeginBlock events
+			for idx := range legacyResp.EndBlock.Events {
+				legacyResp.EndBlock.Events[idx].Attributes = append(legacyResp.EndBlock.Events[idx].Attributes, abci.EventAttribute{
+					Key:   "mode",
+					Value: "EndBlock",
+					Index: false,
+				})
+			}
+			events = append(events, legacyResp.EndBlock.Events...)
+		}
 	}
+
+	response.Events = events
+
+	// NOTE: AppHash is missing in the response but will
+	// be caught and filled in consensus/replay.go
+	return &response
 }
 
 // ----- Util.

state/store_test.go

@@ -180,6 +180,7 @@ func TestPruneStates(t *testing.T) {
 						{Data: []byte{2}},
 						{Data: []byte{3}},
 					},
+					AppHash: make([]byte, 1),
 				})
 				require.NoError(t, err)
 			}
@@ -375,6 +376,7 @@ func TestABCIResPruningStandalone(t *testing.T) {
 		TxResults: []*abci.ExecTxResult{
 			{Code: 32, Data: []byte("Hello"), Log: "Huh?"},
 		},
+		AppHash: make([]byte, 1),
 	}
 	_, bs, txIndexer, blockIndexer, callbackF, stateStore := makeStateAndBlockStoreAndIndexers()
 	defer callbackF()
@@ -468,6 +470,7 @@ func TestFinalizeBlockResponsePruning(t *testing.T) {
 			TxResults: []*abci.ExecTxResult{
 				{Code: 32, Data: []byte("Hello"), Log: "Huh?"},
 			},
+			AppHash: make([]byte, 1),
 		}
 		state, bs, txIndexer, blockIndexer, callbackF, stateStore := makeStateAndBlockStoreAndIndexers()
 		defer callbackF()
@@ -526,6 +529,7 @@ func TestLastFinalizeBlockResponses(t *testing.T) {
 			TxResults: []*abci.ExecTxResult{
 				{Code: 32, Data: []byte("Hello"), Log: "Huh?"},
 			},
+			AppHash: make([]byte, 1),
 		}
 
 		stateDB = dbm.NewMemDB()
@@ -622,7 +626,7 @@ func TestFinalizeBlockRecoveryUsingLegacyABCIResponses(t *testing.T) {
 	resp, err := stateStore.LoadLastFinalizeBlockResponse(height)
 	require.NoError(t, err)
 	require.Equal(t, resp.ConsensusParamUpdates, &cp)
-	require.Equal(t, resp.Events, legacyResp.LegacyAbciResponses.BeginBlock.Events)
+	require.Equal(t, len(resp.Events), len(legacyResp.LegacyAbciResponses.BeginBlock.Events))
 	require.Equal(t, resp.TxResults[0], legacyResp.LegacyAbciResponses.DeliverTxs[0])
 }