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

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.

-Notes:

First (prepare) commit removes the unnecessary __func__ prefix from the debug logs in SendMessages(), as suggested by maflcko. The logging macros already capture the calling function via SourceLocation, available on demand with -logsourcelocations.

Each of the following 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.

One final cleanup commit normalises formatting and simplifies some of the extracted helpers, preserving existing control flow and lock scopes.

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.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35522
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35522.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35522#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	hodlinator
Stale 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:

#36598 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36598.svg"> ([Demo] validation: remove AssumeUTXO by l0rinc)
#36521 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36521.svg"> (Keep witness stacks in serialized form in CScriptWitness by sipa)
#35561 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35561.svg"> (net: move some CNodeState fields to Peer by Crypt-iQ)
#35558 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35558.svg"> (p2p: Prefill compact blocks by davidgumberg)
#35229 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35229.svg"> (refactor: Use CBlockIndex parameters as reference by optout21)
#34824 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/34824.svg"> (net: encapsulate TxRelay state and replace recursive mutexes by w0xlt)

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?

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. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/27553084448/job/81447465260 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”).

<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

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.

DrahtBot added the label Needs rebase on Sep 14, 2026

hodlinator commented at 9:07 AM on September 15, 2026: contributor

Concept ACK

Was recently reviewing some code here and felt it would have been nice to be able to refer to smaller parts of SendMessages() by name.

w0xlt commented at 6:30 PM on September 24, 2026: contributor

I rebased this branch locally onto bfdcd9797cd1a1345bdaf7ee7ef48f94028262da and I tried a few non-blocking cleanups locally, similar to those in #35502:

<details> <summary>Details</summary>

Aligned multiline parameter lists, logging arguments, and the fRevertToInv condition; normalized pointer/reference spacing and single-statement if formatting.
Renamed MaybeSendInitialGetheaders() to MaybeSendInitialGetHeaders(), matching MaybeSendGetHeaders().
Grouped both sends in MaybeSendTxMessages() under one if (ScheduleTxRelayTrickle(...)), removing the temporary and repeated checks.
Combined the nested bloom-filter conditions in MaybeSendMempoolResponse() and staller conditions in QueueBlocksGetData(), preserving short-circuit evaluation.
Replaced comparisons of size() with zero with empty() in MaybeSendTxInventory() and CheckBlockFlightTimeout().
Simplified CheckBlockSyncTimeouts() to return CheckHeadersSyncTimeout(...) directly. In ComputeSyncBlocksAndHeaders(), removed the redundant else after return and returned the final boolean condition directly.

</details>

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ae1593af46..2302ca7379 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -858,9 +858,9 @@ private:
         EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex, tx_relay.m_tx_inventory_mutex);
 
     /** Kick off initial headers sync with a peer if not yet started. */
-    void MaybeSendInitialGetheaders(CNode& node, Peer& peer, CNodeState& state,
-        bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
-        const Consensus::Params& consensusParams)
+    void MaybeSendInitialGetHeaders(CNode& node, Peer& peer, CNodeState& state,
+                                    bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
+                                    const Consensus::Params& consensusParams)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
 
     /** Announce new blocks to a peer via headers, compact blocks, or inv as appropriate. */
@@ -869,8 +869,8 @@ private:
 
     /** Send queued headers as a compact block or headers message; sets fRevertToInv if neither applies. */
     void SendCompactBlockOrHeaders(CNode& node, Peer& peer, CNodeState& state,
-        const std::vector<CBlock>& vHeaders, const CBlockIndex* pBestIndex,
-        bool& fRevertToInv)
+                                   const std::vector<CBlock>& vHeaders, const CBlockIndex* pBestIndex,
+                                   bool& fRevertToInv)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex, !m_most_recent_block_mutex);
 
     /** Fall back to inv-announcing the chain tip when header relay is not possible. */
@@ -883,8 +883,8 @@ private:
 
     /** Check stall and download timeouts; returns true if the caller should return early. */
     bool CheckBlockSyncTimeouts(CNode& node, Peer& peer, CNodeState& state,
-        std::chrono::microseconds current_time,
-        const Consensus::Params& consensusParams)
+                                std::chrono::microseconds current_time,
+                                const Consensus::Params& consensusParams)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
 
     /** Pause block downloads from manual peers or disconnect others on a stall; returns true if detected. */
@@ -893,28 +893,28 @@ private:
 
     /** Disconnect peer if a block in flight has timed out; returns true if disconnected. */
     bool CheckBlockFlightTimeout(CNode& node, CNodeState& state,
-        std::chrono::microseconds current_time, const Consensus::Params& consensusParams)
+                                 std::chrono::microseconds current_time, const Consensus::Params& consensusParams)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
 
     /** Disconnect or reset sync state if initial headers sync has timed out; returns true if disconnected. */
     bool CheckHeadersSyncTimeout(CNode& node, Peer& peer, CNodeState& state,
-        std::chrono::microseconds current_time)
+                                 std::chrono::microseconds current_time)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
 
     /** Build and send getdata requests for blocks and transactions. */
     void MaybeSendGetData(CNode& node, Peer& peer, CNodeState& state,
-        bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time)
+                          bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex, !m_tx_download_mutex);
 
     /** Queue block getdata requests into vGetData. */
     void QueueBlocksGetData(CNode& node, Peer& peer, CNodeState& state,
-        bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
-        std::vector<CInv>& vGetData)
+                            bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
+                            std::vector<CInv>& vGetData)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
 
     /** Queue transaction getdata requests into vGetData. */
     void QueueTxGetData(CNode& node, Peer& peer, std::chrono::microseconds current_time,
-        std::vector<CInv>& vGetData)
+                        std::vector<CInv>& vGetData)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex, !m_tx_download_mutex);
 
     /** Compute whether to sync blocks and headers from this peer; also initialises m_best_header. */
@@ -923,7 +923,7 @@ private:
 
     /** Schedule trickle and send tx inventory messages. */
     void MaybeSendTxMessages(CNode& node, Peer& peer, std::vector<CInv>& vInv,
-        std::chrono::microseconds current_time)
+                             std::chrono::microseconds current_time)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
 
     FastRandomContext m_rng GUARDED_BY(NetEventsInterface::g_msgproc_mutex);
@@ -6195,9 +6195,7 @@ void PeerManagerImpl::MaybeSendMempoolResponse(Peer::TxRelay& tx_relay, Peer& pe
         if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
             continue;
         }
-        if (tx_relay.m_bloom_filter) {
-            if (!tx_relay.m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue;
-        }
+        if (tx_relay.m_bloom_filter && !tx_relay.m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue;
         tx_relay.m_tx_inventory_known_filter.insert(inv.hash);
         vInv.push_back(inv);
         if (vInv.size() == MAX_INV_SZ) {
@@ -6220,7 +6218,7 @@ void PeerManagerImpl::MaybeSendTxInventory(Peer::TxRelay& tx_relay, Peer& peer,
         auto& invs = tx_relay.m_tx_inventory_to_send;
         std::vector<CTransactionRef> res;
 
-        if (invs.size() == 0) return res;
+        if (invs.empty()) return res;
 
         // if previous allocations were excessive, shrink to the current size
         if (invs.capacity() > 2 * invs.size()) invs.shrink_to_fit();
@@ -6263,9 +6261,9 @@ void PeerManagerImpl::MaybeSendTxInventory(Peer::TxRelay& tx_relay, Peer& peer,
     }
 }
 
-void PeerManagerImpl::MaybeSendInitialGetheaders(CNode& node, Peer& peer, CNodeState& state,
-    bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
-    const Consensus::Params& consensusParams)
+void PeerManagerImpl::MaybeSendInitialGetHeaders(CNode& node, Peer& peer, CNodeState& state,
+                                                 bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
+                                                 const Consensus::Params& consensusParams)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6281,8 +6279,7 @@ void PeerManagerImpl::MaybeSendInitialGetheaders(CNode& node, Peer& peer, CNodeS
                the peer's known best block.  This wouldn't be possible
                if we requested starting at m_chainman.m_best_header and
                got back an empty response.  */
-            if (pindexStart->pprev)
-                pindexStart = pindexStart->pprev;
+            if (pindexStart->pprev) pindexStart = pindexStart->pprev;
             if (MaybeSendGetHeaders(node, GetLocator(pindexStart), peer)) {
                 LogDebug(BCLog::NET, "initial getheaders (%d) to peer=%d", pindexStart->nHeight, node.GetId());
 
@@ -6301,8 +6298,8 @@ void PeerManagerImpl::MaybeSendInitialGetheaders(CNode& node, Peer& peer, CNodeS
 }
 
 void PeerManagerImpl::SendCompactBlockOrHeaders(CNode& node, Peer& peer, CNodeState& state,
-    const std::vector<CBlock>& vHeaders, const CBlockIndex* pBestIndex,
-    bool& fRevertToInv)
+                                                const std::vector<CBlock>& vHeaders, const CBlockIndex* pBestIndex,
+                                                bool& fRevertToInv)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6312,7 +6309,7 @@ void PeerManagerImpl::SendCompactBlockOrHeaders(CNode& node, Peer& peer, CNodeSt
         // We only send up to 1 block as header-and-ids, as otherwise
         // probably means we're doing an initial-ish-sync or they're slow
         LogDebug(BCLog::NET, "%s sending header-and-ids %s to peer=%d\n", __func__,
-                vHeaders.front().GetHash().ToString(), node.GetId());
+                 vHeaders.front().GetHash().ToString(), node.GetId());
 
         std::optional<CSerializedNetMsg> cached_cmpctblock_msg;
         {
@@ -6334,12 +6331,12 @@ void PeerManagerImpl::SendCompactBlockOrHeaders(CNode& node, Peer& peer, CNodeSt
     } else if (peer.m_prefers_headers) {
         if (vHeaders.size() > 1) {
             LogDebug(BCLog::NET, "%s: %u headers, range (%s, %s), to peer=%d\n", __func__,
-                    vHeaders.size(),
-                    vHeaders.front().GetHash().ToString(),
-                    vHeaders.back().GetHash().ToString(), node.GetId());
+                     vHeaders.size(),
+                     vHeaders.front().GetHash().ToString(),
+                     vHeaders.back().GetHash().ToString(), node.GetId());
         } else {
             LogDebug(BCLog::NET, "%s: sending header %s to peer=%d\n", __func__,
-                    vHeaders.front().GetHash().ToString(), node.GetId());
+                     vHeaders.front().GetHash().ToString(), node.GetId());
         }
         MakeAndPushMessage(node, NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders));
         state.pindexBestHeaderSent = pBestIndex;
@@ -6367,14 +6364,14 @@ void PeerManagerImpl::SendBlockInvFallback(CNode& node, Peer& peer, CNodeState&
         // Just log for now.
         if (m_chainman.ActiveChain()[pindex->nHeight] != pindex) {
             LogDebug(BCLog::NET, "Announcing block %s not on main chain (tip=%s)\n",
-                hashToAnnounce.ToString(), m_chainman.ActiveChain().Tip()->GetBlockHash().ToString());
+                     hashToAnnounce.ToString(), m_chainman.ActiveChain().Tip()->GetBlockHash().ToString());
         }
 
         // If the peer's chain has this block, don't inv it back.
         if (!PeerHasHeader(&state, pindex)) {
             peer.m_blocks_for_inv_relay.push_back(hashToAnnounce);
             LogDebug(BCLog::NET, "%s: sending inv peer=%d hash=%s\n", __func__,
-                node.GetId(), hashToAnnounce.ToString());
+                     node.GetId(), hashToAnnounce.ToString());
         }
     }
 }
@@ -6395,9 +6392,9 @@ void PeerManagerImpl::MaybeSendBlockAnnouncements(CNode& node, Peer& peer, CNode
     LOCK(peer.m_block_inv_mutex);
     std::vector<CBlock> vHeaders;
     bool fRevertToInv = ((!peer.m_prefers_headers &&
-                         (!state.m_requested_hb_cmpctblocks || peer.m_blocks_for_headers_relay.size() > 1)) ||
+                          (!state.m_requested_hb_cmpctblocks || peer.m_blocks_for_headers_relay.size() > 1)) ||
                          peer.m_blocks_for_headers_relay.size() > MAX_BLOCKS_TO_ANNOUNCE);
-    const CBlockIndex *pBestIndex = nullptr; // last header queued for delivery
+    const CBlockIndex* pBestIndex = nullptr; // last header queued for delivery
     ProcessBlockAvailability(node.GetId()); // ensure pindexBestKnownBlock is up-to-date
 
     if (!fRevertToInv) {
@@ -6474,7 +6471,7 @@ void PeerManagerImpl::MaybeSendBlockInv(CNode& node, Peer& peer, std::vector<CIn
 }
 
 bool PeerManagerImpl::CheckBlockDownloadStall(CNode& node, CNodeState& state,
-    std::chrono::microseconds current_time)
+                                              std::chrono::microseconds current_time)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6507,7 +6504,7 @@ bool PeerManagerImpl::CheckBlockDownloadStall(CNode& node, CNodeState& state,
 }
 
 bool PeerManagerImpl::CheckBlockFlightTimeout(CNode& node, CNodeState& state,
-    std::chrono::microseconds current_time, const Consensus::Params& consensusParams)
+                                              std::chrono::microseconds current_time, const Consensus::Params& consensusParams)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6517,8 +6514,8 @@ bool PeerManagerImpl::CheckBlockFlightTimeout(CNode& node, CNodeState& state,
     // We compensate for other peers to prevent killing off peers due to our own downstream link
     // being saturated. We only count validated in-flight blocks so peers can't advertise non-existing block hashes
     // to unreasonably increase our timeout.
-    if (state.vBlocksInFlight.size() > 0) {
-        QueuedBlock &queuedBlock = state.vBlocksInFlight.front();
+    if (!state.vBlocksInFlight.empty()) {
+        QueuedBlock& queuedBlock = state.vBlocksInFlight.front();
         int nOtherPeersWithValidatedDownloads = m_peers_downloading_from - 1;
         if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) {
             LogInfo("Timeout downloading block %s, %s", queuedBlock.pindex->GetBlockHash().ToString(), node.DisconnectMsg());
@@ -6530,21 +6527,19 @@ bool PeerManagerImpl::CheckBlockFlightTimeout(CNode& node, CNodeState& state,
 }
 
 bool PeerManagerImpl::CheckBlockSyncTimeouts(CNode& node, Peer& peer, CNodeState& state,
-    std::chrono::microseconds current_time,
-    const Consensus::Params& consensusParams)
+                                             std::chrono::microseconds current_time,
+                                             const Consensus::Params& consensusParams)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
 
     if (CheckBlockDownloadStall(node, state, current_time)) return true;
     if (CheckBlockFlightTimeout(node, state, current_time, consensusParams)) return true;
-    if (CheckHeadersSyncTimeout(node, peer, state, current_time)) return true;
-
-    return false;
+    return CheckHeadersSyncTimeout(node, peer, state, current_time);
 }
 
 bool PeerManagerImpl::CheckHeadersSyncTimeout(CNode& node, Peer& peer, CNodeState& state,
-    std::chrono::microseconds current_time)
+                                              std::chrono::microseconds current_time)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6586,8 +6581,8 @@ bool PeerManagerImpl::CheckHeadersSyncTimeout(CNode& node, Peer& peer, CNodeStat
 }
 
 void PeerManagerImpl::QueueBlocksGetData(CNode& node, Peer& peer, CNodeState& state,
-    bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
-    std::vector<CInv>& vGetData)
+                                         bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time,
+                                         std::vector<CInv>& vGetData)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6614,24 +6609,22 @@ void PeerManagerImpl::QueueBlocksGetData(CNode& node, Peer& peer, CNodeState& st
                 get_inflight_budget(),
                 vToDownload, from_tip, historical_blocks->second);
         }
-        for (const CBlockIndex *pindex : vToDownload) {
+        for (const CBlockIndex* pindex : vToDownload) {
             uint32_t nFetchFlags = GetFetchFlags(peer);
             vGetData.emplace_back(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash());
             BlockRequested(node.GetId(), *pindex);
             LogDebug(BCLog::NET, "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(),
-                pindex->nHeight, node.GetId());
+                     pindex->nHeight, node.GetId());
         }
-        if (state.vBlocksInFlight.empty() && staller != -1) {
-            if (State(staller)->m_stalling_since == 0us) {
-                State(staller)->m_stalling_since = current_time;
-                LogDebug(BCLog::NET, "Stall started peer=%d\n", staller);
-            }
+        if (state.vBlocksInFlight.empty() && staller != -1 && State(staller)->m_stalling_since == 0us) {
+            State(staller)->m_stalling_since = current_time;
+            LogDebug(BCLog::NET, "Stall started peer=%d\n", staller);
         }
     }
 }
 
 void PeerManagerImpl::QueueTxGetData(CNode& node, Peer& peer, std::chrono::microseconds current_time,
-    std::vector<CInv>& vGetData)
+                                     std::vector<CInv>& vGetData)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6648,7 +6641,7 @@ void PeerManagerImpl::QueueTxGetData(CNode& node, Peer& peer, std::chrono::micro
 }
 
 void PeerManagerImpl::MaybeSendGetData(CNode& node, Peer& peer, CNodeState& state,
-    bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time)
+                                       bool sync_blocks_and_headers_from_peer, std::chrono::microseconds current_time)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6665,30 +6658,29 @@ void PeerManagerImpl::MaybeSendGetData(CNode& node, Peer& peer, CNodeState& stat
     //
     QueueTxGetData(node, peer, current_time, vGetData);
 
-    if (!vGetData.empty())
-        MakeAndPushMessage(node, NetMsgType::GETDATA, vGetData);
+    if (!vGetData.empty()) MakeAndPushMessage(node, NetMsgType::GETDATA, vGetData);
 }
 
 void PeerManagerImpl::MaybeSendTxMessages(CNode& node, Peer& peer, std::vector<CInv>& vInv,
-    std::chrono::microseconds current_time)
+                                          std::chrono::microseconds current_time)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
 
     if (auto tx_relay = peer.GetTxRelay(); tx_relay != nullptr) {
         LOCK(tx_relay->m_tx_inventory_mutex);
-        const bool fSendTrickle{ScheduleTxRelayTrickle(*tx_relay, node, current_time)};
-
-        // Respond to BIP35 mempool requests
-        if (fSendTrickle) MaybeSendMempoolResponse(*tx_relay, peer, node, vInv);
+        if (ScheduleTxRelayTrickle(*tx_relay, node, current_time)) {
+            // Respond to BIP35 mempool requests
+            MaybeSendMempoolResponse(*tx_relay, peer, node, vInv);
 
-        // Determine transactions to relay
-        if (fSendTrickle) MaybeSendTxInventory(*tx_relay, peer, node, vInv);
+            // Determine transactions to relay
+            MaybeSendTxInventory(*tx_relay, peer, node, vInv);
+        }
     }
 }
 
 bool PeerManagerImpl::ComputeSyncBlocksAndHeaders(const CNode& node, const Peer& peer,
-    const CNodeState& state)
+                                                  const CNodeState& state)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(g_msgproc_mutex);
@@ -6703,7 +6695,8 @@ bool PeerManagerImpl::ComputeSyncBlocksAndHeaders(const CNode& node, const Peer&
     // in IBD (once out of IBD, we sync from all peers).
     if (state.fPreferredDownload) {
         return true;
-    } else if (CanServeBlocks(peer) && !node.IsAddrFetchConn()) {
+    }
+    if (CanServeBlocks(peer) && !node.IsAddrFetchConn()) {
         // Typically this is an inbound peer. If we don't have any outbound
         // peers, or if we aren't downloading any blocks from such peers,
         // then allow block downloads from this peer, too.
@@ -6713,9 +6706,7 @@ bool PeerManagerImpl::ComputeSyncBlocksAndHeaders(const CNode& node, const Peer&
         // the latest blocks is from an inbound peer, we have to be sure to
         // eventually download it (and not just wait indefinitely for an
         // outbound peer to have it).
-        if (m_num_preferred_download_peers == 0 || mapBlocksInFlight.empty()) {
-            return true;
-        }
+        return m_num_preferred_download_peers == 0 || mapBlocksInFlight.empty();
     }
     return false;
 }
@@ -6741,8 +6732,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
     }
 
     // Don't send anything until the version handshake is complete
-    if (!node.fSuccessfullyConnected || node.fDisconnect)
-        return true;
+    if (!node.fSuccessfullyConnected || node.fDisconnect) return true;
 
     const auto now{NodeClock::now()};
     const auto current_time{GetTime<std::chrono::microseconds>()};
@@ -6779,11 +6769,11 @@ bool PeerManagerImpl::SendMessages(CNode& node)
     {
         LOCK(cs_main);
 
-        CNodeState &state = *State(node.GetId());
+        CNodeState& state = *State(node.GetId());
 
         const bool sync_blocks_and_headers_from_peer{ComputeSyncBlocksAndHeaders(node, peer, state)};
 
-        MaybeSendInitialGetheaders(node, peer, state, sync_blocks_and_headers_from_peer, current_time, consensusParams);
+        MaybeSendInitialGetHeaders(node, peer, state, sync_blocks_and_headers_from_peer, current_time, consensusParams);
 
         //
         // Try sending block announcements via headers
@@ -6797,8 +6787,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
         MaybeSendBlockInv(node, peer, vInv);
 
         MaybeSendTxMessages(node, peer, vInv, current_time);
-        if (!vInv.empty())
-            MakeAndPushMessage(node, NetMsgType::INV, vInv);
+        if (!vInv.empty()) MakeAndPushMessage(node, NetMsgType::INV, vInv);
 
         if (CheckBlockSyncTimeouts(node, peer, state, current_time, consensusParams)) return true;

</details>

I also adjusted the commit history:

Documented the four __func__ log-prefix changes in the relevant extraction messages: first SendMessages → MaybeSendBlockAnnouncements, then three logs → SendCompactBlockOrHeaders and one → SendBlockInvFallback.
Moved the premature declarations into their respective extraction commits: MaybeSendTxMessages(), CheckBlockFlightTimeout(), CheckHeadersSyncTimeout(), and QueueTxGetData().

Indentation-only adjustments are included in the extraction commits, with the remaining formatting and refactoring changes batched into one cleanup commit at the end.

Or, if you prefer, you can use the updated branch with these changes already applied:

https://github.com/w0xlt/bitcoin/tree/refactor/extract-sendmessages-helpers

pablomartin4btc force-pushed on Oct 8, 2026

DrahtBot removed the label Needs rebase on Oct 8, 2026

w0xlt commented at 11:21 PM on October 8, 2026: contributor

ACK 78a02a95888086909d184a662c17c54234ef34f7

DrahtBot requested review from hodlinator on Oct 8, 2026

pablomartin4btc commented at 11:22 PM on October 8, 2026: member

I rebased this branch locally onto bfdcd97 and I tried a few non-blocking cleanups locally, similar to those in #35502... ... I also adjusted the commit history... ... Indentation-only adjustments are included in the extraction commits, with the remaining formatting and refactoring changes batched into one cleanup commit at the end...

Thanks for the rebase, the cleanup suggestions, and the commit-history reorganization, @w0xlt — appreciated.

pablomartin4btc commented at 11:37 PM on October 8, 2026: member

-<ins>Updates</ins>:

Rebased.
- Re-threaded a ping-timeout check that landed in SendMessages() upstream after the previous rebase base, now folded into CheckBlockSyncTimeouts() (d83647e7c4 in #36080).
Addressed @w0xlt's feedback adding:
- __func__ log-prefix documentation within affected commit messages;
- premature-declaration moves;
- a cleanup commit.
Updated PR description accordingly.

maflcko commented at 11:18 AM on October 13, 2026: member

No behavior change apart from the __func__ prefix in four debug logs that moved out of SendMessages(), which now print SendCompactBlockOrHeaders (three logs) or SendBlockInvFallback (one log) instead of SendMessages.

This gives me LLM vibes. Can you please write the description in your own words? See https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md

Forcing reviewers to count all __func__ seems like busy work and all the weird formatting will likely look ugly in a terminal-printed git log.

In any case, the logs shouldn't be using __func__ anyway. Each log line should be unique and descriptive by itself. Maybe a prepare commit can be added to do that?

This follows the pattern of similar PRs (#35502, and some commits fa5ab02 - from #35148 -, and fa55723 from #34059)

I think all of those mentions can be removed, as they are about the processing side, not the sending side.

pablomartin4btc commented at 6:56 AM on October 14, 2026: member

Thanks @maflcko!

No behavior change apart from the __func__ prefix in four debug logs that moved out of SendMessages(), which now print SendCompactBlockOrHeaders (three logs) or SendBlockInvFallback (one log) instead of SendMessages.

This gives me LLM vibes. Can you please write the description in your own words? See https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md

Ok, it doesn't sound to me like that but given the reasons below I'll remove it anyway. I originally included it because a reviewer had already flagged that "move-only" wasn't quite accurate — the log calls print different output on this branch (the new extracted function names) versus master (SendMessages) — so I thought it was worth calling out in the description too.

Forcing reviewers to count all __func__ seems like busy work and all the weird formatting will likely look ugly in a terminal-printed git log.

Ok, I'll take it into account for next PRs, and I also forgot about the git log consequences. Again, it will be removed as part of the change you suggested below.

In any case, the logs shouldn't be using __func__ anyway. Each log line should be unique and descriptive by itself. Maybe a prepare commit can be added to do that?

I'd do that, sure — I see the logging macros already capture the function name/file/line automatically via SourceLocation, and it's available on demand with -logsourcelocations, so there's no need to hand-embed __func__ in the message.

This follows the pattern of similar PRs (#35502, and some commits fa5ab02 - from #35148 -, and fa55723 from #34059)

I think all of those mentions can be removed, as they are about the processing side, not the sending side.

I'll remove that too; it seems misleading.

net_processing: Drop __func__ prefix from SendMessages() debug logs

These four debug logs in SendMessages() use __func__ to prefix their
output. The logging macros already capture the calling function, file,
and line automatically via SourceLocation, available on demand with
-logsourcelocations, so the manual __func__ prefix is redundant.

It is also fragile under refactoring: relying on __func__ for a log
line's identity means moving that log into a differently-named helper
silently changes the printed output, even though nothing about what is
being logged changed. Writing each line so it reads clearly without
needing the surrounding function's name avoids that.

No other change.

48a2f4a830

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

6a7ac6d85c

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

87c73a84aa

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

3162aecf70

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

d33231ad4e

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

bdb28b49ee

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

ce08bb9601

move-only: Extract CheckBlockSyncTimeouts() helper

Extracts the stall/timeout checks from SendMessages() into a
self-contained helper: block-download stall detection (with adaptive
timeout backoff), per-block in-flight timeout, ping timeout, and
headers-sync timeout. Returns true when the caller should return
early, preserving identical semantics, including the manual-peer
block-download cooldown.

The ping timeout check was added to SendMessages() upstream after this
branch's original base; it's included here as part of the same move,
taking the now parameter needed for it.

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

71b17d06e6

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

a90ea0730a

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

1b6cfb7ba6

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

ee150647ca

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, pauses block requests to manual peers, and disconnects other
peers. It applies adaptive timeout backoff when local bandwidth may be
insufficient.

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

949774dfae

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

Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>

bacd895dcf

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

Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>

dbee2e9efa

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

709543cc57

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

Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>

9e2e556713

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

ffb7ddbc8d

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

Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>

75db2ad2a6

refactor: Clean up extracted SendMessages helpers

Rename MaybeSendInitialGetheaders() to MaybeSendInitialGetHeaders() to
match MaybeSendGetHeaders(), and normalize pointer/reference spacing and
single-statement conditionals.

Share the transaction relay scheduling guard, combine the bloom-filter
and staller checks, use empty() for container checks, and simplify the
timeout and sync-selection returns. Preserve evaluation order and lock
scopes.

Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>

4d707f65bd

pablomartin4btc force-pushed on Oct 19, 2026

pablomartin4btc commented at 10:40 PM on October 19, 2026: member

-<ins>Updates</ins>:

Addressed @maflcko's feedback.
- Created a new prepare commit removing __func__ prefix from the debug logs in SendMessages().
- Removed misleading processing side references from the PR description.