refactor: extract BlockDownloadManager from PeerManagerImpl #34565

w0xlt commented at 6:54 PM on February 11, 2026: contributor

Motivation

net_processing.cpp is the largest file in the codebase (~6200 lines) and PeerManagerImpl mixes several largely independent subsystems into a single class: transaction relay, address gossip, headers sync, compactblocks, and block download. This makes the file difficult to review, test in isolation, and reason about lock ordering.

#30110 successfully extracted transaction download logic into TxDownloadManager. This PR applies the same approach to block download, continuing the incremental decomposition of PeerManagerImpl.

What this PR does

Extract all block download state and scheduling logic into a new BlockDownloadManager module:

Global state moved (from PeerManagerImpl): mapBlocksInFlight, mapBlockSource, nSyncStarted, m_block_stalling_timeout, m_last_tip_update, m_num_preferred_download_peers, m_peers_downloading_from.

Per-peer state moved (from CNodeState): pindexBestKnownBlock, hashLastUnknownBlock, pindexLastCommonBlock, pindexBestHeaderSent, fSyncStarted, vBlocksInFlight, m_downloading_since, m_stalling_since, fPreferredDownload.

Methods moved (from PeerManagerImpl): FindNextBlocksToDownload, TryDownloadingHistoricalBlocks, ProcessBlockAvailability, UpdateBlockAvailability, BlockRequested, RemoveBlockRequest, IsBlockRequested, IsBlockRequestedFromOutbound, TipMayBeStale.

The result:

net_processing.cpp shrinks from 6193 to 5751 lines (−442 net)
CNodeState loses 10 fields, retaining only compact block relay and chain sync timeout state
Block download logic becomes unit-testable and fuzz-testable in isolation (8 unit test cases and 1 fuzz target added)

Design

Follows the TxDownloadManager pimpl pattern:

blockdownloadman.h — public interface
blockdownloadman_impl.h — implementation class with per-peer state
blockdownloadman_impl.cpp — implementation

Unlike TxDownloadManager, no new mutex is introduced. Block download is inherently tied to chain state through CBlockIndex* pointers, so cs_main remains the synchronizing lock. The extraction is for code organization and testability, not lock granularity.

A fuzz target is included to exercise the extracted manager directly, covering peer lifecycle, availability updates, in-flight request tracking, scheduling, stalling state, and counter consistency.

Commits

Add BlockDownloadManager class with pimpl skeleton: all new module files, no call sites changed
Add unit tests: exercises the module in isolation
Migrate call sites: ~150 call sites in net_processing.cpp rewired through m_blockdownloadman, pure mechanical replacement
Add fuzz target: exercises BlockDownloadManager state transitions and invariants through the new module boundary

DrahtBot added the label Refactoring on Feb 11, 2026

DrahtBot commented at 6:56 PM on February 11, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34565.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	sedited, 0xbrito

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35591 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
#35581 (node: add block template manager and track waitNext fee inflow by ismaelsadeeq)
#35561 (net: move some CNodeState fields to Peer by Crypt-iQ)
#35558 (p2p: Prefill compact blocks by davidgumberg)
#35522 (refactor: Extract per-message helpers from SendMessages() (move-only) by pablomartin4btc)
#35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
#35368 (tracing: add block header and compact block tracepoints by w0xlt)
#35315 (refactor: Use NodeClock::time_point in more places by maflcko)
#35229 (refactor: Use CBlockIndex parameters as reference by optout21)
#34743 (p2p: don't disconnect manual peers for block stalling by willcl-ark)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
#33854 (fix assumevalid is ignored during reindex by Eunovo)
#33637 (refactor: optimize block index comparisons (1.4-6.8x faster) by l0rinc)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sedited commented at 9:48 PM on February 11, 2026: contributor

Concept ACK

0xbrito commented at 6:49 AM on February 12, 2026: none

Concept ACK

Crypt-iQ commented at 4:50 PM on February 14, 2026: contributor

Only glanced at the changes -- does this change make it easier to fuzz individual components?

w0xlt force-pushed on Feb 17, 2026

w0xlt commented at 8:48 PM on February 17, 2026: contributor

Only glanced at the changes -- does this change make it easier to fuzz individual components?

Yes, this is one of the main benefits. Currently block download logic is embedded in PeerManagerImpl and CNodeState, which require standing up the full net_processing stack (connman, addrman, mempool, etc.) to test. There is no way to exercise block scheduling, request tracking, or stalling detection without that heavy setup.

After this PR, BlockDownloadManager can be constructed with just a ChainstateManager reference:

BlockDownloadManager bdm(BlockDownloadOptions{*m_node.chainman});
bdm.ConnectedPeer(peer1, info);
bdm.BlockRequested(peer1, *genesis);

The fuzz target itself isn't included here (keeping scope focused on the extraction), but the architecture now makes it straightforward to add as a follow-up.

DrahtBot added the label Needs rebase on Mar 11, 2026

refactor: add BlockDownloadManager class with pimpl pattern

Create a new BlockDownloadManager module following the established
TxDownloadManager pimpl pattern:

- blockdownloadman.h: public interface with QueuedBlock, options/info
  structs, and full BlockDownloadManager API
- blockdownloadman_impl.h: BlockDownloadManagerImpl with
  PeerBlockDownloadState and per-peer map
- blockdownloadman_impl.cpp: forwarding wrappers and implementations
  for block request tracking, availability processing, download
  scheduling, stalling detection, and tip staleness checks

The module manages all block download state: in-flight tracking
(mapBlocksInFlight), block source attribution (mapBlockSource),
per-peer download state, sync coordination, and scheduling algorithms
(FindNextBlocksToDownload, TryDownloadingHistoricalBlocks).

Key design decisions:
- cs_main provides synchronization (no separate mutex), since block
  download is inherently tied to chain state via CBlockIndex pointers
- Two-phase peer registration: ConnectedPeer handles both initial
  registration (conservative defaults) and re-registration (real
  connection properties from VERSION handshake)
- TipMayBeStale preserves lazy initialization of m_last_tip_update
- CompareExchangeBlockStallingTimeout preserves CAS atomicity for
  the stalling timeout accessed from multiple threads

No call sites are migrated yet; that happens in a subsequent commit.

6958f98094

test: add unit tests for BlockDownloadManager

Exercise BlockDownloadManager in isolation:

- peer_lifecycle: registration, sync state (including idempotency
  and explicit unsetting), blocks in flight, and disconnect cleanup
  for all counters (preferred, sync, downloading)
- block_source_tracking: set/get/erase block sources
- stalling_and_tip_staleness: last tip update get/set, and
  TipMayBeStale lazy initialization and staleness detection
- block_requested_basic: request/remove lifecycle, outbound
  detection, duplicate rejection, in-flight counts
- remove_block_request_from_specific_peer: peer-targeted removal
  leaves other peers' requests intact
- find_block_in_flight: verify BlockInFlightInfo fields across
  no-flight, single-peer, and multi-peer scenarios with and
  without partialBlock
- tip_not_stale_with_blocks_in_flight: blocks in flight suppress
  staleness even with an old tip update
- connected_peer_reregistration: two-phase ConnectedPeer upgrade
  from preferred=false to preferred=true without double-counting
- compare_exchange_stalling_timeout: CAS success/failure semantics
- find_next_blocks_to_download: download scheduling against
  headers-only blocks - height ordering, count cap, in-flight
  skipping, last-common-block update, and the accept/refuse paths
  of TryDownloadingHistoricalBlocks

bd59d03008

refactor: migrate block download state and methods into BlockDownloadManager

Move all block download state and logic from PeerManagerImpl and
CNodeState into BlockDownloadManager, completing the extraction.

Migrated from PeerManagerImpl:
- mapBlocksInFlight, mapBlockSource, nSyncStarted
- m_block_stalling_timeout, m_last_tip_update
- m_num_preferred_download_peers, m_peers_downloading_from
- IsBlockRequested, IsBlockRequestedFromOutbound
- RemoveBlockRequest, BlockRequested, TipMayBeStale
- ProcessBlockAvailability, UpdateBlockAvailability
- FindNextBlocksToDownload, TryDownloadingHistoricalBlocks

Migrated from CNodeState:
- pindexBestKnownBlock, hashLastUnknownBlock
- pindexLastCommonBlock, pindexBestHeaderSent
- fSyncStarted, vBlocksInFlight, m_downloading_since
- m_stalling_since, fPreferredDownload

~150 call sites updated to go through m_blockdownloadman.

MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK is removed from net_processing.h;
its definition moved to blockdownloadman.h with the module.

In EvictExtraOutboundPeers, the manager is only queried after the
CNodeState null check: GetBlocksInFlight() asserts that the peer is
registered, so querying it first would turn the previously tolerated
unknown-peer case into an abort. Keeping the query behind the null
check preserves the existing short-circuit behavior.

78c890442e

w0xlt force-pushed on Jun 11, 2026

DrahtBot removed the label Needs rebase on Jun 11, 2026

fuzz: add BlockDownloadManager target

Exercise BlockDownloadManager through arbitrary sequences of its public
API, against a fixture chainstate consisting of 20 mined blocks, a
headers-only extension of the tip, and a headers-only fork branching
from below the tip. This covers the block download scheduler
(FindNextBlocksToDownload, TryDownloadingHistoricalBlocks) across fork
topologies, the in-flight request bookkeeping whose multimap entries
hold iterators into per-peer lists, availability tracking with both
known and unknown hashes, and the stalling/staleness logic.

The harness keeps a small per-peer mirror to respect the documented
preconditions (registration before per-peer calls, the
MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK call-site guard) and checks
invariants throughout:

- global counters (peers downloading from, preferred peers, sync
  started, total blocks in flight) always agree with per-peer state
- scheduled blocks are capped by count, height-ascending, never
  already stored and never already in flight
- a stale tip implies nothing is in flight
- FindBlockInFlight results are internally consistent
- stalling timeout compare-exchange follows CAS semantics
- after disconnecting all peers, all data structures are empty

d867ea88fe

w0xlt commented at 12:48 AM on June 14, 2026: contributor

I initially left the fuzz target out to keep this PR focused on the extraction and unit coverage, but added it now since it seems to provide useful coverage without expanding the production-code scope.

The target exercises BlockDownloadManager directly through the new module boundary: peer registration/disconnection, block availability, in-flight request tracking/removal, scheduling, stalling/stale-tip state, block source tracking, and global counter consistency. This is the kind of state-machine behavior that was hard to fuzz while it lived inside PeerManagerImpl.

It intentionally stays at the BlockDownloadManager API level and respects the same preconditions as the net_processing call sites. I think this makes it a useful follow-up commit for validating the extraction while keeping the main refactor unchanged.