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 +661 −424

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.
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 and AI policy 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:
- #35839 (p2p: select another peer after empty headers by l0rinc)
- #35561 (net: move some CNodeState fields to Peer by Crypt-iQ)
- #35558 (p2p: Prefill compact blocks by davidgumberg)
- #35229 (refactor: Use CBlockIndex parameters as reference by optout21)
- #34743 (p2p: don't disconnect manual peers for block stalling by willcl-ark)
- #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.
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
DrahtBot added the label Needs rebase on Jul 25, 2026

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

4994f71dc9

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

852d4414c4

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

5bc8bc93e7

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

1ebdde9815

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

47d622a0f1

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

9d24eee37c

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

5c603dd3cc

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

1c93ec4c8d

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

88e1d0ef52

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

e02e05c198

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

6c47f463b2

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

714ece749a

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

d9caa959e7

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

7f39d84c06

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

fc962d9281

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

7350a15e5b

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

03c5b2ba6d

pablomartin4btc force-pushed on Aug 12, 2026
DrahtBot removed the label Needs rebase on Aug 13, 2026
pablomartin4btc commented at 1:14 AM on August 13, 2026: member
-<ins>Updates</ins>:
- Rebased.

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