Skip to content

eth/filters: decouple client notification delivery from event fan-out - #2335

Open
mukul3097 wants to merge 1 commit into
0xPolygon:developfrom
mukul3097:fix/ws-subscription-fanout-starvation
Open

eth/filters: decouple client notification delivery from event fan-out#2335
mukul3097 wants to merge 1 commit into
0xPolygon:developfrom
mukul3097:fix/ws-subscription-fanout-starvation

Conversation

@mukul3097

Copy link
Copy Markdown

Problem

The filter EventSystem fans events out to every installed subscription from a single eventLoop goroutine using blocking channel sends (eth/filters/filter_system.go, handleTxsEvent et al.), and the per-subscription goroutines in eth/filters/api.go deliver to clients with a synchronous notifier.Notify. The two together create a back-pressure chain from an untrusted RPC client into shared node state:

slow WS client ← notifier.Notify ← api.go subscription goroutine ← subscription channel ← shared eventLoop

A WebSocket client that stops reading its connection — or reads just slowly enough never to trip the RPC write deadline — blocks its subscription goroutine in Notify, its subscription channel fills, and the shared eventLoop then blocks on that one subscriber. From that moment every subscription on the node starves: newPendingTransactions, newHeads, logs, transaction receipts, and state-sync deposits (they all share the loop, so the starvation crosses subscription types). The failure is deceptive because eth_subscribe keeps returning valid subscription IDs — the install channel interleaves between blocked sends — while delivery trickles at the pace of the slowest client.

Per the repo's own threat-model framing this is an RPC-user-triggerable DoS on a public endpoint: any single WS client can, accidentally or deliberately, suppress subscription delivery for all other clients of the node.

Production impact

We operate large Polygon PoS RPC infrastructure. On a mainnet full node (bor v2.9.0, 200 peers, at chain tip, txpool ingesting ~64 tx/s throughout), newPendingTransactions subscribers received 2–13 hashes per 15s instead of ~900 for several days. Restarting bor did not help — the offending client auto-reconnected through the load balancer and re-wedged the fresh process; the node recovered only when an LB restart severed all client sessions. We reported the symptom ("progressive mempool starvation on subscribe") through the operator channel in April without a reproduction; this PR includes the reproduction that was missing.

Fix

Insert a bounded queue between each subscription's event feed and the client write (notifyAsync / queueNotification in api.go): enqueueing never blocks, and a per-subscription goroutine drains the queue into notifier.Notify. A client that falls more than clientNotificationBuffer (512) notifications behind loses subsequent notifications for itself only.

Deliberate properties of this approach:

  • EventSystem semantics are untouched. In-process subscribers keep guaranteed, ordered, blocking delivery — all existing eth/filters tests pass unmodified. The isolation boundary sits exactly where the untrusted party (the RPC client) attaches.
  • The subscription goroutines now always drain their channels promptly, so the shared loop can never be held hostage by a connection.
  • Trade-off, stated explicitly: a genuinely-backlogged client silently misses notifications instead of freezing the node's subscription system for everyone (and previously, deadlocking healthy unsubscribes against the wedged loop). This matches the delivery semantics Polygon's Erigon already has for the same surface (rpc/rpchelper's chan_sub.Send drops on overflow), so the two clients become consistent under a slow consumer.

Alternatives considered: per-send timeouts in the fan-out loop (retains head-of-line blocking for the timeout duration, multiplied across subscribers); dropping at the EventSystem layer (breaks the guaranteed-delivery contract that TestBlockSubscription and TestTransactionReceiptsSubscription correctly encode for in-process consumers — rejected after trying it); relying on the RPC write deadline (already insufficient in practice — a trickling client never trips it).

Testing

  • New regression test TestSlowClientDoesNotStarveOtherSubscribers: a raw-pipe client subscribes and then stops reading; a healthy in-proc client must still receive all 200 events promptly. On current develop it fails with got 129 of 200 events — exactly the stalled subscriber's channel buffer (128) plus one in-flight before the shared loop froze. With this change it passes in ~1s.
  • Full eth/filters suite passes, including with -race (29/29).
  • Field verification: we canaried this fix on the affected production mainnet node. Delivery returned to parity with a healthy sibling (~1,300 notifications/15s each), and deliberately re-running the failure scenario — three concurrently wedged subscribers — left a healthy subscriber completely unaffected (~60 tx/s throughout). Multi-day soak shows no regressions.

Happy to adjust details (queue size, a metrics counter for dropped notifications, drop-oldest vs drop-newest) if maintainers prefer — the property we need is that one slow client cannot affect other subscribers.

The filter EventSystem fans events out to every installed subscription
from a single eventLoop goroutine using blocking channel sends, and the
per-subscription goroutines in the RPC API deliver to clients with a
synchronous notifier.Notify. A WebSocket client that stops reading (or
reads very slowly, never tripping the write deadline) therefore
back-pressures through its subscription channel into the shared loop:
one stalled client freezes newPendingTransactions, newHeads, logs,
receipts and state-sync delivery for every other subscriber on the
node, while eth_subscribe keeps returning valid IDs because installs
interleave with the blocked sends.

Observed in production on Polygon mainnet: a single stalled subscriber
reduced newPendingTransactions delivery for all other clients from
~900 to ~5 notifications per 15s for days; node restarts did not help
because the client reconnected immediately.

Insert a bounded queue between each subscription's event feed and the
client write: enqueueing never blocks, and a per-subscription goroutine
drains the queue into notifier.Notify. A client that falls more than
clientNotificationBuffer notifications behind loses subsequent
notifications for itself only; in-process EventSystem delivery
semantics are unchanged.

The regression test stalls a raw-pipe client after subscribing and
asserts a healthy client still receives all events promptly; without
this change it stalls after exactly buffer-size events (129 of 200).

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant