p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll
What the developer wrote
Authored by MarcoFalke
p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll
What changed, and why it matters
This change refactors how Bitcoin Core sends transaction announcements to all connected peers. Previously, the code held a broad internal lock (m_peer_mutex) while iterating over every peer and preparing messages. The patch copies the peer list under the lock, then releases the lock before doing the slower per-peer work. This reduces the chance of lock contention and removes a known ThreadSanitizer suppression for a deadlock in Chainstate::ConnectTip. It is a robustness improvement, not a fix for an exploitable vulnerability.
Treat as a normal code-quality/concurrency-hardening commit. Review that GetAllPeers correctly preserves peer lifetime via shared_ptr and that no new race is introduced by accessing peer state without m_peer_mutex. No urgent security deployment is warranted.
Security signals we found
Deadlock/lock-contention reduction: holding m_peer_mutex across I/O-adjacent peer operations could contribute to deadlocks or severe contention.
ThreadSanitizer suppression removed for Chainstate::ConnectTip deadlock, indicating the change addresses a previously known threading issue.
No input validation, cryptographic, or network parsing changes are present.
No memory safety bug is fixed; the change is concurrency-hardening.
Evidence from the diff
InitiateTxBroadcastToAll previously locked m_peer_mutex and iterated m_peer_map directly, holding the mutex across peer.GetTxRelay(), MaybeSendFeefilter(), and setInventoryTxToSend.insert(). The patch introduces GetAllPeers(), which acquires m_peer_mutex only to copy PeerRef values into a vector, then returns them. InitiateTxBroadcastToAll iterates that vector without holding m_peer_mutex. The ThreadSanitizer suppression for a deadlock in Chainstate::ConnectTip (linked to issue #19303) is removed, suggesting the lock-holding pattern was implicated in that deadlock. PeerRef is a shared_ptr, so peers remain alive during the unlocked iteration.
Changed components
src/net_processing.cpp PeerManagerImplInitiateTxBroadcastToAllGetAllPeersm_peer_mutextest/sanitizer_suppressions/tsanInspect captured patch +18 / −8
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 631e968d..c4be0e61 100644
--- a/src/net_processing.cpp
+++ b/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
{
{
@@ -2243,9 +2257,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;
diff --git a/test/sanitizer_suppressions/tsan b/test/sanitizer_suppressions/tsan
index 66157101..34004c67 100644
--- a/test/sanitizer_suppressions/tsan
+++ b/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
Why this scored 23/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.