Merge bitcoin#23306: Make AddrMan support multiple ports per IP

92617b7 Make AddrMan support multiple ports per IP (Pieter Wuille)

Pull request description:

  For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.

  The folklore justification (eventually actually added as a comment to the codebase in bitcoin#20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in bitcoin#5150 (comment) e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).

  And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.

  One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket _already_ uses the port number, the only change required is making all addrman lookup be (IP,port) (aka `CService`) based, rather than IP (aka `CNetAddr`) based.

  This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.

ACKs for top commit:
  jnewbery:
    Code review ACK 92617b7
  naumenkogs:
    ACK 92617b7
  ajtowns:
    ACK 92617b7
  vasild:
    ACK 92617b7

Tree-SHA512: 9eef06ce97a8b54a3f05fb8acf6941f253a9a5e0be8ce383dd05c44bb567cea243b74ee5667178e7497f6df2db93adab97ac66edbc37c883fd8ec840ee69a33f

Author: MarcoFalke

src/addrman.cpp

@@ -395,7 +395,7 @@ void AddrManImpl::Unserialize(Stream& s_)
     }
 }
 
-AddrInfo* AddrManImpl::Find(const CNetAddr& addr, int* pnId)
+AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId)
 {
     AssertLockHeld(cs);
 
@@ -553,10 +553,6 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
 
     AddrInfo& info = *pinfo;
 
-    // check whether we are talking about the exact same CService (including same port)
-    if (info != addr)
-        return;
-
     // update info
     info.nLastSuccess = nTime;
     info.nLastTry = nTime;
@@ -685,10 +681,6 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi
 
     AddrInfo& info = *pinfo;
 
-    // check whether we are talking about the exact same CService (including same port)
-    if (info != addr)
-        return;
-
     // update info
     info.nLastTry = nTime;
     if (fCountFailure && info.nLastCountAttempt < nLastGood) {
@@ -816,10 +808,6 @@ void AddrManImpl::Connected_(const CService& addr, int64_t nTime)
 
     AddrInfo& info = *pinfo;
 
-    // check whether we are talking about the exact same CService (including same port)
-    if (info != addr)
-        return;
-
     // update info
     int64_t nUpdateInterval = 20 * 60;
     if (nTime - info.nTime > nUpdateInterval)
@@ -838,10 +826,6 @@ void AddrManImpl::SetServices_(const CService& addr, ServiceFlags nServices)
 
     AddrInfo& info = *pinfo;
 
-    // check whether we are talking about the exact same CService (including same port)
-    if (info != addr)
-        return;
-
     // update info
     info.nServices = nServices;
 }

src/addrman_impl.h

@@ -179,8 +179,8 @@ class AddrManImpl
     //! table with information about all nIds
     std::unordered_map<int, AddrInfo> mapInfo GUARDED_BY(cs);
 
-    //! find an nId based on its network address
-    std::unordered_map<CNetAddr, int, CNetAddrHash> mapAddr GUARDED_BY(cs);
+    //! find an nId based on its network address and port.
+    std::unordered_map<CService, int, CServiceHash> mapAddr GUARDED_BY(cs);
 
     //! randomly-ordered vector of all nIds
     //! This is mutable because it is unobservable outside the class, so any
@@ -225,7 +225,7 @@ class AddrManImpl
     const std::vector<bool> m_asmap;
 
     //! Find an entry.
-    AddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Create a new entry and add it to the internal data structures mapInfo, mapAddr and vRandom.
     AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);

src/netaddress.h

@@ -253,7 +253,6 @@ class CNetAddr
         }
     }
 
-    friend class CNetAddrHash;
     friend class CSubNet;
 
 private:
@@ -467,22 +466,6 @@ class CNetAddr
     }
 };
 
-class CNetAddrHash
-{
-public:
-    size_t operator()(const CNetAddr& a) const noexcept
-    {
-        CSipHasher hasher(m_salt_k0, m_salt_k1);
-        hasher.Write(a.m_net);
-        hasher.Write(a.m_addr.data(), a.m_addr.size());
-        return static_cast<size_t>(hasher.Finalize());
-    }
-
-private:
-    const uint64_t m_salt_k0 = GetRand(std::numeric_limits<uint64_t>::max());
-    const uint64_t m_salt_k1 = GetRand(std::numeric_limits<uint64_t>::max());
-};
-
 class CSubNet
 {
 protected:
@@ -565,6 +548,25 @@ class CService : public CNetAddr
         READWRITEAS(CNetAddr, obj);
         READWRITE(Using<BigEndianFormatter<2>>(obj.port));
     }
+
+    friend class CServiceHash;
+};
+
+class CServiceHash
+{
+public:
+    size_t operator()(const CService& a) const noexcept
+    {
+        CSipHasher hasher(m_salt_k0, m_salt_k1);
+        hasher.Write(a.m_net);
+        hasher.Write(a.port);
+        hasher.Write(a.m_addr.data(), a.m_addr.size());
+        return static_cast<size_t>(hasher.Finalize());
+    }
+
+private:
+    const uint64_t m_salt_k0 = GetRand(std::numeric_limits<uint64_t>::max());
+    const uint64_t m_salt_k1 = GetRand(std::numeric_limits<uint64_t>::max());
 };
 
 #endif // BITCOIN_NETADDRESS_H

src/test/addrman_tests.cpp

@@ -89,7 +89,7 @@ class AddrManTest : public AddrMan
         deterministic = makeDeterministic;
     }
 
-    AddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr)
+    AddrInfo* Find(const CService& addr, int* pnId = nullptr)
     {
         LOCK(m_impl->cs);
         return m_impl->Find(addr, pnId);
@@ -222,15 +222,15 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
     BOOST_CHECK_EQUAL(addrman.size(), 1U);
 
     CService addr1_port = ResolveService("250.1.1.1", 8334);
-    BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source));
-    BOOST_CHECK_EQUAL(addrman.size(), 1U);
+    BOOST_CHECK(addrman.Add({CAddress(addr1_port, NODE_NONE)}, source));
+    BOOST_CHECK_EQUAL(addrman.size(), 2U);
     auto addr_ret2 = addrman.Select().first;
-    BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333");
+    BOOST_CHECK(addr_ret2.ToString() == "250.1.1.1:8333" || addr_ret2.ToString() == "250.1.1.1:8334");
 
-    // Test: Add same IP but diff port to tried table, it doesn't get added.
-    //  Perhaps this is not ideal behavior but it is the current behavior.
+    // Test: Add same IP but diff port to tried table; this converts the entry with
+    // the specified port to tried, but not the other.
     addrman.Good(CAddress(addr1_port, NODE_NONE));
-    BOOST_CHECK_EQUAL(addrman.size(), 1U);
+    BOOST_CHECK_EQUAL(addrman.size(), 2U);
     bool newOnly = true;
     auto addr_ret3 = addrman.Select(newOnly).first;
     BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
@@ -369,18 +369,18 @@ BOOST_AUTO_TEST_CASE(addrman_find)
     CNetAddr source2 = ResolveIP("250.1.2.2");
 
     BOOST_CHECK(addrman.Add({addr1}, source1));
-    BOOST_CHECK(!addrman.Add({addr2}, source2));
+    BOOST_CHECK(addrman.Add({addr2}, source2));
     BOOST_CHECK(addrman.Add({addr3}, source1));
 
-    // Test: ensure Find returns an IP matching what we searched on.
+    // Test: ensure Find returns an IP/port matching what we searched on.
     AddrInfo* info1 = addrman.Find(addr1);
     BOOST_REQUIRE(info1);
     BOOST_CHECK_EQUAL(info1->ToString(), "250.1.2.1:8333");
 
-    // Test 18; Find does not discriminate by port number.
+    // Test; Find discriminates by port number.
     AddrInfo* info2 = addrman.Find(addr2);
     BOOST_REQUIRE(info2);
-    BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString());
+    BOOST_CHECK_EQUAL(info2->ToString(), "250.1.2.1:9999");
 
     // Test: Find returns another IP matching what we searched on.
     AddrInfo* info3 = addrman.Find(addr3);

src/test/fuzz/addrman.cpp

@@ -137,24 +137,29 @@ class AddrManDeterministic : public AddrMan
         // Check that all values in `mapInfo` are equal to all values in `other.mapInfo`.
         // Keys may be different.
 
-        using AddrInfoHasher = std::function<size_t(const AddrInfo&)>;
-        using AddrInfoEq = std::function<bool(const AddrInfo&, const AddrInfo&)>;
-
-        CNetAddrHash netaddr_hasher;
-
-        AddrInfoHasher addrinfo_hasher = [&netaddr_hasher](const AddrInfo& a) {
-            return netaddr_hasher(static_cast<CNetAddr>(a)) ^ netaddr_hasher(a.source) ^
-                   a.nLastSuccess ^ a.nAttempts ^ a.nRefCount ^ a.fInTried;
+        auto addrinfo_hasher = [](const AddrInfo& a) {
+            CSipHasher hasher(0, 0);
+            auto addr_key = a.GetKey();
+            auto source_key = a.source.GetAddrBytes();
+            hasher.Write(a.nLastSuccess);
+            hasher.Write(a.nAttempts);
+            hasher.Write(a.nRefCount);
+            hasher.Write(a.fInTried);
+            hasher.Write(a.GetNetwork());
+            hasher.Write(a.source.GetNetwork());
+            hasher.Write(addr_key.size());
+            hasher.Write(source_key.size());
+            hasher.Write(addr_key.data(), addr_key.size());
+            hasher.Write(source_key.data(), source_key.size());
+            return (size_t)hasher.Finalize();
         };
 
-        AddrInfoEq addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) {
-            return static_cast<CNetAddr>(lhs) == static_cast<CNetAddr>(rhs) &&
-                   lhs.source == rhs.source && lhs.nLastSuccess == rhs.nLastSuccess &&
-                   lhs.nAttempts == rhs.nAttempts && lhs.nRefCount == rhs.nRefCount &&
-                   lhs.fInTried == rhs.fInTried;
+        auto addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) {
+            return std::tie(static_cast<const CService&>(lhs), lhs.source, lhs.nLastSuccess, lhs.nAttempts, lhs.nRefCount, lhs.fInTried) ==
+                   std::tie(static_cast<const CService&>(rhs), rhs.source, rhs.nLastSuccess, rhs.nAttempts, rhs.nRefCount, rhs.fInTried);
         };
 
-        using Addresses = std::unordered_set<AddrInfo, AddrInfoHasher, AddrInfoEq>;
+        using Addresses = std::unordered_set<AddrInfo, decltype(addrinfo_hasher), decltype(addrinfo_eq)>;
 
         const size_t num_addresses{m_impl->mapInfo.size()};