[Neo Plugin Bug]fix 3296#3299
Conversation
|
It works on my private chain |
| { | ||
| _snapshot.Put(key, BinarySerializer.Serialize(StackItem.Null, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize })); | ||
| var detailedException = ex.Message; | ||
| if (detailedException.Length > 1024) |
There was a problem hiding this comment.
Thanks @Jim8y. You've create a lot of PRs that makes exception messages much more clear than before. So as this one.
It would be better if we change the magic 1024 to some constant variable like Default.MaxStackSize/2 or something else.
There was a problem hiding this comment.
I'm not sure what's the real value of Default.MaxStackSize. 1024 and 2048 or any number, they all seems good to me. Maybe @AnnaShaleva has more experience on this settings.
There was a problem hiding this comment.
@dusmart, to be honest, I'd prefer not mix the real (even invalid) stackitem with some system-created one. See the #3299 (comment).
There was a problem hiding this comment.
I'm not sure what's the real value of
Default.MaxStackSize. 1024 and 2048 or any number, they all seems good to me. Maybe @AnnaShaleva has more experience on this settings.
Settings comes the config.json so it can be whatever you want.
|
Tested on testnet, it works. |
| catch (NotSupportedException) | ||
| catch (Exception ex) | ||
| { | ||
| _snapshot.Put(key, BinarySerializer.Serialize(StackItem.Null, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize })); | ||
| var detailedException = ex.Message; | ||
| if (detailedException.Length > 1024) |
There was a problem hiding this comment.
Catching any exception is enough to solve the original problem. Why should we change the behavior and serialize exception message instead of Null stackitem?
There was a problem hiding this comment.
Null or exception both works. Using exception message to explicitly tell people there is an issue.
There was a problem hiding this comment.
We think that it's not quite correct to replace the original stackitem by either Null or ByteString because from the user side it's impossible to distinguish the "good" original stackitem from the result of invalid seialization.
Please, take a look at the way how this case is handled in NeoGo: https://github.com/nspcc-dev/neo-go/blob/cf4d4a2611209d8d085ccec6e1f1b8956006a577/pkg/vm/stackitem/serialization.go#L144. We use a special stackitem stub of InvalidT type which can't be produced by any means other than exception during serialisation. With this special type there's no doubts from the user side whether there was an exception during serialization or not.
There was a problem hiding this comment.
We think that it's not quite correct to replace the original stackitem by either Null or ByteString because from the user side it's impossible to distinguish the "good" original stackitem from the result of invalid seialization.
i understand your concern, but issues has being there and this pr is not for that purpose, you can have another pr to fix it. for me, no one but hackers will ever see that information.
There was a problem hiding this comment.
But if we're changing this code anyway, why not to make it work properly?
There was a problem hiding this comment.
But if we're changing this code anyway, why not to make it work properly?
Cause its not the purpose of this pr, though its easy to update, but that is your idea and your property, and most importantly, its another issue.
There was a problem hiding this comment.
I agree with the fix itself. But then let's not to change this line:
_snapshot.Put(key, BinarySerializer.Serialize(StackItem.Null, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize }));
And create the proper issue to improve it.
|
@AnnaShaleva updated |
* fix patch from: neo-project/neo#3261 and neo-project/neo#3299 * add neo-project/neo#3282 * add neo-project/neo#3263 * v3.7.5 hotfix * readme
Description
This pr solves the issue in #3296 that log only catch exception of specific type.
Fixes # #3296
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: