net: drop the only recursive usage of CConnman::m_nodes_mutex
What the developer wrote
Authored by Vasil Dimov
net: drop the only recursive usage of CConnman::m_nodes_mutex
The only recursive usage of `CConnman::m_nodes_mutex` is from
`PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs()` which uses
nested calls to `CConnman::ForNode()` to trim the size of
`lNodesAnnouncingHeaderAndIDs` to `<= 3`. This need not be nested, so
take it out.
Before:
```
fornode(newnode)
if (size >= 3)
fornode(front) handle removal of front
pop front
push back newnode
```
After:
```
fornode(newnode)
push back newnode
if (size > 3)
fornode(front) handle removal of front
pop front
```
`lNodesAnnouncingHeaderAndIDs` is protected by `cs_main` which is locked
during the entire operation.
What changed, and why it matters
This is a small internal cleanup in Bitcoin Core's network code. It removes the only case where a particular network lock (m_nodes_mutex) was acquired twice in a nested/recursive way. The change reorders the logic so the list of high-bandwidth compact-block peers is updated without nested lock calls. There is no direct security vulnerability being fixed; it is a code-quality and future-proofing change that makes the locking simpler and easier to reason about.
No urgent action. Treat as a normal code-quality/refactoring commit. Reviewers may want to confirm that cs_main is held across both ForNode() calls and that the list size check remains correct under concurrency. Consider whether the recursive mutex can now be made non-recursive in a follow-up if desired.
Security signals we found
Eliminates recursive locking on CConnman::m_nodes_mutex
Simplifies lock-order reasoning in peer-manager network path
No functional change to BIP152 compact-block peer selection logic
No input validation, memory safety, or cryptographic changes
Evidence from the diff
The commit refactors PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs() in src/net_processing.cpp. Previously, the code called CConnman::ForNode() for the new peer, and inside that lambda (while holding m_nodes_mutex) it could call ForNode() again to notify the oldest peer being demoted. That made m_nodes_mutex a recursive mutex. The patch splits the logic: first append the new peer via ForNode(), then, only if the list now exceeds three entries, call ForNode() separately to demote and pop the front. The list is protected by cs_main, which is held throughout, so the functional behavior is unchanged. The change is purely structural to eliminate the recursive lock usage.
Changed components
src/net_processing.cppPeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs()CConnman::m_nodes_mutex locking behaviorInspect captured patch +13 / −13
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b870df66..10168d7f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1309,25 +1309,25 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
}
}
}
- m_connman.ForNode(nodeid, [this](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+ const bool nodeid_was_appended{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 (nodeid_was_appended && 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();
+ }
}
bool PeerManagerImpl::TipMayBeStale()
Why this scored 18/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.