fix(eth): keep the peer transaction broadcaster draining after a send error - #2520
fix(eth): keep the peer transaction broadcaster draining after a send error#2520gzliudan 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 |
There was a problem hiding this comment.
Pull request overview
Prevents transaction propagation from blocking when peer network sends fail.
Changes:
- Keeps transaction broadcast and announcement queues draining after failures.
- Buffers failure signals.
- Adds regression tests for both paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
eth/peer.go |
Updates broadcaster failure handling. |
eth/peer_test.go |
Adds send-failure draining tests. |
Suppressed comments (1)
eth/peer.go:287
- This loop has the same lifecycle leak as the full-transaction broadcaster: no production path closes
p.termwhenhandlereturns (removePeeronly unregisters and disconnects). Oncefailedis set, this goroutine therefore survives permanently holding the peer. Ensure teardown closestermexactly once (including concurrentremovePeercalls) so the drain state has a real endpoint.
// p.term is only closed once the peer handler unwinds, so this loop has to
// stay around as a reader or AsyncSendPooledTransactionHashes would block
// forever.
failed, queue, done = true, nil, nil
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
51483d0 to
92b4e9c
Compare
2a7e8e0 to
47e53a3
Compare
47e53a3 to
53ee99f
Compare
peerSet.Unregister never closed p.term, so the broadcast goroutines of every removed peer leaked for the lifetime of the process. Close term under the peer set lock so the draining loops wind down on removal.
broadcastTransactions and announceTransactions returned on the first network send error, leaving p.txBroadcast and p.txAnnounce without a reader while the peer was still registered; every later AsyncSendTransactions blocked forever. On mainnet that stalled txBroadcastLoop, filled pm.txsCh and pinned the transaction pool lock for hours. Stop sending after a failure but keep servicing the queue until p.term, discarding queued events once sending is hopeless. Buffer the fail channel so an in-flight sender whose error races the loop's term exit cannot block forever on an unbuffered send.
Guard the close of term with a sync.Once so repeated or concurrent calls cannot panic on a double close. Ownership of term stays with peerSet.Unregister; the once hardens the exactly-once invariant.
53ee99f to
dca69c9
Compare
Proposed changes
Fixes a production hang on mainnet where a peer with a broken connection stopped servicing its transaction broadcast queues, eventually stalling the transaction pool for hours.
Root cause
broadcastTransactions and announceTransactions returned on the first network send error, leaving p.txBroadcast / p.txAnnounce without a reader. p.term is only ever closed once the peer handler unwinds through removePeer — and in production it was never closed at all, because peerSet.Unregister was missing the p.close() call. A peer that was already tearing down could therefore sit with no reader and an open term, and every later AsyncSendTransactions / AsyncSendPooledTransactionHashes blocked forever.
On mainnet this stalled txBroadcastLoop, filled pm.txsCh and pinned the transaction pool lock for hours. Six peers were found in this state during inspection: they still had a live broadcastBlocks goroutine but no transaction broadcaster.
Changes
eth/peer.go
eth/peer_test.go
The drain tests reproduce the mainnet freeze: with the connection broken, they assert that repeated AsyncSend* calls never block, that exactly one write attempt is made, and that no further writes happen after the failure.
Compatibility / impact
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