Skip to content

Don't return state gas#11031

Merged
flcl42 merged 2 commits into
masterfrom
fix-gas
Apr 7, 2026
Merged

Don't return state gas#11031
flcl42 merged 2 commits into
masterfrom
fix-gas

Conversation

@flcl42
Copy link
Copy Markdown
Contributor

@flcl42 flcl42 commented Apr 3, 2026

Changes

  • Don't reuse state gas spent on create failures

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

github-actions Bot commented Apr 3, 2026

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: MULMOD

Improvements (1)

Opcode Base Median (ns) PR Median (ns) Delta Abs Δ (ns) Base CV PR CV Threshold Uncertainty Effective
MULMOD 289.548 233.934 -19.21% 55.614 8.4% 8.6% ±5.0% ±8.2% ±17.2%

@LukaszRozmej
Copy link
Copy Markdown
Member

@claude review
@benaadams review ;)

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 4, 2026

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


PR Review: Don't return state gas

  • Gathering context and reading changed files
  • Review core gas accounting logic
  • Review tests
  • Review interface/API changes
  • Final assessment

Summary

This PR fixes a correctness bug under EIP-8037: state gas consumed during a failed CREATE (code deposit failure, invalid code, or oversized code) was incorrectly contributing to block-state gas accounting even though the state changes were reverted. The fix ensures that reverted state gas is fully discarded from block accounting.

The PR also fixes a related bug in ValidateGas where EIP-8037 transactions were being checked against header.GasLimit - header.GasUsed, which is incorrect because header.GasUsed in EIP-8037 can track the state dimension (which can be larger than regular gas), unfairly restricting subsequent transactions' gas limits.


Core Logic: VirtualMachine.cs + EthereumGasPolicy.cs

TryChargeAndDepositCode (VirtualMachine.cs:451-454)

bool discardCreateStateCharge = invalidCode || (spec.LimitCodeSize && callResult.Output.Length > spec.MaxCodeSize);
// ...
TGasPolicy.RevertRefundToHalt(ref _currentState.Gas, in previousState.Gas, previousState.InitialStateReservoir);
if (spec.IsEip8037Enabled && discardCreateStateCharge)
{
    TGasPolicy.DiscardStateGas(ref _currentState.Gas, GasCostOf.CreateState, stateGasFloor: 0);
}

Concern: OOG code deposit not covered. The discardCreateStateCharge flag is false when a code deposit fails purely due to out-of-gas (valid, non-oversized code). In that case, the parent's CreateState charge is NOT discarded from state accounting. This appears intentional (OOG deposit means the attempt was real), but it should be documented as an explicit decision, since it's an asymmetry between invalidCode/oversized-code failures and OOG failures. Is there a spec reference for this distinction?

DiscardStateGas (EthereumGasPolicy.cs:230-236)

public static void DiscardStateGas(ref EthereumGasPolicy gas, long amount, long stateGasFloor)
{
    long newFloor = Math.Max(0, stateGasFloor);
    gas.StateGasUsed = Math.Max(gas.StateGasUsed - amount, newFloor);
}

This correctly reduces StateGasUsed without restoring StateReservoir (the gas remains "paid" as a penalty but doesn't count as committed state). The stateGasFloor: 0 call-site floor is correct here since the parent never committed any state from the failed child.


TransactionProcessor.cs: RefundOnFailContractCreation

Before:

long blockGas = spentGas - intrinsicState;
long blockStateGas = TGasPolicy.GetStateGasUsed(in gasAfterExecution);

After:

long revertedExecutionStateGas = Math.Max(0, TGasPolicy.GetStateGasUsed(in gasAfterExecution) - intrinsicState);
long blockGas = spentGas - intrinsicState - revertedExecutionStateGas;
long blockStateGas = 0;

The semantic is clear: for a failed top-level CREATE tx, all state is reverted, so blockStateGas = 0. The revertedExecutionStateGas term correctly removes initcode state ops (e.g., SSTORE, new account creation) from the regular block gas count, since those state costs spill into regular gas tracking but should be excluded from block_regular.

Minor: blockStateGas = 0 is correct for all failing CREATE txs, but if the comment above the GasConsumed constructor mentioned why (e.g., // All state reverted: no committed state gas), it would be clearer.


TransactionProcessor.cs: ValidateGas for EIP-8037

long maxTransactionGasLimit = spec.IsEip8037Enabled
    ? header.GasLimit
    : header.GasLimit - header.GasUsed;

Under EIP-8037, header.GasUsed tracks the state dimension (which can exceed regular gas for state-heavy transactions), so header.GasLimit - header.GasUsed would incorrectly underestimate remaining regular gas capacity. Allowing tx.GasLimit up to the full header.GasLimit is correct IF block-level regular gas capacity is enforced elsewhere (e.g., at the block validation/building layer).

Question: Is there a separate enforcement that the sum of blockGas across all transactions in a block doesn't exceed header.GasLimit? Without that, this change could allow a block to process more regular gas than its limit. The test Eip8037_block_validation_must_not_use_header_max_gas_used_as_remaining_tx_budget validates the per-tx side, but not the cumulative block side.


Interface change: IGasPolicy.cs + DiscardStateGas

Well-structured addition. The default no-op implementation for pre-EIP-8037 policies is correct — those policies don't have a StateGasUsed field to discard from.

Minor: ValidateGas parameter rename adds IReleaseSpec spec — the XdcTransactionProcessor override uses IReleaseSpec _ (using _ to indicate the param is unused locally, because it resolves its own spec). Fine as written, but _spec would be slightly clearer.


Tests

The test changes are substantial and appropriate. Key behavioral reversals:

  • Eip8037_code_deposit_failure_must_include_initcode_state_gas_in_block_accumulator → renamed to ..._discard_... with opposite assertions. The semantic inversion is correct: failed CREATE → state reverted → BlockStateGas = 0. The previous behavior was a bug.
  • New tests cover: nested CREATE failure, oversized initcode, static context CREATE, and the ValidateGas block-budget fix. Coverage is comprehensive.

Hard-coded gas values in tests (e.g., Is.EqualTo(259_698), Is.EqualTo(226_930)) are fragile if gas costs change in future specs, but this is consistent with the existing test style in the file.


Amsterdam/Constants.cs

BalArchiveVersion bumped from v5.5.1v5.6.1. Presumably this includes updated test vectors reflecting the new semantics — worth confirming the new fixtures actually test the create-failure state-gas accounting cases.


Overall

The logic is sound and the direction is correct. The two questions worth resolving before merge:

  1. Is block-level regular gas capacity enforced elsewhere under EIP-8037 to prevent overrun when tx.GasLimit > header.GasLimit - header.GasUsed is allowed?
  2. Is the OOG-code-deposit case (discardCreateStateCharge = false) intentionally excluded from DiscardStateGas? If so, a comment would help future readers.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts EIP-8037 gas/state-gas accounting so reverted/failed CREATE paths don’t incorrectly contribute to committed block state gas (and updates block validation accordingly).

Changes:

  • Update CREATE/code-deposit failure handling to discard specific state-gas charges from block-state accounting under EIP-8037.
  • Modify transaction gas validation to account for EIP-8037’s block gas accounting semantics.
  • Add/extend EIP-8037 regression tests and bump Amsterdam pyspec fixture archive version.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Nethermind/Nethermind.Xdc/XdcTransactionProcessor.cs Adapts ValidateGas override to new base signature and forwards correct spec.
src/Nethermind/Nethermind.Evm/VirtualMachine.cs Adds discard logic for CREATE state charges on specific code-deposit failures.
src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs Updates gas validation for EIP-8037 and adjusts failed contract-creation gas accounting.
src/Nethermind/Nethermind.Evm/TransactionProcessing/SystemTransactionProcessor.cs Updates ValidateGas override to new signature.
src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.Create.cs Adjusts CREATE static/oversized-initcode charging behavior under EIP-8037.
src/Nethermind/Nethermind.Evm/GasPolicy/IGasPolicy.cs Introduces DiscardStateGas hook for policies.
src/Nethermind/Nethermind.Evm/GasPolicy/EthereumGasPolicy.cs Implements DiscardStateGas for Ethereum gas policy.
src/Nethermind/Nethermind.Evm.Test/Eip8037RegressionTests.cs Adds regression coverage for nested CREATE failures, static-context CREATE, and block validation behavior.
src/Nethermind/Ethereum.Blockchain.Pyspec.Test/Amsterdam/Constants.cs Bumps BAL fixture archive version for Amsterdam pyspec tests.

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

Comment thread src/Nethermind/Nethermind.Evm/VirtualMachine.cs
@flcl42 flcl42 marked this pull request as ready for review April 6, 2026 07:15
Copy link
Copy Markdown
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I'm lost with all this gas shit now. Can we not do 8037?

@flcl42 flcl42 merged commit c58978a into master Apr 7, 2026
424 checks passed
@flcl42 flcl42 deleted the fix-gas branch April 7, 2026 15:04
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.

4 participants