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
Stale ACK	bensig, optout21

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:

#34059 (refactor: Use NodeClock::time_point for m_addr_token_timestamp by maflcko)
#33854 (fix assumevalid is ignored during reindex by Eunovo)
#32278 (doc: better document NetEventsInterface and the deletion of “CNode"s by vasild)
#31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections 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

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

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.

fa489fdac1

refactor: Make ProcessMessage private again

It is not used in tests anymore.

fab1f907ed

refactor: Pass CNode& to ProcessMessages and SendMessages

The node is never nullptr.

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

fa9879aa6b

refactor: Pass Peer& to ProcessMessage

The peer is never nullptr.

fa4adfbd5a

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.

fa1e3f9810

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-

fa3b1f0d76

doc: Fix typo found by LLM in net_processing.cpp faba163fc3

maflcko force-pushed on Jan 13, 2026

DrahtBot removed the label Needs rebase on Jan 13, 2026

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

Code Coverage & Benchmarks

Reviews

Conflicts