Fork override trace block#11493
Conversation
|
Claude finished @DarkLord017's task in 4m 25s —— View job PR Review: Fork override trace block
SummaryThe PR adds a Findings
Medium —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude re-review |
|
Claude finished @DarkLord017's task in 3m 58s —— View job PR Re-Review: Fork override trace block
|
| # | Severity | Was | Now |
|---|---|---|---|
| 1 | Medium | null ForkName throws ArgumentNullException |
✅ Fixed — IsNullOrEmpty guard at line 284 |
| 2 | Medium | BPO1–BPO5 missing from _forks |
✅ Fixed — Added to MainnetSpecProvider._forks |
| 3 | Low | ForkActivationParameter was mutable class, no docs |
✅ Fixed — record with param doc |
| 4 | Low | SetOverride/ResetOverride were public |
✅ Fixed — Now internal |
| 5 | Low | AvailableForks iterated in hash order |
✅ Fixed — Now sorted via OrderBy |
| 6 | Design | Only MainnetSpecProvider implemented IForkAwareSpecProvider |
✅ Fixed — ChainSpecBasedSpecProvider now also implements it |
New Findings
Low — ChainSpecBasedSpecProvider._forks should be FrozenDictionary
_forks is populated once during construction in BuildTransitions() and never mutated. MainnetSpecProvider already uses FrozenDictionary<string, IReleaseSpec> for this, making reads faster and preventing accidental mutation after construction. ChainSpecBasedSpecProvider uses a plain Dictionary<string, IReleaseSpec> which is inconsistent. Consider freezing it after CreateNamedForks returns:
_forks = CreateNamedForks(_chainSpec).ToFrozenDictionary(StringComparer.OrdinalIgnoreCase);(Also change the field type to FrozenDictionary<string, IReleaseSpec>.)
Overall Assessment
The implementation is solid. The core feature — passing a ForkActivationParameter to trace_block, resolving the spec from IForkAwareSpecProvider, and injecting it via OverridableSpecProvider into the child DI lifetime — is correct and clean. Key quality observations:
- Correctness: The
specOverrideflows correctly throughOverridableEnvFactory.BuildAndOverride→OverridableSpecProvider.SetOverride, and is reset onScope.Dispose(). The override does not leak between calls, as confirmed bytrace_block_fork_override_does_not_persist_between_calls. - State override interaction: State overrides (
stateOverride) correctly use the originalspecProvider.GetSpec(header)while EVM execution uses the overridden spec — intentional and correct. - Thread safety:
OverridableSpecProvideris intentionally not thread-safe; the class comment correctly documents the single-request-at-a-time constraint. - Error handling: The three error paths (unsupported provider, empty/null fork name, unknown fork name) all return clean
-32602 InvalidParamserrors. - Test coverage: The ModExp gas tests (Istanbul vs Berlin) give strong evidence that the override actually reaches precompile selection; the no-leak test verifies scope isolation.
The remaining FrozenDictionary issue is low severity and doesn't block merge.
|
@claude re-review also why are unnecessary tests failing |
PR Re-Review: Fork override trace block
|
|
@claude re-review |
|
Claude finished @DarkLord017's task in 10m 15s —— View job PR Re-Review: Fork override trace block
Previous Findings — StatusAll seven findings from the prior reviews have been addressed:
CI Failure InvestigationTwo workflows show
Every listed job in both failing runs is an Ethereum Foundation test ( The Nethermind tests (Flat DB) run (25373206664) passed, which is where Root cause of the DI Wiring — Verified CorrectThe key linkage I traced through
This is confirmed by the ModExp gas tests passing (override reaches the precompile) and the no-leak test passing (override does not persist between calls). New FindingsNit — "Genesis" key always present in
|
…mindEth/nethermind into fork-override-trace-block-clean
|
@claude re-review |
|
Claude finished @DarkLord017's task in 9m 4s —— View job PR Re-Review: Fork override trace block
CI Status
Previous Findings — StatusAll seven findings from prior reviews have been addressed:
New FindingsLow —
|
|
@claude re-review |
|
Claude finished @DarkLord017's task in 2m 57s —— View job PR Re-Review: Fork override trace block
CI Status
LukaszRozmej's CHANGES_REQUESTED (2026-05-11) — Status
Implementation Correctness — Confirmed
Remaining IssueLow — Excessive comments at lines 1345–1351 ( LukaszRozmej's CHANGES_REQUESTED on this point is still open. The block comment before the OverallOne Low item remains from LukaszRozmej's CHANGES_REQUESTED (excessive comments). All previous Critical/High/Medium/Low findings from prior review rounds are resolved. Once the comments are removed, the PR is ready for re-approval. |
|
@claude review |
|
Claude finished @DarkLord017's task in 7m 21s —— View job PR Review: Fork override trace block
LukaszRozmej's 2026-05-12 Comments — Status
Findings
Low — Excessive comments (Finding #1)LukaszRozmej's CHANGES_REQUESTED from 2026-05-11 included "excessive comments." Lines 1345–1346 describe what the code does (already clear from the test name Low — Duplicated
|
|
@claude review |
|
Claude finished @DarkLord017's task in 4m 12s —— View job PR Review: Fork override trace block
|
| Check | Result |
|---|---|
| Code formatting | ✅ Success |
| Dependency review | ✅ Success |
| PR labeler | ✅ Success |
| Build solutions | 🔄 In progress |
| Nethermind tests (Flat DB) | 🔄 Pending |
| Nethermind extra test variants | 🔄 Pending |
| Nethermind/Ethereum tests | 🔄 Pending |
| Code Lint | 🔄 In progress |
| Trivy scanner | 🔄 In progress |
New This Round — ForkScheduleSpecProvider Refactoring
The major new change is the introduction of ForkScheduleSpecProvider (base class) + ForkSpec (value type), plus refactoring SepoliaSpecProvider, HoodiSpecProvider, ChiadoSpecProvider, and MordenSpecProvider to inherit from it. This addresses LukaszRozmej's CHANGES_REQUESTED about reducing duplication. The implementation is clean:
ForkSpec:readonly record structwith two constructors for block-based and timestamp-based entries. Well designed.ForkScheduleSpecProvider.GetSpec: correctly handles mixed block/timestamp schedules. Tries timestamp-based entries first (if activation has a timestamp), then falls back to block-based. For real-world block activations this is semantically equivalent to the previous switch-based logic inMainnetSpecProvider.- Lazy initialization:
_schedule,_forks,_availableForksare all lazy.GnosisSpecProvidercorrectly uses theFunc<ForkSpec[]>constructor to avoid a circular static-init dependency onLondonGnosis. - All
ISpecProvidermembers: covered —SealEnginehas a default impl in the interface (Ethash);TimestampFork,NetworkId,BeaconChainGenesisTimestampare abstract;ChainIddefaults toNetworkId; all else are implemented.
LukaszRozmej's CHANGES_REQUESTED — Status
| # | Comment | Status |
|---|---|---|
TraceRpcModuleTests.cs — use nameof from Nethermind.Specs.Forks |
✅ Done | |
TraceRpcModuleTests.cs — excessive comments |
❌ Still present at lines 1345–1351 | |
ChainSpecBasedSpecProviderTests.cs — "London" → expect LondonGnosis |
✅ Addressed (uses canonical name, expects Gnosis variant) | |
ChainSpecBasedSpecProvider — use separate classes for each network |
✅ Done — ForksForChain delegates to GnosisSpecProvider, ChiadoSpecProvider, SepoliaSpecProvider, HoodiSpecProvider, MordenSpecProvider |
|
SepoliaSpecProvider — extract base class to remove GetSpec duplication |
✅ Done — ForkScheduleSpecProvider |
Findings
Low — Excessive comments in TraceRpcModuleTests.cs:1345–1351 (LukaszRozmej CHANGES_REQUESTED, still unresolved)
Lines 1345–1346 describe what the code does (already communicated by the test name trace_block_pre_1559_fork_override_on_london_block_zeroes_base_fee). Line 1351's because string embeds the internal method name AdjustHeaderForSpec. Both should be removed:
// Remove lines 1345-1346 (the block comment before the trace_block call)
// Change line 1350-1351 from:
result.Result.ResultType.Should().Be(ResultType.Success,
"tracing a London block with a pre-EIP-1559 fork override must succeed (AdjustHeaderForSpec zeroes BaseFeePerGas)");
// To:
result.Result.ResultType.Should().Be(ResultType.Success);Nit — ForkScheduleSpecProvider has IForkAwareSpecProvider methods but doesn't declare the interface
ForkScheduleSpecProvider defines AvailableForks, TryGetForkSpec, and Forks — everything IForkAwareSpecProvider requires — but it doesn't declare the interface. MainnetSpecProvider and GnosisSpecProvider have to explicitly add : IForkAwareSpecProvider. In practice, SepoliaSpecProvider, HoodiSpecProvider, ChiadoSpecProvider, and MordenSpecProvider cannot be used directly for fork overrides (the is not IForkAwareSpecProvider guard in TryResolveForkSpec fires). This is acceptable because ChainSpecBasedSpecProvider covers these networks via ForksForChain, but having the base class declare the interface would be cleaner and eliminate the need for the explicit declarations on each subclass.
Implementation Correctness — Confirmed
ForkScheduleSpecProvider.GetSpec: Correct for all real-world block activations. The mixed block/timestamp schedule (as inMainnetSpecProvider) is handled correctly: timestamp lookup first (for post-merge blocks), then block number fallback (for pre-merge blocks). The theoretical edge case of a pre-Paris block with a post-Shanghai timestamp never occurs in practice.- DI flow:
tracerEnv.BuildAndOverride(baseBlock, specOverride: forkSpec)→OverridableSpecProvider.SetOverride→ EVM execution →ResetOverride()onScope.Dispose(). No override leaks. TryResolveForkSpec: Handles all four cases cleanly —null, unsupported provider, empty string, unknown fork name.AdjustHeaderForSpec: Correctly zerosBaseFeePerGasfor pre-EIP-1559; nullsExcessBlobGasfor pre-EIP-4844. No unused parameters.ChainSpecBasedSpecProvider.ForksForChain: Delegates to the correct network-specificFrozenDictionary— Gnosis/Chiado/Sepolia/Hoodi/Morden are all covered.TraceStoreRpcModule: Correctly bypasses the trace store for fork-override requests.
Overall
One Low issue from LukaszRozmej's CHANGES_REQUESTED remains open (excessive comments at lines 1345–1351 in TraceRpcModuleTests.cs). The ForkScheduleSpecProvider refactoring is well-implemented and correctly addresses the duplication LukaszRozmej flagged. All prior Critical/High/Medium findings are resolved.
…mindEth/nethermind into fork-override-trace-block
|
@claude re-review |
|
Claude finished @DarkLord017's task in 5m 5s —— View job PR Re-Review: Fork override trace block
CI Status
Previous Findings — Status
New Finding — RegressionMedium —
|
| # | Comment | Status |
|---|---|---|
Use nameof from Nethermind.Specs.Forks |
✅ Done | |
| Excessive comments in test | ❌ Lines 1345–1351 still present | |
"London" input should map to LondonGnosis spec |
✅ Addressed | |
| Use separate classes for Gnosis/Chiado/Sepolia/Hoodi | ✅ ForkScheduleSpecProvider base class |
|
Extract base class to remove GetSpec duplication |
✅ Done |
Summary
| Severity | Count | Issues |
|---|---|---|
| Critical | 0 | — |
| High | 0 | — |
| Medium | 1 | ChiadoSpecProvider.SealEngine static regression |
| Low | 1 | Excessive comments lines 1345–1351 (LukaszRozmej CHANGES_REQUESTED) |
Two items must be fixed before merge: the static SealEngine regression on ChiadoSpecProvider and the open CHANGES_REQUESTED on the test comments.
|
@claude review |
|
Claude finished @DarkLord017's task in 3m 58s —— View job PR Review: Fork override trace block
|
| Check | Result |
|---|---|
| Code formatting | ✅ Success |
| Dependency review | ✅ Success |
| Spell check | ✅ Success |
| PR labeler | ✅ Success |
| Build solutions | 🔄 In progress |
| Code Lint | 🔄 In progress |
| Nethermind tests (Flat DB) | 🔄 In progress |
| Nethermind extra test variants | 🔄 In progress |
| Nethermind/Ethereum tests | 🔄 Queued |
| Sync PR Gate (Hoodi) | 🔄 In progress |
| Stateless execution tests | 🔄 In progress |
Previous Findings — Status
| # | Severity | Issue | Status |
|---|---|---|---|
| 1–9 | Med/Low | All prior rounds (null guard, BPO forks, record, visibility, ordering, ChainSpec, FrozenDict, BlobGas, unused param) | ✅ Resolved |
| Medium | ChiadoSpecProvider.SealEngine was static, silently returning Ethash via interface |
✅ Fixed — commit 7480b17 removes static, now correctly public string SealEngine => SealEngineType.AuRa; |
|
| Low | Excessive comments in TraceRpcModuleTests.cs:1345–1351 (LukaszRozmej CHANGES_REQUESTED) |
❌ Still present |
GetSpec Algorithm Change — Verified Correct
Commit 452c9f9 rewrote ForkScheduleSpecProvider.GetSpec from backward iteration to forward iteration. The new algorithm is semantically equivalent for all real-world cases:
- All-block schedules (Morden): Forward iteration hits all entries, break fires when block doesn't match — correct.
- All-timestamp schedules (Sepolia, Hoodi, Chiado): No block entries to break on; all timestamps evaluated in order; returns last matching timestamp spec — correct. For Hoodi's all-zero timestamps (
Shanghai=0,Cancun=0), a query withts=0correctly returnsCancun.Instance. - Mixed schedules (Mainnet, Gnosis): Block forks processed forward, break fires when block doesn't match (which only happens before the block's fork level); timestamp forks after Paris are all reachable for any valid mainnet block at or above Paris height — correct.
- No timestamp (
forkActivation.Timestamp is null): Block forks still processed; timestamp conditionforkActivation.Timestamp is ulong tsshort-circuits for null — correct.
Remaining Issue
Low — Excessive comments in TraceRpcModuleTests.cs:1345–1351 (LukaszRozmej CHANGES_REQUESTED, still open)
Lines 1345–1346 describe what the code does (already communicated by the test name trace_block_pre_1559_fork_override_on_london_block_zeroes_base_fee). Line 1351 embeds the internal method name AdjustHeaderForSpec in the because string. Both are noise:
// Remove lines 1345-1346, and strip the `because` string from line 1350-1351:
ResultWrapper<IEnumerable<ParityTxTraceFromStore>> result =
context.TraceRpcModule.trace_block(new BlockParameter(1L), nameof(Berlin));
result.Result.ResultType.Should().Be(ResultType.Success);Overall Assessment
The implementation is correct and complete. Both recent commits address issues from the last review: the static SealEngine regression on ChiadoSpecProvider is fixed, and the GetSpec forward-iteration algorithm is correct for all real-world fork schedules. The DI flow, override isolation, header adjustment, and error handling are all sound and unchanged.
The one remaining item is the Low-severity excessive comments that LukaszRozmej's CHANGES_REQUESTED has flagged across multiple rounds. Once that's addressed, the PR should be ready for re-approval.
2 participants
Fixes Closes Resolves #9056
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Documentation
Requires documentation update
Requires explanation in Release Notes