Improve MaxNestingDepth of WitnessCondition by roman-khimov · Pull Request #3761 · neo-project/neo

roman-khimov · 2025-02-13T16:26:15Z

Description

While digging into nspcc-dev/neo-go#3809 we've checked C# code as well and I think the main problem that got us to that issue is that we have an incorrect MaxNestingDepth. The first patch here adds a UT for nesting, notice that it already has three levels for the "correct" set. But it's completely hidden in the code because the check is not performed at all for items that can not have any children. I think it'd be nice to change it, simplifying the check itself (currently it's duplicated between different types of conditions) and making the MaxNestingDepth have a real number of levels allowed.

Count curly braces in

                        "condition" : {
                           "type" : "And",
                           "expressions" : [
                              {
                                 "hash" : "0x2ba94444d43c9a084a5660982a9f95f43f07422e",
                                 "type" : "CalledByContract"
                              },
                              {
                                 "type" : "Or",
                                 "expressions" : [
                                    {
                                       "type" : "ScriptHash",
                                       "hash" : "0xd2a4cff31913016155e38e474a2c06d08be276cf"
                                    },
                                    {
                                       "hash" : "0xef4073a0f2b305a38ec4050e4d3d28bc40ea63f5",
                                       "type" : "ScriptHash"
                                    }
                                 ]
                              }
                           ]
                        }

if in doubt.

Type of change

No behavior changes here.

[*] Optimization (the change is only an optimization)
[*] Style (the change is only a code style for better maintenance or standard purpose)

cschuchardt88 · 2025-02-13T16:31:41Z

I thought it used to be 2 for max depth for WitnessCondition. I remember seeing this when I built transaction builder. I tried crashing nodes way back when, it was more than 2. Even with RcpServer parameters

This just works as the code works. Signed-off-by: Roman Khimov <roman@nspcc.ru>

1. We have some duplication around "maxNestDepth <= 0" checks. 2. If we're to fix them by moving to DeserializeFrom() and thereby affecting all types of conditions we can discover a simple truth about conditions that was hidden previously: our real supported MaxNestingDepth is 3, not 2. 3. Test_WitnessCondition_Nesting works as before, check it there, we have And->And->Boolean and other nesting cases, 3-4 elements in the chain, not 2. Signed-off-by: Roman Khimov <roman@nspcc.ru>

Signed-off-by: Roman Khimov <roman@nspcc.ru>

Fix typo. Signed-off-by: Roman Khimov <roman@nspcc.ru>

cschuchardt88 · 2025-02-13T16:42:10Z

@roman-khimov please run dotnet format -v diag in the root of repo, than recommit.

roman-khimov · 2025-02-13T18:07:30Z

@roman-khimov please run dotnet format -v diag in the root of repo, than recommit.

Actually that's exactly the formatting I got after dotnet format --no-restore --include <modified files> (--no-restore because I had to use SDK 8.0.406 and that's because of dotnet/sdk#46780). I always try to autoformat, but it's always wrong for me for some reason. Maybe it's a sign I shouldn't be touching C#. But I still do from time to time.

Thanks for fixing this, @shargon.

nan01ab · 2025-02-14T01:31:52Z

Is it compatible with the previous implementation ?

roman-khimov · 2025-02-14T07:02:49Z

Is it compatible with the previous implementation ?

⬆️

No behavior changes here.

Rework 75d1208 to follow neo-project/neo#3761, let's have a real depth value in this constant. Signed-off-by: Roman Khimov <roman@nspcc.ru>

* tests: add UT for nested WitnessCondition This just works as the code works. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify nesting checks, fix MaxNestingDepth 1. We have some duplication around "maxNestDepth <= 0" checks. 2. If we're to fix them by moving to DeserializeFrom() and thereby affecting all types of conditions we can discover a simple truth about conditions that was hidden previously: our real supported MaxNestingDepth is 3, not 2. 3. Test_WitnessCondition_Nesting works as before, check it there, we have And->And->Boolean and other nesting cases, 3-4 elements in the chain, not 2. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify JSON parsing as well Signed-off-by: Roman Khimov <roman@nspcc.ru> * tests: s/WitnessContition/WitnessCondition/ Fix typo. Signed-off-by: Roman Khimov <roman@nspcc.ru> * Format header * Format arrays in UT_WitnessCondition.cs --------- Signed-off-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

@shargon

* Implement NotaryAssisted transaction attribute Close #2896. Use a stub for native Notary contract hash since this contract is not implemented yet. Thus, technically, NotaryAssisted attribute verification will always fail on real network until native Notary is implemented. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: add doc to CalculateNetworkFee method of NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: add NotaryAssisted attributes handler to Gas OnPersist Transactions network fee should be split between Primary node and Notary nodes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: adjust comment to NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: temporary use hard-coded Notary contract hash Once Notary contract is implemented, this hash will be replaced by a proper Notary contract hash. The exact value won't be changed since Notary contract has constant hash as any other native contract. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: replace hard-coded Notary hash value with calculated one No functional changes, just a refactoring for better code readability. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * NeoModules: integrate NotaryAssisted attribute Port the neo-project/neo-modules#884. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: fix XML comment formatting Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * P2P: move NotaryAssisted transaction attribute under D hardfork Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * P2P: move NotaryAssisted transaction attribute under E hardfork D hardfork was occupied by 3.7.5, thus use the next available one. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * tests: fix build errors No functional changes, just build fixes required by updated master branch. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * NotaryAssisted: update copyright date Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: refactor obsolete code Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Persistance: fix UT Related to additional storage entries added by the previous changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Clean comments * Native: improve exception message for Policy's getFee/setFee Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * EC Recover (#3696) * EC Recover * Clean code * Fix ut * Fix ut * Unify hashing * Allow HashAlgorithm.None * Clean comment * Update src/Neo/Cryptography/Crypto.cs * Fix merge * Add None check * Fix GetHash * fix unit testing issues * Clean code * Update src/Neo/SmartContract/Native/CryptoLib.cs --------- Co-authored-by: Jimmy <jinghui@wayne.edu> * Improve MaxNestingDepth of WitnessCondition (#3761) * tests: add UT for nested WitnessCondition This just works as the code works. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify nesting checks, fix MaxNestingDepth 1. We have some duplication around "maxNestDepth <= 0" checks. 2. If we're to fix them by moving to DeserializeFrom() and thereby affecting all types of conditions we can discover a simple truth about conditions that was hidden previously: our real supported MaxNestingDepth is 3, not 2. 3. Test_WitnessCondition_Nesting works as before, check it there, we have And->And->Boolean and other nesting cases, 3-4 elements in the chain, not 2. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify JSON parsing as well Signed-off-by: Roman Khimov <roman@nspcc.ru> * tests: s/WitnessContition/WitnessCondition/ Fix typo. Signed-off-by: Roman Khimov <roman@nspcc.ru> * Format header * Format arrays in UT_WitnessCondition.cs --------- Signed-off-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> * Add Support for Plugin Loading by Assembly (#3755) * Add Support for plugin loading * Added @shargon request of changes --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> * Optimize `StorageKey` creation (#3756) * Optimize key builder * Optimize with GetSpan * Use Span * Clean * Rename * Update src/Neo/SmartContract/KeyBuilder.cs Co-authored-by: nan01ab <yjcc201374@outlook.com> * Update KeyBuilder.cs * @cschuchardt88 feedback * Avoid ToArray in StorageKey constructor * Use sizeof * Optimize ECPoint * Optimize ECPoint * Add ISerializableSpan * Move to StorageKey * Use ReadOnlySpan --------- Co-authored-by: nan01ab <yjcc201374@outlook.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * [`Fix`] Linux File Access (#3769) * Add some stuff to hope once and for all fix the linux workflows * update workflow * update nuget.yml * Change dig to detailed, log too big * Fix targets * revert main.yml * Update .github/workflows/main.yml * disable BuildInParallel --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> * Update src/Neo/SmartContract/Native/PolicyContract.cs * UnitTests: migrate NotaryAssisted test from MSTestV1 to MSTestV2 Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Signed-off-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> Co-authored-by: nan01ab <yjcc201374@outlook.com>

* tests: add UT for nested WitnessCondition This just works as the code works. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify nesting checks, fix MaxNestingDepth 1. We have some duplication around "maxNestDepth <= 0" checks. 2. If we're to fix them by moving to DeserializeFrom() and thereby affecting all types of conditions we can discover a simple truth about conditions that was hidden previously: our real supported MaxNestingDepth is 3, not 2. 3. Test_WitnessCondition_Nesting works as before, check it there, we have And->And->Boolean and other nesting cases, 3-4 elements in the chain, not 2. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify JSON parsing as well Signed-off-by: Roman Khimov <roman@nspcc.ru> * tests: s/WitnessContition/WitnessCondition/ Fix typo. Signed-off-by: Roman Khimov <roman@nspcc.ru> * Format header * Format arrays in UT_WitnessCondition.cs --------- Signed-off-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

@shargon

* Implement NotaryAssisted transaction attribute Close neo-project#2896. Use a stub for native Notary contract hash since this contract is not implemented yet. Thus, technically, NotaryAssisted attribute verification will always fail on real network until native Notary is implemented. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: add doc to CalculateNetworkFee method of NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: add NotaryAssisted attributes handler to Gas OnPersist Transactions network fee should be split between Primary node and Notary nodes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: adjust comment to NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: temporary use hard-coded Notary contract hash Once Notary contract is implemented, this hash will be replaced by a proper Notary contract hash. The exact value won't be changed since Notary contract has constant hash as any other native contract. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: replace hard-coded Notary hash value with calculated one No functional changes, just a refactoring for better code readability. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * NeoModules: integrate NotaryAssisted attribute Port the neo-project/neo-modules#884. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: fix XML comment formatting Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * P2P: move NotaryAssisted transaction attribute under D hardfork Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * P2P: move NotaryAssisted transaction attribute under E hardfork D hardfork was occupied by 3.7.5, thus use the next available one. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * tests: fix build errors No functional changes, just build fixes required by updated master branch. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * NotaryAssisted: update copyright date Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: refactor obsolete code Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Persistance: fix UT Related to additional storage entries added by the previous changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Clean comments * Native: improve exception message for Policy's getFee/setFee Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * EC Recover (neo-project#3696) * EC Recover * Clean code * Fix ut * Fix ut * Unify hashing * Allow HashAlgorithm.None * Clean comment * Update src/Neo/Cryptography/Crypto.cs * Fix merge * Add None check * Fix GetHash * fix unit testing issues * Clean code * Update src/Neo/SmartContract/Native/CryptoLib.cs --------- Co-authored-by: Jimmy <jinghui@wayne.edu> * Improve MaxNestingDepth of WitnessCondition (neo-project#3761) * tests: add UT for nested WitnessCondition This just works as the code works. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify nesting checks, fix MaxNestingDepth 1. We have some duplication around "maxNestDepth <= 0" checks. 2. If we're to fix them by moving to DeserializeFrom() and thereby affecting all types of conditions we can discover a simple truth about conditions that was hidden previously: our real supported MaxNestingDepth is 3, not 2. 3. Test_WitnessCondition_Nesting works as before, check it there, we have And->And->Boolean and other nesting cases, 3-4 elements in the chain, not 2. Signed-off-by: Roman Khimov <roman@nspcc.ru> * WitnessCondition: unify JSON parsing as well Signed-off-by: Roman Khimov <roman@nspcc.ru> * tests: s/WitnessContition/WitnessCondition/ Fix typo. Signed-off-by: Roman Khimov <roman@nspcc.ru> * Format header * Format arrays in UT_WitnessCondition.cs --------- Signed-off-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> * Add Support for Plugin Loading by Assembly (neo-project#3755) * Add Support for plugin loading * Added @shargon request of changes --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> * Optimize `StorageKey` creation (neo-project#3756) * Optimize key builder * Optimize with GetSpan * Use Span * Clean * Rename * Update src/Neo/SmartContract/KeyBuilder.cs Co-authored-by: nan01ab <yjcc201374@outlook.com> * Update KeyBuilder.cs * @cschuchardt88 feedback * Avoid ToArray in StorageKey constructor * Use sizeof * Optimize ECPoint * Optimize ECPoint * Add ISerializableSpan * Move to StorageKey * Use ReadOnlySpan --------- Co-authored-by: nan01ab <yjcc201374@outlook.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * [`Fix`] Linux File Access (neo-project#3769) * Add some stuff to hope once and for all fix the linux workflows * update workflow * update nuget.yml * Change dig to detailed, log too big * Fix targets * revert main.yml * Update .github/workflows/main.yml * disable BuildInParallel --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> * Update src/Neo/SmartContract/Native/PolicyContract.cs * UnitTests: migrate NotaryAssisted test from MSTestV1 to MSTestV2 Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Signed-off-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> Co-authored-by: nan01ab <yjcc201374@outlook.com>

roman-khimov requested review from Jim8y, cschuchardt88, nan01ab and shargon February 13, 2025 16:26

roman-khimov changed the title ~~Maxconditionnesting~~ Improveme MaxNestingDepth of WitnessCondition Feb 13, 2025

roman-khimov changed the title ~~Improveme MaxNestingDepth of WitnessCondition~~ Improve MaxNestingDepth of WitnessCondition Feb 13, 2025

roman-khimov added 4 commits February 13, 2025 19:37

tests: add UT for nested WitnessCondition

40069fb

This just works as the code works. Signed-off-by: Roman Khimov <roman@nspcc.ru>

WitnessCondition: unify JSON parsing as well

c3b5dbb

Signed-off-by: Roman Khimov <roman@nspcc.ru>

tests: s/WitnessContition/WitnessCondition/

4a71e2c

Fix typo. Signed-off-by: Roman Khimov <roman@nspcc.ru>

roman-khimov force-pushed the maxconditionnesting branch from 2547616 to 4a71e2c Compare February 13, 2025 16:37

cschuchardt88 assigned roman-khimov Feb 13, 2025

cschuchardt88 added Waiting for Review Needs Attention labels Feb 13, 2025

Format header

5bc4ec5

cschuchardt88 removed the Needs Attention label Feb 13, 2025

Format arrays in UT_WitnessCondition.cs

d012f62

shargon approved these changes Feb 13, 2025

View reviewed changes

Merge branch 'master' into maxconditionnesting

f8c20d7

cschuchardt88 approved these changes Feb 14, 2025

View reviewed changes

nan01ab approved these changes Feb 14, 2025

View reviewed changes

cschuchardt88 added Ready to Merge and removed Waiting for Review labels Feb 14, 2025

cschuchardt88 and others added 2 commits February 14, 2025 05:57

Merge branch 'master' into maxconditionnesting

ba74b6f

Merge branch 'master' into maxconditionnesting

559f6f7

Merge branch 'master' into maxconditionnesting

6520809

NGDAdmin merged commit 582556a into neo-project:master Feb 17, 2025
6 checks passed

roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request Feb 17, 2025

transaction: fix MaxConditionNesting

d0e47c7

Rework 75d1208 to follow neo-project/neo#3761, let's have a real depth value in this constant. Signed-off-by: Roman Khimov <roman@nspcc.ru>

roman-khimov mentioned this pull request Feb 17, 2025

transaction: fix MaxConditionNesting nspcc-dev/neo-go#3815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve MaxNestingDepth of WitnessCondition#3761

Improve MaxNestingDepth of WitnessCondition#3761
NGDAdmin merged 10 commits intoneo-project:masterfrom
roman-khimov:maxconditionnesting

roman-khimov commented Feb 13, 2025

Uh oh!

cschuchardt88 commented Feb 13, 2025 •

edited

Loading

Uh oh!

cschuchardt88 commented Feb 13, 2025

Uh oh!

roman-khimov commented Feb 13, 2025 •

edited

Loading

Uh oh!

nan01ab commented Feb 14, 2025

Uh oh!

roman-khimov commented Feb 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

roman-khimov commented Feb 13, 2025

Description

Type of change

Uh oh!

cschuchardt88 commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschuchardt88 commented Feb 13, 2025

Uh oh!

roman-khimov commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nan01ab commented Feb 14, 2025

Uh oh!

roman-khimov commented Feb 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cschuchardt88 commented Feb 13, 2025 •

edited

Loading

roman-khimov commented Feb 13, 2025 •

edited

Loading