Skip to content

net: make m_nodes_mutex non-recursive #32394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vasild wants to merge 2 commits into
base: master
Choose a base branch
from
+69 −49
Open

net: make m_nodes_mutex non-recursive #32394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
487f4d1
net: drop the only recursive usage of CConnman::m_nodes_mutex
vasild Oct 15, 2025
a6ccd6f
net: make CConnman::m_nodes_mutex non-recursive
vasild Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 58 additions & 38 deletions src/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,8 +1138,8 @@ class CConnman
bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !m_added_nodes_mutex, !m_addr_fetches_mutex, !mutexMsgProc);

void StopThreads();
void StopNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex);
void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex)
void StopNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_reconnections_mutex);
void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_reconnections_mutex)
{
AssertLockNotHeld(m_reconnections_mutex);
StopThreads();
Expand Down Expand Up @@ -1170,20 +1170,20 @@ class CConnman
ConnectionType conn_type,
bool use_v2transport,
const std::optional<Proxy>& proxy_override = std::nullopt)
EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex);

bool CheckIncomingNonce(uint64_t nonce);
bool CheckIncomingNonce(uint64_t nonce) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
void ASMapHealthCheck();

// alias for thread safety annotations only, not defined
RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex);
Mutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex);

bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);

using NodeFn = std::function<void(CNode*)>;
void ForEachNode(const NodeFn& func)
void ForEachNode(const NodeFn& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex)
{
LOCK(m_nodes_mutex);
for (auto&& node : m_nodes) {
Expand All @@ -1192,7 +1192,7 @@ class CConnman
}
};

void ForEachNode(const NodeFn& func) const
void ForEachNode(const NodeFn& func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex)
{
LOCK(m_nodes_mutex);
for (auto&& node : m_nodes) {
Expand Down Expand Up @@ -1238,21 +1238,22 @@ class CConnman
void StartExtraBlockRelayPeers();

// Count the number of full-relay peer we have.
int GetFullOutboundConnCount() const;
int GetFullOutboundConnCount() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
// Return the number of outbound peers we have in excess of our target (eg,
// if we previously called SetTryNewOutboundPeer(true), and have since set
// to false, we may have extra peers that we wish to disconnect). This may
// return a value less than (num_outbound_connections - num_outbound_slots)
// in cases where some outbound connections are not yet fully connected, or
// not yet fully disconnected.
int GetExtraFullOutboundCount() const;
int GetExtraFullOutboundCount() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
// Count the number of block-relay-only peers we have over our limit.
int GetExtraBlockRelayCount() const;
int GetExtraBlockRelayCount() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

bool AddNode(const AddedNodeParams& add) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
bool RemoveAddedNode(std::string_view node) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
bool AddedNodesContain(const CAddress& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
std::vector<AddedNodeInfo> GetAddedNodeInfo(bool include_connected) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
std::vector<AddedNodeInfo> GetAddedNodeInfo(bool include_connected) const
EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_nodes_mutex);

/**
* Attempts to open a connection. Currently only used from tests.
Expand All @@ -1267,16 +1268,17 @@ class CConnman
* - Max total outbound connection capacity filled
* - Max connection capacity for type is filled
*/
bool AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
bool AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport)
EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex);

size_t GetNodeCount(ConnectionDirection) const;
size_t GetNodeCount(ConnectionDirection) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() const;
uint32_t GetMappedAS(const CNetAddr& addr) const;
void GetNodeStats(std::vector<CNodeStats>& vstats) const;
bool DisconnectNode(std::string_view node);
bool DisconnectNode(const CSubNet& subnet);
bool DisconnectNode(const CNetAddr& addr);
bool DisconnectNode(NodeId id);
void GetNodeStats(std::vector<CNodeStats>& vstats) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
bool DisconnectNode(std::string_view node) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
bool DisconnectNode(const CSubNet& subnet) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
bool DisconnectNode(const CNetAddr& addr) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
bool DisconnectNode(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

//! Used to convey which local services we are offering peers during node
//! connection.
Expand Down Expand Up @@ -1340,13 +1342,27 @@ class CConnman
bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions);
bool InitBinds(const Options& options);

void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex);
void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex,
!m_nodes_mutex,
!m_reconnections_mutex,
!m_unused_i2p_sessions_mutex);

void AddAddrFetch(const std::string& strDest) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex);
void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex);
void ThreadOpenConnections(std::vector<std::string> connect, std::span<const std::string> seed_nodes) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex);
void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc);
void ThreadI2PAcceptIncoming();
void AcceptConnection(const ListenSocket& hListenSocket);

void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex,
!m_nodes_mutex,
!m_unused_i2p_sessions_mutex);

void ThreadOpenConnections(std::vector<std::string> connect, std::span<const std::string> seed_nodes)
EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex,
!m_addr_fetches_mutex,
!m_nodes_mutex,
!m_reconnections_mutex,
!m_unused_i2p_sessions_mutex);

void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc);
void ThreadI2PAcceptIncoming() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
void AcceptConnection(const ListenSocket& hListenSocket) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

/**
* Create a `CNode` object from a socket that has just been accepted and add the node to
Expand All @@ -1359,10 +1375,11 @@ class CConnman
void CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
NetPermissionFlags permission_flags,
const CService& addr_bind,
const CService& addr);
const CService& addr)
EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

void DisconnectNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_nodes_mutex);
void NotifyNumConnectionsChanged();
void NotifyNumConnectionsChanged() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
/** Return true if the peer is inactive and should be disconnected. */
bool InactivityCheck(const CNode& node) const;

Expand All @@ -1376,7 +1393,7 @@ class CConnman
/**
* Check connected and listening sockets for IO readiness and process them accordingly.
*/
void SocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
void SocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc);

/**
* Do the read/write for connected sockets that are ready for IO.
Expand All @@ -1391,7 +1408,8 @@ class CConnman
* Accept incoming connections, one from each read-ready listening socket.
* @param[in] events_per_sock Sockets that are ready for IO.
*/
void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock);
void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock)
EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !m_nodes_mutex, !m_reconnections_mutex);
void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex);
Expand All @@ -1405,7 +1423,7 @@ class CConnman
* @param[in] host String of the form "host[:port]", e.g. "localhost" or "localhost:8333" or "1.2.3.4:8333".
* @return true if connected to `host`.
*/
bool AlreadyConnectedToHost(std::string_view host) const;
bool AlreadyConnectedToHost(std::string_view host) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

/**
* Determine whether we're already connected to a given address:port.
Expand All @@ -1414,14 +1432,14 @@ class CConnman
* @param[in] addr_port Address and port to check.
* @return true if connected to addr_port.
*/
bool AlreadyConnectedToAddressPort(const CService& addr_port) const;
bool AlreadyConnectedToAddressPort(const CService& addr_port) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

/**
* Determine whether we're already connected to a given address.
*/
bool AlreadyConnectedToAddress(const CNetAddr& addr) const;
bool AlreadyConnectedToAddress(const CNetAddr& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

bool AttemptToEvictConnection();
bool AttemptToEvictConnection() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

/**
* Open a new P2P connection.
Expand All @@ -1439,7 +1457,7 @@ class CConnman
ConnectionType conn_type,
bool use_v2transport,
const std::optional<Proxy>& proxy_override)
EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex);

void AddWhitelistPermissionFlags(NetPermissionFlags& flags, std::optional<CNetAddr> addr, const std::vector<NetWhitelistPermissions>& ranges) const;

Expand All @@ -1465,7 +1483,7 @@ class CConnman
/**
* Return vector of current BLOCK_RELAY peers.
*/
std::vector<CAddress> GetCurrentBlockRelayOnlyConns() const;
std::vector<CAddress> GetCurrentBlockRelayOnlyConns() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

/**
* Search for a "preferred" network, a reachable network to which we
Expand All @@ -1477,7 +1495,7 @@ class CConnman
*
* @return bool Whether a preferred network was found.
*/
bool MaybePickPreferredNetwork(std::optional<Network>& network);
bool MaybePickPreferredNetwork(std::optional<Network>& network) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

// Whether the node should be passed out in ForEach* callbacks
static bool NodeFullyConnected(const CNode* pnode);
Expand Down Expand Up @@ -1521,7 +1539,7 @@ class CConnman
mutable Mutex m_added_nodes_mutex;
std::vector<CNode*> m_nodes GUARDED_BY(m_nodes_mutex);
std::list<CNode*> m_nodes_disconnected;
mutable RecursiveMutex m_nodes_mutex;
mutable Mutex m_nodes_mutex;
std::atomic<NodeId> nLastNodeId{0};
unsigned int nPrevNodeCount{0};

Expand Down Expand Up @@ -1701,7 +1719,8 @@ class CConnman
std::list<ReconnectionInfo> m_reconnections GUARDED_BY(m_reconnections_mutex);

/** Attempt reconnections, if m_reconnections non-empty. */
void PerformReconnections() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_unused_i2p_sessions_mutex);
void PerformReconnections()
EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_reconnections_mutex, !m_unused_i2p_sessions_mutex);

/**
* Cap on the size of `m_unused_i2p_sessions`, to ensure it does not
Expand All @@ -1717,6 +1736,7 @@ class CConnman
{
public:
explicit NodesSnapshot(const CConnman& connman, bool shuffle)
EXCLUSIVE_LOCKS_REQUIRED(!connman.m_nodes_mutex)
{
{
LOCK(connman.m_nodes_mutex);
Expand Down
22 changes: 11 additions & 11 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,23 +1285,23 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
}
m_connman.ForNode(nodeid, [this](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
AssertLockHeld(::cs_main);
if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
// As per BIP152, we only get 3 of our peers to announce
// blocks using compact encodings.
m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){
MakeAndPushMessage(*pnodeStop, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION);
// save BIP152 bandwidth state: we select peer to be low-bandwidth
pnodeStop->m_bip152_highbandwidth_to = false;
return true;
});
lNodesAnnouncingHeaderAndIDs.pop_front();
}
MakeAndPushMessage(*pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION);
// save BIP152 bandwidth state: we select peer to be high-bandwidth
pfrom->m_bip152_highbandwidth_to = true;
lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId());
return true;
});
if (lNodesAnnouncingHeaderAndIDs.size() > 3) {
// As per BIP152, we only get 3 of our peers to announce
// blocks using compact encodings.
m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){
MakeAndPushMessage(*pnodeStop, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION);
// save BIP152 bandwidth state: we select peer to be low-bandwidth
pnodeStop->m_bip152_highbandwidth_to = false;
return true;
});
lNodesAnnouncingHeaderAndIDs.pop_front();
Copy link
Contributor

@Crypt-iQ Crypt-iQ Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initially concerned that maybe lNodesAnnouncingHeaderAndIDs.front() could have been removed from in between releasing the lock and acquiring the lock again. It seems fine since if we do not find lNodesAnnouncingHeaderAndIDs.front(), we don't need to set m_bip152_highbandwidth_to false and we still remove it from the list. There was some previous discussion about the locking here.

Copy link
Contributor Author

@vasild vasild Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed from in between releasing the lock and acquiring the lock again

How? lNodesAnnouncingHeaderAndIDs is protected by cs_main which is held in the entire PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs().

Copy link
Contributor

@Crypt-iQ Crypt-iQ Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was a little unclear. I meant that the CNode* that lNodesAnnouncingHeaderAndIDs.front() refers to might have been removed from in m_nodes if we're not holding m_nodes_mutex the entire time. Though I did not write a test to check if this is actually possible, so I might be wrong.

Copy link
Contributor Author

@vasild vasild Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that seems to be fine - if the last m_connman.ForNode(...front()) does nothing (the one at the end, just before pop_front()).

}
}

bool PeerManagerImpl::TipMayBeStale()
Expand Down
Loading