discovery: rotate failed historical syncers

[Roasbeef]

In this PR, we rotate away from an initial historical sync peer when its
reply_channel_range exceeds the local SCID limit or violates the requested
range, continuity, or encoding constraints. The peer remains connected and
returns to its normal synced state while another eligible peer takes over the
historical sync.

The sync manager records failed peer keys in a short-lived exclusion set. The
set applies to immediate replacements, first-peer reconnect startup, and the
next scheduled historical sync pick. It is then cleared, so the peer can be
retried later without turning the quick fix into a ban.

Each failure event also carries the originating syncer instance and a promise.
The manager detaches the failed attempt from its completion signal before it
resolves the promise, then selects a replacement. A delayed report from an old
connection therefore cannot detach a new same-key syncer, and a slow replacement
candidate cannot hold the failed syncer in syncerIdle.

The regression tests cover the public message path for both the SCID limit and
an invalid range, automatic replacement without pruning the peer, bounded
exclusion across a same-key reconnect, stale failure reports, and promise
acknowledgement before replacement selection.

Fixes #11172

[Roasbeef]

Potential concern her if it happens to be a channel peer. This is a small targeted fix for the issue, I have a better paginated version coming that handles this case more natively.

[Lrifton92]

Reviewed at 297e745.

The failover itself is correct: the buffer is released before the disconnect (discovery/syncer.go:960-965, pre-existing), the sentinel is wrapped in exactly one place so errors.Is cannot catch local graph errors (discovery/syncer.go:1061 vs 1161), and the boundary is covered on both sides by the existing test plus the new one (discovery/syncer_test.go:2677-2693, 2735-2745).

Two things I would like to see settled before this merges.

Wiring disconnectPeer straight to peer.Disconnect (discovery/sync_manager.go:676) bypasses the policy this package already enforces for misbehaving gossip peers: handleBadPeer routes through ShouldDisconnect, whose comment is // We should only disconnect non-channel peers (discovery/gossiper.go:3929-3932, 4049-4064). This is the channel-peer concern you raised in your own comment — would routing the limit failure through handleBadPeer, or at least consulting ShouldDisconnect, be preferable to a direct teardown? The catch is that exempting channel peers reinstates the stall for exactly those peers, which may argue for an in-band rotation (drop the syncer back to chansSynced and call forceHistoricalSync) rather than a transport-level disconnect.

Scoping the recovery to errChanRangeReplyTooLarge leaves the other five remote-triggered errors from bufferChanRangeReply on the old path (discovery/syncer.go:983, 1002, 1013, 1031, 1051), all of which hit the same bare return in channelGraphSyncer (discovery/syncer.go:619-622) and reproduce the #11172 stall: the syncer goroutine exits, no staleSyncer is emitted, and the node stays at synced_to_graph:false until the hourly tick. A peer selected as the initial historical syncer can trigger that with a single reply_channel_range whose FirstBlockHeight is one below the query, without ever approaching 100k SCIDs, and nothing increments its ban score on this path (discovery/gossiper.go:4051 is the only call site and it is on the announcement path). Since the PR is marked Fixes #11172, is the intent to close only the oversized-reply door here and handle protocol-validation failures in the paginated version you mentioned?

Two smaller notes. The failing peer is not recorded anywhere, so after a persistent peer reconnects (server.go:5041) its fresh syncer is created in chansSynced (discovery/sync_manager.go:689) and is eligible for forceHistoricalSync again; the reconnect backoff bounds the churn, but the issue's request to exclude the failing peer from the next pick is not implemented. And TestSyncManagerReplacesOversizedHistoricalSyncer calls PruneSyncState by hand (discovery/sync_manager_test.go:233), which is the one link the change depends on and the one the mock does not exercise — in production that call comes from server.peerTerminationWatcher (server.go:4881), so the test proves the pre-existing staleSyncer path rather than Disconnect reaching it.

[Roasbeef]

Addressed in the rewritten branch at c4c4d09.

• The direct peer.Disconnect callback is gone. An unusable peer response now
  moves that syncer to syncerIdle, synchronously asks SyncManager to select
  another historical syncer, and returns the original peer to chansSynced
  without tearing down the connection.
• The range-before-query, range-after-query, discontinuous-range, and unknown
  encoding errors now wrap errInvalidChanRangeReply and take the same recovery
  path as the aggregate SCID limit. The missing-active-query guard remains
  terminal because it is unreachable through ProcessQueryMsg: replies are
  accepted only in waitingQueryRangeReply, which is entered after storing the
  query. Local FilterKnownChanIDs and transport errors also remain terminal.
• The failed syncer is excluded from the immediate replacement choice by its
  idle state. There is no persistent peer blacklist; a later, fresh syncer can
  be considered normally. Failure events carry the originating syncer pointer,
  so a delayed event from an old connection cannot match a new syncer with the
  same public key.
• TestSyncManagerReplacesOversizedHistoricalSyncer now observes automatic
  replacement through the public message path. It no longer calls
  PruneSyncState, and it asserts that the original peer stays connected and
  registered. The syncer-level table also covers an invalid range response.

Local verification is clean: go test -race -mod=mod ./discovery -count=1
passes, the focused recovery tests pass for 10 race-enabled runs, and
make lint reports zero issues.

[github-actions]

🟠 PR Severity: HIGH

gh pr view | 5 files | 422 lines changed

🟠 High (2 files)

• discovery/sync_manager.go - gossip syncer manager: historical syncer selection/replacement logic
• discovery/syncer.go - gossip protocol channel-range sync state machine

🟢 Low (3 files)

• discovery/sync_manager_test.go - test-only change
• discovery/syncer_test.go - test-only change
• docs/release-notes/release-notes-0.22.0.md - release notes

Analysis

The highest-severity files touched are discovery/sync_manager.go and discovery/syncer.go, both under discovery/* (gossip protocol), which maps to HIGH. The change reworks how the SyncManager handles an unusable historical syncer: instead of disconnecting the peer, it now moves the syncer to idle, synchronously selects a replacement historical syncer, and routes several previously-terminal error paths (range-before/after-query, discontinuous range, unknown encoding) through the same recovery path as the aggregate SCID limit. Non-test, non-doc changes total ~179 lines across 2 files, well under the thresholds for a severity bump (>20 files or >500 lines), and no critical packages are touched, so no escalation applies.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[Lrifton92]

Reviewed at 7181543.

The three points from my last pass are addressed. The direct peer.Disconnect is gone, so the ShouldDisconnect policy question no longer applies. The range-before-query, range-after-query, discontinuity and encoding errors are now wrapped in errInvalidChanRangeReply (discovery/syncer.go:1017, :1032, :1053, :1076), so they take the recovery path instead of the bare return that reproduced #11172. And TestSyncManagerReplacesOversizedHistoricalSyncer now drives the failure through ProcessQueryMsg and asserts the peer stays connected and registered, rather than calling PruneSyncState by hand.

Leaving curQueryRangeMsg == nil on the terminal path reads as deliberate, and I think it is right: that is a local invariant violation, not something the remote can steer.

The ordering of the handoff is sound. The syncer moves to syncerIdle before the callback (discovery/syncer.go:575) and only returns to chansSynced after doneChan closes (:580), so chooseRandomSyncer's chansSynced filter cannot re-select the failing syncer during its own replacement, and handleHistoricalSync restores genHistoricalChanRangeQuery (discovery/syncer.go:2031) for whichever syncer is chosen.

Two things I would still raise.

The failing peer is only excluded for the duration of its own replacement. Once handleChanRangeError returns it to chansSynced, it is a full candidate again on the HistoricalSyncTicker path (discovery/sync_manager.go:571, :584), which is the same unfiltered chooseRandomSyncer over m.gossipSyncers(). Nothing on this path touches the ban score; handleBadPeer remains reachable only from the announcement path. A peer that deterministically answers the historical query with an out-of-range reply_channel_range therefore costs one rotation per tick, indefinitely, and with a small peer set the map iteration can hand it the initial historical sync again on the next tick. The previous behaviour was harsh but terminating; this one has no terminating condition. #11172 asks to exclude the failing peer from the next pick, and I do not see that part implemented. Is the intent to leave it to the paginated version, or would a short-lived exclusion set (or a ban-score bump on errInvalidChanRangeReply) be in scope here?

The new path lets a remote peer stall syncerHandler. reportHistoricalSyncFailure (discovery/sync_manager.go:752) blocks the syncer goroutine until doneChan closes, and the handler closes it only after forceHistoricalSync returns (:504). forceHistoricalSync calls s.historicalSync() per candidate, a blocking round-trip bounded by syncTransitionTimeout (5s), and chooseRandomSyncer continues to the next candidate on error. So a set of syncers that are in chansSynced but whose goroutines are not currently reading historicalSyncReqs (blocked in sendMessages to a slow peer, for instance) costs up to 5s each, with the whole syncerHandler select stalled meanwhile: no newSyncers, no staleSyncers, no ticker service. That blocking call already existed on the ticker path, so the code is not doing anything new here; what is new is that a remote peer can now trigger it on demand with a single malformed reply. Would it be worth running the replacement selection off the handler goroutine, or at least not making the failing syncer wait on its result?

[Roasbeef]

The failing peer is only excluded for the duration of its own replacement. Once handleChanRangeError returns it to chansSynced, it is a full candidate again on the HistoricalSyncTicker path (discovery/sync_manager.go:571, :584), which is the same unfiltered chooseRandomSyncer over m.gossipSyncers(). Nothing on this path touches the ban score; handleBadPeer remains reachable only from the announcement path.

Good point, but generally I think we want to be careful before we go to ban peers. For example, if it's a channel peer, then we'd cut off all availability of the channel. This is meant as a quick fix to resolve this issue. I have another PR that moves to a proper page based mechanism, so it doesn't expect to sync the entire graph in one swoop any longer.

[Roasbeef]

The new path lets a remote peer stall syncerHandler. reportHistoricalSyncFailure (discovery/sync_manager.go:752) blocks the syncer goroutine until doneChan closes, and the handler closes it only after forceHistoricalSync returns (:504

Easy to make that async, will reach for a promise here.

[Roasbeef]

Addressed at 5119973.

For the repeated selection point, the manager now keeps a short-lived set of
failed peer keys. It excludes those peers from immediate replacements, an
ordinary first-peer reconnect, and the next scheduled historical sync pick.
The set is cleared after that scheduled attempt. This gives each failed peer a
bounded retry delay without disconnecting or banning a channel peer.

For the blocking handoff, the failure event now carries a promise. The manager
first detaches the failed syncer's tracked completion signal, then resolves the
promise before calling forceHistoricalSync. The syncer waits only for that
acknowledgement, so candidate handoff time no longer delays its return to
chansSynced.

I left replacement selection on syncerHandler; moving it out would require a
result and generation lifecycle that is larger than this quick fix. The existing
bounded handler stall can still occur on the ticker path, but a malformed reply
no longer holds the failing syncer behind that work.

The focused recovery tests pass for 10 race-enabled runs, the full discovery
race suite passes, and make lint reports zero issues. The new tests cover the
next-tick exclusion, acknowledgement before a blocked replacement lock, and
bounded exclusion across reconnect.

[Lrifton92]

Reviewed at 5119973.

All three points from the last pass are addressed. I checked the parts that could go wrong rather than the parts that were claimed.

The exclusion set cannot feed a syncer its own replacement. The failing peer is inserted at discovery/sync_manager.go:511, before the acknowledgement at :524 and before forceHistoricalSync at :530, so the syncer blocked on that acknowledgement is already excluded when the replacement is picked. It is consumed and cleared on the ticker path (:606, :611), and TestSyncManagerRetainsFailureAcrossReconnect pins that with two forced ticks — the first consumes the exclusion, the second retries the peer. That is the property that makes this a retry delay rather than a ban, so it is worth having it in a test rather than only in a comment.

The new delete is safe. forceHistoricalSync mutates the map it received (:932), but gossipSyncers() (:1056-1067) allocates a fresh map on every call, so activeSyncers and inactiveSyncers are untouched.

A rejected reply cannot mark the graph synced. handleChanRangeError ends in chansSynced (discovery/syncer.go:580) without calling markGraphSynced, which is still reached only from the two completion paths (:683, :1205). synced_to_graph stays false across a rotation.

Two things, neither blocking.

The promise removes the second wait, not the first. reportHistoricalSyncFailure still blocks on the unbuffered send at discovery/sync_manager.go:787 until syncerHandler is back in its select. If the handler is inside forceHistoricalSync at that moment — the ticker path at :606, or an earlier failure at :530 — the failing syncer waits there, bounded by syncTransitionTimeout per candidate tried (discovery/syncer.go:2011). It is not a deadlock: by then the peer is already in the exclusion set, and a second peer failing concurrently is not reading historicalSyncReqs, so the handler times out on it rather than blocking. So "candidate handoff time no longer delays its return to chansSynced" is true for a syncer's own replacement, but not for one already in flight. Worth the precision in the release note.

Nit. _, failedHistoricalSync := ... at discovery/sync_manager.go:440 shadows the package type of the same name declared at :89. It compiles because the type is not needed in that block, but it binds a type name to a bool for the rest of the scope.

LGTM.