-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: check tx gas limit against block gas limit #16547
Conversation
… into bez/check-tx-gas-block
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think this fixes the issue at hand, the mempool could still put in an amount of txs that is larger the maxgas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -186,9 +191,15 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand | |||
return &abci.ResponsePrepareProposal{Txs: req.Txs}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a testcase that covers this.
@facundomedica I'll improve readability 👍 |
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 |
…gas limit is higher than maxblockgas it panics
@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 |
|
Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Facundo Medica <[email protected]> (cherry picked from commit 75b4918)
…6628) Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Julien Robert <[email protected]>
@Mergifyio backport release/v0.47.x |
✅ Backports have been created
|
Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Facundo Medica <[email protected]> (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 <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Facundo Medica <[email protected]>
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
Setup
ante decorator to account for block max gasPrepareProposal
handler to account for block max gasProcessProposal
handler to account for block max gasAuthor 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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change