Handle https://github.com/neo-project/neo/issues/2528

[Hecate2]

Description

If we use < instead of <=, there is always possibility to bully the GC.

Fixes #2528
VwEAwkpKAfsHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwAQzm8AnXcAbwAl9////0U=

Type of change

• Optimization (the change is only an optimization)
• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

invoke script VwEAwkpKAfsHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwAQzm8AnXcAbwAl9////0U=

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

[Hecate2]

In Neo.VM.Benchmark my time cost for NeoIssue2528 was reduced from 3.81s to 0.146s.
The cause of #2528 is bullying CheckZeroReferred in GC. We can simply avoid GC with <=.

[Hecate2]

And here's another script that bullies the GC even with the fix in this PR.
VwEAwkpKAfoHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwDBwG8AnXcAbwAl9////0U=
PACK and UNPACK changes stack reference dramatically, but CheckZeroReferred is needed to determine whether the count of items is below the limit. This script costs more than 22s and 4_8096_8685 GAS on my machine.
Visual Studio profile diagnostics are attached. s = v.Successors.GetEnumerator(); is consuming much time.
Report20241009-1417.diagsession.zip

[roman-khimov]

VwEAwkpKAfoHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwDBwG8AnXcAbwAl9////0U=

$ time curl -sd '{ "jsonrpc": "2.0", "id": 1, "method": "invokescript", "params": ["VwEAwkpKAfoHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwDBwG8AnXcAbwAl9////0U="] }' https://rpc10.n3.nspcc.ru:10331
{"id":1,"jsonrpc":"2.0","result":{"state":"HALT","gasconsumed":"480968685","script":"VwEAwkpKAfoHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwDBwG8AnXcAbwAl9////0U=","stack":[],"exception":null,"notifications":[]}}

real    0m0,213s
user    0m0,015s
sys     0m0,011s

[shargon]

This require a hardfork

[Hecate2]

This require a hardfork

I think it does not require a hardfork. It's just trying to skip Tarjan to save time.

[shargon]

This require a hardfork

I think it does not require a hardfork. It's just trying to skip Tarjan to save time.

not sure, maybe there are cases where checkZeroRef FAULT the execution

[Hecate2]

VwEAwkpKAfoHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwDBwG8AnXcAbwAl9////0U=

By the way, replacing UNPACK PACK with NEWARRAY0 DROP does not make a DoS, because the new empty array is not added to tracked_item successfully in AddStackReference.

[cschuchardt88]

In Neo.VM.Benchmark my time cost for NeoIssue2528 was reduced from 3.81s to 0.146s.
The cause of #2528 is bullying CheckZeroReferred in GC. We can simply avoid GC with <=.

If you remove the linq expression. That would increase performance dramatically

[Jim8y]

This require a hardfork

I think it does not require a hardfork. It's just trying to skip Tarjan to save time.

not sure, maybe there are cases where checkZeroRef FAULT the execution

Its just affect when and how often the GC will be triggered, no hardfork is required.

[shargon]

This require a hardfork

I think it does not require a hardfork. It's just trying to skip Tarjan to save time.

not sure, maybe there are cases where checkZeroRef FAULT the execution

Its just affect when and how often the GC will be triggered, no hardfork is required.

I think that's not true @Jim8y.

ReferenceCounter.Count <= Limits.MaxStackSize

If ReferenceCounter.Count now is equal, GC is not triggered, before it may have been triggered with a FAULT result.

So maybe now it's HALT, and before was FAULT.

[roman-khimov]

before it may have been triggered with a FAULT result.

Yeah, it can be tricky the way GC works, no guarantees that execution result is the same.

[cschuchardt88]

Can we remove the linq expression instead of using any use the count instead or is that what's being done?

[Hecate2]

Can we remove the linq expression instead of using any use the count instead or is that what's being done?

Maybe it's possible to let each compound item remember a hashset of its subitems on stack. I'm not 100% sure for now.

[Jim8y]

But will GC change the result? isn't it just reconstruct the RC? it will reducing, instead of increasing the RC, so if it is not fault, it will not fault by GC from what i understand. @shargon @roman-khimov

[shargon]

But will GC change the result? isn't it just reconstruct the RC? it will reducing, instead of increasing the RC, so if it is not fault, it will not fault by GC from what i understand. @shargon @roman-khimov

Reduce the checks, but it was increased before, in all the pushes