Merge #6062: feat: Let addrman handle multiple ports for all networks

76b4300fdf fix: Let addrman handle multiple ports for all networks (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  There is really no need to do have this restriction in `addrman`, we check `AllowMultiplePorts()` in `connman` which should be enough already. We can safely re-align `addrman` to its upstream implementation as suggested in #6043.

  ## What was done?
  Drop port "discrimination" in `addrman`, remove related tests.

  ## How Has This Been Tested?
  Run tests, run dash-qt on mainnet/testnet

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    ACK 76b4300fdf
  PastaPastaPasta:
    utACK 76b4300fdf422097b0b39460e80ee4da98247f03; is it really a fix or a refactor? doesn't change observable system behavior
  knst:
    utACK 76b4300fdf

Tree-SHA512: 50363b5de7a6c6ad4de18c08b0a5cc31e89be8e5304f9684ce2cc609df4de07ff6d8d010556b5f6651c698e1a86960dba8005cc26fdb00bd03c856752d3e06ef

src/addrman.cpp

@@ -99,12 +99,11 @@ double AddrInfo::GetChance(int64_t nNow) const
     return fChance;
 }
 
-AddrManImpl::AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports)
+AddrManImpl::AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio)
     : insecure_rand{deterministic}
     , nKey{deterministic ? uint256{1} : insecure_rand.rand256()}
     , m_consistency_check_ratio{consistency_check_ratio}
     , m_asmap{std::move(asmap)}
-    , m_discriminate_ports{discriminate_ports}
 {
     for (auto& bucket : vvNew) {
         for (auto& entry : bucket) {
@@ -400,12 +399,7 @@ void AddrManImpl::Unserialize(Stream& s_)
 
 AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId)
 {
-    CService addr2 = addr;
+    const auto it = mapAddr.find(addr);
-    if (!m_discriminate_ports) {
-        addr2.SetPort(0);
-    }
-
-    const auto it = mapAddr.find(addr2);
     if (it == mapAddr.end())
         return nullptr;
     if (pnId)
@@ -418,15 +412,11 @@ AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId)
 
 AddrInfo* AddrManImpl::Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId)
 {
-    CService addr2 = addr;
-    if (!m_discriminate_ports) {
-        addr2.SetPort(0);
-    }
     AssertLockHeld(cs);
 
     int nId = nIdCount++;
     mapInfo[nId] = AddrInfo(addr, addrSource);
-    mapAddr[addr2] = nId;
+    mapAddr[addr] = nId;
     mapInfo[nId].nRandomPos = vRandom.size();
     vRandom.push_back(nId);
     if (pnId)
@@ -467,14 +457,9 @@ void AddrManImpl::Delete(int nId)
     assert(!info.fInTried);
     assert(info.nRefCount == 0);
 
-    CService addr = info;
-    if (!m_discriminate_ports) {
-        addr.SetPort(0);
-    }
-
     SwapRandom(info.nRandomPos, vRandom.size() - 1);
     vRandom.pop_back();
-    mapAddr.erase(addr);
+    mapAddr.erase(info);
     mapInfo.erase(nId);
     nNew--;
 }
@@ -645,10 +630,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 (!m_discriminate_ports && info != addr)
-        return;
-
     // update info
     info.nLastSuccess = nTime;
     info.nLastTry = nTime;
@@ -716,10 +697,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 (!m_discriminate_ports && info != addr)
-        return;
-
     // update info
     info.nLastTry = nTime;
     if (fCountFailure && info.nLastCountAttempt < nLastGood) {
@@ -847,10 +824,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 (!m_discriminate_ports && info != addr)
-        return;
-
     // update info
     int64_t nUpdateInterval = 20 * 60;
     if (nTime - info.nTime > nUpdateInterval)
@@ -869,10 +842,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 (!m_discriminate_ports && info != addr)
-        return;
-
     // update info
     info.nServices = nServices;
 }
@@ -885,12 +854,6 @@ AddrInfo AddrManImpl::GetAddressInfo_(const CService& addr)
     if (!pinfo)
         return AddrInfo();
 
-    AddrInfo& info = *pinfo;
-
-    // check whether we are talking about the exact same CService (including same port)
-    if (!m_discriminate_ports && info != addr)
-        return AddrInfo();
-
     return *pinfo;
 }
 
@@ -1187,8 +1150,8 @@ const std::vector<bool>& AddrManImpl::GetAsmap() const
     return m_asmap;
 }
 
-AddrMan::AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports)
+AddrMan::AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio)
-    : m_impl(std::make_unique<AddrManImpl>(std::move(asmap), deterministic, consistency_check_ratio, discriminate_ports)) {}
+    : m_impl(std::make_unique<AddrManImpl>(std::move(asmap), deterministic, consistency_check_ratio)) {}
 
 AddrMan::~AddrMan() = default;
 

src/addrman.h

@@ -57,7 +57,7 @@ class AddrMan
     const std::unique_ptr<AddrManImpl> m_impl;
 
 public:
-    explicit AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports = false);
+    explicit AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio);
 
     ~AddrMan();
 

src/addrman_impl.h

@@ -99,7 +99,7 @@ public:
 class AddrManImpl
 {
 public:
-    AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports);
+    AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio);
 
     ~AddrManImpl();
 
@@ -227,9 +227,6 @@ private:
     // would be re-bucketed accordingly.
     const std::vector<bool> m_asmap;
 
-    //! Discriminate entries based on port.
-    bool m_discriminate_ports GUARDED_BY(cs);
-
     //! Find an entry.
     AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 

src/netaddress.cpp

@@ -1056,11 +1056,6 @@ std::string CService::ToString() const
     return ToStringIPPort();
 }
 
-void CService::SetPort(uint16_t portIn)
-{
-    port = portIn;
-}
-
 CSubNet::CSubNet():
     valid(false)
 {

src/netaddress.h

@@ -544,7 +544,6 @@ public:
     CService(const CNetAddr& ip, uint16_t port);
     CService(const struct in_addr& ipv4Addr, uint16_t port);
     explicit CService(const struct sockaddr_in& addr);
-    void SetPort(uint16_t portIn);
     uint16_t GetPort() const;
     bool GetSockAddr(struct sockaddr* paddr, socklen_t *addrlen) const;
     bool SetSockAddr(const struct sockaddr* paddr);

src/test/addrman_tests.cpp

@@ -26,7 +26,7 @@ public:
     virtual void Serialize(CDataStream& s) const = 0;
 
     AddrManSerializationMock()
-        : AddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100, /* discriminate_ports */ true)
+        : AddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100)
     {}
 };
 
@@ -82,9 +82,8 @@ private:
 
 public:
     explicit AddrManTest(bool makeDeterministic = true,
-                         std::vector<bool> asmap = std::vector<bool>(),
+                         std::vector<bool> asmap = std::vector<bool>())
-                         bool discriminatePorts = true)
+        : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100)
-        : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100, discriminatePorts)
     {
         deterministic = makeDeterministic;
     }
@@ -236,34 +235,6 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
     BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
 }
 
-BOOST_AUTO_TEST_CASE(addrman_ports_nondiscriminate)
-{
-    AddrManTest addrman(/* deterministic */ true, /* asmap */ std::vector<bool>(), /* discriminate */ false);
-
-    CNetAddr source = ResolveIP("252.2.2.2");
-
-    BOOST_CHECK_EQUAL(addrman.size(), 0U);
-
-    // Test 7; Addr with same IP but diff port does not replace existing addr.
-    CService addr1 = ResolveService("250.1.1.1", 8333);
-    BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source));
-    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);
-    auto addr_ret2 = addrman.Select().first;
-    BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333");
-
-    // 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.
-    addrman.Good(CAddress(addr1_port, NODE_NONE));
-    BOOST_CHECK_EQUAL(addrman.size(), 1U);
-    bool newOnly = true;
-    auto addr_ret3 = addrman.Select(newOnly).first;
-    BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
-}
-
 BOOST_AUTO_TEST_CASE(addrman_select)
 {
     AddrManTest addrman;
@@ -415,39 +386,6 @@ BOOST_AUTO_TEST_CASE(addrman_find)
     BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333");
 }
 
-BOOST_AUTO_TEST_CASE(addrman_find_nondiscriminate)
-{
-    AddrManTest addrman(/* deterministic */ true, /* asmap */ std::vector<bool>(), /* discriminate */ false);
-
-    BOOST_CHECK_EQUAL(addrman.size(), 0U);
-
-    CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE);
-    CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE);
-    CAddress addr3 = CAddress(ResolveService("251.255.2.1", 8333), NODE_NONE);
-
-    CNetAddr source1 = ResolveIP("250.1.2.1");
-    CNetAddr source2 = ResolveIP("250.1.2.2");
-
-    BOOST_CHECK(addrman.Add({addr1}, source1));
-    BOOST_CHECK(!addrman.Add({addr2}, source2));
-    BOOST_CHECK(addrman.Add({addr3}, source1));
-
-    // Test: ensure Find returns an IP 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.
-    AddrInfo* info2 = addrman.Find(addr2);
-    BOOST_REQUIRE(info2);
-    BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString());
-
-    // Test: Find returns another IP matching what we searched on.
-    AddrInfo* info3 = addrman.Find(addr3);
-    BOOST_REQUIRE(info3);
-    BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333");
-}
-
 BOOST_AUTO_TEST_CASE(addrman_create)
 {
     AddrManTest addrman;