fix(baseapp): don't share global gas meter in tx execution #19616
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ref: cosmos#19613 Before the ante handler set the gas meter using tx gas-wanted, the gas meter in ctx remains a globally shared one, although it don't have bad effect right now, but it's a race condition when executing in parallel.
WalkthroughWalkthroughThe recent update focuses on enhancing the transaction context setup in the Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional Context UsedPath-based Instructions (1)
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
668
to
675
| panic(fmt.Sprintf("state is nil for mode %v", mode)) | ||
| } | ||
| ctx := modeState.Context(). | ||
| WithTxBytes(txBytes) | ||
| WithTxBytes(txBytes). | ||
| WithGasMeter(storetypes.NewInfiniteGasMeter()) | ||
| // WithVoteInfos(app.voteInfos) // TODO: identify if this is needed | ||
|
|
||
| ctx = ctx.WithIsSigverifyTx(app.sigverifyTx) |
There was a problem hiding this comment.
Change potentially affects state.
Call sequence:
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).getContextForTx (baseapp/baseapp.go:662)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).runTx (baseapp/baseapp.go:824)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).deliverTx (baseapp/baseapp.go:754)
(*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: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- baseapp/baseapp.go (1 hunks)
Additional comments: 1
baseapp/baseapp.go (1)
- 671-672: The addition of a dedicated gas meter for each transaction context is a crucial improvement for concurrency and race condition prevention. This change ensures that each transaction is processed with its own gas meter, eliminating the risk of shared state issues. The implementation appears correct and follows the described objectives. However, it's important to ensure that all other parts of the system that interact with the gas meter are aware of this change and handle the gas meter appropriately. Additionally, thorough testing should be conducted to verify that this change does not introduce any new issues, especially in concurrent transaction processing scenarios.
yihuang
changed the title
alexanderbez
approved these changes
Mar 1, 2024
|
Can we get a changelog please? |
sure, done |
2 tasks
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- baseapp/baseapp.go (1 hunks)
Check Runs (25)
tests (03) completed (1)
tests (02) completed (1)
tests (01) completed (1)
test-x-upgrade completed (1)
test-x-tx completed (1)
test-x-staking completed (1)
test-x-slashing completed (1)
test-x-params completed (1)
test-x-protocolpool completed (1)
test-x-nft completed (1)
test-x-mint completed (1)
test-x-group completed (1)
test-x-gov completed (1)
test-x-feegrant completed (1)
test-x-evidence completed (1)
test-x-distribution completed (1)
test-x-circuit completed (1)
test-x-bank completed (1)
test-x-authz completed (1)
test-x-auth completed (1)
test-x-accounts completed (1)
test-store completed (1)
test-sim-nondeterminism completed (1)
test-simapp completed (1)
test-orm completed (1)
Additional comments: 2
baseapp/baseapp.go (1)
- 669-670: The introduction of
storetypes.NewInfiniteGasMeter()in thegetContextForTxfunction ensures that each transaction context has its own dedicated gas meter. This change is crucial for preventing race conditions in parallel transaction execution by isolating the gas meter to the transaction level rather than sharing it globally. This approach aligns well with the PR's objective to improve transaction processing reliability in the Cosmos SDK.CHANGELOG.md (1)
- 97-97: The changelog entry for PR fix(baseapp): don't share global gas meter in tx execution #19616 is clear and concise. However, it might be beneficial for readers to have a bit more context about the issue. Consider expanding the description to briefly explain the problem of the shared gas meter and how this change resolves it, enhancing the understanding of the change's impact.
julienrbrt
reviewed
Mar 19, 2024
| @@ -666,7 +666,8 @@ func (app *BaseApp) getContextForTx(mode execMode, txBytes []byte) sdk.Context { | |||
| panic(fmt.Sprintf("state is nil for mode %v", mode)) | |||
| } | |||
| ctx := modeState.Context(). | |||
| WithTxBytes(txBytes) | |||
| WithTxBytes(txBytes). | |||
| WithGasMeter(storetypes.NewInfiniteGasMeter()) | |||
There was a problem hiding this comment.
I see you've added the backport tag, but is this really backportable?
There was a problem hiding this comment.
I guess so, the gas meter will be replaced by ante handler, so whatever recorded in the initial gas meter is discarded there.
There was a problem hiding this comment.
The root gas meter is an infinite gas meter. This change solves the race as the root gas meter is not shared anymore with the new context. Looks good for me
julienrbrt
changed the title
julienrbrt
reviewed
Mar 19, 2024
yihuang
commented
Mar 19, 2024
sontrinh16
approved these changes
Apr 15, 2024
| @@ -116,6 +116,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * (crypto) [#20027](https://github.com/cosmos/cosmos-sdk/pull/20027) secp256r1 keys now implement gogoproto's customtype interface. | |||
| * (x/bank) [#20028](https://github.com/cosmos/cosmos-sdk/pull/20028) Align query with multi denoms for send-enabled. | |||
| * (cli) [#20020](https://github.com/cosmos/cosmos-sdk/pull/20020) Make bootstrap-state command support both new and legacy genesis format. | |||
| * (baseapp) [#19616](https://github.com/cosmos/cosmos-sdk/pull/19616) Don't share gas meter in tx execution. | |||
There was a problem hiding this comment.
The changelog entry clearly documents the change made in PR #19616. Consider rephrasing for enhanced clarity:
- * (baseapp) [#19616](https://github.com/cosmos/cosmos-sdk/pull/19616) Don't share gas meter in tx execution.
+ * (baseapp) [#19616](https://github.com/cosmos/cosmos-sdk/pull/19616) Ensure each transaction has its own gas meter to prevent race conditions.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.
Suggested change
| * (baseapp) [#19616](https://github.com/cosmos/cosmos-sdk/pull/19616) Don't share gas meter in tx execution. | |
| * (baseapp) [#19616](https://github.com/cosmos/cosmos-sdk/pull/19616) Ensure each transaction has its own gas meter to prevent race conditions. |
alpe
approved these changes
Apr 15, 2024
| @@ -666,7 +666,8 @@ func (app *BaseApp) getContextForTx(mode execMode, txBytes []byte) sdk.Context { | |||
| panic(fmt.Sprintf("state is nil for mode %v", mode)) | |||
| } | |||
| ctx := modeState.Context(). | |||
| WithTxBytes(txBytes) | |||
| WithTxBytes(txBytes). | |||
| WithGasMeter(storetypes.NewInfiniteGasMeter()) | |||
There was a problem hiding this comment.
The root gas meter is an infinite gas meter. This change solves the race as the root gas meter is not shared anymore with the new context. Looks good for me
mergify bot
pushed a commit
that referenced
this pull request
Apr 15, 2024
(cherry picked from commit 4ed8c84) # Conflicts: # CHANGELOG.md
12 tasks
julienrbrt
pushed a commit
that referenced
this pull request
Apr 16, 2024
alpe
added a commit
to alpe/cosmos-sdk
that referenced
this pull request
Apr 16, 2024
* main: refactor(x/simulation)!: remove accounts string (cosmos#20056) fix(baseapp): don't share global gas meter in tx execution (cosmos#19616) feat(x/accounts): use router service from env (cosmos#20003) refactor(x): remove Address.String() (cosmos#20048) build(deps): Bump github.com/jhump/protoreflect from 1.15.6 to 1.16.0 in /tests (cosmos#20040) feat(collections): add `NewJSONValueCodec` (cosmos#19861)
alpe
added a commit
that referenced
this pull request
Apr 17, 2024
* main: build(deps): Bump github.com/pelletier/go-toml/v2 from 2.2.0 to 2.2.1 in /tools/confix (#20052) build(deps): Bump cosmossdk.io/api from 0.7.3 to 0.7.4 (#20063) fix: secp256r1 json missing quotes (#20060) fix(x/accounts): remove double execute (#20065) refactor(x/accounts): Skip Importing Unregistered Genesis Account Types (#20053) refactor(x/simulation)!: remove accounts string (#20056) fix(baseapp): don't share global gas meter in tx execution (#19616) feat(x/accounts): use router service from env (#20003) refactor(x): remove Address.String() (#20048)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref: #19613
Before the ante handler set the gas meter using tx gas-wanted, the gas meter in ctx remains a globally shared one, although it don't have bad effect right now, but it's a race condition when executing txs in parallel.
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
Bug Fixes
Documentation