net_processing: make m_tx_for_private_broadcast optional #34271

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.

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.

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() = 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.