From 2b6bd12eea0c970881753124ec3f0a67e2de8e17 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 25 Apr 2023 16:29:29 +0100 Subject: [PATCH 1/4] refactor: de-duplicate lookups retain the values needed to prevent redundant node lookups --- src/addrman.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index cdfd079fcd..30ce2cadc8 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -757,10 +757,10 @@ std::pair AddrManImpl::Select_(bool new_only, std::option // 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)}; @@ -777,9 +777,7 @@ std::pair AddrManImpl::Select_(bool new_only, std::option 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}; From 768770771f7db60147943152b34a8dd485cdcc76 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 25 Apr 2023 16:42:36 +0100 Subject: [PATCH 2/4] doc: update `Select` function description Capture potential performance slow down for `Select` by network & clarify return values. --- src/addrman.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 6284b80a52..f41687dcff 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -148,8 +148,10 @@ public: * * @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. * seconds The last time we attempted to connect to that peer. */ From b9f1e86f129e46bb5770fb421d0ba164b5c7aaf8 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 25 Apr 2023 17:04:11 +0100 Subject: [PATCH 3/4] addrman: change asserts to Assumes `Assume` is safer since the checks are non-fatal- errors in these functions should provide feedback in debug builds, but do not need to deter further node operations in production. --- src/addrman.cpp | 18 +++++++++--------- src/addrman_impl.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 30ce2cadc8..19a4f4fa63 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -764,9 +764,7 @@ std::pair AddrManImpl::Select_(bool new_only, std::option 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; } @@ -796,15 +794,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 AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 7aead2812b..9aff408e34 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -255,7 +255,7 @@ private: /** 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); From cd8ef5b3e66b3f766c9c883259b5feb44540d7df Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 25 Apr 2023 17:05:23 +0100 Subject: [PATCH 4/4] test: ensure addrman test is finite Add a counter to ensure that the error case is bounded rather than leading to a CI timeout --- src/test/addrman_tests.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 54d923e4a4..2242c7a75a 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -239,8 +239,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) { @@ -249,6 +250,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)