-
Notifications
You must be signed in to change notification settings - Fork 81
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 some refcounter issues #2498
Conversation
Thanks @ixje for spotting this.
Codecov Report
@@ Coverage Diff @@
## master #2498 +/- ##
==========================================
+ Coverage 85.09% 85.15% +0.06%
==========================================
Files 293 294 +1
Lines 36923 36945 +22
==========================================
+ Hits 31418 31460 +42
+ Misses 4187 4165 -22
- Partials 1318 1320 +2
Continue to review full report at Codecov.
|
Match C# behavior. Thanks to @ixje for finding this.
I think there might be an issue with circular references as well. I'll try to verify and otherwise provide an example. |
Here's a test to reproduce the issue func TestCircularRefs(t *testing.T) {
h := "560110c34a4acf4a10c36058cf5810c34ecf10c34ecf58cf0b604a11d2"
prog, err := hex.DecodeString(h)
require.NoError(t, err)
vm := load(prog)
vm.Run()
assert.Equal(t, 3, vm.refs)
} This script comes from here https://github.com/neo-project/neo-vm/blob/master/tests/neo-vm.Tests/UtReferenceCounter.cs#L14-L41. The format of the comments in that file is
means 2 array items in the evaluation stack, 1 null stack item in the staticfields slot, total ref count of 3. The last opcode is
The problem is with B containing itself, therefore the I don't think this scenario is very realistic on chain, but you might want to solve it regardless of that. The implication I see is that the max stack items limit might be reached sooner than the C# VM. |
Moved that to #2500 and merging this one as is. |
Thanks @ixje for spotting these.