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

capability module increments GasMeter non-deterministically across validators #15015

Closed
prettymuchbryce opened this issue Feb 13, 2023 · 3 comments · Fixed by #15030
Closed

Comments

@prettymuchbryce
Copy link
Contributor

Summary of Bug

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, should we consider adding WithGasMeter to the noGasCtx here?

Version

1e8e923

@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Feb 13, 2023
@alexanderbez
Copy link
Contributor

@prettymuchbryce ty for an excellent writeup. Yes, as you've pointed out we do make this (unforunate) assumption re the GasMeter being set in the AnteHandler stack.

I think your solution of setting the GasMeter on the noGasCtx is sufficient.

Out of curiosity, why do you not set it in your AnteHandler stack?

@alexanderbez alexanderbez self-assigned this Feb 13, 2023
@tac0turtle tac0turtle added C:x/capability C: gas and removed needs-triage Issue that needs to be triaged labels Feb 13, 2023
@prettymuchbryce
Copy link
Contributor Author

Out of curiosity, why do you not set it in your AnteHandler stack?

We mistakenly omitted calling the NewSetUpContextDecorator() from certain transactions due to a bug, and then ran into this issue. 🤦‍♂️

I can't think of a valid reason that an application would want to intentionally omit setting the GasMeter from the AnteHandler stack. Still, because the SDK allows applications to customize this stack, it seems reasonable that even an application with an empty AnteHandler stack should not run into Consensus failures.

I can help with a PR for the change.

@alexanderbez
Copy link
Contributor

Awesome, let's go for it @prettymuchbryce. I'll be happy to review and 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 a pull request may close this issue.

3 participants