fix: check tx gas limit against block gas limit (#16547)

Co-authored-by: Facundo Medica <facundomedica@gmail.com>
Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>

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`.

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()

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

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

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"

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) {

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,

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

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()