refactor: extract BlockDownloadManager from PeerManagerImpl#34565
refactor: extract BlockDownloadManager from PeerManagerImpl#34565w0xlt wants to merge 4 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34565. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
Only glanced at the changes -- does this change make it easier to fuzz individual components? |
w0xlt
force-pushed
the
refactor/extract-block-download-manager
branch
from
6578aa8 to
a24d36e
Compare
Yes, this is one of the main benefits. Currently block download logic is embedded in After this PR, 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. |
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.
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
…Manager 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.
w0xlt
force-pushed
the
refactor/extract-block-download-manager
branch
from
a24d36e to
78c8904
Compare
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
|
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 It intentionally stays at the |
5 participants
Motivation
net_processing.cppis the largest file in the codebase (~6200 lines) andPeerManagerImplmixes 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 ofPeerManagerImpl.What this PR does
Extract all block download state and scheduling logic into a new
BlockDownloadManagermodule: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.cppshrinks from 6193 to 5751 lines (−442 net)CNodeStateloses 10 fields, retaining only compact block relay and chain sync timeout stateDesign
Follows the
TxDownloadManagerpimpl pattern:blockdownloadman.h— public interfaceblockdownloadman_impl.h— implementation class with per-peer stateblockdownloadman_impl.cpp— implementationUnlike
TxDownloadManager, no new mutex is introduced. Block download is inherently tied to chain state throughCBlockIndex*pointers, socs_mainremains 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
net_processing.cpprewired throughm_blockdownloadman, pure mechanical replacementBlockDownloadManagerstate transitions and invariants through the new module boundary