eth: retry sync after peer-set replacements - #2391
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
🟢 Approval recommended
The change is localized to witness fetching, preserves existing retry exhaustion behavior, and is covered by focused unit tests for the new empty-response path.
Pull request overview
This PR updates the eth/fetcher witness fetching flow to treat empty witness responses as “not ready yet” and to retry them with a dedicated backoff strategy, instead of penalizing (dropping) the responding peer. This fits into the block fetcher’s witness acquisition path used during syncing / block import.
Changes:
- Add per-request
emptyRetriestracking plus an exponential backoff helper for empty witness responses. - Route empty witness responses through a retry/reschedule path that does not drop peers.
- Add targeted tests for the new empty-response retry behavior and backoff function.
File summaries
| File | Description |
|---|---|
| eth/fetcher/witness_manager.go | Adds empty-response retry scheduling/backoff and avoids dropping peers on empty witness payloads. |
| eth/fetcher/witness_manager_test.go | Adds tests validating rescheduling, peer-not-dropped behavior, and backoff timing. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e54a683 to
d852ddc
Compare
|
@claude review |
There was a problem hiding this comment.
Looks good — this is a small, correct concurrency bookkeeping fix. What was reviewed: the revision counter is incremented under peerSet's existing write lock in both registerPeer and unregisterPeer, and read through a new RLock-guarded currentRevision(), so there's no new race; onPeerEvent/onSyncDone correctly swap the count-equality check for a revision-inequality check, which fixes the described peer-replacement-at-same-count blind spot; and the updated test explicitly reproduces that scenario (register a second peer, unregister the original, count stays at 1) and asserts the cooldown clears.
Extended reasoning...
Overview
The diff (single commit "eth: retry sync after peer-set replacements") touches only eth/peerset.go, eth/sync.go, and eth/sync_test.go. It adds a revision uint64 field to peerSet, bumped inside registerPeer/unregisterPeer under the existing ps.lock, plus a new currentRevision() accessor guarded by RLock. chainSyncer swaps its old peersUnavailableAtCount/len() count comparison for a peersUnavailableAtRevision/currentRevision() comparison, fixing a real logic gap: a peer replacement (disconnect + connect) leaves the peer count unchanged but should still clear the "peers unavailable" retry cooldown. The test was updated to explicitly unregister the original peer after registering a second one (keeping count at 1) and assert the cooldown clears, directly covering the previously-mishandled case.
Note: the "Pull request" title/description/timeline supplied in this task's context (about eth/fetcher witness-retry logic touching witness_manager.go) does not match this diff at all — that file isn't part of the change, and git log between the given commits shows exactly one commit, matching the peerset/sync revision fix described in "What changed." Per review policy, code is ground truth over description, so this review evaluates the actual diff; the mismatched metadata and prior-review warning appear to belong to unrelated PR content and don't raise any objection against this specific change.
Security risks
None identified. This is internal synchronization bookkeeping in the eth handler's sync-retry cooldown logic — no untrusted input parsing, no crypto, no consensus-path changes. The shared mutable field is protected by the pre-existing mutex consistent with security-common.md's requirement.
Level of scrutiny
Low-to-moderate is appropriate: the change is small, self-contained, and mechanical (swap a count comparison for a monotonic revision comparison), with correct locking and a test that exercises exactly the bug scenario described.
Other factors
No CODEOWNERS conflict apparent for these paths; the fix is well-scoped per the "small, focused PR" guidance in AGENTS.md, and the accompanying test change gives confidence the fix behaves as intended without needing to execute the test suite myself (sandbox denied running go test).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2391 +/- ##
===========================================
+ Coverage 55.57% 55.61% +0.04%
===========================================
Files 918 918
Lines 167133 167145 +12
===========================================
+ Hits 92877 92962 +85
+ Misses 68775 68714 -61
+ Partials 5481 5469 -12
... and 21 files with indirect coverage changes
🚀 New features to boost your workflow:
|
vbhattaccmu
changed the title
d41f64b
There was a problem hiding this comment.
🟡 Changes recommended
The new revision-based cooldown logic has a race where onSyncDone can snapshot a revision that includes an unprocessed topology change, preventing the corresponding peer event from clearing the cooldown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
eth/sync.go:165
onPeerEventcompares the peer-set revision againstpeersUnavailableAtRevision, but it never updatespeersUnavailableAtRevisionafter processing an event. IfonSyncDonestops snapshotting the revision (to avoid missing pending topology changes),onPeerEventshould record the latest revision seen by the sync loop so future events can be classified as topology changes vs. head announcements.
func (cs *chainSyncer) onPeerEvent() {
if !cs.peersUnavailableUntil.IsZero() && cs.handler.peers.currentRevision() != cs.peersUnavailableAtRevision {
cs.peersUnavailableUntil = time.Time{}
}
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The cooldown logic can be cleared spuriously because onSyncDone doesn’t snapshot the current peer-set revision when arming the cooldown, allowing a later non-topology peer event (e.g. block announce) to clear it.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The revision-based peer-topology tracking is correctly synchronized, integrates cleanly into the sync loop, and is backed by focused tests covering the previously missed peer-replacement case.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@claude review |
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, concurrency-safe (revision guarded by the existing peerset lock), and includes targeted tests covering the reported failure mode and ordering edge case.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
peerSet.unregisterPeer does not decrement witPeers, leaving internal peer counters inconsistent after disconnects.
Review details
Suppressed comments (1)
eth/peerset.go:294
peerSet.registerPeerincrementsps.witPeerswhen a peer has a WIT extension, butunregisterPeernever decrements it. This leaveswitPeerspermanently inflated after peers disconnect, making the internal counters inconsistent with the actual peer set (and potentially breaking any prioritization logic that relies on this count).
if peer.snapExt != nil {
ps.snapPeers--
}
return nil
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Good catch. This predates the PR: |
Summary
Addresses the sync-recovery blind spot observed in #2292. The peers-unavailable cooldown previously compared only the peer count, so replacing one peer with another at the same count did not trigger an early retry. This change tracks a peer-set revision, increments it on registration and removal, and clears the cooldown whenever the topology changes.
Executed tests
go test ./eth -run TestChainSyncerCooldownSurvivesBlockAnnounce -count=20go test ./ethgo test -race ./ethgo vet ./ethmake lintRollout notes
This change is not consensus-affecting and does not modify any protocol or wire format. It is backward-compatible, requires no coordinated upgrade, and has no operator-facing configuration changes.