fix: always reset the state for Prepare and Process Proposal

CHANGELOG.md

@@ -160,6 +160,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (baseapp) [#15487](https://github.com/cosmos/cosmos-sdk/pull/15487) Reset state before calling PrepareProposal and ProcessProposal.
 * (x/auth) [#15059](https://github.com/cosmos/cosmos-sdk/pull/15059) `ante.CountSubKeys` returns 0 when passing a nil `Pubkey`.
 * (x/capability) [#15030](https://github.com/cosmos/cosmos-sdk/pull/15030) Prevent `x/capability` from consuming `GasMeter` gas during `InitMemStore`
 * (types/coin) [#14739](https://github.com/cosmos/cosmos-sdk/pull/14739) Deprecate the method `Coin.IsEqual` in favour of  `Coin.Equal`. The difference between the two methods is that the first one results in a panic when denoms are not equal. This panic lead to unexpected behavior

baseapp/abci.go

@@ -62,14 +62,6 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
 	app.setState(runTxModeDeliver, initHeader)
 	app.setState(runTxModeCheck, initHeader)
 
-	// Use an empty header for prepare and process proposal states. The header
-	// will be overwritten for the first block (see getContextForProposal()) and
-	// cleaned up on every Commit(). Only the ChainID is needed so it's set in
-	// the context.
-	emptyHeader := cmtproto.Header{ChainID: req.ChainId}
-	app.setState(runTxPrepareProposal, emptyHeader)
-	app.setState(runTxProcessProposal, emptyHeader)
-
 	// Store the consensus params in the BaseApp's paramstore. Note, this must be
 	// done after the deliver state and context have been set as it's persisted
 	// to state.
@@ -277,6 +269,10 @@ func (app *BaseApp) PrepareProposal(req abci.RequestPrepareProposal) (resp abci.
 		panic("PrepareProposal method not set")
 	}
 
+	// always reset state given that PrepareProposal can timeout and be called again
+	emptyHeader := cmtproto.Header{ChainID: app.chainID}
+	app.setState(runTxPrepareProposal, emptyHeader)
+
 	// CometBFT must never call PrepareProposal with a height of 0.
 	// Ref: https://github.com/cometbft/cometbft/blob/059798a4f5b0c9f52aa8655fa619054a0154088c/spec/core/state.md?plain=1#L37-L38
 	if req.Height < 1 {
@@ -330,6 +326,16 @@ func (app *BaseApp) ProcessProposal(req abci.RequestProcessProposal) (resp abci.
 		panic("app.ProcessProposal is not set")
 	}
 
+	// CometBFT must never call ProcessProposal with a height of 0.
+	// Ref: https://github.com/cometbft/cometbft/blob/059798a4f5b0c9f52aa8655fa619054a0154088c/spec/core/state.md?plain=1#L37-L38
+	if req.Height < 1 {
+		panic("ProcessProposal called with invalid height")
+	}
+
+	// always reset state given that ProcessProposal can timeout and be called again
+	emptyHeader := cmtproto.Header{ChainID: app.chainID}
+	app.setState(runTxProcessProposal, emptyHeader)
+
 	app.processProposalState.ctx = app.getContextForProposal(app.processProposalState.ctx, req.Height).
 		WithVoteInfos(app.voteInfos).
 		WithBlockHeight(req.Height).
@@ -479,12 +485,6 @@ func (app *BaseApp) Commit() abci.ResponseCommit {
 	// Commit. Use the header from this latest block.
 	app.setState(runTxModeCheck, header)
 
-	// Reset state to the latest committed but with an empty header to avoid
-	// leaking the header from the last block.
-	emptyHeader := cmtproto.Header{ChainID: app.chainID}
-	app.setState(runTxPrepareProposal, emptyHeader)
-	app.setState(runTxProcessProposal, emptyHeader)
-
 	// empty/reset the deliver state
 	app.deliverState = nil
 

baseapp/abci_test.go

@@ -1367,7 +1367,8 @@ func TestABCI_Proposal_HappyPath(t *testing.T) {
 		tx2Bytes,
 	}
 	reqProcessProposal := abci.RequestProcessProposal{
-		Txs: reqProposalTxBytes[:],
+		Txs:    reqProposalTxBytes[:],
+		Height: reqPrepareProposal.Height,
 	}
 
 	resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal)
@@ -1418,7 +1419,8 @@ func TestABCI_Proposal_Read_State_PrepareProposal(t *testing.T) {
 
 	reqProposalTxBytes := [][]byte{}
 	reqProcessProposal := abci.RequestProcessProposal{
-		Txs: reqProposalTxBytes,
+		Txs:    reqProposalTxBytes,
+		Height: reqPrepareProposal.Height,
 	}
 
 	resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal)
@@ -1553,7 +1555,68 @@ func TestABCI_ProcessProposal_PanicRecovery(t *testing.T) {
 	})
 
 	require.NotPanics(t, func() {
-		res := suite.baseApp.ProcessProposal(abci.RequestProcessProposal{})
+		res := suite.baseApp.ProcessProposal(abci.RequestProcessProposal{Height: 1})
 		require.Equal(t, res.Status, abci.ResponseProcessProposal_REJECT)
 	})
 }
+
+// TestABCI_Proposal_Reset_State ensures that state is reset between runs of
+// PrepareProposal and ProcessProposal in case they are called multiple times.
+// This is only valid for heights > 1, given that on height 1 we always set the
+// state to be deliverState.
+func TestABCI_Proposal_Reset_State_Between_Calls(t *testing.T) {
+	someKey := []byte("some-key")
+
+	prepareOpt := func(bapp *baseapp.BaseApp) {
+		bapp.SetPrepareProposal(func(ctx sdk.Context, req abci.RequestPrepareProposal) abci.ResponsePrepareProposal {
+			// This key should not exist given that we reset the state on every call.
+			require.False(t, ctx.KVStore(capKey1).Has(someKey))
+			ctx.KVStore(capKey1).Set(someKey, someKey)
+			return abci.ResponsePrepareProposal{Txs: req.Txs}
+		})
+	}
+
+	processOpt := func(bapp *baseapp.BaseApp) {
+		bapp.SetProcessProposal(func(ctx sdk.Context, req abci.RequestProcessProposal) abci.ResponseProcessProposal {
+			// This key should not exist given that we reset the state on every call.
+			require.False(t, ctx.KVStore(capKey1).Has(someKey))
+			ctx.KVStore(capKey1).Set(someKey, someKey)
+			return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}
+		})
+	}
+
+	suite := NewBaseAppSuite(t, prepareOpt, processOpt)
+
+	suite.baseApp.InitChain(abci.RequestInitChain{
+		ConsensusParams: &cmtproto.ConsensusParams{},
+	})
+
+	reqPrepareProposal := abci.RequestPrepareProposal{
+		MaxTxBytes: 1000,
+		Height:     2, // this value can't be 0
+	}
+
+	// Let's pretend something happened and PrepareProposal gets called many
+	// times, this must be safe to do.
+	for i := 0; i < 5; i++ {
+		resPrepareProposal := suite.baseApp.PrepareProposal(reqPrepareProposal)
+		require.Equal(t, 0, len(resPrepareProposal.Txs))
+	}
+
+	reqProposalTxBytes := [][]byte{}
+	reqProcessProposal := abci.RequestProcessProposal{
+		Txs:    reqProposalTxBytes,
+		Height: 2,
+	}
+
+	// Let's pretend something happened and ProcessProposal gets called many
+	// times, this must be safe to do.
+	for i := 0; i < 5; i++ {
+		resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal)
+		require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status)
+	}
+
+	suite.baseApp.BeginBlock(abci.RequestBeginBlock{
+		Header: cmtproto.Header{Height: suite.baseApp.LastBlockHeight() + 1},
+	})
+}

baseapp/baseapp.go

@@ -356,10 +356,6 @@ func (app *BaseApp) Init() error {
 
 	// needed for the export command which inits from store but never calls initchain
 	app.setState(runTxModeCheck, emptyHeader)
-
-	// needed for ABCI Replay Blocks mode which calls Prepare/Process proposal (InitChain is not called)
-	app.setState(runTxPrepareProposal, emptyHeader)
-	app.setState(runTxProcessProposal, emptyHeader)
 	app.Seal()
 
 	if app.cms == nil {