[Neo Plugin Bug]fix 3296 #3299

Jim8y · 2024-06-07T02:40:04Z

Description

This pr solves the issue in #3296 that log only catch exception of specific type.

Fixes # #3296

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Jim8y · 2024-06-07T02:40:21Z

@Hecate2 @dusmart @superboyiii

src/Plugins/ApplicationLogs/Store/LogStorageStore.cs

Hecate2 · 2024-06-07T02:49:37Z

It works on my private chain

dusmart · 2024-06-07T03:24:16Z

src/Plugins/ApplicationLogs/Store/LogStorageStore.cs

            {
-                _snapshot.Put(key, BinarySerializer.Serialize(StackItem.Null, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize }));
+                var detailedException = ex.Message;
+                if (detailedException.Length > 1024)


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.

@dusmart Thank you for the recognition, credits of this fix goes to @Hecate2. Need @dusmart your suggestion to decide that value, is Default.MaxStackSize/2 secure enough.

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.

@dusmart, to be honest, I'd prefer not mix the real (even invalid) stackitem with some system-created one. See the #3299 (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.

superboyiii · 2024-06-07T06:52:32Z

Tested on testnet, it works.

AnnaShaleva · 2024-06-07T07:02:13Z

src/Plugins/ApplicationLogs/Store/LogStorageStore.cs

-            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)


Catching any exception is enough to solve the original problem. Why should we change the behavior and serialize exception message instead of Null stackitem?

Null or exception both works. Using exception message to explicitly tell people there is an issue.

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.

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.

But if we're changing this code anyway, why not to make it work properly?

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.

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.

Ref. #3303.

Jim8y · 2024-06-07T14:28:33Z

@AnnaShaleva updated

neo-project/neo#3261 and neo-project/neo#3299

* 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

fix 3296

a25c723

Jim8y linked an issue Jun 7, 2024 that may be closed by this pull request

some nodes on testnet stopped syncing maybe caused by some modules #3296

Closed

Jim8y added bug Used to tag confirmed bugs critical Issues (bugs) that need to be fixed ASAP waiting for review labels Jun 7, 2024

Jim8y requested a review from a team June 7, 2024 02:41

Jim8y commented Jun 7, 2024

View reviewed changes

src/Plugins/ApplicationLogs/Store/LogStorageStore.cs Show resolved Hide resolved

Update src/Plugins/ApplicationLogs/Store/LogStorageStore.cs

c183732

Jim8y added 2 commits June 7, 2024 10:59

add exception message to the incorrect storeitem

1378d24

optimize the code to apply Hecate suggestion

dc0b574

dusmart reviewed Jun 7, 2024

View reviewed changes

cschuchardt88 previously approved these changes Jun 7, 2024

View reviewed changes

superboyiii approved these changes Jun 7, 2024

View reviewed changes

AnnaShaleva reviewed Jun 7, 2024

View reviewed changes

Jim8y requested a review from shargon June 7, 2024 07:09

shargon previously approved these changes Jun 7, 2024

View reviewed changes

Jim8y and others added 2 commits June 7, 2024 21:36

Merge branch 'master' into fix-3296

612497a

revert change to the exception info

512d738

Jim8y dismissed stale reviews from cschuchardt88 and shargon via 512d738 June 7, 2024 14:27

clean using

ae94667

Jim8y added the ready to merge label Jun 7, 2024

AnnaShaleva mentioned this pull request Jun 7, 2024

ApplicationLogs Plugin: do not replace invalid ApplicationLog's stackitem with Null #3303

Open

AnnaShaleva approved these changes Jun 7, 2024

View reviewed changes

Merge branch 'master' into fix-3296

e04e1be

shargon approved these changes Jun 7, 2024

View reviewed changes

cschuchardt88 approved these changes Jun 7, 2024

View reviewed changes

Jim8y mentioned this pull request Jun 11, 2024

[V3.7.5 Checklist] This is a checklist for version 3.7.5 hotfix #3321

Closed

6 tasks

Jim8y added this to the v3.7.5 milestone Jun 11, 2024

NGDAdmin merged commit b68ad51 into neo-project:master Jun 11, 2024
7 checks passed

Jim8y deleted the fix-3296 branch June 11, 2024 03:16

dusmart mentioned this pull request Jun 11, 2024

make it possible to build large objects lazynode/Tanya#33

Merged

superboyiii added a commit to neo-project/neo-modules that referenced this pull request Jun 11, 2024

fix patch from:

19813f5

neo-project/neo#3261 and neo-project/neo#3299

superboyiii added a commit to neo-project/neo-modules that referenced this pull request Jun 12, 2024

v3.7.5 (#913)

8c6b64b

* 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Neo Plugin Bug]fix 3296 #3299

[Neo Plugin Bug]fix 3296 #3299

Jim8y commented Jun 7, 2024

Jim8y commented Jun 7, 2024 •

edited

Loading

Hecate2 commented Jun 7, 2024

dusmart Jun 7, 2024

Jim8y Jun 7, 2024

dusmart Jun 7, 2024

AnnaShaleva Jun 7, 2024

cschuchardt88 Jun 7, 2024

superboyiii commented Jun 7, 2024

AnnaShaleva Jun 7, 2024

Jim8y Jun 7, 2024

AnnaShaleva Jun 7, 2024 •

edited

Loading

Jim8y Jun 7, 2024

AnnaShaleva Jun 7, 2024

Jim8y Jun 7, 2024

AnnaShaleva Jun 7, 2024 •

edited

Loading

AnnaShaleva Jun 7, 2024

Jim8y commented Jun 7, 2024

[Neo Plugin Bug]fix 3296 #3299

[Neo Plugin Bug]fix 3296 #3299

Conversation

Jim8y commented Jun 7, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

Jim8y commented Jun 7, 2024 • edited Loading

Hecate2 commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superboyiii commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Jun 7, 2024

Jim8y commented Jun 7, 2024 •

edited

Loading

AnnaShaleva Jun 7, 2024 •

edited

Loading

AnnaShaleva Jun 7, 2024 •

edited

Loading