[Neo VM Bug] Fix negative counter issue #3327
Conversation
|
Can you please explain your solution? |
| foreach (StackItem subitem in compound.SubItems) | ||
| { | ||
| if (component.Contains(subitem)) continue; | ||
| if (!NeedTrack(subitem)) continue; | ||
| subitem.ObjectReferences!.Remove(compound); | ||
| references_count--; |
There was a problem hiding this comment.
this ensures that it can be properly removed, but i think there is also problem of adding reference.
There was a problem hiding this comment.
I believe if it goes negative it means the item was never met from previous one StackItem. You can use the CachedComponents to see.
|
I need some more time to check the compatibility with NeoGo, and this code should be symmetric to refs addition. |
src/Neo.VM/ReferenceCounter.cs
Outdated
| if (!NeedTrack(subitem)) continue; | ||
| subitem.ObjectReferences!.Remove(compound); | ||
| references_count--; |
There was a problem hiding this comment.
I'd expect references removal to be symmetric wrt references addition. And currently it's asymmetric because there's if (!NeedTrack(subitem)) continue; a few lines above; and during references addition we always increment refcounter irrespective of NeedTrack:
neo/src/Neo.VM/ReferenceCounter.cs
Lines 49 to 50 in b1d27f0
There was a problem hiding this comment.
Yes, maybe we need to move the line before that if
There was a problem hiding this comment.
It fails if is symmetric, we get a negative counter
There was a problem hiding this comment.
The thing that I'm worried about is that we should find the reason why symmetric approach doesn't work. Right now I don't completely understand it.
|
@shargon any plan about this pr? |
I think that the fix is good, but now with your PR it can be closed |
Code of this PR contains a @Jim8y's work
Description
This pr fixes the negative reference counter issue.
Fixes #3328
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: