#34271 net_processing: make m_tx_for_private_broadcast optional

ReviewabilityReviewsAgreementSize
#34271 vasild net_processing: make m_tx_for_private_broadcast optionalWaiting on author0 (+1)Mild91
  • Avoid constructing unused private broadcast state when the opt-in feature is disabled
  • Cleans up default node startup and memory usage when private broadcast is turned off
  • Needs rebase
  • Waiting on author to address an open objection before further review is worthwhile
  • Unaddressed objection: does not reduce object size without indirection, no author reply (l0rinc)
  • Questioned whether optional storage conflicts with making private broadcast core (mzumsande)
  • Verified by checking all touched call sites and running unit tests locally (optout21)
  • Concept approval without stated reasons (polespinasa, w0xlt)
  • 91 lines added or modified outside tests
  • 0 lines added or modified in tests
  • 40 lines removed in total
  • 2 files, 1 commits
P2P#68 of 71
P4 · cleanup
  • P4 because it is a minor internal cleanup with negligible runtime impact on default nodes
  • Benefits only default nodes by skipping idle initialization for an opt-in feature
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

Make `PeerManagerImpl::m_tx_for_private_broadcast` an `std::optional<PrivateBroadcast>`, instantiating it only when private broadcast mode is enabled (`-privatebroadcast=1`). Guard all private broadcast access paths in `net_processing` to handle uninitialized state, disconnecting peers or returning early if private broadcast methods or messages are reached while disabled.

Problem: When `-privatebroadcast` is disabled (the default), `PeerManagerImpl` still constructs and retains an idle `PrivateBroadcast` object in memory throughout the lifetime of the node.

Evidence of importance:

  • PR description: 'Make `PeerManagerImpl::m_tx_for_private_broadcast` optional because it is needed only in private broadcast mode (`-privatebroadcast=1`). A followup to #29415, requested in: https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2620864832'

Risk of a mistake: medium (p2p). A missed null check or incorrect conditional branch during P2P message processing could trigger a std::bad_optional_access crash or inadvertently disconnect peers.

Changed since the previous assessment: No substantial changes to the patch; previous description already reflected this commit.

Reviewability

Paused · Waiting on author

Open objection from l0rinc with no author reply for 90 days; review would repeat what has been asked.

Agreement

Mild

objection that needs an answer has none yet (l0rinc)

Thread notes: Initial implementation of optional private broadcast storage was reviewed and received support, but l0rinc requested initialization be moved to the constructor to avoid creating state on direct calls when disabled, which the author resolved in a push. mzumsande raised an open question on whether this refactoring is desirable given broader private-broadcast architectural proposals, prompting fanquake to move the PR to draft.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
l0rinc (contributor)objection, correctnessDirect calls to InitiateTxBroadcastPrivate could instantiate private-broadcast state even when the peer manager was configured with private broadcast disabledfixed · fix pushed · v1, not yet re-checkedchangedyes2026-05-15: 'my main concern is that the optional is currently initialized from InitiateTxBroadcastPrivate() instead of from the peer manager’s private-broadcast option.'
Status from: 2026-06-30: vasild: 'Moved the initialization to PeerManagerImpl::PeerManagerImpl().'
l0rinc (contributor)objection, approachstd::optional preserves inline storage for PrivateBroadcast inside PeerManagerImpl and does not reduce object size without indirectionopen · v1, not yet re-checkedansweredno2026-05-15: 'Note that this still reserves storage for a PrivateBroadcast inside every PeerManagerImpl, even when private broadcast is disabled. This only skips construction and active state - avoiding the object-size footprint too would require indirection'
mzumsande (contributor)objection, approachMaking private broadcast storage optional conflicts with proposals that make private broadcast core functionality rather than strictly opt-inanswered · reply on record; standing not assessed yet · v1, not yet re-checkedansweredyes2026-06-24: 'If in the future even nodes that don't opt into private broadcast (-privatebroadcast=0) would send private transactions, then this would move private broadcast into core functionality, and making m_tx_for_private_broadcast optional wouldn't make sense.'

Support:

  • optout21 (contributor) (Concept ACK, 2026-01-13): Review code; verified that all occurence of m_tx_for_private_broadcast is touched, verified building and unit tests locally
  • polespinasa (member) (Concept ACK, 2026-01-14): Concept ACK [not substantive]
  • w0xlt (contributor) (Approach ACK, 2026-01-14): Appraoch ACK [not substantive]
  • optout21 (contributor) (ACK, 2026-05-15): Review code; verified that all occurrences of m_tx_for_private_broadcast are touched, verified building and unit tests locally.
  • l0rinc (contributor) (Approach ACK, 2026-05-15): Making the private-broadcast transaction state optional is a useful cleanup for the default net-processing path

Participants:

  • optout21 (contributor, support, 8 statements 2026-01-13 to 2026-05-15): Concept ACK and full ACK after reviewing code and testing locally
  • polespinasa (member, support, 3 statements 2026-01-14 to 2026-01-14): Concept ACK; asked about the necessity of defensive optional checks in private-broadcast-specific functions
  • w0xlt (contributor, support, 1 statements 2026-01-14 to 2026-01-14): Approach ACK
  • l0rinc (contributor, objection, 5 statements 2026-05-15 to 2026-05-15): Approach ACK with changes requested regarding initialization timing; noted std::optional preserves inline storage footprint
  • mzumsande (contributor, objection, 1 statements 2026-06-24 to 2026-06-24): Questioned whether making the member optional makes sense if private broadcast moves into core functionality
  • fanquake (member, neutral, 1 statements 2026-08-03 to 2026-08-03): Moved PR to draft pending rebase and architectural direction on private broadcast

Dependencies

Depends on: #29415

Files

0 lines under test/bench/ci.

  • src/net_processing.cpp +82/-39
  • src/net_processing.h +9/-1

Card

The short text the ranking pass compares across PRs.

This PR makes PeerManagerImpl's private broadcast tracker an std::optional, constructing it only when running in private broadcast mode (-privatebroadcast=1). It addresses review feedback from the merged private broadcast PR #29415 to avoid constructing unused subsystem state by default. Because std::optional retains inline storage space on the class, memory savings are minimal while requiring defensive checks across P2P message handling paths. PR #29415 is already merged.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-25T00:41, confidence high, input hash a7bf2d31388c5050
thread read through 2026-08-03 (37 statements), head 14a241f3b3
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Needs rebase, Private Broadcast · draft

Categories

P2P

P4 · cleanup

Avoids instantiating an idle PrivateBroadcast struct when -privatebroadcast is disabled. This is a minor internal followup cleanup to #29415 with negligible runtime impact on default nodes.

Membership: Changes PeerManagerImpl in src/net_processing.cpp and src/net_processing.h under the 'Private Broadcast' label.

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