Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs #15303

Merged
merged 34 commits into from
Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
5600add
fix: remove previous header to avoid inconsistencies in Prepare\/Proc…
facundomedica Mar 8, 2023
a07fd6f
nit
facundomedica Mar 8, 2023
fbe491f
get chain id from genesis
facundomedica Mar 8, 2023
ac12759
progress
facundomedica Mar 8, 2023
233a4a4
progress
facundomedica Mar 8, 2023
fe1cd5b
progress
facundomedica Mar 8, 2023
257f4be
progress
facundomedica Mar 8, 2023
06d2021
progress
facundomedica Mar 8, 2023
4a79357
progress
facundomedica Mar 8, 2023
71d93ca
progress
facundomedica Mar 8, 2023
755611d
progress
facundomedica Mar 8, 2023
be89d62
fix leak
facundomedica Mar 8, 2023
bf4c3f2
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…
facundomedica Mar 8, 2023
d7e7bd8
Merge branch 'main' into facu/empty-header-prepproc-prop
facundomedica Mar 9, 2023
09ae43a
cl++
facundomedica Mar 9, 2023
a578ddb
add some extra checks and fix context for prepare and process proposal
facundomedica Mar 10, 2023
630175a
add some extra checks and fix context for prepare and process proposal
facundomedica Mar 10, 2023
950cd21
fix comment
facundomedica Mar 10, 2023
966dba8
fix e2e test in params
facundomedica Mar 10, 2023
820e34e
rm comment
facundomedica Mar 10, 2023
b17f178
attempt to simplify (#15348)
julienrbrt Mar 10, 2023
adfac48
Merge branch 'facu/empty-header-prepproc-prop' of https://github.com/…
facundomedica Mar 10, 2023
3f23381
fix
facundomedica Mar 10, 2023
3bcaa15
rollback rollback.go
facundomedica Mar 10, 2023
bc1b4c1
Merge branch 'main' into facu/empty-header-prepproc-prop
facundomedica Mar 10, 2023
f5e883e
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…
facundomedica Mar 10, 2023
52a64aa
suggestions from @alexanderbez
facundomedica Mar 10, 2023
6b8b529
Merge branch 'main' into facu/empty-header-prepproc-prop
facundomedica Mar 13, 2023
02b6ad1
Merge branch 'main' into facu/empty-header-prepproc-prop
facundomedica Mar 13, 2023
143af44
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…
facundomedica Mar 13, 2023
f307b3b
remove some unnecessary changes, like new lines
facundomedica Mar 13, 2023
89b90da
Merge branch 'facu/empty-header-prepproc-prop' of https://github.com/…
facundomedica Mar 13, 2023
ed69776
Merge branch 'main' into facu/empty-header-prepproc-prop
facundomedica Mar 13, 2023
814fc92
Merge branch 'main' into facu/empty-header-prepproc-prop
facundomedica Mar 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 39 additions & 20 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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
Expand Down Expand Up @@ -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))

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call
}

if app.cms.TracingEnabled() {
app.cms.SetTracingContext(storetypes.TraceContext(
map[string]interface{}{"blockHeight": req.Header.Height},
Expand Down Expand Up @@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now we set app.prepareProposalState.ctx in PrepareProposal. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so the issue here is that we were passing ctx to the prepareProposalHandler but runTx uses app.prepareProposalState.ctx, meaning that we were verifying txs against a context that didn't have the right height, consensusParams, and all the stuff we were setting to the ctx (same for processProposal). Now runTx uses the exact same context as the handler.
This was resulting in txs passing the verification in prepareProposal but failing in processProposal once I stopped passing the previous' block header.
Also in BeginBlock and EndBlock we also apply changes to app.deliverState.ctx the same way as here.

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 {
Expand All @@ -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
}

Expand All @@ -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 {
Expand All @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions baseapp/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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))

Expand Down
4 changes: 3 additions & 1 deletion baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions baseapp/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
3 changes: 2 additions & 1 deletion server/rollback.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import (
"fmt"
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

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.
Expand Down
16 changes: 15 additions & 1 deletion server/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think this should not be a part of app config / flags. ChainID should be always read from the genesis , and panic if it's not there.
  2. Info about this is missing in UPGRADING.md (0.47)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

genesis does not always have the correct genesis, in most cases the file is incorrect

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))
}
Expand Down Expand Up @@ -492,5 +505,6 @@ func DefaultBaseappOptions(appOpts types.AppOptions) []func(*baseapp.BaseApp) {
),
),
baseapp.SetIAVLLazyLoading(cast.ToBool(appOpts.Get(FlagIAVLLazyLoading))),
baseapp.SetChainID(chainID),
}
}
12 changes: 6 additions & 6 deletions simapp/sim_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions simapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
)
}

Expand Down
1 change: 1 addition & 0 deletions tests/e2e/params/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
5 changes: 5 additions & 0 deletions testutil/network/network.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{})
Expand Down Expand Up @@ -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,
Expand Down