p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll #35297

I guess technically this could be racy, but in practice it is not, because InitiateTxBroadcastToAll is not called from a context where the tx_relay status might change?

I don't think this refactor changes anything about races:

• If you are referring to a memory-safety race, where tx_relay could be set to nullptr: There is no code currently doing that, and the bug would be unchanged by this pull request, because m_peer_mutex does not protect m_tx_relay.
• If you are referring to a logic race, where tx_relay may still be nullptr: This certainly does happen in practise, because calling this function on the RPC/wallet thread will race against the p2p threads creating new peers, creating tx_relay structs and setting m_next_inv_send_time. However, this pull request does not change anything about that. If there was a RPC thread that waited for a peer or a peer's tx_relay, or a peer's tx_relay.m_tx_inventory_mutex to be initialized, they are unaffected by this refactor, because they will correctly wait for the status and then correctly observe the intended behavior.
• If you are referring to a race, where the RPC/wallet thread doesn't wait and "blindly" calls this function, then the behavior before and after this refactor will be that the broadcast is initiated on a best-effort basis to all peers which have m_next_inv_send_time currently set.

Of course, it is permitted to lock m_peer_mutex while locking a peer-specific mutex, but I don't see a use-case for that (unless one would want to micro-optimize to avoid taking a copy of a shared pointer). So I think it is better to avoid holding m_peer_mutex longer than needed and only hold it longer when there is a strong rationale.

If m_peer_mutex was used to prevent peer-internal state races, that seems like a confusing layer violation. If this was used, at a minimum it would require a strong rationale, and proper lock annotation (or code comment).

Thanks, this was helpful to further trace some of its usage.

Of course, it is permitted to lock m_peer_mutex while locking a peer-specific mutex, but I don't see a use-case for that (unless one would want to micro-optimize to avoid taking a copy of a shared pointer). So I think it is better to avoid holding m_peer_mutex longer than needed and only hold it longer when there is a strong rationale.

To extend on this: Another place that would be safer by taking the GetAllPeers snapshot and releasing the m_peer_mutex early is RelayAddress/PushAddress. With the current code in master, the following (arguably absurd) diff would compile fine, but lead to runtime-UB/crash later on:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 71cc6ff7ba..5a8f6105f0 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1139,2 +1139,3 @@ void PeerManagerImpl::PushAddress(Peer& peer, const CAddress& addr)
         } else {
+         LOCK(m_peer_mutex);
             peer.m_addrs_to_send.push_back(addr);

I think copying shared pointers here is fine and beneficial to reduce the (recursive) scope of the lock. Though, this style-cleanup can be done in the future when that code is touched the next time.

edit: nvm, this is caught by clang:

src/net_processing.cpp:1140:10: warning: calling function 'MaybeCheckNotHeld' requires negative capability '!m_peer_mutex' [-Wthread-safety-analysis]
 1140 |          LOCK(m_peer_mutex);
      |          ^