fix(core/txpool,eth): prevent a stalled tx subscriber from freezing the pool - #2519
fix(core/txpool,eth): prevent a stalled tx subscriber from freezing the pool#2519gzliudan wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d7614e7 to
18dd2d4
Compare
There was a problem hiding this comment.
Pull request overview
Moves special-transaction event delivery outside the txpool lock to prevent stalled subscribers from freezing pool operations.
Changes:
- Queues special transaction events through the reorg path.
- Adds announcement limits and revised shutdown behavior.
- Adds regression tests for stalled subscribers and reset handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
core/txpool/legacypool/legacypool.go |
Revises event delivery, reorg scheduling, and shutdown handling. |
core/txpool/legacypool/legacypool_test.go |
Adds regression tests for event delivery and queue behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ff3fbb0 to
b70f81c
Compare
b70f81c to
3b1a55b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (5)
eth/peer.go:128
- This meter misses the drops caused by the failure mode it is intended to expose. While the network writer is stalled,
broadcastTransactionscontinues drainingtxBroadcastand truncates its internal hash queue ateth/peer.go:227-230, so the outer channel may never fill and those discarded hashes are not counted. Mark the number removed at that truncation site as well so this metric actually surfaces stalled-peer loss.
txBroadcastDropMeter = metrics.NewRegisteredMeter("eth/peer/transaction/broadcasts/drop", nil)
eth/peer.go:132
- The announcement meter has the same observability gap:
announceTransactionskeeps consuming this channel while its writer is stalled, then silently removes excess hashes ateth/peer.go:289-292. Consequently, sustained stalled-writer drops can remain invisible. Increment this meter by the number removed at the internal queue cap too.
txAnnounceDropMeter = metrics.NewRegisteredMeter("eth/peer/transaction/announces/drop", nil)
eth/peer.go:148
- The buffer is bounded by batch count, but each batch can contain thousands of hashes; this bypasses the existing 4,096-hash queue caps and can retain many times that amount per peer when consumers stop. Bound the buffered work by hash count (for example, cap/split batches before enqueueing) so stalled peers cannot amplify memory use across all connected peers.
txBroadcast: make(chan []common.Hash, txBatchBuffer),
txAnnounce: make(chan []common.Hash, txBatchBuffer),
eth/peer.go:60
- The central non-blocking guarantee is untested:
eth/peer_test.gohas no coverage for either asynchronous transaction queue. Add tests that saturate both queues, assert subsequent calls return without blocking, verify dropped batches are not marked known, and check the drop counters; otherwise a blocking-send regression in this production freeze fix will pass CI.
// announcements) that may be queued for a peer before newer batches are
// dropped. Together with the non-blocking send in the AsyncSend* methods,
// it guards the shared broadcast loop against a stalled or exited per-peer
// writer blocking it indefinitely.
txBatchBuffer = 16
core/txpool/txpool.go:143
- The production
TxPool.Closeordering is not exercised by the added directLegacyPool.Closetest. Add a coordinator-level test with a subpool whoseClosewaits for its transaction subscription to be cancelled, then assertTxPool.Closecompletes; this locks in the requirement thatp.subs.Close()runs before subpool shutdown.
// terminating the subpools: a subscriber that stopped draining its channel
// would otherwise leave a subpool's runReorg blocked in txFeed.Send, and the
// subpool Close (wg.Wait) would hang indefinitely. Closing the scope removes
// the stuck subscription and unblocks Send.
p.subs.Close()
833a565 to
9a39af2
Compare
promoteSpecialTx called txFeed.Send while holding pool.mu, so a subscriber that stopped draining froze the whole pool. On mainnet a peer lost its transaction broadcaster, AsyncSendTransactions blocked, txBroadcastLoop stopped draining pm.txsCh, and the resulting Feed.Send pinned pool.mu for hours: 131 goroutines piled up in Add, 20 in Pending, runReorg never ran and the node stopped importing blocks while RPC stayed responsive. Route the event through queueTxEvent like every other path in add(), so runReorg delivers it after releasing the lock. Add TestSpecialTxPromotionDoesNotBlockOnTxFeed, which adds a special tx while a subscriber refuses to read and asserts both the add and a later pool read complete, and that the tx was actually promoted to pending.
AsyncSendTransactions and AsyncSendPooledTransactionHashes blocked on
unbuffered txBroadcast/txAnnounce when a peer's writer stalled but the
peer was not yet terminated. Since txBroadcastLoop is a single goroutine
iterating peers sequentially, one bad peer froze it, pm.txsCh filled up
and txFeed.Send stalled the whole reorg pipeline.
Buffer the channels (txBatchBuffer) and drop batches when the queue is
full, matching AsyncSendNewBlock/AsyncSendNewBlockHash, and expose drop
meters to surface stalled peers.
Note for node operators: this intentionally diverges from upstream geth,
where AsyncSend* block until the peer's writer accepts the batch. With
this change tx propagation/announcement to a peer is best-effort — a
peer whose queue is full misses the dropped batches until reconnect
(hashes are left unmarked, so it stays eligible for later re-broadcast,
but txpool events only fire for newly added transactions). The drop
meters (eth/peer/transaction/{broadcasts,announces}/drop) are the only
signal that propagation is being lost.
A subscriber that stopped draining its channel left runReorg blocked in txFeed.Send, so close(done) never fired, the reorg loop stopped advancing and LegacyPool.Close (wg.Wait) hung, forcing a hard SIGKILL to terminate. - LegacyPool: track subscriptions in a SubscriptionScope and Close it before wg.Wait() so an unsubscribed channel is removed and the blocked Send returns. - TxPool.Close: unsubscribe listeners (subs.Close) before terminating subpools, so the subpool reorg loop is not left stuck waiting on a wedged subscriber. This complements the non-blocking peer broadcast fix: the peer layer no longer stalls, and a stuck subscriber can no longer block graceful shutdown. Add TestLegacyPoolCloseUnblocksStalledSubscriber, which stalls a subscriber, confirms a sync Add wedges in Send, then asserts Close returns.
9a39af2 to
dac8165
Compare
Proposed changes
Problem
On mainnet a peer lost its transaction broadcaster. AsyncSendTransactions blocked on the stalled peer writer, which froze txBroadcastLoop, filled pm.txsCh, and eventually pinned pool.mu via a txFeed.Send issued while holding the legacy pool lock. The node kept answering RPC but stopped importing blocks: 131 goroutines piled up in Add, 20 in Pending, and runReorg never ran again.
Root cause
LegacyPool.promoteSpecialTx delivered the special-tx event with pool.txFeed.Send(...) while holding pool.mu. event.Feed.Send blocks until every subscriber drains its channel, so a single subscriber that stops reading wedges the entire pool.
Fix
Three layered changes, each independently revertible: (1) stop delivering special tx events under the pool lock — route the event through queueTxEvent (the same async path every other add() branch uses) so runReorg delivers it after releasing the lock; (2) make peer transaction broadcasts non-blocking — buffer txBroadcast/txAnnounce (txBatchBuffer) and drop a batch when the queue is full, matching AsyncSendNewBlock/AsyncSendNewBlockHash, with drop meters to surface stalled peers; (3) unblock shutdown on stalled tx event subscribers — track subscriptions in a SubscriptionScope and Close it before wg.Wait() so a stuck subscriber is removed and the blocked txFeed.Send returns, and unsubscribe listeners in TxPool.Close before terminating subpools.
Scope / follow-up
These commits guarantee the node never freezes, never loses transactions, and shuts down gracefully. A non-peer in-process subscriber that stalls will still freeze the reorg pipeline (stale pending, halted proactive broadcast, growing queued events) until it unsubscribes or the node restarts — this is the deferred L2 hardening (decouple event delivery from the reorg done, bounded drop queue), not done here because no such subscriber exists in the standard deployment.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that