Merge bitcoin/bitcoin#35297: p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll

fa2afba28b p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll (MarcoFalke)

Pull request description:

  The `InitiateTxBroadcastToAll` method holds the `m_peer_mutex` while updating the bloom filters for all peers. This is perfectly fine, because updating the bloom filters is fast. Though, from a style-perspective, the lock does not need to be held for the whole function. Also, holding the lock longer, may confuse Tsan into a lock-order inversion false-positive (ref: https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359).

  So "fix" both issues in this style-refactor.

ACKs for top commit:
  xyzconstant:
    Code review ACK fa2afba28b
  shuv-amp:
    ACK fa2afba28b
  danielabrozzoni:
    Code Review ACK fa2afba28b
  sedited:
    ACK fa2afba28b

Tree-SHA512: c47849a4c3cc11c74b61fec3425db8ec7f78db4ca43d7bf3145ce640f7b0872701c09495f0dfe77109d09d5716d920ad3d7308483fe41564c30867b3e80432e7

src/net_processing.cpp

@@ -575,6 +575,9 @@ private:
      *  May return an empty shared_ptr if the Peer object can't be found. */
     PeerRef RemovePeer(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 
+    /// Get all existing peers in m_peer_map.
+    std::vector<PeerRef> GetAllPeers() const EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+
     /** Mark a peer as misbehaving, which will cause it to be disconnected and its
      *  address discouraged. */
     void Misbehaving(Peer& peer, const std::string& message);
@@ -1785,6 +1788,17 @@ PeerRef PeerManagerImpl::RemovePeer(NodeId id)
     return ret;
 }
 
+std::vector<PeerRef> PeerManagerImpl::GetAllPeers() const
+{
+    std::vector<PeerRef> peers;
+    LOCK(m_peer_mutex);
+    peers.reserve(m_peer_map.size());
+    for (const auto& [_, peer] : m_peer_map) {
+        peers.push_back(peer);
+    }
+    return peers;
+}
+
 bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const
 {
     {
@@ -2244,9 +2258,10 @@ void PeerManagerImpl::SendPings()
 
 void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wtxid)
 {
-    LOCK(m_peer_mutex);
-    for(auto& it : m_peer_map) {
-        Peer& peer = *it.second;
+    for (const PeerRef& peer_ref : GetAllPeers()) {
+        if (!peer_ref) continue;
+        Peer& peer{*peer_ref};
+
         auto tx_relay = peer.GetTxRelay();
         if (!tx_relay) continue;
 

test/sanitizer_suppressions/tsan

@@ -3,11 +3,6 @@
 #
 # https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions
 
-# deadlock (TODO fix)
-# To reproduce, see:
-# https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359
-deadlock:Chainstate::ConnectTip
-
 # Intentional deadlock in tests
 deadlock:sync_tests::potential_deadlock_detected