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

…7161) Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Facundo Medica <[email protected]>
cosmos · Aug 3, 2023 · 4c18ff6 · 4c18ff6
1 parent 3eee96d
commit 4c18ff6
Show file tree

Hide file tree

Showing 8 changed files with 169 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -3,14 +3,51 @@ 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"
 	"github.com/cosmos/cosmos-sdk/x/auth/ante"
 	"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()