From a47cae0bfad0dbc5bcaa7a2e0325142de5fe690b Mon Sep 17 00:00:00 2001 From: lupin012 <58134934+lupin012@users.noreply.github.com.> Date: Mon, 17 Aug 2026 22:37:25 +0200 Subject: [PATCH] rpc/jsonrpc: fail trace_filter instead of mixing error objects into the result array trace_filter documents its result as Array, but filterV3 wrote per-transaction failures into that array as bare {"error":{...}} items and kept going, so the JSON-RPC envelope still reported success. Those items carry no type, action or traceAddress, so a strict decoder breaks on them and a lenient one silently accepts a partial trace as complete. Every iterator, lookup, marshal and execution failure now propagates instead. traceFilterTxn loses its "nil result means the error is already on the stream" convention and just returns the error, which also drops the nil check at the call site. The result array now opens on the first exported trace rather than up front. That way an error raised before any trace leaves runMethod's LazyFieldStream unwritten, so the response carries only "error" instead of "result" plus "error". When traces were already streamed, the handler's ClosePending seals the half-written array. Fixes #23128 --- rpc/jsonrpc/call_traces_test.go | 77 +++++++++++++++- rpc/jsonrpc/trace_filtering.go | 156 +++++++------------------------- 2 files changed, 103 insertions(+), 130 deletions(-) diff --git a/rpc/jsonrpc/call_traces_test.go b/rpc/jsonrpc/call_traces_test.go index 4aa98692142..3809387d97b 100644 --- a/rpc/jsonrpc/call_traces_test.go +++ b/rpc/jsonrpc/call_traces_test.go @@ -17,7 +17,9 @@ package jsonrpc import ( + "bytes" "context" + "encoding/json" "sync" "testing" @@ -383,9 +385,9 @@ func TestFilterRejectedBlockOverrideReturnsError(t *testing.T) { // TestFilterSignerReflectsBlockOverridesNumber is filterV3's analogue of // TestReplayTransactionSignerReflectsBlockOverridesNumber: filterV3 derives // fork rules (lastRules) from the overridden BlockContext but must also -// recompute lastSigner from it, not from the block's real number. filterV3 -// reports per-transaction failures as an "error" field inside the stream -// rather than as a Go error, so the assertion inspects the stream contents. +// recompute lastSigner from it, not from the block's real number. A +// transaction that cannot be traced must fail the whole request instead of +// mixing an error object into the result array. func TestFilterSignerReflectsBlockOverridesNumber(t *testing.T) { if testing.Short() { t.Skip("slow test") @@ -408,6 +410,71 @@ func TestFilterSignerReflectsBlockOverridesNumber(t *testing.T) { err := api.Filter(context.Background(), traceReq, new(bool), &config.TraceConfig{ BlockOverrides: ðapi.BlockOverrides{Number: (*hexutil.U256)(uint256.NewInt(1))}, }, stream) - require.NoError(t, err) - require.Contains(t, string(stream.Buffer()), "protected txn is not supported by signer") + require.ErrorContains(t, err, "protected txn is not supported by signer") + require.Empty(t, string(stream.Buffer())) +} + +// TestFilterErrorAfterExportedTracesKeepsValidJSON covers the other half of +// filterV3's error contract: when a transaction fails after earlier traces were +// already streamed, the request still fails, and the envelope stays valid JSON +// whose result array holds only TraceEntry items. Filtering blocks 1-3 with no +// address filter exports both empty blocks' reward traces before the protected +// transaction in block 3 is rejected by the overridden pre-EIP-155 signer. +// The envelope is assembled the way runMethod does it, since sealing the +// half-written array is the handler's job, not filterV3's. +func TestFilterErrorAfterExportedTracesKeepsValidJSON(t *testing.T) { + if testing.Short() { + t.Skip("slow test") + } + + c := newBaseFeeTestChain(t, delayedSpuriousDragonConfig()) + c.mineProtectedTxAtBlock3(t) + api := c.traceAPI() + + from, to := rpc.BlockNumber(1), rpc.BlockNumber(3) + traceReq := TraceFilterRequest{ + FromBlock: &rpc.BlockNumberOrHash{BlockNumber: &from}, + ToBlock: &rpc.BlockNumberOrHash{BlockNumber: &to}, + } + + var buf bytes.Buffer + stream := jsonstream.New(&buf) + stream.WriteObjectStart() + stream.WriteObjectField("jsonrpc") + stream.WriteString("2.0") + stream.WriteMore() + stream.WriteObjectField("id") + stream.WriteInt(1) + stream.WriteMore() + result := jsonstream.NewLazyFieldStream(stream, "result", false) + + err := api.Filter(context.Background(), traceReq, new(bool), &config.TraceConfig{ + BlockOverrides: ðapi.BlockOverrides{Number: (*hexutil.U256)(uint256.NewInt(1))}, + }, result) + require.ErrorContains(t, err, "protected txn is not supported by signer") + require.True(t, result.Written(), "test needs traces exported before the failure") + + result.CloseIfOpen() + stream.WriteMore() + rpc.HandleError(err, stream) + stream.WriteObjectEnd() + require.NoError(t, stream.Flush()) + + var envelope map[string]json.RawMessage + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), "envelope is not valid JSON: %s", buf.String()) + require.Contains(t, envelope, "error") + + var traces []json.RawMessage + require.NoError(t, json.Unmarshal(envelope["result"], &traces), "result array was left unsealed: %s", envelope["result"]) + require.NotEmpty(t, traces) + for i, trace := range traces { + var entry map[string]json.RawMessage + require.NoError(t, json.Unmarshal(trace, &entry), "item %d is not a JSON object", i) + require.Contains(t, entry, "type", "item %d is not a TraceEntry", i) + if reason, ok := entry["error"]; ok { + var failure string + require.NoError(t, json.Unmarshal(reason, &failure), + "item %d: error must be a TraceEntry failure reason, not an RPC error object", i) + } + } } diff --git a/rpc/jsonrpc/trace_filtering.go b/rpc/jsonrpc/trace_filtering.go index 2fbc0738260..750cd43ef60 100644 --- a/rpc/jsonrpc/trace_filtering.go +++ b/rpc/jsonrpc/trace_filtering.go @@ -412,8 +412,17 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB engine := api.engine() var json = jsoniter.ConfigCompatibleWithStandardLibrary - stream.WriteArrayStart() + // The result array opens on the first exported trace so that an error + // before it yields an error-only response instead of "result" plus "error". first := true + beginItem := func() { + if first { + stream.WriteArrayStart() + first = false + } else { + stream.WriteMore() + } + } // Execute all transactions in picked blocks count := uint64(^uint(0)) // this just makes it easier to use below @@ -438,20 +447,7 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB noop := state.NewNoopWriter() isPos := false - // traceFilterTxn traces one transaction. A nil result means the error was - // already written to the stream; the transaction state is closed either way. traceFilterTxn := func(blockNum, txNum uint64, txIndex int, txn types.Transaction, msg *types.Message) (*TraceCallResult, error) { - writeErr := func(err error) { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - } - stateCache := shards.NewStateCache(32, 0 /* no limit */) // this cache living only during current RPC call, but required to store state writes cachedReader := state.NewCachedReader(state.NewHistoryReaderV3(dbtx, txNum), stateCache) cachedWriter := state.NewCachedWriter(noop, stateCache) @@ -501,30 +497,23 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB if ot.Tracer() != nil && ot.Tracer().Hooks.OnTxEnd != nil { ot.Tracer().OnTxEnd(nil, timeoutErr) } - // Safe to skip FinalizeTx/CommitBlock: each iteration creates a fresh - // stateCache, cachedReader and ibs from the next txNum, and writes go - // to a noop writer, so no partial state escapes this scope. - writeErr(timeoutErr) - return nil, nil + return nil, timeoutErr } if execErr != nil { if ot.Tracer() != nil && ot.Tracer().Hooks.OnTxEnd != nil { ot.Tracer().OnTxEnd(nil, execErr) } - writeErr(execErr) - return nil, nil + return nil, execErr } if ot.Tracer() != nil && ot.Tracer().Hooks.OnTxEnd != nil { ot.Tracer().OnTxEnd(&types.Receipt{GasUsed: execResult.ReceiptGasUsed}, nil) } traceResult.Output = bytes.Clone(execResult.ReturnData) if err := ibs.FinalizeTx(evm.ChainRules(), noop); err != nil { - writeErr(err) - return nil, nil + return nil, err } if err := ibs.CommitBlock(evm.ChainRules(), cachedWriter); err != nil { - writeErr(err) - return nil, nil + return nil, err } return traceResult, nil } @@ -535,39 +524,15 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB } txNum, blockNum, txIndex, isFnalTxn, blockNumChanged, err := it.Next() if err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } if blockNumChanged { if lastHeader, err = api._blockReader.HeaderByNumber(ctx, dbtx, blockNum); err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } if lastHeader == nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(fmt.Errorf("header not found: %d", blockNum), stream) - stream.WriteObjectEnd() - continue + return fmt.Errorf("header not found: %d", blockNum) } if !isPos && chainConfig.TerminalTotalDifficulty != nil { @@ -595,15 +560,7 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB body, _, err := api._blockReader.Body(ctx, dbtx, lastBlockHash, blockNum) if err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } // Block reward section, handle specially minerReward, uncleRewards := ethash.AccumulateRewards(chainConfig, lastHeader, body.Uncles) @@ -612,22 +569,10 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB tr := newRewardTrace(lastBlockHash, blockNum, lastHeader.Coinbase, rewardTypeBlock, minerReward.ToBig()) b, err := json.Marshal(tr) if err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } if nSeen > after && nExported < count { - if first { - first = false - } else { - stream.WriteMore() - } + beginItem() if _, err := stream.Write(b); err != nil { return err } @@ -641,22 +586,10 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB tr := newRewardTrace(lastBlockHash, blockNum, uncle.Coinbase, rewardTypeUncle, uncleRewards[i].ToBig()) b, err := json.Marshal(tr) if err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } if nSeen > after && nExported < count { - if first { - first = false - } else { - stream.WriteMore() - } + beginItem() if _, err := stream.Write(b); err != nil { return err } @@ -674,15 +607,7 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB //fmt.Printf("txNum=%d, blockNum=%d, txIndex=%d\n", txNum, blockNum, txIndex) txn, ok, err := api._txnReader.TxnByIdxInBlock(ctx, dbtx, blockNum, txIndex) if err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } if !ok { continue //guess block doesn't have transactions @@ -690,24 +615,13 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB txHash := txn.Hash() msg, err := txn.AsMessage(*lastSigner, &lastBaseFee, lastRules) if err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } traceResult, err := traceFilterTxn(blockNum, txNum, txIndex, txn, msg) if err != nil { return err } - if traceResult == nil { - continue - } isIntersectionMode := req.Mode == TraceFilterModeIntersection for _, pt := range traceResult.Trace { if includeAll || filterTrace(pt, fromAddresses, toAddresses, isIntersectionMode) { @@ -718,22 +632,10 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB pt.TransactionPosition = &txIndexU64 b, err := json.Marshal(pt) if err != nil { - if first { - first = false - } else { - stream.WriteMore() - } - stream.WriteObjectStart() - rpc.HandleError(err, stream) - stream.WriteObjectEnd() - continue + return err } if nSeen > after && nExported < count { - if first { - first = false - } else { - stream.WriteMore() - } + beginItem() if _, err := stream.Write(b); err != nil { return err } @@ -742,7 +644,11 @@ func (api *TraceAPIImpl) filterV3(ctx context.Context, dbtx kv.TemporalTx, fromB } } } - stream.WriteArrayEnd() + if first { + stream.WriteEmptyArray() + } else { + stream.WriteArrayEnd() + } return nil }