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.

Sign up for GitHub

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

Jump to bottom

fix: catch the out of gas exception in preprocess #2638

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

fix: catch the out of gas exception in preprocess #2638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2a46ad
chores: catch the out of gas exception in preprocess
piux2 Jul 28, 2024
c73b83a
chore: lint
piux2 Jul 29, 2024
a86b93f
add a txtar file that catchs out-of-gas errors during preprocess
piux2 Aug 3, 2024
8c22541
updated txtar
piux2 Aug 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions gno.land/cmd/gnoland/testdata/addpkg_outofgas.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# ensure users get proper out of gas errors when they add packages

# start a new node
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 •
edited
Loading

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 !.



# add bar package
# out of gas at store.GetPackage() with gas 60000

! gnokey maketx addpkg -pkgdir $WORK/bar -pkgpath gno.land/r/bar -gas-fee 1000000ugnot -gas-wanted 60000 -broadcast -chainid=tendermint_test test1

# Out of gas error

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.

stderr '--= /Error =--'



# out of gas at store.store.GetTypeSafe() with gas 63000

! gnokey maketx addpkg -pkgdir $WORK/bar -pkgpath gno.land/r/bar -gas-fee 1000000ugnot -gas-wanted 63000 -broadcast -chainid=tendermint_test test1

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: 63000, gasUsed: 63363 location: ReadFlat'
stderr '--= /Error =--'


-- foo/foo.gno --
package foo

type Counter int

func Inc(i Counter) Counter{
i = i+1
return i
}

-- bar/bar.gno --
package bar

import "gno.land/r/foo"

type NewCounter foo.Counter

func Add2(i NewCounter) NewCounter{
i=i+2

return i
}
13 changes: 13 additions & 0 deletions gnovm/pkg/gnolang/preprocess.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"sync/atomic"

"github.com/gnolang/gno/tm2/pkg/errors"
tmstore "github.com/gnolang/gno/tm2/pkg/store"
)

const (
Expand Down Expand Up @@ -117,6 +118,10 @@
_ = Transcribe(bn, func(ns []Node, ftype TransField, index int, n Node, stage TransStage) (Node, TransCtrl) {
defer func() {
if r := recover(); r != nil {
// Catch the out-of-gas exception and throw it
if _, ok := r.(tmstore.OutOfGasException); ok {
panic(r)

Check warning on line 123 in gnovm/pkg/gnolang/preprocess.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/preprocess.go#L123 

Added line #L123 was not covered by tests
}
// before re-throwing the error, append location information to message.
loc := last.GetLocation()
if nline := n.GetLine(); nline > 0 {
Expand Down Expand Up @@ -393,6 +398,10 @@

defer func() {
if r := recover(); r != nil {
// Catch the out-of-gas exception and throw it
if _, ok := r.(tmstore.OutOfGasException); ok {
panic(r)

Check warning on line 403 in gnovm/pkg/gnolang/preprocess.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/preprocess.go#L403 

Added line #L403 was not covered by tests
}
// before re-throwing the error, append location information to message.
loc := last.GetLocation()
if nline := n.GetLine(); nline > 0 {
Expand Down Expand Up @@ -3169,6 +3178,10 @@
func predefineNow(store Store, last BlockNode, d Decl) (Decl, bool) {
defer func() {
if r := recover(); r != nil {
// Catch the out-of-gas exception and throw it
if _, ok := r.(tmstore.OutOfGasException); ok {
panic(r)
}
// before re-throwing the error, append location information to message.
loc := last.GetLocation()
if nline := d.GetLine(); nline > 0 {
Expand Down
Loading