Always reset state between proposals

CHANGELOG.md

@@ -37,12 +37,18 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+
+## Pending
+
+### Bugfixes
+* (baseapp) [#285](https://github.com/sei-protocol/sei-cosmos/pull/285) Reset state before calling PrepareProposal and ProcessProposal. Copy of [#15487](https://github.com/cosmos/cosmos-sdk/pull/15487)
+
 ## v0.45.9 - 2022-10-14
 
 ATTENTION:
 
-This is a security release for the 
-[Dragonberry security advisory](https://forum.cosmos.network/t/ibc-security-advisory-dragonberry/7702). 
+This is a security release for the
+[Dragonberry security advisory](https://forum.cosmos.network/t/ibc-security-advisory-dragonberry/7702).
 
 All users should upgrade immediately.
 
@@ -66,7 +72,7 @@ replace (
 * [#13323](https://github.com/cosmos/cosmos-sdk/pull/13323) Ensure `withdraw_rewards` rewards are emitted from all actions that result in rewards being withdrawn.
 * [#13321](https://github.com/cosmos/cosmos-sdk/pull/13321) Add flag to disable fast node migration and usage.
 * (store) [#13326](https://github.com/cosmos/cosmos-sdk/pull/13326) Implementation of ADR-038 file StreamingService, backport #8664.
-* (store) [#13540](https://github.com/cosmos/cosmos-sdk/pull/13540) Default fastnode migration to false to prevent suprises. Operators must enable it, unless they have it enabled already. 
+* (store) [#13540](https://github.com/cosmos/cosmos-sdk/pull/13540) Default fastnode migration to false to prevent suprises. Operators must enable it, unless they have it enabled already.
 
 ### API Breaking Changes
 

baseapp/abci.go

@@ -932,12 +932,14 @@ func (app *BaseApp) PrepareProposal(ctx context.Context, req *abci.RequestPrepar
 			},
 		},
 	}
-	if app.prepareProposalState == nil {
-		app.setPrepareProposalState(header)
-	} else {
-		// In the first block, app.prepareProposalState.ctx will already be initialized
+
+	if req.Height == 1 {
+		// In the first block, app.processProposalState.ctx will already be initialized
 		// by InitChain. Context is now updated with Header information.
 		app.setPrepareProposalHeader(header)
+	} else {
+		// always reset state given that ProcessProposal can timeout and be called again
+		app.setPrepareProposalState(header)
 	}
 
 	app.preparePrepareProposalState()
@@ -980,12 +982,14 @@ func (app *BaseApp) ProcessProposal(ctx context.Context, req *abci.RequestProces
 			},
 		},
 	}
-	if app.processProposalState == nil {
-		app.setProcessProposalState(header)
-	} else {
+
+	if req.Height == 1 {
 		// In the first block, app.processProposalState.ctx will already be initialized
 		// by InitChain. Context is now updated with Header information.
 		app.setProcessProposalHeader(header)
+	} else {
+		// always reset state given that ProcessProposal can timeout and be called again
+		app.setProcessProposalState(header)
 	}
 
 	// add block gas meter
@@ -996,8 +1000,6 @@ func (app *BaseApp) ProcessProposal(ctx context.Context, req *abci.RequestProces
 		gasMeter = sdk.NewInfiniteGasMeter()
 	}
 
-	// NOTE: header hash is not set in NewContext, so we manually set it here
-
 	app.prepareProcessProposalState(gasMeter, req.Hash)
 
 	if app.processProposalHandler != nil {

baseapp/abci_test.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/testutil"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/utils"
 )
 
 func TestGetBlockRentionHeight(t *testing.T) {
@@ -200,3 +201,101 @@ func (ps *paramStore) Get(_ sdk.Context, key []byte, ptr interface{}) {
 		panic(err)
 	}
 }
+
+// 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) {
+		bapp.SetPrepareProposalHandler(func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) {
+			// 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{
+				TxRecords: utils.Map(req.Txs, func(tx []byte) *abci.TxRecord {
+					return &abci.TxRecord{Action: abci.TxRecord_UNMODIFIED, Tx: tx}
+				}),
+			}, nil
+		})
+	}
+
+	processOpt := func(bapp *BaseApp) {
+		bapp.SetProcessProposalHandler(func(ctx sdk.Context, req *abci.RequestProcessProposal) (*abci.ResponseProcessProposal, error) {
+			// 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}, nil
+		})
+	}
+
+	app := setupBaseApp(
+		t,
+		processOpt,
+		prepareOpt,
+	)
+
+	_, err := app.InitChain(context.Background(), &abci.RequestInitChain{
+		Validators: []abci.ValidatorUpdate{},
+		// ConsensusParams: simapp.DefaultConsensusParams,
+		// AppStateBytes:   stateBytes,
+	})
+	ctx := app.NewContext(false, tmproto.Header{})
+
+	require.Nil(t, err)
+
+	// Genesis at height 1
+	reqPrepareProposal := abci.RequestPrepareProposal{
+		MaxTxBytes: 1000,
+		Height:     1, // this value can't be 0
+	}
+
+	resPrepareProposal, _ := app.PrepareProposal(ctx.Context(), &reqPrepareProposal)
+	require.Equal(t, 0, len(resPrepareProposal.TxRecords))
+
+	reqProposalTxBytes := [][]byte{}
+	reqProcessProposal := abci.RequestProcessProposal{
+		Txs:    reqProposalTxBytes,
+		Height: 1,
+	}
+
+	resProcessProposal, _ := app.ProcessProposal(ctx.Context(), &reqProcessProposal)
+	require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status)
+
+	app.BeginBlock(ctx, abci.RequestBeginBlock{
+		Header: tmproto.Header{Height: app.LastBlockHeight() + 1},
+	})
+
+	// Post Genesis
+
+	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, _ := app.PrepareProposal(ctx.Context(), &reqPrepareProposal)
+		require.Equal(t, 0, len(resPrepareProposal.TxRecords))
+	}
+
+	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, _ := app.ProcessProposal(ctx.Context(), &reqProcessProposal)
+		require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status)
+	}
+
+	app.BeginBlock(ctx, abci.RequestBeginBlock{
+		Header: tmproto.Header{Height: app.LastBlockHeight() + 1},
+	})
+}