Skip to content

fix(rpc): omit to on failed top-level CREATE frames in callTracer - #13078

Merged
LukaszRozmej merged 3 commits into
masterfrom
fix/calltracer-failed-create-to
Sep 1, 2026
Merged

LukaszRozmej merged 3 commits into
masterfrom
fix/calltracer-failed-create-to

Conversation

@LukaszRozmej

Copy link
Copy Markdown
Member

Changes

  • Omit to on the root call frame when a top-level CREATE/CREATE2 fails — no contract was deployed. NativeCallTracer applied this rule only to nested frames; geth runs the same processOutput at depth 0.
  • Extract the shared failure handling (MarkFrameFailed) so the root and nested frames cannot drift apart again.
  • Add debug_traceCall regression coverage for the callTracer revert contract: a revert is a traced result carrying error/revertReason/output, not a JSON-RPC error.

Conformance with the newly specified callTracer output and debug_traceCall in ethereum/execution-apis#855, whose CallFrame schema states to "MUST be present on all frames except CREATE and CREATE2 frames that failed, where it MUST be omitted since no contract was created".

The base-fee item that PR's conformance table lists for Nethermind ("stop base-fee validation of debug_traceCall calls that carry no fee fields") was already fixed in #12804, after the 2026-08-25 run it reports. This was the remaining CallFrame divergence found when auditing the tracer and debug_traceCall against all of the spec's normative rules and its 19 fixtures.

Two divergences were found and deliberately left alone:

  • revertReason is also populated for Panic(uint256) reverts, where the spec text says Error(string) only (geth omits it). Changing it would make our output strictly less useful and touches revert decoding shared with eth_call.
  • With onlyTopCall + withLog, geth attaches nested frames' logs to the root frame while we drop them. The spec is silent on the interaction and no fixture covers it.

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

Covered at both the tracer level (GethLikeCallTracerTests.TopLevelCreate_Revert) and the RPC level (DebugRpcModuleTests.Debug_traceCall_with_callTracer_omits_to_on_failed_top_level_create, ..._reports_revert_in_the_frame). Verified the CREATE test fails without the fix — the frame comes back with "to":"0x0ffd3e46…".

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

🤖 Generated with Claude Code

The execution-apis `CallFrame` schema (ethereum/execution-apis#855) requires
`to` to be omitted on CREATE/CREATE2 frames that failed, since no contract was
deployed. NativeCallTracer applied that only to nested frames; the root frame
of a failed deployment kept the would-be contract address, unlike geth, which
runs the same processOutput at depth 0.

Also pins the spec's `debug_traceCall` revert contract: a revert is a traced
result carrying error/revertReason/output, not a JSON-RPC error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @LukaszRozmej's task in 5m 30s —— View job


Review: omit to on failed top-level CREATE frames

  • Read PR diff and the full NativeCallTracer
  • Verify the change against geth's callTracer.processOutput / captureEnd
  • Check serializer, downstream consumers, and existing expected-JSON fixtures for regressions
  • Review the new tests
  • Post findings

Verdict: correct and well-scoped. No Critical / High / Medium findings — three Low notes, all test-side.


What I verified

The behaviour change matches geth. geth's callTracer.OnExit routes depth == 0 to captureEnd, which calls the same processOutput used for nested frames, and that nulls To for CREATE/CREATE2 whenever err != nil. Nethermind's root frame went through MarkAsFailed instead, which only set Error. Folding both onto MarkFrameFailed (NativeCallTracer.cs:255) closes exactly that gap, and the nested path is byte-identical to what it did before — the extracted method is a verbatim move of the two statements.

The refactor doesn't create a double-application hazard. OnExit early-returns on Depth == 0 (NativeCallTracer.cs:233), so a root frame is only ever touched by MarkAsFailed; ProcessOutput never sees it. No frame gets MarkFrameFailed applied twice.

Serialization already handles the null. NativeCallTracerCallFrameConverter.WriteFrameHeader guards to behind if (value.To is not null) (line 74), so the field is omitted rather than emitted as "to": null — which is what the execution-apis CallFrame schema requires. No converter change needed.

No fixture regressions. NativeCallTracerCallFrame is referenced by only six files, and every existing expected-JSON trace with a root-level "error" (GethLikeCallTracerTests.cs:288, :466; DebugRpcModuleTests.TraceTransaction.cs:292; TraceBlock.cs:226, :249) has either a CALL root or a successful CREATE root, so none of them shift.

No enum gap. ExecutionTypeExtensions.ToInstruction only ever produces CREATE/CREATE2 for creates (no EOF variants exist), so the Type is Instruction.CREATE or Instruction.CREATE2 test covers every create frame the tracer can build.

The hand-rolled revert contract is right. 604e600c600039604e6000fd is CODECOPY(dest=0, src=0x0c, len=0x4e) then REVERT(0, 0x4e); the prefix is exactly 12 bytes and the Error(string) payload is exactly 78, so the copy window lines up. stateOverrides is plumbed for debug_traceCall via GethTraceOptions.StateOverrides → BuildAndOverride → ApplyStateOverridesNoCommit, and AccountOverride.Code is the right field.

Findings (all Low, all test-side)

  1. DebugRpcModuleTests.cs:425 — the CREATE test dereferences ["result"]! without the error is null guard its sibling test has; a pre-flight rejection would surface as a bare NullReferenceException. Inline comment with suggestion
  2. GethLikeCallTracerTests.cs:613 — MarkFrameFailed keys on error is not null, not on Revert, but both new tests only cover the revert flavour; a non-revert halt case (INVALID) would pin the other branch. Inline comment
  3. DebugRpcModuleTests.cs:398 — the stateOverrides triple-brace raw string + JsonSerializer.Deserialize<object> round-trip can just be a Dictionary<string, object>, since RpcTest.BuildJsonRequest serializes params by runtime type. Inline comment with suggestion

Notes, not findings

  • Comment quality: the <remarks> on MarkFrameFailed cites the execution-apis CallFrame rule and the why (no contract was deployed) — that is the kind of comment AGENTS.md asks for. The opcode gloss above RevertingContractCode is likewise earning its keep against raw hex. Nothing to strip.
  • Pre-existing, out of scope: _error is a single field overwritten by ReportActionError at any depth, so if a tx reaches MarkAsFailed without a depth-0 EVM error while a nested frame had one, the root frame inherits that stale error description. This PR makes such a root CREATE also drop to — which happens to be the right answer there (a failed tx deployed nothing), so it isn't a new defect. The stale Error string itself predates this change; Test_CallTrace_MarkAsFailed_WithoutEvmError_NoCrash shows the _error is null variant of that path is real.
  • Performance: tracing-only path, one extra static call, no allocation change. Nothing to flag on a hot path.

Not done

I could not compile or run the test suites — dotnet test isn't in this job's allowed tools, so my analysis of the two new RPC tests is static only. CI on this PR is the authority on whether they pass.
• branch fix/calltracer-failed-create-to

null,
new { tracer = "callTracer" });

JToken frame = JToken.Parse(response)["result"]!;

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.

Low — unlike the sibling test above, this one dereferences ["result"]! without first asserting the response isn't a JSON-RPC error. If debug_traceCall ever starts rejecting this payload (e.g. an intrinsic-gas or base-fee pre-flight change), the test fails with a bare NullReferenceException on line 428 instead of showing the actual error object. Cheap to make the failure legible:

Suggested change
JToken frame = JToken.Parse(response)["result"]!;
JToken parsed = JToken.Parse(response);
Assert.That(parsed["error"], Is.Null, "the trace must come back as a result, not a JSON-RPC error");
JToken frame = parsed["result"]!;

Comment on lines +398 to +399
stateOverrides = JsonSerializer.Deserialize<object>(
$$$"""{"{{{RevertingContractAddress}}}":{"code":"{{{RevertingContractCode}}}"}}""")

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.

Low (style) — RpcTest.BuildJsonRequest serializes each param by runtime type, so a plain dictionary works here and avoids the raw-string/triple-brace interpolation and the Deserialize<object> round-trip:

Suggested change
stateOverrides = JsonSerializer.Deserialize<object>(
$$$"""{"{{{RevertingContractAddress}}}":{"code":"{{{RevertingContractCode}}}"}}""")
stateOverrides = new Dictionary<string, object>
{
[RevertingContractAddress] = new { code = RevertingContractCode }
}

System.Collections.Generic is already imported; System.Text.Json then becomes unused in this partial only if no other test needs it (it is used elsewhere in the file — leave the using).

Assert.That(frame!.Type, Is.EqualTo(Instruction.CREATE));
if (revert)
{
Assert.That(frame.Error, Is.Not.Null, "expected error description on reverted CREATE");

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.

Low (coverage) — the new to-omission rule fires on any halt, not just REVERT (MarkFrameFailed is keyed on error is not null, not on EvmExceptionType.Revert). Both new tests only exercise the revert flavour. A third [TestCase] here with init code that halts non-revertingly — e.g. Prepare.EvmCode.Op(Instruction.INVALID) — would pin the OutOfGas/BadInstruction path through MarkAsFailed, which is the one where output is empty and RevertReason stays null.

LukaszRozmej and others added 2 commits September 1, 2026 18:46
- Cover a non-reverting halt (INVALID) as well as REVERT: the `to`-omission
  fires on any EVM halt, not just REVERT. Replaces the two-bool parameters
  with a CreateOutcome enum so the fourth case stays readable.
- Assert no JSON-RPC error before dereferencing `result` in the traceCall
  create test, so a pre-flight rejection reports the error object instead of
  a NullReferenceException.
- Build the state override from a dictionary instead of a raw-string JSON
  round-trip.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@LukaszRozmej

Copy link
Copy Markdown
Member Author

@claude re-review

@claude

claude Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @LukaszRozmej's task in 3m 10s —— View job


Re-review: omit to on failed top-level CREATE frames

  • Diff the current head against master and against my last review
  • Re-check the tracer change and the follow-up test commit (8cd96b0)
  • Verify the three prior Low findings
  • Post findings

Verdict: all three prior Low findings are addressed; no new findings at any severity. LGTM.


What changed since my last pass

8cd96b0 is test-only — NativeCallTracer.cs is byte-identical to what I reviewed before. Each earlier note is closed:

  1. Missing error is null guard — both RPC tests now assert parsed["error"] is null with a message before dereferencing ["result"] (DebugRpcModuleTests.cs:404, :427), so a pre-flight rejection surfaces the error object instead of an NRE.
  2. Non-revert halt uncovered — Test_CallTrace_TopLevelCreate is now driven by a CreateOutcome enum with an InvalidOpcode case (GethLikeCallTracerTests.cs:579, init code Prepare.EvmCode.Op(Instruction.INVALID)). That pins the BadInstruction path through ReportActionError → _error → MarkAsFailed, which is the branch where output is empty and RevertReason stays null — the error is not null keying of MarkFrameFailed, not just Revert, is now exercised. The TestName = labels were kept, so existing test IDs are stable.
  3. Raw-string state override — replaced with new Dictionary<string, object> { [RevertingContractAddress] = new { code = RevertingContractCode } }. RpcTest.BuildJsonRequest serializes the whole object?[] with EthereumJsonSerializer, and STJ resolves object-declared dictionary values by runtime type, so the anonymous value still emits {"code":"0x…"}. using System.Text.Json is still needed elsewhere in the file (lines 133, 183, 205, 299, 347) and System.Collections.Generic was already imported — no unused-using lint break.

The assertion refactor also tightened the test rather than just widening it: the failure branch now asserts frame.To is null, which the old revert-only branch never did.

Re-verified on the current head

  • Depth starts at -1 (GethLikeNativeTxTracer.cs:12) and the root frame is Depth == 0, so OnExit's Depth > 0 guard (NativeCallTracer.cs:233) still means the root reaches MarkFrameFailed only via MarkAsFailed — no double application after the extraction.
  • MarkAsFailed is only reached from TransactionProcessor.cs:685 under statusCode == Failure, which implies ReportActionError/ReportActionRevert already ran, so _error is null there stays the synthetic case that Test_CallTrace_MarkAsFailed_WithoutEvmError_NoCrash guards — the omission rule not applying on that path is not a reachable gap.
  • NativeCallTracerCallFrameConverter.WriteFrameHeader guards to behind is not null, so the field is omitted rather than emitted as "to": null, matching the execution-apis CallFrame schema.
  • AccountOverride code-only overrides on a non-existent address work through IOverridableCodeInfoRepository (StateOverridesExtensions.cs:47), which is the same pattern the pre-existing tests in this file use.
  • git diff --check is clean.
  • Comments: the <remarks> on MarkFrameFailed gives the why plus the spec rule, and the opcode gloss on RevertingContractCode earns its keep against raw hex. Nothing to strip per AGENTS.md.

Not done

dotnet test is not in this job's allowed tools, so the two RPC tests and the new InvalidOpcode case are reviewed statically only. CI on this PR is the authority on whether they pass.
• branch fix/calltracer-failed-create-to

@LukaszRozmej
LukaszRozmej merged commit 5112c19 into master Sep 1, 2026
502 checks passed
@LukaszRozmej
LukaszRozmej deleted the fix/calltracer-failed-create-to branch September 1, 2026 21:31
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.

3 participants