diff --git a/src/addrman.cpp b/src/addrman.cpp index 9974c756b5..39cfcae961 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; } @@ -707,72 +707,98 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi } } -std::pair AddrManImpl::Select_(bool newOnly) const +std::pair AddrManImpl::Select_(bool new_only, std::optional network) const { AssertLockHeld(cs); if (vRandom.empty()) return {}; - if (newOnly && nNew == 0) return {}; + size_t new_count = nNew; + size_t tried_count = nTried; - // 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.nLastTry}; - } - // Otherwise start over with a (likely) different bucket, and increased chance factor. - fChanceFactor *= 1.2; - } + 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; + 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; + } } - // 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.nLastTry}; - } - // 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.nLastTry}; + } + + // 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 AddrManImpl::SelectTriedCollision() return ret; } -std::pair AddrManImpl::Select(bool newOnly) const +std::pair AddrManImpl::Select(bool new_only, std::optional network) const { LOCK(cs); Check(); - const auto addrRet = Select_(newOnly); + const auto addrRet = Select_(new_only, network); Check(); return addrRet; } @@ -1274,9 +1300,9 @@ std::pair AddrMan::SelectTriedCollision() return m_impl->SelectTriedCollision(); } -std::pair AddrMan::Select(bool newOnly) const +std::pair AddrMan::Select(bool new_only, std::optional network) const { - return m_impl->Select(newOnly); + 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 b1bc97a572..ecae67d77c 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -157,11 +157,14 @@ 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. 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. * int64_t The last time we attempted to connect to that peer. */ - std::pair Select(bool newOnly = 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 6172164c84..5464d860f5 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -88,7 +88,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(int64_t nNow = GetAdjustedTime()) const; @@ -125,7 +125,7 @@ public: std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::pair Select(bool newOnly) 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 @@ -252,7 +252,13 @@ private: void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); - std::pair Select_(bool newOnly) 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. + * + * @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); diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index 9eee2f7d3e..6989252670 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -69,6 +70,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) @@ -93,6 +108,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 = GetAdjustedTime(); + 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}; @@ -133,5 +183,7 @@ static void AddrManAddThenGood(benchmark::Bench& bench) BENCHMARK(AddrManAdd); BENCHMARK(AddrManSelect); +BENCHMARK(AddrManSelectFromAlmostEmpty); +BENCHMARK(AddrManSelectByNetwork); BENCHMARK(AddrManGetAddr); BENCHMARK(AddrManAddThenGood); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 4aaa23059e..bca28d65ab 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -125,45 +125,45 @@ 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"); } 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 newOnly = true; - auto addr_ret1 = addrman->Select(newOnly).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(newOnly).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. @@ -171,17 +171,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()); @@ -189,6 +189,88 @@ 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_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));