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

alexanderbez · 2023-06-14T15:42:45Z

Description

CometBFT has an explicit post-CheckTx check that asserts the gas limit of a tx cannot exceed the block gas limit. However, with the advent of ABCI++ and app-side mempools, this would would not prevent txs that exceed the block gas limit to enter the app's mempool since the CometBFT check is ran after the app returns from CheckTx. I.e., the CometBFT check is too late, so we need to ensure we do this app-side.

Changelog

Update Setup ante decorator to account for block max gas
Update default PrepareProposal handler to account for block max gas
Update default ProcessProposal handler to account for block max gas

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

… into bez/check-tx-gas-block

tac0turtle · 2023-06-15T08:21:08Z

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 {


i dont think this fixes the issue at hand, the mempool could still put in an amount of txs that is larger the maxgas.

No, because txs are only inserted into the app's mempool if CheckTx passes. CheckTx would fail here so such a tx would never enter the app's mempool.

However, we also need to update the SDK's default ABCI++ proposal handlers so we can handle an entire proposal.

… into bez/check-tx-gas-block

tac0turtle

looks good, two asks:

testcase on prepareproposal
update the building app section to include this as a warning. Users must check both gas and size

tac0turtle · 2023-06-16T11:53:50Z

baseapp/abci_utils.go

@@ -186,9 +191,15 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand
 			return &abci.ResponsePrepareProposal{Txs: req.Txs}, nil


can we add a testcase that covers this.

baseapp/abci_utils.go

facundomedica · 2023-06-20T14:33:49Z

Meant to restart only the e2e cause it seems like the issue is on github and ended up restarting also tests(00) which has a couple of unit test fails. I'll try to fix them

baseapp/abci_utils.go

…gas limit is higher than maxblockgas it panics

facundomedica · 2023-06-20T15:41:37Z

@alexanderbez I was checking this and pushed the fix for the tests. Maybe we need to add a test that shows that if we set gas limit higher than the max block gas then we get an invalid gas error (panic)

alexanderbez · 2023-06-20T15:52:58Z

@alexanderbez I was checking this and pushed the fix for the tests. Maybe we need to add a test that shows that if we set gas limit higher than the max block gas then we get an invalid gas error (panic)

I already have a test in setup_test.go. Any tx with a gas limit that exceeds the block gas limit, it'll fail CheckTx. Is that what you meant?

facundomedica · 2023-06-20T18:18:27Z

I already have a test in setup_test.go. Any tx with a gas limit that exceeds the block gas limit, it'll fail CheckTx. Is that what you meant?
Yes 👌👌

Co-authored-by: Facundo Medica <facundomedica@gmail.com> Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> (cherry picked from commit 75b4918)

…6628) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

julienrbrt · 2023-07-27T12:15:27Z

@Mergifyio backport release/v0.47.x

mergify · 2023-07-27T12:15:30Z

backport release/v0.47.x

✅ Backports have been created

#17161 fix: check tx gas limit against block gas limit (backport #16547) has been created for branch release/v0.47.x but encountered conflicts

Co-authored-by: Facundo Medica <facundomedica@gmail.com> Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> (cherry picked from commit 75b4918) # Conflicts: # CHANGELOG.md # baseapp/abci_test.go # baseapp/abci_utils.go # baseapp/block_gas_test.go # baseapp/utils_test.go # x/auth/ante/setup_test.go

…7161) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Devon Bear <itsdevbear@berachain.com> Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>

updates

b29ea13

github-actions bot added the C:x/auth label Jun 14, 2023

alexanderbez added 4 commits June 14, 2023 11:44

updates

9816ad5

updates

4ee99d9

updates

b2bd73f

updates

c83162f

alexanderbez marked this pull request as ready for review June 14, 2023 15:49

alexanderbez requested a review from a team as a code owner June 14, 2023 15:49

Merge branch 'main' into bez/check-tx-gas-block

e94dc4c

github-prbot requested review from a team, facundomedica and samricotta and removed request for a team June 14, 2023 15:49

alexanderbez added backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release and removed backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release labels Jun 14, 2023

alexanderbez requested a review from tac0turtle June 14, 2023 16:10

alexanderbez added 2 commits June 14, 2023 12:13

updates

f804f1e

Merge branch 'bez/check-tx-gas-block' of github.com:cosmos/cosmos-sdk…

87153cd

… into bez/check-tx-gas-block

tac0turtle reviewed Jun 15, 2023

View reviewed changes

alexanderbez and others added 6 commits June 15, 2023 09:57

updates

20fc493

updates

bbcdece

updates

ac72869

Merge branch 'main' into bez/check-tx-gas-block

cfb9caf

updates

e8837b7

Merge branch 'bez/check-tx-gas-block' of github.com:cosmos/cosmos-sdk…

2540103

… into bez/check-tx-gas-block

tac0turtle assigned facundomedica and tac0turtle Jun 16, 2023

tac0turtle reviewed Jun 16, 2023

View reviewed changes

facundomedica reviewed Jun 16, 2023

View reviewed changes

baseapp/abci_utils.go Outdated Show resolved Hide resolved

alexanderbez enabled auto-merge June 20, 2023 11:26

alexanderbez requested review from facundomedica and tac0turtle June 20, 2023 11:26

facundomedica approved these changes Jun 20, 2023

View reviewed changes

facundomedica requested changes Jun 20, 2023

View reviewed changes

baseapp/abci_utils.go Outdated Show resolved Hide resolved

alexanderbez and others added 4 commits June 20, 2023 11:34

updates

ff0fd01

Merge branch 'main' into bez/check-tx-gas-block

46f9c77

fix tests, might need to add an extra test that shows that if the tx …

5360e0f

…gas limit is higher than maxblockgas it panics

remove extra line

4ea9b70

alexanderbez requested a review from facundomedica June 20, 2023 16:40

Merge branch 'main' into bez/check-tx-gas-block

2dd1b26

facundomedica approved these changes Jun 20, 2023

View reviewed changes

alexanderbez added this pull request to the merge queue Jun 20, 2023

Merged via the queue into main with commit 75b4918 Jun 20, 2023
50 checks passed

alexanderbez deleted the bez/check-tx-gas-block branch June 20, 2023 18:41

mergify bot mentioned this pull request Jun 20, 2023

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

Merged

julienrbrt added a commit that referenced this pull request Jun 22, 2023

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

466d6e8

…6628) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

mergify bot mentioned this pull request Jul 27, 2023

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

Merged

linear bot mentioned this pull request Oct 16, 2023

chore(sdk): Bump cosmos sdk to v0.47.5 evmos/evmos#1869

Merged

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexanderbez commented Jun 14, 2023 •

edited

Loading

tac0turtle Jun 15, 2023

alexanderbez Jun 15, 2023 •

edited

Loading

tac0turtle left a comment

tac0turtle Jun 16, 2023

facundomedica commented Jun 20, 2023 •

edited

Loading

facundomedica commented Jun 20, 2023

alexanderbez commented Jun 20, 2023 •

edited

Loading

facundomedica commented Jun 20, 2023

julienrbrt commented Jul 27, 2023

mergify bot commented Jul 27, 2023 •

edited

Loading

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

		@@ -186,9 +191,15 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand
		return &abci.ResponsePrepareProposal{Txs: req.Txs}, nil

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

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

Conversation

alexanderbez commented Jun 14, 2023 • edited Loading

Description

Changelog

Author Checklist

Reviewers Checklist

tac0turtle Jun 15, 2023

Choose a reason for hiding this comment

alexanderbez Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

tac0turtle left a comment

Choose a reason for hiding this comment

tac0turtle Jun 16, 2023

Choose a reason for hiding this comment

facundomedica commented Jun 20, 2023 • edited Loading

facundomedica commented Jun 20, 2023

alexanderbez commented Jun 20, 2023 • edited Loading

facundomedica commented Jun 20, 2023

julienrbrt commented Jul 27, 2023

mergify bot commented Jul 27, 2023 • edited Loading

✅ Backports have been created

alexanderbez commented Jun 14, 2023 •

edited

Loading

alexanderbez Jun 15, 2023 •

edited

Loading

facundomedica commented Jun 20, 2023 •

edited

Loading

alexanderbez commented Jun 20, 2023 •

edited

Loading

mergify bot commented Jul 27, 2023 •

edited

Loading