fix(evm): address devnet-6 state-gas review findings

[Marchhill]

Changes

Addresses the review findings on #12117:

• Medium — asymmetric clamp in RemoveStateGasRefundFromReservoir: with a net-spill (negative) reservoir, revoking an advanced state-gas refund reclaimed spilled gas and over-deducted StateGasUsed. Now clamped to zero (matching DiscardStateGas), with a regression test.
• Low — ConsumeStateGas now sets the OutOfGas flag when the spill-path consume fails.
• Low — CreateAvailableFromIntrinsic saturates the EIP-7825 cap subtraction instead of wrapping.
• Low — duplicated destroy-finalization blocks in TransactionProcessor extracted into a shared DestroyAccount helper.
• Low — SELFDESTRUCT/EIP-2780 divergence: confirmed intentional (EIP-2780 reprices CALL value costs only); no change needed.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• Refactoring

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

Regression test for the negative-reservoir refund removal; Evm.Test and Blockchain.Test suites pass; devnet-6 consensus behavior covered by the EEST v6.1.1 fixtures in CI.

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

[flcl42]

Reviewed against current origin/master (0f70752) and the applicable EIP-8037, EIP-2780, and EIP-8246 behavior. Four inline findings.

[github-actions]

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: KECCAK256, TLOAD

No significant regressions or improvements detected.

[LukaszRozmej]

Deep review of the state-gas changes. The core fixes check out under adversarial verification: the new clamp in RemoveStateGasRefundFromReservoir inverts the advance exactly (the old unclamped Math.Min zeroed a net-spill hole and could over-deduct StateGasUsed by up to twice the advance), on revert/halt paths the parent's resulting state is bit-identical to the "advance never happened" ground truth, the sticky OutOfGas flag is benign at every ConsumeStateGas caller, the DestroyAccount extraction is behavior-identical at both sites, and the over-cap intrinsic input to CreateAvailableFromIntrinsic is unreachable on all paths (ValidateStatic checks ExceedsCap unconditionally, not just under consensus validation), so the assert→SaturatingSub swap is pure hardening.

Five comments inline: one open design question on the spill-mark propagation rule, and four cleanups.

[LukaszRozmej]

The Math.Min(childState.StateGasSpillRefundAdvanced, unappliedRefund) combinator is an underived choice: when an advance is partially tracked and partially discharged (e.g. P=100, S=40, applied=30), it differs from the alternative Max(0, S - applied) in how much remains revocable on a later revert, and nothing in code or tests pins down which is spec-correct.

Related (pre-existing on master, not introduced here, but this is the code that would fix it): on success paths one refund can be spill-marked twice — DiscardStateGas(trackSpillRefund: true) marks the applied portion against the parent's own spill, while TGasPolicy.Refund also merges the child's marks for the same money. Traced scenario: child carries 100 unrefunded spill imported from a reverted grandchild, receives a 100 advance (marks 100), returns successfully into a parent with its own 100 unrefunded spill → parent ends with StateGasSpillRefunded=200 for one 100-gas refund, so a later halt computes childNetSpill=0 and under-adds StateGasSpillBurned.

A differential test against the EIP-8037 reference (nested revert-with-spill → cross-frame refund → outer halt) would settle both. Happy to defer to a follow-up since the double-marking predates this PR.

[Marchhill]

Agreed the combinator is underived — I've documented it as a conservative bound at the site and would rather settle it (together with the pre-existing double-marking you traced, which is master-side) with the differential test against the EIP-8037 reference in a follow-up, as you offered. Filing that as a tracked follow-up rather than growing this PR; the 8037 fixture suite passes with the current bound.

[LukaszRozmej]

Two notes on the counter set:

1. StateGasRefundPending and StateGasRefundAdvanced are provably equal at every program point — every site increments both with the same delta and always zeroes them together (the one asymmetric reset in RemoveAdvancedStateGasRefund preserves equality). This PR grows the lockstep set to three fields that must be maintained identically across five sites; a future frame-exit path that misses one silently corrupts StateGasSpillRefunded on a later revocation with no assert to catch it — the exact bug class this PR is fixing. Consider collapsing to a single amount plus its spill-tracked component.

2. The new field could use a one-liner like its sibling's comment above, e.g. // Portion of StateGasRefundAdvanced counted into StateGasSpillRefunded; un-marked exactly on revocation (invariant: <= StateGasRefundAdvanced).

[Marchhill]

Both done in b9e493b: StateGasRefundPending is collapsed into StateGasRefundAdvanced (verified lockstep at every site), shrinking the set to two fields with the <= invariant now asserted at revocation, and the new field carries your suggested one-liner.

[LukaszRozmej]

The comment above is garbled — "then usage the credit funded" isn't a sentence (presumably "then revoke from usage what the credit funded").

Also Math.Max(0, Math.Min(amount, gas.StateReservoir)) is Math.Clamp(gas.StateReservoir, 0, amount) — the established idiom in the codebase. If you want to go further, the whole body reduces to a branch-free equivalent (verified against both new tests):

long fromUsed = Math.Min(Math.Max(0, amount - Math.Max(0, gas.StateReservoir)), gas.StateGasUsed);
gas.StateGasUsed -= fromUsed;
gas.StateReservoir -= amount - fromUsed;

[Marchhill]

Fixed the wording and switched to Math.Clamp(gas.StateReservoir, 0, amount). Kept the structured form over the branch-free equivalent — for consensus code I'd rather the three buckets stay legible.

[LukaszRozmej]

This return contract belongs in an XML <returns> on the member (per AGENTS.md member-documentation guidance), and its other half — the trackedSpillRefund parameter on RemoveStateGasRefundFromReservoir below — is undocumented. The two are one protocol split across two members; an implementer of a future gas policy sees neither half in IntelliSense and could wire trackedSpillRefund to the wrong quantity (e.g. the full amount).

[Marchhill]

Documented the protocol as a pair: <returns> on the add ("pass back verbatim as trackedSpillRefund") and <param> on the remove.

[LukaszRozmej]

Since StateGasSpillRefundAdvanced <= StateGasRefundAdvanced holds at every site, this is a dead store whenever the guard above is false, and the asymmetric placement (sibling reset inside the if, this one outside) reads as if the invariant doesn't hold. Suggest moving it inside next to StateGasRefundAdvanced = 0, optionally with a Debug.Assert(vmState.StateGasSpillRefundAdvanced <= vmState.StateGasRefundAdvanced).

[Marchhill]

Moved the reset inside the guard next to its sibling and added the Debug.Assert(StateGasSpillRefundAdvanced <= StateGasRefundAdvanced).

[AnkushinDaniil]

Just flagging for the follow-up: I think this deferred combinator isn't purely internal. On a top-level halt the extra spill-marks flow through StateGasSpillRefunded -> netSpill -> StateGasSpillBurned (which survives ResetForHalt) -> block gas, so an over-attribution here can change header.GasUsed. On success/revert it nets out via the Value/reservoir swap, but the halt path has no clamp. Not blocking, just want to make sure the follow-up covers the halt case too.

[Marchhill]

see followup #12419