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

Make apply func if no err, out of gas panic, match std behavior #3746

Merged
merged 5 commits into from
Feb 1, 2023

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Dec 16, 2022

Closes: #2487

What is the purpose of the change

So it turns out that CacheCtx and Ctx bizarrely share the same gas meter. Source: https://github.com/cosmos/cosmos-sdk/blob/main/types/context.go#L289-L303 , and that GasMeter is a pointer. (Otherwise https://github.com/cosmos/cosmos-sdk/blob/main/types/context.go#L58 ctx,GasMeter().ConsumeGas() wouldn't work)

So this PR makes it so that if in apply func if no err block fails due to out of gas, it preserves the standard out of gas behavior flow. This is not an issue for beginblock / endblock code as they should not get panics for out of gas issues. Whereas for txs, right now you get a different error code / distinct flow right now.

This ideally would be handled upstream, but alas.

This is state breaking, as it effects tx execution codes.

Brief Changelog

  • #3746 Make ApplyFuncIfNoErr logic preserve panics for OutOfGas behavior.

Testing and Verifying

This change added a test which shows:

  • When gas limit not hit, its being updated correctly
  • When we out of gas panic, we are:
    • still panicing with the expected value
    • underlying gas meter still updated

The QA requirement for this will be to test a superfluid delegate action with and without this change, in the out of gas behavior setting. (Namely that they should both return an error, just a different error code)

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? Code comment

@ValarDragon ValarDragon added the V:state/breaking State machine breaking PR label Dec 16, 2022
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Dec 31, 2022
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +48 to +53
func IsOutOfGasError(err any) (bool, string) {
switch e := err.(type) {
case types.ErrorOutOfGas:
return true, e.Descriptor
case types.ErrorGasOverflow:
return true, e.Descriptor
Copy link
Member

Choose a reason for hiding this comment

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

What about types.ErrorNegativeGasConsumed?

Copy link
Member Author

@ValarDragon ValarDragon Jan 3, 2023

Choose a reason for hiding this comment

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

oh wow didn't know that existed, let me think about it

great catch

@github-actions github-actions bot removed the Stale label Jan 3, 2023
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

LGTM once @czarcas7ic 's suggestion of negative error has been added!

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Jan 29, 2023
@ValarDragon ValarDragon merged commit 62757d3 into main Feb 1, 2023
@ValarDragon ValarDragon deleted the dev/improve_applyfuncifnoerr_out_of_gas_logging branch February 1, 2023 08:04
@ValarDragon ValarDragon mentioned this pull request Feb 1, 2023
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale V:state/breaking State machine breaking PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ApplyFuncIfNoErr: catch out of gas panics, write gas amount anyway
3 participants