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
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:

#34293 (Bugfix: net_processing: Restore missing comma between peer and peeraddr in “receive version message” by luke-jr)
#34267 (net: avoid unconditional privatebroadcast logging (+ warn for debug logs) by l0rinc)
#33854 (fix assumevalid is ignored during reindex by Eunovo)
#32430 (test: Add and use ElapseTime helper by maflcko)
#32278 (doc: better document NetEventsInterface and the deletion of “CNode"s by vasild)
#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 fa1f90938e1a99139e00731ae1edf29e2c3635a9 Re-review following rebase.

Prev: 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

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.

fa932cf8d6

refactor: Make ProcessMessage private again

It is not used in tests anymore.

fa1e4dd1b5

refactor: Pass CNode& to ProcessMessages and SendMessages

The node is never nullptr.

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

fa0cb7f23f

refactor: Pass Peer& to ProcessMessage

The peer is never nullptr.

fa2026dd75

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.

fa6c3acb28

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-

fa66f33930

doc: Fix LLM nits in net_processing.cpp

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

fa1f90938e

maflcko force-pushed on Jan 20, 2026

maflcko commented at 5:55 pm on January 20, 2026: member

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

DrahtBot removed the label Needs rebase on Jan 20, 2026

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

Code Coverage & Benchmarks

Reviews

Conflicts