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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34271.
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 <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
LogPeer by l0rinc)privatebroadcast logging (+ warn for debug logs) 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.
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.
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?
0 if (pfrom.IsPrivateBroadcastConn()) {
1 const auto pushed_tx_opt{!m_tx_for_private_broadcast.has_value() ? std::nullopt : m_tx_for_private_broadcast->GetTxForNode(pfrom.GetId())};
2 if (!pushed_tx_opt) {
3 LogInfo( ...
4 ...
5 }
6 ...
7 }
That can happen if
InitiateTxBroadcastPrivate()was not called before.
Ok, but how could pfrom.IsPrivateBroadcastConn() be true if InitiateTxBroadcastPrivate() was not called before?
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).
IsPrivateBroadcastConn() true.
m_tx_for_private_broadcast is touched, verified building and unit tests locally; left some comments.
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
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.
