p2p: do not make automatic outbound connections to addnode peers

jonatack commented at 4:44 pm on November 16, 2023: member

to allocate our limited outbound slots correctly, and to ensure addnode connections benefit from their intended protections.

Our addnode logic usually connects the addnode peers before the automatic outbound logic does, but not always, as a connection race can occur. If an addnode peer disconnects us and if it was the only one from its network, there can be a race between reconnecting to it with the addnode thread, and it being picked as automatic network-specific outbound peer. Or our internet connection or router or the addnode peer could be temporarily offline, and then return online during the automatic outbound thread. Or we could add a new manual peer using the addnode RPC at that time.

The race can be more apparent when our node doesn’t know many peers, or with networks like cjdns that currently have few bitcoin peers.

When an addnode peer is connected as an automatic outbound peer and is the only connection we have to a network, it can be protected by our new outbound eviction logic and persist in the “wrong role”.

Finally, there does not seem to be a reason to make block-relay or short-lived feeler connections to addnode peers, as the addnode logic will ensure we connect to them if they are up, within the addnode connection limit.

Fix these issues by checking if the address is an addnode peer in our automatic outbound connection logic.

p2p: do not make automatic outbound connections to addnode peers

to allocate our limited outbound slots correctly, and to ensure addnode
connections benefit from their intended protections.

Our addnode logic usually connects the addnode peers before the automatic
outbound logic does, but not always, as a connection race can occur.  If an
addnode peer disconnects us and if it was the only one from its network, there
can be a race between reconnecting to it with the addnode thread, and it being
picked as automatic network-specific outbound peer.  Or our internet connection
or router, or the addnode peer, could be temporarily offline, and then return
online during the automatic outbound thread.  Or we could add a new manual peer
using the addnode RPC at that time.

The race can be more apparent when our node doesn't know many peers, or with
networks like cjdns that currently have few bitcoin peers.

When an addnode peer is connected as an automatic outbound peer and is the only
connection we have to a network, it can be protected by our new outbound
eviction logic and persist in the "wrong role".

Examples on mainnet using logging added in the same pull request:

2023-08-12T14:51:05.681743Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic network-specific outbound-full-relay connection
to i2p peer selected for manual (addnode) connection: [geh...odq.b32.i2p]:0

2023-08-13T03:59:28.050853Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic block-relay-only connection to onion peer
selected for manual (addnode) connection: kpg...aid.onion:8333

2023-08-13T16:21:26.979052Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic network-specific outbound-full-relay connection
to cjdns peer selected for manual (addnode) connection: [fcc...8ce]:8333

2023-08-14T20:43:53.401271Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic network-specific outbound-full-relay connection
to cjdns peer selected for manual (addnode) connection: [fc7...59e]:8333

2023-08-15T00:10:01.894147Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic feeler connection to i2p peer selected for
manual (addnode) connection: geh...odq.b32.i2p:8333

Finally, there does not seem to be a reason to make block-relay or short-lived
feeler connections to addnode peers, as the addnode logic will ensure we connect
to them if they are up, within the addnode connection limit.

Fix these issues by checking if the address is an addnode peer in our automatic
outbound connection logic.

cc62716920

test: add unit test for CConnman::AddedNodesContain() 5e7cc4144b

DrahtBot commented at 4:44 pm on November 16, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, vasild, guggero, brunoerg

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

DrahtBot added the label P2P on Nov 16, 2023

jonatack commented at 4:49 pm on November 16, 2023: member

Reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and extracted from an earlier version of #28248.

The first commit ought to be feasible to backport, if desired. The second test commit would require #28155.

dergoegge commented at 5:17 pm on November 16, 2023: member

Thank you for isolating this change and making it more clear what you were going for.

From what I understand the “regression” is not that we might make automatic connections to addnode peers but rather that if that happens these connections might stick around as automatic connections for longer (due to the new eviction logic), while not being labeled as manual.

So in the end a user might end up with less outbound connections than expected for longer than is currently possible, with some of their manual connections incorrectly labeled as automatic.

If the above is correct, then I am not sure if this is worthy of a backport at this stage in the release cycle because: 1) This should be rare and 2) the manual connections are still being made and persisted (just labeled incorrectly).

jonatack commented at 5:23 pm on November 16, 2023: member

Empirically, the regression is that we’re actively adding addnode peers as outbound ones, and keeping them there. Since a couple of years I run a node with all three of our privacy networks and addnode entries for each, along with clearnet, and I didn’t see my addnode peers being connected to as non-manual outbounds. This is new behavior for my node, it’s frequent, it removes the intended privileges that addnode entries benefit from, and it impacts precisely the nodes running multiple networks including ones with fewer peers that used addnode entries to ensure connection to them.

dergoegge commented at 6:05 pm on November 16, 2023: member

Empirically, the regression is that we’re actively adding addnode peers as outbound ones, and keeping them there.

Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.

I’m wondering why you only now noticed this, since the duration of these mislabeled connections prior to the new eviction logic (i assume) depended on regular outbound eviction behavior, which could also result in longer living connections.

jonatack commented at 6:30 pm on November 16, 2023: member

From what I understand the “regression” is not that we might make automatic connections to addnode peers but rather that if that happens these connections might stick around as automatic connections for longer (due to the new eviction logic), while not being labeled as manual.

This stems from a change in this release where it was observed and reported. We did not previously automatically add outbound connections to addnode entries in sparse networks to the degree and regularity that we do now.

So in the end a user might end up with less outbound connections than expected for longer than is currently possible, with some of their manual connections incorrectly labeled as automatic.

Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.

This should be rare and 2) the manual connections are still being made and persisted (just labeled incorrectly).

No longer rare, and not only about mislabeled connections. The new change is also misallocating rare outbound connection slots, and attributing incorrect peer privileges as addnode peers benefit from non-eviction, are not disconnected or discouraged for bad behavior, etc.

I’m wondering why you only now noticed this, since the duration of these mislabeled connections prior to the new eviction logic (i assume) depended on regular outbound eviction behavior, which could also result in longer living connections.

Observed and reported in August on the pull that made the change (there was no reaction). While it looks like it could occur somewhat rarely before, with the change it is now frequent, and more noticeable as the connections are protected.

mzumsande commented at 9:01 pm on November 16, 2023: contributor

Tested ACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1

I tested this by running on cjdns and ipv4, and then manually disconnecting an addnode cjdns peer and observing the log - both on v26.0rc2 and this branch.

Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.

I think I disagree with that. They might now be protected from eviction due to network-specific extra peers, but that eviction is also new: Before #27213, we only had the stale-tip eviction, and I think that it would only extremely rarely evict such a peer because of the way it works (see this post for why I think so), so once an automatic connections was made, it was probably never evicted unless there were other causes like we got offline or they disconnected us.

I think that the mechanism mentioned in the OP (race after getting disconnected) is probably what makes this most noticeable, because ThreadOpenAddedConnections() has a long sleep (60s) which it wouldn’t interrupt, and even though EXTRA_NETWORK_PEER_INTERVAL = 5min is larger, it would immediately kick in after the only peer from a network disconnected us (and then the next time ~5 minutes later), so ThreadOpenConnection would usually be quicker than ThreadOpenAddedConnections().

A completely unrelated problem is that this problem exists because cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I’m not sure what to do about that.

jonatack commented at 9:16 pm on November 16, 2023: member

cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I’m not sure what to do about that.

I also saw the network-specific issue with i2p peers, and when adding a new network, though cjdns is indeed a good way to test it. As to what we can do to further cjdns adoption, when one-click I2P support was added to Raspiblitz and Umbrel in December 2022 after this workshop and blog post, the number of recent peers returned by -addrinfo for me went from a couple hundred to quickly more than a thousand, and now a couple thousand. Cjdns being a friend-of-a-friend topology requires the additional step of finding a friend to connect to the network with, but we have a couple of stable ones in the hardcoded DNS seeds, and maybe I can encourage and work with the node-in-a-box packages to get support for it in (and do more related workshops).

in src/test/net_peer_connection_tests.cpp:126 in 5e7cc4144b

121@@ -122,6 +122,13 @@ BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection)
122     BOOST_CHECK_EQUAL(connman->GetAddedNodeInfo(/*include_connected=*/true).size(), nodes.size());
123     BOOST_CHECK(connman->GetAddedNodeInfo(/*include_connected=*/false).empty());
124 
125+    // Test AddedNodesContain()
126+    for (auto node : connman->TestNodes()) {

jonatack commented at 9:30 pm on November 16, 2023:

Note to self when retouching here or in follow-ups (i.e. #28248)

0    for (const auto& node : connman->TestNodes()) {

vasild approved

vasild commented at 1:35 pm on November 17, 2023: contributor

ACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1

in src/net.cpp:3476 in 5e7cc4144b

3471+    const std::string addr_str{addr.ToStringAddr()};
3472+    const std::string addr_port_str{addr.ToStringAddrPort()};
3473+    LOCK(m_added_nodes_mutex);
3474+    return (m_added_node_params.size() < 24 // bound the query to a reasonable limit
3475+            && std::any_of(m_added_node_params.cbegin(), m_added_node_params.cend(),
3476+                           [&](const auto& p) { return p.m_added_node == addr_str || p.m_added_node == addr_port_str; }));

vasild commented at 1:40 pm on November 17, 2023:

(hit “submit” too early)

nit, non-blocker: I do not think that this < 24 cap is needed. What is the max number of added nodes expected?

I think it is not worth doing that, but in theory, if this is to be optimized - then we can observe that addr_port_str begins with addr_str. For example aaa:8333 and aaa and if we are checking if string aaaa is equal to either one, the way this is done now is that it would traverse the entire aaaa twice whereas it can be done with a single traverse. But please ignore this :)

jonatack commented at 1:47 pm on November 17, 2023:

I think the number of added nodes is up to the user without a limit, though 8 at a time is the addnode connection limit. We can change m_added_node_params to an unordered set later on as you suggested in #28248 to make the query constant-time on average and drop the size check. Kept it simple here.

guggero approved

guggero commented at 2:54 pm on November 17, 2023: contributor

utACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1

I think it’s important that nodes added by addnode always get the elevated protection level.

A completely unrelated problem is that this problem exists because cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I’m not sure what to do about that.

I’m still trying to wrap my head around CJDNS and how it can be set up in a docker based environment. If successful, I’ll try creating an Umbrel app for it, which should hopefully help with growing the network.

brunoerg commented at 4:36 pm on November 17, 2023: contributor

utACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1

DrahtBot requested review from brunoerg on Nov 17, 2023

DrahtBot removed review request from brunoerg on Nov 17, 2023

fanquake merged this on Nov 22, 2023

fanquake closed this on Nov 22, 2023

fanquake commented at 11:57 am on November 22, 2023: member

Added the fix to #28872 for backporting.

fanquake referenced this in commit 55af112565 on Nov 22, 2023

fanquake referenced this in commit e4fef4ae65 on Nov 22, 2023

jonatack deleted the branch on Nov 22, 2023

kwvg referenced this in commit 909e293e1e on Oct 26, 2024

kwvg referenced this in commit ffe8a36e74 on Oct 26, 2024

kwvg referenced this in commit 098a56747a on Oct 26, 2024

kwvg referenced this in commit 09504bdd1f on Oct 26, 2024

PastaPastaPasta referenced this in commit 25c3355053 on Oct 27, 2024

bitcoin locked this on Nov 21, 2024

p2p: do not make automatic outbound connections to addnode peers #28895

Code Coverage

Reviews