netsync: change startSync to be based off of processed headers
What changed, and why it matters
This commit refactors how the Bitcoin node decides which peer to sync blocks from. It switches the sync logic to rely on the new 'processed headers' state rather than the older internal header list. The change removes direct handling of SegWit peer filtering, checkpoint-based headers-first mode, and some equality fallback logic, delegating those concerns to helper functions like fetchHigherPeers, fetchHeaders, and fetchHeaderBlocks. There is no explicit security fix described in the commit, but any refactor of consensus-critical sync code carries a risk of subtle bugs that could affect which blocks a node accepts.
Review the implementations of the newly introduced helper methods (isInIBDMode, fetchHigherPeers, fetchHeaders, fetchHeaderBlocks) and the ProcessBlockHeaders integration to confirm that SegWit peer requirements, checkpoint handling, and stall detection remain correct. Run sync tests against mainnet/testnet and regression test networks, and monitor for any changes in accepted chains or stall behavior.
Security signals we found
Refactor of P2P block/header sync peer selection
Removal of explicit SegWit-enabled peer filtering from startSync
Removal of checkpoint-based headers-first mode branching from startSync
Delegation to new helper methods whose implementations are not shown in the diff
Change of sync basis from best block snapshot to best processed header height
Evidence from the diff
In netsync/manager.go, startSync was rewritten. Previously it queried segwit activation, filtered peers by IsWitnessEnabled, compared peer LastBlock() against sm.chain.BestSnapshot().Height, built higher/equal peer sets, decided headers-first vs block-first mode using sm.nextCheckpoint, and pushed getheaders/getblocks messages. The new version checks sm.isInIBDMode(), uses sm.chain.BestHeader() height, calls sm.fetchHigherPeers for both header and block peer selection, calls sm.fetchHeaders(), and finally calls sm.fetchHeaderBlocks. Segwit-specific peer filtering and checkpoint headers-first branching are no longer visible in this function. The diff alone does not show the implementations of the helper methods or the new ProcessBlockHeaders behavior, so the full security effect cannot be determined from this single commit.
Changed components
btcd/netsync/manager.goSyncManager.startSyncinitial block download (IBD) logicpeer selection for header/block downloadInspect captured patch +35 / −102
diff --git a/netsync/manager.go b/netsync/manager.go
index 47e60e9..3954376 100644
--- a/netsync/manager.go
+++ b/netsync/manager.go
@@ -322,60 +322,35 @@ func (sm *SyncManager) startSync() {
return
}
- // Once the segwit soft-fork package has activated, we only
- // want to sync from peers which are witness enabled to ensure
- // that we fully validate all blockchain data.
- segwitActive, err := sm.chain.IsDeploymentActive(chaincfg.DeploymentSegwit)
- if err != nil {
- log.Errorf("Unable to query for segwit soft-fork state: %v", err)
+ // Check to see if we're in the initial block download mode.
+ if !sm.isInIBDMode() {
return
}
- best := sm.chain.BestSnapshot()
- var higherPeers, equalPeers []*peerpkg.Peer
- for peer, state := range sm.peerStates {
- if !state.syncCandidate {
- continue
- }
-
- if segwitActive && !peer.IsWitnessEnabled() {
- log.Debugf("peer %v not witness enabled, skipping", peer)
- continue
- }
-
- // Remove sync candidate peers that are no longer candidates due
- // to passing their latest known block. NOTE: The < is
- // intentional as opposed to <=. While technically the peer
- // doesn't have a later block when it's equal, it will likely
- // have one soon so it is a reasonable choice. It also allows
- // the case where both are at 0 such as during regression test.
- if peer.LastBlock() < best.Height {
- state.syncCandidate = false
- continue
- }
-
- // If the peer is at the same height as us, we'll add it a set
- // of backup peers in case we do not find one with a higher
- // height. If we are synced up with all of our peers, all of
- // them will be in this set.
- if peer.LastBlock() == best.Height {
- equalPeers = append(equalPeers, peer)
- continue
+ // If we're in the initial block download mode, check if we have
+ // peers that we can download headers from.
+ _, bestHeaderHeight := sm.chain.BestHeader()
+ higherHeaderPeers := sm.fetchHigherPeers(bestHeaderHeight)
+ if len(higherHeaderPeers) != 0 {
+ sm.fetchHeaders()
+
+ // Reset the last progress time now that we have a
+ // non-nil syncPeer to avoid the stall handler firing
+ // before any headers have been received.
+ if sm.syncPeer != nil {
+ sm.lastProgressTime = time.Now()
}
-
- // This peer has a height greater than our own, we'll consider
- // it in the set of better peers from which we'll randomly
- // select.
- higherPeers = append(higherPeers, peer)
- }
-
- if sm.chain.IsCurrent() && len(higherPeers) == 0 {
- log.Infof("Caught up to block %s(%d)", best.Hash.String(), best.Height)
return
}
- // Pick randomly from the set of peers greater than our block height,
- // falling back to a random peer of the same height if none are greater.
+ // We don't have any more headers to download at this
+ // point so start asking for blocks.
+ best := sm.chain.BestSnapshot()
+ higherPeers := sm.fetchHigherPeers(best.Height)
+
+ // Pick randomly from the set of peers greater than our
+ // block height, falling back to a random peer of the same
+ // height if none are greater.
//
// TODO(conner): Use a better algorithm to ranking peers based on
// observed metrics and/or sync in parallel.
@@ -383,66 +358,24 @@ func (sm *SyncManager) startSync() {
switch {
case len(higherPeers) > 0:
bestPeer = higherPeers[rand.Intn(len(higherPeers))]
-
- case len(equalPeers) > 0:
- bestPeer = equalPeers[rand.Intn(len(equalPeers))]
}
- // Start syncing from the best peer if one was selected.
- if bestPeer != nil {
- // Clear the requestedBlocks if the sync peer changes, otherwise
- // we may ignore blocks we need that the last sync peer failed
- // to send.
- sm.requestedBlocks = make(map[chainhash.Hash]struct{})
+ if bestPeer == nil {
+ log.Warnf("No sync peer candidates available")
+ return
+ }
- locator, err := sm.chain.LatestBlockLocator()
- if err != nil {
- log.Errorf("Failed to get block locator for the "+
- "latest block: %v", err)
- return
- }
+ sm.syncPeer = bestPeer
+ sm.ibdMode = true
- log.Infof("Syncing to block height %d from peer %v",
- bestPeer.LastBlock(), bestPeer.Addr())
-
- // When the current height is less than a known checkpoint we
- // can use block headers to learn about which blocks comprise
- // the chain up to the checkpoint and perform less validation
- // for them. This is possible since each header contains the
- // hash of the previous header and a merkle root. Therefore if
- // we validate all of the received headers link together
- // properly and the checkpoint hashes match, we can be sure the
- // hashes for the blocks in between are accurate. Further, once
- // the full blocks are downloaded, the merkle root is computed
- // and compared against the value in the header which proves the
- // full block hasn't been tampered with.
- //
- // Once we have passed the final checkpoint, or checkpoints are
- // disabled, use standard inv messages learn about the blocks
- // and fully validate them. Finally, regression test mode does
- // not support the headers-first approach so do normal block
- // downloads when in regression test mode.
- if sm.nextCheckpoint != nil &&
- best.Height < sm.nextCheckpoint.Height &&
- sm.chainParams != &chaincfg.RegressionNetParams {
-
- bestPeer.PushGetHeadersMsg(locator, sm.nextCheckpoint.Hash)
- sm.headersFirstMode = true
- log.Infof("Downloading headers for blocks %d to "+
- "%d from peer %s", best.Height+1,
- sm.nextCheckpoint.Height, bestPeer.Addr())
- } else {
- bestPeer.PushGetBlocksMsg(locator, &zeroHash)
- }
- sm.syncPeer = bestPeer
+ // Reset the last progress time now that we have a non-nil
+ // syncPeer to avoid instantly detecting it as stalled in the
+ // event the progress time hasn't been updated recently.
+ sm.lastProgressTime = time.Now()
- // Reset the last progress time now that we have a non-nil
- // syncPeer to avoid instantly detecting it as stalled in the
- // event the progress time hasn't been updated recently.
- sm.lastProgressTime = time.Now()
- } else {
- log.Warnf("No sync peer candidates available")
- }
+ log.Infof("Syncing to block height %d from peer %v",
+ sm.syncPeer.LastBlock(), sm.syncPeer.Addr())
+ sm.fetchHeaderBlocks(sm.syncPeer)
}
// isSyncCandidate returns whether or not the peer is a candidate to consider
Why this scored 31/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.