merge bitcoin#27745: select addresses by network follow-up

kwvg · kwvg · commit 7d149c97be97 · 2024-09-11T18:34:07.000Z
diff --git a/src/addrman.cpp b/src/addrman.cpp
@@ -750,16 +750,14 @@ std::pair<CAddress, int64_t> AddrManImpl::Select_(bool new_only, std::optional<N
 
         // Iterate over the positions of that bucket, starting at the initial one,
         // and looping around.
-        int i;
+        int i, position, node_id;
         for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
-            int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
-            int node_id = GetEntry(search_tried, bucket, position);
+            position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
+            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;
+                    if (Assume(it != mapInfo.end()) && it->second.GetNetwork() == *network) break;
                 } else {
                     break;
                 }
@@ -770,9 +768,7 @@ std::pair<CAddress, int64_t> AddrManImpl::Select_(bool new_only, std::optional<N
         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)};
+        const auto it_found{mapInfo.find(node_id)};
         assert(it_found != mapInfo.end());
         const AddrInfo& info{it_found->second};
 
@@ -791,15 +787,17 @@ 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];
+        if (Assume(position < ADDRMAN_BUCKET_SIZE) && Assume(bucket < ADDRMAN_TRIED_BUCKET_COUNT)) {
+            return vvTried[bucket][position];
+        }
     } else {
-        assert(bucket < ADDRMAN_NEW_BUCKET_COUNT);
-        return vvNew[bucket][position];
+        if (Assume(position < ADDRMAN_BUCKET_SIZE) && Assume(bucket < ADDRMAN_NEW_BUCKET_COUNT)) {
+            return vvNew[bucket][position];
+        }
     }
+
+    return -1;
 }
 
 std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
diff --git a/src/addrman.h b/src/addrman.h
@@ -159,8 +159,10 @@ class AddrMan
      *
      * @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)
+     *                     empty pair or 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). Passing a network may
+     *                     slow down the search.
      * @return    CAddress The record for the selected peer.
      *            int64_t  The last time we attempted to connect to that peer.
      */
diff --git a/src/addrman_impl.h b/src/addrman_impl.h
@@ -256,7 +256,7 @@ class AddrManImpl
 
     /** 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.
+     *  @return  int The nid of the entry. If the addrman position is empty or not found, returns -1.
      * */
     int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
@@ -237,8 +237,9 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network)
     // ensure that both new and tried table are selected from
     bool new_selected{false};
     bool tried_selected{false};
+    int counter = 256;
 
-    while (!new_selected || !tried_selected) {
+    while (--counter > 0 && (!new_selected || !tried_selected)) {
         const CAddress selected{addrman->Select(/*new_only=*/false, NET_I2P).first};
         BOOST_REQUIRE(selected == i2p_addr || selected == i2p_addr2);
         if (selected == i2p_addr) {
@@ -247,6 +248,9 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network)
             new_selected = true;
         }
     }
+
+    BOOST_CHECK(new_selected);
+    BOOST_CHECK(tried_selected);
 }
 
 BOOST_AUTO_TEST_CASE(addrman_select_special)

Original file line number	Diff line number	Diff line change
`@@ -256,7 +256,7 @@ class AddrManImpl`
`256`	`256`
`257`	`257`	`/** Helper to generalize looking up an addrman entry from either table.`
`258`	`258`	`*`
`259`		`- * @return int The nid of the entry or -1 if the addrman position is empty.`
	`259`	`+ * @return int The nid of the entry. If the addrman position is empty or not found, returns -1.`
`260`	`260`	`* */`
`261`	`261`	`int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);`
`262`	`262`
Original file line number	Diff line number	Diff line change
`@@ -237,8 +237,9 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network)`
`237`	`237`	`// ensure that both new and tried table are selected from`
`238`	`238`	`bool new_selected{false};`
`239`	`239`	`bool tried_selected{false};`
	`240`	`+ int counter = 256;`
`240`	`241`
`241`		`- while (!new_selected \|\| !tried_selected) {`
	`242`	`+ while (--counter > 0 && (!new_selected \|\| !tried_selected)) {`
`242`	`243`	`const CAddress selected{addrman->Select(/new_only=/false, NET_I2P).first};`
`243`	`244`	`BOOST_REQUIRE(selected == i2p_addr \|\| selected == i2p_addr2);`
`244`	`245`	`if (selected == i2p_addr) {`
`@@ -247,6 +248,9 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network)`
`247`	`248`	`new_selected = true;`
`248`	`249`	`}`
`249`	`250`	`}`
	`251`	`+`
	`252`	`+ BOOST_CHECK(new_selected);`
	`253`	`+ BOOST_CHECK(tried_selected);`
`250`	`254`	`}`
`251`	`255`
`252`	`256`	`BOOST_AUTO_TEST_CASE(addrman_select_special)`