Fix amsterdam hive tests

[flcl42]

Changes

• Fix refunds
• Account for collisions
• Map errors

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

[github-actions]

EVM Opcode Benchmark Diff

Aggregated runs: base=1, pr=1

No significant regressions or improvements detected.

[LukaszRozmej]

@claude review

[claude]

Claude finished @LukaszRozmej's task in 4m 27s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Analyze changes for correctness, bugs, and style
• Post review feedback

---

Summary

This PR fixes several EIP-8037 (Amsterdam) correctness issues for hive tests: contract creation collision handling, CREATE state-gas charge ordering, delegation refund timing, and code-deposit failure semantics. The changes are non-trivial and touch the critical gas-accounting path.

---

Issues Found

Medium — Parameter naming confusion in RefundOnFailContractCreationBeforeExecution

TransactionProcessor.cs:859

protected virtual GasConsumed RefundOnFailContractCreationBeforeExecution(
    Transaction tx, IReleaseSpec spec, in TGasPolicy gasAfterExecution)

The parameter is named gasAfterExecution but at the call site it receives gasAvailable — the pre-execution gas state. The misleading name makes the logic hard to audit.

// Call site (line 738):
gasConsumed = RefundOnFailContractCreationBeforeExecution(tx, spec, in gasAvailable);

Suggestion: rename to gasAvailable or gasBeforeExecution.

Fix this →

---

Medium — RefundOnFailContractCreationBeforeExecution skips SkipValidation guard

TransactionProcessor.cs:870–873

Both RefundOnFailContractCreation (post-execution) and the new pre-execution variant unconditionally call PayRefund. The existing Refund(...) path uses gasPrice passed via the call chain (ultimately from BuyGas), while these methods reach directly into VirtualMachine.TxExecutionContext.GasPrice. When SkipValidation is set, BuyGas is not called and GasPrice may be stale/zero. This is probably benign today since VirtualMachine.TxExecutionContext is initialised early, but the inconsistency with how RefundOnFailContractCreation obtains gas price is worth unifying.

---

Minor — Implicit assumption: StateReservoir == 0 at collision point

TransactionProcessor.cs:865

long spentGas = tx.GasLimit - TGasPolicy.GetStateReservoir(in gasAfterExecution);

The test Eip8037_top_level_create_collision_counts_only_create_state_as_block_gas asserts SpentGas == gasLimit, which requires GetStateReservoir == 0 at the collision point. A comment explaining why the reservoir is guaranteed to be zero here (all state budget consumed by the CREATE intrinsic charge) would help future readers understand this is intentional.

---

Minor — Duplicate mappings for Withdrawals/Consolidations

BlockchainTestBase.cs:448–465

if (validationError.Contains("WithdrawalsFailed: Contract execution failed", ...))
    return ["BlockException.SYSTEM_CONTRACT_CALL_FAILED"];

if (validationError.Contains("WithdrawalsEmpty: Contract is not deployed", ...))
    return ["BlockException.SYSTEM_CONTRACT_EMPTY"];

if (validationError.Contains("ConsolidationsFailed: Contract execution failed", ...))
    return ["BlockException.SYSTEM_CONTRACT_CALL_FAILED"];

if (validationError.Contains("ConsolidationsEmpty: Contract is not deployed", ...))
    return ["BlockException.SYSTEM_CONTRACT_EMPTY"];

These four mappings collapse to only two distinct outcomes. If the error strings share a stable suffix pattern (e.g. ": Contract execution failed", ": Contract is not deployed"), the four can be merged to two checks — minor cleanup, not blocking.

---

Positive observations

CREATE state gas ordering (EvmInstructions.Create.cs:163): Moving ConsumeStateGas(CreateState) to after the memory-expansion OOG check is correct — if you OOG on memory, the CREATE state charge must not be consumed. The new test Eip8037_create_memory_oog_must_not_charge_create_state_gas correctly covers this.

Code-deposit failure semantics (VirtualMachine.cs): Removing discardCreateStateCharge and the special DiscardStateGas call on code-deposit failure is the right call. The CREATE opcode's state charge belongs to the parent frame and must survive the child's exceptional halt. The renamed test must_keep_create_state_in_parent accurately reflects the corrected semantics.

ApplyCodeInsertRefunds hoisted before execution (TransactionProcessor.cs:220–226): For EIP-8037, applying delegation refunds before execution so the state reservoir can absorb them is clearly the correct ordering, and the comment in IGasPolicy.cs was updated to match.

EffectiveBlockGas logic (GasConsumed.cs): The change from BlockGas > 0 ? BlockGas : SpentGas to BlockGas > 0 || BlockStateGas > 0 ? BlockGas : SpentGas correctly allows zero regular-dimension gas to be reported explicitly when state dimension gas is nonzero (collision case).

RegularGasUsed / ReceiptGasUsed on BlockHeader (BlockHeader.cs): Both are [JsonIgnore], appropriate for transient accounting state that must not appear in JSON serialisation.

Per-tx gas-limit check (BlockValidationTransactionsExecutor.cs:48–51): Adding the GasUsed > GasLimit guard after each transaction (not just at block end) is important for EIP-8037 where two-dimensional gas means the cumulative max(regular, state) can overflow the limit mid-block. The ExceededGasLimit error string maps correctly in the existing normaliser.

Error mapping ordering (BlockchainTestBase.cs): HeaderExcessBlobGasMismatch is checked before HeaderBlobGasMismatch (correct — more specific first), and the two-condition InvalidBlockLevelAccessList substring check precedes the generic one.

---

Minor style note

Commit message typo: "collissions" → "collisions" (non-blocking).

---

Overall the logic is sound and the new tests cover the key scenarios. The main actionable items are the parameter rename and a comment explaining the reservoir invariant.

[LukaszRozmej]

Can changes in TransactionProcessor be somewhat simplified/easier to reason about?

[Copilot]

Pull request overview

This PR updates core EVM transaction execution and block/receipt gas accounting to align with Amsterdam-era spec behavior (notably EIP-8037/EIP-7778), adds/adjusts regression coverage, and improves Ethereum engine/pyspec test handling to fix failing Amsterdam hive tests.

Changes:

• Refactors transaction gas validation to use tracer-provided cumulative gas (regular vs receipt) for correct per-tx allowance under EIP-8037/EIP-7778.
• Adjusts CREATE/code-deposit gas/state-gas accounting and failure/collision handling, including VM execution gas tracking changes.
• Improves test infrastructure and adds Amsterdam pyspec validation/transition fixtures plus expanded EIP-8037/EIP-7778 tests.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Nethermind/Nethermind.Xdc/XdcTransactionProcessor.cs	Updates ValidateGas/ValidateStatic call patterns to match new base signatures (tracer-aware).
src/Nethermind/Nethermind.Taiko/TaikoTransactionProcessor.cs	Updates ValidateStatic override to include tracer and forward correctly.
src/Nethermind/Nethermind.Optimism/OptimismTransactionProcessor.cs	Adjusts Refund signature for updated base method parameter passing.
src/Nethermind/Nethermind.Evm/VirtualMachine.cs	Revises CREATE/code deposit halt semantics and uses ref gas for frame-accurate accounting.
src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs	Core changes: tracer-aware gas allowance validation, EIP-8037 delegation refund handling, refund/failure accounting refactor.
src/Nethermind/Nethermind.Evm/TransactionProcessing/SystemTransactionProcessor.cs	Adapts ValidateGas override to new tracer-aware signature.
src/Nethermind/Nethermind.Evm/TransactionProcessing/GasConsumed.cs	Updates EffectiveBlockGas semantics for EIP-8037 “zero regular gas” cases.
src/Nethermind/Nethermind.Evm/Tracing/ITxTracer.cs	Introduces IBlockGasAccountingTracer for block-local allowance validation.
src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.Create.cs	Reorders CREATE charging so state-gas isn’t consumed on memory OOG.
src/Nethermind/Nethermind.Evm/GasPolicy/IGasPolicy.cs	Updates documentation around EIP-7702 refunds under EIP-8037 timing/semantics.
src/Nethermind/Nethermind.Evm/GasPolicy/EthereumGasPolicy.cs	Adjusts ApplyCodeInsertRefunds to make state refund available for later state-gas consumption.
src/Nethermind/Nethermind.Evm.Test/TestAllTracerWithOutput.cs	Implements IBlockGasAccountingTracer and tracks cumulative regular/receipt gas for tests.
src/Nethermind/Nethermind.Evm.Test/Eip8037Tests.cs	Adds unit coverage for code-insert state refund being usable by later state gas.
src/Nethermind/Nethermind.Evm.Test/Eip8037RegressionTests.cs	Updates/extends regression expectations around CREATE failures/collisions and state-gas accounting.
src/Nethermind/Nethermind.Evm.Test/Eip7778Tests.cs	Adds coverage for allowance based on post-refund receipt gas under EIP-7778.
src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.BlockValidationTransactionsExecutor.cs	Adds explicit block invalidation if cumulative header gasUsed exceeds gasLimit during validation.
src/Nethermind/Nethermind.Blockchain/Tracing/BlockReceiptsTracer.cs	Implements IBlockGasAccountingTracer and exposes cumulative regular/receipt gas.
src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs	Improves engine newPayload test behavior (timeout) and maps validation errors to expected EEST exception IDs.
src/Nethermind/Ethereum.Blockchain.Pyspec.Test/Amsterdam/ValidationTests.cs	Adds Amsterdam BAL validation fixtures for selected pyspec subsets.
src/Nethermind/Ethereum.Blockchain.Pyspec.Test/Amsterdam/TransitionTests.cs	Adds Amsterdam transition pyspec fixtures keyed by EIP wildcard.
.github/workflows/nethermind-tests.yml	Removes TEST_SKIP_HEAVY env from one workflow variant.
.github/workflows/nethermind-tests-flat.yml	Removes TEST_SKIP_HEAVY env from one workflow variant.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[benaadams]

Nicely done!

[benaadams]

Though would want this one merged in first #11182

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (6599a2f) to head (6394d6e).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11151       +/-   ##
===========================================
- Coverage   77.34%        0   -77.35%     
===========================================
  Files        2664        0     -2664     
  Lines      104979        0   -104979     
  Branches    15017        0    -15017     
===========================================
- Hits        81192        0    -81192     
+ Misses      21080        0    -21080     
+ Partials     2707        0     -2707     

☔ View full report in Codecov by Sentry.
📢 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[LukaszRozmej]

@claude re-review

[claude]

Claude finished @LukaszRozmej's task in 5m 56s —— View job

---

Re-review

• Gather context from PR and review history
• Read rule files
• Read changed files (current state)
• Analyze changes and post review feedback

---

Three commits have landed since Review #3: 3cb2056 (fix review items), 540173b (remove tracer dependency from ValidateGas), and 1491f08 (don't check remaining gas if reads match).

---

All previously outstanding items resolved

Item	Status
ValidateGas tracer cast (medium)	✅ Resolved — instance fields in TransactionProcessorBase
s_ prefix inconsistency	✅ Resolved — s_newPayloadParamCounts → NewPayloadParamCounts
BlockGasCalculation delegate undocumented	✅ Resolved — XML doc added
GAS_USED_OVERFLOW dual mappings	✅ Resolved — // alternate error string comment added

---

Commit 540173b — Remove tracer dependency from ValidateGas

This directly addresses the primary concern LukaszRozmej raised across all three reviews.

Approach: _blockCumulativeRegularGas and _blockCumulativeReceiptGas are instance fields in TransactionProcessorBase, reset in SetBlockExecutionContext, and accumulated after each transaction at TransactionProcessor.cs:255–256. ValidateGas now reads these fields directly (TransactionProcessor.cs:504–509):

long gasUsedForAllowance = spec switch
{
    { IsEip8037Enabled: true } => _blockCumulativeRegularGas,
    { IsEip7778Enabled: true } => _blockCumulativeReceiptGas,
    _ => header.GasUsed,
};

This is clean: no interface cast, no tracer coupling, the correctness-critical check owns its own state.

Correctness of accumulation: The counters are updated with spentGas.EffectiveBlockGas and spentGas.SpentGas respectively — these are the out GasConsumed gasConsumed from ExecuteEvmCall, so they capture all paths: normal success, revert, FailContractCreate, and collision (goto Complete). For the collision case, EffectiveBlockGas = 0 (correct — no regular gas charged), and SpentGas = tx.GasLimit - StateReservoir (correct receipt-gas accounting). ✅

SkipValidation guard: Both the counter update (line 253) and the downstream ValidateGas call respect SkipValidation, keeping warmup and CallAndRestore paths clean. ✅

SetBlockExecutionContext(BlockHeader) overload (line 142–147): Correctly delegates to the in BlockExecutionContext overload, so the reset happens there too. ✅

Minor observation — Apply8037DelegationRefunds side effect

TransactionProcessor.cs:226:

Apply8037DelegationRefunds(spec, in intrinsicGas, ref gasAvailable, ref delegationRefunds);

The method zeroes delegationRefunds when EIP-8037 applies, so the same refunds aren't double-applied later in Refund. This is correct but the mutation of delegationRefunds via ref int is a non-obvious side effect. A one-line comment at the call site would make the intent clear:

// For EIP-8037, delegation refunds are applied pre-execution so the state reservoir can absorb them.
// delegationRefunds is zeroed to prevent re-application in Refund().
Apply8037DelegationRefunds(spec, in intrinsicGas, ref gasAvailable, ref delegationRefunds);

Not blocking.

---

Commit 1491f08 — Don't check remaining gas if reads match

ParallelWorldState.cs:335–336:

int surplusSuggestedReads = suggestedReads - generatedReads;
if (surplusSuggestedReads > 0 && gasRemaining < surplusSuggestedReads * GasCostOf.BlockAccessListItem)

The fix is correct. The surplusSuggestedReads > 0 guard prevents a false-positive throw when gasRemaining goes negative due to normal block gas consumption (tracked separately from the BAL gas budget). Without the guard, gasRemaining < (suggestedReads - generatedReads) * item can spuriously fire when surplusSuggestedReads <= 0, throwing "invalid storage reads" instead of the correct "exceeded gas limit" exception. The new ThrowInvalidBlockForGasLimit at BlockValidationTransactionsExecutor.cs:48–51 catches the real gas overflow mid-block. ✅

The regression test (BlockProcessorTests.cs) correctly documents the invariant: when all suggested reads are already in the generated BAL, the gas check must be skipped regardless of gasRemaining.

---

Existing design, confirmed correct

RefundOnFail collision vs post-fail dispatch: static (_, _) => 0 for pre-execution collision (zero regular block gas, state gas committed) and static (s, st) => s - st for post-execution halt (regular gas = spent minus state). Both correct per EIP-8037. ✅

CalculateBlockGas / Calculate8037NonRevertDeduction decomposition: Clean. The three helpers have single responsibilities, and the substate.ShouldRevert ? blockStateGas : Calculate8037NonRevertDeduction(...) split correctly handles the revert (full state gas refunded to block) vs success (only reservoir + spill + intrinsic deducted) cases. ✅

TryChargeCodeDeposit simplification: The merged hasEnoughGas check and !spec.ChargeForTopLevelCreate return are cleaner and equivalent to the old dual-condition structure. ✅

---

Summary

All blocking items from previous reviews have been addressed. The 540173b commit cleanly resolves the tracer-coupling concern. The 1491f08 fix is correct and well-tested. The only remaining item is the non-blocking comment suggestion on Apply8037DelegationRefunds. The PR is ready to merge.

[benaadams]

🥳