diff --git a/CHANGELOG.md b/CHANGELOG.md index 31bca17a0809..39711a2bf337 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (baseapp) [#17159](https://github.com/cosmos/cosmos-sdk/pull/17159) Validators can propose blocks that exceed the gas limit. +* (baseapp) [#16547](https://github.com/cosmos/cosmos-sdk/pull/16547) Ensure a transaction's gas limit cannot exceed the block gas limit. * (x/gov,x/group) [#17220](https://github.com/cosmos/cosmos-sdk/pull/17220) Do not try validate `msgURL` as web URL in `draft-proposal` command. * (cli) [#17188](https://github.com/cosmos/cosmos-sdk/pull/17188) Fix `--output-document` flag in `tx multi-sign`. * (x/auth) [#17209](https://github.com/cosmos/cosmos-sdk/pull/17209) Internal error on AccountInfo when account's public key is not set. diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 61f8a7d551f2..c3d606db018a 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "fmt" + "strconv" "strings" "testing" @@ -1480,6 +1481,79 @@ func TestABCI_PrepareProposal_BadEncoding(t *testing.T) { require.Equal(t, 1, len(resPrepareProposal.Txs)) } +func TestABCI_PrepareProposal_OverGasUnderBytes(t *testing.T) { + pool := mempool.NewSenderNonceMempool() + suite := NewBaseAppSuite(t, baseapp.SetMempool(pool)) + baseapptestutil.RegisterCounterServer(suite.baseApp.MsgServiceRouter(), NoopCounterServerImpl{}) + + // set max block gas limit to 99, this will allow 9 txs of 10 gas each. + suite.baseApp.InitChain(abci.RequestInitChain{ + ConsensusParams: &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{MaxGas: 99}, + }, + }) + + // insert 100 txs, each with a gas limit of 10 + for i := int64(0); i < 100; i++ { + msg := &baseapptestutil.MsgCounter{Counter: i, FailOnHandler: false} + msgs := []sdk.Msg{msg} + + builder := suite.txConfig.NewTxBuilder() + err := builder.SetMsgs(msgs...) + require.NoError(t, err) + 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 := suite.baseApp.PrepareProposal(abci.RequestPrepareProposal{ + MaxTxBytes: 1_000_000, // large enough to ignore restriction + Height: 1, + }) + + // Should include 9 transactions + require.Len(t, res.Txs, 9, "invalid number of transactions returned") +} + +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: &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{MaxGas: 100}, + }, + }) + + // insert 100 txs, each with a gas limit of 10 + for i := int64(0); i < 100; i++ { + msg := &baseapptestutil.MsgCounter{Counter: i, FailOnHandler: false} + 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 := suite.baseApp.PrepareProposal(abci.RequestPrepareProposal{ + MaxTxBytes: 1_000_000, // large enough to ignore restriction + Height: 1, + }) + 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/baseapp.go b/baseapp/baseapp.go index 3a90eaf2b815..9b4faa5376ed 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -958,9 +958,15 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand return abci.ResponsePrepareProposal{Txs: req.Txs} } + 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) @@ -979,12 +985,33 @@ 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.(interface{ GetGas() uint64 }) + 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 { + if (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 } } diff --git a/baseapp/block_gas_test.go b/baseapp/block_gas_test.go index 009748cb686c..81b39a0c2a81 100644 --- a/baseapp/block_gas_test.go +++ b/baseapp/block_gas_test.go @@ -2,6 +2,7 @@ package baseapp_test import ( "context" + "fmt" "math" "testing" @@ -63,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}, } @@ -140,7 +141,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} @@ -166,13 +167,13 @@ func TestBaseApp_BlockGas(t *testing.T) { require.Equal(t, []byte("ok"), okValue) } // check block gas is always consumed - baseGas := uint64(51732) // baseGas is the gas consumed before tx msg + baseGas := uint64(51682) // 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, expGasConsumed, ctx.BlockGasMeter().GasConsumed()) + require.Equal(t, expGasConsumed, ctx.BlockGasMeter().GasConsumed(), fmt.Sprintf("exp: %d, got: %d", expGasConsumed, ctx.BlockGasMeter().GasConsumed())) // tx fee is always deducted require.Equal(t, int64(0), bankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount.Int64()) // sender's sequence is always increased diff --git a/docs/docs/building-apps/02-app-mempool.md b/docs/docs/building-apps/02-app-mempool.md index 3957c8a316ce..ee90b1a551d8 100644 --- a/docs/docs/building-apps/02-app-mempool.md +++ b/docs/docs/building-apps/02-app-mempool.md @@ -42,7 +42,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 @@ -79,7 +82,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 e7e03314ecdf..f6cd882a680e 100644 --- a/testutil/sims/app_helpers.go +++ b/testutil/sims/app_helpers.go @@ -36,7 +36,7 @@ const DefaultGenTxGas = 10000000 var DefaultConsensusParams = &tmproto.ConsensusParams{ Block: &tmproto.BlockParams{ MaxBytes: 200000, - MaxGas: 2000000, + MaxGas: 100_000_000, }, Evidence: &tmproto.EvidenceParams{ MaxAgeNumBlocks: 302400, diff --git a/x/auth/ante/setup.go b/x/auth/ante/setup.go index 4e9ffe862b9c..5a101a24950a 100644 --- a/x/auth/ante/setup.go +++ b/x/auth/ante/setup.go @@ -39,6 +39,14 @@ func (sud SetUpContextDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate newCtx = SetGasMeter(simulate, ctx, gasTx.GetGas()) + if cp := ctx.ConsensusParams(); cp != nil && 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, sdkerrors.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 e7138af70e19..e3488ad2e9ea 100644 --- a/x/auth/ante/setup_test.go +++ b/x/auth/ante/setup_test.go @@ -3,7 +3,10 @@ package ante_test import ( "testing" + tmproto "github.com/cometbft/cometbft/proto/tendermint/types" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + storetypes "github.com/cosmos/cosmos-sdk/store/types" "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" @@ -11,6 +14,40 @@ import ( "github.com/stretchr/testify/require" ) +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(privs, accNums, accSeqs, suite.ctx.ChainID()) + require.NoError(t, err) + + sud := ante.NewSetUpContextDecorator() + antehandler := sdk.ChainAnteDecorators(sud) + + suite.ctx = suite.ctx. + WithBlockHeight(1). + WithGasMeter(storetypes.NewGasMeter(0)). + WithConsensusParams(&tmproto.ConsensusParams{ + Block: &tmproto.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()