net: improves addnode / m_added_nodes logic #28155

sr-gi · 2023-07-25T20:47:02Z

Rationale

Currently, addnode has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to m_added_nodes, hence making Bitcoin iterate through useless data on a regular basis.

Connecting to the same node more than once

In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via addnode in two different ways:

Calling addnode more than once in a short time period, using two equivalent but distinct addresses
Calling addnode add using an IP, and addnode onetry after with an address that resolved to the same IP

For the former, the issue boils down to CConnman::ThreadOpenAddedConnections calling CConnman::GetAddedNodeInfo once, and iterating over the result to open connections (CConman::OpenNetworkConnection) on the same loop for all addresses.CConnman::ConnectNode only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.

An example to test this would be calling:

bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" add

And check how it allows us to perform both connections some times, and some times it fails.

The latter boils down to the same issue, but takes advantage of onetry bypassing the CConnman::ThreadOpenAddedConnections logic and calling CConnman::OpenNetworkConnection straightaway. A way to test this would be:

bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" onetry

Adding the same peer with two different, yet equivalent, addresses

The current implementation of addnode is pretty naive when checking what data is added to m_added_nodes. Given the collection stores strings, the checks at CConnman::AddNode() basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.

Two examples would be 127.0.0.1 being equal to 127.1 and [::1] being equal to [0:0:0:0:0:0:0:1]. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by getaddednodeinfo, given they map to the same CService.

This is less severe than the previous issue, since even tough both nodes are reported as connected by getaddednodeinfo, there is only a single connection to them (as properly reported by getpeerinfo). However, this adds redundant data to m_added_nodes, which is undesirable.

Parametrize `CConnman::GetAddedNodeInfo`

Finally, this PR also parametrizes CConnman::GetAddedNodeInfo so it returns either all added nodes info, or only info about the nodes we are not connected to. This method is used both for rpc, in getaddednodeinfo, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in CConnman::ThreadOpenAddedConnections, in which we are currently returning more data than needed and then actively filtering using CService.fConnected()

DrahtBot · 2023-07-25T20:47:05Z

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	jonatack, kashifs, mzumsande
Stale ACK	vasild

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:

#28248 (p2p: peer connection bug fixes by jonatack)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

sr-gi · 2023-07-25T20:49:43Z

I've purposely split this into three commits according to each of the fixes so they can be discussed, and considered independently. That way it'll be easier to remove any if the change does not reach enough consensus.

kevkevinpal · 2023-07-26T21:29:18Z

would it make sense to add a functional test like the below under

ip_port = "localhost:[]".format(p2p_port='add')
self.nodes[0].addnode(node=ip_port, command='add')

in test/functional/rpc_net.py on line 212
because we shortly after assert that there is only 1 added node added

sr-gi · 2023-07-28T16:45:48Z

would it make sense to add a functional test like the below under
ip_port = "localhost:[]".format(p2p_port='add')
self.nodes[0].addnode(node=ip_port, command='add')
in test/functional/rpc_net.py on line 212 because we shortly after assert that there is only 1 added node added

I don't think that'd work. You'll still be able to add two nodes that resolve to the same IP using addnode as long as they belong to different namespaces. Lookups are not performed. The difference though is that both connections won't be established. However, the connection is not being checked by the test.

What could be added, however, is a check for 127.0.0.1 and 127.1.

PS: This actually made me realize that the check for whether to add something to m_added_nodes was not good enough.

sr-gi · 2023-07-28T17:32:02Z

Added a test to test/functional/rpc_net.py and fixed the check on CConnman::AddNode

test/functional/rpc_net.py

jonatack · 2023-08-12T15:23:23Z

Concept ACK. I've been looking at this code lately as well, e.g. bug fixes (5ae9fe1 / c9dbf0b), logic (4820f7f) and logging (d6b0da5) improvements and agree with the issues you report. Will review soon.

vasild

Approach ACK a6fcf17

src/net.cpp

sr-gi · 2023-08-23T08:40:13Z

Addressed @vasild comments

vasild

ACK d0e7e18

Would be nice to print the hostname here: #28155 (comment)

vasild

ACK fd8b1db

that are otherwise private: - CConnman::m_nodes - CConnman::ConnectNodes() - CConnman::AlreadyConnectedToAddress() and update the #include headers per iwyu.

sr-gi · 2023-10-30T17:31:52Z

Addresses review comments and add @jonatack's test commits

jonatack · 2023-10-30T20:38:03Z

ACK fc1e9e4

94e8882, and maybe also 2574b7e, may be worth backporting.

(If any review feedback related to the unit tests, happy to address them in test coverage follow-ups.)

src/test/net_peer_connection_tests.cpp

for initial partial unit test coverage of these CConnman class methods: - AddNode() - ConnectNode() - GetAddedNodeInfo() - AlreadyConnectedToAddress() - ThreadOpenAddedConnections() and of the GetAddedNodeInfo() call in RPC addnode.

jonatack · 2023-10-31T17:59:07Z

re-ACK 0420f99

kashifs · 2023-11-02T23:16:52Z

tACK 0420f9

mzumsande

Tested ACK 0420f99

sr-gi · 2023-11-03T19:22:13Z

tACK 0420f9

Thanks for reviewing! Would you mind sharing what did you test and how?

@vasild would you mind re-acking this when you have some time?

kashifs · 2023-11-04T18:12:38Z

tACK 0420f9

Thanks for reviewing! Would you mind sharing what did you test and how?

@vasild would you mind re-acking this when you have some time?

I pulled the branch, compiled from source on my Mac, and ran:

./src/test/test_bitcoin -t net_peer_connection_tests -l message -- -printtoconsole=1

and

./src/test/test_bitcoin -t net_peer_connection_tests -l all -- -printtoconsole=1

I also pored over every line of modified code in order to better understand the design.

Is there anything else that I should do that might be helpful?

sr-gi · 2023-11-04T21:57:44Z

tACK 0420f9

Thanks for reviewing! Would you mind sharing what did you test and how?

@vasild would you mind re-acking this when you have some time?

I pulled the branch, compiled from source on my Mac, and ran:

./src/test/test_bitcoin -t net_peer_connection_tests -l message -- -printtoconsole=1

and

./src/test/test_bitcoin -t net_peer_connection_tests -l all -- -printtoconsole=1

I also pored over every line of modified code in order to better understand the design.

Is there anything else that I should do that might be helpful?

Nope, that's good. Thanks again for reviewing and testing.

vasild

ACK 0420f99

vasild · 2023-11-14T12:40:54Z

src/test/util/net.cpp

+CNode* ConnmanTestMsg::ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type)
+{
+    CNode* node = ConnectNode(CAddress{}, pszDest, /*fCountFailure=*/false, conn_type, /*use_v2transport=*/true);
+    if (!node) return nullptr;
+    node->SetCommonVersion(PROTOCOL_VERSION);
+    peerman.InitializeNode(*node, ServiceFlags(NODE_NETWORK | NODE_WITNESS));
+    node->fSuccessfullyConnected = true;
+    AddTestNode(*node);
+    return node;
+}


nit, naming: maybe a different name would have been better than ConnectNodePublic(), given that it contains extra logic on top of the private ConnectNode().

True. I'm touching that code for #28248, if you have a naming suggestion.

Dunno, maybe ConnectNodeAndInitialize()?

… connecting to a node fd81a37 net: attempts to connect to all resolved addresses when connecting to a node (Sergi Delgado Segura) Pull request description: This is a follow-up of #28155 motivated by #28155 (comment) ## Rationale Prior to this, when establishing a network connection via `CConnman::ConnectNode`, if the connection needed address resolution, a single address would be picked at random from the resolved addresses and our node would try to connect to it. However, this would lead to the behavior of `ConnectNode` being unpredictable when the address was resolved to various ips (e.g. the address resolving to IPv4 and IPv6, but we only support one of them). This patches the aforementioned behavior by going over all resolved IPs until a valid one is found or until we exhaust them. ACKs for top commit: mzumsande: re-ACK fd81a37 (just looked at diff, only small logging change) achow101: ACK fd81a37 vasild: ACK fd81a37 Tree-SHA512: fa1ebc5c84fe61dd0a7fe1113ae2d594a75ad661c43ed8984a31fc9bc50f166b2759b0d8d84ee5dc247691eff78c8156fac970af797bbcbf67492eec0353fb58

… one The current `addnode` rpc command has some edge cases in where it is possible to connect to the same node twice by combining ip and address requests. This can happen under two situations: The two commands are run one right after each other, in which case they will be processed under the same loop in `CConnman::ThreadOpenAddedConnections` without refreshing `vInfo`, so both will go trough. An example of this would be: ``` bitcoin-cli addnode "localhost:port" add ``` A node is added by IP using `addnode "add"` while the other is added by name using `addnode "onetry"` with an address that resolves to multiple IPs. In this case, we currently only check one of the resolved IPs (picked at random), instead of all the resolved ones, meaning this will only probabilistically fail/succeed. An example of this would be: ``` bitcoin-cli addnode "127.0.0.1:port" add [...] bitcoin-cli addnode "localhost:port" onetry ``` Both cases can be fixed by iterating over all resolved addresses in `CConnman::ConnectNode` instead of picking one at random Github-Pull: bitcoin#28155 Rebased-From: 2574b7e

…ently Currently it is possible to add the same node twice when formatting IPs in different, yet equivalent, manner. This applies to both ipv4 and ipv6, e.g: 127.0.0.1 = 127.1 | [::1] = [0:0:0:0:0:0:0:1] `addnode` will accept both and display both as connected (given they translate to the same IP). This will not result in multiple connections to the same node, but will report redundant info when querying `getaddednodeinfo` and populate `m_added_nodes` with redundant data. This can be avoided performing comparing the contents of `m_added_addr` and the address to be added as `CServices` instead of as strings. Github-Pull: bitcoin#28155 Rebased-From: 94e8882

…nected address on request `CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in `getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however the nodes we are already connected to are only relevant to the latter, in the former they are actively discarded. Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to, to avoid passing useless information around. Github-Pull: bitcoin#28155 Rebased-From: 34b9ef4

that are otherwise private: - CConnman::m_nodes - CConnman::ConnectNodes() - CConnman::AlreadyConnectedToAddress() and update the #include headers per iwyu. Github-Pull: bitcoin#28155 Rebased-From: 4b834f6

for initial partial unit test coverage of these CConnman class methods: - AddNode() - ConnectNode() - GetAddedNodeInfo() - AlreadyConnectedToAddress() - ThreadOpenAddedConnections() and of the GetAddedNodeInfo() call in RPC addnode. Github-Pull: bitcoin#28155 Rebased-From: 0420f99

sr-gi force-pushed the 202307-addednodeinfo branch from ad3fee2 to 2110ac1 Compare July 25, 2023 21:03

DrahtBot added the CI failed label Jul 25, 2023

sr-gi force-pushed the 202307-addednodeinfo branch from 2110ac1 to 866bc36 Compare July 26, 2023 15:24

DrahtBot removed the CI failed label Jul 26, 2023

sr-gi force-pushed the 202307-addednodeinfo branch 2 times, most recently from 6304950 to b5763bc Compare July 28, 2023 17:23

DrahtBot added the CI failed label Jul 28, 2023

kevkevinpal reviewed Jul 28, 2023

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

sr-gi force-pushed the 202307-addednodeinfo branch from b5763bc to a6fcf17 Compare July 28, 2023 18:41

DrahtBot removed the CI failed label Jul 28, 2023

This was referenced Aug 5, 2023

Use shared_ptr for CNode inside CConnman #28222

Closed

p2p: Allow whitelisting manual connections #27114

Merged

p2p: peer connection bug fixes #28248

Closed

vasild reviewed Aug 17, 2023

View reviewed changes

sr-gi force-pushed the 202307-addednodeinfo branch from a6fcf17 to d0e7e18 Compare August 23, 2023 08:39

DrahtBot mentioned this pull request Aug 24, 2023

BIP324 integration #28331

Merged

vasild approved these changes Aug 24, 2023

View reviewed changes

sr-gi force-pushed the 202307-addednodeinfo branch from d0e7e18 to fd8b1db Compare August 25, 2023 13:47

vasild approved these changes Aug 25, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 10, 2023

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

Allow unit tests to access additional CConnman members

4b834f6

that are otherwise private: - CConnman::m_nodes - CConnman::ConnectNodes() - CConnman::AlreadyConnectedToAddress() and update the #include headers per iwyu.

sr-gi force-pushed the 202307-addednodeinfo branch from 3d1854c to fc1e9e4 Compare October 30, 2023 15:45

This was referenced Oct 30, 2023

build: Introduce internal kernel library #28690

Draft

test: add end-to-end tests for CConnman and PeerManager #26812

Open

DrahtBot requested a review from mzumsande October 30, 2023 20:38

DrahtBot mentioned this pull request Oct 30, 2023

test: Extend stale_tip_peer_management test #23352

Closed

fanquake reviewed Oct 31, 2023

View reviewed changes

src/test/net_peer_connection_tests.cpp Outdated Show resolved Hide resolved

Create net_peer_connection unit tests

0420f99

for initial partial unit test coverage of these CConnman class methods: - AddNode() - ConnectNode() - GetAddedNodeInfo() - AlreadyConnectedToAddress() - ThreadOpenAddedConnections() and of the GetAddedNodeInfo() call in RPC addnode.

sr-gi force-pushed the 202307-addednodeinfo branch from fc1e9e4 to 0420f99 Compare October 31, 2023 17:37

mzumsande reviewed Nov 3, 2023

View reviewed changes

glozow merged commit 9ad19fc into bitcoin:master Nov 8, 2023
16 checks passed

sr-gi mentioned this pull request Nov 9, 2023

net: attempts to connect to all resolved addresses when connecting to a node #28834

Merged

hebasto mentioned this pull request Nov 9, 2023

Rebased cmake-staging branch (post PRs 45 and 34) hebasto/bitcoin#47

Closed

vasild reviewed Nov 14, 2023

View reviewed changes

This was referenced Nov 14, 2023

[26.x] Changes for rc3 #28872

Merged

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

Merged

theStack mentioned this pull request Dec 9, 2023

test: fix addnode functional test failure on OpenBSD #29035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: improves addnode / m_added_nodes logic #28155

net: improves addnode / m_added_nodes logic #28155

sr-gi commented Jul 25, 2023

DrahtBot commented Jul 25, 2023 •

edited

sr-gi commented Jul 25, 2023

kevkevinpal commented Jul 26, 2023

sr-gi commented Jul 28, 2023

sr-gi commented Jul 28, 2023

jonatack commented Aug 12, 2023 •

edited

vasild left a comment

sr-gi commented Aug 23, 2023

vasild left a comment

vasild left a comment

sr-gi commented Oct 30, 2023

jonatack commented Oct 30, 2023 •

edited

jonatack commented Oct 31, 2023

kashifs commented Nov 2, 2023

mzumsande left a comment

sr-gi commented Nov 3, 2023

kashifs commented Nov 4, 2023 •

edited

sr-gi commented Nov 4, 2023

vasild left a comment

vasild Nov 14, 2023

jonatack Nov 14, 2023

vasild Nov 15, 2023

net: improves addnode / m_added_nodes logic #28155

net: improves addnode / m_added_nodes logic #28155

Conversation

sr-gi commented Jul 25, 2023

Rationale

Connecting to the same node more than once

Adding the same peer with two different, yet equivalent, addresses

Parametrize CConnman::GetAddedNodeInfo

DrahtBot commented Jul 25, 2023 • edited

Code Coverage

Reviews

Conflicts

sr-gi commented Jul 25, 2023

kevkevinpal commented Jul 26, 2023

sr-gi commented Jul 28, 2023

sr-gi commented Jul 28, 2023

jonatack commented Aug 12, 2023 • edited

vasild left a comment

Choose a reason for hiding this comment

sr-gi commented Aug 23, 2023

vasild left a comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

sr-gi commented Oct 30, 2023

jonatack commented Oct 30, 2023 • edited

jonatack commented Oct 31, 2023

kashifs commented Nov 2, 2023

mzumsande left a comment

Choose a reason for hiding this comment

sr-gi commented Nov 3, 2023

kashifs commented Nov 4, 2023 • edited

sr-gi commented Nov 4, 2023

vasild left a comment

Choose a reason for hiding this comment

vasild Nov 14, 2023

Choose a reason for hiding this comment

jonatack Nov 14, 2023

Choose a reason for hiding this comment

vasild Nov 15, 2023

Choose a reason for hiding this comment

Parametrize `CConnman::GetAddedNodeInfo`

DrahtBot commented Jul 25, 2023 •

edited

jonatack commented Aug 12, 2023 •

edited

jonatack commented Oct 30, 2023 •

edited

kashifs commented Nov 4, 2023 •

edited