#34271 net_processing: make m_tx_for_private_broadcast optional
https://github.com/bitcoin/bitcoin/pull/34271 · · +91/-40 in 2 files, 1 commits · labels: Needs rebase, Private Broadcast · draft
Goal
- Avoid holding idle private broadcast state when the feature is disabled
This pull request wraps `PeerManagerImpl::m_tx_for_private_broadcast` in `std::optional`, initializing it only when `-privatebroadcast=1` is configured. It adds guards across private broadcast call sites in `net_processing` to disconnect or ignore requests if invoked when disengaged.
Problem: When private broadcast is disabled, `PeerManagerImpl` still instantiates and holds an idle `PrivateBroadcast` object. However, `std::optional` retains inline storage regardless, providing minimal memory benefit while introducing defensive checks for theoretically unreachable states.
Category: P2P (#65 of 65)
P4 · cleanup
- P4 because it is a minor internal cleanup with no user-facing impact or performance gains
- Memory usage is not reduced and it adds defensive branches for unreachable states
P4 because the change is a minor internal refactoring without user-facing impact, performance benefit, or memory savings. Because std::optional reserves inline storage, it does not reduce PeerManagerImpl memory footprint, while adding defensive error branches that the author noted add 'visual clutter and unreachable code'.
Membership: Touches PeerManagerImpl in src/net_processing.cpp and carries the Private Broadcast label.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0
Reviewability: Stale: Needs rebase
- Needs rebase and has merge conflicts
- Author has been inactive for over 60 days
The PR has merge conflicts with master, has the 'Needs rebase' label, and the author has been inactive for over 60 days.
Author status: Silent for over 60 days; expressed neutrality (~0) on whether the change is net-positive.
Open concerns:
- Making the member optional requires defensive runtime checks or assertions in private broadcast methods that should never be reachable if disabled.
- std::optional preserves inline storage within PeerManagerImpl, so it does not reduce object size without pointer indirection.
- Decoy transaction proposals and other private broadcast followups may move private broadcast into core functionality rather than keeping it strictly opt-in.
Resolved concerns:
- Deferred initialization was moved from the first call of InitiateTxBroadcastPrivate to the PeerManagerImpl constructor based on configuration options.
- Handled silent conflicts where newly added private broadcast methods needed optional checks.
Agreement: Mild
- Concept and approach support, but questioned if worth the visual clutter (vasild)
- Does not reduce memory footprint without pointer indirection (l0rinc)
- Future private broadcast plans may make opt-in handling obsolete (mzumsande)
- Moved to draft pending direction on private broadcast (fanquake)
Mild: reviewers supported the concept, but the author and others question whether it is worth the visual clutter and whether future private broadcast plans make it obsolete.
Reviewers offered Concept and Approach ACKs, but mzumsande and the author noted architectural reservations about whether private broadcast remains an isolated opt-in feature, and the author noted that wrapping it in std::optional creates clutter and defensive branches for impossible states.
- vasild noted: 'I am ~0 on making m_tx_for_private_broadcast optional... it adds visual clutter and unreachable code.'
- l0rinc noted: 'Note that this still reserves storage for a PrivateBroadcast inside every PeerManagerImpl... avoiding the object-size footprint too would require indirection'
- mzumsande noted: 'If in the future even nodes that don't opt into private broadcast... 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.'
- fanquake moved to draft: 'given it needs rebase, and there needs to be some decisions made on general private-broadcast direction.'
Review verdicts (DrahtBot): 0 (+1)
- Stale ACK: optout21
- Approach ACK: l0rinc
- Concept ACK: polespinasa, w0xlt
Files
File list not available for this run.
Card
This PR modifies PeerManagerImpl in net_processing to make m_tx_for_private_broadcast an std::optional, instantiating it only when private broadcast mode is active. It addresses a post-merge review comment from #29415 seeking cleaner separation of opt-in private broadcast state. Because std::optional retains inline storage, the change provides no memory footprint savings and adds defensive checks for unreachable states, leading both the author and reviewers to question its utility. The PR is currently marked draft and needs a rebase while broader architectural decisions on private broadcast evolve.