fix(eth): move initial transaction sync off the peer handshake path - #2540
Open
gzliudan wants to merge 2 commits into
Open
fix(eth): move initial transaction sync off the peer handshake path#2540gzliudan wants to merge 2 commits into
gzliudan wants to merge 2 commits into
Conversation
handle() called syncTransactions between registering a peer and entering its message loop, so a peer could not read a single message until txpool.Pending returned. While the pool lock was held the peer answered nothing and the downloader eventually dropped it with err=timeout. A stack dump from a frozen mainnet node showed 20 peer registrations parked in makeProtocol -> handle -> syncTransactions -> Pending. The freeze itself is fixed elsewhere: the pool lock was pinned by promoteSpecialTx delivering an event while holding it, and by a peer broadcaster that stopped draining its queue after a send error. This change only removes the coupling, so peer registration no longer depends on the transaction pool being responsive, and a full Pending scan stays off the handshake path even when the pool is healthy. Move the call into a goroutine tracked by pm.wg. Since the sync now runs concurrently with the peer message loop, filter the pending set against the peer's known transactions so transactions received from the peer after registration are not echoed back to it, and abandon the sync if shutdown started while the pool was being scanned. Stop() waits for the sync goroutine via pm.wg, so a pinned pool lock can still delay shutdown, same as before when handle() itself was parked in Pending. Concurrency note: p.knownTxs is a mapset.NewSet from golang-set/v2 v2.7.0, whose operations are individually locked, so the concurrent reader added here is safe. Compound sequences such as Cardinality check then Pop then Add remain non-atomic across calls, which can only cause benign over/under- marking, not a crash. Add TestPeerServesRequestsWhileTxPoolIsBlocked, which serves a header request while Pending is blocked, and split newTestProtocolManagerWithTxPool out of newTestProtocolManager so the test can inject a blocking pool.
Running the initial transaction sync off the handshake path keeps peers usable when the transaction pool is contended, but it also makes a wedged pool silent: peers look healthy while their tx sync never completes. Nothing in the logs would point at the pool anymore. Watch the sync from its own goroutine and warn on the peer's logger if it is still pending after txSyncStatusLogCycle, which is much shorter than syncStatusLogCycle so a wedged pool shows up within a minute instead of tens of minutes. A sync normally finishes in milliseconds, so only one that is still pending a full cycle later is reported, which keeps healthy churn from producing warnings. A sync that finished in time, a peer that was dropped, or a node that began shutting down is not reported. The warning logs the actual elapsed duration rather than the configured cycle. Add TestTxSyncStallWatch covering the stall detection, including the finished, still-running, dropped-peer, and shutting-down paths. This watchdog is an XDC-specific enhancement: upstream go-ethereum has no equivalent, so there is no upstream pattern to follow here.
|
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 |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
August 24, 2026 10:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Summary
ProtocolManager.handle()ransyncTransactionssynchronously between registering a peer and entering its message loop, so a peer could not read a single message untiltxpool.Pendingreturned. While the pool lock was held, the peer answered nothing and the downloader eventually dropped it witherr=timeout. A stack dump from a frozen mainnet node showed 20 peer registrations parked inmakeProtocol -> handle -> syncTransactions -> Pending.The freeze itself is fixed elsewhere (the pool lock was pinned by
promoteSpecialTxdelivering an event while holding it, and by a peer broadcaster that stopped draining its queue after a send error). This PR removes the coupling so peer registration no longer depends on the transaction pool being responsive, and adds a watchdog so a wedged pool stays visible in the logs instead of going silent.Changes
Commit 1 —
refactor(eth): move initial transaction sync off the peer handshake pathpm.wg, so the peer enters its message loop immediately and a fullPendingscan stays off the handshake path even when the pool is healthy.p.termwhen handing the batch to the txsync loop, so a dropped peer never parks the sync goroutine.Stop()waits for the sync goroutine viapm.wg, so a pinned pool lock can still delay shutdown — same as before, whenhandle()itself was parked inPending.Commit 2 —
feat(eth): report stalled initial transaction syncstxSyncStallWatch, a per-peer watchdog goroutine that reports at warn level once the sync is still pending aftertxSyncStatusLogCycle(1 minute, much shorter thansyncStatusLogCycle), then re-reports everytxSyncStallRepeatCycle(10 minutes) for as long as it remains pending — so a permanently wedged pool keeps warning for already connected peers instead of going silent after the first report.txpool/pendingandtxpool/queuedgauges (read atomically, so the diagnostic itself can never be blocked by the contended pool it reports on).Notes
p.knownTxsis amapset.NewSetfrom golang-set/v2 v2.7.0, whose operations are individually locked, so the concurrent reader added here is safe. Compound sequences such as Cardinality-check-then-Pop-then-Add remain non-atomic across calls, which can only cause benign over/under-marking, not a crash.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