netsync: recover after losing sync peer near tip - #2607
Conversation
In this commit, we clear ibdMode when updateSyncPeer removes the active sync peer. Near the chain tip, startSync can find no strictly higher replacement and return with syncPeer nil. Leaving IBD enabled in that state causes handleInvMsg to discard every new block announcement, so the node cannot recover until a new higher peer connects. The regression test covers the near-tip disconnect and verifies that a remaining peer announcement is requested after the sync state is reset.
Coverage Report for CI Build 34637539728Coverage increased (+0.1%) to 54.16%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions771 previously-covered lines in 8 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
cc: @kcalvinalvin for review |
|
Two property-based tests added on top of the regression test (rapid is already a go.mod dependency, used in TestPropertyLostSyncPeerRecovers generalizes the near-tip regression across arbitrary chain heights and remaining-peer height combinations. For any generated scenario (chain height 1–31, 1–4 remaining peers at tip or tip+1), losing the sync peer with IBD enabled must end in one of exactly two live states: a strictly higher peer was promoted and the header-first path recovers (verified end-to-end: header delivered, block requested), or the sync state was fully reset and a new-block announcement from any remaining peer is requested. The dead state TestPropertySyncStateTransition is a stateful property over the whole sync state machine: a generated schedule of events (peer connects at arbitrary heights, header/block delivery from the sync peer, stall detection, sync-peer loss) is replayed against the manager with the same invariant asserted after every event. One supporting change was needed and is worth flagging for reviewers: With the fix reverted, the properties fail with exactly the dead-state message; with the fix, 100 generated cases pass per property ( On issue coverage: the issue describes a node 1–2 blocks behind tip whose sync peer departs while no other peer is strictly higher — that exact scenario is now generated by both properties. The one behavioral gap the issue mentions that this PR does not address is the "at minimum it should log that it is stuck" ask — the fix removes the stuck state rather than logging it, which resolves the incident class outright. |
3d7b5a0 to
1ecc502
Compare
In this commit, we add two rapid-based property tests that generalize the regression test for the lost-sync-peer stall. TestPropertyLostSyncPeerRecovers covers the near-tip disconnect across arbitrary chain heights and remaining-peer height combinations: for any generated scenario, losing the sync peer with IBD enabled must either promote a strictly higher replacement peer (re-entering IBD through the header-first path, which the test drives to completion) or fully reset the sync state so that a new-block announcement from any remaining peer is requested. TestPropertySyncStateTransition is a stateful property over the whole header-first sync state machine. A generated schedule of events (new peers at arbitrary heights, header and block delivery from the sync peer, stalls, and peer loss) is replayed against the manager, asserting after every event that the dead state (nil sync peer with IBD enabled) in which handleInvMsg discards announcements never holds. generateTestBlocks now anchors timestamps to the current time rather than the 2011 regtest genesis timestamp. A chain built from the genesis timestamp is never IsCurrent, which keeps the near-tip paths where the stall occurs unreachable; starting from now keeps them reachable at any generated height, which is what lets the properties reproduce the bug. Both properties fail on the parent commit and pass with the fix. Tests: go test ./netsync/ ./blockchain/ ./peer/ ./mempool/; go test ./...
1ecc502 to
8ad3174
Compare
saubyk
left a comment
saubyk
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on #2606. I had the change checked out and exercised locally: build, vet, and the netsync tests (including the new ones, with -race) all pass on this head.
From a product/risk angle the fix does what the issue asks, but it also opens a couple of side doors that I'd like a second look at before this ships. Details are inline; two more that don't sit on changed lines:
handleInvMsg(~line 1165) still trusts theibdModeflag alone. This PR fixes the flag at one write site. If any future code path clears the sync peer without clearing the flag, we're back to #2606 with no test catching it.handleHeadersMsgalready double-checkssyncPeer != nillocally. Is there a reason not to do the same here? It should be behavior-neutral today.- Full UTXO cache flush when the sync peer is lost mid-IBD (~line 834). With
ibdModenow cleared even when the node is far from the tip, the first block from a remaining peer can hit the periodic-flush branch and write the whole cache to disk, which IBD then refills. One-time cost, not a correctness issue, but it's a new user-visible slowdown that didn't exist before. Gating that flush on chain currency instead of the flag would avoid it.
Small one: the doc comment on updateSyncPeer still mentions resetting header prefetch state, which was removed a while back, and doesn't mention that the function now leaves IBD mode. Worth folding the inline note into the doc comment.
| // IBD requires an active sync peer. Clear it with the peer so a lost | ||
| // sync peer cannot leave the manager in a state where announcements are | ||
| // ignored indefinitely. | ||
| sm.ibdMode = false |
There was a problem hiding this comment.
This clears IBD mode unconditionally, including when the node is far from the tip and no higher-height peer is available to take over. In that window syncPeer is nil and ibdMode is false, so a headers message from any peer now falls through to fetchHeaderBlocks (~line 1014) with no check that the sender is the sync peer or even a sync candidate.
As I understand it, that can reserve up to 50k block hashes in requestedBlocks from a peer that never serves them, and nothing clears those entries while there's no sync peer. A later honest sync peer then skips every reserved hash and stalls out. Pre-PR the ibdMode guard in handleHeadersMsg blocked this. Can you confirm whether that's reachable, and if so gate the fall-through on the peer being the sync peer or a sync candidate?
| blockHash := block2.Hash() | ||
| inv := wire.NewMsgInvSizeHint(1) | ||
| err = inv.AddInvVect(wire.NewInvVect( | ||
| wire.InvTypeWitnessBlock, blockHash, |
There was a problem hiding this comment.
The announcement here uses InvTypeWitnessBlock, but real peers announce with InvTypeBlock. When I swapped this to InvTypeBlock the requestedBlocks assertion fails even with the fix present, because the mock peer from newSyncCandidate isn't witness-enabled and handleInvMsg drops the inv. So this test currently proves recovery through the direct ibdMode field check, not through the announcement path users actually hit.
The two property tests below (lines 1366 and 1594) announce the same way. Could the test peer get the witness service flag so the tests can use InvTypeBlock? Ideally the announced block also goes through handleBlockMsg so we see it accepted end to end.
| "ibdMode should not be activated when chain is already current") | ||
| } | ||
|
|
||
| // TestLostSyncPeerNearTip verifies that losing the sync peer near the tip |
There was a problem hiding this comment.
Coverage gap worth flagging against the PR description ("remaining peers announce and request blocks normally"): all three new tests start with headers and blocks at the same height. The state from the issue report is headers ahead of blocks. In that state, if the sync peer is lost and no higher peer exists, the already-validated headers are never re-requested and recovery waits for the next block to be mined. That's pre-existing rather than introduced here, but the description reads as if it's covered. Could we either add a case that processes a header ahead of the tip before the disconnect, or narrow the description to what's actually fixed?
3 participants
In this PR, we clear ibdMode when updateSyncPeer removes the active sync peer. Near the chain tip, startSync can find no strictly higher replacement and return with syncPeer nil. Leaving IBD enabled in that state causes handleInvMsg to discard every new block announcement, so the node remains stalled until a newly connected peer advertises a higher height.
Resetting both pieces of sync state together lets the remaining peers announce and request blocks normally, while a replacement peer still re-enters IBD through startSync. The regression test covers the near-tip disconnect and verifies that the next block announcement is requested.
Fixes #2606
Tests: go test ./...
The repository-wide make lint target still reports 13 existing findings outside this diff, including pre-existing unchecked calls in netsync.