refactor: Extract per-message helpers from SendMessages() (move-only) #35522

pull pablomartin4btc wants to merge 17 commits into bitcoin:master from pablomartin4btc:refactor/extract-sendmessages-helpers changing 1 files +664 −427

pablomartin4btc commented at 11:48 PM on June 12, 2026: member

PeerManagerImpl::SendMessages() is a ~500-line function handling every p2p conditional send message type inline, which makes it hard to navigate and review. This was reduced to less than 90 lines.

I've identified this issue/ possible improvement while reviewing #34824.

This follows the pattern of similar PRs (#35502, and some commits fa5ab02 - from #35148 -, and fa55723 from #34059 ) and there was also a previous attempt in #9579 mentioned in some related PR's refactoring comment.

-<ins>Notes</ins>:

Each of the 17 commits represents a helper function extraction so it can be easily reviewed, it's only a pure code move, the only new lines are the declaration (with thread-safety annotations), the function signature, and the one-line call site, and they can be reviewed with the git options: --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space.

move-only: Extract ScheduleTxRelayTrickle() helper

Extracts the periodic relay scheduling logic from the tx-relay section
of SendMessages() into a self-contained helper. The helper determines
whether it is time to trickle inventory to a peer and clears the
pending inventory set if the peer has requested no transaction relay.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

a87a8e6b1f

move-only: Extract MaybeSendMempoolResponse() helper

Extracts the BIP35 mempool-response block from the tx-relay section of
SendMessages() into a self-contained helper. The m_send_mempool guard
moves inside as an early return, keeping the call site a single line.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

681f19a00a

move-only: Extract MaybeSendTxInventory() helper

Extracts the regular tx-inventory relay selection block from the
tx-relay section of SendMessages() into a self-contained helper.
The helper covers candidate collection, heap-based fee/topology
sorting, bloom/feerate filtering, and the m_last_inv_sequence update.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

e6d1d2bf18

move-only: Extract MaybeSendInitialGetheaders() helper

Extracts the initial headers-sync kick-off block from SendMessages()
into a self-contained helper. The helper handles the fSyncStarted
guard, the single-peer-or-close-to-tip condition, the getheaders
locator construction, and the timeout initialisation.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

d93acbabb6

move-only: Extract MaybeSendBlockAnnouncements() helper

Extracts the block-announcement section from SendMessages() into a
self-contained helper. The helper covers header-relay candidate
selection, compact-block sending (with cached-message optimisation),
multi-header relay, and the inv fallback path.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

8642e35a6c

move-only: Extract MaybeSendBlockInv() helper

Extracts the block-inv drain loop from SendMessages() into a
self-contained helper. The helper flushes m_blocks_for_inv_relay
into the caller-provided vInv vector so downstream tx-relay
logic can continue appending to the same batch.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

677e97e63a

move-only: Extract CheckBlockSyncTimeouts() helper

Extracts the three timeout/stall checks from SendMessages() into a
self-contained helper: block-download stall detection (with adaptive
timeout backoff), per-block in-flight timeout, and headers-sync
timeout. Returns true when a disconnect was triggered so the caller
can early-return, preserving identical semantics.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

c87093449d

move-only: Extract MaybeSendGetData() helper

Extracts the getdata section from SendMessages() into a self-contained
helper. The helper covers block requests (FindNextBlocksToDownload and
historical-block ranges), transaction download requests, and the final
getdata flush. vGetData is local to the helper; the m_tx_download_mutex
acquisition moves inside.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

6eb3f3e051

move-only: Extract SendCompactBlockOrHeaders() helper

Extracts the compact-block/headers send path from
MaybeSendBlockAnnouncements() into a self-contained helper. The helper
chooses between a compact-block (with cached-message optimisation),
a headers message, or sets fRevertToInv when neither format applies.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

6d022c3135

move-only: Extract SendBlockInvFallback() helper

Extracts the inv-fallback path from MaybeSendBlockAnnouncements() into
a self-contained helper. The helper inv-announces the chain tip when
neither a headers nor a compact-block relay was possible.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

c656841fcc

move-only: Extract CheckBlockDownloadStall() helper

Extracts the stall-detection block from CheckBlockSyncTimeouts() into a
self-contained helper. The helper detects when the block-download window
has stalled and disconnects the peer, applying an adaptive timeout backoff
to avoid disconnecting multiple peers when local bandwidth is insufficient.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

2790fecd56

move-only: Extract CheckBlockFlightTimeout() helper

Extracts the per-block in-flight timeout check from CheckBlockSyncTimeouts()
into a self-contained helper. The helper disconnects a peer when a requested
block has been in flight longer than the threshold, compensating for the
number of peers with validated downloads.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

608b92fdac

move-only: Extract CheckHeadersSyncTimeout() helper

Extracts the initial headers-sync timeout check from CheckBlockSyncTimeouts()
into a self-contained helper. The helper disconnects (or resets sync state for
NoBan peers) when initial headers sync has stalled, and resets the timeout
once the chain has caught up.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

0164ba0c2b

move-only: Extract QueueBlocksGetData() helper

Extracts the block getdata request queuing logic from MaybeSendGetData() into
a self-contained helper. The helper queues blocks via FindNextBlocksToDownload
and TryDownloadingHistoricalBlocks, and marks a stalling peer when the
in-flight queue runs empty.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

0518a402df

move-only: Extract QueueTxGetData() helper

Extracts the transaction getdata request queuing logic from MaybeSendGetData()
into a self-contained helper. The helper acquires m_tx_download_mutex, iterates
pending tx requests from m_txdownloadman, and flushes intermediate batches when
the getdata size limit is reached.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

062920c034

move-only: Extract ComputeSyncBlocksAndHeaders() helper

Extracts the sync-peer eligibility computation from the SendMessages() cs_main
block into a self-contained helper. The helper initialises m_best_header if
needed and returns whether blocks and headers should be synced from this peer
based on preferred-download status and current in-flight state.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

1baa4160a2

DrahtBot added the label Refactoring on Jun 12, 2026
DrahtBot commented at 11:48 PM on June 12, 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/35522.



Reviews

See the guideline for information on the review process.

Type Reviewers

Approach ACK w0xlt

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)
- #35561 (net: move some CNodeState fields to Peer by Crypt-iQ)
- #35558 (p2p: Prefill compact blocks by davidgumberg)
- #35511 (RFC: consensus: Make CAmount a class by hodlinator)
- #35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
- #35315 (refactor: Use NodeClock::time_point in more places by maflcko)
- #35229 (refactor: Use CBlockIndex parameters as reference by optout21)
- #34824 (net: encapsulate TxRelay state and replace recursive mutexes by w0xlt)
- #34743 (p2p: don't disconnect manual peers for block stalling by willcl-ark)
- #34628 (p2p: Replace per-peer transaction rate-limiting with global rate limits by ajtowns)
- #34565 (refactor: extract BlockDownloadManager from PeerManagerImpl by w0xlt)
- #33854 (fix assumevalid is ignored during reindex by Eunovo)
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.
pablomartin4btc commented at 11:52 PM on June 12, 2026: member

@w0xlt, my first impulse is to put it into draft till your both PRs #34824 and #35502 land and get merged before but perhaps you find this useful and easier to work on those changes on top of this one?

Type	Reviewers
Approach ACK	w0xlt

in src/net_processing.cpp:830 in f69c587028

 825 | +
 826 | +    /** Compute whether to sync blocks and headers from this peer; also initialises m_best_header. */
 827 | +    bool ComputeSyncBlocksAndHeaders(const CNode& node, const Peer& peer, const CNodeState& state)
 828 | +        EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
 829 | +
 830 | +    /** Schedule trickle and send tx inventory messages; requires tx_relay lock held. */

w0xlt commented at 7:48 AM on June 14, 2026:

MaybeSendTxMessages acquires tx_relay lock internally. It only requires cs_main and g_msgproc_mutex held.

    /** Schedule trickle and send tx inventory messages. */

pablomartin4btc commented at 2:25 PM on June 15, 2026:

Done. Thanks!

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

Approach ACK, same pattern as #35502.
w0xlt commented at 7:52 AM on June 14, 2026: contributor

@w0xlt, my first impulse is to put it into draft till your both PRs #34824 and #35502 land and get merged before but perhaps you find this useful and easier to work on those changes on top of this one?

I don’t think this PR needs to be draft. Keeping it open is useful for review.

If this, #34824, or #35502 lands first, rebasing should be straightforward.

move-only: Extract MaybeSendTxMessages() helper

Extracts the tx-relay trickle and inventory dispatch block from the
SendMessages() cs_main block into a self-contained helper. The helper acquires
the per-peer tx_inventory_mutex, schedules the trickle interval, and
conditionally sends the mempool response and tx inventory.

This commit can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

06741a6f9d

pablomartin4btc force-pushed on Jun 15, 2026
pablomartin4btc commented at 3:05 PM on June 15, 2026: member
-<ins>Updates</ins>:
- Corrected misleading comment on MaybeSendTxMessages(), observed by @w0xlt.
(iwyu CI failure is not related and I think it's being fixed in #35535)
DrahtBot added the label CI failed on Jun 15, 2026
DrahtBot commented at 3:48 PM on June 15, 2026: contributor


🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/27553084448/job/81447465260</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU detected and auto-modified an include in src/bench/pool.cpp (and intentionally returned failure: “Failure generated from IWYU”).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
- Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
- A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
- An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.

</details>
DrahtBot removed the label CI failed on Jun 16, 2026

Contributors

Labels

Linked (view graph)

#9579 Net: Trivial-review: Make SendMessages easier to review #9608 Net: Divide ProcessMessage in smaller functions #34059 refactor: Use NodeClock::time_point for m_addr_token_timestamp #34824 net: encapsulate TxRelay state and replace recursive mutexes #35148 refactor: Remove confusing DataStream::in_avail() alias #35502 refactor: extract per-message helpers from ProcessMessage (move-only)

refactor: Extract per-message helpers from SendMessages() (move-only) #35522

Code Coverage & Benchmarks

Reviews

Conflicts