feat(debug): emit struct-log returnData in geth opcode tracers#12101
feat(debug): emit struct-log returnData in geth opcode tracers#121010xDevNinja wants to merge 2 commits into
Conversation
The default debug_trace* opcode (struct) logger never exposed the per-step return-data buffer, unlike geth/Besu/Erigon/Reth, which emit `returnData` when the `enableReturnData` flag is set. Add the `EnableReturnData` geth trace option and a gated op-level `SetOperationReturnData` tracer hook fed from the VM's return-data buffer, and emit `returnData` (hex, when non-empty) from both the in-memory and streaming geth tracers. The flag is off by default, so existing traces are unchanged. Together with the refund counter, this completes the struct-log fields. Closes NethermindEth#12057
|
@claude review |
|
Claude finished @svlachakis's task in 5m 11s —— View job Review: feat(debug): emit struct-log returnData in geth opcode tracers
SummaryThe feature is well-scoped and correctly wired end-to-end. The opt-in flag ( One Medium correctness gap and two Low issues found: Medium
Practically, the concrete Geth tracers in this PR are not affected because they always have See inline comment on LowString allocation in the streaming tracer's
See inline comment on
See inline comment on No issues found in
|
| if (IsTracingFullMemory) WriteMemoryArrayIfPresent(); | ||
| if (IsTracingOpLevelStorage) WriteStorageObjectIfPresent(); | ||
| if (IsTracingReturnData && _returnDataByteCount > 0) | ||
| _writer.WriteString("returnData"u8, _returnDataBuffer.AsSpan(0, _returnDataByteCount).ToHexString(true)); |
There was a problem hiding this comment.
Low – string allocation in the zero-alloc streaming path.
ToHexString(true) allocates a new string before writing. The rest of the streaming tracer intentionally avoids per-opcode allocations, using HexWriter.WriteFixed32HexRawValue and HexWriter.WriteUInt256HexRawValue instead. Return data can be arbitrarily large (up to the full call output), so this creates a proportional allocation on every traced opcode after an inner call returns.
A zero-copy alternative would write the 0x prefix and then the hex bytes directly into the Utf8JsonWriter without materialising an intermediate string. If HexWriter doesn't already have a variable-length variant, one could be added there; or JsonWriterHelper.WriteHexEncodedPropertyValue (if available) could be used instead.
This is gated by the opt-in EnableReturnData flag so it has no impact on the default path.
| IsTracingStack = !options.DisableStack; | ||
| IsTracingFullMemory = options.EnableMemory; | ||
| IsTracingReturnData = options.EnableReturnData; | ||
| IsTracing = IsTracing || IsTracingFullMemory; |
There was a problem hiding this comment.
Low – IsTracing override doesn't cover IsTracingReturnData.
IsTracing = IsTracing || IsTracingFullMemory was added so that IsTracingFullMemory (which isn't a standard ITxTracer flag) still keeps IsTracing true. IsTracingReturnData is a standard interface flag, so it should instead be fixed in the ITxTracer.IsTracing default (see comment there). As a belt-and-suspenders measure, consider also adding || IsTracingReturnData here — or, better, fix the source of truth in the interface.
In practice this is benign today because IsTracingInstructions returns true unconditionally from GethLikeTxTracer, ensuring IsTracing is always true regardless.
| public virtual bool IsTracingMemory { get; protected set; } | ||
| public virtual bool IsTracingInstructions { get; protected set; } | ||
| public virtual bool IsTracingRefunds { get; protected set; } | ||
| public virtual bool IsTracingReturnData { get; protected set; } |
There was a problem hiding this comment.
Medium – IsTracingReturnData must also be included in IsTracing.
Both TxTracer.IsTracing (the lazy-cached property at lines 19–30) and ITxTracer.IsTracing (the default interface property) enumerate every gated flag, but neither includes IsTracingReturnData. This is the only new tracing flag missing from both IsTracing computations.
Impact: any custom tracer that sets only IsTracingReturnData = true (all other flags false) will have IsTracing return false, and the VM skips all tracing for it entirely. In practice today's Geth tracers are unaffected (they always have IsTracingInstructions = true), but the interface contract is broken.
// TxTracer.IsTracing should include:
get => _isTracing ??= ... || IsTracingReturnData || ...;The same fix is needed in ITxTracer.cs in the bool IsTracing => default property.
| /// </summary> | ||
| /// <param name="returnData">The current return-data buffer.</param> | ||
| /// <remarks>Depends on <see cref="IsTracingReturnData"/></remarks> | ||
| void SetOperationReturnData(ReadOnlyMemory<byte> returnData); |
There was a problem hiding this comment.
Does a plain dotnet build Debug still pass? DebugTracer is #if DEBUG and implements ITxTracer directly, so it looks like it'd need the two new members forwarded too - and since CI only builds Release, it'd slip through green
- Forward IsTracingReturnData / SetOperationReturnData in DebugTracer (guarded by #if DEBUG, so the missing members were not caught by the Release-only CI build). - Include IsTracingReturnData in both the ITxTracer.IsTracing default and TxTracer.IsTracing, so a tracer that enables only return-data tracing is not skipped entirely by the VM. - Stream returnData through a pooled hex scratch buffer instead of allocating a string per opcode, matching the streaming tracer's zero-allocation design.
|
Thanks for the careful review — addressed in 194ba69:
|
|
@claude review again |
|
Claude finished @svlachakis's task in 2m 20s —— View job Re-review: feat(debug): emit struct-log returnData in geth opcode tracers
All three issues from the first review are cleanly resolved in 194ba69. No new issues found. Verified fixesMedium — Low — String allocation in streaming tracer ✓ Low (AnkushinDaniil's inline) — No new issues
Mergeable. Ready to land. |
Changes
The default
debug_trace*opcode (struct) logger never exposed the per-step return-data buffer, unlike geth/Besu/Erigon/Reth, which emitreturnDatawhen theenableReturnDataflag is set. This completes #12057 (the refund counter landed in #12081).EnableReturnDataoption toGethTraceOptions(mirrors geth'senableReturnDatatracer config key; off by default).ITxTracer.SetOperationReturnData(withIsTracingReturnData), fed from the VM's return-data buffer inStartInstructionTrace. Wired through theTxTracerbase and the forwarding tracers (Cancellation,Composite,BlockReceipts,AlwaysCancel).returnDataas a hex string (only when non-empty) from both the in-memory (GethLikeTxMemoryTracer) and streaming (GethLikeTxDirectStreamingTracer) geth tracers; the streaming path copies the bytes into a pooled scratch buffer since the source is reused across opcodes.Because the flag defaults to off, the hook is never invoked and existing traces are byte-for-byte unchanged.
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Added
Can_trace_returndata_when_enabled/ReturnData_is_omitted_when_not_enabled(in-memory) andStreams_returndata_when_enabled/Omits_returndata_when_not_enabled(streaming), asserting the buffer is captured after an inner call returns and stays absent by default. Verified no regressions across the geth tracing (403),debug_trace*JSON-RPC (116), and streaming blockchain-test runner suites.Documentation
Requires documentation update
Requires explanation in Release Notes
When
enableReturnDatais set, thedebug_traceTransaction/debug_traceBlock*default (struct) tracer now includes thereturnDatafield (hex, when non-empty) on each step, aligning the opcode trace output with geth/Besu/Erigon/Reth.