net: make m_nodes_mutex non-recursive #32394

vasild · 2025-05-01T12:04:32Z

The only case of a recursive lock was a nested ForNode() call 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.

Partially resolves: #19303

This PR included #32326 (first 3 commits in this PR). That PR was merged first, so the size of this was reduced.

DrahtBot · 2025-05-01T12:04:35Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32394.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Crypt-iQ, hodlinator
Concept ACK	hebasto
Approach ACK	w0xlt

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32278 (doc: better document NetEventsInterface and the deletion of "CNode"s by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto · 2025-05-01T14:18:50Z

Partially resolves: #19303

Concept ACK on that.

vasild · 2025-05-21T11:33:53Z

98bba932bf...22d4c57a99: rebase due to conflicts

vasild · 2025-09-29T13:00:14Z

22d4c57a99...9a49877f26: rebase due to conflicts

hebasto · 2025-10-02T10:53:39Z

Time to rebase once more?

vasild · 2025-10-07T09:08:01Z

aaa75c1a41...492be23a18: rebase due to conflicts

src/net_processing.cpp

hodlinator

Concept ACK 492be23a1873d7165e6cc75d1282f4feb63f87b9

src/net_processing.cpp

vasild · 2025-10-15T14:03:26Z

492be23a18...4aad3714d6: split in two commits: #32394 (comment)

hodlinator

Reviewed 4aad3714d66cc80c08cb1c827fb49fba7dcf8d50

All annotations make sense, but left Q regarding 1-2 potentially missing ones.

src/net_processing.cpp

src/net.h

vasild · 2025-10-16T08:42:50Z

4aad3714d6...4b3a2c2360: do #32394 (comment)

hodlinator

ACK 4b3a2c23608e709a63b9d5bd69c3eb16cf08e462

Manually went through all functions using CConnman::m_nodes_mutex and only remaining issue I could find is now fixed (https://github.com/bitcoin/bitcoin/pull/32394/files#r2433600255).

Also checked out first commit and only cherrypicked mutex type change to re-confirm Clang spews warnings about missing annotations (and no warnings at HEAD of PR).

src/net_processing.cpp

w0xlt · 2025-11-04T18:19:23Z

Approach ACK

vasild · 2025-11-05T18:03:08Z

4b3a2c2360...1a9274f391: rebase due to conflicts

hodlinator

re-ACK 1a9274f391df0465c17b6b6664988af70910e39b

Changes since previous review (#32394 (review)):

Resolves conflict with #32983.

Crypt-iQ

crACK 1a9274f391df0465c17b6b6664988af70910e39b

Needs a small rebase since #33956 added 2 annotations.

Crypt-iQ · 2025-12-05T20:29:12Z

src/net_processing.cpp

+            pnodeStop->m_bip152_highbandwidth_to = false;
+            return true;
+        });
+        lNodesAnnouncingHeaderAndIDs.pop_front();


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.

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().

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.

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

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.

This change includes `s/RecursiveMutex/Mutex/` and a pile of annotations to keep the compiler happy after the type change. Partially resolves: bitcoin#19303

vasild · 2025-12-09T15:21:49Z

1a9274f391...a6ccd6fdbf: rebase due to conflicts

Crypt-iQ · 2025-12-09T16:34:47Z

re-ACK a6ccd6f

hodlinator

re-ACK a6ccd6f

Rebase to fix conflict with #33956

DrahtBot · 2026-01-12T22:00:28Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the P2P label May 1, 2025

vasild mentioned this pull request May 1, 2025

net: improve the interface around FindNode() and avoid a recursive mutex lock #32326

Merged

This comment was marked as spam.

Sign in to view

hebasto mentioned this pull request May 1, 2025

Replace all of the RecursiveMutex instances with the Mutex ones #19303

Open

35 tasks

DrahtBot mentioned this pull request May 10, 2025

threading: drop CSemaphore in favor of c++20 std::counting_semaphore #32466

Merged

DrahtBot added the Needs rebase label May 20, 2025

vasild force-pushed the m_nodes_mutex branch from 98bba93 to 22d4c57 Compare May 21, 2025 11:33

DrahtBot removed the Needs rebase label May 21, 2025

This was referenced Jul 14, 2025

rpc,net: Add getpeerbyid and listpeersbyids RPCs #32972

Closed

rpc: refactor: use string_view in Arg/MaybeArg #32983

Merged

DrahtBot added CI failed and removed CI failed labels Jul 30, 2025

DrahtBot mentioned this pull request Sep 17, 2025

net: do not apply whitelist permissions to onion inbounds #33395

Merged

DrahtBot added the Needs rebase label Sep 17, 2025

vasild force-pushed the m_nodes_mutex branch from 22d4c57 to 9a49877 Compare September 29, 2025 13:00

DrahtBot removed the Needs rebase label Sep 29, 2025

DrahtBot mentioned this pull request Sep 29, 2025

net: support overriding the proxy selection in ConnectNode() #33454

Merged

DrahtBot added the Needs rebase label Oct 1, 2025

DrahtBot removed the Needs rebase label Oct 7, 2025

l0rinc reviewed Oct 9, 2025

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

hodlinator reviewed Oct 14, 2025

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

vasild force-pushed the m_nodes_mutex branch from 492be23 to 4aad371 Compare October 15, 2025 14:02

hodlinator reviewed Oct 15, 2025

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

src/net.h Show resolved Hide resolved

vasild force-pushed the m_nodes_mutex branch from 4aad371 to 4b3a2c2 Compare October 16, 2025 08:42

hodlinator approved these changes Oct 18, 2025

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

DrahtBot requested a review from hebasto October 18, 2025 21:00

DrahtBot added the Needs rebase label Oct 24, 2025

vasild force-pushed the m_nodes_mutex branch from 4b3a2c2 to 1a9274f Compare November 5, 2025 18:02

DrahtBot removed the Needs rebase label Nov 5, 2025

hodlinator approved these changes Nov 6, 2025

View reviewed changes

DrahtBot mentioned this pull request Nov 30, 2025

net: fix use-after-free with v2->v1 reconnection logic #33956

Merged

DrahtBot added the Needs rebase label Dec 5, 2025

Crypt-iQ reviewed Dec 5, 2025

View reviewed changes

vasild added 2 commits December 9, 2025 16:15

net: make CConnman::m_nodes_mutex non-recursive

a6ccd6f

This change includes `s/RecursiveMutex/Mutex/` and a pile of annotations to keep the compiler happy after the type change. Partially resolves: bitcoin#19303

vasild force-pushed the m_nodes_mutex branch from 1a9274f to a6ccd6f Compare December 9, 2025 15:21

DrahtBot removed the Needs rebase label Dec 9, 2025

DrahtBot requested a review from hodlinator December 9, 2025 16:34

hodlinator approved these changes Dec 11, 2025

View reviewed changes

DrahtBot added the Needs rebase label Jan 12, 2026

net: make m_nodes_mutex non-recursive #32394

Are you sure you want to change the base?

net: make m_nodes_mutex non-recursive #32394

Conversation

vasild commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

This comment was marked as spam.

Uh oh!

hebasto commented May 1, 2025

Uh oh!

vasild commented May 21, 2025

Uh oh!

vasild commented Sep 29, 2025

Uh oh!

hebasto commented Oct 2, 2025

Uh oh!

vasild commented Oct 7, 2025

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Oct 15, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vasild commented Oct 16, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w0xlt commented Nov 4, 2025

Uh oh!

vasild commented Nov 5, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

vasild Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

vasild commented Dec 9, 2025

Uh oh!

Crypt-iQ commented Dec 9, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

vasild commented May 1, 2025 •

edited

Loading

DrahtBot commented May 1, 2025 •

edited

Loading

Crypt-iQ Dec 5, 2025 •

edited

Loading