From 9bf078f66c8f286e1ab5e34b8eeed7d80290a897 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 17:38:58 -0700 Subject: [PATCH 01/12] refactor: update Select_ function Extract the logic that decides whether the new or the tried table is going to be searched to the beginning of the function. Co-authored-by: Martin Zumsande --- src/addrman.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index f5ca9a5c343..ec5b0213b3c 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -719,12 +719,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const AssertLockHeld(cs); if (vRandom.empty()) return {}; - if (newOnly && nNew == 0) return {}; + // Decide if we are going to search the new or tried table + bool search_tried; + // Use a 50% chance for choosing between tried and new table entries. if (!newOnly && - (nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) { + (nTried > 0 && + (nNew == 0 || insecure_rand.randbool() == 0))) { + search_tried = true; + } else { + search_tried = false; + } + + if (search_tried) { // use a tried node double fChanceFactor = 1.0; while (1) { From 052fbcd5a791855406141e85d32e42e297220fe9 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 17:46:13 -0700 Subject: [PATCH 02/12] addrman: Introduce helper to generalize looking up an addrman entry Unused until later commit. Co-authored-by: Martin Zumsande --- src/addrman.cpp | 15 +++++++++++++++ src/addrman_impl.h | 6 ++++++ 2 files changed, 21 insertions(+) diff --git a/src/addrman.cpp b/src/addrman.cpp index ec5b0213b3c..966cf6b0432 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -792,6 +792,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const } } +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]; + } +} + std::vector AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const { AssertLockHeld(cs); diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 94fe81aca9b..5410c3342c5 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -253,6 +253,12 @@ private: std::pair Select_(bool newOnly) 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 GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); void Connected_(const CService& addr, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); From ca2a9c5f8f14b792a14e81f73b1910a4c8799b93 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:01:18 -0700 Subject: [PATCH 03/12] refactor: generalize select logic in preparation for consolidating the logic for searching the new and tried tables, generalize the call paths for both Co-authored-by: Martin Zumsande --- src/addrman.cpp | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 966cf6b0432..afaa040b0f1 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -723,14 +723,17 @@ std::pair AddrManImpl::Select_(bool newOnly) const // Decide if we are going to search the new or tried table bool search_tried; + int bucket_count; // Use a 50% chance for choosing between tried and new table entries. if (!newOnly && (nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) { search_tried = true; + bucket_count = ADDRMAN_TRIED_BUCKET_COUNT; } else { search_tried = false; + bucket_count = ADDRMAN_NEW_BUCKET_COUNT; } if (search_tried) { @@ -738,18 +741,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const 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 nKBucket = insecure_rand.randrange(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; + int position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, nKBucket, position); + if (node_id != -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]; + int position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int nId = GetEntry(search_tried, nKBucket, position); const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; @@ -766,18 +772,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const 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 nUBucket = insecure_rand.randrange(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; + int position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, nUBucket, position); + if (node_id != -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]; + int position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int nId = GetEntry(search_tried, nUBucket, position); const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; From 48806412e2bcd023b78fc05f6c9ce092360d1db1 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:13:00 -0700 Subject: [PATCH 04/12] refactor: consolidate select logic for new and tried tables Co-authored-by: Martin Zumsande --- src/addrman.cpp | 91 +++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 60 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index afaa040b0f1..69ad13a912e 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -736,68 +736,39 @@ std::pair AddrManImpl::Select_(bool newOnly) const bucket_count = ADDRMAN_NEW_BUCKET_COUNT; } - if (search_tried) { - // 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(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) { - int position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int node_id = GetEntry(search_tried, nKBucket, position); - if (node_id != -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 position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int nId = GetEntry(search_tried, nKBucket, position); - 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; + double fChanceFactor = 1.0; + while (1) { + // Pick a bucket, and an initial position in that bucket. + int nBucket = insecure_rand.randrange(bucket_count); + int nBucketPos = 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 = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, nBucket, position); + if (node_id != -1) break; } - } 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(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) { - int position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int node_id = GetEntry(search_tried, nUBucket, position); - if (node_id != -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 position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int nId = GetEntry(search_tried, nUBucket, position); - 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}; - } - // 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 = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int nId = GetEntry(search_tried, nBucket, position); + 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 %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. + fChanceFactor *= 1.2; } } From 26c3bf11e2487ed0ac578fb92619c148336003cb Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sun, 19 Feb 2023 10:09:28 -0700 Subject: [PATCH 05/12] scripted-diff: rename local variables to match modern conventions -BEGIN VERIFY SCRIPT- sed -i 's/fChanceFactor/chance_factor/g' src/addrman.cpp sed -i 's/nBucketPos/initial_position/g' src/addrman.cpp sed -i 's/nBucket/bucket/g' src/addrman.cpp src/addrman_impl.h sed -i 's/newOnly/new_only/g' src/addrman.cpp src/addrman_impl.h src/addrman.h src/test/addrman_tests.cpp -END VERIFY SCRIPT- Co-authored-by: Martin Zumsande --- src/addrman.cpp | 38 +++++++++++++++++++------------------- src/addrman.h | 4 ++-- src/addrman_impl.h | 6 +++--- src/test/addrman_tests.cpp | 10 +++++----- 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 69ad13a912e..fe4a79b2e66 100644 --- a/src/addrman.cpp +++ b/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,19 +714,19 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds } } -std::pair AddrManImpl::Select_(bool newOnly) const +std::pair AddrManImpl::Select_(bool new_only) const { AssertLockHeld(cs); if (vRandom.empty()) return {}; - if (newOnly && nNew == 0) return {}; + if (new_only && nNew == 0) return {}; // Decide if we are going to search the new or tried table bool search_tried; int bucket_count; // Use a 50% chance for choosing between tried and new table entries. - if (!newOnly && + if (!new_only && (nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) { search_tried = true; @@ -736,18 +736,18 @@ std::pair AddrManImpl::Select_(bool newOnly) const bucket_count = ADDRMAN_NEW_BUCKET_COUNT; } - double fChanceFactor = 1.0; + double chance_factor = 1.0; while (1) { // Pick a bucket, and an initial position in that bucket. - int nBucket = insecure_rand.randrange(bucket_count); - int nBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); + 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 = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int node_id = GetEntry(search_tried, nBucket, position); + int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, bucket, position); if (node_id != -1) break; } @@ -755,20 +755,20 @@ std::pair AddrManImpl::Select_(bool newOnly) const if (i == ADDRMAN_BUCKET_SIZE) continue; // Find the entry to return. - int position = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int nId = GetEntry(search_tried, nBucket, position); + 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() * fChanceFactor, return the entry. - if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + // 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. - fChanceFactor *= 1.2; + chance_factor *= 1.2; } } @@ -1168,11 +1168,11 @@ std::pair AddrManImpl::SelectTriedCollision() return ret; } -std::pair AddrManImpl::Select(bool newOnly) const +std::pair AddrManImpl::Select(bool new_only) const { LOCK(cs); Check(); - auto addrRet = Select_(newOnly); + auto addrRet = Select_(new_only); Check(); return addrRet; } @@ -1266,9 +1266,9 @@ std::pair AddrMan::SelectTriedCollision() return m_impl->SelectTriedCollision(); } -std::pair AddrMan::Select(bool newOnly) const +std::pair AddrMan::Select(bool new_only) const { - return m_impl->Select(newOnly); + return m_impl->Select(new_only); } std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const diff --git a/src/addrman.h b/src/addrman.h index 4985fc764cf..374996e501f 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -146,11 +146,11 @@ public: /** * 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. * @return CAddress The record for the selected peer. * seconds The last time we attempted to connect to that peer. */ - std::pair Select(bool newOnly = false) const; + std::pair Select(bool new_only = 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 5410c3342c5..3cf3b838d68 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -90,7 +90,7 @@ public: } //! 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()) const; @@ -127,7 +127,7 @@ public: std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::pair Select(bool newOnly) const + std::pair Select(bool new_only) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const @@ -251,7 +251,7 @@ private: void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); - std::pair Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::pair Select_(bool new_only) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Helper to generalize looking up an addrman entry from either table. * diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 758691cfdea..ad59e123d2a 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -127,8 +127,8 @@ BOOST_AUTO_TEST_CASE(addrman_ports) // the specified port to tried, but not the other. addrman->Good(CAddress(addr1_port, NODE_NONE)); BOOST_CHECK_EQUAL(addrman->Size(), 2U); - bool newOnly = true; - auto addr_ret3 = addrman->Select(newOnly).first; + bool new_only = true; + auto addr_ret3 = addrman->Select(new_only).first; BOOST_CHECK_EQUAL(addr_ret3.ToStringAddrPort(), "250.1.1.1:8333"); } @@ -144,14 +144,14 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - bool newOnly = true; - auto addr_ret1 = addrman->Select(newOnly).first; + bool new_only = true; + auto addr_ret1 = addrman->Select(new_only).first; BOOST_CHECK_EQUAL(addr_ret1.ToStringAddrPort(), "250.1.1.1:8333"); // Test: move addr to tried, select from new expected nothing returned. BOOST_CHECK(addrman->Good(CAddress(addr1, NODE_NONE))); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - auto addr_ret2 = addrman->Select(newOnly).first; + auto addr_ret2 = addrman->Select(new_only).first; BOOST_CHECK_EQUAL(addr_ret2.ToStringAddrPort(), "[::]:0"); auto addr_ret3 = addrman->Select().first; From 6b229284fd2209938ee8fdffed4d080395b3aa05 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:29:45 -0700 Subject: [PATCH 06/12] addrman: add functionality to select by network Add an optional parameter to the addrman Select function that allows callers to specify which network the returned address should be on. Ensure that the proper table is selected with different cases of whether the new or tried table has network addresses that match. Co-authored-by: Martin Zumsande --- src/addrman.cpp | 60 +++++++++++++++++++++++++++++++--------------- src/addrman.h | 5 ++-- src/addrman_impl.h | 4 ++-- 3 files changed, 46 insertions(+), 23 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index fe4a79b2e66..cdfd079fcdf 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -714,28 +714,41 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds } } -std::pair AddrManImpl::Select_(bool new_only) const +std::pair AddrManImpl::Select_(bool new_only, std::optional network) const { AssertLockHeld(cs); if (vRandom.empty()) return {}; - if (new_only && nNew == 0) return {}; - // Decide if we are going to search the new or tried table - bool search_tried; - int bucket_count; + size_t new_count = nNew; + size_t tried_count = nTried; - // Use a 50% chance for choosing between tried and new table entries. - if (!new_only && - (nTried > 0 && - (nNew == 0 || insecure_rand.randbool() == 0))) { - search_tried = true; - bucket_count = ADDRMAN_TRIED_BUCKET_COUNT; - } else { - search_tried = false; - bucket_count = ADDRMAN_NEW_BUCKET_COUNT; + 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 { + 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. @@ -748,7 +761,16 @@ std::pair AddrManImpl::Select_(bool new_only) const 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) break; + 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; + } + } } // If the bucket is entirely empty, start over with a (likely) different one. @@ -1168,11 +1190,11 @@ std::pair AddrManImpl::SelectTriedCollision() return ret; } -std::pair AddrManImpl::Select(bool new_only) const +std::pair AddrManImpl::Select(bool new_only, std::optional network) const { LOCK(cs); Check(); - auto addrRet = Select_(new_only); + auto addrRet = Select_(new_only, network); Check(); return addrRet; } @@ -1266,9 +1288,9 @@ std::pair AddrMan::SelectTriedCollision() return m_impl->SelectTriedCollision(); } -std::pair AddrMan::Select(bool new_only) const +std::pair AddrMan::Select(bool new_only, std::optional network) const { - return m_impl->Select(new_only); + return m_impl->Select(new_only, network); } std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const diff --git a/src/addrman.h b/src/addrman.h index 374996e501f..c8e31fe2833 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -146,11 +146,12 @@ public: /** * Choose an address to connect to. * - * @param[in] new_only Whether to only select addresses from the new table. + * @param[in] new_only Whether to only select addresses from the new table. + * @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 Select(bool new_only = false) const; + std::pair Select(bool new_only = false, std::optional network = std::nullopt) const; /** * Return all or many randomly selected addresses, optionally by network. diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 3cf3b838d68..7aead2812ba 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -127,7 +127,7 @@ public: std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::pair Select(bool new_only) const + std::pair Select(bool new_only, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const @@ -251,7 +251,7 @@ private: void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); - std::pair Select_(bool new_only) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::pair Select_(bool new_only, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Helper to generalize looking up an addrman entry from either table. * From 5c8b4baff27e0ccd27fda6e915b956d1e8dd7ce2 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 6 Feb 2023 20:15:50 -0700 Subject: [PATCH 07/12] tests: add addrman_select_by_network test this adds coverage for the 7 different cases of which table should be selected when the network is specified. the different cases are the result of new_only being true or false and whether there are network addresses on both, neither, or one of new vs tried tables. the only case not covered is when new_only is false and the only network addresses are on the new table. Co-authored-by: Martin Zumsande Co-authored-by: Vasil Dimov --- src/test/addrman_tests.cpp | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index ad59e123d2a..1acdb02c9a7 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -192,6 +192,66 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_CHECK_EQUAL(ports.size(), 3U); } +BOOST_AUTO_TEST_CASE(addrman_select_by_network) +{ + auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); + BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_IPV4).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_IPV4).first.IsValid()); + + // add ipv4 address to the new table + CNetAddr source = ResolveIP("252.2.2.2"); + CService addr1 = ResolveService("250.1.1.1", 8333); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + + BOOST_CHECK(addrman->Select(/*new_only=*/true, NET_IPV4).first == addr1); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_IPV4).first == addr1); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_IPV6).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_ONION).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_I2P).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_CJDNS).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_CJDNS).first.IsValid()); + BOOST_CHECK(addrman->Select(/*new_only=*/false).first == addr1); + + // add I2P address to the new table + CAddress i2p_addr; + i2p_addr.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p"); + BOOST_CHECK(addrman->Add({i2p_addr}, source)); + + BOOST_CHECK(addrman->Select(/*new_only=*/true, NET_I2P).first == i2p_addr); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_I2P).first == i2p_addr); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_IPV4).first == addr1); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_IPV6).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_ONION).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_CJDNS).first.IsValid()); + + // bump I2P address to tried table + BOOST_CHECK(addrman->Good(i2p_addr)); + + BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_I2P).first.IsValid()); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_I2P).first == i2p_addr); + + // add another I2P address to the new table + CAddress i2p_addr2; + i2p_addr2.SetSpecial("c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p"); + BOOST_CHECK(addrman->Add({i2p_addr2}, source)); + + BOOST_CHECK(addrman->Select(/*new_only=*/true, NET_I2P).first == i2p_addr2); + + // ensure that both new and tried table are selected from + bool new_selected{false}; + bool tried_selected{false}; + + while (!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) { + tried_selected = true; + } else { + new_selected = true; + } + } +} + BOOST_AUTO_TEST_CASE(addrman_new_collisions) { auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); From a98e542e0c18f7cb2340179631806f14b07430c3 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:34:06 -0700 Subject: [PATCH 08/12] test: add addrman test for special case if an addr matching the network requirements is only on the new table and select is invoked with new_only = false, ensure that the code selects the new table. in order to test this case, we use a non deterministic addrman. this means we cannot have more than one address in any addrman table, or risk sporadic failures when the second address happens to conflict. if the code chose a table at random, the test would fail 50% of the time Co-authored-by: Martin Zumsande --- src/test/addrman_tests.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 1acdb02c9a7..c97a29eb1e8 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -252,6 +252,28 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network) } } +BOOST_AUTO_TEST_CASE(addrman_select_special) +{ + // use a non-deterministic addrman to ensure a passing test isn't due to setup + auto addrman = std::make_unique(EMPTY_NETGROUPMAN, /*deterministic=*/false, GetCheckRatio(m_node)); + + // add ipv4 address to the new table + CNetAddr source = ResolveIP("252.2.2.2"); + CService addr1 = ResolveService("250.1.1.3", 8333); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + + // add I2P address to the tried table + CAddress i2p_addr; + i2p_addr.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p"); + BOOST_CHECK(addrman->Add({i2p_addr}, source)); + BOOST_CHECK(addrman->Good(i2p_addr)); + + // since the only ipv4 address is on the new table, ensure that the new + // table gets selected even if new_only is false. if the table was being + // selected at random, this test will sporadically fail + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_IPV4).first == addr1); +} + BOOST_AUTO_TEST_CASE(addrman_new_collisions) { auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); From 22a4d1489c0678a90c00318203cfce61672f20b7 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:41:25 -0700 Subject: [PATCH 09/12] test: increase coverage of addrman select (without network) Co-authored-by: Martin Zumsande --- src/test/addrman_tests.cpp | 39 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index c97a29eb1e8..b5f9558337a 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -132,41 +132,40 @@ BOOST_AUTO_TEST_CASE(addrman_ports) BOOST_CHECK_EQUAL(addr_ret3.ToStringAddrPort(), "250.1.1.1:8333"); } - BOOST_AUTO_TEST_CASE(addrman_select) { auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); + BOOST_CHECK(!addrman->Select(false).first.IsValid()); + BOOST_CHECK(!addrman->Select(true).first.IsValid()); CNetAddr source = ResolveIP("252.2.2.2"); - // Test: Select from new with 1 addr in new. + // Add 1 address to the new table CService addr1 = ResolveService("250.1.1.1", 8333); BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - bool new_only = true; - auto addr_ret1 = addrman->Select(new_only).first; - BOOST_CHECK_EQUAL(addr_ret1.ToStringAddrPort(), "250.1.1.1:8333"); + BOOST_CHECK(addrman->Select(/*new_only=*/true).first == addr1); + BOOST_CHECK(addrman->Select(/*new_only=*/false).first == addr1); - // Test: move addr to tried, select from new expected nothing returned. + // Move address to the tried table BOOST_CHECK(addrman->Good(CAddress(addr1, NODE_NONE))); - BOOST_CHECK_EQUAL(addrman->Size(), 1U); - auto addr_ret2 = addrman->Select(new_only).first; - BOOST_CHECK_EQUAL(addr_ret2.ToStringAddrPort(), "[::]:0"); - - auto addr_ret3 = addrman->Select().first; - BOOST_CHECK_EQUAL(addr_ret3.ToStringAddrPort(), "250.1.1.1:8333"); + BOOST_CHECK_EQUAL(addrman->Size(), 1U); + BOOST_CHECK(!addrman->Select(/*new_only=*/true).first.IsValid()); + BOOST_CHECK(addrman->Select().first == addr1); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - - // Add three addresses to new table. + // Add one address to the new table CService addr2 = ResolveService("250.3.1.1", 8333); + BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, addr2)); + BOOST_CHECK(addrman->Select(/*new_only=*/true).first == addr2); + + // Add two more addresses to the new table CService addr3 = ResolveService("250.3.2.2", 9999); CService addr4 = ResolveService("250.3.3.3", 9999); - BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - BOOST_CHECK(addrman->Add({CAddress(addr3, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr3, NODE_NONE)}, addr2)); BOOST_CHECK(addrman->Add({CAddress(addr4, NODE_NONE)}, ResolveService("250.4.1.1", 8333))); // Add three addresses to tried table. @@ -174,17 +173,17 @@ BOOST_AUTO_TEST_CASE(addrman_select) CService addr6 = ResolveService("250.4.5.5", 7777); CService addr7 = ResolveService("250.4.6.6", 8333); - BOOST_CHECK(addrman->Add({CAddress(addr5, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr5, NODE_NONE)}, addr3)); BOOST_CHECK(addrman->Good(CAddress(addr5, NODE_NONE))); - BOOST_CHECK(addrman->Add({CAddress(addr6, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr6, NODE_NONE)}, addr3)); BOOST_CHECK(addrman->Good(CAddress(addr6, NODE_NONE))); BOOST_CHECK(addrman->Add({CAddress(addr7, NODE_NONE)}, ResolveService("250.1.1.3", 8333))); BOOST_CHECK(addrman->Good(CAddress(addr7, NODE_NONE))); - // Test: 6 addrs + 1 addr from last test = 7. + // 6 addrs + 1 addr from last test = 7. BOOST_CHECK_EQUAL(addrman->Size(), 7U); - // Test: Select pulls from new and tried regardless of port number. + // Select pulls from new and tried regardless of port number. std::set ports; for (int i = 0; i < 20; ++i) { ports.insert(addrman->Select().first.GetPort()); From 9b91aae08579c77d2fd5506804c8e2e0cda0d274 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 17:03:56 -0700 Subject: [PATCH 10/12] bench: add coverage for addrman select with network parameter to evaluate the worst case performance with the network parameter passed through, fill the new table with addresses then add a singular I2P address to retrieve Co-authored-by: Martin Zumsande --- src/bench/addrman.cpp | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index d6b52eb587a..b8c69d0a637 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -71,6 +72,13 @@ static void FillAddrMan(AddrMan& addrman) AddAddressesToAddrMan(addrman); } +static CNetAddr ResolveIP(const std::string& ip) +{ + CNetAddr addr; + LookupHost(ip, addr, false); + return addr; +} + /* Benchmarks */ static void AddrManAdd(benchmark::Bench& bench) @@ -95,6 +103,25 @@ static void AddrManSelect(benchmark::Bench& bench) }); } +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(); + 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 +162,6 @@ static void AddrManAddThenGood(benchmark::Bench& bench) BENCHMARK(AddrManAdd, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManSelect, benchmark::PriorityLevel::HIGH); +BENCHMARK(AddrManSelectByNetwork, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::HIGH); From b0010c83a1b4a3d21719cb68e37faf9b1172522a Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Thu, 23 Feb 2023 13:53:52 -0800 Subject: [PATCH 11/12] bench: test select for a new table with only one address the addrman select function will demonstrate it's worst case performance when it is almost empty, because it might have to linearly search several buckets. add a bench test to cover this case Co-authored-by: Martin Zumsande --- src/bench/addrman.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index b8c69d0a637..8a5cab443f2 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -79,6 +79,13 @@ static CNetAddr ResolveIP(const std::string& ip) 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) @@ -103,6 +110,22 @@ 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}; @@ -162,6 +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); From 17e705428ddf80c7a7f31fe5430d966cf08a37d6 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 10 Mar 2023 18:25:59 -0800 Subject: [PATCH 12/12] doc: clarify new_only param for Select function Co-authored-by: Martin Zumsande --- src/addrman.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/addrman.h b/src/addrman.h index c8e31fe2833..6284b80a52e 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -146,7 +146,9 @@ public: /** * Choose an address to connect to. * - * @param[in] new_only 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.