net_processing: make m_tx_for_private_broadcast optional by vasild · Pull Request #34271 · bitcoin/bitcoin

vasild · 2026-01-13T11:32:27Z

Make PeerManagerImpl::m_tx_for_private_broadcast optional because it is needed for in private broadcast mode (-privatebroadcast=1).

A followup to #29415, requested in:
#29415 (comment)
https://github.com/bitcoin/bitcoin/pull/29415/files#r2620864832

DrahtBot · 2026-01-13T11:32:33Z

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/34271.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	optout21, polespinasa, w0xlt

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34707 (net: keep finished private broadcast txs in memory, rebroadcast if evicted from mempool by andrewtoth)
#34628 (p2p: Replace per-peer transaction rate-limiting with global rate limits by ajtowns)
#34389 (net/log: standardize peer+addr log formatting via LogPeer by l0rinc)

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.

vasild · 2026-01-13T11:40:18Z

src/net_processing.cpp


-        if (pfrom.IsPrivateBroadcastConn()) {
-            const auto pushed_tx_opt{m_tx_for_private_broadcast.GetTxForNode(pfrom.GetId())};
+        if (pfrom.IsPrivateBroadcastConn() && m_tx_for_private_broadcast.has_value()) {


This change creates a situation that should be impossible: to have a private broadcast connection existent and have the optional m_tx_for_private_broadcast empty. Anyway it has to be handled somehow in the code and I think that maybe an assert() or Assume() is too strong? So here it would behave as if it is not a private broadcast connection. Maybe this is not the correct behavior?

I am ~0 on making m_tx_for_private_broadcast optional. In other words, it has some pros and some cons, I am fine either way.

That can happen if InitiateTxBroadcastPrivate() was not called before. That could happen even without this change, and the behavior is very similar: m_tx_for_private_broadcast was initialized with an 'empty' PrivateBroadcast, so the PickTxForSend(), GetStale(), etc. methods would return nothing. However, the difference is that in that case LogDebug's are emitted. An assert might be too strong, as there is no guarantee that InitiateTxBroadcastPrivate() is called upfront, but the LogDebug messages should be emitted the same way if m_tx_for_private_broadcast none, or it is set but does not contain the expected transaction.
Another way would be to lazily init m_tx_for_private_broadcast before every access, but that may be an overkill.

That can happen if InitiateTxBroadcastPrivate() was not called before.

Ok, but how could pfrom.IsPrivateBroadcastConn() be true if InitiateTxBroadcastPrivate() was not called before?

I can't answer that, maybe it can't, but there is no straightforward guarantee in the code; one is a check in CNode, one is a call in PeerManagerImpl. Regardless whether it is possible (now) or not, I'm saying that the behavior regarding logging should be the same after this change than before (even if it is there is no way to get that combination today, there may be some (incorrect) change in the future, so keeping the log is a good idea).

Yeah, I changed it to disconnect the peer if that ever happens in the future.

optout21 · 2026-01-13T13:23:19Z

src/net_processing.cpp


-        if (pfrom.IsPrivateBroadcastConn()) {
-            const auto pushed_tx_opt{m_tx_for_private_broadcast.GetTxForNode(pfrom.GetId())};
+        if (pfrom.IsPrivateBroadcastConn() && m_tx_for_private_broadcast.has_value()) {


How about this approach to have the same LogDebug for the case when m_tx_for_private_broadcast is unset and there is not TX?

if (pfrom.IsPrivateBroadcastConn()) { const auto pushed_tx_opt{!m_tx_for_private_broadcast.has_value() ? std::nullopt : m_tx_for_private_broadcast->GetTxForNode(pfrom.GetId())}; if (!pushed_tx_opt) { LogInfo( ... ... } ... }

Added some logs and disconnect the peer that has IsPrivateBroadcastConn() true.

optout21 · 2026-01-13T13:30:56Z

ConceptACK
Review code; verified that all occurence of m_tx_for_private_broadcast is touched, verified building and unit tests locally; left some comments.

vasild · 2026-01-13T15:53:36Z

340d10d26d0163201045594b30d96edd544a2f93...c218ad88764601814204d65ce21c06c851f04014: disconnect the peer if private broadcast storage is not initialized and log those events that cannot happen with the current code and must not happen in the future either.

polespinasa

Concept ACK

I don't understand why we check if m_tx_for_private_broadcast has value inside functions that imply that we are in private broadcast mode.

polespinasa · 2026-01-14T17:07:37Z

src/net_processing.cpp

-                        stale_tx->GetHash().ToString(), stale_tx->GetWitnessHash().ToString(),
-                        mempool_acceptable.m_state.ToString());
-                m_tx_for_private_broadcast.Remove(stale_tx);
+    if (m_tx_for_private_broadcast.has_value()) {


is this check here necessary? ReattemptPrivateBroadcast is only called in StartScheduledTasks where it is only called if private broadcast option is enabled.

I mean if I understood correctly we should never get into this function if private broadcast mode is not set.

Same answer to the question in the main-thread: #34271 (review) and the question from #34271 (comment)

If we don't check whether the optional has value and try to access it and it does not have a value then the program will crash (it is undefined behavior). While this shouldn't be possible in the current code nothing guarantees that future changes will keep it that way. It is not immediately obvious by reading the surrounding code. So, to be more robust, it is better to have some handling of it.

This is the downside of this change any why I am ~0 about it - it adds visual clutter and unreachable code.

polespinasa · 2026-01-14T17:13:48Z

src/net_processing.cpp

    if (node.IsPrivateBroadcastConn() &&
-        !m_tx_for_private_broadcast.DidNodeConfirmReception(nodeid) &&
-        m_tx_for_private_broadcast.HavePendingTransactions()) {
+        m_tx_for_private_broadcast.has_value() &&


Is there a case where we can have node.IsPrivateBroadcastConn() = true and m_tx_for_private_broadcast.has_value() = false? If that's the case maybe we want to close that connection as you did in PushPrivateBroadcastTx or ProcessMessage? (c218ad8#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R3542)

Edit: I see that this was already discussed here #34271 (comment) maybe want to do the same?
(I'm not really familiar with the net processing code maybe I am missing something :) )

Is there a case where we can have node.IsPrivateBroadcastConn() = true and m_tx_for_private_broadcast.has_value() = false

Currently "no", but it is not immediately obvious and not guaranteed to remain like this after future changes.

This code here is during the connection tear down. It is being disconnected anyway already.

w0xlt

Appraoch ACK

vasild · 2026-02-05T11:55:57Z

c218ad88764601814204d65ce21c06c851f04014...e1600e02a8c46b97efd47e5e6d38cf068483216f: rebase due to conflicts

Make `PeerManagerImpl::m_tx_for_private_broadcast` optional because it is needed for in private broadcast mode (`-privatebroadcast=1`). Requested in: bitcoin#29415 (comment) https://github.com/bitcoin/bitcoin/pull/29415/files#r2620864832

vasild · 2026-02-25T05:01:46Z

e1600e02a8c46b97efd47e5e6d38cf068483216f...41f0bc0332ff6942809812bbc76cc5fc38e9389e: rebase due to conflicts

DrahtBot · 2026-03-11T13:47:26Z

🐙 This pull request conflicts with the target branch and needs rebase.

vasild commented Jan 13, 2026

View reviewed changes

optout21 reviewed Jan 13, 2026

View reviewed changes

vasild force-pushed the optional_m_tx_for_private_broadcast branch from 340d10d to c218ad8 Compare January 13, 2026 15:51

DrahtBot mentioned this pull request Jan 13, 2026

net: avoid unconditional privatebroadcast logging (+ warn for debug logs) #34267

Merged

vasild mentioned this pull request Jan 14, 2026

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

4 tasks

polespinasa reviewed Jan 14, 2026

View reviewed changes

w0xlt reviewed Jan 14, 2026

View reviewed changes

This was referenced Jan 18, 2026

kernel, refactor: return error status on all fatal errors #29700

Draft

net/log: standardize peer+addr log formatting via LogPeer #34389

Merged

DrahtBot mentioned this pull request Jan 26, 2026

rpc,net: Add private broadcast RPCs #34329

Merged

DrahtBot added the Needs rebase label Jan 27, 2026

vasild mentioned this pull request Feb 2, 2026

Private broadcast tracking #34476

Open

13 tasks

vasild force-pushed the optional_m_tx_for_private_broadcast branch from c218ad8 to e1600e0 Compare February 5, 2026 11:55

DrahtBot removed the Needs rebase label Feb 5, 2026

DrahtBot added the Needs rebase label Feb 19, 2026

vasild force-pushed the optional_m_tx_for_private_broadcast branch from e1600e0 to 41f0bc0 Compare February 25, 2026 05:01

DrahtBot mentioned this pull request Feb 25, 2026

p2p: Replace per-peer transaction rate-limiting with global rate limits #34628

Open

DrahtBot removed the Needs rebase label Feb 25, 2026

DrahtBot mentioned this pull request Mar 2, 2026

net: keep finished private broadcast txs in memory, rebroadcast if evicted from mempool #34707

Open

DrahtBot added the Needs rebase label Mar 11, 2026

Conversation

vasild commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 commented Jan 13, 2026

Uh oh!

vasild commented Jan 13, 2026

Uh oh!

polespinasa left a comment

Choose a reason for hiding this comment

Uh oh!

polespinasa Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polespinasa Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Feb 5, 2026

Uh oh!

vasild commented Feb 25, 2026

Uh oh!

DrahtBot commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vasild commented Jan 13, 2026 •

edited

Loading

DrahtBot commented Jan 13, 2026 •

edited

Loading

optout21 Jan 13, 2026 •

edited

Loading

polespinasa Jan 14, 2026 •

edited

Loading

polespinasa Jan 14, 2026 •

edited

Loading