refactor: [p2p] Make ProcessMessage private again, Use references when non-null

maflcko commented at 12:31 pm on December 30, 2025: member

There is a single unit test, which calls the internal ProcessMessage function. This is problematic, because it makes future changes harder, since they will need to carry over this public internal interface each time.

Also, there is a mixed use of pointers and references in p2p code, where just based on context, a pointer may sometimes assumed to be null, or non-null. This is confusing when reading the code, or making or reading future changes.

Fix both issues in a series of commits, to:

refactor the single unit test to call higher-level functions
Make ProcessMessage private again
Use references instead of implicit non-null pointers, mostly in a scripted-diff

DrahtBot renamed this:
~~refactor: [p2p] Make ProcessMessage private again, Use references when non-null~~
refactor: [p2p] Make ProcessMessage private again, Use references when non-null
on Dec 30, 2025

DrahtBot added the label Refactoring on Dec 30, 2025

DrahtBot commented at 12:31 pm on December 30, 2025: 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/34181.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, ajtowns, Crypt-iQ, achow101
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34444 (refactor: rename remaining BIP14 subversion identifiers to user agent by l0rinc)
#34389 (net/log: standardize peer+addr log formatting via LogPeer by l0rinc)
#34293 (Bugfix: net_processing: Restore missing comma between peer and peeraddr in “receive version message” and “New ___ peer connected” by luke-jr)
#33854 (fix assumevalid is ignored during reindex by Eunovo)
#32430 (test: Add and use NodeClockContext by maflcko)
#32278 (doc: better document NetEventsInterface and the deletion of “CNode"s by vasild)
#30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

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.

DrahtBot added the label CI failed on Dec 30, 2025

DrahtBot removed the label CI failed on Dec 30, 2025

bensig commented at 10:56 pm on January 3, 2026: contributor

ACK fa4dbf1b299dd7bf0fa4b0d5f713d5a393215cdc

Build and tests pass (denialofservice_tests, p2p_ping.py).

Clean refactor - using references instead of implicit non-null pointers makes the code clearer… and of course, ProcessMessage is private again.

optout21 commented at 1:51 pm on January 9, 2026: contributor

reACK fa43897c1d14549e7af0d9f912e765875b634c39 Re-review following rebase.

Prev: reACK fa1f90938e1a99139e00731ae1edf29e2c3635a9 reACK faba163fc38085d9d25b4b6df1d8c3abe4663aeb utACK fa4dbf1b299dd7bf0fa4b0d5f713d5a393215cdc The single external usage of a method is removed by changing a test, the method is changed to private. Some additional limited-scope refactors.

code reviewed
verified the diff script to produce the same result
checked unit tests locally

DrahtBot added the label Needs rebase on Jan 12, 2026

maflcko force-pushed on Jan 13, 2026

DrahtBot removed the label Needs rebase on Jan 13, 2026

DrahtBot added the label Needs rebase on Jan 19, 2026

maflcko force-pushed on Jan 19, 2026

DrahtBot removed the label Needs rebase on Jan 19, 2026

maflcko force-pushed on Jan 19, 2026

DrahtBot added the label Needs rebase on Jan 20, 2026

maflcko force-pushed on Jan 20, 2026

DrahtBot removed the label Needs rebase on Jan 20, 2026

DrahtBot added the label Needs rebase on Jan 27, 2026

test: [refactor] Avoid calling private ProcessMessage() function

Calling this low-level function from tests is confusing, and also makes
it harder to change the peer manager implementation.

So juse use the pre-existing test helpers to achieve the same.

fa80cd3cee

refactor: Make ProcessMessage private again

It is not used in tests anymore.

fada838014

refactor: Pass CNode& to ProcessMessages and SendMessages

The node is never nullptr.

This can be reviewed with the git option:
--word-diff-regex=.

fa376095a0

refactor: Pass Peer& to ProcessMessage

The peer is never nullptr.

fac529188e

refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages

Introducing two names to refer to the peer makes it possible to have one
refer to a non-null reference in a later commit.

fac5415466

scripted-diff: Use references when nullptr is not possible

This allows to skip nullptr checks later in the code, both mentally and literally.

This can be reviewed via the git option:
--word-diff-regex=.

-BEGIN VERIFY SCRIPT-

   sed --regexp-extended --in-place '
      /^bool PeerManagerImpl::SendMessages\(/,/^}$/   {
         s/auto& peer\{maybe_peer\}; .. alias cleaned up .*/Peer\& peer{*maybe_peer};/;
         s/peer->/peer./g;
         s/\*peer\>/peer/g;

         /CNode\* pto\{&node\}; .. alias removed .*/d;
         s/pto->/node./g;
         s/\*pto\>/node/g;
      }
   ' src/net_processing.cpp

   sed --regexp-extended --in-place '
      /^void PeerManagerImpl::ProcessMessage\(/,/^}$/   {
         /Peer\* peer\{&peer_alias_removed_in_later_commit};/d;
         s/peer_alias_removed_in_later_commit/peer/;
         s/peer->/peer./g;
         s/\*peer\>/peer/g;
      }
   ' src/net_processing.cpp

   sed --regexp-extended --in-place '
      /^bool PeerManagerImpl::ProcessMessages\(/,/^}$/   {
         s/auto& peer\{maybe_peer\}; .. alias cleaned up .*/Peer\& peer{*maybe_peer};/;
         s/peer->/peer./g;
         s/\*peer\>/peer/g;

         /CNode\* pfrom\{&node\}; .. alias removed .*/d;
         s/pfrom->/node./g;
         s/\*pfrom\>/node/g;
      }
   ' src/net_processing.cpp

-END VERIFY SCRIPT-

bbbba0fd4b

doc: Fix LLM nits in net_processing.cpp

Fix a typo and use a named arg, where the LLM suggested it.

fa43897c1d

maflcko force-pushed on Jan 27, 2026

DrahtBot removed the label Needs rebase on Jan 27, 2026

maflcko commented at 4:06 pm on January 27, 2026: member

rebased, should be trivial to re-review via git range-diff

ajtowns commented at 6:09 pm on February 6, 2026: contributor

ACK fa43897c1d14549e7af0d9f912e765875b634c39

Crypt-iQ commented at 6:22 pm on February 6, 2026: contributor

crACK fa43897c1d14549e7af0d9f912e765875b634c39

achow101 commented at 1:02 am on February 7, 2026: member

ACK fa43897c1d14549e7af0d9f912e765875b634c39

achow101 merged this on Feb 7, 2026

achow101 closed this on Feb 7, 2026

maflcko deleted the branch on Feb 9, 2026

refactor: [p2p] Make ProcessMessage private again, Use references when non-null #34181

Code Coverage & Benchmarks

Reviews

Conflicts