[net, addrman] Remove external dependencies on CAddrInfo objects

CAddrInfo objects are an implementation detail of how AddrMan manages and adds
metadata to different records. Encapsulate this logic by updating Select &
SelectTriedCollision to return the additional info that the callers need.

6 files changed, 70 insertions, 60 deletions

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 2fc6ca29a1..324bab7292 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -694,15 +694,13 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi
     }
 }
 
-CAddrInfo AddrManImpl::Select_(bool newOnly) const
+std::pair<CAddress, int64_t> AddrManImpl::Select_(bool newOnly) const
 {
     AssertLockHeld(cs);
 
-    if (vRandom.empty())
-        return CAddrInfo();
+    if (vRandom.empty()) return {};
 
-    if (newOnly && nNew == 0)
-        return CAddrInfo();
+    if (newOnly && nNew == 0) return {};
 
     // Use a 50% chance for choosing between tried and new table entries.
     if (!newOnly &&
@@ -720,8 +718,9 @@ CAddrInfo AddrManImpl::Select_(bool newOnly) const
             const auto it_found{mapInfo.find(nId)};
             assert(it_found != mapInfo.end());
             const CAddrInfo& info{it_found->second};
-            if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30))
-                return info;
+            if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) {
+                return {info, info.nLastTry};
+            }
             fChanceFactor *= 1.2;
         }
     } else {
@@ -738,8 +737,9 @@ CAddrInfo AddrManImpl::Select_(bool newOnly) const
             const auto it_found{mapInfo.find(nId)};
             assert(it_found != mapInfo.end());
             const CAddrInfo& info{it_found->second};
-            if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30))
-                return info;
+            if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) {
+                return {info, info.nLastTry};
+            }
             fChanceFactor *= 1.2;
         }
     }
@@ -883,11 +883,11 @@ void AddrManImpl::ResolveCollisions_()
     }
 }
 
-CAddrInfo AddrManImpl::SelectTriedCollision_()
+std::pair<CAddress, int64_t> AddrManImpl::SelectTriedCollision_()
 {
     AssertLockHeld(cs);
 
-    if (m_tried_collisions.size() == 0) return CAddrInfo();
+    if (m_tried_collisions.size() == 0) return {};
 
     std::set<int>::iterator it = m_tried_collisions.begin();
 
@@ -898,7 +898,7 @@ CAddrInfo AddrManImpl::SelectTriedCollision_()
     // If id_new not found in mapInfo remove it from m_tried_collisions
     if (mapInfo.count(id_new) != 1) {
         m_tried_collisions.erase(it);
-        return CAddrInfo();
+        return {};
     }
 
     const CAddrInfo& newInfo = mapInfo[id_new];
@@ -907,9 +907,8 @@ CAddrInfo AddrManImpl::SelectTriedCollision_()
     int tried_bucket = newInfo.GetTriedBucket(nKey, m_asmap);
     int tried_bucket_pos = newInfo.GetBucketPosition(nKey, false, tried_bucket);
 
-    int id_old = vvTried[tried_bucket][tried_bucket_pos];
-
-    return mapInfo[id_old];
+    const CAddrInfo& info_old = mapInfo[vvTried[tried_bucket][tried_bucket_pos]];
+    return {info_old, info_old.nLastTry};
 }
 
 void AddrManImpl::Check() const
@@ -1059,20 +1058,20 @@ void AddrManImpl::ResolveCollisions()
     Check();
 }
 
-CAddrInfo AddrManImpl::SelectTriedCollision()
+std::pair<CAddress, int64_t> AddrManImpl::SelectTriedCollision()
 {
     LOCK(cs);
     Check();
-    const CAddrInfo ret = SelectTriedCollision_();
+    const auto ret = SelectTriedCollision_();
     Check();
     return ret;
 }
 
-CAddrInfo AddrManImpl::Select(bool newOnly) const
+std::pair<CAddress, int64_t> AddrManImpl::Select(bool newOnly) const
 {
     LOCK(cs);
     Check();
-    const CAddrInfo addrRet = Select_(newOnly);
+    const auto addrRet = Select_(newOnly);
     Check();
     return addrRet;
 }
@@ -1159,12 +1158,12 @@ void CAddrMan::ResolveCollisions()
     m_impl->ResolveCollisions();
 }
 
-CAddrInfo CAddrMan::SelectTriedCollision()
+std::pair<CAddress, int64_t> CAddrMan::SelectTriedCollision()
 {
     return m_impl->SelectTriedCollision();
 }
 
-CAddrInfo CAddrMan::Select(bool newOnly) const
+std::pair<CAddress, int64_t> CAddrMan::Select(bool newOnly) const
 {
     return m_impl->Select(newOnly);
 }
diff --git a/src/addrman.h b/src/addrman.h
index 2135295b89..d176d0a42c 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -171,13 +171,23 @@ public:
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
     void ResolveCollisions();
 
-    //! Randomly select an address in tried that another address is attempting to evict.
-    CAddrInfo SelectTriedCollision();
+    /**
+     * Randomly select an address in the tried table that another address is
+     * attempting to evict.
+     *
+     * @return CAddress The record for the selected tried peer.
+     *         int64_t  The last time we attempted to connect to that peer.
+     */
+    std::pair<CAddress, int64_t> SelectTriedCollision();
 
     /**
      * Choose an address to connect to.
+     *
+     * @param[in] newOnly  Whether to only select addresses from the new table.
+     * @return    CAddress The record for the selected peer.
+     *            int64_t  The last time we attempted to connect to that peer.
      */
-    CAddrInfo Select(bool newOnly = false) const;
+    std::pair<CAddress, int64_t> Select(bool newOnly = false) const;
 
     /**
      * Return all or many randomly selected addresses, optionally by network.
diff --git a/src/addrman_impl.h b/src/addrman_impl.h
index ee4b55e5c4..6752d5b81d 100644
--- a/src/addrman_impl.h
+++ b/src/addrman_impl.h
@@ -31,9 +31,9 @@ public:
 
     void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    CAddrInfo SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    std::pair<CAddress, int64_t> SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    CAddrInfo Select(bool newOnly) const
+    std::pair<CAddress, int64_t>  Select(bool newOnly) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
@@ -161,7 +161,7 @@ private:
     void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Select an address to connect to, if newOnly is set to true, only the new table is selected from.
-    CAddrInfo Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    std::pair<CAddress, int64_t> Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /**
      * Return all or many randomly selected addresses, optionally by network.
@@ -193,7 +193,7 @@ private:
     void ResolveCollisions_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Return a random to-be-evicted tried table address.
-    CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
+    std::pair<CAddress, int64_t> SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Consistency check, taking into account m_consistency_check_ratio. Will std::abort if an inconsistency is detected.
     void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs);
diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
index bebf86a09d..b4c2e35f42 100644
--- a/src/bench/addrman.cpp
+++ b/src/bench/addrman.cpp
@@ -87,7 +87,7 @@ static void AddrManSelect(benchmark::Bench& bench)
 
     bench.run([&] {
         const auto& address = addrman.Select();
-        assert(address.GetPort() > 0);
+        assert(address.first.GetPort() > 0);
     });
 }
 
diff --git a/src/net.cpp b/src/net.cpp
index b8ff0b13ea..df3b88725e 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2006,17 +2006,18 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
             if (nTries > 100)
                 break;
 
-            CAddrInfo addr;
+            CAddress addr;
+            int64_t addr_last_try{0};
 
             if (fFeeler) {
                 // First, try to get a tried table collision address. This returns
                 // an empty (invalid) address if there are no collisions to try.
-                addr = addrman.SelectTriedCollision();
+                std::tie(addr, addr_last_try) = addrman.SelectTriedCollision();
 
                 if (!addr.IsValid()) {
                     // No tried table collisions. Select a new table address
                     // for our feeler.
-                    addr = addrman.Select(true);
+                    std::tie(addr, addr_last_try) = addrman.Select(true);
                 } else if (AlreadyConnectedToAddress(addr)) {
                     // If test-before-evict logic would have us connect to a
                     // peer that we're already connected to, just mark that
@@ -2025,11 +2026,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                     // a currently-connected peer.
                     addrman.Good(addr);
                     // Select a new table address for our feeler instead.
-                    addr = addrman.Select(true);
+                    std::tie(addr, addr_last_try) = addrman.Select(true);
                 }
             } else {
                 // Not a feeler
-                addr = addrman.Select();
+                std::tie(addr, addr_last_try) = addrman.Select();
             }
 
             // Require outbound connections, other than feelers, to be to distinct network groups
@@ -2046,7 +2047,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 continue;
 
             // only consider very recently tried nodes after 30 failed attempts
-            if (nANow - addr.nLastTry < 600 && nTries < 30)
+            if (nANow - addr_last_try < 600 && nTries < 30)
                 continue;
 
             // for non-feelers, require all the services we'll want,
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 41c298e036..022f60a8ed 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -172,14 +172,14 @@ BOOST_AUTO_TEST_CASE(addrman_simple)
 
     // Test: Does Addrman respond correctly when empty.
     BOOST_CHECK_EQUAL(addrman->size(), 0U);
-    CAddrInfo addr_null = addrman->Select();
+    auto addr_null = addrman->Select().first;
     BOOST_CHECK_EQUAL(addr_null.ToString(), "[::]:0");
 
     // Test: Does Addrman::Add work as expected.
     CService addr1 = ResolveService("250.1.1.1", 8333);
     BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source));
     BOOST_CHECK_EQUAL(addrman->size(), 1U);
-    CAddrInfo addr_ret1 = addrman->Select();
+    auto addr_ret1 = addrman->Select().first;
     BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333");
 
     // Test: Does IP address deduplication work correctly.
@@ -224,7 +224,7 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
     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);
-    CAddrInfo addr_ret2 = addrman.Select();
+    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.
@@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
     addrman.Good(CAddress(addr1_port, NODE_NONE));
     BOOST_CHECK_EQUAL(addrman.size(), 1U);
     bool newOnly = true;
-    CAddrInfo addr_ret3 = addrman.Select(newOnly);
+    auto addr_ret3 = addrman.Select(newOnly).first;
     BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
 }
 
@@ -249,16 +249,16 @@ BOOST_AUTO_TEST_CASE(addrman_select)
     BOOST_CHECK_EQUAL(addrman.size(), 1U);
 
     bool newOnly = true;
-    CAddrInfo addr_ret1 = addrman.Select(newOnly);
+    auto addr_ret1 = addrman.Select(newOnly).first;
     BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333");
 
     // Test: move addr to tried, select from new expected nothing returned.
     addrman.Good(CAddress(addr1, NODE_NONE));
     BOOST_CHECK_EQUAL(addrman.size(), 1U);
-    CAddrInfo addr_ret2 = addrman.Select(newOnly);
+    auto addr_ret2 = addrman.Select(newOnly).first;
     BOOST_CHECK_EQUAL(addr_ret2.ToString(), "[::]:0");
 
-    CAddrInfo addr_ret3 = addrman.Select();
+    auto addr_ret3 = addrman.Select().first;
     BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
 
     BOOST_CHECK_EQUAL(addrman.size(), 1U);
@@ -291,7 +291,7 @@ BOOST_AUTO_TEST_CASE(addrman_select)
     // Test: Select pulls from new and tried regardless of port number.
     std::set<uint16_t> ports;
     for (int i = 0; i < 20; ++i) {
-        ports.insert(addrman.Select().GetPort());
+        ports.insert(addrman.Select().first.GetPort());
     }
     BOOST_CHECK_EQUAL(ports.size(), 3U);
 }
@@ -869,7 +869,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision)
     BOOST_CHECK(addrman.size() == 0);
 
     // Empty addrman should return blank addrman info.
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 
     // Add twenty two addresses.
     CNetAddr source = ResolveIP("252.2.2.2");
@@ -880,7 +880,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision)
 
         // No collisions yet.
         BOOST_CHECK(addrman.size() == i);
-        BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+        BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
     }
 
     // Ensure Good handles duplicates well.
@@ -889,7 +889,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision)
         addrman.Good(addr);
 
         BOOST_CHECK(addrman.size() == 22);
-        BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+        BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
     }
 
 }
@@ -907,7 +907,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
 
         // No collision yet.
         BOOST_CHECK(addrman.size() == i);
-        BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+        BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
     }
 
     // Collision between 36 and 19.
@@ -916,11 +916,11 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
     addrman.Good(addr36);
 
     BOOST_CHECK(addrman.size() == 36);
-    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.19:0");
+    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.19:0");
 
     // 36 should be discarded and 19 not evicted.
     addrman.ResolveCollisions();
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 
     // Lets create two collisions.
     for (unsigned int i = 37; i < 59; i++) {
@@ -929,7 +929,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
         addrman.Good(addr);
 
         BOOST_CHECK(addrman.size() == i);
-        BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+        BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
     }
 
     // Cause a collision.
@@ -938,16 +938,16 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
     addrman.Good(addr59);
     BOOST_CHECK(addrman.size() == 59);
 
-    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.10:0");
+    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.10:0");
 
     // Cause a second collision.
     BOOST_CHECK(!addrman.Add({CAddress(addr36, NODE_NONE)}, source));
     addrman.Good(addr36);
     BOOST_CHECK(addrman.size() == 59);
 
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() != "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() != "[::]:0");
     addrman.ResolveCollisions();
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 }
 
 BOOST_AUTO_TEST_CASE(addrman_evictionworks)
@@ -957,7 +957,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
     BOOST_CHECK(addrman.size() == 0);
 
     // Empty addrman should return blank addrman info.
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 
     // Add 35 addresses
     CNetAddr source = ResolveIP("252.2.2.2");
@@ -968,7 +968,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
 
         // No collision yet.
         BOOST_CHECK(addrman.size() == i);
-        BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+        BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
     }
 
     // Collision between 36 and 19.
@@ -977,7 +977,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
     addrman.Good(addr);
 
     BOOST_CHECK_EQUAL(addrman.size(), 36);
-    CAddrInfo info = addrman.SelectTriedCollision();
+    auto info = addrman.SelectTriedCollision().first;
     BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0");
 
     // Ensure test of address fails, so that it is evicted.
@@ -985,23 +985,23 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
 
     // Should swap 36 for 19.
     addrman.ResolveCollisions();
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 
     // If 36 was swapped for 19, then this should cause no collisions.
     BOOST_CHECK(!addrman.Add({CAddress(addr, NODE_NONE)}, source));
     addrman.Good(addr);
 
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 
     // If we insert 19 it should collide with 36
     CService addr19 = ResolveService("250.1.1.19");
     BOOST_CHECK(!addrman.Add({CAddress(addr19, NODE_NONE)}, source));
     addrman.Good(addr19);
 
-    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.36:0");
+    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.36:0");
 
     addrman.ResolveCollisions();
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
+    BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 }
 
 BOOST_AUTO_TEST_CASE(load_addrman)