diff --git a/CHANGELOG.md b/CHANGELOG.md index 67003dc255fd..c796a584801f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +### Bug Fixes + +* [#16547](https://github.com/cosmos/cosmos-sdk/pull/16547) Ensure a transaction's gas limit cannot exceed the block gas limit. + ### Improvements * (all) [#16497](https://github.com/cosmos/cosmos-sdk/pull/16497) Removed all exported vestiges of `sdk.MustSortJSON` and `sdk.SortJSON`. diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index f4ccaec78ef1..a67c5d1fb8bf 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -5,23 +5,22 @@ import ( "context" "errors" "fmt" + "strconv" "strings" "testing" errorsmod "cosmossdk.io/errors" "cosmossdk.io/log" - + pruningtypes "cosmossdk.io/store/pruning/types" + "cosmossdk.io/store/snapshots" + snapshottypes "cosmossdk.io/store/snapshots/types" + storetypes "cosmossdk.io/store/types" abci "github.com/cometbft/cometbft/abci/types" cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" dbm "github.com/cosmos/cosmos-db" "github.com/cosmos/gogoproto/jsonpb" "github.com/stretchr/testify/require" - pruningtypes "cosmossdk.io/store/pruning/types" - "cosmossdk.io/store/snapshots" - snapshottypes "cosmossdk.io/store/snapshots/types" - storetypes "cosmossdk.io/store/types" - "github.com/cosmos/cosmos-sdk/baseapp" baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil" "github.com/cosmos/cosmos-sdk/testutil" @@ -1286,6 +1285,43 @@ func TestABCI_PrepareProposal_BadEncoding(t *testing.T) { require.Equal(t, 1, len(resPrepareProposal.Txs)) } +func TestABCI_PrepareProposal_MaxGas(t *testing.T) { + pool := mempool.NewSenderNonceMempool() + suite := NewBaseAppSuite(t, baseapp.SetMempool(pool)) + baseapptestutil.RegisterCounterServer(suite.baseApp.MsgServiceRouter(), NoopCounterServerImpl{}) + + // set max block gas limit to 100 + suite.baseApp.InitChain(&abci.RequestInitChain{ + ConsensusParams: &cmtproto.ConsensusParams{ + Block: &cmtproto.BlockParams{MaxGas: 100}, + }, + }) + + // insert 100 txs, each with a gas limit of 10 + _, _, addr := testdata.KeyTestPubAddr() + for i := int64(0); i < 100; i++ { + msg := &baseapptestutil.MsgCounter{Counter: i, FailOnHandler: false, Signer: addr.String()} + msgs := []sdk.Msg{msg} + + builder := suite.txConfig.NewTxBuilder() + builder.SetMsgs(msgs...) + builder.SetMemo("counter=" + strconv.FormatInt(i, 10) + "&failOnAnte=false") + builder.SetGasLimit(10) + setTxSignature(t, builder, uint64(i)) + + err := pool.Insert(sdk.Context{}, builder.GetTx()) + require.NoError(t, err) + } + + // ensure we only select transactions that fit within the block gas limit + res, err := suite.baseApp.PrepareProposal(&abci.RequestPrepareProposal{ + MaxTxBytes: 1_000_000, // large enough to ignore restriction + Height: 1, + }) + require.NoError(t, err) + require.Len(t, res.Txs, 10, "invalid number of transactions returned") +} + func TestABCI_PrepareProposal_Failures(t *testing.T) { anteKey := []byte("ante-key") pool := mempool.NewSenderNonceMempool() diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 465dfa516416..930be5cd8197 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -40,6 +40,11 @@ type ( GetValidatorByConsAddr(sdk.Context, cryptotypes.Address) (Validator, error) TotalBondedTokens(ctx sdk.Context) math.Int } + + // GasTx defines the contract that a transaction with a gas limit must implement. + GasTx interface { + GetGas() uint64 + } ) // ValidateVoteExtensions defines a helper function for verifying vote extension @@ -186,9 +191,15 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand return &abci.ResponsePrepareProposal{Txs: req.Txs}, nil } + var maxBlockGas int64 + if b := ctx.ConsensusParams().Block; b != nil { + maxBlockGas = b.MaxGas + } + var ( selectedTxs [][]byte totalTxBytes int64 + totalTxGas uint64 ) iterator := h.mempool.Select(ctx, req.Txs) @@ -207,12 +218,31 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand panic(err) } } else { + var txGasLimit uint64 txSize := int64(len(bz)) - if totalTxBytes += txSize; totalTxBytes <= req.MaxTxBytes { - selectedTxs = append(selectedTxs, bz) - } else { - // We've reached capacity per req.MaxTxBytes so we cannot select any - // more transactions. + + gasTx, ok := memTx.(GasTx) + if ok { + txGasLimit = gasTx.GetGas() + } + + // only add the transaction to the proposal if we have enough capacity + if (txSize + totalTxBytes) < req.MaxTxBytes { + // If there is a max block gas limit, add the tx only if the limit has + // not been met. + if maxBlockGas > 0 && (txGasLimit+totalTxGas) <= uint64(maxBlockGas) { + totalTxGas += txGasLimit + totalTxBytes += txSize + selectedTxs = append(selectedTxs, bz) + } else { + totalTxBytes += txSize + selectedTxs = append(selectedTxs, bz) + } + } + + // Check if we've reached capacity. If so, we cannot select any more + // transactions. + if totalTxBytes >= req.MaxTxBytes || (maxBlockGas > 0 && (totalTxGas >= uint64(maxBlockGas))) { break } } @@ -244,11 +274,29 @@ func (h DefaultProposalHandler) ProcessProposalHandler() sdk.ProcessProposalHand } return func(ctx sdk.Context, req *abci.RequestProcessProposal) (*abci.ResponseProcessProposal, error) { + var totalTxGas uint64 + + var maxBlockGas int64 + if b := ctx.ConsensusParams().Block; b != nil { + maxBlockGas = b.MaxGas + } + for _, txBytes := range req.Txs { - _, err := h.txVerifier.ProcessProposalVerifyTx(txBytes) + tx, err := h.txVerifier.ProcessProposalVerifyTx(txBytes) if err != nil { return &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT}, nil } + + if maxBlockGas > 0 { + gasTx, ok := tx.(GasTx) + if ok { + totalTxGas += gasTx.GetGas() + } + + if totalTxGas > uint64(maxBlockGas) { + return &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT}, nil + } + } } return &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil diff --git a/baseapp/block_gas_test.go b/baseapp/block_gas_test.go index 518f796fa5f1..0374246c2713 100644 --- a/baseapp/block_gas_test.go +++ b/baseapp/block_gas_test.go @@ -64,7 +64,7 @@ func TestBaseApp_BlockGas(t *testing.T) { {"less than block gas meter", 10, false, false}, {"more than block gas meter", blockMaxGas, false, true}, {"more than block gas meter", uint64(float64(blockMaxGas) * 1.2), false, true}, - {"consume MaxUint64", math.MaxUint64, false, true}, + {"consume MaxUint64", math.MaxUint64, true, true}, {"consume MaxGasWanted", txtypes.MaxGasWanted, false, true}, {"consume block gas when panicked", 10, true, true}, } @@ -150,7 +150,7 @@ func TestBaseApp_BlockGas(t *testing.T) { require.NoError(t, txBuilder.SetMsgs(msg)) txBuilder.SetFeeAmount(feeAmount) - txBuilder.SetGasLimit(txtypes.MaxGasWanted) // tx validation checks that gasLimit can't be bigger than this + txBuilder.SetGasLimit(uint64(simtestutil.DefaultConsensusParams.Block.MaxGas)) senderAccountNumber := accountKeeper.GetAccount(ctx, addr1).GetAccountNumber() privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{senderAccountNumber}, []uint64{0} @@ -176,11 +176,11 @@ func TestBaseApp_BlockGas(t *testing.T) { require.Equal(t, []byte("ok"), okValue) } // check block gas is always consumed - baseGas := uint64(57554) // baseGas is the gas consumed before tx msg + baseGas := uint64(57504) // baseGas is the gas consumed before tx msg expGasConsumed := addUint64Saturating(tc.gasToConsume, baseGas) - if expGasConsumed > txtypes.MaxGasWanted { + if expGasConsumed > uint64(simtestutil.DefaultConsensusParams.Block.MaxGas) { // capped by gasLimit - expGasConsumed = txtypes.MaxGasWanted + expGasConsumed = uint64(simtestutil.DefaultConsensusParams.Block.MaxGas) } require.Equal(t, int(expGasConsumed), int(ctx.BlockGasMeter().GasConsumed())) // tx fee is always deducted diff --git a/baseapp/utils_test.go b/baseapp/utils_test.go index e436b47ad73e..154a73e84bda 100644 --- a/baseapp/utils_test.go +++ b/baseapp/utils_test.go @@ -25,6 +25,7 @@ import ( "cosmossdk.io/core/appconfig" storetypes "cosmossdk.io/store/types" + "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/cosmos/cosmos-sdk/baseapp" diff --git a/docs/docs/building-apps/02-app-mempool.md b/docs/docs/building-apps/02-app-mempool.md index a8083f76f42b..9701df1d23f0 100644 --- a/docs/docs/building-apps/02-app-mempool.md +++ b/docs/docs/building-apps/02-app-mempool.md @@ -44,7 +44,10 @@ Allowing the application to handle ordering enables the application to define ho it would like the block constructed. The Cosmos SDK defines the `DefaultProposalHandler` type, which provides applications with -`PrepareProposal` and `ProcessProposal` handlers. +`PrepareProposal` and `ProcessProposal` handlers. If you decide to implement your +own `PrepareProposal` handler, you must be sure to ensure that the transactions +selected DO NOT exceed the maximum block gas (if set) and the maximum bytes provided +by `req.MaxBytes`. ```go reference https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/baseapp/baseapp.go#L868-L916 @@ -81,7 +84,10 @@ Here is the implementation of the default implementation: https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/baseapp/baseapp.go#L927-L942 ``` -Like `PrepareProposal` this implementation is the default and can be modified by the application developer in [`app.go`](./01-app-go-v2.md): +Like `PrepareProposal` this implementation is the default and can be modified by +the application developer in [`app.go`](./01-app-go-v2.md). If you decide to implement +your own `ProcessProposal` handler, you must be sure to ensure that the transactions +provided in the proposal DO NOT exceed the maximum block gas (if set). ```go processOpt := func(app *baseapp.BaseApp) { diff --git a/testutil/sims/app_helpers.go b/testutil/sims/app_helpers.go index 6fdbec1f5b71..cbc22ad2a52d 100644 --- a/testutil/sims/app_helpers.go +++ b/testutil/sims/app_helpers.go @@ -34,7 +34,7 @@ const DefaultGenTxGas = 10000000 var DefaultConsensusParams = &cmtproto.ConsensusParams{ Block: &cmtproto.BlockParams{ MaxBytes: 200000, - MaxGas: 2000000, + MaxGas: 100_000_000, }, Evidence: &cmtproto.EvidenceParams{ MaxAgeNumBlocks: 302400, diff --git a/x/auth/ante/setup.go b/x/auth/ante/setup.go index 4b6e281ac394..06ffc6e4ce06 100644 --- a/x/auth/ante/setup.go +++ b/x/auth/ante/setup.go @@ -40,6 +40,14 @@ func (sud SetUpContextDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate newCtx = SetGasMeter(simulate, ctx, gasTx.GetGas()) + if cp := ctx.ConsensusParams(); cp.Block != nil { + // If there exists a maximum block gas limit, we must ensure that the tx + // does not exceed it. + if cp.Block.MaxGas > 0 && gasTx.GetGas() > uint64(cp.Block.MaxGas) { + return newCtx, errorsmod.Wrapf(sdkerrors.ErrInvalidGasLimit, "tx gas limit %d exceeds block max gas %d", gasTx.GetGas(), cp.Block.MaxGas) + } + } + // Decorator will catch an OutOfGasPanic caused in the next antehandler // AnteHandlers must have their own defer/recover in order for the BaseApp // to know how much gas was used! This is because the GasMeter is created in diff --git a/x/auth/ante/setup_test.go b/x/auth/ante/setup_test.go index 67c408218af1..70964e4075c6 100644 --- a/x/auth/ante/setup_test.go +++ b/x/auth/ante/setup_test.go @@ -4,6 +4,7 @@ import ( "testing" storetypes "cosmossdk.io/store/types" + cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" "github.com/stretchr/testify/require" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" @@ -14,6 +15,40 @@ import ( "github.com/cosmos/cosmos-sdk/x/auth/ante" ) +func TestSetupDecorator_BlockMaxGas(t *testing.T) { + suite := SetupTestSuite(t, true) + suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() + + // keys and addresses + priv1, _, addr1 := testdata.KeyTestPubAddr() + + // msg and signatures + msg := testdata.NewTestMsg(addr1) + feeAmount := testdata.NewTestFeeAmount() + require.NoError(t, suite.txBuilder.SetMsgs(msg)) + suite.txBuilder.SetFeeAmount(feeAmount) + suite.txBuilder.SetGasLimit(101) + + privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0} + tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT) + require.NoError(t, err) + + sud := ante.NewSetUpContextDecorator() + antehandler := sdk.ChainAnteDecorators(sud) + + suite.ctx = suite.ctx. + WithBlockHeight(1). + WithGasMeter(storetypes.NewGasMeter(0)). + WithConsensusParams(cmtproto.ConsensusParams{ + Block: &cmtproto.BlockParams{ + MaxGas: 100, + }, + }) + + _, err = antehandler(suite.ctx, tx, false) + require.Error(t, err) +} + func TestSetup(t *testing.T) { suite := SetupTestSuite(t, true) suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()