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 (review) https://github.com/bitcoin/bitcoin/pull/29415/files#r2620864832
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34271.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | optout21 |
| Concept ACK | polespinasa, w0xlt |
| Approach ACK | l0rinc |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
4210 | @@ -4200,8 +4211,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 4211 | LogDebug(BCLog::NET, "received getdata for: %s peer=%d\n", vInv[0].ToString(), pfrom.GetId()); 4212 | } 4213 | 4214 | - if (pfrom.IsPrivateBroadcastConn()) { 4215 | - const auto pushed_tx_opt{m_tx_for_private_broadcast.GetTxForNode(pfrom.GetId())}; 4216 | + 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.
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( ...
...
}
...
}
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).
Added some logs and disconnect the peer that has IsPrivateBroadcastConn() true.
Yeah, I changed it to disconnect the peer if that ever happens in the future.
ConceptACK
Review code; verified that all occurence of m_tx_for_private_broadcast is touched, verified building and unit tests locally; left some comments.
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.
1655 | - } else { 1656 | - LogInfo("[privatebroadcast] Giving up broadcast attempts for txid=%s wtxid=%s: %s", 1657 | - stale_tx->GetHash().ToString(), stale_tx->GetWitnessHash().ToString(), 1658 | - mempool_acceptable.m_state.ToString()); 1659 | - m_tx_for_private_broadcast.Remove(stale_tx); 1660 | + 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#pullrequestreview-3661806909 and the question from #34271 (review)
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.
1738 | @@ -1737,8 +1739,9 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) 1739 | m_headers_presync_stats.erase(nodeid); 1740 | } 1741 | if (node.IsPrivateBroadcastConn() && 1742 | - !m_tx_for_private_broadcast.DidNodeConfirmReception(nodeid) && 1743 | - m_tx_for_private_broadcast.HavePendingTransactions()) { 1744 | + 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? (https://github.com/bitcoin/bitcoin/commit/c218ad88764601814204d65ce21c06c851f04014#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R3542)
Edit: I see that this was already discussed here #34271 (review) 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() = trueandm_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.
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.
Appraoch ACK
c218ad88764601814204d65ce21c06c851f04014...e1600e02a8c46b97efd47e5e6d38cf068483216f: rebase due to conflicts
e1600e02a8c46b97efd47e5e6d38cf068483216f...41f0bc0332ff6942809812bbc76cc5fc38e9389e: rebase due to conflicts
41f0bc0332ff6942809812bbc76cc5fc38e9389e...75259b79443cb1c7ff73af505a5f6a00f655e4d8: rebase due to conflicts
1859 | @@ -1857,19 +1860,19 @@ PeerManagerInfo PeerManagerImpl::GetInfo() const 1860 | 1861 | std::vector<PrivateBroadcast::TxBroadcastInfo> PeerManagerImpl::GetPrivateBroadcastInfo() const 1862 | { 1863 | - return m_tx_for_private_broadcast.GetBroadcastInfo(); 1864 | + return m_tx_for_private_broadcast->GetBroadcastInfo();
Can m_tx_for_private_broadcast be nullopt here?
Yes, indeed. Fixed.
1865 | }
1866 |
1867 | std::vector<CTransactionRef> PeerManagerImpl::AbortPrivateBroadcast(const uint256& id)
1868 | {
1869 | - const auto snapshot{m_tx_for_private_broadcast.GetBroadcastInfo()};
1870 | + const auto snapshot{m_tx_for_private_broadcast->GetBroadcastInfo()};
Can m_tx_for_private_broadcast be nullopt here?
Same as above, fixed. Thanks!
Re-reviewing 75259b79443cb1c7ff73af505a5f6a00f655e4d8 ; left two comments, otherwise LGTM
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/25670945156/job/75355721137</sub>
<sub>LLM reason (✨ experimental): CI failed because the fuzz target rpc reported “Error processing input …” and exited with code 1.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Make `PeerManagerImpl::m_tx_for_private_broadcast` optional because it
is needed for in private broadcast mode (`-privatebroadcast=1`).
Requested in:
https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2620864832
https://github.com/bitcoin/bitcoin/pull/29415/files#r2620864832
75259b79443cb1c7ff73af505a5f6a00f655e4d8...87a42882a94f89eb7d418d3f2f3558fb3bc74a17: fix a silent conflict where new methods were added after this PR was created. Added a guard to them as well. This was the reason for the CI failure.
I wonder if this still makes sense, given the followups to the original PR where this was suggested. @mzumsande?
ACK 87a42882a94f89eb7d418d3f2f3558fb3bc74a17
Review code; verified that all occurrences of m_tx_for_private_broadcast are touched, verified building and unit tests locally.
2275 | @@ -2266,8 +2276,12 @@ void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wt 2276 | 2277 | void PeerManagerImpl::InitiateTxBroadcastPrivate(const CTransactionRef& tx) 2278 | { 2279 | + // Lazily initialize `m_tx_for_private_broadcast` the first time it is needed. 2280 | + if (!m_tx_for_private_broadcast.has_value()) { 2281 | + m_tx_for_private_broadcast.emplace(); 2282 | + } 2283 | const auto txstr{strprintf("txid=%s, wtxid=%s", tx->GetHash().ToString(), tx->GetWitnessHash().ToString())};
Shouldn't we do this in PeerManagerImpl::PeerManagerImpl instead?
If so, would it be possible to cover this case with a test? I'm surprised they all passed.
Here we could log instead:
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 9d0eb588db..23a840daa2 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2026,6 +2026,10 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
m_warnings{warnings},
m_opts{opts}
{
+ if (m_opts.private_broadcast) {
+ m_tx_for_private_broadcast.emplace();
+ }
+
// While Erlay support is incomplete, it must be enabled explicitly via -txreconciliation.
// This argument can go away after Erlay support is complete.
if (opts.reconcile_txs) {
@@ -2276,11 +2280,12 @@ void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wt
void PeerManagerImpl::InitiateTxBroadcastPrivate(const CTransactionRef& tx)
{
- // Lazily initialize `m_tx_for_private_broadcast` the first time it is needed.
+ const auto txstr{strprintf("txid=%s, wtxid=%s", tx->GetHash().ToString(), tx->GetWitnessHash().ToString())};
if (!m_tx_for_private_broadcast.has_value()) {
- m_tx_for_private_broadcast.emplace();
+ LogDebug(BCLog::PRIVBROADCAST, "Ignoring request to privately broadcast transaction because private broadcast is not enabled: %s", txstr);
+ return;
}
- const auto txstr{strprintf("txid=%s, wtxid=%s", tx->GetHash().ToString(), tx->GetWitnessHash().ToString())};
+
if (m_tx_for_private_broadcast->Add(tx)) {
LogDebug(BCLog::PRIVBROADCAST, "Requesting %d new connections due to %s", NUM_PRIVATE_BROADCAST_PER_TX, txstr);
m_connman.m_private_broadcast.NumToOpenAdd(NUM_PRIVATE_BROADCAST_PER_TX);
we could likely verify that a peer manager constructed without private broadcast enabled does not create private-broadcast queue state or request private-broadcast connections when InitiateTxBroadcastPrivate is called directly, something like:
diff --git a/src/test/peerman_tests.cpp b/src/test/peerman_tests.cpp
--- a/src/test/peerman_tests.cpp (revision 419182b0c6b622eee9d19dbd3938c396a494a1ae)
+++ b/src/test/peerman_tests.cpp (revision 22b159e9217b2fbe125e8d6a8fc7baac1359dfc6)
@@ -6,6 +6,7 @@
#include <node/miner.h>
#include <net_processing.h>
#include <pow.h>
+#include <primitives/transaction.h>
#include <test/util/setup_common.h>
#include <validation.h>
@@ -75,4 +76,18 @@
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));
}
+BOOST_AUTO_TEST_CASE(private_broadcast_disabled_ignores_initiate)
+{
+ BOOST_REQUIRE_EQUAL(m_node.connman->m_private_broadcast.NumToOpen(), 0U);
+
+ const auto peerman{PeerManager::make(*m_node.connman, *m_node.addrman, /*banman=*/nullptr, *m_node.chainman, *m_node.mempool, *m_node.warnings, {.private_broadcast = false})};
+ peerman->InitiateTxBroadcastPrivate(MakeTransactionRef(CMutableTransaction{}));
+
+ BOOST_CHECK(peerman->GetPrivateBroadcastInfo().empty());
+ BOOST_CHECK_EQUAL(m_node.connman->m_private_broadcast.NumToOpen(), 0U);
+}
+
BOOST_AUTO_TEST_SUITE_END()
3569 | @@ -3556,7 +3570,15 @@ void PeerManagerImpl::PushPrivateBroadcastTx(CNode& node) 3570 | { 3571 | Assume(node.IsPrivateBroadcastConn()); 3572 | 3573 | - const auto opt_tx{m_tx_for_private_broadcast.PickTxForSend(node.GetId(), CService{node.addr})}; 3574 | + if (!m_tx_for_private_broadcast.has_value()) {
Since std::optional has a dedicated operator bool() with the same semantics as has_value(), I think if (m_tx_for_private_broadcast) is preferable here.
It is idiomatic C++20 and avoids adding noise around already defensive private-broadcast guards. I would reserve .has_value() for cases where the boolean value is being named or passed onward, not for simple control flow.
if (!m_tx_for_private_broadcast) {
(applies to a few more cases here)
3569 | @@ -3556,7 +3570,15 @@ void PeerManagerImpl::PushPrivateBroadcastTx(CNode& node) 3570 | { 3571 | Assume(node.IsPrivateBroadcastConn()); 3572 | 3573 | - const auto opt_tx{m_tx_for_private_broadcast.PickTxForSend(node.GetId(), CService{node.addr})}; 3574 | + if (!m_tx_for_private_broadcast.has_value()) { 3575 | + LogWarning("[privatebroadcast] Disconnecting: private broadcast storage is "
Other private-broadcast runtime logs are already almost entirely LogDebug(BCLog::PRIVBROADCAST, ...), consider if this really needs to expose to the logs that we are using private broadcast.
1091 | @@ -1092,7 +1092,7 @@ class PeerManagerImpl final : public PeerManager 1092 | void LogBlockHeader(const CBlockIndex& index, const CNode& peer, bool via_compact_block); 1093 | 1094 | /// The transactions to be broadcast privately. 1095 | - PrivateBroadcast m_tx_for_private_broadcast; 1096 | + std::optional<PrivateBroadcast> m_tx_for_private_broadcast;
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, e.g. std::unique_ptr<PrivateBroadcast>.
Approach ACK 87a42882a94f89eb7d418d3f2f3558fb3bc74a17
Making the private-broadcast transaction state optional is a useful cleanup for the default net-processing path, but my main concern is that the optional is currently initialized from InitiateTxBroadcastPrivate() instead of from the peer manager’s private-broadcast option. That means a direct call can create private-broadcast state even when the peer manager was constructed with private broadcast disabled. I left a suggested patch and a small unit test for that boundary.
I also left smaller comments around idiomatic optional checks, matching nearby private-broadcast logging style, and clarifying that std::optional<PrivateBroadcast> still reserves inline storage even when disengaged.
Could you also summarize the relevant motivation from the linked #29415 discussion in the PR description or commit message? The links are useful, but having the actual rationale here would make this PR easier to review on its own.