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: catch the out of gas exception in preprocess #2638

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

piux2
Copy link
Contributor

@piux2 piux2 commented Jul 28, 2024

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 28, 2024
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 60.06%. Comparing base (0e3c050) to head (8c22541).
Report is 168 commits behind head on master.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/preprocess.go 0.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2638      +/-   ##
==========================================
+ Coverage   54.99%   60.06%   +5.06%     
==========================================
  Files         595      560      -35     
  Lines       79775    75825    -3950     
==========================================
+ Hits        43872    45542    +1670     
+ Misses      32581    26834    -5747     
- Partials     3322     3449     +127     
Flag Coverage Δ
contribs/gnodev 60.58% <ø> (+34.58%) ⬆️
contribs/gnofaucet 14.46% <ø> (ø)
gno.land 64.18% <ø> (+0.02%) ⬆️
gnovm 64.12% <0.00%> (+3.91%) ⬆️
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (+2.50%) ⬆️
tm2 62.07% <ø> (+7.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Conceptually the same as this PR

Do you think it could make sense to swap with using errors.As, as I suggested here?

Also, do you have a testcase that can show what happens when there's an out-of-gas in preprocess/predefine? (that's what we were missing on the other PR)

@piux2
Copy link
Contributor Author

piux2 commented Jul 31, 2024

Conceptually the same as this PR

Yes, Same. This PR catch all places that could throw Out Of Gas Exception in preprocess.

Do you think it could make sense to swap with using errors.As, as I suggested here?

Using return errors in preprocess will change many signatures and also involves catching errors in recursive calls during transcribe. It will be easier to make mistakes in future changes not handling errors or shadowing error in closures and switch case block. Recover and throwing panic(r) is a clearer and simpler approach.

I would prefer panic everywhere with exceptions in the VM itself and catch the panics outside of VM and process them as errors in Keeper.

In addition, to handle it properly, an OutOfGas exception should be thrown all the way to the ante handler and base app where the gas meter was originally set.

Also, do you have a testcase that can show what happens when there's an out-of-gas in preprocess/predefine? (that's what we were missing on the other PR)

I will add testing cases.

@thehowl
Copy link
Member

thehowl commented Jul 31, 2024

Using return errors in preprocess will change many signatures and also involves catching errors in recursive calls during transcribe. It will be easier to make mistakes in future changes not handling errors or shadowing error in closures and switch case block. Recover and throwing panic(r) is a clearer and simpler approach.

I'm not suggesting we returns error. Simply, that we are panic'ing with an error type; the value returned by recover() in the keeper will be of the same type too. So we can use errors.As instead -- thus we can special-case how we handle OutOfGasException in the keeper rather than in the VM.

The advantage I see out of this is that we don't have to think to add a condition to not wrap the error any other time we add a recover in vm. (They should happen seldom; but anyway I think the responsibility to do this kind of handling relies in the keeper).

@piux2 piux2 requested review from zivkovicmilos and a team as code owners August 3, 2024 08:20
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Aug 3, 2024
@piux2
Copy link
Contributor Author

piux2 commented Aug 3, 2024

I found right gas amount to replicate the errors that PR2368 could not reproduce and added proper integration test for that. Both store.GetPackage and store.GetTypeSafe could cause out-of-gas exceptions during preprocess.

I'm not suggesting we returns error. Simply, that we are panic'ing with an error type; the value returned by recover() in the keeper will be of the same type too. So we can use errors.As instead

using error.As() adds additional unnecessary abstraction.

We do use different exception types for VM panics. OutOfGasException and PreprocessError are the examples. To use errors.As(), we not only have to wrap exceptions with errors in VM, but also when we recover from a panic in Keeper, instead of type switching on r.(type), directly, we need to use r.(type) checking for error first and if else errors.As(r, &Exception) for the r=recover(). On the other hand, type switching exception types on recover is cleaner and faster than type checking error with if-else blocks.

we can special-case how we handle OutOfGasException in the keeper rather than in the VM.
I'm not suggesting we returns error. Simply, that we are panic'ing with an error type; the value returned by recover() in the keeper will be of the same type too. So we can use errors.As instead -- thus we can special-case how we handle OutOfGasException in the keeper rather than in the VM.

It only works if we do not recover any panic and wrap additional information on the call path to the Keeper. However, this is not the case. Currently, we recover from panic, wrap the stack trace, and preprocess trace in PreprocessError. Therefore, we need to differentiate the OutOfGas execution in the VM. This is actually the root cause of the issue we are fixing.

@piux2 piux2 changed the title chores: catch the out of gas exception in preprocess fix: catch the out of gas exception in preprocess Aug 3, 2024
@MikaelVallenet
Copy link
Contributor

Hello, I wanted to report the issue, then I saw that the problem was already being fixed, to check, I cloned the fix branch, install gno from this branch and I have the impression of still having the problem, or is it something else? @piux2

https://www.loom.com/share/4c507cb56f0e45b6bec819fdd85c6152?sid=558ebe55-0cb8-4758-ac76-1fe87584d69b

@piux2
Copy link
Contributor Author

piux2 commented Aug 14, 2024

Hello, I wanted to report the issue, then I saw that the problem was already being fixed, to check, I cloned the fix branch, install gno from this branch and I have the impression of still having the problem, or is it something else? @piux2

https://www.loom.com/share/4c507cb56f0e45b6bec819fdd85c6152?sid=558ebe55-0cb8-4758-ac76-1fe87584d69b

@MikaelVallenet thank you for the feedback.

This is a normal case and not an issue. The chain checks the size of the transaction and consumes gas based on that size. The transaction runs out of gas before it reaches the VM.

newCtx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*store.Gas(len(newCtx.TxBytes())), "txSize")

For the issue we are trying to solve in this PR, the out-of-gas event happens during preprocessing. Before the fix, it returned the stack trace and preprocess stack. After the fix, it returns a normal out-of-gas message and stack trace, as seen in your video clip.

The issue is tricky to reproduce. Only a specific gas amount will be able to trigger it. You can try it out in the newly added txtar file and compare the results before and after the fix.

@MikaelVallenet
Copy link
Contributor

Hello, I wanted to report the issue, then I saw that the problem was already being fixed, to check, I cloned the fix branch, install gno from this branch and I have the impression of still having the problem, or is it something else? @piux2
https://www.loom.com/share/4c507cb56f0e45b6bec819fdd85c6152?sid=558ebe55-0cb8-4758-ac76-1fe87584d69b

@MikaelVallenet thank you for the feedback.

This is a normal case and not an issue. The chain checks the size of the transaction and consumes gas based on that size. The transaction runs out of gas before it reaches the VM.

newCtx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*store.Gas(len(newCtx.TxBytes())), "txSize")

For the issue we are trying to solve in this PR, the out-of-gas event happens during preprocessing. Before the fix, it returned the stack trace and preprocess stack. After the fix, it returns a normal out-of-gas message and stack trace, as seen in your video clip.

The issue is tricky to reproduce. Only a specific gas amount will be able to trigger it. You can try it out in the newly added txtar file and compare the results before and after the fix.

Hello, thanks for information but i mean not the first error but the second one around 0:55 of my video (cf. screenshot below)

image

I think it should print the out of gas exception, isn't ?

@piux2
Copy link
Contributor Author

piux2 commented Oct 4, 2024

@MikaelVallenet

I think it should print the out of gas exception, isn't ?

Have you checked if you ran the test against the correct version of the chain?

I noticed you only ran make install. Did you start the chain with the patch?

Also, here is the stack trace from the video at 1:04.

image

At line numbers preprocess.go:137 and preprocess.go:173, they do not match the code that follows the trace in this fixed patch.

@MikaelVallenet
Copy link
Contributor

@MikaelVallenet

I think it should print the out of gas exception, isn't ?

Have you checked if you ran the test against the correct version of the chain?

I noticed you only ran make install. Did you start the chain with the patch?

Also, here is the stack trace from the video at 1:04.

image

At line numbers preprocess.go:137 and preprocess.go:173, they do not match the code that follows the trace in this fixed patch.

I see, indeed i did not start chain w/ patch thanks for clarification 🙏

stderr '--= Error =--'
stderr 'Data: out of gas error'
stderr 'Msg Traces:'
stderr ' 0 /home/runner/work/gno/gno/tm2/pkg/crypto/keys/client/maketx.go:215 - deliver transaction failed: log:out of gas, gasWanted: 60000, gasUsed: 60343 location: ReadFlat'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stderr ' 0 /home/runner/work/gno/gno/tm2/pkg/crypto/keys/client/maketx.go:215 - deliver transaction failed: log:out of gas, gasWanted: 60000, gasUsed: 60343 location: ReadFlat'
stderr ' 0 /home/runner/work/gno/gno/tm2/pkg/crypto/keys/client/maketx.go:215 - deliver transaction failed: log:out of gas, gasWanted: 60000, gasUsed: \d+ location: ReadFlat'

Same for the other gas used. I'd rather we checked the values of gas in gas_test.go rather than in filetests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The chain consumes gas at multiple points outside the VM before reaching the VM. Even inside the VM, a transaction may run out of gas during preprocessing or runtime. To precisely replicate and verify that the bug is fixed in the preprocessing stage, we need to specify an exact gas amount in the integration test.

Copy link
Member

Choose a reason for hiding this comment

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

This is checking against the error message. If we change how gas calculated, even slightly, this is yet another file where we need to go update the tests. It's an annoyance.

In this case, it's not necessary for the test; this is just checking the error message.

gnoland start

# add foo package
gnokey maketx addpkg -pkgdir $WORK/foo -pkgpath gno.land/r/foo -gas-fee 1000000ugnot -gas-wanted 211483 -broadcast -chainid=tendermint_test test1
Copy link
Member

@thehowl thehowl Oct 30, 2024

Choose a reason for hiding this comment

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

Suggested change
gnokey maketx addpkg -pkgdir $WORK/foo -pkgpath gno.land/r/foo -gas-fee 1000000ugnot -gas-wanted 211483 -broadcast -chainid=tendermint_test test1
gnokey maketx addpkg -pkgdir $WORK/foo -pkgpath gno.land/r/foo -gas-fee 1000000ugnot -gas-wanted 500000 -broadcast -chainid=tendermint_test test1

like the other comment, let's maybe not make this a test against specific gas results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as above:

The chain consumes gas at multiple points outside the VM before reaching the VM. Even inside the VM, a transaction may run out of gas during preprocessing or runtime. To precisely replicate and verify that the bug is fixed in the preprocessing stage, we need to specify an exact gas amount in the integration test.

Copy link
Member

Choose a reason for hiding this comment

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

Same here, this command is not supposed fail; in fact it's not prefixed with !.

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Let's change the test and merge this; I'd rather we did it slightly differently but I'm happy to do a follow-up PR myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants