fix: always reset the state for Prepare and Process Proposal (backport …

…#15487) (#15503)

Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
Co-authored-by: Facundo Medica <facundomedica@gmail.com>
Co-authored-by: Marko <marbar3778@yahoo.com>

CHANGELOG.md

@@ -51,6 +51,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.
 * (cli) [#15123](https://github.com/cosmos/cosmos-sdk/pull/15123) Fix the CLI `offline` mode behavior to be really offline. The API of `clienttx.NewFactoryCLI` is updated to return an error. 
 
 ### Deprecated

baseapp/abci.go

@@ -59,14 +59,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 := tmproto.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.
@@ -258,8 +250,12 @@ func (app *BaseApp) PrepareProposal(req abci.RequestPrepareProposal) (resp abci.
 		panic("PrepareProposal method not set")
 	}
 
-	// Tendermint must never call PrepareProposal with a height of 0.
-	// Ref: https://github.com/tendermint/tendermint/blob/059798a4f5b0c9f52aa8655fa619054a0154088c/spec/core/state.md?plain=1#L37-L38
+	// always reset state given that PrepareProposal can timeout and be called again
+	emptyHeader := tmproto.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 {
 		panic("PrepareProposal called with invalid height")
 	}
@@ -311,6 +307,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 := tmproto.Header{ChainID: app.chainID}
+	app.setState(runTxProcessProposal, emptyHeader)
+
 	app.processProposalState.ctx = app.getContextForProposal(app.processProposalState.ctx, req.Height).
 		WithVoteInfos(app.voteInfos).
 		WithBlockHeight(req.Height).
@@ -450,12 +456,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 := tmproto.Header{ChainID: app.chainID}
-	app.setState(runTxPrepareProposal, emptyHeader)
-	app.setState(runTxProcessProposal, emptyHeader)
-
 	// empty/reset the deliver state
 	app.deliverState = nil
 

baseapp/abci_test.go

@@ -1365,7 +1365,8 @@ func TestABCI_Proposal_HappyPath(t *testing.T) {
 		tx2Bytes,
 	}
 	reqProcessProposal := abci.RequestProcessProposal{
-		Txs: reqProposalTxBytes[:],
+		Txs:    reqProposalTxBytes[:],
+		Height: reqPrepareProposal.Height,
 	}
 
 	resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal)
@@ -1416,7 +1417,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)
@@ -1551,7 +1553,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: &tmproto.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: tmproto.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()
 
 	rms, ok := app.cms.(*rootmulti.Store)