-
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
Ensure System.Runtime.GetNotifications
handler operates with a deep copy of notifications
#3484
Comments
Our refcounting model slightly differs from C#, thus I'm not sure we ever have this problem, need to investigate. |
#2525 is probably related |
This test ensures that NeoGo node doesn't have the DeepCopy problem described in neo-project/neo#3300 and fixed in neo-project/neo#3301. This problem leads to the fact that Notifications items are not being properly refcounted by C# node which leads to possibility to build an enormously large object on stack. Go node doesn't have this problem. The reason (at least, as I understand it) is in the fact that C# node performs objects refcounting inside the DeepCopy even if the object itself is not yet on stack. I.e. System.Runtime.Notify handler immediately adds references to the notification argumetns inside DeepCopy: /~https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L108 /~https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L75 Whereas Go node just performs the honest DeepCopy without references counting: /~https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stackitem/item.go#L1223 Going further, C# node clears refs for notification arguments (for array and underlying array items). System.Runtime.GetNotifications pushes the notificaiton args array back on stack and increments counter only for the external array, not for its arguments. Which results in negative refcounter once notificaiton is removed from the stack. The fix itself (/~https://github.com/Jim8y/neo/blob/f471c0542ddd8f527478fbdcda76a3ab9194b958/src/Neo/SmartContract/NotifyEventArgs.cs#L84) doesn't need to be ported to NeoGo because Go node adds object to the refcounter only at the moment when it's being pushed to stack by System.Runtime.GetNotifications handler. This object is treated as new object since it was deepcopied earlier by System.Runtime.Notify handler: /~https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stack.go#L178. Thus, no functoinal changes from the NeoGo side. And we won't intentionally break our node to follow C# pre-Domovoi invalid behaviour. Close #3484, close #3482. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Also, here are some T5 scrapping results I got on hands for the recent blocks. Application log differences between C# and Go nodes caused by the transactions similar to the ones described in #3482 (comment). I agree with @roman-khimov in that we shouldn't break the NeoGo node behaviour in order to match pre-Domovoi C# node behaviour.
|
Ref. neo-project/neo#3300, port neo-project/neo#3301 if needed.
The text was updated successfully, but these errors were encountered: