feat(baseapp): add option to disable block gas meter #19626
Conversation
WalkthroughThe recent changes involve updates to the Changes
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 (
|
yihuang
added
the
backport/v0.50.x
![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
Commits
Files that changed from the base of the PR and between 7628592 and 2e8c86649404d5a4cac344128388716f44072512.Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- baseapp/baseapp.go (2 hunks)
- baseapp/noopgasmeter.go (1 hunks)
- baseapp/options.go (2 hunks)
Additional comments: 4
baseapp/noopgasmeter.go (1)
- 5-17: The implementation of
noopGasMetercorrectly adheres to the no-op behavior expected from a gas meter that does not perform any operation. This ensures that when the block gas meter is disabled, transactions can execute without gas metering interference.baseapp/options.go (2)
- 122-124: The
DisableBlockGasMeterfunction is correctly implemented, providing a straightforward way for developers to configure their applications to disable the block gas meter if desired. This enhances the flexibility and safety of transaction execution within the Cosmos SDK framework.- 395-396: The
SetDisableBlockGasMetermethod is correctly implemented, allowing for thedisableBlockGasMeterflag to be set on theBaseAppstruct. This method is essential for the functionality introduced by the PR, enabling the block gas meter to be disabled as needed.baseapp/baseapp.go (1)
- 661-664: The modification to the
getBlockGasMetermethod to return anoopGasMeterwhendisableBlockGasMeteris true is a direct implementation of the feature to disable block gas metering. This change is logically sound and aligns with the objective of the feature. However, it's important to ensure that thenoopGasMeterimplementation correctly adheres to theGasMeterinterface without any unintended side effects, especially since it will be used in place of the standard gas meter when the feature is enabled.Ensure that the
noopGasMeterimplementation is thoroughly tested, particularly its interaction with other components that expect a functioning gas meter. This includes testing scenarios where transactions are executed in parallel to verify that disabling the block gas meter does not introduce any inconsistencies or errors in transaction processing.
baseapp/baseapp.go
Outdated
| // disableBlockGasMeter will disable the block gas meter if true, block gas meter is tricky to support | ||
| // when executing transactions in parallel. | ||
| // when disabled, the block gas meter in context is `nil`. | ||
| // | ||
| // SAFTY: it's safe to do if validators validates the total gas wanted in the proposal. | ||
| disableBlockGasMeter bool |
There was a problem hiding this comment.
The addition of the disableBlockGasMeter field to the BaseApp struct introduces the capability to disable the block gas meter. This change is crucial for supporting parallel transaction execution without the complications of block gas metering. However, it's important to ensure that this feature is well-documented, especially the safety aspects mentioned in the comment. The comment mentions that it's safe if validators validate the total gas wanted in the proposal, which implies a reliance on external validation mechanisms. This should be clearly documented in the official documentation to avoid misuse of the feature.
Consider enhancing the documentation around this feature, both in code comments and the official Cosmos SDK documentation, to clearly explain its usage, the conditions under which it is safe to disable the block gas meter, and the responsibilities of validators in this context.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between 2e8c86649404d5a4cac344128388716f44072512 and 0a839baab5d443bee73cee7b6ff3e425568e4b05.Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
I understand that the block gas meter is causing issues, but I mentioned in a similar PR there really is no need for the block gas meter -- it serves no purpose at all.
Now given that we don't want to break APIs in 0.50.x, this seems like a decent patch for 0.50.x
maybe there are a few extreme cases, like using a noop process proposal hander, I agree most of chains wouldn't care.
we can direct this PR to 0.50.x only, or merge it into main and backport to 0.50.x first, then on top of it, we can still merge your PR that remove it completely if we decided to do that, WDYT? |
|
@yihuang let's target this on 0.50.x release line please. WE can merge from there |
ref: cosmos#19613 block gas meter is not safe when executing transactions in parallel.
yihuang
force-pushed
the
optional-block-gas-meter
branch
from
05f44cb to
4bcd297
Compare
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:Keys
github-actions
bot
removed
C:x/nft
C:orm
Type: Build
Type: ADR
Type: CI
C:x/consensus
C:collections
C:Confix
done |
RELEASE_NOTES.md
Outdated
|
|
||
| 💬 [**Release Discussion**](https://github.com/orgs/cosmos/discussions/58) | ||
|
|
||
| ## 🚀 Highlights | ||
|
|
||
| While early in our monthly release cadence, this release includes a few notable fixes: | ||
| Some months ago Cosmos SDK Eden was released. Missed the announcement? Read it [here](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1). |
There was a problem hiding this comment.
Consider adding a comma after "Some months ago" for better readability: "Some months ago, Cosmos SDK Eden was released."
- Some months ago Cosmos SDK Eden was released.
+ Some months ago, Cosmos SDK Eden was released.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.
| Some months ago Cosmos SDK Eden was released. Missed the announcement? Read it [here](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1). | |
| Some months ago, Cosmos SDK Eden was released. Missed the announcement? Read it [here](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1). |
For clarity, consider rephrasing the sentence to include a comma after the introductory phrase: "For this month, the patch release of the v0.50.x line includes a few features and improvements."
- For this month patch release of the v0.50.x line, a few features and improvements were added to the SDK.
+ For this month, the patch release of the v0.50.x line includes a few features and improvements.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.
| Some months ago Cosmos SDK Eden was released. Missed the announcement? Read it [here](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1). | |
| Some months ago Cosmos SDK Eden was released. Missed the announcement? Read it [here](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1). |
RELEASE_NOTES.md
Outdated
|
|
||
| * Adds in-place testnet CLI command for creating testnets from local state (kudos to @czarcas7ic) | ||
| * Multiple fixes in baseapp, with fixes in `DefaultProposalHandler` and vote extensions | ||
| * Add a missed check in `x/auth/vesting`: [GHSA-4j93-fm92-rp4m](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-4j93-fm92-rp4m) | ||
|
|
||
| We recommended to upgrade to this patch release as soon as possible. |
There was a problem hiding this comment.
To improve clarity, consider rephrasing to: "We recommend upgrading to this patch release as soon as possible."
- We recommended to upgrade to this patch release as soon as possible.
+ We recommend upgrading to this patch release as soon as possible.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.
| We recommended to upgrade to this patch release as soon as possible. | |
| We recommend upgrading to this patch release as soon as possible. |
RELEASE_NOTES.md
Outdated
|
|
||
| We recommended to upgrade to this patch release as soon as possible. | ||
| When upgrading from <= v0.50.3, please ensure that 2/3 of the validator power upgrade to v0.50.4. | ||
|
|
||
| ## 📝 Changelog | ||
|
|
||
| Check out the [changelog](https://github.com/cosmos/cosmos-sdk/blob/v0.50.5/CHANGELOG.md) for an exhaustive list of changes, or [compare changes](https://github.com/cosmos/cosmos-sdk/compare/release/v0.50.4...v0.50.5) from the last release. | ||
| Check out the [changelog](https://github.com/cosmos/cosmos-sdk/blob/v0.50.4/CHANGELOG.md) for an exhaustive list of changes, or [compare changes](https://github.com/cosmos/cosmos-sdk/compare/release/v0.50.3...v0.50.4) from the last release. | ||
|
|
||
| Refer to the [upgrading guide](https://github.com/cosmos/cosmos-sdk/blob/release/v0.50.x/UPGRADING.md) when migrating from `v0.47.x` to `v0.50.1`. | ||
| Note, that the next SDK release, v0.51.0, will not include `x/params` migration, when migrating from < v0.47, v0.50.x **or** v0.47.x, is a mandatory migration. |
There was a problem hiding this comment.
To clarify the migration requirements, consider rephrasing: "Note that the next SDK release, v0.51.0, will not include the x/params migration. When migrating from versions earlier than v0.47, including v0.50.x or v0.47.x, this migration is mandatory."
- Note, that the next SDK release, v0.51.0, will not include `x/params` migration, when migrating from < v0.47, v0.50.x **or** v0.47.x, is a mandatory migration.
+ Note that the next SDK release, v0.51.0, will not include the `x/params` migration. When migrating from versions earlier than v0.47, including v0.50.x or v0.47.x, this migration is mandatory.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.
| Note, that the next SDK release, v0.51.0, will not include `x/params` migration, when migrating from < v0.47, v0.50.x **or** v0.47.x, is a mandatory migration. | |
| Note that the next SDK release, v0.51.0, will not include the `x/params` migration. When migrating from versions earlier than v0.47, including v0.50.x or v0.47.x, this migration is mandatory. |
| @@ -38,6 +38,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Features | |||
|
|
|||
| * (baseapp) [#19626](https://github.com/cosmos/cosmos-sdk/pull/19626) Adds `DisableBlockGasMeter` option to `BaseApp`, which removes the block gas meter during transaction execution. | |||
There was a problem hiding this comment.
this will get in v0.50.5, can you move it below?
Agreed with this take personally, we need to make sure we've removed it in v0.51 on baseapp or backport this then. |
julienrbrt
changed the title
…0.50.5 * fix: in-place-testnet edgecases (backport cosmos#19516) (cosmos#19526) Co-authored-by: Adam Tucker <[email protected]> * fix(simapp): typo in GetStoreKeys (cosmos#19544) * build(deps): Bump cosmossdk.io/math from 1.2.0 to 1.3.0 (cosmos#19562) * fix(depinject): Authtx was not accepting custom signers (backport cosmos#19549) (cosmos#19551) Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Qt <[email protected]> Co-authored-by: Julien Robert <[email protected]> * build(deps): Bump github.com/cosmos/cosmos-db from 1.0.0 to 1.0.2 (cosmos#19566) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Julien Robert <[email protected]> * Merge pull request from GHSA-86h5-xcpx-cfqc * fix slashing hole with test * release notes + changelog * word * date * updates --------- Co-authored-by: Julien Robert <[email protected]> * ci: run test pipeline on merge v0.50 branch (cosmos#19582) * fix(staking): fix impossible conditions (cosmos#19621) * docs: add section on creating a testnets from mainnet exports (backport cosmos#19475) (cosmos#19648) Co-authored-by: Marko <[email protected]> * build(deps): Bump cosmossdk.io/x/tx from 0.13.0 to 0.13.1 (cosmos#19665) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/v2): marshal enum as string (cosmos#19653) * refactor(x/auth): allow empty public keys for GetSignBytesAdapter (backport cosmos#19651) (cosmos#19675) Co-authored-by: mmsqe <[email protected]> Co-authored-by: Julien Robert <[email protected]> * fix(types): check for HasABCIGenesis in CoreAppModuleBasicAdaptor (cosmos#19709) * build(deps): Bump deps (backport cosmos#19655) (cosmos#19711) Co-authored-by: Julien Robert <[email protected]> * Merge pull request from GHSA-95rx-m9m5-m94v * validate ExtendedCommit against LastCommit test cases * account for core.comet types * logging * linting * cherry-pick staking fix * nits * linting fix * run tests --------- Co-authored-by: Marko <[email protected]> Co-authored-by: Marko Baricevic <[email protected]> * feat(baseapp): add option to disable block gas meter (cosmos#19626) * feat(x/distribution): add rewards-by-validator autocli config (backport cosmos#19707) (cosmos#19714) Co-authored-by: Julien Robert <[email protected]> * fix(x/gov): grpc query tally for failed proposal (backport cosmos#19725) (cosmos#19727) Co-authored-by: David Tumcharoen <[email protected]> Co-authored-by: Julien Robert <[email protected]> * chore: prepare v0.50.5 (cosmos#19715) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: yihuang <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Qt <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: khanh <[email protected]> Co-authored-by: Tom <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: mmsqe <[email protected]> Co-authored-by: Nikhil Vasan <[email protected]> Co-authored-by: Marko Baricevic <[email protected]> Co-authored-by: David Tumcharoen <[email protected]>
ref: #19613
reasons
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
Summary by CodeRabbit
flagsin various test files to improve code organization and readability.