From 6a0358607411cd83d22c6f1deea69f399f803007 Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Mon, 13 Mar 2023 14:15:39 -0300 Subject: [PATCH] fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs (#15303) ## Description ### Issue Some values (like chain ID) were being leaked from the previous block/initialization into PrepareProposal and ProcessProposal, these values are only available if: 1. The node has never been stopped since the genesis block (as these values are set on `InitChain`) 2. The node has already commited a block (as the previous header was being used for the new state of prepare and process proposal). So if a node is restarted, during the first prepare and process proposal these values won't be populated, and that will cause issues if they are being used. ### Solution Remove any previous header information from a previous block in the prepare and process proposal contexts, making things consistent at every height. - Added ChainID to baseapp - Use an empty header in Commit() with only the chain id set - Fix context for prepare and process proposal Closes: #15269 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- baseapp/abci.go | 59 ++++++++++++++++++++++++------------- baseapp/abci_test.go | 9 ++++-- baseapp/baseapp.go | 4 ++- baseapp/options.go | 5 ++++ server/rollback.go | 3 +- server/util.go | 16 +++++++++- simapp/sim_test.go | 12 ++++---- simapp/test_helpers.go | 1 + tests/e2e/params/suite.go | 1 + testutil/network/network.go | 5 ++++ 10 files changed, 84 insertions(+), 31 deletions(-) diff --git a/baseapp/abci.go b/baseapp/abci.go index 4ec863cc0eb0..b0de6ce94244 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -37,6 +37,10 @@ const ( // InitChain implements the ABCI interface. It runs the initialization logic // directly on the CommitMultiStore. func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitChain) { + if req.ChainId != app.chainID { + panic(fmt.Sprintf("invalid chain-id on InitChain; expected: %s, got: %s", app.chainID, req.ChainId)) + } + // On a new chain, we consider the init chain block height as 0, even though // req.InitialHeight is 1 by default. initHeader := cmtproto.Header{ChainID: req.ChainId, Time: req.Time} @@ -57,8 +61,14 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC // initialize states with a correct header app.setState(runTxModeDeliver, initHeader) app.setState(runTxModeCheck, initHeader) - app.setState(runTxPrepareProposal, initHeader) - app.setState(runTxProcessProposal, 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 @@ -154,6 +164,10 @@ func (app *BaseApp) FilterPeerByID(info string) abci.ResponseQuery { // BeginBlock implements the ABCI application interface. func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeginBlock) { + if req.Header.ChainID != app.chainID { + panic(fmt.Sprintf("invalid chain-id on BeginBlock; expected: %s, got: %s", app.chainID, req.Header.ChainID)) + } + if app.cms.TracingEnabled() { app.cms.SetTracingContext(storetypes.TraceContext( map[string]interface{}{"blockHeight": req.Header.Height}, @@ -264,15 +278,15 @@ func (app *BaseApp) PrepareProposal(req abci.RequestPrepareProposal) (resp abci. panic("PrepareProposal called with invalid height") } - gasMeter := app.getBlockGasMeter(app.prepareProposalState.ctx) - ctx := app.getContextForProposal(app.prepareProposalState.ctx, req.Height) - - ctx = ctx.WithVoteInfos(app.voteInfos). + app.prepareProposalState.ctx = app.getContextForProposal(app.prepareProposalState.ctx, req.Height). + WithVoteInfos(app.voteInfos). WithBlockHeight(req.Height). WithBlockTime(req.Time). - WithProposer(req.ProposerAddress). - WithConsensusParams(app.GetConsensusParams(ctx)). - WithBlockGasMeter(gasMeter) + WithProposer(req.ProposerAddress) + + app.prepareProposalState.ctx = app.prepareProposalState.ctx. + WithConsensusParams(app.GetConsensusParams(app.prepareProposalState.ctx)). + WithBlockGasMeter(app.getBlockGasMeter(app.prepareProposalState.ctx)) defer func() { if err := recover(); err != nil { @@ -287,7 +301,7 @@ func (app *BaseApp) PrepareProposal(req abci.RequestPrepareProposal) (resp abci. } }() - resp = app.prepareProposal(ctx, req) + resp = app.prepareProposal(app.prepareProposalState.ctx, req) return resp } @@ -311,17 +325,16 @@ func (app *BaseApp) ProcessProposal(req abci.RequestProcessProposal) (resp abci. panic("app.ProcessProposal is not set") } - gasMeter := app.getBlockGasMeter(app.processProposalState.ctx) - ctx := app.getContextForProposal(app.processProposalState.ctx, req.Height) - - ctx = ctx. + app.processProposalState.ctx = app.getContextForProposal(app.processProposalState.ctx, req.Height). WithVoteInfos(app.voteInfos). WithBlockHeight(req.Height). WithBlockTime(req.Time). WithHeaderHash(req.Hash). - WithProposer(req.ProposerAddress). - WithConsensusParams(app.GetConsensusParams(ctx)). - WithBlockGasMeter(gasMeter) + WithProposer(req.ProposerAddress) + + app.processProposalState.ctx = app.processProposalState.ctx. + WithConsensusParams(app.GetConsensusParams(app.processProposalState.ctx)). + WithBlockGasMeter(app.getBlockGasMeter(app.processProposalState.ctx)) defer func() { if err := recover(); err != nil { @@ -336,7 +349,7 @@ func (app *BaseApp) ProcessProposal(req abci.RequestProcessProposal) (resp abci. } }() - resp = app.processProposal(ctx, req) + resp = app.processProposal(app.processProposalState.ctx, req) return resp } @@ -450,8 +463,12 @@ func (app *BaseApp) Commit() abci.ResponseCommit { // NOTE: This is safe because CometBFT holds a lock on the mempool for // Commit. Use the header from this latest block. app.setState(runTxModeCheck, header) - app.setState(runTxPrepareProposal, header) - app.setState(runTxProcessProposal, 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 @@ -969,6 +986,8 @@ func SplitABCIQueryPath(requestPath string) (path []string) { func (app *BaseApp) getContextForProposal(ctx sdk.Context, height int64) sdk.Context { if height == 1 { ctx, _ = app.deliverState.ctx.CacheContext() + // clear all context data set during InitChain to avoid inconsistent behavior + ctx = ctx.WithBlockHeader(cmtproto.Header{}) return ctx } return ctx diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 9cccf7a35ecf..8758b18175ac 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -48,7 +48,7 @@ func TestABCI_InitChain(t *testing.T) { name := t.Name() db := dbm.NewMemDB() logger := log.NewTestLogger(t) - app := baseapp.NewBaseApp(name, logger, db, nil) + app := baseapp.NewBaseApp(name, logger, db, nil, baseapp.SetChainID("test-chain-id")) capKey := storetypes.NewKVStoreKey("main") capKey2 := storetypes.NewKVStoreKey("key2") @@ -67,8 +67,13 @@ func TestABCI_InitChain(t *testing.T) { Data: key, } + // initChain is nil and chain ID is wrong - panics + require.Panics(t, func() { + app.InitChain(abci.RequestInitChain{ChainId: "wrong-chain-id"}) + }) + // initChain is nil - nothing happens - app.InitChain(abci.RequestInitChain{}) + app.InitChain(abci.RequestInitChain{ChainId: "test-chain-id"}) res := app.Query(query) require.Equal(t, 0, len(res.Value)) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 80fd8578d76d..44368c509936 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -144,6 +144,8 @@ type BaseApp struct { //nolint: maligned // abciListeners for hooking into the ABCI message processing of the BaseApp // and exposing the requests and responses to external consumers abciListeners []storetypes.ABCIListener + + chainID string } // NewBaseApp returns a reference to an initialized BaseApp. It accepts a @@ -351,7 +353,7 @@ func (app *BaseApp) Init() error { panic("cannot call initFromMainStore: baseapp already sealed") } - emptyHeader := cmtproto.Header{} + emptyHeader := cmtproto.Header{ChainID: app.chainID} // needed for the export command which inits from store but never calls initchain app.setState(runTxModeCheck, emptyHeader) diff --git a/baseapp/options.go b/baseapp/options.go index 39398826f60e..7ef64e96ea04 100644 --- a/baseapp/options.go +++ b/baseapp/options.go @@ -96,6 +96,11 @@ func SetMempool(mempool mempool.Mempool) func(*BaseApp) { return func(app *BaseApp) { app.SetMempool(mempool) } } +// SetChainID sets the chain ID in BaseApp. +func SetChainID(chainID string) func(*BaseApp) { + return func(app *BaseApp) { app.chainID = chainID } +} + func (app *BaseApp) SetName(name string) { if app.sealed { panic("SetName() on sealed BaseApp") diff --git a/server/rollback.go b/server/rollback.go index 9be4dfa32c37..ccd7e3213902 100644 --- a/server/rollback.go +++ b/server/rollback.go @@ -4,9 +4,10 @@ import ( "fmt" cmtcmd "github.com/cometbft/cometbft/cmd/cometbft/commands" + "github.com/spf13/cobra" + "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/server/types" - "github.com/spf13/cobra" ) // NewRollbackCmd creates a command to rollback CometBFT and multistore state by one height. diff --git a/server/util.go b/server/util.go index d697de1e9fe7..c655b283c322 100644 --- a/server/util.go +++ b/server/util.go @@ -37,6 +37,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/mempool" "github.com/cosmos/cosmos-sdk/version" + genutiltypes "github.com/cosmos/cosmos-sdk/x/genutil/types" ) // ServerContextKey defines the context key used to retrieve a server.Context from @@ -455,7 +456,19 @@ func DefaultBaseappOptions(appOpts types.AppOptions) []func(*baseapp.BaseApp) { panic(err) } - snapshotDir := filepath.Join(cast.ToString(appOpts.Get(flags.FlagHome)), "data", "snapshots") + homeDir := cast.ToString(appOpts.Get(flags.FlagHome)) + chainID := cast.ToString(appOpts.Get(flags.FlagChainID)) + if chainID == "" { + // fallback to genesis chain-id + appGenesis, err := genutiltypes.AppGenesisFromFile(filepath.Join(homeDir, "config", "genesis.json")) + if err != nil { + panic(err) + } + + chainID = appGenesis.ChainID + } + + snapshotDir := filepath.Join(homeDir, "data", "snapshots") if err = os.MkdirAll(snapshotDir, os.ModePerm); err != nil { panic(fmt.Errorf("failed to create snapshots directory: %w", err)) } @@ -492,5 +505,6 @@ func DefaultBaseappOptions(appOpts types.AppOptions) []func(*baseapp.BaseApp) { ), ), baseapp.SetIAVLLazyLoading(cast.ToBool(appOpts.Get(FlagIAVLLazyLoading))), + baseapp.SetChainID(chainID), } } diff --git a/simapp/sim_test.go b/simapp/sim_test.go index 9411b2c0192a..539bed0c05af 100644 --- a/simapp/sim_test.go +++ b/simapp/sim_test.go @@ -82,7 +82,7 @@ func TestFullAppSimulation(t *testing.T) { appOptions[flags.FlagHome] = DefaultNodeHome appOptions[server.FlagInvCheckPeriod] = simcli.FlagPeriodValue - app := NewSimApp(logger, db, nil, true, appOptions, fauxMerkleModeOpt) + app := NewSimApp(logger, db, nil, true, appOptions, fauxMerkleModeOpt, baseapp.SetChainID(SimAppChainID)) require.Equal(t, "SimApp", app.Name()) // run randomized simulation @@ -127,7 +127,7 @@ func TestAppImportExport(t *testing.T) { appOptions[flags.FlagHome] = DefaultNodeHome appOptions[server.FlagInvCheckPeriod] = simcli.FlagPeriodValue - app := NewSimApp(logger, db, nil, true, appOptions, fauxMerkleModeOpt) + app := NewSimApp(logger, db, nil, true, appOptions, fauxMerkleModeOpt, baseapp.SetChainID(SimAppChainID)) require.Equal(t, "SimApp", app.Name()) // Run randomized simulation @@ -167,7 +167,7 @@ func TestAppImportExport(t *testing.T) { require.NoError(t, os.RemoveAll(newDir)) }() - newApp := NewSimApp(log.NewNopLogger(), newDB, nil, true, appOptions, fauxMerkleModeOpt) + newApp := NewSimApp(log.NewNopLogger(), newDB, nil, true, appOptions, fauxMerkleModeOpt, baseapp.SetChainID(SimAppChainID)) require.Equal(t, "SimApp", newApp.Name()) var genesisState GenesisState @@ -243,7 +243,7 @@ func TestAppSimulationAfterImport(t *testing.T) { appOptions[flags.FlagHome] = DefaultNodeHome appOptions[server.FlagInvCheckPeriod] = simcli.FlagPeriodValue - app := NewSimApp(logger, db, nil, true, appOptions, fauxMerkleModeOpt) + app := NewSimApp(logger, db, nil, true, appOptions, fauxMerkleModeOpt, baseapp.SetChainID(SimAppChainID)) require.Equal(t, "SimApp", app.Name()) // Run randomized simulation @@ -288,7 +288,7 @@ func TestAppSimulationAfterImport(t *testing.T) { require.NoError(t, os.RemoveAll(newDir)) }() - newApp := NewSimApp(log.NewNopLogger(), newDB, nil, true, appOptions, fauxMerkleModeOpt) + newApp := NewSimApp(log.NewNopLogger(), newDB, nil, true, appOptions, fauxMerkleModeOpt, baseapp.SetChainID(SimAppChainID)) require.Equal(t, "SimApp", newApp.Name()) newApp.InitChain(abci.RequestInitChain{ @@ -343,7 +343,7 @@ func TestAppStateDeterminism(t *testing.T) { } db := dbm.NewMemDB() - app := NewSimApp(logger, db, nil, true, appOptions, interBlockCacheOpt()) + app := NewSimApp(logger, db, nil, true, appOptions, interBlockCacheOpt(), baseapp.SetChainID(SimAppChainID)) fmt.Printf( "running non-determinism simulation; seed %d: %d/%d, attempt: %d/%d\n", diff --git a/simapp/test_helpers.go b/simapp/test_helpers.go index 3f53ffc9cf13..fdd19ac7e230 100644 --- a/simapp/test_helpers.go +++ b/simapp/test_helpers.go @@ -231,6 +231,7 @@ func NewTestNetworkFixture() network.TestFixture { simtestutil.NewAppOptionsWithFlagHome(val.GetCtx().Config.RootDir), bam.SetPruning(pruningtypes.NewPruningOptionsFromString(val.GetAppConfig().Pruning)), bam.SetMinGasPrices(val.GetAppConfig().MinGasPrices), + bam.SetChainID(val.GetCtx().Viper.GetString(flags.FlagChainID)), ) } diff --git a/tests/e2e/params/suite.go b/tests/e2e/params/suite.go index febf4c5ff1f8..0d4c05df0fb4 100644 --- a/tests/e2e/params/suite.go +++ b/tests/e2e/params/suite.go @@ -72,6 +72,7 @@ func (s *E2ETestSuite) SetupSuite() { nil, baseapp.SetPruning(pruningtypes.NewPruningOptionsFromString(val.GetAppConfig().Pruning)), baseapp.SetMinGasPrices(val.GetAppConfig().MinGasPrices), + baseapp.SetChainID(s.cfg.ChainID), ) s.Require().NoError(app.Load(false)) diff --git a/testutil/network/network.go b/testutil/network/network.go index aaf898023b7b..14655c4ad31a 100644 --- a/testutil/network/network.go +++ b/testutil/network/network.go @@ -31,6 +31,7 @@ import ( "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/client" + "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/client/grpc/cmtservice" "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/codec" @@ -211,6 +212,7 @@ func DefaultConfigWithAppConfig(appConfig depinject.Config) (Config, error) { nil, baseapp.SetPruning(pruningtypes.NewPruningOptionsFromString(val.GetAppConfig().Pruning)), baseapp.SetMinGasPrices(val.GetAppConfig().MinGasPrices), + baseapp.SetChainID(cfg.ChainID), ) testdata.RegisterQueryServer(app.GRPCQueryRouter(), testdata.QueryImpl{}) @@ -571,6 +573,9 @@ func New(l Logger, baseDir string, cfg Config) (*Network, error) { WithTxConfig(cfg.TxConfig). WithAccountRetriever(cfg.AccountRetriever) + // Provide ChainID here since we can't modify it in the Comet config. + ctx.Viper.Set(flags.FlagChainID, cfg.ChainID) + network.Validators[i] = &Validator{ AppConfig: appCfg, ClientCtx: clientCtx,