From 3270f0adad6ccbb8c004fb222f420e9b3ea32ea6 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Fri, 8 Mar 2024 16:27:38 -0500 Subject: [PATCH 1/2] net: Favor peers from addrman over fetching seednodes The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues: - First, if permanently set (e.g. running with seednode in a config file) we'd be signaling such seed every time we restart our node - Second, we will be giving the seed node way too much influence over our addrman, populating the latter even with data from the former even when unnecessary This changes the behavior to only add seednodes to `m_addr_fetch` if our addrman is empty, or little by little after we've spent some time trying addresses from our addrman. Also, seednodes are added to `m_addr_fetch` in random order, to avoid signaling the same node in case more than one seed is added and we happen to try them over multiple restarts --- src/net.cpp | 43 +++++++++++++++++++++++++++++++++++-------- src/net.h | 2 +- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 3d3f9f4ba7d..8e0f4b2a85e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -65,6 +65,9 @@ static constexpr std::chrono::minutes DUMP_PEERS_INTERVAL{15}; /** Number of DNS seeds to query when the number of connections is low. */ static constexpr int DNSSEEDS_TO_QUERY_AT_ONCE = 3; +/** Minimum number of outbound connections under which we will keep fetching our address seeds. */ +static constexpr int SEED_OUTBOUND_CONNECTION_THRESHOLD = 2; + /** How long to delay before querying DNS seeds * * If we have more than THRESHOLD entries in addrman, then it's likely @@ -2184,7 +2187,6 @@ void CConnman::WakeMessageHandler() void CConnman::ThreadDNSAddressSeed() { - constexpr int TARGET_OUTBOUND_CONNECTIONS = 2; int outbound_connection_count = 0; if (gArgs.IsArgSet("-seednode")) { @@ -2203,7 +2205,7 @@ void CConnman::ThreadDNSAddressSeed() } outbound_connection_count = GetFullOutboundConnCount(); - if (outbound_connection_count >= TARGET_OUTBOUND_CONNECTIONS) { + if (outbound_connection_count >= SEED_OUTBOUND_CONNECTION_THRESHOLD) { LogPrintf("P2P peers available. Finished fetching data from seed nodes.\n"); break; } @@ -2226,7 +2228,7 @@ void CConnman::ThreadDNSAddressSeed() } // Proceed with dnsseeds if seednodes hasn't reached the target or if forcednsseed is set - if (outbound_connection_count < TARGET_OUTBOUND_CONNECTIONS || seeds_right_now) { + if (outbound_connection_count < SEED_OUTBOUND_CONNECTION_THRESHOLD || seeds_right_now) { // goal: only query DNS seed if address need is acute // * If we have a reasonable number of peers in addrman, spend // some time trying them first. This improves user privacy by @@ -2257,7 +2259,7 @@ void CConnman::ThreadDNSAddressSeed() if (!interruptNet.sleep_for(w)) return; to_wait -= w; - if (GetFullOutboundConnCount() >= TARGET_OUTBOUND_CONNECTIONS) { + if (GetFullOutboundConnCount() >= SEED_OUTBOUND_CONNECTION_THRESHOLD) { if (found > 0) { LogPrintf("%d addresses found from DNS seeds\n", found); LogPrintf("P2P peers available. Finished DNS seeding.\n"); @@ -2452,7 +2454,7 @@ bool CConnman::MaybePickPreferredNetwork(std::optional& network) return false; } -void CConnman::ThreadOpenConnections(const std::vector connect) +void CConnman::ThreadOpenConnections(const std::vector connect, Span seed_nodes) { AssertLockNotHeld(m_unused_i2p_sessions_mutex); AssertLockNotHeld(m_reconnections_mutex); @@ -2492,12 +2494,28 @@ void CConnman::ThreadOpenConnections(const std::vector connect) bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS); const bool use_seednodes{gArgs.IsArgSet("-seednode")}; + auto seed_node_timer = NodeClock::now(); + bool add_addr_fetch{addrman.Size() == 0 && !seed_nodes.empty()}; + constexpr std::chrono::seconds ADD_NEXT_SEEDNODE = 10s; + if (!add_fixed_seeds) { LogPrintf("Fixed seeds are disabled\n"); } while (!interruptNet) { + if (add_addr_fetch) { + add_addr_fetch = false; + const auto& seed{SpanPopBack(seed_nodes)}; + AddAddrFetch(seed); + + if (addrman.Size() == 0) { + LogInfo("Empty addrman, adding seednode (%s) to addrfetch\n", seed); + } else { + LogInfo("Couldn't connect to peers from addrman after %d seconds. Adding seednode (%s) to addrfetch\n", ADD_NEXT_SEEDNODE.count(), seed); + } + } + ProcessAddrFetch(); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) @@ -2598,6 +2616,13 @@ void CConnman::ThreadOpenConnections(const std::vector connect) } } + if (!seed_nodes.empty() && nOutboundFullRelay < SEED_OUTBOUND_CONNECTION_THRESHOLD) { + if (NodeClock::now() > seed_node_timer + ADD_NEXT_SEEDNODE) { + seed_node_timer = NodeClock::now(); + add_addr_fetch = true; + } + } + ConnectionType conn_type = ConnectionType::OUTBOUND_FULL_RELAY; auto now = GetTime(); bool anchor = false; @@ -3254,8 +3279,10 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) i2p_sam, &interruptNet); } - for (const auto& strDest : connOptions.vSeedNodes) { - AddAddrFetch(strDest); + // Randomize the order in which we may query seednode to potentially prevent connecting to the same one every restart (and signal that we have restarted) + std::vector seed_nodes = connOptions.vSeedNodes; + if (!seed_nodes.empty()) { + std::shuffle(seed_nodes.begin(), seed_nodes.end(), FastRandomContext{}); } if (m_use_addrman_outgoing) { @@ -3316,7 +3343,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) if (connOptions.m_use_addrman_outgoing || !connOptions.m_specified_outgoing.empty()) { threadOpenConnections = std::thread( &util::TraceThread, "opencon", - [this, connect = connOptions.m_specified_outgoing] { ThreadOpenConnections(connect); }); + [this, connect = connOptions.m_specified_outgoing, seed_nodes = std::move(seed_nodes)] { ThreadOpenConnections(connect, seed_nodes); }); } // Process messages diff --git a/src/net.h b/src/net.h index a4e4b503607..e17d3c38a2f 100644 --- a/src/net.h +++ b/src/net.h @@ -1272,7 +1272,7 @@ private: void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); void AddAddrFetch(const std::string& strDest) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex); void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex); - void ThreadOpenConnections(std::vector connect) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); + void ThreadOpenConnections(std::vector connect, Span seed_nodes) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); void ThreadI2PAcceptIncoming(); void AcceptConnection(const ListenSocket& hListenSocket); From 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 30 Apr 2024 23:36:52 -0400 Subject: [PATCH 2/2] test: adds seednode functional tests Adds functional tests to test the interaction between seednode and the AddrMan --- test/functional/p2p_seednode.py | 55 +++++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 56 insertions(+) create mode 100755 test/functional/p2p_seednode.py diff --git a/test/functional/p2p_seednode.py b/test/functional/p2p_seednode.py new file mode 100755 index 00000000000..6c510a6a0b0 --- /dev/null +++ b/test/functional/p2p_seednode.py @@ -0,0 +1,55 @@ +#!/usr/bin/env python3 +# Copyright (c) 2019-2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +""" +Test seednode interaction with the AddrMan +""" +import random +import time + +from test_framework.test_framework import BitcoinTestFramework + +ADD_NEXT_SEEDNODE = 10 + + +class P2PSeedNodes(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.disable_autoconnect = False + + def test_no_seednode(self): + # Check that if no seednode is provided, the node proceeds as usual (without waiting) + with self.nodes[0].assert_debug_log(expected_msgs=[], unexpected_msgs=["Empty addrman, adding seednode", f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode"], timeout=ADD_NEXT_SEEDNODE): + self.restart_node(0) + + def test_seednode_empty_addrman(self): + seed_node = "0.0.0.1" + # Check that the seednode is added to m_addr_fetches on bootstrap on an empty addrman + with self.nodes[0].assert_debug_log(expected_msgs=[f"Empty addrman, adding seednode ({seed_node}) to addrfetch"], timeout=ADD_NEXT_SEEDNODE): + self.restart_node(0, extra_args=[f'-seednode={seed_node}']) + + def test_seednode_addrman_unreachable_peers(self): + seed_node = "0.0.0.2" + node = self.nodes[0] + # Fill the addrman with unreachable nodes + for i in range(10): + ip = f"{random.randrange(128,169)}.{random.randrange(1,255)}.{random.randrange(1,255)}.{random.randrange(1,255)}" + port = 8333 + i + node.addpeeraddress(ip, port) + + # Restart the node so seednode is processed again + with node.assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=ADD_NEXT_SEEDNODE * 1.5): + self.restart_node(0, extra_args=[f'-seednode={seed_node}']) + node.setmocktime(int(time.time()) + ADD_NEXT_SEEDNODE + 1) + + def run_test(self): + self.test_no_seednode() + self.test_seednode_empty_addrman() + self.test_seednode_addrman_unreachable_peers() + + +if __name__ == '__main__': + P2PSeedNodes(__file__).main() + diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 67693259d3c..8bd51fa4b13 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -405,6 +405,7 @@ BASE_SCRIPTS = [ 'feature_shutdown.py', 'wallet_migration.py', 'p2p_ibd_txrelay.py', + 'p2p_seednode.py', # Don't append tests at the end to avoid merge conflicts # Put them in a random line within the section that fits their approximate run-time ]