Merge bitcoin/bitcoin#32338: net: remove unnecessary check from AlreadyConnectedToAddress()

f1b142856a4ecd0a0d90bc3d73ef5137997b14ff test: Same addr, diff port is already connected (David Gumberg)
94e85a82a753a0aa5ad688fc46330e83c9a697fe net: remove unnecessary check from AlreadyConnectedToAddress() (Vasil Dimov)

Pull request description:

  `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.

  ---

  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`.

ACKs for top commit:
  davidgumberg:
    crACK f1b142856a4ecd0a0d90bc3d
  achow101:
    ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
  theuni:
    utACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
  mzumsande:
    Code Review ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff

Tree-SHA512: d744b60e9bace121faa3a746463f6b6e0e6ef08eac0e7879326cbd5f4721e47e6e10f6203dfd3870a2057c4ddd1860692c070ef048a76d773b84e6c2f840cc86

src/net.cpp

@@ -367,7 +367,7 @@ CNode* CConnman::FindNode(const CService& addr)
 
 bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
 {
-    return FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort());
+    return FindNode(static_cast<CNetAddr>(addr));
 }
 
 bool CConnman::CheckIncomingNonce(uint64_t nonce)

src/test/fuzz/netaddress.cpp

@@ -101,10 +101,14 @@ FUZZ_TARGET(netaddress)
     (void)net_addr.GetReachabilityFrom(other_net_addr);
     (void)sub_net.Match(other_net_addr);
 
-    const CService other_service{net_addr, fuzzed_data_provider.ConsumeIntegral<uint16_t>()};
+    const CService other_service{fuzzed_data_provider.ConsumeBool() ? net_addr : other_net_addr, fuzzed_data_provider.ConsumeIntegral<uint16_t>()};
     assert((service == other_service) != (service != other_service));
     (void)(service < other_service);
 
+    if (service.ToStringAddrPort() == other_service.ToStringAddrPort()) {
+        assert(static_cast<CNetAddr>(service) == static_cast<CNetAddr>(other_service));
+    }
+
     const CSubNet sub_net_copy_1{net_addr, other_net_addr};
     const CSubNet sub_net_copy_2{net_addr};
 

src/test/net_peer_connection_tests.cpp

@@ -155,6 +155,13 @@ BOOST_FIXTURE_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection,
         BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr));
     }
 
+    BOOST_TEST_MESSAGE("\nCheck that peers with the same addresses as connected peers but different ports are detected as connected.");
+    for (auto node : connman->TestNodes()) {
+        uint16_t changed_port = node->addr.GetPort() + 1;
+        CService address_with_changed_port{node->addr, changed_port};
+        BOOST_CHECK(connman->AlreadyConnectedPublic(CAddress{address_with_changed_port, NODE_NONE}));
+    }
+
     // Clean up
     for (auto node : connman->TestNodes()) {
         peerman->FinalizeNode(*node);