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

ApplyFuncIfNoErr: catch out of gas panics, write gas amount anyway #2487

Closed
2 tasks
Tracked by #4170
ValarDragon opened this issue Aug 22, 2022 · 2 comments · Fixed by #3746
Closed
2 tasks
Tracked by #4170

ApplyFuncIfNoErr: catch out of gas panics, write gas amount anyway #2487

ValarDragon opened this issue Aug 22, 2022 · 2 comments · Fixed by #3746

Comments

@ValarDragon
Copy link
Member

Background

Right now if a tx out of gas panics during an ApplyFuncIfNoErr block, we log a spooky looking message, and don't correctly write the out of gas up, to ensure the tx gets fully reverted. In concrete instantiations today, due to superfluid, we are safe. But this should be addressed to make the API more generally safe, and to remove the ominous logs.

Suggested Design

https://github.com/osmosis-labs/osmosis/blob/main/osmoutils/cache_ctx.go#L16-L34

  • Find a reliable way to detect if a panic is an out of gas (we may need to edit SDK fork ante handler, or fork our own gas handler -- tbh SDK out of gas handling is pretty insane)
  • If out of gas, panic catch, then out of gas panic again. Write a test that this gets caught as a normal out of gas panic would get caught in a tx
@08d2
Copy link

08d2 commented Aug 22, 2022

It's insane that running out of gas triggers a panic instead of returning an error. Does that happen in the SDK? Is it infeasible to fix it there?

@ValarDragon
Copy link
Member Author

ValarDragon commented Aug 22, 2022

Yeah it happens in the SDK. In general the SDK's guarantees around gas are pretty scant, so its a huge pain to fix things like this. The decision is also fairly complex, because the SDK framework only has the opportunity at the end of a message exec to cleanly alter execution flow, without a panic to abort the flow. (This is unlike with a VM interpreter, where you can just halt at the next instruction. The base 'instruction' unit of the SDK is poorly documented, but is the sdk.Msg)

I don't want to derail this issue too much with the SDK's design to panic for gas control flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
2 participants