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

ReviewabilityReviewsAgreementSize
#35522 pablomartin4btc refactor: Extract per-message helpers from SendMessages() (move-only)Ready0 (+1)Strong679
  • Make peer-to-peer message sending logic easier to navigate, review, and analyze for thread safety
  • Break apart monolithic message dispatching and timeout handling into discrete, named steps
  • Ready to review
  • Substantive support with no open objections (w0xlt, hodlinator)
  • Approach approval noting it follows a proven cleanup pattern (w0xlt)
  • Concept approval because named helpers make code navigation and review easier (hodlinator)
  • Concerns resolved regarding PR description policy and debug log formatting (maflcko)
  • 679 lines added or modified outside tests
  • 0 lines added or modified in tests
  • 453 lines removed in total
  • 1 files, 19 commits
P2P#67 of 71
P4 · cleanup
  • P4 because it is an internal move-only refactoring of message dispatching logic
  • Brings no behavioral, protocol, or performance changes
  • Aids maintainers navigating the code rather than addressing an active P2P issue
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Extracts 17 helper functions from PeerManagerImpl::SendMessages() in src/net_processing.cpp, reducing the main P2P message dispatch function from roughly 500 lines to under 90 lines. Message generation logic, stall and timeout checks, getdata queues, and transaction trickling are separated into discrete subroutines across move-only commits. The PR also removes manual __func__ prefixes from four debug logs and standardizes helper naming and conditional formatting.

Problem: PeerManagerImpl::SendMessages() is a monolithic ~500-line function handling all P2P message dispatching and timeout logic inline. Interleaved lock scopes and send conditions make the function difficult to navigate, analyze for thread safety, and review.

Evidence of importance:

  • PR description: '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.'
  • Author note: Identified as an improvement while reviewing #34824 (net: encapsulate TxRelay state and replace recursive mutexes).

Scope: All changes are in src/net_processing.cpp across 19 commits; 17 commits are pure code moves structured to be verified with git --color-moved.

Risk of a mistake: medium (p2p). A mistake in extracted message dispatch helpers, lock acquisitions, or timeout checks could disrupt block sync or transaction relay for connected peers.

Changed since the previous assessment: Rebased over upstream changes, incorporating the ping timeout check into the extracted timeout helpers, and added co-authorship credits.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: reviewers to review the updated PR and new prepare commit 2026-10-19

Agreement

Strong

substantive support, no open objection (w0xlt, hodlinator)

Thread notes: Author pushed an update adding a preparatory commit to remove __func__ prefixes from SendMessages() debug logs and revised the PR description to remove processing-side references and address AI policy concerns.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
w0xlt (contributor)objection, correctnessmisleading comment regarding lock requirements for MaybeSendTxMessagesfixed · fix pushed · author fixed the comment in rc:3414210019 and c:4709320065answeredyes2026-06-14: 'MaybeSendTxMessages acquires tx_relay lock internally. It only requires cs_main and g_msgproc_mutex held.'
Status from: 2026-06-15: pablomartin4btc: 'Done. Thanks!'
w0xlt (contributor)suggestion, maintenanceadopt optional code cleanups and adjusted commit reorganization prepared on a local rebase branchfixed · fix pushed · author adopted the cleanups and reorganized commits in the rebaseansweredyes2026-09-24: 'I tried a few non-blocking cleanups locally, similar to those in #35502'
Status from: 2026-10-08: pablomartin4btc: 'Addressed @w0xlt's feedback adding'
maflcko (member)objection, maintenancePR description violates the project AI policy due to suspected use of LLM-generated textfixed · fix pushed · author updated the PR description addressing the feedbackansweredyes2026-10-13: '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'
Status from: 2026-10-19: pablomartin4btc: 'Addressed @maflcko's feedback.'
maflcko (member)suggestion, approachavoid using __func__ in logs and make each log line unique and descriptive in a preparatory commitfixed · fix pushed · author added a prepare commit removing the __func__ prefix from debug logsansweredyes2026-10-13: '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?'
Status from: 2026-10-19: pablomartin4btc: 'Created a new prepare commit removing `__func__` prefix from the debug logs in `SendMessages()`.'
maflcko (member)suggestion, maintenanceremove references to processing-side PRs and commits from the PR descriptionfixed · fix pushed · author removed processing side references from the PR descriptionansweredyes2026-10-13: 'I think all of those mentions can be removed, as they are about the processing side, not the sending side.'
Status from: 2026-10-19: pablomartin4btc: 'Removed misleading processing side references from the PR description.'

Support:

  • w0xlt (contributor) (Approach ACK, 2026-06-14) [read]: same pattern as https://github.com/bitcoin/bitcoin/pull/35502.
  • hodlinator (member) (Concept ACK, 2026-09-15) [read: SendMessages()]: 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 (contributor) (ACK, 2026-10-08): (no reason given) [not substantive]

Participants:

  • w0xlt (contributor, support, 5 statements 2026-06-14 to 2026-10-08): Approach ACK and ACK after providing cleanup suggestions and commit reorganization
  • hodlinator (member, support, 1 statements 2026-09-15 to 2026-09-15): Concept ACK; noted it is helpful to refer to parts of SendMessages() by name
  • maflcko (member, objection, 1 statements 2026-10-13 to 2026-10-13): objected to suspected LLM-generated PR description citing AI policy, and suggested cleaning up log lines and removing unrelated PR references

Files

0 lines under test/bench/ci.

  • src/net_processing.cpp +679/-453

Card

The short text the ranking pass compares across PRs.

This PR refactors PeerManagerImpl::SendMessages() in src/net_processing.cpp by extracting 17 helper functions across move-only commits, reducing the main dispatch loop from ~500 lines to under 90 lines. It addresses maintainability and review difficulty in the P2P message sending loop, an issue surfaced during review of #34824. The refactor removes redundant __func__ prefixes from debug log lines but makes no intentional behavioral changes. No dependencies are declared.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-21T01:17, confidence high, input hash af74e8105def858f
thread read through 2026-10-19 (15 statements), head 4d707f65bd
labels: Refactoring

Categories

P2P

P4 · cleanup

Extracts 17 helper functions from SendMessages() to improve code readability and maintainability. The PR is an internal move-only refactoring without behavioral, protocol, or performance impact, addressing code navigation rather than an active P2P hazard.

Membership: Modifies PeerManagerImpl::SendMessages in src/net_processing.cpp, restructuring message dispatching logic.

Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1