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: panic in store.SprintStoreOps #1036

Merged
merged 6 commits into from
Sep 7, 2023

Conversation

tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Aug 9, 2023

2 ways to reproduce :

  • Using a filetest:
 go run ./gnovm/cmd/gno test -verbose ./examples/gno.land/p/demo/tests/ -run file/z0
?       ./examples/gno.land/p/demo/tests/subtests 	[no test files]
=== RUN   file/z0_filetest.gno
panic: expected JSON object but got "20"

goroutine 1 [running]:
github.com/gnolang/gno/tm2/pkg/amino.(*Codec).MustMarshalJSON(...)
	/home/tom/src/gno/tm2/pkg/amino/amino.go:818
github.com/gnolang/gno/tm2/pkg/amino.MustMarshalJSON({0xda1d40?, 0xc0001305a0?})
	/home/tom/src/gno/tm2/pkg/amino/amino.go:140 +0x53
github.com/gnolang/gno/gnovm/pkg/gnolang.StoreOp.String({0x0, {0x1026bf0, 0xc0001305a0}, {0x0, 0x0}})
	/home/tom/src/gno/gnovm/pkg/gnolang/store.go:654 +0xb2
github.com/gnolang/gno/gnovm/pkg/gnolang.(*defaultStore).SprintStoreOps(0xc0000166c0)
	/home/tom/src/gno/gnovm/pkg/gnolang/store.go:687 +0xff
github.com/gnolang/gno/gnovm/tests.RunFileTest({0xc000357158, 0x11}, {0xc0003691a0, 0x2e}, {0xc0001499e8, 0x1, 0x1013280?})
	/home/tom/src/gno/gnovm/tests/file.go:325 +0x44e
main.gnoTestPkg({0xc000116500, 0x20}, {0x0?, 0x0, 0x1?}, {0xc00035b720, 0x1, 0xc000129af8?}, 0xc00034c8c0, 0xc0002948c0)
	/home/tom/src/gno/gnovm/cmd/gno/test.go:337 +0x876
main.execTest(0xc00034c8c0, {0xc00035b590, 0x1, 0x1}, 0xc000036220?)
	/home/tom/src/gno/gnovm/cmd/gno/test.go:208 +0xb67
main.newTestCmd.func1({0x0?, 0x0?}, {0xc00035b590?, 0xc00035f118?, 0x0?})
	/home/tom/src/gno/gnovm/cmd/gno/test.go:51 +0x32
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0x5?, {0x1018768?, 0xc00003a130?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:233 +0x1ac
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0xc00035ee70?, {0x1018768?, 0xc00003a130?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:237 +0x154
github.com/gnolang/gno/tm2/pkg/commands.(*Command).ParseAndRun(0x4059dc?, {0x1018768, 0xc00003a130}, {0xc0000361f0?, 0x401240?, 0x0?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:118 +0x4f
main.main()
	/home/tom/src/gno/gnovm/cmd/gno/main.go:14 +0x75
exit status 2
  • using a go unit test:
$  go test ./gnovm/pkg/gnolang/ -v -run Amino
=== RUN   TestAminoMustMarshalJSONPanics
"20"
--- FAIL: TestAminoMustMarshalJSONPanics (0.00s)
panic: expected JSON object but got "20" [recovered]
	panic: expected JSON object but got "20"

goroutine 6 [running]:
testing.tRunner.func1.2({0xc15460, 0xc0002d2fc0})
	/usr/lib/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/usr/lib/go/src/testing/testing.go:1529 +0x39f
panic({0xc15460, 0xc0002d2fc0})
	/usr/lib/go/src/runtime/panic.go:884 +0x213
github.com/gnolang/gno/tm2/pkg/amino.(*Codec).MustMarshalJSON(...)
	/home/tom/src/gno/tm2/pkg/amino/amino.go:818
github.com/gnolang/gno/tm2/pkg/amino.MustMarshalJSON({0xc59ec0?, 0xc0001c42d0?})
	/home/tom/src/gno/tm2/pkg/amino/amino.go:140 +0x53
github.com/gnolang/gno/gnovm/pkg/gnolang.TestAminoMustMarshalJSONPanics(0x0?)
	/home/tom/src/gno/gnovm/pkg/gnolang/values_test.go:20 +0x134
testing.tRunner(0xc000300000, 0xcd8f40)
	/usr/lib/go/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/usr/lib/go/src/testing/testing.go:1629 +0x3ea
FAIL	github.com/gnolang/gno/gnovm/pkg/gnolang	0.008s
FAIL

Additional notes:

  • when // Realm: instruction is present, it triggers a comparison between the store and the content of the instruction.
  • when the store content is serialized before the comparison, this panic happens in the amino code
  • this doesn't affect all // Realm: instruction, for instance examples/gno.land/p/demo/avl/z_0_filetest.gno is still working well
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.

@tbruyelle tbruyelle requested a review from a team as a code owner August 9, 2023 10:29
@tbruyelle tbruyelle marked this pull request as draft August 9, 2023 10:29
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Aug 9, 2023
@moul
Copy link
Member

moul commented Aug 9, 2023

Output with standard JSON: https://gist.github.com/moul/cc7fda42d648ceae144d9bff55a20d7e

@moul
Copy link
Member

moul commented Aug 9, 2023

I think this is because // Realm: contains a non-json thing. You don't have the error if you just have // Realm: without any content.

I think it's related to one of your other PRs, with the goal to make --update-golden-tests also for // Realm:, nope?

Maybe the best move is:

  • improve error handling, with a more explicit message if we are in comparison mode (not in update mode).
  • make -update-golden-tests overwrite the content whatever it is so that we quickly only have valid JSON, and no need for handcrafted JSON here.

@tbruyelle
Copy link
Contributor Author

tbruyelle commented Aug 9, 2023

I think this is because // Realm: contains a non-json thing. You don't have the error if you just have // Realm: without any content.

That's also what I thought initially, but store.SprintStoreOps() doesn't rely on the content // Realm: only on the content of the store. Proof of this, if you change the // Realm: instruction of the one that works ( examples/gno.land/p/demo/avl/z_0_filetest.gno), to

// Realm:
// xxx

then there is no panic.

I think the panic is due to :

  • an unexpected content of the store
    OR
  • a bug in the amino encoder

I think it's related to one of your other PRs, with the goal to make --update-golden-tests also for // Realm:, nope?

The PR you're referring to doesn't exist yet, there's just a preliminary PR that brings test to make it doable (#1023).

@tbruyelle
Copy link
Contributor Author

Output with standard JSON: gist.github.com/moul/cc7fda42d648ceae144d9bff55a20d7e

How did you achieve that? using only json.Encode from the stdlib ?

@tbruyelle
Copy link
Contributor Author

I found the culprit :

file gnovm/stdlibgs/std/crypto.gno :

package std

type Address string // NOTE: bech32

func (a Address) String() string {
	return string(a)
}

const RawAddressSize = 20   // <--------------------- interpreted as a bigint

type RawAddress [RawAddressSize]byte

Proof of this, if I change the value to 21, the panic becomes

panic: expected JSON object but got "21"

So I think it's related to how gno bigint are interpreted in amino. If I change again this const to

const RawAddressSize = int(20)

Everything works properly.

Does anyone know if something has changed in the way that const are interpreted ? Why is it a bigint here ?

@tbruyelle tbruyelle force-pushed the tbruyelle/bug/realm-instruction branch from e2ddccd to d301dad Compare August 10, 2023 11:44
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 10, 2023
@tbruyelle tbruyelle changed the title bug: this change raises a panic in store.SprintStoreOps fix: panic in store.SprintStoreOps Aug 10, 2023
@moul
Copy link
Member

moul commented Aug 10, 2023

bigint is in the codebase from day 1: 6ff6762#diff-14db1b285d73487af27af13336ae8cf7797355a680b7049b5dd36771d72794ef and we need to support such types and will even work on improving the support (several related discussions).

I believe using bigint as the default type for numeric constants is a bug.

The implementation of bigint seems incomplete, possibly due to lacking marshalling helpers in values.go, causing the mentioned panic.

@jaekwon wdyt?

```
 go run ./gnovm/cmd/gno test -verbose ./examples/gno.land/p/demo/tests/ -run file/z0
?       ./examples/gno.land/p/demo/tests/subtests 	[no test files]
=== RUN   file/z0_filetest.gno
panic: expected JSON object but got "20"

goroutine 1 [running]:
github.com/gnolang/gno/tm2/pkg/amino.(*Codec).MustMarshalJSON(...)
	/home/tom/src/gno/tm2/pkg/amino/amino.go:818
github.com/gnolang/gno/tm2/pkg/amino.MustMarshalJSON({0xda1d40?, 0xc0001305a0?})
	/home/tom/src/gno/tm2/pkg/amino/amino.go:140 +0x53
github.com/gnolang/gno/gnovm/pkg/gnolang.StoreOp.String({0x0, {0x1026bf0, 0xc0001305a0}, {0x0, 0x0}})
	/home/tom/src/gno/gnovm/pkg/gnolang/store.go:654 +0xb2
github.com/gnolang/gno/gnovm/pkg/gnolang.(*defaultStore).SprintStoreOps(0xc0000166c0)
	/home/tom/src/gno/gnovm/pkg/gnolang/store.go:687 +0xff
github.com/gnolang/gno/gnovm/tests.RunFileTest({0xc000357158, 0x11}, {0xc0003691a0, 0x2e}, {0xc0001499e8, 0x1, 0x1013280?})
	/home/tom/src/gno/gnovm/tests/file.go:325 +0x44e
main.gnoTestPkg({0xc000116500, 0x20}, {0x0?, 0x0, 0x1?}, {0xc00035b720, 0x1, 0xc000129af8?}, 0xc00034c8c0, 0xc0002948c0)
	/home/tom/src/gno/gnovm/cmd/gno/test.go:337 +0x876
main.execTest(0xc00034c8c0, {0xc00035b590, 0x1, 0x1}, 0xc000036220?)
	/home/tom/src/gno/gnovm/cmd/gno/test.go:208 +0xb67
main.newTestCmd.func1({0x0?, 0x0?}, {0xc00035b590?, 0xc00035f118?, 0x0?})
	/home/tom/src/gno/gnovm/cmd/gno/test.go:51 +0x32
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0x5?, {0x1018768?, 0xc00003a130?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:233 +0x1ac
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0xc00035ee70?, {0x1018768?, 0xc00003a130?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:237 +0x154
github.com/gnolang/gno/tm2/pkg/commands.(*Command).ParseAndRun(0x4059dc?, {0x1018768, 0xc00003a130}, {0xc0000361f0?, 0x401240?, 0x0?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:118 +0x4f
main.main()
	/home/tom/src/gno/gnovm/cmd/gno/main.go:14 +0x75
exit status 2
```
- change std.RawAddressSize type to int
- run z0_filetest.gno with -update-golden-tests flag
@tbruyelle tbruyelle force-pushed the tbruyelle/bug/realm-instruction branch from d10a42b to 1107a00 Compare September 4, 2023 16:02
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Sep 4, 2023
@tbruyelle tbruyelle force-pushed the tbruyelle/bug/realm-instruction branch from 1107a00 to b2ffe3b Compare September 4, 2023 16:13
@github-actions github-actions bot removed the 🧾 package/realm Tag used for new Realms or Packages. label Sep 4, 2023
@tbruyelle
Copy link
Contributor Author

tbruyelle commented Sep 4, 2023

@moul found a fix \o/
see commit 4b6891a for content and explanation (this commit message should be used for the merge commit if we're OK on the review).

EDIT: commit hash

@tbruyelle tbruyelle marked this pull request as ready for review September 4, 2023 16:16
When checking if `info.IsJSONAnyValueType`, if this type is a
AminoMarshaler, then also check `info.ReprType.IsJSONAnyValueType`.

This prevents a panics that happens during the sanity check phase of
`encodeReflectJSONInterface()` when the type `ConcreteInfo` doesn't match
the JSON structure. For instance, before that fix, that used to happen
with `BigintValue` which is a struct (so expected JSON is `{...}` and
`IsJSONAnyValueType=false`), but is marshaled into a simple string (so
expected JSON is only `"..."`, and `ReprType.IsJSONAnyValueType=true`).

`decodeReflectJSONInterface` is also impacted.
@tbruyelle tbruyelle force-pushed the tbruyelle/bug/realm-instruction branch from b2ffe3b to 4b6891a Compare September 4, 2023 16:27
@moul moul added this to the 🚀 main.gno.land milestone Sep 6, 2023
@thehowl thehowl merged commit 207d66d into gnolang:master Sep 7, 2023
67 checks passed
@moul
Copy link
Member

moul commented Sep 7, 2023

Output with standard JSON: gist.github.com/moul/cc7fda42d648ceae144d9bff55a20d7e

How did you achieve that? using only json.Encode from the stdlib ?

Yeah, I often use my library for debugging: https://pkg.go.dev/moul.io/godev#PrettyJSONPB :)

@tbruyelle tbruyelle deleted the tbruyelle/bug/realm-instruction branch February 22, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gnopher-hole 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants