fix(baseapp): Reset GasMeter before deliverTX #18714

Eric-Warehime · 2023-12-12T22:35:05Z

Description

Resets the BlockGasMeter before deliverTx is called to ensure the AnteHandlers aren't relied upon for that.

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...

included the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
targeted the correct branch (see PR Targeting)
provided a link to the relevant issue or specification
reviewed "Files changed" and left comments if necessary
included the necessary unit and integration tests
added a changelog entry to CHANGELOG.md
updated the relevant documentation or specification, including comments for documenting Go code
confirmed all CI checks have passed

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 all author checklist items have been addressed
reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

baseapp/abci.go

coderabbitai · 2023-12-12T22:36:46Z

Walkthrough

The changes address a bug in the Cosmos SDK's BaseApp related to gas consumption tracking during block execution. Specifically, the updates ensure that the blockGasMeter accurately reflects the gas used in BeginBlock, DeliverTx, and EndBlock, and that the gasMeter is correctly reset after BeginBlock to prevent incorrect attribution of gas usage to the first transaction's AnteHandler.

Changes

File Path	Change Summary
`CHANGELOG.md`	Bug fix related to gas consumption idiosyncrasies in `BeginBlock` and `DeliverTx` in the `baseapp`.
`baseapp/abci.go`	Changes in the `internalFinalizeBlock` function to reset the gas meter and update the context with the new gas meter for `finalizeBlockState` and `checkState`.
`baseapp/baseapp_test.go`	Modification of the `NewBaseAppSuiteWithSnapshots` function to include additional options and addition of the `TestAnteHandlerGasMeter` function to test the behavior of the gas meter within the AnteHandler.

Assessment against linked issues

Objective	Addressed	Explanation
Bug Description	✅
Simplest Fix	✅
Context	✅
Additional Information	✅

The code changes appear to address the key objectives outlined in the linked issue, ensuring that gas consumption is tracked and managed correctly within the Cosmos SDK's BaseApp during the block execution process.

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 X ?

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

You can reply to a review comment made by CodeRabbit.
You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

…ock-gas-usage

…ock-gas-usage' into ericw/18609-beginblock-gas-usage

alexanderbez · 2023-12-13T18:35:19Z

baseapp/abci.go

+	// Reset the gas meter so that the AnteHandlers aren't required to
+	gasMeter = app.getBlockGasMeter(app.finalizeBlockState.ctx)
+	app.finalizeBlockState.ctx = app.finalizeBlockState.ctx.WithBlockGasMeter(gasMeter)
+	app.checkState.ctx = app.checkState.ctx.WithBlockGasMeter(gasMeter)


sorry, why are we setting CheckTx state here?

Good call out, there's no need to modify the checkTx state's meter here.

alexanderbez · 2023-12-13T18:36:02Z

baseapp/abci.go

@@ -780,6 +780,11 @@ func (app *BaseApp) internalFinalizeBlock(ctx context.Context, req *abci.Request

 	events = append(events, beginBlock.Events...)

+	// Reset the gas meter so that the AnteHandlers aren't required to
+	gasMeter = app.getBlockGasMeter(app.finalizeBlockState.ctx)
+	app.finalizeBlockState.ctx = app.finalizeBlockState.ctx.WithBlockGasMeter(gasMeter)


CHANGELOG.md

…ock-gas-usage

baseapp/abci.go

facundomedica · 2023-12-15T14:42:25Z

LGTM but I have some questions to fully understand this. This means that we can also remove the gas meter resetting in the ante handlers? Why was this done in ante handlers in the first place?

alexanderbez · 2023-12-15T17:37:06Z

LGTM but I have some questions to fully understand this. This means that we can also remove the gas meter resetting in the ante handlers? Why was this done in ante handlers in the first place?

@facundomedica it is done in the AnteHandler, specifically the Setup decorator. However, this patch addresses two things:

Handling the case where a chain doesn't use the Setup decorator (not sure why they wouldn't but it has happened)
Addressing the fact that the 1st tx in a block may not be an sdk.Tx and so the ante-handler is never called for that 'data blob'.

julienrbrt · 2023-12-15T20:19:07Z

@alexanderbez is it a backportable fix?

Eric-Warehime · 2023-12-15T20:27:20Z

@alexanderbez is it a backportable fix?

It should be--we found this issue when using 0.47. But the fix is in a different place so I'm thinking it makes sense for me to open a different PR for the backport instead of trying to have mergify/automation do it. I have a branch with changes staged already on my fork.

julienrbrt · 2023-12-15T20:31:40Z

For v0.47 that makes sense not to use mergify. For v0.50, if we backport, that will be easier to use it.

julienrbrt · 2023-12-18T09:34:19Z

@Mergifyio backport release/v0.50.x

mergify · 2023-12-18T09:34:31Z

backport release/v0.50.x

✅ Backports have been created

#18774 fix(baseapp): Reset GasMeter before deliverTX (backport #18714) has been created for branch release/v0.50.x but encountered conflicts

(cherry picked from commit e7f5c2e) # Conflicts: # CHANGELOG.md

julienrbrt · 2023-12-19T10:53:23Z

@alexanderbez is it a backportable fix?

It should be--we found this issue when using 0.47. But the fix is in a different place so I'm thinking it makes sense for me to open a different PR for the backport instead of trying to have mergify/automation do it. I have a branch with changes staged already on my fork.

Are you willing to open a PR targeting v0.47?

alexanderbez · 2023-12-19T16:54:54Z

@alexanderbez is it a backportable fix?

It should be--we found this issue when using 0.47. But the fix is in a different place so I'm thinking it makes sense for me to open a different PR for the backport instead of trying to have mergify/automation do it. I have a branch with changes staged already on my fork.

Are you willing to open a PR targeting v0.47?

++ @Eric-Warehime 🙏

Eric-Warehime · 2023-12-19T17:58:30Z

@alexanderbez is it a backportable fix?

It should be--we found this issue when using 0.47. But the fix is in a different place so I'm thinking it makes sense for me to open a different PR for the backport instead of trying to have mergify/automation do it. I have a branch with changes staged already on my fork.

Are you willing to open a PR targeting v0.47?

++ @Eric-Warehime 🙏

Yes I can get it open probably in the next 24-48 hours.

julienrbrt · 2023-12-19T19:20:42Z

Yes I can get it open probably in the next 24-48 hours.

Cool, that's our only blocker for v0.47.7 ;)

Eric-Warehime · 2023-12-19T20:54:27Z

Opening 0.47.X backport in #18826

Eric-Warehime added 3 commits December 12, 2023 12:54

Add test for gas usage in AnteHandler

ee0afc0

Zero gas meter before deliverTx

bb866cc

Merge branch 'main' into ericw/18609-beginblock-gas-usage

f89461c

github-actions bot reviewed Dec 12, 2023

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

Eric-Warehime marked this pull request as ready for review December 12, 2023 22:36

Eric-Warehime requested a review from a team as a code owner December 12, 2023 22:36

julienrbrt changed the title ~~fix: Reset GasMeter before deliverTX~~ fix(baseapp): Reset GasMeter before deliverTX Dec 12, 2023

Eric-Warehime added 3 commits December 13, 2023 08:57

Lint fix

eceb320

Merge remote-tracking branch 'upstream/main' into ericw/18609-beginbl…

f5596e7

…ock-gas-usage

Merge remote-tracking branch 'refs/remotes/origin/ericw/18609-beginbl…

e4f3cbf

…ock-gas-usage' into ericw/18609-beginblock-gas-usage

alexanderbez reviewed Dec 13, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Eric-Warehime added 3 commits December 14, 2023 09:57

Don't modify checkState gasmeter

7604bf7

Update changelog message

047edaf

Merge remote-tracking branch 'upstream/main' into ericw/18609-beginbl…

4449e17

…ock-gas-usage

github-actions bot reviewed Dec 14, 2023

View reviewed changes

baseapp/abci.go Show resolved Hide resolved

Eric-Warehime requested a review from alexanderbez December 14, 2023 18:13

Merge branch 'main' into ericw/18609-beginblock-gas-usage

58fc856

alexanderbez approved these changes Dec 15, 2023

View reviewed changes

alexanderbez assigned alexanderbez and facundomedica Dec 15, 2023

facundomedica approved these changes Dec 15, 2023

View reviewed changes

facundomedica enabled auto-merge December 15, 2023 18:34

facundomedica added this pull request to the merge queue Dec 15, 2023

Merged via the queue into cosmos:main with commit e7f5c2e Dec 15, 2023
54 of 56 checks passed

julienrbrt mentioned this pull request Dec 18, 2023

chore: prepare v0.47.7 #18765

Merged

12 tasks

mergify bot pushed a commit that referenced this pull request Dec 18, 2023

fix(baseapp): Reset GasMeter before deliverTX (#18714)

c193dea

(cherry picked from commit e7f5c2e) # Conflicts: # CHANGELOG.md

mergify bot mentioned this pull request Dec 18, 2023

fix(baseapp): Reset GasMeter before deliverTX (backport #18714) #18774

Merged

julienrbrt pushed a commit that referenced this pull request Dec 18, 2023

fix(baseapp): Reset GasMeter before deliverTX (backport #18714) (#18774)

7a43e49

chen-robert mentioned this pull request Jan 8, 2024

[Bug]: Incomplete Patch #18714 #18978

Closed

1 task

faddat mentioned this pull request Nov 8, 2024

faddat/many times #22476

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(baseapp): Reset GasMeter before deliverTX #18714

fix(baseapp): Reset GasMeter before deliverTX #18714

Eric-Warehime commented Dec 12, 2023 •

edited

Loading

coderabbitai bot commented Dec 12, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

alexanderbez Dec 13, 2023

Eric-Warehime Dec 14, 2023

alexanderbez Dec 13, 2023

facundomedica commented Dec 15, 2023

alexanderbez commented Dec 15, 2023 •

edited

Loading

julienrbrt commented Dec 15, 2023

Eric-Warehime commented Dec 15, 2023

julienrbrt commented Dec 15, 2023

julienrbrt commented Dec 18, 2023

mergify bot commented Dec 18, 2023 •

edited

Loading

julienrbrt commented Dec 19, 2023 •

edited

Loading

alexanderbez commented Dec 19, 2023

Eric-Warehime commented Dec 19, 2023

julienrbrt commented Dec 19, 2023

Eric-Warehime commented Dec 19, 2023

fix(baseapp): Reset GasMeter before deliverTX #18714

fix(baseapp): Reset GasMeter before deliverTX #18714

Conversation

Eric-Warehime commented Dec 12, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

coderabbitai bot commented Dec 12, 2023 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

alexanderbez Dec 13, 2023

Choose a reason for hiding this comment

Eric-Warehime Dec 14, 2023

Choose a reason for hiding this comment

alexanderbez Dec 13, 2023

Choose a reason for hiding this comment

facundomedica commented Dec 15, 2023

alexanderbez commented Dec 15, 2023 • edited Loading

julienrbrt commented Dec 15, 2023

Eric-Warehime commented Dec 15, 2023

julienrbrt commented Dec 15, 2023

julienrbrt commented Dec 18, 2023

mergify bot commented Dec 18, 2023 • edited Loading

✅ Backports have been created

julienrbrt commented Dec 19, 2023 • edited Loading

alexanderbez commented Dec 19, 2023

Eric-Warehime commented Dec 19, 2023

julienrbrt commented Dec 19, 2023

Eric-Warehime commented Dec 19, 2023

Eric-Warehime commented Dec 12, 2023 •

edited

Loading

coderabbitai bot commented Dec 12, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

alexanderbez commented Dec 15, 2023 •

edited

Loading

mergify bot commented Dec 18, 2023 •

edited

Loading

julienrbrt commented Dec 19, 2023 •

edited

Loading