refactor: extract per-message helpers from ProcessMessage (move-only) #35502

w0xlt commented at 1:54 AM on June 10, 2026: contributor

PeerManagerImpl::ProcessMessage() is a ~1000-line function handling every p2p message type inline, which makes it hard to navigate and review.

This PR continues splitting it into per-message helper functions, following the pattern of the recently merged fa5ab0220e02377c3c855042ecdf1f5f950d0965 (ProcessPong()) and fa55723b8fbd4fd056dddac5b35daf2e86021422 (ProcessAddrs()), as suggested by maflcko #34588 (comment).

Nine handlers are extracted, one move-only commit each:

ProcessGetAddr() — getaddr
ProcessGetDataMessage() — getdata (named to avoid colliding with the existing ProcessGetData(), which services the request queue this handler fills)
ProcessGetBlocks() — getblocks
ProcessGetHeaders() — getheaders
ProcessInv() — inv
ProcessSendTxRcncl() — sendtxrcncl
ProcessTx() — tx
ProcessCompactBlock() — cmpctblock
ProcessVersion() — version

Each extraction commit is a code move with indentation adjustments: the only new lines are the declaration (with thread-safety annotations), the function signature, and the one-line call site. Each call site is Helper(...); return;, so return statements inside the moved bodies keep identical semantics. No behavior change apart from the __func__ prefix in one getheaders debug log, which now prints ProcessGetHeaders instead of ProcessMessage.

One final cleanup commit normalizes the remaining formatting, replaces equivalent break statements with return in ProcessSendTxRcncl(), and simplifies the send_getaddr conditional in ProcessVersion().

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

Declarations and definitions are placed next to related existing helpers (e.g. ProcessGetDataMessage next to ProcessGetData, ProcessGetBlocks next to ProcessGetBlockData, ProcessGetAddr after ProcessAddrs).

DrahtBot added the label Refactoring on Jun 10, 2026

DrahtBot commented at 1:54 AM on June 10, 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/35502.

Reviews

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

Type	Reviewers
ACK	l0rinc, pablomartin4btc
Concept ACK	stickies-v, theStack
Stale ACK	pseudoramdom, thomasbuilds

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:

#36277 (net: always complete all initial private broadcast connections by andrewtoth)
#36080 (p2p: Suspend ping timeout while downloading blocks from a peer by mzumsande)
#35936 (net: reject oversized locators before allocating by l0rinc)
#35920 (net_processing: Ignore MSG_WITNESS_TX entries from INV messages by ajtowns)
#35820 (refactor: keep duration calculations typed by l0rinc)
#35646 (RFC: Separate out runtime errors from BlockValidationState using util::Expected by yuvicc)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35561 (net: move some CNodeState fields to Peer by Crypt-iQ)
#35558 (p2p: Prefill compact blocks by davidgumberg)
#35321 (p2p: Misbehave on invalid compact block in optimistic reconstruction by ViniciusCestarii)
#34824 (net: encapsulate TxRelay state and replace recursive mutexes by w0xlt)
#34707 (net: keep finished private broadcast txs in memory by andrewtoth)
#30951 (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)

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.

stickies-v commented at 10:49 AM on June 10, 2026: contributor

Concept ACK. Pretty straightforward change that makes it easier to navigate net_processing and better encapsulates logic.

theStack commented at 1:19 PM on June 10, 2026: contributor

Concept ACK

Fwiw this has been proposed at least once: #9608 (it seems to have failed more due to lack of review back then rather than on strong pushback, as far as I understand)

pablomartin4btc commented at 3:06 PM on June 10, 2026: member

Concept ACK.

I'm in favour of these refactoring, while reviewing #34824 also identified another possible candidate SendMessages() which is in the same file.

pseudoramdom commented at 4:21 AM on June 11, 2026: contributor

code review ACK 4fe745f27bd8a4a637df522421a4a7104923ca3e Except for a new comment for ProcessGetDataMessage, verified the change is a move-only refactor. Also verified using git show--color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change <commit> against each commit.

DrahtBot requested review from stickies-v on Jun 11, 2026

DrahtBot requested review from theStack on Jun 11, 2026

DrahtBot requested review from pablomartin4btc on Jun 11, 2026

thomasbuilds commented at 6:08 AM on June 11, 2026: contributor

code review ACK 4fe745f

Verified each commit is a pure code move (only new lines are declarations, call sites, and one doc comment), call sites preserve return semantics, and the thread-safety annotations match what each body locks. It compiles cleanly with clang -Wthread-safety.

pablomartin4btc commented at 4:01 AM on June 12, 2026: member

ACK https://github.com/bitcoin/bitcoin/commit/4fe745f27bd8a4a637df522421a4a7104923ca3e.

ProcessMessage() went from ~1500 to ~1000 lines, but CMPCTBLOCK (248 lines) and VERSION (235 lines) remain the two dominant inline blocks — extracting those two alone would drop it another ~480 lines, bringing it under 550. The same move-only pattern applied here would make both independently reviewable. Not a blocker, perhaps worth a follow-up PR in the same vein?

On a side note, on the previous attempt mentioned above, there were concerns about "obscuring the control flow", I don't think this is the case here.

davidgumberg commented at 12:44 AM on June 19, 2026: contributor

Concept -0

Let's say I'm reading a function called ProcessBlock() in net_processing.cpp:

Is this function called by ProcessMessage()?
Does ProcessMessage() do any set up before invoking ProcessBlock()?
Are there any other callers of ProcessBlock() and what are their expectations?
Are all the callers of ProcessBlock() in the process messages thread?

These questions have to be answered when reading / modifying code and for all of the logic that lives in the ProcessMessage() ~switch statement these ambiguities don't exist. I think ProcessBlock() is a good example because it has multiple callers, and anyone reading it or modifying it should find them and think carefully about what they all do. That is a cost paid for by the reusability of ProcessBlock().

I think the main thing I don't share is the feeling that ProcessMessage() is too long. It's pretty flat, and there is ~0 state that lives outside of the if(msg_type) {, so I don't feel burdened by any of the rest of the function when I'm reading the logic for the message I'm interested in.

Not trying to bikeshed, the PR seems reasonable and everything would be fine if it was merged, just the POV of one person that happens to like ProcessMessage().

w0xlt commented at 7:33 AM on June 19, 2026: contributor

@davidgumberg Fair point. I agree the cost is real when a helper has multiple callers like ProcessBlock(), because setup and caller assumptions become less local.

These extractions are different though: they are private, single-caller helpers from ProcessMessage(), called as Helper(...); return;, so the control flow remains simple.

The benefit I see is lock reasoning. ProcessMessage() has to carry the broad lock contract needed by all branches:

EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex,
                         !m_headers_presync_mutex, g_msgproc_mutex,
                         !m_tx_download_mutex)

After extraction, each handler states only what it actually needs. For example, ProcessGetAddr() requires only g_msgproc_mutex, while ProcessTx() requires g_msgproc_mutex, !m_peer_mutex, !m_tx_download_mutex.

So this does not add runtime safety by itself, but it makes the per-message lock contract more local and compiler-checked.

Besides that, ProcessMessage() arguably accumulates disproportionate responsibility, and this extraction pattern addresses that, though I agree this is a more subjective point.

maflcko commented at 8:17 AM on June 19, 2026: member

Let's say I'm reading a function called ProcessBlock() in net_processing.cpp:
* Is this function called by `ProcessMessage()`?

Should be easy to answer with 'yes', otherwise the function wouldn't sit in the peer manager impl?

* Does `ProcessMessage()` do any set up before invoking `ProcessBlock()`?

Edit: I'd say generally the goal should be to avoid setup, but here it should be obvious from the function signature, which is passed min_pow_checked (and other stuff). There is also the possibility of different message types doing different setup steps, one would have to read the handshake code either way, before and after this pull request.

* Are there any other callers of `ProcessBlock()` and what are their expectations?

Should be easy to answer by looking at the header: If the function is public in the header, it is called externally. If it is not in the header, then it is not called externally. Generally, I don't think any Process* handler was ever exposed in the header?

* Are all the callers of `ProcessBlock()` in the process messages thread?

Should be easy to answer with yes, because no process handler was ever exposed publicly. Also, it should be easy to verify with a single call to git grep.

Generally, I think the benefits here are limited, because devs generally know that at most a single message is handled in the large body (and thus no variables leak from one message handling block to the next). However, I think there is still a benefit in being able to review code changes with the git option git diff --function-context easier.

Previously, it would basically print the whole file, even if only a single line in a single message type handling was changed. At least for me this makes review harder because I have to scroll past the irrelevant white lines every time and risk missing a green or red line.

After this change, git diff --function-context nicely prints only the relevant context, so at least for me review would be easier. Also, parts of this file already use this pattern, so for consistency it also makes sense. So I am Concept +1, but this is just me, and maybe other people are using a different review flow?

@w0xlt Please don't @ in pull descriptions. If this pull was merged, it would lead to ping spam every time the merge is cherry-picked.

davidgumberg commented at 2:15 AM on June 25, 2026: contributor

Let's say I'm reading a function called ProcessBlock() in net_processing.cpp:

Is this function called by ProcessMessage()?

Should be easy to answer with 'yes', otherwise the function wouldn't sit in the peer manager impl?

I meant directly, this is basically the same point as the set-up one.

Does ProcessMessage() do any set up before invoking ProcessBlock()?

Edit: I'd say generally the goal should be to avoid setup, but here it should be obvious from the function signature, which is passed min_pow_checked (and other stuff). There is also the possibility of different message types doing different setup steps, one would have to read the handshake code either way, before and after this pull request.

Right, but after this PR you have to look in multiple places, even if there is only one caller that doesn't do any setup and it's ProcessMessage()

Are there any other callers of ProcessBlock() and what are their expectations?

Should be easy to answer by looking at the header: If the function is public in the header, it is called externally. If it is not in the header, then it is not called externally. Generally, I don't think any Process* handler was ever exposed in the header?

Are all the callers of ProcessBlock() in the process messages thread?

Should be easy to answer with yes, because no process handler was ever exposed publicly. Also, it should be easy to verify with a single call to git grep.

Yes, all of these questions can be answered quite easily by checking in one or two files and doing a grep or two, my point is that one doesn't have to just check the header and do an extra grep or two now.

e.g. I don't use git diff --function-context but it sounds like reviewing ProcessMessage() might be pretty annoying if one does that. Of course I could recommend that you use a different set of flags or a different tool, or just open another window with the function in it, but any of these "solutions" would add friction for you i.e. they suck in comparison to using the tool / workflow you already like and are used to.

That being said, it sounds like other contributors prefer the style of this PR to the style of master, and given that this is a stylistic question the thing that people prefer is definitely the thing that should happen.

DrahtBot added the label Needs rebase on Jul 25, 2026

w0xlt force-pushed on Sep 16, 2026

w0xlt commented at 11:07 PM on September 16, 2026: contributor

Rebased. All suggestions addressed.

DrahtBot removed the label Needs rebase on Sep 17, 2026

DrahtBot added the label Needs rebase on Sep 22, 2026

move-only: Extract ProcessGetAddr() helper

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

c17c3d526b

w0xlt force-pushed on Sep 22, 2026

DrahtBot removed the label Needs rebase on Sep 22, 2026

in src/net_processing.cpp:3265 in 0c9fdcd21e outdated

3260 | +        pindex = m_chainman.m_blockman.LookupBlockIndex(hashStop);
3261 | +        if (!pindex) {
3262 | +            return;
3263 | +        }
3264 | +        if (!BlockRequestAllowed(*pindex)) {
3265 | +            LogDebug(BCLog::NET, "%s: ignoring request from peer=%i for old block header that isn't in the main chain\n", __func__, pfrom.GetId());

l0rinc commented at 1:12 AM on September 23, 2026:

0c9fdcd move-only: Extract ProcessGetHeaders() helper:

nit: this log changed because of __func__ returning "ProcessGetHeaders" intead of "ProcessMessage" - which is probably fine, give it's a debug log, but might be worth a mention

w0xlt commented at 9:06 PM on September 23, 2026:

Thanks. Done.

in src/net_processing.cpp:3269 in 0c9fdcd21e

3264 | +        if (!BlockRequestAllowed(*pindex)) {
3265 | +            LogDebug(BCLog::NET, "%s: ignoring request from peer=%i for old block header that isn't in the main chain\n", __func__, pfrom.GetId());
3266 | +            return;
3267 | +        }
3268 | +    }
3269 | +    else

l0rinc commented at 1:13 AM on September 23, 2026:

0c9fdcd move-only: Extract ProcessGetHeaders() helper:

nit: in a separate commit consider reformatting these, we shouldn't add new code that's off from the start

w0xlt commented at 9:07 PM on September 23, 2026:

Thanks. Done.

in src/net_processing.cpp:5159 in c17c3d526b outdated

5186 | -        }
5187 | -        for (const CAddress &addr : vAddr) {
5188 | -            PushAddress(peer, addr);
5189 | -        }
5190 | +        ProcessGetAddr(pfrom, peer);
5191 |          return;

l0rinc commented at 1:22 AM on September 23, 2026:

c17c3d5 move-only: Extract ProcessGetAddr() helper:

Note: these trailing returns allow us to cleanly extract the helpers since the previous return values short-circuited the whole external method - while after extraction they only cut the extracted method short.

w0xlt commented at 9:07 PM on September 23, 2026:

Thanks. Done.

in src/net_processing.cpp:5893 in 6bba1fc96b

5888 | +    switch (result) {
5889 | +    case ReconciliationRegisterResult::NOT_FOUND:
5890 | +        LogDebug(BCLog::NET, "Ignore unexpected txreconciliation signal from peer=%d\n", pfrom.GetId());
5891 | +        break;
5892 | +    case ReconciliationRegisterResult::SUCCESS:
5893 | +        break;

l0rinc commented at 1:29 AM on September 23, 2026:

6bba1fc move-only: Extract ProcessSendTxRcncl() helper:

Can be return after the move for consistency, it's confusing to have both break and return (doing the same in this context)

w0xlt commented at 9:07 PM on September 23, 2026:

Thanks. Done.

in src/net_processing.cpp:4137 in 0e211e593b

4132 | +        return;
4133 | +    }
4134 | +
4135 | +    if (!m_chainman.m_blockman.LookupBlockIndex(blockhash)) {
4136 | +        received_new_header = true;
4137 | +    }

l0rinc commented at 1:33 AM on September 23, 2026:

0e211e5 move-only: Extract ProcessCompactBlock() helper:

indentation is weird here, in a cleanup commit at the end we should normalize these

w0xlt commented at 9:07 PM on September 23, 2026:

Thanks. Done.

in src/net_processing.cpp:1915 in ace762905f

1910 | +    // inbound, feelers, or outbound block-relay-only peers.
1911 | +    bool send_getaddr{false};
1912 | +    if (!pfrom.IsInboundConn()) {
1913 | +        send_getaddr = SetupAddressRelay(pfrom, peer);
1914 | +    }
1915 | +    if (send_getaddr) {

l0rinc commented at 1:42 AM on September 23, 2026:

ace7629 move-only: Extract ProcessVersion() helper:

Seems to me this is basically just:

    if (!pfrom.IsInboundConn() && SetupAddressRelay(pfrom, peer)) {

w0xlt commented at 9:07 PM on September 23, 2026:

Thanks. Done.

l0rinc approved

l0rinc commented at 2:48 AM on September 23, 2026: contributor

tested ACK ace762905f6f481d2b80f8b2613608908758b314

Rebased and reimplemented the extractions, and generated missing test coverage locally to test that the before/after behavior doesn't change. I also compared the implementation with this series and reviewed each commit using --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space. The change looks good to me. Thanks for this cleanup.

My inline suggestions are nonblocking, but since the extractions are straightforward, I think one or two cleanup and formatting commits after the move-only series would be reasonable.

The PR description also needs some updates: it still states "Seven handlers" although there are nine, omits ProcessCompactBlock() and ProcessVersion() from the list, and has a double comma after ProcessAddrs().

DrahtBot requested review from pseudoramdom on Sep 23, 2026

DrahtBot requested review from pablomartin4btc on Sep 23, 2026

w0xlt force-pushed on Sep 23, 2026

move-only: Extract ProcessGetDataMessage() helper

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

57c32028df

move-only: Extract ProcessGetBlocks() helper

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

116b45b5c8

move-only: Extract ProcessGetHeaders() helper

The __func__ prefix in the debug log for ignored old block header
requests changes from ProcessMessage to ProcessGetHeaders.

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

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

01c65dd796

move-only: Extract ProcessInv() helper

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

31a5f7c493

move-only: Extract ProcessSendTxRcncl() helper

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

0b80c046f9

move-only: Extract ProcessTx() helper

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

b66c398623

move-only: Extract ProcessCompactBlock() helper

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

Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

5f71554b39

move-only: Extract ProcessVersion() helper

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

Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com>

dabe1c4d77

refactor: Clean up extracted message handlers

Normalize brace placement, pointer and reference spacing, blank lines,
and single-statement conditionals in the extracted handlers.

Use returns in ProcessSendTxRcncl() and remove the send_getaddr temporary
in ProcessVersion(), preserving the existing control flow.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

cf0c89d6dc

w0xlt force-pushed on Sep 23, 2026

DrahtBot added the label CI failed on Sep 23, 2026

DrahtBot removed the label CI failed on Sep 23, 2026

l0rinc commented at 4:13 AM on September 24, 2026: contributor

retested ACK cf0c89d6dc895ea0323e7680d6fe6c394fe25288

This remains a straightforward refactor. Since my last ACK, the changes are mostly minimal reformatting and the equivalent break→return and send_getaddr cleanups as suggested.

pablomartin4btc commented at 3:06 PM on September 24, 2026: member

ACK cf0c89d6dc895ea0323e7680d6fe6c394fe25288

Thanks for taking my suggestion to also extract ProcessCompactBlock() and ProcessVersion().

Also, since my last review, feedback from @l0rinc has been addressed and a new cleanup commit has been added.