rpc/jsonrpc: push fresh witnesses via debug_subscribe("executionWitnesses")

[awskii]

zilkworm pulls witnesses; PR 1 (erigontech/z6m#114) gave it the WS transport and tip watermark, but each block still costs a debug_executionWitness round trip. This adds the node-side push: every witness the eager cache builder completes is pushed to subscribers.

Changes

• debug_subscribe("executionWitnesses", {encoding?}): fresh-only, stateless; per-subscriber cap-4 channel with drop-oldest fan-out (common.PrioritizedSend)
• feed rides on the witness cache object; both build paths — durable and head-capture — insert through store, so every built witness is also published, reusing the cache's pre-marshaled JSON with no per-subscriber marshal
• notification {blockNumber, blockHash, witness}; reorgs re-push the same height with the new hash via the existing rebuild
• encoding param reserved (json only; rlp later); nil cache (standalone rpcdaemon / flag off) → explicit error
• subscribeRPC generalized: the filters-nil guard moved into its callers so any namespace can reuse the shared pump
• witness_feed_drop_total and witness_feed_subscribers; no new flags — embedded-only, gated by --witness.cache.blocks

[Copilot]

Pull request overview

This PR adds a new debug_subscribe("executionWitnesses") WebSocket subscription that pushes freshly built execution witnesses to subscribers as the eager witness cache completes them, avoiding an extra debug_executionWitness round-trip per new block for tip-following consumers.

Changes:

• Add a witness push feed (witnessFeed) and wire it into the eager witness cache insert path so every cached witness is also published.
• Implement DebugAPIImpl.ExecutionWitnesses subscription endpoint with encoding validation (currently JSON-only) and explicit erroring when the embedded cache is unavailable.
• Generalize the shared subscription pump (subscribeRPC) so it can be reused beyond filter-backed subscriptions; add focused tests for feed + subscription wiring.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
rpc/jsonrpc/witness_subscription.go	Adds the executionWitnesses debug subscription endpoint and payload types.
rpc/jsonrpc/witness_subscription_test.go	Tests encoding validation, nil-cache behavior, notifier behavior, and end-to-end WS delivery.
rpc/jsonrpc/witness_feed.go	Introduces a non-blocking fan-out feed with drop-oldest behavior for slow subscribers.
rpc/jsonrpc/witness_feed_test.go	Tests fan-out, unsubscribe behavior, overflow drop policy, and concurrency.
rpc/jsonrpc/witness_cache.go	Reworks witness cache into a struct that includes an internal push feed and a single store+publish path.
rpc/jsonrpc/witness_cache_wiring_test.go	Validates builder/serve-side share the same cache+feed and that builder publishes reach subscribers.
rpc/jsonrpc/witness_cache_test.go	Updates cache tests for the new struct-based cache layout.
rpc/jsonrpc/witness_cache_builder.go	Routes cache inserts through a shared storeWitness method that also publishes to the feed.
rpc/jsonrpc/witness_cache_builder_test.go	Adds a test ensuring storeWitness both caches and publishes verbatim bytes.
rpc/jsonrpc/eth_filters.go	Updates subscribeRPC signature and moves the filters == nil guard into callers.
rpc/jsonrpc/debug_api_test.go	Updates cache-hit tests to use the embedded LRU field.

---

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

[AskAlexSharov]

Design and tests look solid — the fan-out is small, the ownership of the pre-marshaled bytes is clear, and the wire-dispatch test covers teardown. One item must be resolved before merge: the type this PR restructures was rewritten on main.

Merge main first — witnessResultCache changed under you

The branch base is 56 commits behind main. #22663 ("serve debug_executionWitness on minimal nodes via head-capture") rewrote witnessResultCache:

• it now embeds *lru.Cache[...], so Get/Contains/Len are already promoted — the wrappers added in witness_cache.go become dead weight
• newWitnessResultCache takes 4 args (blocks, maxBytes, headCapture, cacheOnly)
• Add is overridden for resident-byte accounting, so main already has one insert choke point

Most important: main added a second build+insert site, tryHeadCaptureBuild (rpc/jsonrpc/witness_cache_builder.go:495). A plain merge keeps storeWitness only in buildAndCache, so a head-capture node would fill the cache and never publish anything. That silently turns the feature off on exactly the node type that wants push most.

Suggestion: keep store(num, hash, enc) as the single wrapper and call it from both build sites. Publishing from witnessResultCache.Add would be cleaner, but ExecutionWitnessResult carries no block number, so the wrapper is simpler.

The drop rationale does not hold in cache-only mode

witness_feed.go:38 says a dropped push "stays servable by an on-demand debug_executionWitness request". That is true on a normal node — it recomputes from commitment history. It is not true under --witness.cache.head-capture: CacheOnly() makes a serve miss return errWitnessOutOfWindow and never recompute (debug_execution_witness.go:737). There a dropped block is recoverable only while the LRU entry is still resident, bounded by --witness.cache.blocks and --witness.cache.maxmb. Please narrow that comment after the merge.

Drops are invisible in production

The only signal for a slow subscriber is log.Debug (witness_feed.go:75). The package already has witness_cache_* counters, so two cheap additions fit the existing style:

• witness_feed_drop_total counter on the overflow branch
• witness_feed_subscribers gauge on subscribe/unsubscribe — this also gives subCount() a non-test caller

publish holds the lock across a blocking send

common.PrioritizedSend ends with a plain ch <- msg. It is safe at witnessFeedBuffer = 4: the drain frees cap/2 = 2 slots and publish is the only writer under f.mu. But cap(ch)/2 == 0 for a buffer of 1, and then the builder goroutine blocks forever while holding the feed lock. The buffer is a tuning constant, so this is worth a guard.

Also the select/default in publish repeats the non-blocking attempt PrioritizedSend already makes. Only the log line needs the two-step.

Stale doc comment on subscribeRPC

eth_filters.go:156 still says "subscribe is called inside the goroutine". It is called synchronously, before CreateSubscription. That detail is load-bearing here: it is why a witness finished between the call and the client receiving the subscription ID lands in the feed channel instead of being lost. The rewritten comment is a good place to state it correctly.

Tests bypass the invariant they document

cache.lru.Add(...) in debug_api_test.go and witness_cache_test.go reaches past store, right under the comment saying store is the only insert path. store works in those tests — with no subscribers, publish is a no-op. This resolves itself after the merge, since main promotes Add.

Nits

• ExecutionWitnesses returns nil on its two pre-checks, while the four sibling subscription methods return &rpc.Subscription{}. Harmless — the handler takes the subscription from the notifier — but inconsistent.
• fmt.Errorf with no verbs → errors.New.
• --witness.cache.blocks in docs/site/docs/fundamentals/configuring-erigon.mdx:324 is where a user learns the flag exists. One sentence saying it also gates debug_subscribe("executionWitnesses") would help.

Checked, no problem

• Overflow order: publishing 1..6 into a cap-4 channel keeps {3,4,5,6}, matching PrioritizedSend's drop-half. The test asserts exactly that.
• No aliasing hazard from sharing one enc between the LRU entry and every subscriber: MarshalFastJSON returns either the cached slice or a fresh json.Marshal buffer, nothing pooled, and nobody mutates it.
• No lost notification at subscription start: the feed channel is registered synchronously, and RemoteNotifier buffers until the subscription ID is sent.
• ExecutionWitnesses registers as a subscription callback only (isPubSub), so no stray debug_executionWitnesses method appears.
• Pump teardown: defer unsubscribe() runs before dbg.LogPanic(), so a panic still deregisters the subscriber.
• The four api.filters == nil guards cover every subscribeRPC caller, and keep the original check order.

[AskAlexSharov]

Correction to my earlier comment, with the actual merge result.

git merge-tree origin/main awskii/witness-push conflicts in 4 files:

rpc/jsonrpc/witness_cache.go
rpc/jsonrpc/witness_cache_test.go
rpc/jsonrpc/witness_cache_builder_test.go
rpc/jsonrpc/debug_api_test.go

witness_cache_builder.go is not in that list — git auto-merges it, and the result is wrong. I said the head-capture path would lose the publish. It is the reverse:

// buildAndCache — normal commitment-history path
api.witnessCache.Add(hash, &ExecutionWitnessResult{cachedJSON: enc})   // publish lost
witnessCacheEntriesResidentGauge.SetInt(api.witnessCache.Len())

// tryHeadCaptureBuild — head-capture path
api.storeWitness(num, hash, enc)                                       // publish landed here

The cause: main's tryHeadCaptureBuild tail (MarshalFastJSON → Add → witnessCacheBuildOKCounter.Inc()) is byte-identical to the old buildAndCache tail, so your storeWitness hunk applies there instead. Main's newer buildAndCache wins as-is.

Net effect after a naive merge: a normal node stops pushing entirely — the main use case — while only head-capture nodes push. No conflict marker warns about it. It compiles as long as store survives your hand-resolution of witness_cache.go; drop store there and the build breaks instead, which is the luckier outcome.

Worth adding a test that pins the invariant rather than relying on review — assert both build paths publish, so a future refactor of either one cannot silently unhook the feed.

[awskii]

Merged main (1c9c4a9) and resolved the findings in 2875e2c.

The auto-merge hazard

Confirmed exactly as you described: after the merge, storeWitness sat on tryHeadCaptureBuild and buildAndCache kept main's direct Add. Both go through storeWitness now.

TestBuildPathsPublish pins it behaviorally — each path is driven end-to-end with a subscriber attached (durable via RunWitnessCacheBuilder, head-capture via buildAndCacheHeadCapture) and must publish the bytes it cached. Mutation-checked both ways: reverting either call site to witnessCache.Add fails the matching subtest and nothing else.

witnessResultCache

Took main's version wholesale. The Get/Contains/Len wrappers are gone — the embedded pointer promotes them — and the cache carries feed alongside the mode fields. store is now a thin wrapper over the overridden Add plus the publish.

Drop rationale

Narrowed. It no longer claims a dropped block stays servable; recovery is a re-request, and a node without commitment history can only serve it while that entry is still resident.

Drop visibility

Added witness_feed_drop_total and witness_feed_subscribers. The gauge is set from subCount(), which gives it a production caller.

Blocking send under the lock

Guarded at compile time rather than by comment:

const witnessFeedBuffer = 4

// PrioritizedSend ends in a blocking send after freeing cap/2 slots, so a buffer under
// 2 frees nothing and would stall the builder under the feed lock.
const _ = uint(witnessFeedBuffer - 2)

Setting the buffer to 1 now fails the build with constant -1 overflows uint. I kept the outer select/default: it is what distinguishes a drop from a normal send, so both the counter and the log line need it.

subscribeRPC comment

Rewritten — subscribe runs synchronously, before the subscription exists, which is why a witness finished mid-handshake queues instead of being lost. Same fix covers Copilot's inline comment.

Nits

• ExecutionWitnesses returns &rpc.Subscription{} on both pre-checks, matching its siblings.
• The nil-cache error is an errors.New sentinel (errWitnessSubscriptionNeedsCache); the test asserts identity rather than prose.
• --witness.cache.blocks in configuring-erigon.mdx now says it also gates the subscription.

make lint clean; go test ./rpc/jsonrpc/ green, including -race.

[Copilot]

Pull request overview

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

Suppressed comments (2)

rpc/jsonrpc/witness_subscription.go:37

• The comment refers to debug_subscription("executionWitnesses"), but the RPC entrypoint is debug_subscribe. This is likely a typo and can confuse readers when grepping for the API name.

// WitnessNotification is one debug_subscription("executionWitnesses") payload: the
// completed block's number and hash plus its witness as raw pre-marshaled JSON.

rpc/jsonrpc/witness_subscription.go:53

• validateWitnessEncoding returns a plain error, which will be encoded as JSON-RPC error code -32000. Since this is a user parameter validation failure, it should return *rpc.InvalidParamsError so clients get the standard -32602 invalid params code.

	case "", "json":
		return nil
	default:
		return fmt.Errorf("unsupported witness encoding %q (supported: json)", opts.Encoding)
	}

[awskii]

Copilot suppressed two comments; taking one:

• Invalid params code — applied in c69f914. A rejected encoding is a parameter failure, so validateWitnessEncoding now returns *rpc.InvalidParamsError (-32602) like the rest of the package, with a test pinning the code.
• debug_subscription "typo" — not a typo, leaving it. rpc/json.go:40 sets notificationMethodSuffix = "_subscription" and rpc/subscription.go:232 builds the notification method as namespace + suffix, so notifications on this subscription really do arrive as debug_subscription. The comment documents the payload, not the entrypoint.

[AskAlexSharov]

Approving. The design is sound and the tests pin the parts that matter. A few things to fix or confirm; none of them block.

1. witness_feed_subscribers can stick at a wrong value

subscribe/unsubscribe mutate subs under f.mu, release the lock, then call subCount(), which takes the lock again. Two concurrent unsubscribes can publish the gauge in the reverse order of their mutations, so the gauge stays at 1 after the last subscriber left, until the next sub/unsub repairs it.

Set the gauge in the same critical section:

func (f *witnessFeed) subscribe() chan witnessPush {
	ch := make(chan witnessPush, witnessFeedBuffer)
	f.mu.Lock()
	f.subs[ch] = struct{}{}
	witnessFeedSubscribersGauge.SetInt(len(f.subs))
	f.mu.Unlock()
	return ch
}

Same for unsubscribe. subCount() then stays test-only.

2. witness_feed_drop_total counts overflow events, not dropped witnesses

PrioritizedSend discards up to cap(ch)/2 queued pushes per overflow — 2 with witnessFeedBuffer = 4 — but publish increments the counter once. An operator who reads witness_feed_drop_total to answer "how many witnesses did this subscriber lose?" gets an undercount of up to 2x. Either rename it to witness_feed_overflow_total, or inline the drain and count the real discards.

3. The push stream is not contiguous, and nothing on the wire says so

shouldBuild builds only for a single-header advance that is not already cached. A catch-up burst (multi-header batch), a stale-pin skip, or any build failure produces no push for those heights. That is the intended contract, but a client learns about it only by watching blockNumber for holes.

The line added to configuring-erigon.mdx reads "pushes each witness over WebSocket as the cache builds it", which sounds contiguous. Worth one clause there and on the ExecutionWitnesses docstring: pushes are best-effort, gaps are normal during catch-up, and the client re-requests the missing heights with debug_executionWitness.

4. ExecutionWitnesses is missing from the PrivateDebugAPI interface

daemon.go:158 registers Service: PrivateDebugAPI(debugImpl). It works today because registerName reflects the dynamic type (*DebugAPIImpl), so a method outside the interface is still found. But the interface no longer describes the debug surface, and TestWitnessSubscriptionWireDispatch registers the concrete api, so it does not exercise the production registration shape. Add the method to the interface, and register PrivateDebugAPI(api) in that test.

5. No cap on subscribers

The debug namespace is Public: true. Every subscription adds a channel that publish walks while holding f.mu, on the builder's critical path. Per-subscriber work is bounded — PrioritizedSend always frees a slot before its blocking send, given witnessFeedBuffer >= 2 — so this is not a stall. But there is no limit. With the double gate (--witness.cache.blocks plus an exposed WS debug namespace) I think it is acceptable; just confirm it is a conscious choice.

Minor

subscribeRPC returns nil, err on the subscribe-error path, while the two guards above it return &rpc.Subscription{}, err. Pre-existing, but the function is already being touched.

Verified while reading

• store is the single insert on both build paths. TestCacheAddAloneDoesNotPublish and TestBuildPathsPublish catch a regression back to a bare Add — good tests, they name the bypass instead of only asserting the happy path.
• The enc aliasing between the cache entry and witnessPush.json is safe: MarshalFastJSON returns a fresh json.Marshal buffer with no pooling, and nothing mutates it after the store. Extra retention is bounded by the buffered pushes, which point at the same bytes the LRU holds.
• const _ = uint(witnessFeedBuffer - 2) is a good compile-time guard for the cap/2 assumption inside PrioritizedSend.
• Moving the filters == nil guard into the callers also fixed a stale comment: subscribe() has always run synchronously, not inside the goroutine.
• go test ./rpc/jsonrpc/ -run 'TestWitnessFeed|TestWitnessSubscription|TestWitnessNotification|TestWitnessCacheStorePublishes|TestCacheAddAloneDoesNotPublish|TestWitnessCacheWiringSharedFeed' is green locally.

[awskii]

Thanks. All five fixed in 2ff1c0d, plus the minor.

1. Gauge race — real, fixed as suggested. Both subscribe and unsubscribe now set the gauge inside the critical section from len(f.subs). subCount() is test-only again.

2. Drop counter — renamed to witness_feed_overflow_total. It counts overflow events, so the name now says that; I did not inline the drain, since duplicating PrioritizedSend to get an exact discard count buys little when the actionable signal is "subscribers are falling behind".

3. Gaps — you are right that the docs line implied contiguity. Both the mdx and the ExecutionWitnesses docstring now say the stream is best-effort, that catch-up bursts, skipped or failed builds and slow subscribers leave holes, and that a client needing every height re-requests with debug_executionWitness.

4. Interface — added, and the wire test registers PrivateDebugAPI(api). One correction worth recording: this does not make the test pin interface membership. I removed the method from the interface again and TestWitnessSubscriptionWireDispatch still passes — RegisterName reflects the dynamic type, exactly as you said, so the interface conversion never narrows the visible method set. The value is that the interface once again describes the debug surface; signature drift is already caught at compile time by the PrivateDebugAPI(debugImpl) conversion in daemon.go. I did not add a var _ PrivateDebugAPI assertion for that reason.

5. Subscriber cap — conscious, no cap. It needs --witness.cache.blocks set and debug exposed over WS; an operator who has published debug already hands out debug_traceTransaction, so a subscriber cap is not the control that matters. Per-subscriber cost is one buffered channel and the bounded PrioritizedSend, and the compile-time witnessFeedBuffer >= 2 guard keeps publish off a blocking send. Happy to add a cap if you would rather have the ceiling.

Minor — subscribeRPC now returns &rpc.Subscription{}, err on the subscribe-error path, matching the two guards above it.

make lint clean; go test ./rpc/jsonrpc/ green including -race.