net: remove unnecessary check from AlreadyConnectedToAddress()

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

CConnman::AlreadyConnectedToAddress() searches the existent nodes by address or by address-and-port:

0FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort())

but:

if there is a match by just the address, then the address-and-port search will not be evaluated and the whole condition will be true
if the there is no node with the same address, then the second search by address-and-port will not find a match either.

The search by address-and-port is comparing against CNode::m_addr_name which could be a hostname, e.g. "node.foobar.com:8333", but addr.ToStringAddrPort() is always going to be numeric.

In other words: let A be “CNetAddr equals” and B be “addr:port string matches”, then:

If A (is true), then B is irrelevant, so the condition A || B is equivalent to A is true.
Observation in this PR: if !A (A is false), then !B for sure, thus the condition A || B is equivalent to A is false.

So, simplify A || B to A.

https://en.wikipedia.org/wiki/Modus_tollens !A => !B is equivalent to B => A. So the added fuzz test asserts that if B is true, then A is true.

DrahtBot commented at 2:37 pm on April 24, 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/32338.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theuni, mzumsande, davidgumberg, achow101
Concept ACK	jonatack
Approach ACK	shahsb

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:

#32326 (net: improve the interface around FindNode() and avoid a recursive mutex lock 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.

DrahtBot added the label P2P on Apr 24, 2025

mzumsande commented at 9:24 pm on April 24, 2025: contributor

This being correct relies on the assumption that if ToStringAddrPort is equal for two addresses, these addresses must also be equal on the level of CNetAddr - I’m pretty sure that this is the case today, but could we add a fuzz test asserting this to fuzz/netaddress.cpp?

davidgumberg commented at 11:34 pm on April 24, 2025: contributor

Concept ACK

It would also be good (maybe sufficient) to add a unit test which enforces/documents the behavior that a peer with the same address as an existing peer and a different peer will be seen as already connected to. e.g.:

 0diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp
 1index e60ce8b99d..4441a8dde4 100644
 2--- a/src/test/net_peer_connection_tests.cpp
 3+++ b/src/test/net_peer_connection_tests.cpp
 4@@ -155,6 +155,13 @@ BOOST_FIXTURE_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection,
 5         BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr));
 6     }
 7
 8+    BOOST_TEST_MESSAGE("\nCheck that peers with the same addresses as connected peers and different peers are detected as connected.");
 9+    for (auto node : connman->TestNodes()) {
10+        uint16_t changed_port = node->addr.GetPort() + 1;
11+        CService address_with_changed_port{node->addr, changed_port};
12+        BOOST_CHECK(connman->AlreadyConnectedPublic(CAddress{address_with_changed_port, NODE_NONE}));
13+    }
14+
15     // Clean up
16     for (auto node : connman->TestNodes()) {
17         peerman->FinalizeNode(*node);

(https://github.com/davidgumberg/bitcoin/commit/8dc032cf711885df1277bb03f753ae887b45943e)

net: remove unnecessary check from AlreadyConnectedToAddress()

`CConnman::AlreadyConnectedToAddress()` searches the existent nodes by
address or by address-and-port:

```cpp
FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort())
```

but:

* if there is a match by just the address, then the address-and-port
  search will not be evaluated and the whole condition will be `true`
* if the there is no node with the same address, then the second search
  by address-and-port will not find a match either.

The search by address-and-port is comparing against `CNode::m_addr_name`
which could be a hostname, e.g. `"node.foobar.com:8333"`, but
`addr.ToStringAddrPort()` is always going to be numeric.

94e85a82a7

test: Same addr, diff port is already connected f1b142856a

vasild force-pushed on Apr 25, 2025

vasild commented at 1:22 pm on April 25, 2025: contributor

0eac006a00...f1b142856a: address suggestions - add fuzz test and an unit test.

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

Looking over the old code, I believe it used to be true that -connect, -addnode, etc. used m_addr_name (and a dummy for addr) because they may have required a dns lookup. Which would have meant that checking both was required. There was also the case where we were connecting to an address (which may or may not need to be resolved) through a proxy.

These days I believe we’re smarter and detect addresses which don’t need to be resolved.

I’m pretty sure that addr should match m_addr_name now in the ip:port case, and we’d want to know if that didn’t hold for some reason.

So, concept ACK unless anyone can come up with a reason why these might differ in the case of an ip/port.

in src/net.cpp:370 in f1b142856a

366@@ -367,7 +367,7 @@ CNode* CConnman::FindNode(const CService& addr)
367 
368 bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
369 {
370-    return FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort());
371+    return FindNode(static_cast<CNetAddr>(addr));

jonatack commented at 8:09 pm on April 25, 2025:

we’d want to know if that didn’t hold for some reason

Wonder if the code (for debug builds) and/or the fuzz test should assert they are the same.

vasild commented at 8:08 am on April 28, 2025:

The added fuzz test does assert that if addr:port equals then CNetAddr must also equal. Could add Assume() here, something like:

0bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
1{
2    const bool match{FindNode(static_cast<CNetAddr>(addr))};
3    if (!match) {
4        Assume(!FindNode(addr.ToStringAddrPort()));
5    }
6    return match;
7}

I think that is a kind of an overkill, given the fuzz and unit tests added, but if reviewers want it, then I will add that.

theuni commented at 4:33 pm on April 28, 2025:

I agree this is overkill.

jonatack commented at 8:09 pm on April 25, 2025: member

Concept ACK

shahsb commented at 11:35 am on April 27, 2025: none

Concept ACK

shahsb commented at 11:35 am on April 27, 2025: none

Approach ACK

theuni approved

theuni commented at 4:36 pm on April 28, 2025: member

utACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff

As I mentioned above, I believe this was needed at one point when connecting to an IP through a proxy, or when using addnode for an IP, as those were stored as strings rather than resolved addresses.

But now that we always attempt to resolve ips, m_addr_name is only interesting for non-ip connections. So the check here should be redundant and unnecessary.

DrahtBot requested review from davidgumberg on Apr 28, 2025

DrahtBot requested review from shahsb on Apr 28, 2025

DrahtBot requested review from jonatack on Apr 28, 2025

mzumsande commented at 3:43 pm on April 29, 2025: contributor

Code Review ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff

davidgumberg commented at 7:13 pm on April 29, 2025: contributor

crACK f1b142856a4ecd0a0d90bc3d

To rephrase the justification in 94e85a82a753a0aa’s commit message and the PR description, because the first check (FindNode(CNetAddr(addr))), is indifferent to addr’s port (CNetAddr has no port member.), if the first, less specific check for just the address fails, the second more specific check for the address and port will always fail.

I ran the fuzz harness modified here for a few minutes and didn’t observe any issues.

achow101 commented at 9:22 pm on April 29, 2025: member

ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff

achow101 merged this on Apr 29, 2025

achow101 closed this on Apr 29, 2025

jonatack commented at 10:38 pm on April 29, 2025: member

Post-merge ACK

pablomartin4btc referenced this in commit 16b1645e70 on Apr 30, 2025

pablomartin4btc referenced this in commit 5748f4aba2 on Apr 30, 2025

vasild deleted the branch on May 1, 2025

net: remove unnecessary check from AlreadyConnectedToAddress() #32338

Code Coverage & Benchmarks

Reviews

Conflicts