Skip to content

fix(devnet-7): reject txs exceeding EIP-8037 intrinsic gas cap; skip BAL no-op code#11635

Open
benaadams wants to merge 5 commits into
masterfrom
devnet-7-fix
Open

fix(devnet-7): reject txs exceeding EIP-8037 intrinsic gas cap; skip BAL no-op code#11635
benaadams wants to merge 5 commits into
masterfrom
devnet-7-fix

Conversation

@benaadams
Copy link
Copy Markdown
Member

Changes

  • Reject transactions at both validation (TxValidator) and execution (TransactionProcessor) when the EIP-8037 intrinsic regular or floor gas exceeds the EIP-7825 per-tx cap. A calldata-heavy tx (e.g. 262k 0xff bytes) can push floor pricing over the cap even when the declared GasLimit is well below it, and must be rejected before execution. Shared check lives on IntrinsicGas.ExceedsCap.

  • BlockAccessList.AddCodeChange short-circuits before invalidating the cached item count and allocating an AccountChanges entry when before equals after, so EIP-7702 authorizations that resolve to no code change (e.g. null-address delegation) no longer create empty AccountChanges entries in the BAL.

  • Regression tests added for both behaviors.

Types of changes

What types of changes does your code introduce?

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

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes

…BAL no-op code changes

Reject transactions at both validation (TxValidator) and execution
(TransactionProcessor) when the EIP-8037 intrinsic regular or floor gas
exceeds the EIP-7825 per-tx cap. A calldata-heavy tx (e.g. 262k 0xff
bytes) can push floor pricing over the cap even when the declared
GasLimit is well below it, and must be rejected before execution.
Shared check lives on IntrinsicGas<TGasPolicy>.ExceedsCap.

BlockAccessList.AddCodeChange short-circuits before invalidating the
cached item count and allocating an AccountChanges entry when before
equals after, so EIP-7702 authorizations that resolve to no code
change (e.g. null-address delegation) no longer create empty
AccountChanges entries in the BAL.

Regression tests added for both behaviors.
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 17, 2026

Claude finished @benaadams's task in 7m 13s —— View job


PR Review

  • Read context and understand the PR
  • Read relevant rule files
  • Review changed files
  • Analyze correctness, edge cases, and potential regressions
  • Post findings

Summary

Two distinct fixes in this PR — the EIP-8037 intrinsic gas cap rejection and the BAL no-op short-circuit — are both logically correct. The tests are well-targeted. One medium finding in the refactored validator path, one low.

Findings

Medium — StateReservoir dropped from mempool gas check (TxValidator.cs:142)

Switching from IntrinsicGasCalculator.Calculate → EthereumIntrinsicGas to EthereumGasPolicy.CalculateIntrinsicGas → IntrinsicGas<EthereumGasPolicy> quietly changes the minimum-gas comparison. The EthereumIntrinsicGas implicit conversion folds StateReservoir into Standard:

new(gas.Standard.Value + gas.Standard.StateReservoir, gas.FloorGas.Value)

…so the old MinimalGas was Max(regularGas + stateReservoir, floorCost).

intrinsicGas.MinimalGas.Value compares only .Value fields and returns Max(regularGas, floorCost) — the intrinsic state cost is silently excluded.

Concrete impact for EIP-8037 CREATE txs: stateReservoir = 183,600 gas. A tx with GasLimit = 53001 passes the new TxValidator but is correctly rejected by TransactionProcessor.ValidateGas (which includes StateReservoir in minGasRequired). Similarly, auth-list txs lose 35,190 × authCount from the threshold.

This weakens the mempool guard without affecting consensus correctness (execution validator still catches it), but allows invalid-at-execution txs to reach block candidates before being dropped.

Low — lost error detail in TransactionProcessor.ValidateGas (TransactionProcessor.cs:645)

The previous GasLimitBelowIntrinsicGas return used .WithDetail("intrinsic gas too low: have X, want Y"). The replacement returns the bare ErrorType via implicit conversion, which yields an empty errorDescription. The TraceLogInvalidTx call still captures the values, so this is purely a debug/logging regression. (The cached static TransactionResult.GasLimitBelowIntrinsicGas exists with "intrinsic gas too low" and would be the right substitution.)


What looks good

  • ExceedsCap placement and gating — checking floor and regular gas against the EIP-7825 cap at both validator and processor boundaries before the GasLimit < MinimalGas check is the correct order.
  • BAL no-op short-circuit — reordering the equality guard ahead of _itemCount = null and GetOrAddAccountChanges is correct and consistent with AddStorageChange; the journal (_changeStream) is untouched so Restore() is unaffected.
  • Test coverageIsWellFormed_Eip8037FloorGasExceedingRegularCap_ReturnsFalse, Eip8037_rejects_tx_when_calldata_floor_exceeds_tx_max_regular_gas, AddCodeChange_with_equal_before_after_does_not_create_account_changes, and Eip7702_null_address_delegation_to_empty_code_records_nonce_without_code_change all test the right paths.
  • Math.Max(regular, floor) in error messages — correct choice for reporting the dominant violation.

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

This PR tightens EIP-8037 transaction validation by rejecting transactions whose intrinsic regular or floor gas exceeds the EIP-7825 cap, and avoids creating empty block access list code-change entries for no-op code updates.

Changes:

  • Adds an IntrinsicGas<TGasPolicy>.ExceedsCap helper and uses it in transaction validation/execution.
  • Skips BlockAccessList.AddCodeChange work when code bytes are unchanged.
  • Adds regression tests for EIP-8037 intrinsic gas cap rejection and BAL no-op code changes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs Adds execution-time intrinsic cap rejection and adjusts intrinsic-gas error return.
src/Nethermind/Nethermind.Evm/GasPolicy/IGasPolicy.cs Adds shared intrinsic gas cap helper.
src/Nethermind/Nethermind.Evm.Test/Eip8037RegressionTests.cs Adds execution regression test for calldata floor gas exceeding the cap.
src/Nethermind/Nethermind.Evm.Test/Eip7928Tests.cs Adds BAL regression coverage for null-address delegation with no code change.
src/Nethermind/Nethermind.Core/BlockAccessLists/BlockAccessList.cs Avoids creating account changes for no-op code changes.
src/Nethermind/Nethermind.Core.Test/BlockAccessLists/BlockAccessListJournalTests.cs Adds unit coverage for no-op code changes not creating account changes.
src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs Adds validation-time intrinsic cap rejection.
src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs Adds validator regression coverage for EIP-8037 floor gas cap rejection.
Comments suppressed due to low confidence (2)

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs:143

  • This now compares the transaction gas limit only against MinimalGas.Value, which ignores EIP-8037 intrinsic state gas stored on intrinsicGas.Standard.StateReservoir. The previous EthereumIntrinsicGas conversion included Standard.Value + StateReservoir, and TransactionProcessor.ValidateStatic still requires Math.Max(standard.Value + stateReservoir, minimal.Value), so set-code/contract-creation transactions with gas between max(regular, floor) and regular + intrinsic state will be accepted by TxValidator but rejected during execution.
        return transaction.GasLimit < intrinsicGas.MinimalGas.Value
            ? TxErrorMessages.IntrinsicGasTooLow

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs:645

  • This drops the detailed error text that included both the supplied gas limit and the required intrinsic gas. TransactionResult.ErrorDescription is surfaced in block production/validation and JSON-RPC errors, so under-intrinsic transactions will now only report the generic “intrinsic gas too low” message even though the exact have/want values are available here.
            if (tx.GasLimit < minGasRequired)
            {
                TraceLogInvalidTx(tx, $"GAS_LIMIT_BELOW_INTRINSIC_GAS {tx.GasLimit} < {minGasRequired}");
                return TransactionResult.ErrorType.GasLimitBelowIntrinsicGas;

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

Comment thread src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs Outdated
Comment thread src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs Outdated
Comment thread src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs Outdated
Comment thread src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs Outdated
@benaadams
Copy link
Copy Markdown
Member Author

@claude re-review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 17, 2026

Claude finished @benaadams's task in 4m 23s —— View job


PR Re-review

  • Read context and understand the PR
  • Read relevant rule files
  • Review changed files (fresh state after fixes)
  • Analyze correctness, edge cases, and potential regressions
  • Post findings

Summary

Both findings from the prior review have been addressed in the Feedback commit. No new issues found.

Previous findings — status

Medium (resolved) — StateReservoir included in mempool gas check

The Feedback commit added:

// Implicit conversion combines Standard + StateReservoir (EIP-8037) before max'ing with FloorGas.
EthereumIntrinsicGas combined = intrinsicGas;
return transaction.GasLimit < combined.MinimalGas
    ? TxErrorMessages.IntrinsicGasTooLow
    : ValidationResult.Success;

The implicit operator (IntrinsicGasCalculator.cs:18-19) produces new(gas.Standard.Value + gas.Standard.StateReservoir, gas.FloorGas.Value), so combined.MinimalGas = Max(regularGas + stateReservoir, floorCost) — matching the TransactionProcessor logic. StateReservoir is no longer silently dropped.

Low (resolved) — diagnostic detail preserved

TransactionProcessor.cs:646 still returns .WithDetail($"intrinsic gas too low: have {tx.GasLimit}, want {minGasRequired}") — just reformatted onto one line. No regression.

What looks good

  • ExceedsCap semantics — correctly compares Standard.Value (regular gas without state reservation) and FloorGas.Value against the EIP-7825 cap. StateReservoir is intentionally excluded from the cap check, which is the right interpretation: EIP-7825 bounds the regular/floor calldata dimension, not the state reservation.
  • Trace messageTX_INTRINSIC_GAS_EXCEEDS_CAP regular={regular} floor={floor} accurately names both failing dimensions.
  • BAL fix ordering — equality guard is now before _itemCount = null and GetOrAddAccountChanges, correctly preventing empty AccountChanges entries for no-op code changes. Journal (_changeStream) is only written after the guard, so Restore() is unaffected.
  • Fork specAmsterdam.Instance correctly has EIP-7623 (Prague), EIP-7825 (Osaka), and EIP-8037 (Amsterdam) all active, making the TxValidator test's Amsterdam.Instance usage correct.
  • Tests — all four regression tests target the right paths and assertions.

The EIP-8037 per-tx cap rejection is semantically "a per-tx maximum was
exceeded", not "intrinsic gas too low" - the failing tx's GasLimit may
be well within the cap; what overflows is the intrinsic regular or
floor dimension vs the cap. Use the existing TxGasLimitCapExceeded:
prefix (already mapped to GAS_LIMIT_EXCEEDS_MAXIMUM for EIP-7825) so
both EIP-7825 and EIP-8037 cap-overflow share a single semantically
accurate exception type.

Upstream EEST fixtures still expect INTRINSIC_GAS_TOO_LOW for these
test cases; bridge the mapping in BlockchainTestBase until those
fixtures are updated.
@benaadams
Copy link
Copy Markdown
Member Author

@claude re-review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 17, 2026

Claude finished @benaadams's task in 3m 45s —— View job


PR Re-review

  • Read context and understand the PR
  • Read relevant rule files
  • Review changed files (fresh state after latest commit)
  • Analyze correctness, edge cases, and potential regressions
  • Post findings

Summary

All findings from prior reviews have been addressed. The latest commit (f93c39b) closes the remaining Copilot concerns about misleading error messages and trace text. One low-level observation below, by design.

Prior findings — status

Medium (resolved) — StateReservoir included in mempool gas check (cd51898)

EthereumIntrinsicGas combined = intrinsicGas; uses the implicit operator that folds StateReservoir into Standard, so combined.MinimalGas = Max(regularGas + stateReservoir, floorCost). Matches the TransactionProcessor logic. ✓

Low (resolved) — error detail preserved (cd51898 / f93c39b)

TransactionProcessor.cs:646 still returns .WithDetail($"intrinsic gas too low: have {tx.GasLimit}, want {minGasRequired}"). The cap path gets its own detail via TxErrorMessages.TxIntrinsicGasExceedsCap. ✓

Copilot concerns — status

Misleading error format (f93c39b)

Error now reads TxGasLimitCapExceeded: Intrinsic gas (regular X, floor Y) exceeded cap of Z. — accurately names both failing dimensions, and the TxGasLimitCapExceeded: prefix routes correctly through the existing EEST exception mapping. ✓

Trace message named wrong dimension (cd51898)

Trace message is now TX_INTRINSIC_GAS_EXCEEDS_CAP regular={regular} floor={floor} > {cap} — covers both dimensions. ✓

EEST bridge mapping (f93c39b)

BlockchainTestBase.cs line 465 maps INTRINSIC_GAS_TOO_LOWTxGasLimitCapExceeded: Intrinsic gas. The matching logic in MapValidationErrorsToEestExceptions collects all pattern matches (not first match), so a single actual error string:

TxGasLimitCapExceeded: Intrinsic gas (regular X, floor Y) exceeded cap of Z.

maps to both GAS_LIMIT_EXCEEDS_MAXIMUM and INTRINSIC_GAS_TOO_LOW. The AssertValidationError check requires expected.Any(e => mapped.Contains(e)), so fixtures expecting either name pass. The bridge will work until EEST fixtures are updated to emit GAS_LIMIT_EXCEEDS_MAXIMUM instead. ✓

Low (by design) — GasLimitBelowIntrinsicGas reused for cap case

TransactionProcessor.cs:628 returns ErrorType.GasLimitBelowIntrinsicGas for the intrinsic-cap failure. Two downstream callers are affected:

  • GasEstimator.IsGasRelatedFailure classifies this as a gas-related failure and will binary-search to the block gas limit before giving up. The tx is unsalvageable by raising the gas limit, so the estimator wastes iterations. Not a correctness issue — just an inefficiency for a tx that should never succeed.
  • SimulateTxExecutor maps GasLimitBelowIntrinsicGas → ErrorCodes.IntrinsicGas for JSON-RPC, which is a reasonable mapping since the root cause is intrinsic gas exceeding a limit.

This is intentional — no new ErrorType variant was introduced. The ErrorDescription string (TxGasLimitCapExceeded: Intrinsic gas...) carries the precise failure reason for any consumer that reads it.

What looks good

  • ExceedsCap semantics — checks Standard.Value (regular gas without state reservation) and FloorGas.Value independently against the cap. StateReservoir is deliberately excluded from the cap comparison: EIP-7825 bounds the regular/floor calldata dimension, not the state reservation component. ✓
  • BAL short-circuit ordering — equality guard before _itemCount = null and GetOrAddAccountChanges, consistent with AddStorageChange. Journal write only happens after the guard, so Restore() is unaffected. ✓
  • combined.MinimalGas after cap check — the ExceedsCap check gates before the implicit EthereumIntrinsicGas conversion and MinimalGas comparison, so cap-violated txs never reach the declared-gas-limit check. ✓
  • Tests — all four regression tests (IsWellFormed_Eip8037FloorGasExceedingRegularCap_ReturnsFalse, Eip8037_rejects_tx_when_calldata_floor_exceeds_tx_max_regular_gas, AddCodeChange_with_equal_before_after_does_not_create_account_changes, Eip7702_null_address_delegation_to_empty_code_records_nonce_without_code_change) target the right paths. ✓

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.

2 participants