From f698636ec86c004ab331994559c163b7319e6423 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Thu, 15 Feb 2024 09:33:24 -0300 Subject: [PATCH 1/3] net: add `All()` in `ReachableNets` Co-authored-by: Vasil Dimov --- src/netbase.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/netbase.h b/src/netbase.h index 8ef6c28996..bf4d7ececc 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -134,6 +134,13 @@ public: return Contains(addr.GetNetwork()); } + [[nodiscard]] std::unordered_set All() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + { + AssertLockNotHeld(m_mutex); + LOCK(m_mutex); + return m_reachable; + } + private: mutable Mutex m_mutex; From 829becd990b504a2e8a57fa8a6ff6ac6ae8ff900 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Mon, 12 Feb 2024 18:10:33 -0300 Subject: [PATCH 2/3] addrman: change `Select` to support multiple networks --- src/addrman.cpp | 33 ++++++++++++++++------------ src/addrman.h | 5 +++-- src/addrman_impl.h | 4 ++-- src/bench/addrman.cpp | 2 +- src/net.cpp | 6 +++++- src/test/addrman_tests.cpp | 44 +++++++++++++++++++++----------------- src/test/fuzz/addrman.cpp | 10 ++++++++- 7 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 673d25efcf..7aa97fa1b7 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -710,7 +710,7 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds } } -std::pair AddrManImpl::Select_(bool new_only, std::optional network) const +std::pair AddrManImpl::Select_(bool new_only, const std::unordered_set& networks) const { AssertLockHeld(cs); @@ -719,13 +719,18 @@ std::pair AddrManImpl::Select_(bool new_only, std::option 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 (!networks.empty()) { + new_count = 0; + tried_count = 0; + for (auto& network : networks) { + auto it = m_network_counts.find(network); + if (it == m_network_counts.end()) { + continue; + } + auto counts = it->second; + new_count += counts.n_new; + tried_count += counts.n_tried; + } } if (new_only && new_count == 0) return {}; @@ -758,9 +763,9 @@ std::pair AddrManImpl::Select_(bool new_only, std::option position = (initial_position + i) % ADDRMAN_BUCKET_SIZE; node_id = GetEntry(search_tried, bucket, position); if (node_id != -1) { - if (network.has_value()) { + if (!networks.empty()) { const auto it{mapInfo.find(node_id)}; - if (Assume(it != mapInfo.end()) && it->second.GetNetwork() == *network) break; + if (Assume(it != mapInfo.end()) && networks.contains(it->second.GetNetwork())) break; } else { break; } @@ -1208,11 +1213,11 @@ std::pair AddrManImpl::SelectTriedCollision() return ret; } -std::pair AddrManImpl::Select(bool new_only, std::optional network) const +std::pair AddrManImpl::Select(bool new_only, const std::unordered_set& networks) const { LOCK(cs); Check(); - auto addrRet = Select_(new_only, network); + auto addrRet = Select_(new_only, networks); Check(); return addrRet; } @@ -1315,9 +1320,9 @@ std::pair AddrMan::SelectTriedCollision() return m_impl->SelectTriedCollision(); } -std::pair AddrMan::Select(bool new_only, std::optional network) const +std::pair AddrMan::Select(bool new_only, const std::unordered_set& networks) const { - return m_impl->Select(new_only, network); + return m_impl->Select(new_only, networks); } std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered) const diff --git a/src/addrman.h b/src/addrman.h index be2ee8c2cb..ba6e13bf97 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -154,12 +155,12 @@ public: * an address from the new table or an empty pair. Passing `false` will return an * 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 + * @param[in] networks Select only addresses of these networks (empty = all). Passing networks may * slow down the search. * @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, std::optional network = std::nullopt) const; + std::pair Select(bool new_only = false, const std::unordered_set& networks = {}) const; /** * Return all or many randomly selected addresses, optionally by network. diff --git a/src/addrman_impl.h b/src/addrman_impl.h index dd7f7b318f..a581f1f6f9 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -125,7 +125,7 @@ public: std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::pair Select(bool new_only, std::optional network) const + std::pair Select(bool new_only, const std::unordered_set& networks) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered = true) const @@ -252,7 +252,7 @@ private: void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); - std::pair Select_(bool new_only, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::pair Select_(bool new_only, const std::unordered_set& networks) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Helper to generalize looking up an addrman entry from either table. * diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index c0ef7b2279..ceef6c29ab 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -133,7 +133,7 @@ static void AddrManSelectByNetwork(benchmark::Bench& bench) FillAddrMan(addrman); bench.run([&] { - (void)addrman.Select(/*new_only=*/false, NET_I2P); + (void)addrman.Select(/*new_only=*/false, {NET_I2P}); }); } diff --git a/src/net.cpp b/src/net.cpp index 380146ed65..2d504e7c4c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2739,7 +2739,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect, Spa // If preferred_net has a value set, pick an extra outbound // peer from that network. The eviction logic in net_processing // ensures that a peer from another network will be evicted. - std::tie(addr, addr_last_try) = addrman.Select(false, preferred_net); + std::unordered_set preferred_nets; + if (preferred_net.has_value()) { + preferred_nets = {*preferred_net}; + } + std::tie(addr, addr_last_try) = addrman.Select(false, preferred_nets); } // Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index da6ff77924..c4f58ebecf 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -196,21 +196,21 @@ BOOST_AUTO_TEST_CASE(addrman_select) 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()); + 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=*/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 @@ -218,25 +218,29 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network) 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()); + 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); + std::unordered_set nets_with_entries = {NET_IPV4, NET_I2P}; + BOOST_CHECK(addrman->Select(/*new_only=*/false, nets_with_entries).first.IsValid()); + 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()); + std::unordered_set nets_without_entries = {NET_IPV6, NET_ONION, NET_CJDNS}; + BOOST_CHECK(!addrman->Select(/*new_only=*/false, nets_without_entries).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); + 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); + 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}; @@ -244,7 +248,7 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network) int counter = 256; while (--counter > 0 && (!new_selected || !tried_selected)) { - const CAddress selected{addrman->Select(/*new_only=*/false, NET_I2P).first}; + 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; @@ -277,7 +281,7 @@ BOOST_AUTO_TEST_CASE(addrman_select_special) // 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_CHECK(addrman->Select(/*new_only=*/false, {NET_IPV4}).first == addr1); } BOOST_AUTO_TEST_CASE(addrman_new_collisions) diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index c324b4fdd9..593086af21 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -285,7 +285,15 @@ FUZZ_TARGET(addrman, .init = initialize_addrman) auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange(0, 4096); auto filtered = fuzzed_data_provider.ConsumeBool(); (void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered); - (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network); + + std::unordered_set nets; + for (const auto& net : ALL_NETWORKS) { + if (fuzzed_data_provider.ConsumeBool()) { + nets.insert(net); + } + } + (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), nets); + std::optional in_new; if (fuzzed_data_provider.ConsumeBool()) { in_new = fuzzed_data_provider.ConsumeBool(); From e4e3b44e9cc7227b3ad765397c884999f57bac2e Mon Sep 17 00:00:00 2001 From: brunoerg Date: Tue, 13 Feb 2024 10:40:00 -0300 Subject: [PATCH 3/3] net: call `Select` with reachable networks in `ThreadOpenConnections` Calling `Select` with reachable networks can avoid unecessary calls and avoid exceed the max number of tries. --- src/net.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 2d504e7c4c..7629c8ab56 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2692,6 +2692,8 @@ void CConnman::ThreadOpenConnections(const std::vector connect, Spa const auto current_time{NodeClock::now()}; int nTries = 0; + const auto reachable_nets{g_reachable_nets.All()}; + while (!interruptNet) { if (anchor && !m_anchors.empty()) { @@ -2723,7 +2725,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, Spa if (!addr.IsValid()) { // No tried table collisions. Select a new table address // for our feeler. - std::tie(addr, addr_last_try) = addrman.Select(true); + std::tie(addr, addr_last_try) = addrman.Select(true, reachable_nets); } else if (AlreadyConnectedToAddress(addr)) { // If test-before-evict logic would have us connect to a // peer that we're already connected to, just mark that @@ -2732,18 +2734,16 @@ void CConnman::ThreadOpenConnections(const std::vector connect, Spa // a currently-connected peer. addrman.Good(addr); // Select a new table address for our feeler instead. - std::tie(addr, addr_last_try) = addrman.Select(true); + std::tie(addr, addr_last_try) = addrman.Select(true, reachable_nets); } } else { // Not a feeler // If preferred_net has a value set, pick an extra outbound // peer from that network. The eviction logic in net_processing // ensures that a peer from another network will be evicted. - std::unordered_set preferred_nets; - if (preferred_net.has_value()) { - preferred_nets = {*preferred_net}; - } - std::tie(addr, addr_last_try) = addrman.Select(false, preferred_nets); + std::tie(addr, addr_last_try) = preferred_net.has_value() + ? addrman.Select(false, {*preferred_net}) + : addrman.Select(false, reachable_nets); } // Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups