feat(rpc): add debug_intermediateRoots#11524
feat(rpc): add debug_intermediateRoots#115240xDevNinja wants to merge 2 commits intoNethermindEth:masterfrom
Conversation
0xDevNinja
requested review from
LukaszRozmej,
MarekM25,
benaadams,
flcl42,
smartprogrammer93 and
svlachakis
as code owners
|
@claude review |
|
Claude finished @svlachakis's task in 3m 28s —— View job PR Review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| IntermediateRootsBlockTracer tracer = new(scope.Component.WorldState); | ||
| scope.Component.BlockchainProcessor.Process(block, ProcessingOptions.Trace, tracer.WithCancellation(cancellationToken), cancellationToken); | ||
|
|
||
| return tracer.BuildResult(); |
There was a problem hiding this comment.
Low — inconsistent exception-safety pattern
All other block processing paths in GethStyleTracer guard the tracer with a try/catch + tracer.TryDispose() call on exception (see TraceBlockImpl, Trace(long, Transaction, ...), etc.). IntermediateRootsBlockTracer is not currently IDisposable, so this doesn't leak today — but the pattern exists precisely so that tracers can become disposable without requiring every call site to be updated.
Suggested fix:
IntermediateRootsBlockTracer tracer = new(scope.Component.WorldState);
try
{
scope.Component.BlockchainProcessor.Process(block, ProcessingOptions.Trace, tracer.WithCancellation(cancellationToken), cancellationToken);
return tracer.BuildResult();
}
catch
{
tracer.TryDispose();
throw;
}
| IReadOnlyCollection<GethLikeTxTrace> TraceBlock(BlockParameter blockParameter, GethTraceOptions options, CancellationToken cancellationToken); | ||
| IReadOnlyCollection<GethLikeTxTrace> TraceBlock(Rlp blockRlp, GethTraceOptions options, CancellationToken cancellationToken); | ||
| IReadOnlyCollection<GethLikeTxTrace> TraceBlock(Block block, GethTraceOptions options, CancellationToken cancellationToken); | ||
| IReadOnlyCollection<Hash256> TraceBlockIntermediateRoots(Hash256 blockHash, GethTraceOptions options, CancellationToken cancellationToken); |
There was a problem hiding this comment.
Low — missing XML doc
Per the project's coding style, all public API members should have <summary> documentation. Every other method in this interface currently lacks docs too, but since this is new surface area it's a good opportunity to set the standard.
/// <summary>Replays <paramref name="blockHash"/> and returns the world-state root after each transaction, in execution order.</summary>
IReadOnlyCollection<Hash256> TraceBlockIntermediateRoots(Hash256 blockHash, GethTraceOptions options, CancellationToken cancellationToken);
|
|
||
| actual.Result.ResultType.Should().Be(ResultType.Failure); | ||
| actual.ErrorCode.Should().Be(ErrorCodes.ResourceUnavailable); | ||
| } |
There was a problem hiding this comment.
Medium — missing integration test + Low — missing "block not found" RPC test
Two gaps in coverage:
-
Integration path: Both existing tests mock
IDebugBridgeentirely. TheIntermediateRootsBlockTracer.Capturelogic — particularly theworldState.RecalculateStateRoot()fallback for post-Byzantium blocks — is never exercised. Adding a test that drivesTraceBlockIntermediateRootsthroughTestBlockchain(as other tracer tests do) would give confidence that the root accumulation across transactions is correct. -
Header-not-found path:
StandardTraceBlockToFile_returns_error_when_missing_blockexists fordebug_standardTraceBlockToFilebut the equivalent test is absent fordebug_intermediateRoots. Consider adding:
[Test]
public void Debug_intermediateRoots_fails_when_block_not_found()
{
Hash256 blockHash = TestItem.KeccakA;
_blockFinder.FindHeader(blockHash).ReturnsNull();
DebugRpcModule rpcModule = CreateDebugRpcModule(_debugBridge);
ResultWrapper<IReadOnlyCollection<Hash256>> actual = rpcModule.debug_intermediateRoots(blockHash);
actual.Result.ResultType.Should().Be(ResultType.Failure);
actual.ErrorCode.Should().Be(ErrorCodes.ResourceNotFound);
}
|
please fix claude marked issues |
|
@claude please review also the following findings.
GethStyleTracer.cs · TraceBlockIntermediateRoots — Genesis NRE. FindParent(block) returns null for genesis (GethStyleTracer.cs:224) and is passed straight into DebugRpcModule.cs · debug_intermediateRoots catch — InvalidOperationException is mapped to ResourceNotFound, but it's raised for "block not found", "parent hash
GethStyleTracer.cs · bad-block fallback — geth's debug_intermediateRoots (#23594) was designed for diagnosing client divergence on bad blocks; it falls back to IntermediateRootsBlockTracer.cs · partial-result semantics — geth returns roots[0..i] with nil error on mid-block tx failure (the diagnostic use case). Behavior
DebugModuleTests.cs — both new tests just verify the bridge is called. The "matches geth" claim hinges on these, none of which are covered:
IntermediateRootsBlockTracer.cs:7 — unused using Nethermind.Evm.TransactionProcessing;. IntermediateRootsBlockTracer.cs · StateRoot = Keccak.Zero — dead initializer; Capture always overwrites before OnEnd reads (IsTracingReceipt is hardcoded true). IDebugRpcModule.cs · JsonRpcMethod description — spec it: "post-tx state root, in execution order; excludes EIP-4788/2935 system calls and withdrawals; cost |
|
Claude finished @svlachakis's task in 3m 51s —— View job Review: additional findings on
|
| ArgumentNullException.ThrowIfNull(options); | ||
|
|
||
| Block block = blockTree.FindBlock(blockHash) ?? throw new InvalidOperationException($"Cannot find block {blockHash}"); | ||
| BlockHeader? parent = FindParent(block); |
There was a problem hiding this comment.
High — Genesis NRE / wrong behaviour
FindParent returns null when block.IsGenesis (see line 240: the body is skipped and null is returned). That null is then passed to BuildAndOverride on line 135. Whether this produces an NRE or silent wrong output depends on BuildAndOverride's implementation, but either way the behaviour is incorrect — geth explicitly rejects genesis with "genesis is not traceable".
Add a guard before the FindParent call:
| BlockHeader? parent = FindParent(block); | |
| Block block = blockTree.FindBlock(blockHash) ?? throw new InvalidOperationException($"Cannot find block {blockHash}"); | |
| if (block.IsGenesis) throw new InvalidOperationException("genesis is not traceable"); | |
| BlockHeader? parent = FindParent(block); |
| catch (InvalidOperationException ex) | ||
| { | ||
| return ResultWrapper<IReadOnlyCollection<Hash256>>.Fail(ex.Message, ErrorCodes.ResourceNotFound); | ||
| } |
There was a problem hiding this comment.
High — Overly broad exception mapping
InvalidOperationException is thrown for at least three distinct conditions: "cannot find block" (missing from tree), "cannot trace blocks with invalid parents" (bad chain state), and — once the genesis guard is added — "genesis is not traceable". All three land on ErrorCodes.ResourceNotFound, which is wrong for the latter two.
Compare to the existing debug_standardTraceBlockToFile, which does not catch InvalidOperationException — the RPC framework maps unhandled exceptions to an internal error, which is at least not misleading. Either remove the catch block entirely (matching that pattern), or introduce typed exceptions (BlockNotFoundException, TraceNotSupportedException) and map them individually.
| ArgumentNullException.ThrowIfNull(blockHash); | ||
| ArgumentNullException.ThrowIfNull(options); | ||
|
|
||
| Block block = blockTree.FindBlock(blockHash) ?? throw new InvalidOperationException($"Cannot find block {blockHash}"); |
There was a problem hiding this comment.
Medium — No bad-block fallback (compat gap vs geth)
blockTree.FindBlock(blockHash) only searches canonical storage. Geth's debug_intermediateRoots (ethereum/go-ethereum#23594) was specifically designed for diagnosing client divergence on bad blocks — blocks that were received and rejected — so it also checks rawdb.ReadBadBlock.
GethStyleTracer already has badBlockStore injected and used in TraceBadBlockToFile. This path should either:
- Also check
badBlockStore.GetAll()on lookup miss, or - Open a follow-up issue and add a
// TODO:comment here so the gap is tracked explicitly.
| public override void MarkAsSuccess(Address recipient, in GasConsumed gasSpent, byte[] output, LogEntry[] logs, Hash256? stateRoot = null) => | ||
| Capture(stateRoot); | ||
|
|
||
| public override void MarkAsFailed(Address recipient, in GasConsumed gasSpent, byte[] output, string? error, Hash256? stateRoot = null) => | ||
| Capture(stateRoot); |
There was a problem hiding this comment.
Medium — Partial-result semantics unspecified and untested
If a transaction fails mid-block (e.g. an OOG reverted tx), MarkAsFailed is still called and a root is still appended — so the count of returned roots always equals the number of transactions regardless of success/failure. This is the correct and intended semantics, but:
- It is not documented: the
JsonRpcMethoddescription and XML docs don't mention whether failed txs contribute a root. - It is not tested: there's no test with a mixed success/failure block that verifies the final count and the state after a failed tx.
Geth returns roots[0..i] with nil error when a tx fails mid-block (the exact diagnostic use case). Add a test asserting that a block with one successful tx and one OOG tx returns exactly two roots, and the second root equals the state root after the failed tx's gas refund.
| using Nethermind.Core.Crypto; | ||
| using Nethermind.Evm.State; | ||
| using Nethermind.Evm.Tracing; | ||
| using Nethermind.Evm.TransactionProcessing; |
There was a problem hiding this comment.
Low — Unused using
Nethermind.Evm.TransactionProcessing is not referenced anywhere in this file. Remove it.
| using Nethermind.Evm.TransactionProcessing; |
| { | ||
| public override bool IsTracingReceipt => true; | ||
|
|
||
| public Hash256 StateRoot { get; private set; } = Keccak.Zero; |
There was a problem hiding this comment.
Low — Dead initializer
IsTracingReceipt is hardcoded true, so MarkAsSuccess or MarkAsFailed is guaranteed to run before OnEnd reads StateRoot. The = Keccak.Zero initializer is never observable and misleads readers into thinking a tx could exit with Keccak.Zero (it never can). Remove it or replace with just the auto-property declaration.
| public Hash256 StateRoot { get; private set; } = Keccak.Zero; | |
| public Hash256 StateRoot { get; private set; } |
| [JsonRpcMethod(Description = "Similar to debug_traceBlock, this method accepts a block hash and replays the block that is already present in the database.", IsImplemented = true, IsSharable = false)] | ||
| ResultWrapper<IReadOnlyCollection<GethLikeTxTrace>> debug_traceBlockByHash(Hash256 blockHash, GethTraceOptions options = null); | ||
|
|
||
| [JsonRpcMethod(Description = "Replays the block already present in the database and returns the world state root after each transaction.", IsImplemented = true, IsSharable = false)] |
There was a problem hiding this comment.
Low — Weak JsonRpcMethod description
The current description omits details that callers need to understand the contract. Geth's docs are equally sparse. Suggested richer description:
| [JsonRpcMethod(Description = "Replays the block already present in the database and returns the world state root after each transaction.", IsImplemented = true, IsSharable = false)] | |
| [JsonRpcMethod(Description = "Replays a block and returns the world-state root after each transaction in execution order. EIP-4788/EIP-2935 system calls and withdrawals do not produce an entry. Cost scales O(txs × state_diff). Block must be canonical or in the bad-block store.", IsImplemented = true, IsSharable = false)] |
|
|
||
| actual.Result.ResultType.Should().Be(ResultType.Failure); | ||
| actual.ErrorCode.Should().Be(ErrorCodes.ResourceUnavailable); | ||
| } |
There was a problem hiding this comment.
Medium — Critical test scenarios missing
Both current tests mock IDebugBridge at the boundary — the "matches geth" claim is unverified. The following scenarios are completely uncovered:
| Scenario | Why it matters |
|---|---|
| N txs in → exactly N roots out | Basic contract verification |
| Genesis hash → clean error (not NRE) | Once genesis guard is added |
Block-not-found → ResourceNotFound |
Exercises FindHeader null path |
| Failed tx still produces a root | Partial-result semantics |
| System calls (EIP-4788/2935) don't add entry | Spec correctness |
| Withdrawals don't add entry | Spec correctness |
| Cancellation → correct error | Parity with other trace methods |
| Root equals hand-computed post-tx stateRoot | Correctness against known reference |
At minimum, add a TestBlockchain-based integration test (similar to those in GethStyleTracerTests) that replays a two-transaction block and asserts the two returned roots are distinct and non-zero. The block-not-found and genesis error tests can stay at the DebugRpcModule unit level.
3 participants
Fixes #3456
Changes
IntermediateRootsBlockTracer(Nethermind.Blockchain.Tracing) — captures the world state root after each transaction. Falls back toIWorldState.RecalculateStateRoot()post-Byzantium whereTransactionProcessorno longer pre-computes the root.IGethStyleTracer.TraceBlockIntermediateRoots(blockHash, options, ct)and the matchingGethStyleTracerimplementation, mirroring the existingTraceBlockpipeline (parent header, scoped world state,BlockchainProcessor.Process).IDebugBridge.GetBlockIntermediateRootsplumbing.debug_intermediateRoots(Hash256 blockHash, GethTraceOptions? options = null)onIDebugRpcModule/DebugRpcModule. Behaviour matches geth (eth/tracers: implement debug.intermediateRoots ethereum/go-ethereum#23594): one root per transaction, in execution order.DebugModuleTestscover the success path and the no-state-available failure path.Types of changes
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes
Adds the
debug_intermediateRootsJSON-RPC method, which replays a block and returns the world state root after each transaction in execution order. Matches the equivalent geth endpoint.