feat: exceeding block gas meter don't fail #19318
Conversation
|
Warning Rate Limit Exceeded@yihuang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 27 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update introduces a mechanism to gracefully handle situations where the execution of a transaction exceeds the block gas limit within the baseapp. By adding a deferred function to recover from panics specifically caused by gas limit breaches, and adjusting the control flow in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // fails. Hence, it's execution is deferred. | ||
| consumeBlockGas := func() { | ||
| if !blockGasConsumed { | ||
| defer func() { | ||
| // log and ignore, see: https://github.com/cosmos/cosmos-sdk/issues/19317 | ||
| if err := recover(); err != nil { | ||
| ctx.Logger().Error("block gas limit exceeded", "err", err) |
There was a problem hiding this comment.
Change potentially affects state.
Call sequence:
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).runTx (baseapp/baseapp.go:814)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).deliverTx (baseapp/baseapp.go:744)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).internalFinalizeBlock (baseapp/baseapp.go:713)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/baseapp.go:874)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- baseapp/baseapp.go (1 hunks)
Additional comments: 1
baseapp/baseapp.go (1)
- 845-850: The implementation of the deferred function to recover from a panic caused by exceeding the block gas limit is correct. However, it's essential to ensure that this change does not inadvertently mask other types of panics that could occur in the
runTxmethod. Consider adding additional logging or metrics to monitor the frequency of these recoveries to assess the impact on the system's overall stability and performance.
CHANGELOG.md
Outdated
| @@ -73,6 +73,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * (crypto/keys) [#18026](https://github.com/cosmos/cosmos-sdk/pull/18026) Made public key generation constant time on `secp256k1` | |||
| * (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle. | |||
| * (types) [#18963](https://github.com/cosmos/cosmos-sdk/pull/18963) Swap out amino json encoding of `ABCIMessageLogs` for std lib json encoding | |||
| * (baseapp) [#]() Exceeding block gas meter don't make transaction fail. | |||
There was a problem hiding this comment.
The changelog entry for the modification in baseapp behavior regarding the block gas meter is missing a PR link. Including the PR link provides valuable context and allows readers to easily access more detailed information about the changes. Consider adding the PR link for consistency with other entries and to improve the documentation's utility.
There's a grammatical error in the changelog entry. The correct form should maintain proper subject-verb agreement. Consider revising the sentence for clarity and correctness.
- Exceeding block gas meter don't make transaction fail.
+ Exceeding the block gas meter does not cause transactions to fail.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * (baseapp) [#]() Exceeding block gas meter don't make transaction fail. | |
| * (baseapp) [#]() Exceeding the block gas meter does not cause transactions to fail. |
| if err := recover(); err != nil { | ||
| ctx.Logger().Error("block gas limit exceeded", "err", err) | ||
| } | ||
| }() | ||
| blockGasConsumed = true | ||
| ctx.BlockGasMeter().ConsumeGas( |
There was a problem hiding this comment.
Sorry, can you please explain again how this could panic, thus making recovery needed?
There was a problem hiding this comment.
It's related to unused gas refunding, if we include tx by user provided gas limit, they can fill block space with unused gas, the actually used gas is always lower than the block gas limit, we did some hack to include more txs, so we got exceeding block gas limit situation occasionally.
If we plan to remove block gas meter at all, maybe we can simply live with the situation.
Closes: #19317
Description
Closes: #XXXX
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit