fix(gnovm): correctly update refs count when self-assigning #960

n0izn0iz · 2023-07-09T19:31:09Z

Fixes #939
See #960 (comment) for explanations of the fix
Thanks to @peter7891 for most of the actual fixing

Checklists...

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.

Maintainers Checklist

Checked that the author followed the guidelines in CONTRIBUTING.md
Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

peter7891 · 2023-07-12T22:00:03Z

Before merging anything, we need to answer the following questions convincingly, in the following order.

Is the bug reproducible?
What is broken?
How does this change fix it?

I haven't been able to reproduce the bug, which limits me in making a strong case to answering the other 2 questions.
Can you @n0izn0iz provide me with detailed steps you perform to reproduce it?

I've ran the functions you listed in a regular Go test inside the VM and Gno test contract (locally), it works fine.

n0izn0iz · 2023-07-12T22:42:50Z

@peter7891
I don't want to merge this, like I said I barely understand my changes (and the commit is called "no idea what I'm doing")
Though I did retry the steps outlined in #939 on latest master (02b88c0) and I still get undead-element in render after Pop and Push
We can do peer-programing so I can show you how to reproduce if you want

peter7891 · 2023-07-12T22:52:17Z

@peter7891 I don't want to merge this, like I said I barely understand my changes (and the commit is called "no idea what I'm doing") Though I did retry the steps outlined in #939 on latest master (02b88c0) and I still get undead-element in render after Pop and Push We can do peer-programing so I can show you how to reproduce if you want

That would be great. Discord? Petar#4367

peter7891 · 2023-07-14T23:51:29Z

Before merging anything, we need to answer the following questions convincingly, in the following order.

Is the bug reproducible?

What is broken?

How does this change fix it?

I haven't been able to reproduce the bug, which limits me in making a strong case to answering the other 2 questions. Can you @n0izn0iz provide me with detailed steps you perform to reproduce it?

I've ran the functions you listed in a regular Go test inside the VM and Gno test contract (locally), it works fine.

I think, i can answer these questions now.

I've reproduced the bug locally.
the refcount for the slice, when it's being assigned to itself is increasing inside realm.DidUpdate()
here

With the code of the original bug report,

var slice = []string{"undead-element"}

func Pop() {
	slice = slice[:len(slice)-1]
}

After the assignment inside Pop gets evaluated, slice has a refcount of 2, for a brief period, which should not be the case. This makes it being treated as NewEscaped and it doesn't get marked as dirty.

I think, we should either create a separate function to handle this corner case or only increment and decrement refs when the old and new object are not the same.

@jaekwon What do you think?

n0izn0iz · 2023-07-15T15:33:09Z

I updated the commit with the changes proposed by @peter7891

@peter7891

this solution was mostly found by @peter7891 Signed-off-by: Norman Meier <norman@berty.tech>

zivkovicmilos · 2023-07-27T16:58:00Z

Are we able to reproduce this in a unit test at all?

n0izn0iz · 2023-07-27T17:28:45Z

Yeah I managed to have a test using manfred's pattern in #979, will push it here

Signed-off-by: Norman Meier <norman@berty.tech>

n0izn0iz · 2023-07-27T17:32:20Z

The test I just added fails on master but passes on this PR, it reproduces the original bug description in #939
I'm not sure it's run automatically, to run it manually you can do
go test -timeout 30s -run ^TestSimpleFlow$ github.com/gnolang/gno/examples/gno.land/r/demo/multitxtest -v for example

Signed-off-by: Norman Meier <norman@berty.tech>

zivkovicmilos

Thank you for adding a fix for this 🙏
Can you please update the PR description with a brief summary of the changes (from Peter's comment)?

Pinging @peter7891 and @jaekwon to give a look as well

zivkovicmilos · 2023-08-03T14:07:36Z

examples/gno.land/r/demo/multitxtest/multitx_testing.go

@@ -0,0 +1,62 @@
+package multitxtest


Should these tests be contained in the examples subdirectory, instead of gnovm, since these are .go files after all?

I believe we should keep them in the examples folder, alongside the contract implementation.

I started this in #979, but paused to prioritize #1016, which will also provide a go API for advanced gno tests.

In the future, we plan to remove those _test.go files completely in favor of a gno-centric multi-step approach defined in #934.

By the way, I suggest moving the entire realm to the r/demo/x/ subfolder for experiments and things we don't want people to interact with.

Given that the _test.go file is loading the local .gno file, my suggestion is to create an official folder for these flows, located at gnovm/tests/flows/. We can place multitx.go in this folder as well as the other file, issue939/flow_test.go. This way, the examples folder can remain dedicated to showcasing real examples.

moul

See comments, thank you.

zivkovicmilos · 2023-08-17T12:29:30Z

@n0izn0iz Ping for visibility on this PR, please check out the comments so we can go ahead and merge this 🙏

moul · 2023-08-17T16:11:14Z

@n0izn0iz just switched the PR to draft; please remove the draft flag when you'll apply the last changes.

n0izn0iz · 2023-08-29T17:44:16Z

sorry for moving slowly on that, I'd like to take the time to properly implement the multi-tx test helper, currently it barely works, I barely understand it and is very hacky/brittle, maybe I should revert to include it as-is directly in the test and do the mutli-tx test helper in another PR

tbruyelle · 2023-10-04T12:06:05Z

@n0izn0iz Hey sounds this is related to a bug we had on gnochess, see #1170, can you confirm we're talking about the same bug ?
About the multi tx test helper, you should have a look at the recent work from @moul & @gfanton with testscript on gnochess : gnolang/gnochess@df9744d

Basically you setup your "*_test.go" file with gno/gno.land/pkg/integration (unfortunately not yet merged on master, still in the patch/gnochess branch), and then you create txtar files that are executed by your setup. The txtar files contain commands that look like shell, but some of them are overridden, some other are customs like cmp. At the end of the file you can generate files with the --- filename --- instructions, those files will be created before the execution of the commands from above.

@deelawn

So I think we can state that the bugs we have in gnolang#960, gnolang#1167 and gnolang#1170, are all related to slice storage when its capacity is different than its length. @deelawn found a great way to overcome that bug, but the bug is still there, somewhere in the VM code I think. I spent the last couple of days trying to find it, unfortunately without success. That said, I found a workaround, that could be also applied: when a slice is stored, ignore any capacity different than the slice's length. I think this is a good workaround because its one-line and because we don't really care about storing slice with capacity higher than their length (unless I'm missing something).

@tbruyelle

Addresses #1167, #960, and #1170 Consider the following situation: - A slice of structs exists with a length of zero and a capacity of one - A new struct literal is appended to the slice - The code panics because the newly allocated struct literal was never marked as "new" ``` go package append import ( "gno.land/p/demo/ufmt" ) type T struct{ i int } var a []T func init() { a = make([]T, 0, 1) } func Append(i int) { a = append(a, T{i: i}) } ``` Invoking the `Append` function will cause a panic. The solution is to traverse each of the array elements after slice append assignment to make sure any new or updated elements are marked as such. This PR also includes a change to ensure that marking an object as dirty and managing references to the object are mutually exclusive. I think this is correct but am not sure. The changes include txtar test cases that incorporate the issue described by @tbruyelle in #1170 --------- Co-authored-by: jaekwon <jae@tendermint.com>

deelawn · 2024-01-10T16:21:07Z

The issue this PR was meant to address has been fixed by #1305

@tbruyelle

…lang#1305) Addresses gnolang#1167, gnolang#960, and gnolang#1170 Consider the following situation: - A slice of structs exists with a length of zero and a capacity of one - A new struct literal is appended to the slice - The code panics because the newly allocated struct literal was never marked as "new" ``` go package append import ( "gno.land/p/demo/ufmt" ) type T struct{ i int } var a []T func init() { a = make([]T, 0, 1) } func Append(i int) { a = append(a, T{i: i}) } ``` Invoking the `Append` function will cause a panic. The solution is to traverse each of the array elements after slice append assignment to make sure any new or updated elements are marked as such. This PR also includes a change to ensure that marking an object as dirty and managing references to the object are mutually exclusive. I think this is correct but am not sure. The changes include txtar test cases that incorporate the issue described by @tbruyelle in gnolang#1170 --------- Co-authored-by: jaekwon <jae@tendermint.com>

github-actions bot assigned n0izn0iz Jul 9, 2023

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 9, 2023

moul marked this pull request as ready for review July 12, 2023 16:20

moul requested review from jaekwon and moul as code owners July 12, 2023 16:20

moul requested a review from peter7891 July 12, 2023 16:21

n0izn0iz force-pushed the fix_pop_push_slice branch 2 times, most recently from 304dcf2 to cb0665c Compare July 15, 2023 15:25

n0izn0iz changed the title ~~fix: pop/push slice~~ fix(gnovm): correctly update refs count when self-assigning Jul 15, 2023

n0izn0iz force-pushed the fix_pop_push_slice branch 2 times, most recently from 36bdbe7 to 2e26a35 Compare July 15, 2023 15:55

fix(gnovm): correctly update refs count when self-assigning

351a0f1

this solution was mostly found by @peter7891 Signed-off-by: Norman Meier <norman@berty.tech>

n0izn0iz force-pushed the fix_pop_push_slice branch from 2e26a35 to 351a0f1 Compare July 15, 2023 15:56

zivkovicmilos self-requested a review July 26, 2023 15:09

michelleellen mentioned this pull request Jul 26, 2023

Create Agenda (26.07.2023)_#10.md gnolang/meetings#20

Closed

14 tasks

moul mentioned this pull request Jul 27, 2023

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

chore: add empty slice assign test

286ede3

Signed-off-by: Norman Meier <norman@berty.tech>

n0izn0iz requested a review from a team as a code owner July 27, 2023 17:31

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jul 27, 2023

n0izn0iz and others added 2 commits July 31, 2023 13:22

chore: add util to run multitx tests

bee1d29

Signed-off-by: Norman Meier <norman@berty.tech>

Merge branch 'master' into fix_pop_push_slice

f665d03

zivkovicmilos approved these changes Aug 3, 2023

View reviewed changes

moul requested changes Aug 3, 2023

View reviewed changes

moul marked this pull request as draft August 17, 2023 16:10

moul added this to the 🚀 main.gno.land milestone Sep 6, 2023

n0izn0iz mentioned this pull request Oct 27, 2023

fix: properly mark array elements when an realm slice is updated #1305

Merged

deelawn closed this Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gnovm): correctly update refs count when self-assigning #960

fix(gnovm): correctly update refs count when self-assigning #960

n0izn0iz commented Jul 9, 2023 •

edited

Loading

peter7891 commented Jul 12, 2023 •

edited

Loading

n0izn0iz commented Jul 12, 2023 •

edited

Loading

peter7891 commented Jul 12, 2023

peter7891 commented Jul 14, 2023

n0izn0iz commented Jul 15, 2023

zivkovicmilos commented Jul 27, 2023

n0izn0iz commented Jul 27, 2023

n0izn0iz commented Jul 27, 2023 •

edited

Loading

zivkovicmilos left a comment

zivkovicmilos Aug 3, 2023

moul Aug 3, 2023

moul Aug 3, 2023 •

edited

Loading

moul left a comment

zivkovicmilos commented Aug 17, 2023

moul commented Aug 17, 2023

n0izn0iz commented Aug 29, 2023

tbruyelle commented Oct 4, 2023 •

edited

Loading

deelawn commented Jan 10, 2024

fix(gnovm): correctly update refs count when self-assigning #960

fix(gnovm): correctly update refs count when self-assigning #960

Conversation

n0izn0iz commented Jul 9, 2023 • edited Loading

Contributors Checklist

Maintainers Checklist

peter7891 commented Jul 12, 2023 • edited Loading

n0izn0iz commented Jul 12, 2023 • edited Loading

peter7891 commented Jul 12, 2023

peter7891 commented Jul 14, 2023

n0izn0iz commented Jul 15, 2023

zivkovicmilos commented Jul 27, 2023

n0izn0iz commented Jul 27, 2023

n0izn0iz commented Jul 27, 2023 • edited Loading

zivkovicmilos left a comment

Choose a reason for hiding this comment

zivkovicmilos Aug 3, 2023

Choose a reason for hiding this comment

moul Aug 3, 2023

Choose a reason for hiding this comment

moul Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

moul left a comment

Choose a reason for hiding this comment

zivkovicmilos commented Aug 17, 2023

moul commented Aug 17, 2023

n0izn0iz commented Aug 29, 2023

tbruyelle commented Oct 4, 2023 • edited Loading

deelawn commented Jan 10, 2024

n0izn0iz commented Jul 9, 2023 •

edited

Loading

peter7891 commented Jul 12, 2023 •

edited

Loading

n0izn0iz commented Jul 12, 2023 •

edited

Loading

n0izn0iz commented Jul 27, 2023 •

edited

Loading

moul Aug 3, 2023 •

edited

Loading

tbruyelle commented Oct 4, 2023 •

edited

Loading