From bf46b77b92b8014a106215f716304d59cf39ab37 Mon Sep 17 00:00:00 2001 From: Alex | Interchain Labs Date: Mon, 9 Jun 2025 11:53:34 -0400 Subject: [PATCH] chore: fix `TimeoutCommit` confusion (#24853) --- UPGRADING.md | 11 +++++----- baseapp/baseapp.go | 16 +++++++-------- baseapp/options.go | 28 +++++++++++++------------- depinject/provider_desc_test.go | 2 +- server/util.go | 6 ++++++ simapp/simd/cmd/testnet.go | 21 ++++++++++--------- tests/systemtests/upgrade_test.go | 8 +++----- testutil/network/network.go | 1 + x/group/client/cli/tx.go | 2 +- x/group/client/cli/util.go | 2 +- x/group/keeper/genesis.go | 2 +- x/group/keeper/grpc_query.go | 2 +- x/group/keeper/keeper.go | 2 +- x/group/keeper/msg_server.go | 2 +- x/group/keeper/msg_server_priv_test.go | 2 +- x/group/keeper/proposal_executor.go | 2 +- x/group/keeper/tally.go | 2 +- 17 files changed, 59 insertions(+), 52 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index bc043cd579..2d533feaf4 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -15,11 +15,12 @@ For a full list of changes, see the [Changelog](https://github.com/cosmos/cosmos #### Deprecation of `TimeoutCommit` CometBFT v2 has deprecated the use of `TimeoutCommit` for a new field, `NextBlockDelay`, that is part of the -`FinalizeBlockResponse` ABCI message that is returned to CometBFT via the SDK baseapp. More information from +`FinalizeBlockResponse` ABCI message that is returned to CometBFT via the SDK baseapp. More information from the CometBFT repo can be found [here](https://github.com/cometbft/cometbft/blob/88ef3d267de491db98a654be0af6d791e8724ed0/spec/abci/abci%2B%2B_methods.md?plain=1#L689). -For SDK application developers and node runners, this means that the `timeout_commit` value in the `config.toml` file -is now **ignored**. +For SDK application developers and node runners, this means that the `timeout_commit` value in the `config.toml` file +is still used if `NextBlockDelay` is 0 (its default value). This means that when upgrading to Cosmos SDK v0.54.x, if +the existing `timout_commit` values that validators have been using will be maintained and have the same behavior. -For similar behavior, there is a new `baseapp` option, `SetNextBlockDelay` which can be passed to your application upon -initialization in `app.go`. \ No newline at end of file +For setting the field in your application, there is a new `baseapp` option, `SetNextBlockDelay` which can be passed to your application upon +initialization in `app.go`. Setting this value to any non-zero value will override anything that is set in validators' `config.toml`. \ No newline at end of file diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 90598d7a55..d13ba98b34 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -56,10 +56,6 @@ const ( execModeVoteExtension = sdk.ExecModeVoteExtension // Extend or verify a pre-commit vote execModeVerifyVoteExtension = sdk.ExecModeVerifyVoteExtension // Verify a vote extension execModeFinalize = sdk.ExecModeFinalize // Finalize a block proposal - - // defaultNextBlockDelay is chosen following documentation in CometBFT: - // https://github.com/cometbft/cometbft/blob/88ef3d267de491db98a654be0af6d791e8724ed0/spec/abci/abci%2B%2B_methods.md?plain=1#L689 - defaultNextBlockDelay = time.Second ) var _ servertypes.ABCI = (*BaseApp)(nil) @@ -113,10 +109,6 @@ type BaseApp struct { // flag for sealing options and parameters to a BaseApp sealed bool - // nextBlockDelay is the delay to wait until the next block after ABCI has committed. - // This gives the application more time to receive precommits. - nextBlockDelay time.Duration - // block height at which to halt the chain and gracefully shutdown haltHeight uint64 @@ -170,6 +162,12 @@ type BaseApp struct { // // SAFETY: it's safe to do if validators validate the total gas wanted in the `ProcessProposal`, which is the case in the default handler. disableBlockGasMeter bool + + // nextBlockDelay is the delay to wait until the next block after ABCI has committed. + // This gives the application more time to receive precommits. This is the same as TimeoutCommit, + // but can new be set from the application. This value defaults to 0, and CometBFT will use the + // legacy value set in config.toml if it is 0. + nextBlockDelay time.Duration } // NewBaseApp returns a reference to an initialized BaseApp. It accepts a @@ -190,7 +188,7 @@ func NewBaseApp( fauxMerkleMode: false, sigverifyTx: true, gasConfig: config.GasConfig{QueryGasLimit: math.MaxUint64}, - nextBlockDelay: defaultNextBlockDelay, + nextBlockDelay: 0, // default to 0 so that the legacy CometBFT config.toml value is used } for _, option := range options { diff --git a/baseapp/options.go b/baseapp/options.go index 574c510acd..1edcdbfe7c 100644 --- a/baseapp/options.go +++ b/baseapp/options.go @@ -24,6 +24,20 @@ import ( // File for storing in-package BaseApp optional functions, // for options that need access to non-exported fields of the BaseApp +// SetNextBlockDelay sets the next block delay for the baseapp. +// +// The application is initialized with a default value of 0. +// +// More information on this value and how it affects CometBFT can be found here: +// https://github.com/cometbft/cometbft/blob/88ef3d267de491db98a654be0af6d791e8724ed0/spec/abci/abci%2B%2B_methods.md?plain=1#L689 +func (app *BaseApp) SetNextBlockDelay(delay time.Duration) { + if app.sealed { + panic("SetNextBlockDelay() on sealed BaseApp") + } + + app.nextBlockDelay = delay +} + // SetPruning sets a pruning option on the multistore associated with the app func SetPruning(opts pruningtypes.PruningOptions) func(*BaseApp) { return func(bapp *BaseApp) { bapp.cms.SetPruning(opts) } @@ -334,20 +348,6 @@ func (app *BaseApp) SetMempool(mempool mempool.Mempool) { app.mempool = mempool } -// SetNextBlockDelay sets the next block delay for the baseapp. -// -// The application is initialized with a default value of 1s. -// -// More information on this value and how it affects CometBFT can be found here: -// https://github.com/cometbft/cometbft/blob/88ef3d267de491db98a654be0af6d791e8724ed0/spec/abci/abci%2B%2B_methods.md?plain=1#L689 -func (app *BaseApp) SetNextBlockDelay(delay time.Duration) { - if app.sealed { - panic("SetNextBlockDelay() on sealed BaseApp") - } - - app.nextBlockDelay = delay -} - // SetProcessProposal sets the process proposal function for the BaseApp. func (app *BaseApp) SetProcessProposal(handler sdk.ProcessProposalHandler) { if app.sealed { diff --git a/depinject/provider_desc_test.go b/depinject/provider_desc_test.go index 4f16849149..a5b30fd666 100644 --- a/depinject/provider_desc_test.go +++ b/depinject/provider_desc_test.go @@ -45,7 +45,7 @@ func StructInAndOut(_ float32, _ StructIn, _ byte) (int16, StructOut, int32, err return int16(0), StructOut{}, int32(0), nil } -func BadErrorPosition() (error, int) { return nil, 0 } //nolint:stylecheck,staticcheck // Deliberately has error as first of multiple arguments. +func BadErrorPosition() (error, int) { return nil, 0 } //nolint:staticcheck // Deliberately has error as first of multiple arguments. func BadOptionalFn(_ BadOptional) int { return 0 } diff --git a/server/util.go b/server/util.go index 8bab2ce7e1..ef64d63b55 100644 --- a/server/util.go +++ b/server/util.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strings" "syscall" + "time" cmtcmd "github.com/cometbft/cometbft/v2/cmd/cometbft/commands" cmtcfg "github.com/cometbft/cometbft/v2/config" @@ -262,6 +263,11 @@ func interceptConfigs(rootViper *viper.Viper, customAppTemplate string, customCo } defaultCometCfg := cmtcfg.DefaultConfig() + // The SDK is opinionated about those comet values, so we set them here. + // We verify first that the user has not changed them for not overriding them. + if conf.Consensus.TimeoutCommit == defaultCometCfg.Consensus.TimeoutCommit { // nolint: staticcheck // we are continuing to use this value for backwards compatibility + conf.Consensus.TimeoutCommit = 5 * time.Second // nolint: staticcheck // we are continuing to use this value for backwards compatibility + } if conf.RPC.PprofListenAddress == defaultCometCfg.RPC.PprofListenAddress { conf.RPC.PprofListenAddress = "localhost:6060" } diff --git a/simapp/simd/cmd/testnet.go b/simapp/simd/cmd/testnet.go index b1664d7357..883da38a68 100644 --- a/simapp/simd/cmd/testnet.go +++ b/simapp/simd/cmd/testnet.go @@ -51,11 +51,8 @@ var ( flagAPIAddress = "api.address" flagPrintMnemonic = "print-mnemonic" flagStakingDenom = "staking-denom" - // flagCommitTimeout is a deprecated flag whose value will not be used. - // - // Deprecated: set NextBlockDelay on the app with baseapp.SetNextBlockDelay() - flagCommitTimeout = "commit-timeout" - flagSingleHost = "single-host" + flagCommitTimeout = "commit-timeout" + flagSingleHost = "single-host" ) type initArgs struct { @@ -84,7 +81,6 @@ type startArgs struct { outputDir string printMnemonic bool rpcAddress string - // Deprecated: use baseapp.SetNextBlockDelay() instead. timeoutCommit time.Duration } @@ -160,6 +156,7 @@ Example: args.algo, _ = cmd.Flags().GetString(flags.FlagKeyType) args.bondTokenDenom, _ = cmd.Flags().GetString(flagStakingDenom) args.singleMachine, _ = cmd.Flags().GetBool(flagSingleHost) + config.Consensus.TimeoutCommit, err = cmd.Flags().GetDuration(flagCommitTimeout) // nolint: staticcheck // we are continuing to use this value for backwards compatibility if err != nil { return err } @@ -205,6 +202,7 @@ Example: args.apiAddress, _ = cmd.Flags().GetString(flagAPIAddress) args.grpcAddress, _ = cmd.Flags().GetString(flagGRPCAddress) args.printMnemonic, _ = cmd.Flags().GetBool(flagPrintMnemonic) + args.timeoutCommit, _ = cmd.Flags().GetDuration(flagCommitTimeout) return startTestnet(cmd, args) }, @@ -454,9 +452,14 @@ func initGenFiles( } func collectGenFiles( - clientCtx client.Context, nodeConfig *cmtconfig.Config, chainID string, - nodeIDs []string, valPubKeys []cryptotypes.PubKey, numValidators int, - outputDir, nodeDirPrefix, nodeDaemonHome string, genBalIterator banktypes.GenesisBalancesIterator, + clientCtx client.Context, + nodeConfig *cmtconfig.Config, + chainID string, + nodeIDs []string, + valPubKeys []cryptotypes.PubKey, + numValidators int, + outputDir, nodeDirPrefix, nodeDaemonHome string, + genBalIterator banktypes.GenesisBalancesIterator, rpcPortStart, p2pPortStart int, singleMachine bool, ) error { diff --git a/tests/systemtests/upgrade_test.go b/tests/systemtests/upgrade_test.go index 6a98c5a439..c741df3c1d 100644 --- a/tests/systemtests/upgrade_test.go +++ b/tests/systemtests/upgrade_test.go @@ -19,7 +19,7 @@ import ( const ( testSeed = "scene learn remember glide apple expand quality spawn property shoe lamp carry upset blossom draft reject aim file trash miss script joy only measure" - upgradeHeight int64 = 45 + upgradeHeight int64 = 22 upgradeName = "v053-to-v054" // must match UpgradeName in simapp/upgrades.go ) @@ -28,7 +28,7 @@ func TestChainUpgrade(t *testing.T) { // start a legacy chain with some state // when a chain upgrade proposal is executed // then the chain upgrades successfully - systest.Sut.ResetChain(t) + systest.Sut.StopChain() currentBranchBinary := systest.Sut.ExecBinary() currentInitializer := systest.Sut.TestnetInitializer() @@ -67,10 +67,8 @@ func TestChainUpgrade(t *testing.T) { raw := cli.CustomQuery("q", "gov", "proposal", proposalID) t.Log(raw) - // generous timeout as this could run faster or slower based on HW or in CI - systest.Sut.AwaitBlockHeight(t, upgradeHeight-1, 2*time.Minute) + systest.Sut.AwaitBlockHeight(t, upgradeHeight-1, 60*time.Second) t.Logf("current_height: %d\n", systest.Sut.CurrentHeight()) - raw = cli.CustomQuery("q", "gov", "proposal", proposalID) proposalStatus := gjson.Get(raw, "proposal.status").String() require.Equal(t, "PROPOSAL_STATUS_PASSED", proposalStatus, raw) diff --git a/testutil/network/network.go b/testutil/network/network.go index 01c9613b36..e4c0ee50f9 100644 --- a/testutil/network/network.go +++ b/testutil/network/network.go @@ -381,6 +381,7 @@ func New(l Logger, baseDir string, cfg Config) (*Network, error) { ctx := server.NewDefaultContext() cmtCfg := ctx.Config + cmtCfg.Consensus.TimeoutCommit = cfg.TimeoutCommit // nolint: staticcheck // we are continuing to use this value for backwards compatibility // Only allow the first validator to expose an RPC, API and gRPC // server/client due to CometBFT in-process constraints. diff --git a/x/group/client/cli/tx.go b/x/group/client/cli/tx.go index ac89bd93d5..b5d3f05239 100644 --- a/x/group/client/cli/tx.go +++ b/x/group/client/cli/tx.go @@ -15,7 +15,7 @@ import ( "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/version" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed "github.com/cosmos/cosmos-sdk/x/group/internal/math" ) diff --git a/x/group/client/cli/util.go b/x/group/client/cli/util.go index f0eb1a4bff..1b6ff108ce 100644 --- a/x/group/client/cli/util.go +++ b/x/group/client/cli/util.go @@ -7,7 +7,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed ) // parseDecisionPolicy reads and parses the decision policy. diff --git a/x/group/keeper/genesis.go b/x/group/keeper/genesis.go index 21e6fddeb2..375c765f1b 100644 --- a/x/group/keeper/genesis.go +++ b/x/group/keeper/genesis.go @@ -9,7 +9,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed ) // InitGenesis initializes the group module's genesis state. diff --git a/x/group/keeper/grpc_query.go b/x/group/keeper/grpc_query.go index 8c82582d08..abce101526 100644 --- a/x/group/keeper/grpc_query.go +++ b/x/group/keeper/grpc_query.go @@ -11,7 +11,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed "github.com/cosmos/cosmos-sdk/x/group/errors" "github.com/cosmos/cosmos-sdk/x/group/internal/orm" ) diff --git a/x/group/keeper/keeper.go b/x/group/keeper/keeper.go index 2a15dc5075..0454bdda3e 100644 --- a/x/group/keeper/keeper.go +++ b/x/group/keeper/keeper.go @@ -13,7 +13,7 @@ import ( "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed "github.com/cosmos/cosmos-sdk/x/group/errors" "github.com/cosmos/cosmos-sdk/x/group/internal/orm" ) diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go index 4686182861..e13f8dff4c 100644 --- a/x/group/keeper/msg_server.go +++ b/x/group/keeper/msg_server.go @@ -15,7 +15,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed "github.com/cosmos/cosmos-sdk/x/group/errors" "github.com/cosmos/cosmos-sdk/x/group/internal/math" "github.com/cosmos/cosmos-sdk/x/group/internal/orm" diff --git a/x/group/keeper/msg_server_priv_test.go b/x/group/keeper/msg_server_priv_test.go index 4181744e07..9f82c3a9e4 100644 --- a/x/group/keeper/msg_server_priv_test.go +++ b/x/group/keeper/msg_server_priv_test.go @@ -19,7 +19,7 @@ import ( "github.com/cosmos/cosmos-sdk/testutil" sdk "github.com/cosmos/cosmos-sdk/types" moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed "github.com/cosmos/cosmos-sdk/x/group/internal/math" ) diff --git a/x/group/keeper/proposal_executor.go b/x/group/keeper/proposal_executor.go index 6330acc2da..1c821dd2df 100644 --- a/x/group/keeper/proposal_executor.go +++ b/x/group/keeper/proposal_executor.go @@ -10,7 +10,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed "github.com/cosmos/cosmos-sdk/x/group/errors" ) diff --git a/x/group/keeper/tally.go b/x/group/keeper/tally.go index b3436096e1..81bd2b1ce0 100644 --- a/x/group/keeper/tally.go +++ b/x/group/keeper/tally.go @@ -5,7 +5,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - "github.com/cosmos/cosmos-sdk/x/group" //nolint:staticcheck // deprecated and to be removed + "github.com/cosmos/cosmos-sdk/x/group" // nolint: staticcheck // to be removed "github.com/cosmos/cosmos-sdk/x/group/errors" "github.com/cosmos/cosmos-sdk/x/group/internal/orm" )