RPC: Debug* streaming approach#11693
Conversation
|
Claude finished @svlachakis's task in 5m 57s —— View job PR Review: RPC Debug* Streaming ApproachTasks
SummaryThe architecture is well-designed: Two High severity issues need addressing before merge, plus several Medium findings. FindingsHigh1. Unclosed JSON array/object on cancellation — 2. Medium3. Sync-over-async on EVM thread — 4. 5. No streaming-specific tests — tests verify the final serialised JSON output via
Low6. 7. What's good
|
|
@claude review again carefully, check possible functionality regressions. |
|
Claude finished @svlachakis's task in 10m 41s —— View job PR Review: RPC Debug* Streaming Approach — Regression AnalysisTasks
SummaryThe core streaming architecture is sound, and several issues from the prior review (unclosed JSON arrays, 4 High findings and 2 Medium findings that need addressing before merge. High Severity1. JSON field order changed — breaks Geth-compatible tooling → inlineFile: Non-streaming path ( {"gas":…,"failed":…,"returnValue":"0x","structLogs":[…]}Streaming path emits: {"structLogs":[…],"gas":…,"failed":…,"returnValue":"0x"}Geth's reference implementation uses the first order. Any conformance tooling, Hive test, or vendor dashboard that compares traces byte-for-byte against Geth will fail. The PR ticks Optimization but not Breaking change — this is a silent format regression. The rewritten tests use field-by-field assertion so they cannot detect a future regression back. The field order is forced by the streaming design (the array must be opened before execution), but the breaking-change checkbox must be ticked and the change documented. 2. Block body not found silently returns
|
|
@claude review again. Follow-up: Errors and messages need to be architected properly as separate work - surfacing TransactionResult validation failures, lazy-envelope for pre-flight error parity with Geth, per-scenario Geth-format messages, and differentiated errorCodes. Touches ITxTracer / TransactionProcessor and exception classes used by sequencer/simulate/MEV adapters; scoped to its own ticket. |
|
Claude finished @svlachakis's task in 5m 15s —— View job PR Review: RPC Debug* Streaming Approach — Third PassTasks
What has been fixed since the previous passes ✓
Remaining FindingsHigh1. JSON field order is a silent breaking change — PR checkbox not ticked → inline The streaming path emits: {"structLogs":[…], "gas":…, "failed":…, "returnValue":"0x"}The non-streaming path (and Geth's reference format) emits: {"gas":…, "failed":…, "returnValue":"0x", "structLogs":[…]}The field order is forced by the streaming design ( Medium2.
Fix: store 3.
Simplest fix: add an eager null-guard in 4. No streaming tests for block-trace error paths The new tests cover single-tx cancellation and mid-trace throws. Missing:
Low5. Since the streaming path always resolves to a SummarySecond pass raised 6 High/Medium issues; 5 are now resolved. One High remains (breaking-change checkbox + documentation for field order). Two Mediums remain (missing Recommended before merge:
|
|
Claude finished @svlachakis's task in 9m 31s —— View job PR Review: RPC Debug* Streaming — Sixth Pass (Geth Parity Error Messages)Tasks
Status of All Prior FindingsAll 17 previously raised High/Medium findings remain resolved. The two Low nits from pass 5 ( Geth Parity Error Messages — AnalysisThe new For if (UInt256.SubtractUnderflow(in senderBalance, in tx.ValueRef, out UInt256 balanceLeft))
return InsufficientFundsForTransfer(tx, senderBalance); // returns TransactionResultThis returns
New FindingsLow1. 2. 3. Two tests still missing What's Good
SummaryAll High and Medium issues from all five prior passes remain resolved. Three Low nits remain (error prefix inconsistency in |
|
@claude do one last pass, check streaming design, any possible race conditions, memory leaks, or lower possible allocation paths. |
|
Claude finished @svlachakis's task in 9m 9s —— View job PR Review: RPC Debug* Streaming — Final Pass (Race Conditions, Memory Leaks, Allocations)Tasks
Race ConditionsNone introduced by this PR. All new streaming code runs serially within The one pre-existing race: The deferred-execution model ( Resource LeaksNone found. The CTS ownership chain is sound:
Memory / Allocation FindingsMedium1. Block-trace streaming is O(N_transactions) in result-object memory, not O(1)
Each The PR's O(1) claim is accurate for struct-log entries — entries are correct. Block-level result objects remain O(N_tx). Fix: override Low2.
3. byte[] bigEndian = new byte[32];
storageIndex.ToBigEndian(bigEndian);
4. In Streaming Design5.
All Prior FindingsAll High and Medium findings from passes 1–6 remain resolved. The two previously-open Low nits (test SummaryNo race conditions or resource leaks introduced. The dominant actionable finding is that block-trace streaming still holds O(N_tx) |
|
@claude review again 1 - fixed |
|
Claude finished @svlachakis's task in 5m 55s —— View job PR Review: RPC Debug* Streaming — Eighth PassTasks
Status of All Pass-7 Findings
Status of Pass-6 Low Nits
New Findings (this pass)None. After reviewing the complete set of changed files:
What's Good
SummaryAll High and Medium findings from all seven prior passes are resolved. All Low nits from passes 5–7 are fixed or reasonably acknowledged. No new issues found in this pass. The PR is mergeable. |
|
Claude finished @svlachakis's task in 7m 35s —— View job PR Review: RPC Debug* Streaming — Ninth Pass (ready_for_review)Tasks
Status of All Prior FindingsAll 17+ High and Medium findings from passes 1–8 remain resolved. The three Low nits raised in pass 8 (test
Architecture Correctness — Verified
New Findings (Low only)1.
The non-streaming path in What's Good
SummaryAll High and Medium findings from all eight prior passes are resolved. This ninth pass finds only one Low-severity observation (null trace produces |
|
@claude review again |
|
Claude finished @svlachakis's task in 3m 39s —— View job PR Review: RPC Debug* Streaming — Tenth PassTasks
Status of All Prior FindingsAll 17+ High and Medium findings from passes 1–9 remain resolved. The three Low nits from pass 9 are confirmed fixed:
Key Correctness Paths — Verified ✓
New FindingsNone. No new High, Medium, or Low findings in this pass. What's GoodThe PR is clean and correct across all failure modes. The architecture delivers on its stated goals:
SummaryAll High and Medium findings from all nine prior passes are resolved. No new issues found. The PR is mergeable. |
LukaszRozmej
left a comment
There was a problem hiding this comment.
- Any benchmarks? (Including memory pressure and node total memory!)
- Do the same for trace* endpoints
- To some extent this should be configurable, not sure if on the node, or maybe in the Options on the call itself?
- A bit too much delegates flying around? Maybe better to have proper types.
|
As for benchmark I would love to see real-life benchmark on real node, not only synthetic one |
|
@LukaszRozmej sure will do it and share results before merging |
Fixes Closes Resolves #11718
Overview
Default-tracer struct-log traces are now streamed per-opcode through:
IStreamableResult → Utf8JsonWriter → PipeWriterinstead of being accumulated into a
List<GethTxTraceEntry>and serialized at the end.Peak memory is now
O(1)entries regardless of trace length, and time-to-first-byte drops from “after trace completes” to “first opcode boundary”.Changes
Scope
Streaming activates only when
options.Traceris null/empty (default struct-log tracer).Custom tracers still use the existing buffered path:
callTracerprestateTracer4byteTracerStreamed Methods
debug_traceTransactiondebug_traceCalldebug_traceTransactionByBlockAndIndexdebug_traceTransactionByBlockhashAndIndexdebug_traceTransactionInBlockByHashdebug_traceTransactionInBlockByIndexdebug_traceBlockdebug_traceBlockByNumberdebug_traceBlockByHashdebug_traceCallManyBehavior Changes
JSON field order for the default struct-log tracer: streamed responses emit
structLogsfirst, thengas/failed/returnValue. The non-streaming (custom-tracer) path and Geth's reference implementation emitgas/failed/returnValue/structLogs. The order is forced by the streaming design — the array must be opened before EVM execution begins so entries can be emitted per opcode. Clients that parse by field name are unaffected; byte-for-byte comparators against Geth (Hive conformance, vendor dashboards) will see a diff.Pre-flight failures on
debug_traceCall/debug_traceCallMany: previously returned a JSON-RPCerrorenvelope; now return aresultenvelope withfailed: true,error: "tracing failed: …",errorCode: -32000. On-chain trace methods (debug_traceTransaction*,debug_traceBlock*) are unaffected — their txs already passed validation at block-creation time.Partial
structLogson mid-execution failure: failing traces may now contain the N opcode entries that executed before the failure, instead of an empty array. Inherent to streaming.Follow-up — error/message architecture
Geth-compat parity on error semantics is scoped as separate work, not bundled here:
TransactionResultvalidation failures (nonce, intrinsic gas, EIP-3607, etc.) through to the tracer — fixes a pre-existing silent-failure gap that affects buffered too.errorenvelope instead of mid-streamfailed: true."insufficient funds for gas * price + value: address X have N want M", etc.).errorCodes beyond the currentInvalidInput/InternalErrorsplit.These touch
ITxTracer/TransactionProcessorand exception classes used by sequencer/simulate/MEV adapters.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?