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

Author: amitiuttarwar

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);
 }

src/addrman.h

@@ -171,13 +171,23 @@ class CAddrMan
     //! 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.

src/addrman_impl.h

@@ -31,9 +31,9 @@ class AddrManImpl
 
     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 @@ class AddrManImpl
     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 @@ class AddrManImpl
     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);

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);
     });
 }
 

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,