refactor: extract BlockDownloadManager from PeerManagerImpl#34565
Open
w0xlt wants to merge 3 commits intobitcoin:masterfrom
Open
refactor: extract BlockDownloadManager from PeerManagerImpl#34565w0xlt wants to merge 3 commits intobitcoin:masterfrom
w0xlt wants to merge 3 commits intobitcoin:masterfrom
Conversation
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
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
Possible places where named args for integral literals may be used (e.g.
2026-02-17 19:29:10 |
Contributor
|
Concept ACK |
1 similar comment
|
Concept ACK |
This was referenced Feb 12, 2026
Contributor
|
Only glanced at the changes -- does this change make it easier to fuzz individual components? |
…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.
6578aa8 to
a24d36e
Compare
Contributor
Author
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. |
Contributor
|
🐙 This pull request conflicts with the target branch and needs rebase. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Commits
net_processing.cpprewired throughm_blockdownloadman, pure mechanical replacement