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

vasild commented at 2:36 pm on April 22, 2025: contributor

CConnman::FindNode() would lock m_nodes_mutex, find the node in m_nodes, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from FindNode() without owning m_nodes_mutex and without having that node’s reference count incremented.

Change FindNode() to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the return value.

Remove a recursive lock on m_nodes_mutex.

Rename FindNode() to better describe what it does.

DrahtBot commented at 2:36 pm on April 22, 2025: contributor

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/32326.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator
Concept ACK	shahsb, theuni
Stale ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
#30988 (Split CConnman by vasild)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

DrahtBot added the label P2P on Apr 22, 2025

DrahtBot added the label CI failed on Apr 22, 2025

DrahtBot commented at 2:57 pm on April 22, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40947126551

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

in src/net.cpp:392 in 1e415d2cb7 outdated

388@@ -404,10 +389,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
389             return nullptr;
390 
391         // Look for an existing connection
392-        CNode* pnode = FindNode(static_cast<CService>(addrConnect));
393-        if (pnode)
394-        {
395-            LogPrintf("Failed to open new connection, already connected\n");
396+        if (OutboundConnectedToService(addrConnect)) {

vasild commented at 2:59 pm on April 22, 2025:

This rename makes obvious that maybe something is wrong in the current code in master. Is the intention really to ignore inbound connections when, according to the comment we intent to “Look for an existing connection”?

For example, if 1.1.1.1 connects to 2.2.2.2:8333, then the TCP connection will look like: 1.1.1.1:53481 <-> 2.2.2.2:8333 where 53481 is a random port picked by the initiator’s OS. Now, if 2.2.2.2 intends to connect to 1.1.1.1:8333 then this check will return false because 1.1.1.1:53481 is not 1.1.1.1:8333 and the code will proceed with connecting.

Is this really the intention? Shouldn’t this check ignore the port?

jonatack commented at 3:14 pm on April 22, 2025:

I ought to resuscitate some of #28248 that was looking at this (or will review if you do).

mzumsande commented at 3:17 pm on April 22, 2025:

I don’t know if this is intentional, or a bug or something in betweeen.

If I remember correctly , #28248 suggested to change that, or something very similar. But changing it is a bit scary to review, because sometimes IP addresses are shared - especially local IPs such as 127.0.0.1. So just changing it to ignore the port would have the potential to make local setups that probably exist out there not work any longer.

jonatack commented at 3:30 pm on April 22, 2025:

Yes, I see review feedback on that in #28248 (comment) and #28248 (review).

vasild commented at 3:40 pm on April 22, 2025:

Love how you deciphered this at an instant! :heart:

I will look into adding a comment to the call sites about this.

vasild commented at 3:55 pm on April 22, 2025:

Hmm, in CConnman::OpenNetworkConnection() the code does:

if pszDest is not set then use AlreadyConnectedToAddress(addrConnect) which ignores the port
if pszDest is set then OutboundConnectedToStr(pszDest) which compares the port as well.

Changing that logic is, I guess, out of the scope of this non-functional refactor PR. If to be done it would better be assessed in isolation in its own PR.

vasild commented at 4:12 pm on April 22, 2025:

I will look into adding a comment to the call sites about this.

Can’t come up with a good comment. Leaving it as it is, unless somebody has suggestions.

hodlinator commented at 8:35 am on April 24, 2025:

Interesting aspect. Agree current name makes sense - inbound nodes, if they are already connected to us would not be using their listening port (even if behind NAT/router). So we could be attempting to establish an outbound connection to a different node behind the same IP.

But even if ConnectNode here distinguishes between ports, the only caller may override and filter out connections without regard for port, as pointed out above (https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054408043). That part of that other function seems like a bug, that needn’t be fixed in this PR.

I assume addr-gossip only contains address+port from outbound peers (unless gossip is spoofed), otherwise we would be gossiping non-listening ports, which are pretty useless. Maybe inbound nodes already share their listening port with us upon connection, for us to be able to gossip about it? (Pointers to RTFM/code with links are welcome!).

vasild commented at 11:54 am on April 24, 2025:

I assume addr-gossip only contains address+port from outbound peers …

The origin of the gossip is the self-announcement done by each node:

https://github.com/bitcoin/bitcoin/blob/458720e5e98c6e9103aea1fdfcd39bafc26c27bb/src/net_processing.cpp#L5286-L5300

then as this address is received by a node, it is further relayed to other nodes. Eventually everybody stores it in their addrman. Then GETADDR requests are served from addrman. Outbound connections are made to addresses that are already in addrman.

hodlinator commented at 1:59 pm on April 24, 2025:

Good that we advertise our listening port for outbound peers! Seems kind of unnecessary to tell inbound peers about the address they report as seeing us as (CService{node.GetAddrLocal()}): https://github.com/bitcoin/bitcoin/blob/458720e5e98c6e9103aea1fdfcd39bafc26c27bb/src/net.cpp#L246-L266 But I guess it simplifies the addrman-logic a little bit, not having to populate/refresh it from the local outbound connections.

theuni commented at 5:20 pm on April 28, 2025:

Hmm, in CConnman::OpenNetworkConnection() the code does:
* if `pszDest` is not set then use `AlreadyConnectedToAddress(addrConnect)` which ignores the port

* if `pszDest` is set then `OutboundConnectedToStr(pszDest)` which compares the port as well.
Changing that logic is, I guess, out of the scope of this non-functional refactor PR. If to be done it would better be assessed in isolation in its own PR.

Also, later on in ConnectNode():

 0    if (pszDest == nullptr) {
 1        if (IsLocal(addrConnect))
 2            return nullptr;
 3
 4        // Look for an existing connection
 5        CNode* pnode = FindNode(static_cast<CService>(addrConnect));
 6        if (pnode)
 7        {
 8            LogPrintf("Failed to open new connection, already connected\n");
 9            return nullptr;
10        }

Which seems to be redundant, as it’s a strictly narrower check because of the port?

vasild commented at 6:45 am on April 29, 2025:

True, and ConnectNode() is only called from OpenNetworkConnection(). Looks like another room for improvement.

While this may seem minor, the amount of developers’ time spent on grasping it is not. In the past I have stared at these calls disproportionally long. Would be nice if this is spared to future developers. I will go after that separately from this PR.

in src/net.cpp:349 in 1e415d2cb7 outdated

370 }
371 
372 bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
373 {
374-    return FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort());
375+    const CNetAddr net_addr{addr};

jonatack commented at 3:07 pm on April 22, 2025:

In 9e657358271e7a045c, to make the CI happy

0    const CNetAddr& net_addr{addr};

vasild commented at 3:33 pm on April 22, 2025:

Fixed, thanks!

jonatack commented at 3:11 pm on April 22, 2025: member

Review ACK 1e415d2cb703fcd1b622e5343fa264425601c8d modulo CI fixup

Happy to see these methods see an update.

I tried adding EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) to the declarations, but it looks like that lock is already held by some of their callers.

vasild force-pushed on Apr 22, 2025

vasild commented at 3:33 pm on April 22, 2025: contributor

1e415d2cb7...cc8f239663: fix CI tidy

DrahtBot removed the label CI failed on Apr 22, 2025

jonatack commented at 5:32 pm on April 22, 2025: member

ACK cc8f239663a0874c307efa30815b40d9ef40badb

in src/net.cpp:335 in cc8f239663 outdated

331@@ -332,42 +332,27 @@ bool IsLocal(const CService& addr)
332     return mapLocalHost.count(addr) > 0;
333 }
334 
335-CNode* CConnman::FindNode(const CNetAddr& ip)
336+bool CConnman::OutboundConnectedToStr(const std::string& str_addr)

w0xlt commented at 6:48 pm on April 22, 2025:

0bool CConnman::OutboundConnectedToAddress(const std::string& str_addr)

The caller already knows that the parameter is a string type, but clarifying what the string represents (a network address) seems better to me.

vasild commented at 7:21 am on April 23, 2025:

Alright, I agree OutboundConnectedToStr() is a bit odd. If it is renamed to OutboundConnectedToAddress(), then what about OutboundConnectedToService()? Leave it like this? Then it would be confusing because we have the CAddress type and the ...Address() suffix would imply such an argument. I wanted to avoid overload (two different methods with the same name) because that is not grep-friendly. Should I drop that and have an overload?

Maybe:

ConnectedToAddrPort(string) and ConnectedToAddrPort(CService) (overload), or
ConnectedToAddrPortStr() and ConnectedToAddrPortService()?

There is this old rule that “if you cannot find a good name for a method, something is wrong with that method”, but this method already exists as it is. Leave its name to FindNode()?

w0xlt commented at 4:50 pm on April 23, 2025:

FindNode() is not bad, but I personally like ConnectedToAddrPort(). This name makes the purpose of the function very clear. The parameter type (CService and str) makes the difference clear. It is not necessary for the function name to describe the parameter (except in Rust).

vasild commented at 12:30 pm on April 24, 2025:

So this:

 0--- i/src/net.h
 1+++ w/src/net.h
 2@@ -1355,24 +1355,24 @@ private:
 3     /**
 4      * Determine whether we're already connected to a given "address:port".
 5      * Note that for inbound connections, the peer is likely using a random outbound
 6      * port on their side, so this will likely not match any inbound connections.
 7      * [@param](/bitcoin-bitcoin/contributor/param/)[in] str_addr String of the form "address:port", e.g. "1.2.3.4:8333".
 8      * [@return](/bitcoin-bitcoin/contributor/return/) true if connected to addrName.
 9      */
10-    bool OutboundConnectedToStr(const std::string& str_addr);
11+    bool ConnectedToAddrPort(const std::string& str_addr);
12 
13     /**
14      * Determine whether we're already connected to a given address:port.
15      * Note that for inbound connections, the peer is likely using a random outbound
16      * port on their side, so this will likely not match any inbound connections.
17      * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Address and port to check.
18      * [@return](/bitcoin-bitcoin/contributor/return/) true if connected to addr.
19      */
20-    bool OutboundConnectedToService(const CService& addr);
21+    bool ConnectedToAddrPort(const CService& addr);
22 
23     /**
24      * Determine whether we're already connected to a given address, in order to
25      * avoid initiating duplicate connections.
26      */
27     bool AlreadyConnectedToAddress(const CAddress& addr);
28

?

Note the similar function that has an “Already” prefix. Make all 3 consistent, either all with or all without an “Already” prefix? @jonatack, @hodlinator, you already ACKed the current naming, what do you think?

hodlinator commented at 1:24 pm on April 24, 2025:

These are the best I could come up with:

IsConnectedToStrAddr
{IsConnectedToService | IsConnectedToAddrPort}
IsConnectedToAddress

Is-prefix == short way to signify boolean query function.

vasild commented at 1:22 pm on April 28, 2025:

I think this:

After #32338 makes it, then here in this PR:

rename AlreadyConnectedToAddress() to IsConnectedToAddr(), compare just the address
rename the two remaining FindNode() to IsConnectedToAddrPort(), overload, one taking a string and the other taking a CService, both comparing the address and the port.

vasild commented at 9:08 am on May 1, 2025:

Done.

in src/net.cpp:349 in cc8f239663 outdated

370 }
371 
372 bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
373 {
374-    return FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort());
375+    const CNetAddr& net_addr{addr};

hodlinator commented at 8:11 am on April 24, 2025:

nit: It is somewhat opaque what is going on here. Would be good to add comment:

0    // Downcast to CNetAddr as we don't care to match on port or other parts of CAddress.
1    const CNetAddr& net_addr{addr};

An even clearer alternative would be to take CNetAddr as the function argument and instead clarify this behavior in the comment of the header.

vasild commented at 1:03 pm on April 24, 2025:

but, but, but… :) this function does care to compare the port as it compares node->m_addr_name to addr.ToStringAddrPort(). If the argument is changed from CAddress to CNetAddr, then it would need another argument - the string “addr:port”. This oddness is the same in master. Now I wonder if this condition:

0node->addr == net_addr || node->m_addr_name == str_addr
1
2// or in master, which is the same:
3// FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort())

is not the same as:

0node->addr == net_addr

I think it is…

hodlinator commented at 1:39 pm on April 24, 2025:

Inconsistency seems to have been introduced when we started checking for string-version in 9bab521df895c149579b9e64931405c56b008afb in April of 2012. Not on purpose I would wager.

vasild commented at 2:39 pm on April 24, 2025:

Opened #32338 with this single change, dropping the || FindNode(addr.ToStringAddrPort()) condition.

vasild commented at 9:09 am on May 1, 2025:

#32338 was merged and these variables are not necessary now.

hodlinator commented at 12:04 pm on May 2, 2025:

Nice!

in src/net.cpp:393 in cc8f239663 outdated

388@@ -404,10 +389,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
389             return nullptr;
390 
391         // Look for an existing connection
392-        CNode* pnode = FindNode(static_cast<CService>(addrConnect));
393-        if (pnode)
394-        {
395-            LogPrintf("Failed to open new connection, already connected\n");
396+        if (OutboundConnectedToService(addrConnect)) {
397+            LogPrintf("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort());

hodlinator commented at 8:19 am on April 24, 2025:

nit: Should avoid deprecated macro:

0            LogInfo("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort());

vasild commented at 9:20 am on May 1, 2025:

Done in latest push.

hodlinator approved

hodlinator commented at 9:56 am on April 24, 2025: contributor

ACK cc8f239663a0874c307efa30815b40d9ef40badb

Fixes thread-unsafe pattern (originally?) raised by me here: #32015 (review) (Even though current master is not susceptible to the found CNode being deleted randomly on another thread).

shahsb commented at 11:34 am on April 24, 2025: none

Concept ACK

theuni commented at 8:21 pm on April 25, 2025: member

Is making m_nodes_mutex non-recursive a goal? That sounds nice to me and I’m not opposed, but I’d rather not make changes just to get halfway there.

Concept ACK on turning the FindNodes into bools, though. That’s a nice improvement.

hebasto commented at 12:35 pm on April 26, 2025: member

Is making m_nodes_mutex non-recursive a goal? That sounds nice to me and I’m not opposed, but I’d rather not make changes just to get halfway there.

I’m curious how far we are from a non-recursive m_nodes_mutex?

vasild commented at 12:14 pm on April 28, 2025: contributor

Is making m_nodes_mutex non-recursive a goal?

No, I did not think about that before you mentioned it. The goal here is to make the interface around FindNode() safer and simpler (don’t return a possibly dangling pointer and don’t return a pointer at all since callers can do with a boolean).

I did mention dropping the recursive lock because I think less recursive locking is good, even if the mutex remains recursive.

I’m curious how far we are from a non-recursive m_nodes_mutex?

This far away:

 0--- i/src/net_processing.cpp
 1+++ w/src/net_processing.cpp
 2@@ -1284,29 +1284,29 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
 3                 std::swap(lNodesAnnouncingHeaderAndIDs.front(), *std::next(lNodesAnnouncingHeaderAndIDs.begin()));
 4             }
 5         }
 6     }
 7     m_connman.ForNode(nodeid, [this](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
 8         AssertLockHeld(::cs_main);
 9-        if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
10-            // As per BIP152, we only get 3 of our peers to announce
11-            // blocks using compact encodings.
12-            m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){
13-                MakeAndPushMessage(*pnodeStop, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION);
14-                // save BIP152 bandwidth state: we select peer to be low-bandwidth
15-                pnodeStop->m_bip152_highbandwidth_to = false;
16-                return true;
17-            });
18-            lNodesAnnouncingHeaderAndIDs.pop_front();
19-        }
20         MakeAndPushMessage(*pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION);
21         // save BIP152 bandwidth state: we select peer to be high-bandwidth
22         pfrom->m_bip152_highbandwidth_to = true;
23         lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId());
24         return true;
25     });
26+    if (lNodesAnnouncingHeaderAndIDs.size() > 3) {
27+        // As per BIP152, we only get 3 of our peers to announce
28+        // blocks using compact encodings.
29+        m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){
30+            MakeAndPushMessage(*pnodeStop, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION);
31+            // save BIP152 bandwidth state: we select peer to be low-bandwidth
32+            pnodeStop->m_bip152_highbandwidth_to = false;
33+            return true;
34+        });
35+        lNodesAnnouncingHeaderAndIDs.pop_front();
36+    }
37 }
38 
39 bool PeerManagerImpl::TipMayBeStale()
40 {
41     AssertLockHeld(cs_main);
42     const Consensus::Params& consensusParams = m_chainparams.GetConsensus();

On top of this PR, plus a pile of EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) additions in net.h. Should I PR that (separately from the current PR)?

hebasto commented at 1:04 pm on April 28, 2025: member

On top of this PR, plus a pile of EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) additions in net.h. Should I PR that (separately from the current PR)?

I’d appreciate it. It would be good to cross m_nodes_mutex off the list.

theuni commented at 3:21 pm on April 28, 2025: member

I’d appreciate it. It would be good to cross m_nodes_mutex off the list.

Yeah, agreed. That’s a good justification for the change to non-recursive here.

Looking at that diff, it’s not clear to me if it’s safe to move that hunk out of cs_main, but we can review/test/argue about it in a new PR.

vasild marked this as a draft on Apr 29, 2025

vasild commented at 6:52 am on April 29, 2025: contributor

Converted to draft, waiting for:

#32338 to make it
then change this PR to use naming as in #32326 (review)

Further improvements away from this PR:

make m_nodes_mutex non-recursive, #32326 (comment)
assess all the checks that compare just the address or address+port and remove redundant ones and make the calls consistent regardless of whether pzsDest is set or not, #32326 (review)

DrahtBot added the label Needs rebase on Apr 29, 2025

net: merge AlreadyConnectedToAddress() and FindNode(CNetAddr)

`CConnman::AlreadyConnectedToAddress()` is the only caller of
`CConnman::FindNode(CNetAddr)`, so merge the two in one function and
rename it to `IsConnectedToAddr()` to show that it compares just the
address and not the port.

The unit test that checked whether `AlreadyConnectedToAddress()` ignores
the port is now unnecessary because now the function takes a `CNetAddr`
argument. It has no access to the port.

f8201774b5

net: avoid recursive m_nodes_mutex lock in DisconnectNode()

Have `CConnman::DisconnectNode()` iterate `m_nodes` itself instead of
using `FindNode()`. This avoids recursive mutex lock and drops the only
caller of `FindNode()` which used the return value for something else
than a boolean found/notfound.

78e0345867

vasild force-pushed on May 1, 2025

vasild commented at 9:07 am on May 1, 2025: contributor

cc8f239663...8ef769cc2d: rebase and use the naming:

0bool IsConnectedToAddrPort(const std::string& addr_port);
1bool IsConnectedToAddrPort(const CService& addr_port);
2bool IsConnectedToAddr(const CNetAddr& addr);

net: change FindNode() to not return a node and rename it

All callers of `CConnman::FindNode()` use its return value `CNode*` only
as a boolean null/notnull. So change that method to return `bool`.

This removes the dangerous pattern of handling a `CNode` object (the
return value of `FindNode()`) without holding `CConnman::m_nodes_mutex`
and without having that object's reference count incremented for the
duration of the usage.

Also rename the method to better describe what it does.

4f635d100d

vasild force-pushed on May 1, 2025

vasild commented at 9:13 am on May 1, 2025: contributor

8ef769cc2d...4f635d100d: make the methods const

vasild marked this as ready for review on May 1, 2025

DrahtBot removed the label Needs rebase on May 1, 2025

vasild commented at 12:13 pm on May 1, 2025: contributor

Made m_nodes_mutex non-recursive in #32394 which is this PR + one more commit.

hodlinator approved

hodlinator commented at 12:49 pm on May 2, 2025: contributor

re-ACK 4f635d100dd1ab5e83bb392d8fbee4a3f4400315

Concept unchanged since first ACK.

Changes since then:

Includes now merged #32338 which simplified AlreadyConnectedToAddress() (spawned from this PR: #32326 (review))
Changed from OutboundConnectedTo(Str|Service) -> IsConnectedToAddr(|Port) const and str_addr -> addr_port, nice!
AlreadyConnectedToAddress(const CAddress&) -> IsConnectedToAddr(const CNetAddr&) const.

DrahtBot requested review from shahsb on May 2, 2025

DrahtBot requested review from jonatack on May 2, 2025

DrahtBot requested review from theuni on May 2, 2025

in src/net.h:1376 in 4f635d100d

1377-     * Determine whether we're already connected to a given address, in order to
1378-     * avoid initiating duplicate connections.
1379+     * Determine whether we're already connected to a given address.
1380      */
1381-    bool AlreadyConnectedToAddress(const CAddress& addr);
1382+    bool IsConnectedToAddr(const CNetAddr& addr) const;

jonatack commented at 6:30 pm on May 3, 2025:

Note the similar function that has an “Already” prefix. Make all 3 consistent, either all with or all without an “Already” prefix?

I prefer the original function naming AlreadyConnectedToAddress and it looks like retaining it would also reduce the diff here.

In alignment, IsConnectedToAddrPort could be named AlreadyConnectedToAddressPort.

DrahtBot requested review from jonatack on May 3, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts