Skip to content
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: x/capability InitMemStore should not consume gas #15030

Conversation

prettymuchbryce
Copy link
Contributor

@prettymuchbryce prettymuchbryce commented Feb 14, 2023

Description

Closes: #15015

The capability module's InitMemStore function increments gasMeter non-deterministically across validators.

When a validator is restarted, InitMemStore is called during BeginBlock of the next committed block. This function performs some store operations which increment GasUsed on the GasMeter here.

So long as SetGasMeter is always called as part of the AnteHandler chain (which is the current behavior of the default AnteHandler chain here), then this is not an issue as the GasMeter will be reset between BeginBlock and the first DeliverTx call.

However, if the application uses a custom AnteHandler chain that omits this behavior, the GasUsed within each transaction will vary, as the GasMeter will be the initial meter set during BeginBlock here which increments gas differently if the capability module's MemStore has not yet been initialized.

As a solution, this PR adds WithGasMeter to the noGasCtx here which ensures that InitMemStore consumes no gas from GasMeter.


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
  • added ! to 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
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • 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 ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@prettymuchbryce prettymuchbryce marked this pull request as ready for review February 14, 2023 19:39
@prettymuchbryce prettymuchbryce requested a review from a team as a code owner February 14, 2023 19:39
@github-prbot github-prbot requested review from a team, tac0turtle and julienrbrt and removed request for a team February 14, 2023 19:40
@prettymuchbryce
Copy link
Contributor Author

I am not sure what is going on with the Lint job in CI, but the failures look unrelated to my changes.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Thanks @prettymuchbryce!

Yeah, you can ignore the lint stuff for now...not sure what's up with that.

@alexanderbez alexanderbez enabled auto-merge (squash) February 14, 2023 21:27
Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, a nit


suite.Require().Equal(prevBlockGas, blockGasUsed, "beginblocker consumed block gas during execution")
Copy link
Contributor

@atheeshp atheeshp Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
suite.Require().Equal(prevBlockGas, blockGasUsed, "beginblocker consumed block gas during execution")
suite.Require().Equal(prevBlockGas, blockGasUsed, "ensure beginblocker consume no block gas during execution")

can you update the below check's description as well.

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@amaury1093
Copy link
Contributor

@prettymuchbryce could you merge/rebase main so that our bot automerges this PR? Or altnertively give write access to maintainers

auto-merge was automatically disabled February 15, 2023 15:11

Head branch was pushed to by a user without write access

@prettymuchbryce prettymuchbryce force-pushed the prettymuchbryce/capability-infinite-gas-meter branch from 0a6c7ae to ca7706b Compare February 15, 2023 15:12
@prettymuchbryce
Copy link
Contributor Author

prettymuchbryce commented Feb 15, 2023

@AmauryM I rebased onto main, but it looks like this disabled auto-merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

capability module increments GasMeter non-deterministically across validators
7 participants