fix(eth/downloader): don't mistake a lagging head for a stalling peer - #2515
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
Fixes false errStallingPeer detection during fast sync when block imports temporarily move the current header backward.
Changes:
- Tracks the highest successfully inserted header for the current sync cycle.
- Uses that header for the terminating TD check when
CurrentHeaderlags. - Adds regression tests across supported protocol versions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
eth/downloader/downloader.go |
Corrects the fast/light sync stalling-peer check. |
eth/downloader/downloader_test.go |
Simulates header-head lag and verifies successful fast sync. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6d0fc63 to
860f906
Compare
860f906 to
5f581b5
Compare
5f581b5 to
9d7f977
Compare
9d7f977 to
4a663db
Compare
e18d48a to
4141121
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
eth/downloader/downloader_test.go:1159
- This test cannot reliably prove that its own two sync cycles increment the counter while it runs in parallel: increments from the other eight cases can satisfy
delta >= 2even if this case never records an occurrence. Run these metric-delta tests serially (or use an instance-local hook/counter) so regressions are not masked.
t.Parallel()
4141121 to
b5d376f
Compare
Importing the post-pivot blocks moves the header head back to the block being inserted, so CurrentHeader can trail the headers the peer already delivered. When processHeaders reached the terminating batch inside that window, the fast sync TD check saw a head below the promised total difficulty and returned errStallingPeer, which rolled the whole chain back and dropped the only peer. Track the highest header written to the light chain in this cycle and use it when it is ahead of CurrentHeader. Attack detection is unchanged: a peer that bails out of delivering the post-pivot headers never advances it. This is what made TestFastSyncDisabling flaky under load: once the peer was dropped, BestPeer stayed nil and the sync loop could never recover.
b5d376f to
4d18e40
Compare
Proposed changes
Importing the post-pivot blocks moves the header head back to the block being inserted, so
CurrentHeadercan trail the headers the peer already delivered. WhenprocessHeadersreached the terminating batch inside that window, the fast sync TD check saw a head below the promised total difficulty and returnederrStallingPeer, which rolled the whole chain back and dropped the only peer. This is what madeTestFastSyncDisablingflaky under load: once the peer was dropped,BestPeerstayed nil and the sync loop could never recover.processHeadersnow tracks the highest header written to the light chain in this cycle (lastInserted) and uses it when it is ahead ofCurrentHeader. Attack detection is unchanged: a peer that bails out of delivering the post-pivot headers never advances it, soTestHighTDStarvationAttack*still returnserrStallingPeer.The stalling-peer checks are also guarded against a missing local TD:
GetTdreturns nil when the TD is absent from the database (legacy XDPoS chaindata), whichtd.Cmp(nil)would turn into a panic. Both checks now skip the comparison and report the occurrence via a neweth/downloader/headers/missingtdcounter metric, warning once per downloader lifetime.Compatibility: no consensus, protocol, or database schema changes; this is a sync-logic fix only and needs no migration or operator coordination.
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