ExecutionEngine: collect parameters affecting opcode price

[Turalchik]

This PR adds collection of parameters in the OpcodePriceArgs struct (length, reference count changes, etc.) that affect opcode costs for dynamic pricing. The pricing is calculated in the related PR neo-project/neo#4536.

A partial port of nspcc-dev/neo-go#4087. Ref. nspcc-dev/neo-go#4043.

[codecov]

Codecov Report

❌ Patch coverage is 87.69231% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.39%. Comparing base (99095c2) to head (8d0a4d1).

Files with missing lines	Patch %	Lines
src/Neo.VM/JumpTable/JumpTable.Compound.cs	75.26%	22 Missing and 1 partial ⚠️
src/Neo.VM/JumpTable/JumpTable.Numeric.cs	58.62%	12 Missing ⚠️
src/Neo.VM/JumpTable/JumpTable.Control.cs	91.11%	4 Missing ⚠️
src/Neo.VM/JumpTable/JumpTable.Push.cs	96.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
- Coverage   85.47%   85.39%   -0.08%     
==========================================
  Files          41       42       +1     
  Lines        2720     2993     +273     
  Branches      345      349       +4     
==========================================
+ Hits         2325     2556     +231     
- Misses        304      345      +41     
- Partials       91       92       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AnnaShaleva]

Will submit detailed handlers review later.

[AnnaShaleva]

Will check it one more time together with neo-project/neo#4536.

[AnnaShaleva]

We still need to update parameters construction for Map-related opcodes (ref. nspcc-dev/neo-go#4222); other than that the PR looks legit, let's wait for the update.

[shargon]

Why this is not initialized every time? Is this required?

[AnnaShaleva]

It's required for some of opcodes. For other opcodes price doesn't depend on the operand type.

[shargon]

Could you give me an example please?

[AnnaShaleva]

Sure. CONVERT requires Type since it works differently for Array/Struct and ByteStrings:
https://github.com/Turalchik/neo/blob/f0699631792accf4cdd717073091199f0bd650a3/src/Neo/SmartContract/Fee.cs#L357

PICKITEM requires Type argument since its price depends on whether the operand is a compound type or not:
https://github.com/Turalchik/neo/blob/f0699631792accf4cdd717073091199f0bd650a3/src/Neo/SmartContract/Fee.cs#L368

And APPEND doesn't actually depend on the operand type. RefsDelta and NClonedItems arguments cover possible price fluctuations for different operand types:
https://github.com/Turalchik/neo/blob/f0699631792accf4cdd717073091199f0bd650a3/src/Neo/SmartContract/Fee.cs#L353

[AnnaShaleva]

In general, different combinations of Type, RefsDelta, Length and NClonedItems are used for every opcode. The set of required parameters is defined separately for every opcode via opcode handler investigation and via benchmarks of edge cases in nspcc-dev/neo-go#4218.

[Jim8y]

Looks good to me now. The latest update covers the two RunStats gaps I was concerned about.

[AnnaShaleva]

@neo-project/core this PR does not depend on #576 and may be reviewed&merged in parallel with #576.

[Jim8y]

One blocker remains around faulting dynamic-price paths. Because PostExecuteInstruction now runs from a finally block, opcodes that fault after doing work still reach the dynamic pricing hook, but some of them leave runStats null. I reproduced this with ASSERTMSG on a false condition and with an unhandled THROW: both call PostExecuteInstruction with null stats after consuming their operands. ASSERTMSG loses the message Length used by AssertGas, and THROW loses RefsDelta on the unhandled path. Please populate RunStats before throwing, or otherwise avoid charging these faulting executions with default stats.

[AnnaShaleva]

Please populate RunStats before throwing, or otherwise avoid charging these faulting executions with default stats.

The execution is already FAULT-ed, and the exception is not catchable. From that point, further charging doesn't make sense, so populating RunStats before throwing is not an option to me. At the same time, avoiding charging with default stats requires an additional check in PostExecuteInstruction (check if an uncatchable exception was thrown). I'd prefer not to introduce this check because it complicates PostExecuteInstruction for no good reason.

So I'd leave it as it is.

[Jim8y]

Thanks, that makes sense. I agree this faulting path does not need to block the PR.

[Wi1l-B0t]

Conflicts.

[Jim8y]

SETITEM still seems to miss the first value pop in RefsDelta. With a direct map SETITEM, ReferenceCounter goes 3 -> 0 but RunStats reports RefsDelta = 2; with a retained map it goes 4 -> 3 but reports 0. Should we capture r1 before popping value, or otherwise add that delta?

[Turalchik]

This will be fixed after the merge https://github.com/neo-project/neo-vm/pull/575/changes, in which the value is extracted using the PopNoRef() method:

neo-vm/src/Neo.VM/JumpTable/JumpTable.Compound.cs

Line 478 in e882525

var value = engine.PopNoRef();