Skip to content
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

Improve MaxNestingDepth of WitnessCondition #3761

Merged
merged 10 commits into from
Feb 17, 2025

Conversation

roman-khimov
Copy link
Contributor

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)

@roman-khimov roman-khimov changed the title Maxconditionnesting Improveme MaxNestingDepth of WitnessCondition Feb 13, 2025
@roman-khimov roman-khimov changed the title Improveme MaxNestingDepth of WitnessCondition Improve MaxNestingDepth of WitnessCondition Feb 13, 2025
@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 13, 2025

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 <[email protected]>
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 <[email protected]>
Fix typo.

Signed-off-by: Roman Khimov <[email protected]>
@cschuchardt88
Copy link
Member

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

@roman-khimov
Copy link
Contributor Author

roman-khimov commented Feb 13, 2025

@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
Copy link
Contributor

nan01ab commented Feb 14, 2025

Is it compatible with the previous implementation ?

@roman-khimov
Copy link
Contributor Author

Is it compatible with the previous implementation ?

⬆️

No behavior changes here.

@NGDAdmin 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
Rework 75d1208 to follow neo-project/neo#3761,
let's have a real depth value in this constant.

Signed-off-by: Roman Khimov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants