-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Neo VM] adopt light gc from neogo #3581
base: HF_Echidna
Are you sure you want to change the base?
Conversation
A few unit test was updated to adapt light GC. Need help from neogo to review this pr, @roman-khimov @AnnaShaleva. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It should be a part of Echidna, we can't easily throw away the old RC.
- Probably the best way to test is to keep both RCs in the code and run through testnet/mainnet tx set, comparing values after every instruction. My expectation is they will be the same (at least if don't have any circular references in contract code).
src/Neo.VM/ReferenceCounterV2.cs
Outdated
if (item is CompoundType compoundType) | ||
{ | ||
// Increment the item's stack references by the specified count. | ||
compoundType.StackReferences--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In circular references this counter will become negative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for stack item A, B:
-
A->C, B->D => ref.A = 1, ref.B = 1, ref.C = 1, ref.D = 1
-
C->D => ref.C = 1, ref.D = 2.
-
D->C => ref.C = 2, ref.D = 2
when we try to remvoe C from A, we will have:
- C--; => ref(C) == 1
5.then, remve D from B, we will have:
D--; => ref.D = 1, // from here the operation related to D and C will be over, the RC will never be decreased anymore.
- but if we revert step 5, and try to remove C from D, we will have:
C--; => ref.C =0,
causing D--; ref.D = 1.
- following setp 6, remove D from B,
D--; ref.D =0, but since C is removed from D in step 6, no further operation will be taken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can become negative, rather it will have some positive count that is not exactly true (like 1 in RC with no elements on the stack), but this is a safe behavior.
For me this change seems good, but the test changed to fit this version, so if we want to bee 100% safe we will need a HF and be able to execute with the previous version |
should be the same if properly implementated, some ut was updated cause gc no longer work the same way, for instance, in this pr, you dont have to worry about adding a compound type whose subitems are not RC counted to an array, the GC will do it,,, so the test for adding RC uncounted compound type will no longer work. |
Array array = new(new StackItem[] { 1, 2, 3, 4 }); | ||
engine.CurrentContext.EvaluationStack.Push(array); | ||
Assert.AreEqual(array.Count + 1, engine.ReferenceCounter.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to avoid the UT changes, in this case, what's wrong exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because of how we process reference counter for compound type, previously we can test the RC without the engine, cause stackitem itself will process the RC, but in the new version, only executionstack will process the stack, as a result, we must push the item to the stack to get a RC count.
That is why it was 4 in the past, as the array is not in the stack. 4 = Array.subitems.
While new version its 5 (4+1), cause we have to push the array itself to the stack. 5 = Array.subitems+Array
I already made my optimizations/review, just we should ensure that it works with tons of UT and checking all the UT that was not working with the old logic |
_referencesCount += count; | ||
|
||
if (_referencesCount > _limits.MaxStackSize) | ||
throw new IndexOutOfRangeException("Circular reference detected, execution stopped."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the best exception message. It can be simply caused by too many objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opcode handlers part is nice and simple. But I haven't seen slot-related changes, e.g.: https://github.com/nspcc-dev/neo-go/blob/8952922c4d4f4474e390cf3afa4abcdf20c7960d/pkg/vm/slot.go#L26
Am I missing something?
/// <inheritdoc/> | ||
public void AddZeroReferred(StackItem item) | ||
{ | ||
// This call should not be made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In such cases I prefer to throw an uncatchable exception or even stop the node since it's a program bug if this code is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be compile time issue, should not be called at all...how to enforce that @shargon @cschuchardt88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Obsolete]
public extern void AddZeroReferred(StackItem item);
Which will throw then calling it System.BadImageFormatException: 'An attempt was made to load a program with an incorrect format. (0x8007000B)'
If you dont like then just throw new NotImplementedException();
or throw new NotSupportedException();
.
if (compoundType.StackReferences < 0) | ||
throw new IndexOutOfRangeException("Circular reference detected, execution stopped."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, NeoGo doesn't have this code, so I'd like to keep the compatibility. RC diffs are always hard to track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added just to ensure the correctness, if anything like these happens, the transaction should fault directly, but the error message is not accurate....
Slot works the same, adding removing, all the same. |
// | PoC_ReverseN | 8,399,264.69 us | 152,328.560 us | 135,035.301 us | | ||
// | PoC_Substr | 65.07 us | 1.122 us | 1.050 us | | ||
// | PoC_NewArray | 2,903,663.37 us | 15,827.726 us | 14,805.265 us | | ||
// | PoC_NewStruct | 3,027,978.35 us | 12,747.892 us | 11,300.674 us | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried to get some CPU profiling to find why operations on Array
and Struct
are so slow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally they call dotnet built in methods, profiling result shows the hotpoint are there.
Co-authored-by: nan01ab <[email protected]>
Co-authored-by: nan01ab <[email protected]>
Haven't looked into detail yet, but my test shows this branch already has incompatible behaviour at block 5570001 on mainnet {"block":5570001,"size":2,"storage":[{"state":"Changed","key":"\u002Bv///xTxfC2GMXu0A8EgTt24xG/TSsdwqw==","value":"QQEhBvGUR7LKDA=="},{"state":"Changed","key":"\u002Bv///ws=","value":"1vXR9XVmGA=="}]} Storage from this {"block":5570001,"size":5,"storage":[{"state":"Changed","key":"/////wjA7znO4OTpJcbCoGp54UQN2G/OrA==","value":"QAUhAf4hAQEoFMDvOc7g5OklxsKgannhRA3Yb86sKO9ORUYzbmVvLWNvcmUtdjMuMAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAoRBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAnXjWn0EIKAZTdGRMaWJAAEgAQABBAkAXQQUoBGF0b2lAAUECKAV2YWx1ZSEBEyEBESEAIAFBBSgEYXRvaUACQQIoBXZhbHVlIQETQQIoBGJhc2UhAREhAREhAQcgAUEFKBFiYXNlNThDaGVja0RlY29kZUABQQIoAXMhARMhARIhAQ4gAUEFKBFiYXNlNThDaGVja0VuY29kZUABQQIoBGRhdGEhARIhARMhARUgAUEFKAxiYXNlNThEZWNvZGVAAUECKAFzIQETIQESIQEcIAFBBSgMYmFzZTU4RW5jb2RlQAFBAigEZGF0YSEBEiEBEyEBIyABQQUoDGJhc2U2NERlY29kZUABQQIoAXMhARMhARIhASogAUEFKAxiYXNlNjRFbmNvZGVAAUECKARkYXRhIQESIQETIQExIAFBBSgPYmFzZTY0VXJsRGVjb2RlQAFBAigBcyEBEyEBEyEBOCABQQUoD2Jhc2U2NFVybEVuY29kZUABQQIoBGRhdGEhARMhARMhAT8gAUEFKAtkZXNlcmlhbGl6ZUABQQIoBGRhdGEhARIhACEBRiABQQUoBGl0b2FAAUECKAV2YWx1ZSEBESEBEyEBTSABQQUoBGl0b2FAAkECKAV2YWx1ZSEBEUECKARiYXNlIQERIQETIQFUIAFBBSgPanNvbkRlc2VyaWFsaXplQAFBAigEanNvbiEBEiEAIQFbIAFBBSgNanNvblNlcmlhbGl6ZUABQQIoBGl0ZW0hACEBEiEBYiABQQUoDW1lbW9yeUNvbXBhcmVAAkECKARzdHIxIQESQQIoBHN0cjIhARIhAREhAWkgAUEFKAxtZW1vcnlTZWFyY2hAAkECKANtZW0hARJBAigFdmFsdWUhARIhAREhAXAgAUEFKAxtZW1vcnlTZWFyY2hAA0ECKANtZW0hARJBAigFdmFsdWUhARJBAigFc3RhcnQhAREhAREhAXcgAUEFKAxtZW1vcnlTZWFyY2hABEECKANtZW0hARJBAigFdmFsdWUhARJBAigFc3RhcnQhARFBAigIYmFja3dhcmQhARAhAREhAX4gAUEFKAlzZXJpYWxpemVAAUECKARpdGVtIQAhARIhAoUAIAFBBSgGc3RyTGVuQAFBAigDc3RyIQETIQERIQKMACABQQUoC3N0cmluZ1NwbGl0QAJBAigDc3RyIQETQQIoCXNlcGFyYXRvciEBEyEBICECkwAgAUEFKAtzdHJpbmdTcGxpdEADQQIoA3N0ciEBE0ECKAlzZXBhcmF0b3IhARNBAigScmVtb3ZlRW1wdHlFbnRyaWVzIQEQIQEgIQKaACABQABAAUECAABAACgEbnVsbA=="},{"state":"Changed","key":"/////wj1Y\u002BpAvCg9TQ4FxI6jBbPyoHNA7w==","value":"QAUhAfshAQIoFPVj6kC8KD1NDgXEjqMFs/Kgc0DvKNpORUYzbmVvLWNvcmUtdjMuMAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAjBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dA4V0ET0EIKAhOZW9Ub2tlbkAASABAASgGTkVQLTE3QQJAFEEFKAliYWxhbmNlT2ZAAUECKAdhY2NvdW50IQEUIQERIQAgAUEFKAhkZWNpbWFsc0AAIQERIQEHIAFBBSgPZ2V0QWNjb3VudFN0YXRlQAFBAigHYWNjb3VudCEBFCEBICEBDiABQQUoEGdldEFsbENhbmRpZGF0ZXNAACEBMCEBFSABQQUoEGdldENhbmRpZGF0ZVZvdGVAAUECKAZwdWJLZXkhARYhAREhARwgAUEFKA1nZXRDYW5kaWRhdGVzQAAhASAhASMgAUEFKAxnZXRDb21taXR0ZWVAACEBICEBKiABQQUoE2dldENvbW1pdHRlZUFkZHJlc3NAACEBFCEBMSABQQUoDmdldEdhc1BlckJsb2NrQAAhAREhATggAUEFKBZnZXROZXh0QmxvY2tWYWxpZGF0b3JzQAAhASAhAT8gAUEFKBBnZXRSZWdpc3RlclByaWNlQAAhAREhAUYgAUEFKBFyZWdpc3RlckNhbmRpZGF0ZUABQQIoBnB1YmtleSEBFiEBECEBTSAAQQUoDnNldEdhc1BlckJsb2NrQAFBAigLZ2FzUGVyQmxvY2shAREhAv8AIQFUIABBBSgQc2V0UmVnaXN0ZXJQcmljZUABQQIoDXJlZ2lzdGVyUHJpY2UhAREhAv8AIQFbIABBBSgGc3ltYm9sQAAhARMhAWIgAUEFKAt0b3RhbFN1cHBseUAAIQERIQFpIAFBBSgIdHJhbnNmZXJABEECKARmcm9tIQEUQQIoAnRvIQEUQQIoBmFtb3VudCEBEUECKARkYXRhIQAhARAhAXAgAEEFKAx1bmNsYWltZWRHYXNAAkECKAdhY2NvdW50IQEUQQIoA2VuZCEBESEBESEBdyABQQUoE3VucmVnaXN0ZXJDYW5kaWRhdGVAAUECKAZwdWJrZXkhARYhARAhAX4gAEEFKAR2b3RlQAJBAigHYWNjb3VudCEBFEECKAZ2b3RlVG8hARYhARAhAoUAIABABEECKAhUcmFuc2ZlckADQQIoBGZyb20hARRBAigCdG8hARRBAigGYW1vdW50IQERQQIoFUNhbmRpZGF0ZVN0YXRlQ2hhbmdlZEADQQIoBnB1YmtleSEBFkECKApyZWdpc3RlcmVkIQEQQQIoBXZvdGVzIQERQQIoBFZvdGVABEECKAdhY2NvdW50IQEUQQIoBGZyb20hARZBAigCdG8hARZBAigGYW1vdW50IQERQQIoEENvbW1pdHRlZUNoYW5nZWRAAkECKANvbGQhASBBAigDbmV3IQEgQAFBAgAAQAAoBG51bGw="},{"state":"Changed","key":"/////wjileORVEwXitlPA\u002BxNzf94U07PSQ==","value":"QAUhAfghAQEoFOKV45FUTBeK2U8D7E3N/3hTTs9JKFxORUYzbmVvLWNvcmUtdjMuMAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADhBBGvd7Z0AQQRr3e2dApiGhOkEIKA5Sb2xlTWFuYWdlbWVudEAASABAAEECQAJBBSgPZGVzaWduYXRlQXNSb2xlQAJBAigEcm9sZSEBEUECKAVub2RlcyEBICEC/wAhACAAQQUoE2dldERlc2lnbmF0ZWRCeVJvbGVAAkECKARyb2xlIQERQQIoBWluZGV4IQERIQEgIQEHIAFAAUECKAtEZXNpZ25hdGlvbkAEQQIoBFJvbGUhARFBAigKQmxvY2tJbmRleCEBEUECKANPbGQhASBBAigDTmV3IQEgQAFBAgAAQAAoBG51bGw="},{"state":"Changed","key":"\u002Bv///xTxfC2GMXu0A8EgTt24xG/TSsdwqw==","value":"QQEhBvGUR7LKDA=="},{"state":"Changed","key":"\u002Bv///ws=","value":"1vXR9XVmGA=="}]} Good news is only this block shows incompatible data :) |
We discussed that in a Core Dev meeting, I did not remember exactly. |
There is a bug somewhere here, because NeoGo has exactly the same state. But given that we know the problematic block it should be relatively easy to figure out what's the problem. |
This may not be a bug. |
@roman-khimov @vncoelho @Jim8y Guys, seems it's not a bug, because Line 42 in 216c39e
So, it works as expected. It's nef and manifest update for StdLib, @Hecate2 is correct. I will check testnet state as well to ensure everything is OK. |
Data is compatible for testnet as well! |
private readonly ExecutionEngineLimits _limits; | ||
|
||
// Keeps the total count of references. | ||
private int _referencesCount = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding this: #3581 (comment)
What do you think about making it unsigned integer, as @roman-khimov suggested and as we have it in NeoGo? Then there won't be any compatibility issues.
Co-authored-by: Hecate2 <[email protected]>
Point 2. We have both RCs in C# codebase with this PR. So what can be done is:
Expectation: this will work fine. Reality: we don't know. If there is any divergence we need to check why it happens. Cases like this can then become unit tests (if any). |
Description
This pr tries to adopt the light GC solution from neogo.
It:
Fixes # (issue)
Type of change
Test Configuration:
Checklist: