Merge bitcoin/bitcoin#27214: addrman: Enable selecting addresses by network

17e7054 doc: clarify new_only param for Select function (Amiti Uttarwar)
b0010c8 bench: test select for a new table with only one address (Amiti Uttarwar)
9b91aae bench: add coverage for addrman select with network parameter (Amiti Uttarwar)
22a4d14 test: increase coverage of addrman select (without network) (Amiti Uttarwar)
a98e542 test: add addrman test for special case (Amiti Uttarwar)
5c8b4ba tests: add addrman_select_by_network test (Amiti Uttarwar)
6b22928 addrman: add functionality to select by network (Amiti Uttarwar)
26c3bf1 scripted-diff: rename local variables to match modern conventions (Amiti Uttarwar)
4880641 refactor: consolidate select logic for new and tried tables (Amiti Uttarwar)
ca2a9c5 refactor: generalize select logic (Amiti Uttarwar)
052fbcd addrman: Introduce helper to generalize looking up an addrman entry (Amiti Uttarwar)
9bf078f refactor: update Select_ function (Amiti Uttarwar)

Pull request description:

  For the full context & motivation of this patch, see #27213

  This is joint work with mzumsande.

  This PR adds functionality to `AddrMan::Select` to enable callers to specify a network they are interested in.

  Along the way, it refactors the function to deduplicate the logic, updates the local variables to match modern conventions, adds test coverage for both the new and existing `Select` logic, and adds bench tests for the worst case performance of both the new and existing `Select` logic.

  This functionality is used in the parent PR.

ACKs for top commit:
  vasild:
    ACK 17e7054
  brunoerg:
    re-ACK 17e7054
  ajtowns:
    ACK 17e7054
  mzumsande:
    Code Review ACK 17e7054

Tree-SHA512: e99d1ce0c44a15601a3daa37deeadfc9d26208a92969ecffbea358d57ca951102d759734ccf77eacd38db368da0bf5b6fede3cd900d8a77b3061f4adc54e52d8

Author: achow101

src/addrman.cpp

@@ -58,9 +58,9 @@ int AddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const NetGr
     return hash2 % ADDRMAN_NEW_BUCKET_COUNT;
 }
 
-int AddrInfo::GetBucketPosition(const uint256& nKey, bool fNew, int nBucket) const
+int AddrInfo::GetBucketPosition(const uint256& nKey, bool fNew, int bucket) const
 {
-    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? uint8_t{'N'} : uint8_t{'K'}) << nBucket << GetKey()).GetCheapHash();
+    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? uint8_t{'N'} : uint8_t{'K'}) << bucket << GetKey()).GetCheapHash();
     return hash1 % ADDRMAN_BUCKET_SIZE;
 }
 
@@ -714,72 +714,98 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds
     }
 }
 
-std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool newOnly) const
+std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::optional<Network> network) const
 {
     AssertLockHeld(cs);
 
     if (vRandom.empty()) return {};
 
-    if (newOnly && nNew == 0) return {};
-
-    // Use a 50% chance for choosing between tried and new table entries.
-    if (!newOnly &&
-       (nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) {
-        // use a tried node
-        double fChanceFactor = 1.0;
-        while (1) {
-            // Pick a tried bucket, and an initial position in that bucket.
-            int nKBucket = insecure_rand.randrange(ADDRMAN_TRIED_BUCKET_COUNT);
-            int nKBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
-            // Iterate over the positions of that bucket, starting at the initial one,
-            // and looping around.
-            int i;
-            for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
-                if (vvTried[nKBucket][(nKBucketPos + i) % ADDRMAN_BUCKET_SIZE] != -1) break;
-            }
-            // If the bucket is entirely empty, start over with a (likely) different one.
-            if (i == ADDRMAN_BUCKET_SIZE) continue;
-            // Find the entry to return.
-            int nId = vvTried[nKBucket][(nKBucketPos + i) % ADDRMAN_BUCKET_SIZE];
-            const auto it_found{mapInfo.find(nId)};
-            assert(it_found != mapInfo.end());
-            const AddrInfo& info{it_found->second};
-            // With probability GetChance() * fChanceFactor, return the entry.
-            if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) {
-                LogPrint(BCLog::ADDRMAN, "Selected %s from tried\n", info.ToStringAddrPort());
-                return {info, info.m_last_try};
-            }
-            // Otherwise start over with a (likely) different bucket, and increased chance factor.
-            fChanceFactor *= 1.2;
-        }
+    size_t new_count = nNew;
+    size_t tried_count = nTried;
+
+    if (network.has_value()) {
+        auto it = m_network_counts.find(*network);
+        if (it == m_network_counts.end()) return {};
+
+        auto counts = it->second;
+        new_count = counts.n_new;
+        tried_count = counts.n_tried;
+    }
+
+    if (new_only && new_count == 0) return {};
+    if (new_count + tried_count == 0) return {};
+
+    // Decide if we are going to search the new or tried table
+    // If either option is viable, use a 50% chance to choose
+    bool search_tried;
+    if (new_only || tried_count == 0) {
+        search_tried = false;
+    } else if (new_count == 0) {
+        search_tried = true;
     } else {
-        // use a new node
-        double fChanceFactor = 1.0;
-        while (1) {
-            // Pick a new bucket, and an initial position in that bucket.
-            int nUBucket = insecure_rand.randrange(ADDRMAN_NEW_BUCKET_COUNT);
-            int nUBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
-            // Iterate over the positions of that bucket, starting at the initial one,
-            // and looping around.
-            int i;
-            for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
-                if (vvNew[nUBucket][(nUBucketPos + i) % ADDRMAN_BUCKET_SIZE] != -1) break;
-            }
-            // If the bucket is entirely empty, start over with a (likely) different one.
-            if (i == ADDRMAN_BUCKET_SIZE) continue;
-            // Find the entry to return.
-            int nId = vvNew[nUBucket][(nUBucketPos + i) % ADDRMAN_BUCKET_SIZE];
-            const auto it_found{mapInfo.find(nId)};
-            assert(it_found != mapInfo.end());
-            const AddrInfo& info{it_found->second};
-            // With probability GetChance() * fChanceFactor, return the entry.
-            if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) {
-                LogPrint(BCLog::ADDRMAN, "Selected %s from new\n", info.ToStringAddrPort());
-                return {info, info.m_last_try};
+        search_tried = insecure_rand.randbool();
+    }
+
+    const int bucket_count{search_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT};
+
+    // Loop through the addrman table until we find an appropriate entry
+    double chance_factor = 1.0;
+    while (1) {
+        // Pick a bucket, and an initial position in that bucket.
+        int bucket = insecure_rand.randrange(bucket_count);
+        int initial_position = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
+
+        // Iterate over the positions of that bucket, starting at the initial one,
+        // and looping around.
+        int i;
+        for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
+            int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
+            int node_id = GetEntry(search_tried, bucket, position);
+            if (node_id != -1) {
+                if (network.has_value()) {
+                    const auto it{mapInfo.find(node_id)};
+                    assert(it != mapInfo.end());
+                    const auto info{it->second};
+                    if (info.GetNetwork() == *network) break;
+                } else {
+                    break;
+                }
             }
-            // Otherwise start over with a (likely) different bucket, and increased chance factor.
-            fChanceFactor *= 1.2;
         }
+
+        // If the bucket is entirely empty, start over with a (likely) different one.
+        if (i == ADDRMAN_BUCKET_SIZE) continue;
+
+        // Find the entry to return.
+        int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
+        int nId = GetEntry(search_tried, bucket, position);
+        const auto it_found{mapInfo.find(nId)};
+        assert(it_found != mapInfo.end());
+        const AddrInfo& info{it_found->second};
+
+        // With probability GetChance() * chance_factor, return the entry.
+        if (insecure_rand.randbits(30) < chance_factor * info.GetChance() * (1 << 30)) {
+            LogPrint(BCLog::ADDRMAN, "Selected %s from %s\n", info.ToStringAddrPort(), search_tried ? "tried" : "new");
+            return {info, info.m_last_try};
+        }
+
+        // Otherwise start over with a (likely) different bucket, and increased chance factor.
+        chance_factor *= 1.2;
+    }
+}
+
+int AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const
+{
+    AssertLockHeld(cs);
+
+    assert(position < ADDRMAN_BUCKET_SIZE);
+
+    if (use_tried) {
+        assert(bucket < ADDRMAN_TRIED_BUCKET_COUNT);
+        return vvTried[bucket][position];
+    } else {
+        assert(bucket < ADDRMAN_NEW_BUCKET_COUNT);
+        return vvNew[bucket][position];
     }
 }
 
@@ -1164,11 +1190,11 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision()
     return ret;
 }
 
-std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool newOnly) const
+std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::optional<Network> network) const
 {
     LOCK(cs);
     Check();
-    auto addrRet = Select_(newOnly);
+    auto addrRet = Select_(new_only, network);
     Check();
     return addrRet;
 }
@@ -1262,9 +1288,9 @@ std::pair<CAddress, NodeSeconds> AddrMan::SelectTriedCollision()
     return m_impl->SelectTriedCollision();
 }
 
-std::pair<CAddress, NodeSeconds> AddrMan::Select(bool newOnly) const
+std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, std::optional<Network> network) const
 {
-    return m_impl->Select(newOnly);
+    return m_impl->Select(new_only, network);
 }
 
 std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const

src/addrman.h

@@ -146,11 +146,14 @@ class AddrMan
     /**
      * Choose an address to connect to.
      *
-     * @param[in] newOnly  Whether to only select addresses from the new table.
+     * @param[in] new_only Whether to only select addresses from the new table. Passing `true` returns
+     *                     an address from the new table or an empty pair. Passing `false` will return an
+     *                     address from either the new or tried table (it does not guarantee a tried entry).
+     * @param[in] network  Select only addresses of this network (nullopt = all)
      * @return    CAddress The record for the selected peer.
      *            seconds  The last time we attempted to connect to that peer.
      */
-    std::pair<CAddress, NodeSeconds> Select(bool newOnly = false) const;
+    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<Network> network = std::nullopt) const;
 
     /**
      * Return all or many randomly selected addresses, optionally by network.

src/addrman_impl.h

@@ -90,7 +90,7 @@ class AddrInfo : public CAddress
     }
 
     //! Calculate in which position of a bucket to store this entry.
-    int GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const;
+    int GetBucketPosition(const uint256 &nKey, bool fNew, int bucket) const;
 
     //! Determine whether the statistics about this entry are bad enough so that it can just be deleted
     bool IsTerrible(NodeSeconds now = Now<NodeSeconds>()) const;
@@ -127,7 +127,7 @@ class AddrManImpl
 
     std::pair<CAddress, NodeSeconds> SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    std::pair<CAddress, NodeSeconds> Select(bool newOnly) const
+    std::pair<CAddress, NodeSeconds> Select(bool new_only, std::optional<Network> network) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
@@ -251,7 +251,13 @@ class AddrManImpl
 
     void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    std::pair<CAddress, NodeSeconds> Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    std::pair<CAddress, NodeSeconds> Select_(bool new_only, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+
+    /** Helper to generalize looking up an addrman entry from either table.
+     *
+     *  @return  int The nid of the entry or -1 if the addrman position is empty.
+     * */
+    int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 

src/bench/addrman.cpp

@@ -4,6 +4,7 @@
 
 #include <addrman.h>
 #include <bench/bench.h>
+#include <netbase.h>
 #include <netgroup.h>
 #include <random.h>
 #include <util/check.h>
@@ -71,6 +72,20 @@ static void FillAddrMan(AddrMan& addrman)
     AddAddressesToAddrMan(addrman);
 }
 
+static CNetAddr ResolveIP(const std::string& ip)
+{
+    CNetAddr addr;
+    LookupHost(ip, addr, false);
+    return addr;
+}
+
+static CService ResolveService(const std::string& ip, uint16_t port = 0)
+{
+    CService serv;
+    Lookup(ip, serv, port, false);
+    return serv;
+}
+
 /* Benchmarks */
 
 static void AddrManAdd(benchmark::Bench& bench)
@@ -95,6 +110,41 @@ static void AddrManSelect(benchmark::Bench& bench)
     });
 }
 
+// The worst case performance of the Select() function is when there is only
+// one address on the table, because it linearly searches every position of
+// several buckets before identifying the correct bucket
+static void AddrManSelectFromAlmostEmpty(benchmark::Bench& bench)
+{
+    AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
+
+    // Add one address to the new table
+    CService addr = ResolveService("250.3.1.1", 8333);
+    addrman.Add({CAddress(addr, NODE_NONE)}, ResolveService("250.3.1.1", 8333));
+
+    bench.run([&] {
+        (void)addrman.Select();
+    });
+}
+
+static void AddrManSelectByNetwork(benchmark::Bench& bench)
+{
+    AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
+
+    // add single I2P address to new table
+    CService i2p_service;
+    i2p_service.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p");
+    CAddress i2p_address(i2p_service, NODE_NONE);
+    i2p_address.nTime = Now<NodeSeconds>();
+    CNetAddr source = ResolveIP("252.2.2.2");
+    addrman.Add({i2p_address}, source);
+
+    FillAddrMan(addrman);
+
+    bench.run([&] {
+        (void)addrman.Select(/*new_only=*/false, NET_I2P);
+    });
+}
+
 static void AddrManGetAddr(benchmark::Bench& bench)
 {
     AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
@@ -135,5 +185,7 @@ static void AddrManAddThenGood(benchmark::Bench& bench)
 
 BENCHMARK(AddrManAdd, benchmark::PriorityLevel::HIGH);
 BENCHMARK(AddrManSelect, benchmark::PriorityLevel::HIGH);
+BENCHMARK(AddrManSelectFromAlmostEmpty, benchmark::PriorityLevel::HIGH);
+BENCHMARK(AddrManSelectByNetwork, benchmark::PriorityLevel::HIGH);
 BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH);
 BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::HIGH);