Skip to content

eth estimategas & eth call does not support block overrides#11267

Merged
svlachakis merged 27 commits into
masterfrom
11183-eth_estimategas-nethermind-does-not-support-block-overrides
Apr 24, 2026
Merged

eth estimategas & eth call does not support block overrides#11267
svlachakis merged 27 commits into
masterfrom
11183-eth_estimategas-nethermind-does-not-support-block-overrides

Conversation

@DarkLord017
Copy link
Copy Markdown
Contributor

@DarkLord017 DarkLord017 commented Apr 20, 2026

Fixes Closes Resolves #11183 , #11171 , #8480

Changes

  • Added BlockOverride to both Eth_Call and Eth_EstimateGas with adding coinbase override
  • So now both endpoint supports 4 parameters instead of three previously
  • Built a unified decorator for BlobBaseFeeOverrides for both simulate & eth_*
  • Added tests for blob , opcode changes and in gasestimate to verify the changes
  • Added test for open issue 8480 , but it was passing now so it is already resolved , but just added that as a test :/

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

Notes on testing

Documentation

Requires documentation update

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes
  • No

Remarks

@DarkLord017 DarkLord017 linked an issue Apr 20, 2026 that may be closed by this pull request
@github-actions github-actions Bot added the rpc label Apr 20, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 20, 2026

Claude finished @DarkLord017's task in 5m 51s —— View job


PR Review: eth_estimateGas & eth_call Block Overrides

  • Read agent rules and coding guidelines
  • Examine changed files
  • Review implementation correctness
  • Review tests
  • Post final review

Summary

The PR adds blockOverride parameter support to eth_call and eth_estimateGas, mirroring how it already works in eth_simulateV1. The core approach is sound: store the override on the TxExecutor instance, use it to default the gas limit before execution, and apply it to the cloned block header before calling into the EVM.


Findings

Medium — Missing overflow guard on GasLimit cast in gas-defaulting path

File: src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.TransactionExecutor.cs, lines 68–71

transactionCall.Gas = _blockOverride?.GasLimit is not null
    ? (long)_blockOverride.GasLimit.Value   // ← unchecked cast
    : searchResult.Value.Object!.GasLimit;

BlockOverride.ApplyOverrides already guards against this with an explicit OverflowException throw when GasLimit > long.MaxValue. The gas-defaulting code here uses the same GasLimit value without the same check. In an unchecked context, (long)ulong.MaxValue = -1, leaving transactionCall.Gas = -1 before ApplyOverrides eventually throws. The exception is still raised (so the call ultimately fails correctly), but the mutation of transactionCall.Gas is inconsistent with the existing pattern.

Suggested fix: add the same guard used in ApplyOverrides:

if (_blockOverride.GasLimit > (ulong)long.MaxValue)
    return ResultWrapper<TResult>.Fail("GasLimit value is too large", ErrorCodes.InvalidInput);
transactionCall.Gas = (long)_blockOverride.GasLimit.Value;

Fix this →


Low — NoBaseFee zeroing vs. blockOverride.BaseFeePerGas ordering (subtle footgun)

In TxExecutor.Execute(BlockHeader, ...):

if (NoBaseFee)
    clonedHeader.BaseFeePerGas = 0;   // ← step 1
clonedHeader.GasUsed = 0;
_blockOverride?.ApplyOverrides(clonedHeader);   // ← step 2 (wins if BaseFeePerGas is set)

ApplyOverrides runs after the NoBaseFee zeroing, so a non-zero blockOverride.baseFeePerGas will override the zero. If a caller omits gasPrice (causing NoBaseFee = true) but then sets baseFeePerGas: 1000 in the block override, the tx's effective gasPrice(0) < baseFee(1000) and the call will fail. This matches the simulate path behaviour (same ordering in SimulateBridgeHelper), so it's consistent. Worth a short comment explaining the intended semantics.


Low — eth_createAccessList not updated

eth_createAccessList also uses TxExecutor (via CreateAccessListTxExecutor) but doesn't expose a blockOverride parameter. Minor inconsistency; probably out of scope for this PR, but worth a follow-up issue.


Low — Simulate regression tests reference issue #8480, no corresponding implementation change

The new Test_eth_simulateV1_block_override_time_is_seen_by_timestamp_opcode tests exercise the existing simulate path (SimulateBridgeHelper.cs line 298 already calls block.BlockOverrides?.ApplyOverrides(result)). These tests appear to be clean regression tests for pre-existing behaviour. A short comment clarifying that would help future readers.


Low — PR description template is mostly unfilled

The "Changes", "Types of changes" checkboxes, and "Testing" section are all blank. Filling these in enables automatic labelling and makes the change easier to review/triage.


What works well

  • Core implementation is minimal and focused — exactly the right scope.
  • _blockOverride stored on the (per-request, non-reused) executor is consistent with how NoBaseFee is already handled.
  • Gas-limit defaulting correctly prefers blockOverride.GasLimit over the real block's gas limit.
  • eth_call block-override test covers NUMBER, TIMESTAMP, and GASLIMIT opcodes via inline bytecode — good coverage.
  • eth_estimateGas test exercises the key use case (baseFeePerGas=0 override to allow gasPrice < realBaseFee).

@DarkLord017
Copy link
Copy Markdown
Contributor Author

@claude review pls
Also should i add more tests and is the impl correct for block overrides and do any other endpoint needs the impl or not ?

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 20, 2026

Claude finished @DarkLord017's task in 5m 37s —— View job


PR Review: eth_estimateGas & eth_call Block Overrides

  • Read agent rules and coding guidelines
  • Examine changed files
  • Review implementation correctness
  • Review tests
  • Check other endpoints that may need block overrides
  • Post final review

Implementation Correctness

The core idea is sound. BlockOverride already existed (used by eth_simulateV1 and debug_*), and the PR correctly threads it through the TxExecutor<T> call chain. Applying overrides after the header clone and after NoBaseFee ensures EVM sees the right context.

One notable ordering: NoBaseFee zeros BaseFeePerGas first, then ApplyOverrides may overwrite it (see inline comment). This is probably correct behavior (explicit override wins), but the intent should be documented.


Findings

High

  • Wrong test assertion in EthRpcModuleTests.EstimateGas.cs:563: JToken.Parse(x).Should().BeEquivalentTo(rawJsonString) is a type mismatch (JToken vs string). The assertion either always passes or fails for the wrong reason. JToken.ToString() also emits indented JSON, not the compact string supplied. See inline comment with fix suggestion.

Medium

  • eth_createAccessList not updated: CreateAccessListTxExecutor uses the same TxExecutor<T> base, and ExecuteTx already accepts blockOverride = null. For consistency with eth_call and eth_estimateGas, eth_createAccessList should also accept and forward a BlockOverride?. This is especially relevant since access lists can depend on block-context-sensitive state (e.g., BLOCKHASH, TIMESTAMP).
  • Missing baseFeePerGas test for eth_call: The existing Eth_call_with_block_override test cases cover NUMBER, TIMESTAMP, and GASLIMIT opcodes — but not BASEFEE. Overriding baseFeePerGas (to allow calls with low-gasPrice txs) is the most common block override use case, and it should have its own test in eth_call, mirroring the one added for eth_estimateGas.

Low

  • Hardcoded address in test ("0x7f554713be84160fdf0178cc8df86f5aabd33397"): use TestItem.AddressA (see inline comment).
  • PR description is blank: type-of-change checkboxes are unchecked, changes list is empty. Please fill these in before merge.

Answers to Your Questions

Is the implementation correct?
Yes, the overall approach is correct. ApplyOverrides is called on the cloned header before EVM execution, so all EVM opcodes (NUMBER, TIMESTAMP, GASLIMIT, BASEFEE, COINBASE, PREVRANDAO) will see the overridden values. The GasLimit override is also correctly applied when defaulting tx.Gas.

Should you add more tests?
Yes — specifically:

  1. A BASEFEE opcode test for eth_call (the opcode returns header.BaseFeePerGas).
  2. A COINBASE/FeeRecipient opcode test would also add coverage.
  3. Consider a test that verifies eth_estimateGas without stateOverride also works (the current test always pairs state + block overrides).

Do other endpoints need block override?
eth_createAccessList is the main one — it already extends TxExecutor<T>, so adding BlockOverride? to its signature in IEthRpcModule.cs and EthRpcModule.cs is a one-liner each. All other single-call RPC endpoints (eth_getBalance, eth_getCode, etc.) operate on a raw block header without going through TxExecutor, so they would need a different approach and are out of scope.

@DarkLord017 DarkLord017 force-pushed the 11183-eth_estimategas-nethermind-does-not-support-block-overrides branch from 888ec52 to 9af95be Compare April 21, 2026 15:36
@github-actions github-actions Bot added the evm label Apr 21, 2026
@DarkLord017
Copy link
Copy Markdown
Contributor Author

@claude review pls
is there a better way to handle blob parameters as it is not in the header , this one's too complicated

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Claude finished @DarkLord017's task in 6m 4s —— View job


PR Review: eth_estimateGas & eth_call Block Overrides

  • Read agent rules and changed files
  • Examine blob parameter handling
  • Review implementation correctness and design
  • Post final review

Answering Your Question: Blob Parameter Handling

Is the decorator approach necessary? Yes — and it's the right design choice. Here's why blob fee is special:

BlockHeader has no BlobBaseFee field. What it has is ExcessBlobGas, from which the per-gas blob fee is derived via the EIP-4844 update formula. To override the blob base fee, you'd need to either (a) invert the formula to back-compute an ExcessBlobGas that produces the target fee — fragile and complex — or (b) intercept the fee calculation, which is exactly what SingleCallBlobBaseFeeCalculatorDecorator does.

The decorator serves two distinct roles:

  1. Total blob cost (for gas purchasing): The decorator computes blobGasUsed × overriddenFeePerGas when the TransactionProcessor calls IBlobBaseFeeCalculator.TryCalculateBlobBaseFee
  2. Per-gas price for fee-cap validation (MaxFeePerBlobGas >= blobBaseFee): Handled separately in CallAndRestore via BlockExecutionContext

The approach is consistent with the existing simulate path (SimulateBlobBaseFeeCalculatorDecorator). It's not over-complicated — it just looks that way because two things need overriding via two channels.


Design Issues Found

Medium — blobBaseFeeOverride flows through two redundant paths

File: src/Nethermind/Nethermind.Facade/BlockchainBridge.cs, Call/EstimateGas/CreateAccessList

// Path 1: set on requestState → decorator handles total blob gas cost
scope.Component.RequestState.BlobBaseFeeOverride = blobBaseFeeOverride;

// Path 2: passed as parameter → CallAndRestore → BlockExecutionContext.BlobBaseFee
TryCallAndRestore(scope.Component, header, tx, false, tracer, blobBaseFeeOverride);

Both paths carry the same value but for different purposes (total cost vs. per-gas cap check). This is correct but confusing — a reader has to dig into both paths to understand why both exist. Since CallAndRestore already receives components (which has RequestState), you could eliminate the parameter by reading components.RequestState.BlobBaseFeeOverride directly there instead:

// In CallAndRestore, instead of the blobBaseFeeOverride parameter:
UInt256? blobBaseFeeOverride = components.RequestState.BlobBaseFeeOverride;

This removes the redundant parameter from TryCallAndRestore/CallAndRestore and has a single source of truth.

Medium — SingleCallBlobBaseFeeCalculatorDecorator duplicates SimulateBlobBaseFeeCalculatorDecorator

The two decorators are ~95% identical — same logic, same formula, different state type:

// SingleCallBlobBaseFeeCalculatorDecorator.cs
if (requestState.BlobBaseFeeOverride is not null)
    return !UInt256.MultiplyOverflow(BlobGasCalculator.CalculateBlobGas(transaction),
        requestState.BlobBaseFeeOverride.Value, out blobBaseFee);

// SimulateBlobBaseFeeCalculatorDecorator.cs — identical logic
if (simulateState.BlobBaseFeeOverride is not null)
    return !UInt256.MultiplyOverflow(BlobGasCalculator.CalculateBlobGas(transaction),
        simulateState.BlobBaseFeeOverride.Value, out blobBaseFee);

A shared interface IBlobBaseFeeOverrideProvider with UInt256? BlobBaseFeeOverride { get; } would let a single generic decorator handle both cases, eliminating the duplication. Not blocking, but worth a follow-up.

Low — BlobBaseFee silently skipped in BlockOverride.ApplyOverrides

ApplyOverrides applies all fields to the header but silently skips BlobBaseFee (because it's not a header field). A comment here would save the next reader from wondering why it's missing:

// BlobBaseFee is not a header field — it's derived from ExcessBlobGas via the EIP-4844
// formula. Override is handled via SingleCallBlobBaseFeeCalculatorDecorator instead.

Low — Test assertion format (previously flagged as "High" — reassessed)

The previous review called JToken.Parse(x).Should().BeEquivalentTo(jsonString) a type mismatch bug. On re-inspection: FluentAssertions.Json does provide a BeEquivalentTo(string) overload for JTokenAssertions that parses the string as JSON before comparing. However, since the library's overload resolution depends on version, it's worth verifying this assertion actually fails when the response differs — run a quick sanity check by temporarily changing the expected string.

Low — PR description is unfilled

The "Changes", type-of-change checkboxes, and "Testing" sections are all blank. Please fill these in — they drive automatic PR labeling.


What Works Well

  • ApplyOverrides is called on the cloned header correctly — EVM opcodes NUMBER, TIMESTAMP, GASLIMIT, BASEFEE, COINBASE, PREVRANDAO all see the override
  • GasLimit overflow guard added in ExecuteTx before the (long) cast matches the guard in ApplyOverrides — consistent
  • Gas defaulting via _blockOverride?.GasLimit correctly caps the tx gas limit before estimation starts
  • eth_estimateGas blob fee test exercises the meaningful case (override forces rejection of insufficient MaxFeePerBlobGas)
  • eth_call block override tests cover the key opcodes inline

Verdict

Mergeable after addressing the two Medium items (or explicitly acknowledging them with rationale). The blob decorator approach is correct — the main improvements are reducing the dual-path redundancy and adding a comment explaining why BlobBaseFee isn't in ApplyOverrides.

Comment thread src/Nethermind/Nethermind.Evm/BlockOverride.cs
public CallOutput Call(BlockHeader header, Transaction tx, Dictionary<Address, AccountOverride>? stateOverride, UInt256? blobBaseFeeOverride, CancellationToken cancellationToken)
{
using Scope<BlockProcessingComponents> scope = processingEnv.BuildAndOverride(header, stateOverride);
scope.Component.RequestState.BlobBaseFeeOverride = blobBaseFeeOverride;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Medium: blobBaseFeeOverride is now set on requestState here (line 155) and passed as a method parameter to TryCallAndRestore (line 159). Both are needed for correctness but for different reasons:

  • requestState.BlobBaseFeeOverride → feeds SingleCallBlobBaseFeeCalculatorDecorator → overrides the total blob gas cost computed inside TransactionProcessor
  • The method parameter → CallAndRestoreBlockExecutionContext.BlobBaseFee → overrides the per-gas price used for the MaxFeePerBlobGas fee-cap validation

The same value flows through two channels. A simpler design: since CallAndRestore already receives components (which holds RequestState), you could drop the parameter from TryCallAndRestore/CallAndRestore entirely and read components.RequestState.BlobBaseFeeOverride there directly. That would give a single source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@claude is this design simplification resolved?

Comment thread src/Nethermind/Nethermind.Facade/SingleCallBlobBaseFeeCalculatorDecorator.cs Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

EVM Opcode Benchmark Diff

Aggregated runs: base=1, pr=1

No significant regressions or improvements detected.

DarkLord017 and others added 2 commits April 22, 2026 02:49
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@DarkLord017
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Claude finished @DarkLord017's task in 7m 56s —— View job


PR Review: eth_estimateGas & eth_call Block Overrides

  • Read agent rules and coding guidelines
  • Read changed files
  • Review implementation correctness
  • Review tests
  • Check other endpoints
  • Post inline comments
  • Post final review

Summary

The design is now cleaner than earlier iterations: SimulateBlobBaseFeeCalculatorDecorator and the new BlobBaseFeeOverrideCalculatorDecorator have been unified via IBlobBaseFeeOverrideProvider, SingleCallRequestState carries the override per-request, and ApplyOverrides is called at the right point on the cloned header. Two new regressions were introduced in the latest commit, and one pre-existing test assertion is fragile.


Findings

High — Duplicate PrevRandao assignment in BlockOverride.ApplyOverrides

File: src/Nethermind/Nethermind.Evm/BlockOverride.cs, line 43

The result.MixHash = PrevRandao line appears twice back-to-back. This is a copy-paste error from when the BlobBaseFee comment was added — the preceding PrevRandao line was accidentally duplicated. Remove line 43.

Fix this →


High — eth_call gas default regressed from gasCap to blockGasLimit

File: src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.TransactionExecutor.cs, line 63

By moving both eth_call and eth_estimateGas into the same TxExecutor.Execute override, the default gas for eth_call (when caller omits gas) changed from gasCap to min(blockGasLimit, gasCap). The regression test Eth_call_without_gas_defaults_to_gas_cap_not_block_gas_limit — specifically added after a Gnosis incident where gasCap=600 M and blockGasLimit=17 M caused silent under-execution — was deleted.

The blockOverride.gasLimit override path is correct, but the non-override fallback for eth_call must remain gasCap. This also affects debug_traceCall and debug_traceBundle which got the same change.

Fix this →


Medium — JToken.BeEquivalentTo(string) assertion is ambiguous

File: src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EstimateGas.cs, line 564

JToken.Parse(withOverride).Should().BeEquivalentTo("""{"jsonrpc":"2.0","result":"0x5208","id":67}""");

The subject is a JToken; the expected value is a string. Whether this performs structural JSON comparison or object equality depends on which FluentAssertions extension is in scope. The explicit form is unambiguous:

JToken.Parse(withOverride)["result"]!.Value<string>().Should().Be("0x5208");

Low — Hardcoded address in test

File: src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EstimateGas.cs, line 553

const string sender = "0x7f554713be84160fdf0178cc8df86f5aabd33397" — use TestItem.AddressA.ToString() for consistency (also makes state-override key match).


Low — PR description is unfilled

Type-of-change checkboxes, changes list, and testing section are blank. These drive automatic labelling and help reviewers triage.


What Works Well

  • IBlobBaseFeeOverrideProvider interface unifies SimulateRequestState and SingleCallRequestState — eliminates the duplication flagged in the previous review.
  • BlobBaseFeeOverrideCalculatorDecorator replaces SimulateBlobBaseFeeCalculatorDecorator cleanly; simulate path uses the same class now.
  • GasLimit > long.MaxValue guard in ExecuteTx before the cast is correct and consistent with ApplyOverrides.
  • Author = FeeRecipient fix ensures COINBASE opcode returns the overridden value (via GasBeneficiary = Author ?? Beneficiary).
  • NoBaseFee removal is safe — CallAndRestore uses CommitAndRestore = Commit | Restore | SkipValidation, so fee validation is skipped for zero-fee calls regardless of BaseFeePerGas.
  • Feeless-call test with positive baseFeePerGas override is correct for the same reason.
  • BlobBaseFee comment in ApplyOverrides explains why the field is absent there.

Verdict: not mergeable until the two High items are resolved — the duplicate PrevRandao line is a clear commit error, and reverting the eth_call gas-cap regression is needed to avoid breaking Gnosis and similar chains.

Comment thread src/Nethermind/Nethermind.Evm/BlockOverride.cs Outdated
Comment thread src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.TransactionExecutor.cs Outdated
DarkLord017 and others added 2 commits April 22, 2026 03:05
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…eTests.EstimateGas.cs

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@DarkLord017
Copy link
Copy Markdown
Contributor Author

@claude re-review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

Claude finished @DarkLord017's task in 3m 31s —— View job


PR Re-Review: eth_estimateGas & eth_call Block Overrides

  • Read agent rules and coding guidelines
  • Read current state of changed files
  • Verify all previous findings
  • Look for new issues
  • Post final review

Summary

All previous High and Medium findings are resolved. The implementation is correct and complete.


Resolved Findings ✅

Finding Prior Severity Status
Missing [Test] on Eth_call_blobBaseFeePerGas_override_test High ✅ Fixed — [Test] present at line 864
eth_call defaults gas to blockGasLimit instead of gasCap High ✅ Fixed — base TxExecutor.Execute no longer sets gas; EnsureDefaults applies gasCap correctly
Deleted BASEFEE opcode regression tests Medium ✅ Fixed — Eth_call_with_base_fee_opcode_should_return_0 and _without_from_address present at lines 270–294
JToken.BeEquivalentTo(string) assertion Medium ✅ Fixed — now ["result"]!.Value<string>().Should().Be("0x5208")
IBlobBaseFeeOverrideProvider unification Medium ✅ Resolved — SimulateGasCalculatorDecorator deleted; single BlobBaseFeeOverrideCalculatorDecorator serves both paths via interface
Dual-path blobBaseFeeOverride redundancy Medium ✅ Resolved — CallAndRestore reads components.RequestState.BlobBaseFeeOverride directly
Eth_call_blobBaseFeePerGas_override_test leaks original ctx.Test Low ✅ Fixed — test now uses using TestRpcBlockchain test = await TestRpcBlockchain.ForTest(...).Build(specProvider) directly
Unused import System.Reflection.Metadata Low ✅ Removed
Duplicate comment line in BlockOverride.cs Low ✅ Fixed
Hardcoded address in test Low ✅ Fixed — uses TestItem.AddressA
GasLimit overflow guard before cast Medium ✅ Correct — ExecuteTx guards before setting _blockOverride

eth_createAccessList

Confirmed out of scope — eth_createAccessList does not support block overrides in go-ethereum either.


Remaining Open Item

Low — No comment on ordering of NoBaseFee zeroing vs ApplyOverrides in TransactionExecutor.cs:47–53

The ordering (zero BaseFeePerGas first, then let ApplyOverrides overwrite it) is intentional — explicit override wins. This was a Low finding from the first review that was never addressed. A one-liner comment would prevent confusion:

// Apply NoBaseFee first; blockOverride.BaseFeePerGas intentionally takes precedence when set.
if (NoBaseFee)
    clonedHeader.BaseFeePerGas = 0;

Not blocking.


What Works Well

  • EstimateGasTxExecutor.Execute correctly sets tx.GasLimit from the override before calling base.Execute, bounding the binary search. CallTxExecutor leaves gas null so EnsureDefaults applies gasCap. These serve different roles and are both correct.
  • BlobBaseFeeOverrideCalculatorDecorator handles total blob cost; CallAndRestore/BlockExecutionContext.BlobBaseFee handles fee-cap validation. Both channels are needed and correctly wired.
  • Block override tests cover NUMBER, TIMESTAMP, GASLIMIT, COINBASE, BASEFEE, and BLOBBASEFEE opcodes.
  • The gas-cap regression test (Eth_call_without_gas_defaults_to_gas_cap_not_block_gas_limit) is present and validates the correct behavior on Gnosis-style chains.
  • Eth_call_blobBaseFeePerGas_override_test is properly isolated with its own TestRpcBlockchain — no resource leak.

Verdict: mergeable. All High and Medium findings are resolved. One Low remains (missing comment on NoBaseFee ordering) — up to the author whether to add it before merge.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

Claude finished @DarkLord017's task in 5m 0s —— View job


PR Review: eth_estimateGas & eth_call Block Overrides

  • Read agent rules and coding guidelines
  • Read changed files and diff vs master
  • Review implementation correctness
  • Review tests
  • Post final review

Summary

This PR correctly adds blockOverride parameter support to eth_call and eth_estimateGas. The implementation has gone through many review iterations and all previously flagged High and Medium issues are resolved. The code is clean, well-structured, and consistent with the existing simulate path.


Resolved Previous Findings ✅

Finding Prior Severity Status
eth_call defaults gas to blockGasLimit instead of gasCap High ✅ Fixed — base TxExecutor only sets gas when override is active; EnsureDefaults applies gasCap for unoverridden eth_call
Missing [Test] on Eth_call_blobBaseFeePerGas_override_test High ✅ Fixed — [Test] present at line 864
Duplicate PrevRandao assignment in BlockOverride.cs High ✅ Fixed
JToken.BeEquivalentTo(string) assertion in EstimateGas test Medium ✅ Fixed — now ["result"]!.Value<string>().Should().Be("0x5208")
IBlobBaseFeeOverrideProvider / decorator duplication Medium ✅ Resolved — single BlobBaseFeeOverrideCalculatorDecorator serves both paths
Dual-path blobBaseFeeOverride redundancy in BlockchainBridge Medium ✅ Fixed — CallAndRestore reads components.RequestState.BlobBaseFeeOverride directly
GasLimit > long.MaxValue overflow guard before cast Medium ✅ Correct — ExecuteTx validates before setting _blockOverride
Unused import System.Reflection.Metadata Low ✅ Removed
Duplicate comment line in BlockOverride.cs Low ✅ Fixed
Hardcoded address in test Low ✅ Fixed — uses TestItem.AddressA
Eth_call_blobBaseFeePerGas_override_test resource leak Low ✅ Fixed — uses using TestRpcBlockchain test directly

Remaining Findings

Low — Unused name parameter in Eth_call_with_block_override test

File: src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs, line 835

Each [TestCase] passes a name string as the first argument, but the test method body never uses it. The standard NUnit pattern for case names is the TestName = attribute property, not a method parameter:

// Instead of: [TestCase("NUMBER opcode returns...", ...)]
// Use:
[TestCase(..., TestName = "NUMBER opcode returns overridden block number")]

Not a functional bug — NUnit silently passes the unused argument.

Low — BeEquivalentTo vs Be on string in Estimate_gas_block_override_gasLimit_bounds_estimation

File: src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EstimateGas.cs, line 635

.Should().BeEquivalentTo("gas limit below intrinsic gas");

On a string, FluentAssertions' BeEquivalentTo does a case-insensitive comparison. The actual message is already lowercase (TransactionProcessor.GasLimitBelowIntrinsicGas has errorDescription: "gas limit below intrinsic gas"), so this passes correctly today — but Should().Be(...) would be more precise and catch any capitalization drift.

Low — Missing comment on NoBaseFee / ApplyOverrides ordering

File: src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.TransactionExecutor.cs, lines 47–53

if (NoBaseFee)
    clonedHeader.BaseFeePerGas = 0;
clonedHeader.GasUsed = 0;
_blockOverride?.ApplyOverrides(clonedHeader);  // may re-set BaseFeePerGas

ApplyOverrides runs after NoBaseFee zeroing, so an explicit blockOverride.baseFeePerGas will override the zero. This is intentional (the new test Eth_call_feeless_with_positive_blockOverride_baseFeePerGas_succeeds documents this works because SkipValidation bypasses the fee-cap check). A one-line comment here would prevent future readers from wondering about the ordering.


What Works Well

  • Gas defaulting is correctly separated: CallTxExecutor leaves gas null → EnsureDefaults applies gasCap. EstimateGasTxExecutor.Execute sets gas from blockOverride.GasLimit (or real block gas limit) before EnsureDefaults — bounding the binary search correctly. The regression test Eth_call_without_gas_defaults_to_gas_cap_not_block_gas_limit is present and validates this.
  • BlobBaseFeeOverrideCalculatorDecorator correctly handles both channels: total blob gas cost (decorator's TryCalculateBlobBaseFee) and per-gas fee-cap validation (BlockExecutionContext.BlobBaseFee in CallAndRestore). Both channels receive the same per-gas price value.
  • IBlobBaseFeeOverrideProvider unifies SimulateRequestState and SingleCallRequestState cleanly — no code duplication between the two paths.
  • All 7 block override fields are covered: NUMBER, TIMESTAMP, GASLIMIT, BASEFEE, FEERECIPIENT/COINBASE, PREVRANDAO — via ApplyOverrides on the cloned header; BLOBBASEFEE — via BlobBaseFeeOverrideCalculatorDecorator.
  • Author = FeeRecipient fix ensures COINBASE opcode returns the overridden value (via GasBeneficiary = Author ?? Beneficiary).
  • GasLimit overflow guard in ExecuteTx (line 75) fires before _blockOverride is stored, making the safe (long) cast in EstimateGasTxExecutor.Execute safe by invariant.
  • CreateAccessList correctly excluded from block override support — consistent with go-ethereum.
  • BlockchainBridge.CreateAccessList parameter order fixed (bool optimize before CancellationToken) and BlockchainBridgeTests.cs updated to match.

Verdict: mergeable. All High and Medium findings from prior reviews are resolved. Three Low items remain — none of them are functional issues.

Comment thread src/Nethermind/Nethermind.Evm/BlockOverride.cs Outdated
Comment thread src/Nethermind/Nethermind.Facade/BlobBaseFeeOverrideCalculatorDecorator.cs Outdated
DarkLord017 and others added 3 commits April 23, 2026 16:50
Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
…Decorator.cs

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
=> Execute(transactionCall, blockParameter, stateOverride);
public ResultWrapper<TResult> ExecuteTx(TransactionForRpc transactionCall, BlockParameter? blockParameter, Dictionary<Address, AccountOverride>? stateOverride = null, BlockOverride? blockOverride = null)
{
if (blockOverride?.GasLimit > (ulong)long.MaxValue)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be linked to config rather than be 2Bn?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in JsonRpcConfig we do have max gas cap for RPC calls

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied pls see @LukaszRozmej @benaadams

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 updates Nethermind’s JSON-RPC eth_call and eth_estimateGas implementations to accept and apply the optional 4th blockOverrides parameter (including coinbase/feeRecipient), and unifies blob base fee override handling across eth_* and eth_simulateV1.

Changes:

  • Extend eth_call / eth_estimateGas signatures to accept BlockOverride and apply overrides to the simulated header.
  • Plumb blobBaseFee overrides through IBlockchainBridge and apply via a shared BlobBaseFeeOverrideCalculatorDecorator.
  • Add/extend tests covering block override fields (number/time/gasLimit/feeRecipient), baseFee override behavior, blob base fee opcode behavior, and a regression test for simulate block time override.

Reviewed changes

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

Show a summary per file
File Description
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/IEthRpcModule.cs Adds optional BlockOverride param to eth_call / eth_estimateGas RPC contract.
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs Wires new optional BlockOverride param into executors.
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.TransactionExecutor.cs Applies block overrides to cloned headers; passes blob base fee override into bridge calls; bounds estimateGas by overridden gasLimit.
src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/Simulate/EthSimulateTestsBlocksAndTransactions.cs Adds regression test verifying blockOverrides.time affects TIMESTAMP in simulate.
src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.cs Adds reusable EVM bytecode for COINBASE opcode tests.
src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs Adds eth_call tests for block override fields and blob base fee override behavior.
src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EstimateGas.cs Adds estimateGas tests for baseFeePerGas override and gasLimit override bounding behavior.
src/Nethermind/Nethermind.Facade/SingleCallRequestState.cs Introduces per-request state for single-call blob base fee override.
src/Nethermind/Nethermind.Facade/Simulate/SimulateRequestState.cs Implements IBlobBaseFeeOverrideProvider for simulate request state.
src/Nethermind/Nethermind.Facade/Simulate/SimulateReadOnlyBlocksProcessingEnvFactory.cs Switches simulate env to shared blob base fee override decorator + provider binding.
src/Nethermind/Nethermind.Facade/Simulate/SimulateGasCalculatorDecorator.cs Removes simulate-specific blob base fee decorator in favor of shared implementation.
src/Nethermind/Nethermind.Facade/IBlockchainBridge.cs Extends Call/EstimateGas/CreateAccessList signatures to accept optional blob base fee override.
src/Nethermind/Nethermind.Facade/IBlobBaseFeeOverrideProvider.cs Adds shared interface used by both simulate and single-call paths.
src/Nethermind/Nethermind.Facade/BlockchainBridge.cs Stores blob base fee override in request state; sets execution context blob base fee accordingly; updates DI wiring.
src/Nethermind/Nethermind.Facade/BlobBaseFeeOverrideCalculatorDecorator.cs Adds shared decorator applying blob base fee overrides when provided.
src/Nethermind/Nethermind.Facade.Test/BlockchainBridgeTests.cs Updates tests for CreateAccessList signature change.
src/Nethermind/Nethermind.Evm/BlockOverride.cs Ensures feeRecipient override also sets Author; documents blob base fee override handling (needs minor comment update).

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

Comment thread src/Nethermind/Nethermind.Evm/BlockOverride.cs Outdated
Comment thread src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs Outdated
DarkLord017 and others added 5 commits April 23, 2026 23:16
…eTests.EthCall.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@DarkLord017
Copy link
Copy Markdown
Contributor Author

@claude review if the merge conflict was applied correctlly

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 24, 2026

Claude finished @DarkLord017's task in 2m 54s —— View job


PR Review: Merge Conflict Resolution Check

  • Read agent rules and coding guidelines
  • Examine git log and merge commit
  • Check diff vs master for conflict markers
  • Review changed files for correctness post-merge
  • Post final review

Merge Resolution: Correct ✅

No leftover conflict markers (<<<<<<<, =======, >>>>>>>) found in any of the 17 changed files. The merge from master (4adecd3) was resolved cleanly. Here's a file-by-file verification:

File Status
BlockOverride.cs ✅ Clean — Author = FeeRecipient fix and BlobBaseFee comment intact
EthRpcModule.TransactionExecutor.cs ✅ Clean — gas cap guard, ApplyOverrides, EstimateGasTxExecutor separation all correct
EthRpcModule.cs ✅ Clean — both eth_call and eth_estimateGas forward blockOverride
IEthRpcModule.cs ✅ Clean — signatures updated with BlockOverride? parameter
BlockchainBridge.cs ✅ Clean — CreateAccessList parameter order fixed (bool optimize before CancellationToken), RequestState.BlobBaseFeeOverride set in all 3 methods
IBlockchainBridge.cs ✅ Clean — optional UInt256? blobBaseFeeOverride added to all 3 methods
SimulateGasCalculatorDecorator.cs ✅ Correctly deleted — replaced by unified BlobBaseFeeOverrideCalculatorDecorator
SimulateReadOnlyBlocksProcessingEnvFactory.cs ✅ Clean — switches to BlobBaseFeeOverrideCalculatorDecorator + IBlobBaseFeeOverrideProvider binding
SimulateRequestState.cs ✅ Clean — implements IBlobBaseFeeOverrideProvider
BlobBaseFeeOverrideCalculatorDecorator.cs ✅ Clean — new file, correct implementation
IBlobBaseFeeOverrideProvider.cs ✅ Clean — new interface file
SingleCallRequestState.cs ✅ Clean — new file, correctly implements interface
BlockchainBridgeTests.cs ✅ Clean — updated for new CreateAccessList(header, tx, null, false) parameter order
Test files (EthCall, EstimateGas, Simulate) ✅ Clean — all new tests present with [Test] attributes

One Low finding

GasCap null guard uses !.Value (Low, pre-existing pattern)

src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.TransactionExecutor.cs:76

if (blockOverride?.GasLimit > (ulong)_rpcConfig.GasCap!.Value)

If GasCap were null (it defaults to 100M but is typed long?), the !.Value throws NullReferenceException. This is consistent with the existing pattern in SimulateTxExecutor.cs (_rpcConfig.GasCap!.Value) so it's not a regression, and the default makes it safe in practice. Not blocking.


All previously flagged High/Medium findings remain resolved

All findings from prior review rounds (gas cap regression, [Test] attribute, JToken assertion, decorator duplication, unused import, duplicate comment line, resource leak) are confirmed resolved in the current HEAD.

Verdict: mergeable. The merge conflict was applied correctly.

@svlachakis svlachakis merged commit bb9a34f into master Apr 24, 2026
438 of 439 checks passed
@svlachakis svlachakis deleted the 11183-eth_estimategas-nethermind-does-not-support-block-overrides branch April 24, 2026 20:07
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.

eth_estimateGas: Nethermind does not support block overrides eth_call: 4th parameter (blockOverrides) not supported

5 participants