From 3a7a2645f05f0aecdf96ebda14a7c62f6d4ddc55 Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Tue, 21 Mar 2023 16:42:19 -0300 Subject: [PATCH] fix: always reset the state for Prepare and Process Proposal (#15487) Co-authored-by: Marko --- CHANGELOG.md | 1 + baseapp/abci.go | 28 +++++++++--------- baseapp/abci_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++-- baseapp/baseapp.go | 4 --- 4 files changed, 81 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d734590702..c439ec69a2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/baseapp/abci.go b/baseapp/abci.go index f5e8aa04e3..dc89c364c1 100644 --- a/baseapp/abci.go +++ b/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 diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 8758b18175..b4e4abca02 100644 --- a/baseapp/abci_test.go +++ b/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}, + }) +} diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 55ed3c3ac4..5cff5a206b 100644 --- a/baseapp/baseapp.go +++ b/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 {