Skip to content

net: call Select with reachable networks in ThreadOpenConnections #29436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
achow101 merged 3 commits into from
Sep 16, 2024
Merged

net: call Select with reachable networks in ThreadOpenConnections #29436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f698636
net: add `All()` in `ReachableNets`
brunoerg Feb 15, 2024
829becd
addrman: change `Select` to support multiple networks
brunoerg Feb 12, 2024
e4e3b44
net: call `Select` with reachable networks in `ThreadOpenConnections`
brunoerg Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions src/addrman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds
}
}

std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::optional<Network> network) const
std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, const std::unordered_set<Network>& networks) const
Comment thread
brunoerg marked this conversation as resolved.
Outdated
{
AssertLockHeld(cs);

Expand All @@ -719,13 +719,18 @@ std::pair<CAddress, NodeSeconds> 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()) {
Comment thread
brunoerg marked this conversation as resolved.
Outdated
new_count = 0;
tried_count = 0;
for (auto& network : networks) {

@jonatack jonatack Sep 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Network is a cheaply-copied int enum; no need to use it by reference, unless I'm missing something.

Suggested change
for (auto& network : networks) {
for (Network 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;
Comment thread
brunoerg marked this conversation as resolved.
Outdated
tried_count += counts.n_tried;
}
}

if (new_only && new_count == 0) return {};
Expand Down Expand Up @@ -758,9 +763,9 @@ std::pair<CAddress, NodeSeconds> 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;
}
Expand Down Expand Up @@ -1208,11 +1213,11 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision()
return ret;
}

std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::optional<Network> network) const
std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, const std::unordered_set<Network>& networks) const

@jonatack jonatack Sep 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing #include <unordered_set> in this file (include what you use)

/src/addrman.cpp should add these lines:
#include <algorithm>                // for max, min
#include <compare>                  // for operator<, strong_ordering, operator>, operator<=
#include <iterator>                 // for advance
#include <ratio>                    // for ratio
#include <set>                      // for set, _Rb_tree_const_iterator, operator==
#include <unordered_map>            // for unordered_map
#include "netgroup.h"               // for NetGroupManager
#include "sync.h"                   // for AssertLockHeldInternal, AssertLockHeld, LOCK, UniqueLock

{
LOCK(cs);
Check();
auto addrRet = Select_(new_only, network);
auto addrRet = Select_(new_only, networks);
Check();
return addrRet;
}
Expand Down Expand Up @@ -1315,9 +1320,9 @@ std::pair<CAddress, NodeSeconds> AddrMan::SelectTriedCollision()
return m_impl->SelectTriedCollision();
}

std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, std::optional<Network> network) const
std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, const std::unordered_set<Network>& networks) const
{
return m_impl->Select(new_only, network);
return m_impl->Select(new_only, networks);
}

std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
Expand Down
5 changes: 3 additions & 2 deletions src/addrman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <cstdint>
#include <memory>
#include <optional>
#include <unordered_set>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -154,12 +155,12 @@ class AddrMan
* 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<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<Network> network = std::nullopt) const;
std::pair<CAddress, NodeSeconds> Select(bool new_only = false, const std::unordered_set<Network>& networks = {}) const;

/**
* Return all or many randomly selected addresses, optionally by network.
Expand Down
4 changes: 2 additions & 2 deletions src/addrman_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ class AddrManImpl

std::pair<CAddress, NodeSeconds> SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);

std::pair<CAddress, NodeSeconds> Select(bool new_only, std::optional<Network> network) const
std::pair<CAddress, NodeSeconds> Select(bool new_only, const std::unordered_set<Network>& networks) const
EXCLUSIVE_LOCKS_REQUIRED(!cs);

std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const
Expand Down Expand Up @@ -252,7 +252,7 @@ class AddrManImpl

void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);

std::pair<CAddress, NodeSeconds> Select_(bool new_only, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs);
std::pair<CAddress, NodeSeconds> Select_(bool new_only, const std::unordered_set<Network>& networks) const EXCLUSIVE_LOCKS_REQUIRED(cs);

/** Helper to generalize looking up an addrman entry from either table.
*
Expand Down
2 changes: 1 addition & 1 deletion src/bench/addrman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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});
});
}

Expand Down
10 changes: 7 additions & 3 deletions src/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2692,6 +2692,8 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> 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()) {
Expand Down Expand Up @@ -2723,7 +2725,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> 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
Expand All @@ -2732,14 +2734,16 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> 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::tie(addr, addr_last_try) = addrman.Select(false, preferred_net);
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
Expand Down
7 changes: 7 additions & 0 deletions src/netbase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ class ReachableNets {
return Contains(addr.GetNetwork());
}

[[nodiscard]] std::unordered_set<Network> All() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
AssertLockNotHeld(m_mutex);
LOCK(m_mutex);
return m_reachable;
}

private:
mutable Mutex m_mutex;

Expand Down
44 changes: 24 additions & 20 deletions src/test/addrman_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,55 +196,59 @@ BOOST_AUTO_TEST_CASE(addrman_select)
BOOST_AUTO_TEST_CASE(addrman_select_by_network)
{
auto addrman = std::make_unique<AddrMan>(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
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());
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<Network> 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<Network> 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};
bool tried_selected{false};
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;
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 9 additions & 1 deletion src/test/fuzz/addrman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,15 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(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<Network> nets;
for (const auto& net : ALL_NETWORKS) {

@jonatack jonatack Sep 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (const auto& net : ALL_NETWORKS) {
for (Network net : ALL_NETWORKS) {

if (fuzzed_data_provider.ConsumeBool()) {
nets.insert(net);
}
}
(void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), nets);

std::optional<bool> in_new;
if (fuzzed_data_provider.ConsumeBool()) {
in_new = fuzzed_data_provider.ConsumeBool();
Expand Down