fix(hive): use 60M default gas limit for testing_buildBlockV1

[newmanifold]

Closes #11156

Upstream fixture fix merged: ethereum/execution-apis#783

Changes

• Add 60M default gas limit target in TestingRpcModule when TargetBlockGasLimit is not configured
• Remove testing_buildBlockV1 tests from known-failing hive tests
• Add unit tests verifying gas limit target behavior (default fallback and config override)

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

• Unit tests: 10/10 pass (TestingRpcModuleTests)
• Hive rpc-compat testing_buildBlockV1: 5/5 pass locally

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[newmanifold]

@claude review please

[claude]

Claude finished @newmanifold's task in 2m 36s —— View job

---

PR Review: fix(testing-rpc): use 60M default gas limit for testing_buildBlockV1

Tasks

• Read repository rules and guidelines
• Read changed files
• Analyze correctness and edge cases
• Check test coverage
• Post findings

---

Summary

Verdict: ✅ Mergeable — 0 Critical · 0 High · 0 Medium · 3 Low

The fix is correct, focused, and well-reasoned. Adding a local 60M fallback in TestingRpcModule rather than changing the global TargetBlockGasLimit default is the right call — it's the minimum-blast-radius fix and doesn't risk changing production block production behavior for nodes that already ship with an explicit 60M config.

The two failing hive tests (build-block-empty-transactions, build-block-with-transactions) are correctly removed from known-failing-hive-tests.txt.

---

Findings

Low — Shallow BlocksConfig copy silently drops other properties

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcModule.cs:50

private readonly TargetAdjustedGasLimitCalculator _gasLimitCalculator = new(
    specProvider, new BlocksConfig { TargetBlockGasLimit = blocksConfig.TargetBlockGasLimit ?? DefaultTestingGasLimit });

A new BlocksConfig is constructed carrying only TargetBlockGasLimit — all other IBlocksConfig properties silently fall back to their defaults. TargetAdjustedGasLimitCalculator currently only reads TargetBlockGasLimit, so this is safe today. However the coupling is implicit: if the calculator ever reads another property (e.g. future gas-pricing fields), the behaviour in TestingRpcModule will silently diverge from the actual config.

A slightly more robust option is to forward the real blocksConfig and only override the target via a thin wrapper, or to make the intent explicit with a comment. Not a blocker, but worth considering if the team expects this module to evolve.

Fix this →

---

Low — Verbose multi-line XML doc on internal const

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcModule.cs:41-47

The 5-line <summary> block on an internal const is over-documented relative to project style (coding-style.md: "Use documentation comments for all public APIs"). The why is genuinely non-obvious (cross-client parity), so a comment is warranted, but a single // line would suffice:

// Matches geth (miner.Config.GasCeil) and erigon (DefaultBlockGasLimit) which both default to 60M.
internal const long DefaultTestingGasLimit = 60_000_000;

---

Low — Test assertion verifies direction only, not exact value

src/Nethermind/Nethermind.Merge.Plugin.Test/TestingRpcModuleTests.cs:199-217

Gas_limit_uses_configured_or_default_target asserts that the gas limit moved in the expected direction (greater/less than parent) rather than comparing against the exact value TargetAdjustedGasLimitCalculator.GetGasLimit() would produce. This is a valid smoke test but would silently pass if, say, the calculator were bypassed entirely and any non-parent value were used. Consider strengthening to:

long expected = new TargetAdjustedGasLimitCalculator(specProvider, blocksConfig).GetGasLimit(parentHeader);
payloadResult.ExecutionPayload.GasLimit.Should().Be(expected);

Not a blocker since the direction check already proves the fix is wired up correctly.

---

Notes on WIP status

The PR remarks note a pending team decision on whether the fallback should live in TestingRpcModule (this PR) or as a global TargetBlockGasLimit default. From a correctness standpoint, the local approach is clearly safer: all shipped configs already set 60M explicitly, so changing the global default would be a no-op for production and only risks test-environment behaviour. Recommending the local approach be accepted.

[newmanifold]

@claude review again

[claude]

Claude finished @newmanifold's task in 2m 34s —— View job

---

PR Review: fix(testing-rpc): use 60M default gas limit for testing_buildBlockV1

Tasks

• Read repository rules and guidelines
• Read changed files
• Analyze correctness and edge cases
• Check test coverage
• Post findings

---

Summary

Verdict: ✅ Mergeable — 0 Critical · 0 High · 0 Medium · 1 Low

The fix is correct. Replacing IGasLimitCalculator with IBlocksConfig and constructing a local TargetAdjustedGasLimitCalculator with the ?? 60M fallback is the right approach. DI compatibility is maintained — IBlocksConfig is registered globally and resolves cleanly.

Corrections to the previous review:

• Previous Low Feature/networking #2 ("verbose 5-line XML doc on internal const") — already resolved in the current code; the internal const has a single // comment.
• Previous Low Netcore #3 ("test assertion verifies direction only") — inaccurate; the test uses .Should().Be(expectedGasLimit) for an exact value comparison. The TestSpecProvider(Osaka.Instance) and the mocked specProvider.GetSpec(Arg.Any<ForkActivation>()) both return Osaka.Instance, so both calculators produce identical output.

---

Findings

Low — Shallow BlocksConfig copy silently drops other properties

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcModule.cs:42-43

private readonly TargetAdjustedGasLimitCalculator _gasLimitCalculator = new(
    specProvider, new BlocksConfig { TargetBlockGasLimit = blocksConfig.TargetBlockGasLimit ?? DefaultTestingGasLimit });

TargetAdjustedGasLimitCalculator.GetGasLimit currently only reads TargetBlockGasLimit from IBlocksConfig, so the shallow copy is safe today. If the calculator ever reads another property, TestingRpcModule will silently diverge. A minimal guard would be a comment, or forward the real blocksConfig via a thin wrapper that overrides just the null case:

// Only TargetBlockGasLimit is read by TargetAdjustedGasLimitCalculator today; update if that changes.
private readonly TargetAdjustedGasLimitCalculator _gasLimitCalculator = new(
    specProvider, new BlocksConfig { TargetBlockGasLimit = blocksConfig.TargetBlockGasLimit ?? DefaultTestingGasLimit });

Not a blocker.

---

[claude]

Claude finished @newmanifold's task in 2m 1s —— View job

---

PR Review: fix(hive): use 60M default gas limit for testing_buildBlockV1

Tasks

• Read repository rules and guidelines
• Read changed files
• Analyze correctness and edge cases
• Check test coverage
• Post findings

---

Summary

Verdict: ✅ Mergeable — 0 Critical · 0 High · 0 Medium · 1 Low

The fix is correct. The change from injecting IGasLimitCalculator directly to accepting IBlocksConfig and constructing a local TargetAdjustedGasLimitCalculator with ?? 60M is the right approach. The IGasLimitCalculator DI registration in MergePlugin.cs (line 341) remains valid — it is still consumed by PostMergeBlockProducerFactory and PostMergeBlockProducer, so the removal of IGasLimitCalculator from TestingRpcModule's constructor does not cause a dead-registration.

Correctness notes:

• When TargetBlockGasLimit is null, the old TargetAdjustedGasLimitCalculator returned parentGasLimit unchanged (no target → no adjustment). The fix correctly installs a 60M target so the calculator moves toward 60M as hive fixtures expect.
• Gas limit will only approach 60M gradually (bounded by parentGasLimit / GasLimitBoundDivisor per block), but that's consistent with EIP-1559 behavior and is what the hive fixture tests verify.
• Three entries correctly removed from known-failing-hive-tests.txt.
• Previous review findings on verbose XML docs and direction-only assertions are resolved: the constant now has a single // comment, and tests assert exact values via expectedCalculator.GetGasLimit(parentHeader).

---

Findings

Low — Shallow BlocksConfig copy silently drops other properties

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcModule.cs:42-43

private readonly TargetAdjustedGasLimitCalculator _gasLimitCalculator = new(
    specProvider, new BlocksConfig { TargetBlockGasLimit = blocksConfig.TargetBlockGasLimit ?? DefaultTestingGasLimit });

TargetAdjustedGasLimitCalculator.GetGasLimit currently only reads TargetBlockGasLimit, so this is safe today. If the calculator ever reads another IBlocksConfig property, TestingRpcModule will silently use defaults rather than the actual node config. A minimal guard is a note comment, or alternatively forward the real blocksConfig only when its TargetBlockGasLimit is non-null:

// Only TargetBlockGasLimit is read by TargetAdjustedGasLimitCalculator today; review if that changes.
private readonly TargetAdjustedGasLimitCalculator _gasLimitCalculator = new(
    specProvider, new BlocksConfig { TargetBlockGasLimit = blocksConfig.TargetBlockGasLimit ?? DefaultTestingGasLimit });

Not a blocker — same pattern already exists in the codebase.

Fix this →

---

[newmanifold]

Other alternative

1. is to use 60m default gas limit everywhere - matching geth and erigon.
2. Update both execution-api's and Hive's config for nethermind client to start with 60m target gas limit
   LMK if we need to go that route?

[claude]

Claude finished @newmanifold's task in 2m 36s —— View job

---

PR Review: fix(hive): use 60M default gas limit for testing_buildBlockV1

Tasks

• Read repository rules and guidelines
• Read changed files (latest DI/KeyFilter refactor)
• Analyze DI wiring and KeyFilter usage
• Check test coverage
• Post findings

---

Summary

Verdict: ✅ Mergeable — 0 Critical · 0 High · 0 Medium · 1 Low

The DI refactor correctly addresses LukaszRozmej's feedback. The keyed-singleton approach isolates the testing calculator from the global IGasLimitCalculator, and the [KeyFilter] injection works correctly.

DI wiring is sound:

• AddKeyedSingleton<IGasLimitCalculator>("testing", ctx => ...) registers with .Named<T>("testing") — in Autofac, Named is Keyed with a string, so they are the same mechanism.
• RegisterSingletonJsonRpcModule<ITestingRpcModule, TestingRpcModule>() calls AddSingleton<T, TImpl>() → CommonNethermindConfig() → WithAttributeFiltering(), which enables the [KeyFilter(GasLimitCalculatorKey)] constructor parameter to resolve correctly.
• Plugin extensibility is maintained: any plugin can re-register IGasLimitCalculator keyed "testing" in its own module and override the default, without touching the global IGasLimitCalculator.

Corrections to previous review:

• The factory lambda in AddKeyedSingleton is a necessary exception to the di-patterns anti-pattern rule (ctx => new Foo(...)) because the override of TargetBlockGasLimit ?? Default can only be expressed at factory time, not via type-based registration. No structural alternative exists.

---

Findings

Low — Shallow BlocksConfig copy (unchanged from previous reviews)

src/Nethermind/Nethermind.Merge.Plugin/MergePlugin.cs:345-348

new TargetAdjustedGasLimitCalculator(
    ctx.Resolve<ISpecProvider>(),
    new BlocksConfig { TargetBlockGasLimit = ctx.Resolve<IBlocksConfig>().TargetBlockGasLimit ?? TestingRpcModule.DefaultTestingGasLimit })

TargetAdjustedGasLimitCalculator currently only reads TargetBlockGasLimit from IBlocksConfig, so the shallow copy is safe today. All other BlocksConfig properties silently fall back to their defaults rather than reflecting the actual node config. A guarding comment would prevent a future reader from changing TargetAdjustedGasLimitCalculator to read another property without noticing the divergence:

// Only TargetBlockGasLimit is read by TargetAdjustedGasLimitCalculator; review if that changes.
new BlocksConfig { TargetBlockGasLimit = ctx.Resolve<IBlocksConfig>().TargetBlockGasLimit ?? TestingRpcModule.DefaultTestingGasLimit }

Not a blocker — same pattern exists elsewhere in the codebase and the scope is testing-only.

---

[newmanifold]

@claude review please

[claude]

Claude finished @newmanifold's task in 2m 19s —— View job

---

PR Review: fix(hive): use 60M default gas limit for testing_buildBlockV1

Tasks

• Read repository rules and guidelines
• Read changed files
• Analyze correctness, DI wiring, and edge cases
• Check test coverage
• Post findings

---

Summary

Verdict: ✅ Mergeable — 0 Critical · 0 High · 0 Medium · 2 Low

The DI-based refactor is the right final shape. Key validation points:

• DI registration is clean: .AddSingleton<ITestingRpcGasLimitCalculator, TestingRpcGasLimitCalculator>() uses type-based registration (no factory lambda), so Autofac resolves ISpecProvider and IBlocksConfig via constructor injection — avoids the anti-pattern from di-patterns.md.
• Separation is correct: ITestingRpcGasLimitCalculator is distinct from the global IGasLimitCalculator used by block production. Plugins can override the testing calculator without touching mainnet block production.
• DefaultGasLimit is accessible from tests: InternalsVisibleTo("Nethermind.Merge.Plugin.Test") is present in PeerRefresher.cs, so internal const long DefaultGasLimit resolves in the test project.
• _inner field initialization: blocksConfig.TargetBlockGasLimit ?? DefaultGasLimit is evaluated once at construction time — correct.
• Three entries correctly removed from known-failing-hive-tests.txt.

---

Findings

Low — DefaultGasLimit should be public for plugin extensibility

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcGasLimitCalculator.cs:25

internal const long DefaultGasLimit = 60_000_000;

The XML doc on ITestingRpcGasLimitCalculator says "Plugins can override by registering their own ITestingRpcGasLimitCalculator". A plugin implementing their own calculator and wanting to fall back to the standard default would need to hardcode 60_000_000L since they can't access an internal member across assembly boundaries (only the test project gets InternalsVisibleTo). If this is a genuine extension point, public is appropriate.

Fix this →

---

Low — Comment is misleading about "mainnet gas limit"

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcGasLimitCalculator.cs:24

// Matches the current mainnet gas limit target.
internal const long DefaultGasLimit = 60_000_000;

The current mainnet block gas limit is not 60M (it has been raised well above that). What 60M actually represents is the default gas ceiling in geth (miner.Config.GasCeil) and erigon (DefaultBlockGasLimit) when no specific target is configured. The comment should reflect this:

// Matches geth (miner.Config.GasCeil) and erigon (DefaultBlockGasLimit) default gas ceiling.

---

Low (previously noted, not blocking) — Shallow BlocksConfig copy

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcGasLimitCalculator.cs:27-28

private readonly TargetAdjustedGasLimitCalculator _inner = new(
    specProvider, new BlocksConfig { TargetBlockGasLimit = blocksConfig.TargetBlockGasLimit ?? DefaultGasLimit });

TargetAdjustedGasLimitCalculator currently only reads TargetBlockGasLimit, so the shallow copy is safe today. This has been flagged in prior reviews; no change needed for merge.

---

[newmanifold]

@claude review again, addressed the comments

[claude]

PR Review: fix(hive): use 60M default gas limit for testing_buildBlockV1

Tasks

• Read repository rules and guidelines
• Read changed files (latest state)
• Check if previous findings are addressed
• Post findings

Working...

View job run

[newmanifold]

@claude review please, you were stuck

[claude]

Claude finished @newmanifold's task in 1m 25s —— View job

---

PR Review: fix(hive): use 60M default gas limit for testing_buildBlockV1

Tasks

• Read repository rules and guidelines
• Read changed files (latest state)
• Check if previous findings are addressed
• Analyze correctness, DI wiring, and edge cases
• Check test coverage
• Post findings

---

Summary

Verdict: ✅ Mergeable — 0 Critical · 0 High · 0 Medium · 1 Low

The PR is in good shape. All actionable previous findings are resolved:

• Comment accuracy — "Matches the current mainnet gas limit target." (incorrect, mainnet is above 60M) is replaced with "Default gas limit target when not configured." — accurate and no longer misleading.
• DefaultGasLimit visibility — left internal. The author's reasoning is sound: internal → public is non-breaking and can be done later; the reverse is a breaking change. No plugin extension scenario exists today, so keeping it internal is the right call.
• DI registration — clean type-based registration AddSingleton<ITestingRpcGasLimitCalculator, TestingRpcGasLimitCalculator>(), no factory lambda anti-pattern. Autofac resolves ISpecProvider and IBlocksConfig via constructor injection.
• Hive tests — 2 entries correctly removed from known-failing-hive-tests.txt (build-block-empty-transactions, build-block-with-transactions).
• Test assertions — Gas_limit_uses_configured_or_default_target asserts exact values via expectedCalculator.GetGasLimit(parentHeader), covering both the null-config (adjusts up from DefaultGasLimit / 2) and the 20M-config (adjusts down from DefaultGasLimit / 2) cases.

---

Findings

Low — Shallow BlocksConfig copy (carried from prior reviews, not a blocker)

src/Nethermind/Nethermind.Merge.Plugin/TestingRpcGasLimitCalculator.cs:27-28

private readonly TargetAdjustedGasLimitCalculator _inner = new(
    specProvider, new BlocksConfig { TargetBlockGasLimit = blocksConfig.TargetBlockGasLimit ?? DefaultGasLimit });

TargetAdjustedGasLimitCalculator today only reads TargetBlockGasLimit (confirmed by inspection: _blocksConfig.TargetBlockGasLimit is the single IBlocksConfig access in that class). The shallow copy is safe. This concern has been flagged across several review rounds and is unchanged — no action required for merge.

---

[svlachakis]

@newmanifold should we bring that up to date and merge it?

[newmanifold]

@svlachakis There was a discussion in the eth Discord about having the target gas limit as part of the RPC request. So i was hoping they make gas limit part of spec, but if we need to match Geth's behavior here, then I can fix up the conflicts.

[svlachakis]

@newmanifold can you please add one more entry to this table? #11412 we are going to discuss it in the RPC Standards call today.

[newmanifold]

@svlachakis Added, though this is more of a spec addition than a discrepancy gasLimit isn't specifiable in the request currently.

[MysticRyuujin]

Related: ethereum/hive#1496

[svlachakis]

@LukaszRozmej

ethereum/hive#1496 resolves the same issue upstream by setting HIVE_TARGET_GAS_LIMIT → Blocks.TargetBlockGasLimit, which uses Nethermind's existing config so I think no new code path needed here.

Maybe leave it open as a fallback and tracking? If hive#1496 isn't merged in a reasonable amount of time we can land this PR's 60M default instead?

[LukaszRozmej]

@LukaszRozmej

ethereum/hive#1496 resolves the same issue upstream by setting HIVE_TARGET_GAS_LIMIT → Blocks.TargetBlockGasLimit, which uses Nethermind's existing config so I think no new code path needed here.

Maybe leave it open as a fallback and tracking? If hive#1496 isn't merged in a reasonable amount of time we can land this PR's 60M default instead?

So we expect to close this one?