rpc: fix trace result field written on exec error

[lupin012]

Problem

`traceBlock` wrote `"result":` eagerly before calling `TraceTx`; on error this produced `{"txHash":"...","result":,"error":{...}}` — invalid JSON rejected by decoders with "unexpected value type".

`ExecuteTraceTx` called `WriteNil()` unconditionally on error, causing `"result":null` alongside `"error"` in `debug_traceCall` responses, violating JSON-RPC 2.0 (Hive rpc-compat tests 16, 20, 21, 29).

Fix

LazyFieldStream

Introduce `jsonstream.LazyFieldStream`, a lazy wrapper that emits the field name (and an optional leading comma) only on the first tracer write. If execution errors before writing anything the field is omitted entirely.

This consolidates two previously separate private types (`resultFieldStream` in `rpc/handler.go` and `txResultFieldStream` in `rpc/jsonrpc/tracing.go`) into one reusable type with a `prependSeparator bool` constructor flag.

`LazyFieldStream` also records the enclosing object's stack depth at the moment the field name is first written (`openDepth`), and exposes a `CloseIfOpen()` method that calls `ClosePending(openDepth)` to close any partial value back to that level — without the caller needing to track depth.

ClosePending semantics

`StackStream.ClosePending` now takes a `targetDepth` (absolute stack index to preserve) instead of `skipLast` (relative skip count). After closing, the stack is truncated to `targetDepth` entries, leaving the caller inside the preserved nesting level rather than resetting the whole stack.

Depth() on Stream interface

Added `Depth() int` to the `Stream` interface. `StackStream` returns `len(stack)`; `JsoniterStream` returns 0 (no tracking, `ClosePending` is a no-op on that implementation anyway).

WriteNil removals

Removed `stream.WriteNil()` from error paths in:

• `rpc/transactions/tracing.go` — `TraceTx` (`AssembleTracer` failure) and `ExecuteTraceTx` (`GetResult` and `stream.Write` failures)
• `polygon/tracer/trace_bor_state_sync_txn.go` — `AssembleTracer` failure

Tests added

`TestTraceErrorPathsWriteNoStream` — verifies the stream is empty when a method returns early with an error:

• `TraceBlockByNumber_genesis` / `TraceBlockByHash_genesis`: genesis block is not traceable
• `TraceTransaction_genesis`: transaction not found
• `TraceCall_execution_error_streaming` / `_callTracer`: execution fails with insufficient funds
• `TraceTransaction_bad_timeout` / `TraceCall_bad_timeout`: invalid timeout string causes `AssembleTracer` to fail before any write

`TestTxResultFieldStreamLazy` — unit tests for `LazyFieldStream` with `prependSeparator=true`:

• `no_writes_when_unused`: creating the wrapper without writing leaves the buffer empty
• `writes_separator_and_field_on_first_value`: first write emits `,"result":` before the value
• `field_written_only_once`: `ensure()` is idempotent across multiple writes

`TestTraceBlockErrorAfterWrite` — regression test for `traceBlock` per-tx error handling: traces a block with an invalid tracer timeout, verifies the output is valid JSON with `txHash` and `error` inside each tx object.

`TestStackStream_Depth` — verifies `Depth()` tracks nesting correctly through object/array/field pushes and pops.

`TestStackStream_ClosePendingPreservesStack` — verifies the new `targetDepth` semantics: `ClosePending(N)` closes structures above depth N and leaves the stack at depth N for subsequent writes.

Notes

Supersedes #22079.

[Copilot]

Pull request overview

Fixes malformed/invalid JSON-RPC responses from tracing endpoints when execution fails before any trace output is written, by making "result" emission lazy and avoiding writing "result":null on error paths.

Changes:

• Remove unconditional WriteNil() on ExecuteTraceTx execution errors so JSON-RPC streaming handler can omit "result" when returning an error.
• Introduce txResultFieldStream to lazily emit ,"result": only when the tracer writes its first value, and update traceBlock to use it.
• Add tests covering early-error/exec-error streaming behavior and unit tests for the lazy wrapper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
rpc/transactions/tracing.go	Stops writing a null result on execution error to preserve JSON-RPC compliance in streaming mode.
rpc/jsonrpc/tracing.go	Adds a lazy "result" field wrapper and uses it in block tracing to avoid malformed JSON when tracing fails early.
rpc/jsonrpc/debug_api_test.go	Adds regression tests for “no stream writes on error” and unit tests for the lazy result wrapper.
.github/workflows/scripts/rpc_version.env	Updates the rpc-tests version reference used by CI workflows.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[yperbasis]

Thanks for tackling this — the lazy-result approach (extending the resultFieldStream mechanism from #21922 to the per-tx case) is the right direction, and the debug_traceCall insufficient-funds case is fixed correctly. A few issues need addressing before this can merge.

1. traceBlock emits malformed JSON when a tx errors after a write (regression)

In traceBlock's per-tx error handler:

if err != nil {
    if inner.written {
        _ = stream.ClosePending(1)
    }
    stream.WriteMore()
    rpc.HandleError(err, stream)
}

When inner.written == true at the error point, stream.ClosePending(1) is the wrong call here. At that moment the StackStream stack is [obj(response), arr(result), obj(tx)], and skipLast=1 protects only stack[0] (the outer response object) — so it closes the result array (stack[1]) along with the tx object, and then ClosePending resets the whole stack (s.stack = s.stack[:0]). The subsequent WriteMore() + HandleError() then write "error" as a sibling of result, and traceBlock's closing stream.WriteArrayEnd() emits a stray, unmatched ].

For a single-tx block the response comes out as:

{"jsonrpc":"2.0","id":1,"result":[{"txHash":"0x..","result":null}],"error":{...}}]}

which is invalid JSON. Pre-PR the same path produced valid JSON ({...,"result":[{...,"result":null,"error":{...}}]}), so this is a valid→invalid regression.

Reachable triggers (each leaves inner.written==true before the error handler):

• debug_traceBlockByNumber(n, {"tracer":"callTracer","timeout":"garbage"}), or an unknown tracer name → AssembleTracer fails → TraceTx calls stream.WriteNil() (see point 2) → inner.written=true;
• a custom tracer whose GetResult() errors;
• a FinalizeTx failure after a successful trace.

The new TestTraceErrorPathsWriteNoStream cases don't exercise this — they cover only early-return errors (genesis / not-found) and the single-trace TraceCall/TraceTransaction methods, none of which run the per-tx loop's error branch. A regression test that traces a multi-tx block with an invalid tracer config would have caught it.

2. The WriteNil-on-error removal is incomplete

Only the exec-error WriteNil (if !streaming { stream.WriteNil() }) and the asMessageErr branch were removed. The remaining WriteNil-on-error sites reintroduce the exact result + error coexistence this PR set out to eliminate:

• rpc/transactions/tracing.go — TraceTx, the AssembleTracer-failure branch (stream.WriteNil() immediately after the AssembleTracer call);
• rpc/transactions/tracing.go — ExecuteTraceTx, the non-streaming GetResult()-error and stream.Write(r)-error branches;
• polygon/tracer/trace_bor_state_sync_txn.go:58.

With a bad tracer name/timeout (or a tracer that throws in GetResult), debug_traceCall/debug_traceTransaction still return {...,"result":null,"error":{...}} — valid JSON, but result-XOR-error is violated, which is the same defect the PR is fixing elsewhere. This is also the root cause that triggers point 1 in traceBlock: removing these residual WriteNil calls keeps inner.written==false on every error path and fixes both points at once. (The ClosePending(1) in point 1 should still be dropped/corrected — it's a latent hazard for any future tracer that streams partial output before erroring, regardless of point 2.)

3. txResultFieldStream duplicates resultFieldStream

txResultFieldStream is a near-verbatim copy of resultFieldStream in rpc/handler.go: the ~25 forwarder methods are identical, and only ensure() differs (this one prepends WriteMore() for the leading comma inside the per-tx object). Consider generalizing the existing wrapper — e.g. a constructor flag for whether to emit a leading separator — rather than maintaining two copies that can drift out of sync.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

[yperbasis]

A few test/clarity items before this can go in:

• TestTraceBlockErrorAfterWrite doesn't cover the case it's named for. The garbage timeout makes AssembleTracer fail before any write, so inner.Written stays false and CloseIfOpen() is a no-op — the partial-result-then-error close path (the most intricate part of this PR) is never exercised through traceBlock; only TestStackStream_ClosePendingPreservesStack hits it, in isolation. The test also asserts Contains("error") but not NotContains("result"), so it would pass against the old buggy output ("result":null,"error":…) and wouldn't catch a regression. The docstring contradicts itself too — it says the error fires "after the result field has already been written (inner.written==true)", but the inline comment says inner.Written stays false. Please add a tracer/scenario that writes partial output and then errors (a real Written==true case) routed through traceBlock, add a NotContains("result") assertion, and fix the docstring.

• Needs a rebase. The debug_api_test.go and stack_stream_test.go hunks don't apply cleanly on the current main tip (the production-code hunks do).

• LazyFieldStream.ensure() — the if d := s.Stream.Depth(); d > 0 guard. This only matters for JsoniterStream (Depth()==0, no-op ClosePending), which production never uses (factory.go AutoCloseOnError=true → always StackStream). The guard silently defends a const-dead path; please drop it, or add a one-line note, so the "no closing" degradation isn't a surprise if that const ever flips.

[lupin012]

@yperbasis
Point 1 — test doesn't cover the Written==true path
Renamed the garbage-timeout test to TestTraceBlockErrorBeforeWrite with a corrected docstring and added NotContains("result") on every entry assertion.
For the Written==true path: a real Written==true + error scenario through TraceBlockByNumber I have not found a way to make I have Added TestTraceBlockErrorAfterWrite as a synthetic unit test that directly replicates the per-tx loop structure (open array → open tx object → write txHash → ResetField
→ partial write to inner → CloseIfOpen → WriteMore → write error field → close tx object), verifying that CloseIfOpen seals the partial result back to the tx-object level and that both "result" and "error" appear correctly in the output.

Point 2 — rebase done

Point 3 — if d > 0 guard in ensure()
Guard removed. ensure() now simply captures s.openDepth = uint(s.Stream.Depth()) before the WriteMore call, which gives the correct enclosing-object depth in both the separator and no-separator cases.

[yperbasis]

CI is red

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

[lupin012]

@yperbasis: CI was red because RPC_VERSION was not pinned to a branch version, but to the latest tag on main. I've now prepared it for merge with the new tag(v2.21.0) that contains the modified test