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

@@ -2489,16 +2489,38 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> 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 && !m_seed_nodes.empty()};
+    constexpr std::chrono::seconds ADD_NEXT_SEEDNODE = 10s;
+
     if (!add_fixed_seeds) {
         LogPrintf("Fixed seeds are disabled\n");
     }
 
     while (!interruptNet)
     {
+        if (NodeClock::now() > seed_node_timer + ADD_NEXT_SEEDNODE && !m_seed_nodes.empty()) {
+            seed_node_timer = NodeClock::now();
+            add_addr_fetch = true;
+        }
+
+        if (add_addr_fetch) {
+            add_addr_fetch = false;
+            auto seed = m_seed_nodes.back();
+            m_seed_nodes.pop_back();
+            AddAddrFetch(seed);
+
+            if (addrman.Size() == 0) {
+                LogPrintf("Empty addrman, adding seednode (%s) to addrfetch\n", seed);
+            } else {
+                LogPrintf("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)))
-            return;
+        return;
 
         PerformReconnections();
 
@@ -3240,8 +3262,10 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
                                                                 i2p_sam.proxy, &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)
+    if (!connOptions.vSeedNodes.empty()) {
+        m_seed_nodes = connOptions.vSeedNodes;
+        Shuffle(m_seed_nodes.begin(), m_seed_nodes.end(), FastRandomContext{});
     }
 
     if (m_use_addrman_outgoing) {

src/net.h

@@ -1422,6 +1422,8 @@ class CConnman
     mutable RecursiveMutex m_nodes_mutex;
     std::atomic<NodeId> nLastNodeId{0};
     unsigned int nPrevNodeCount{0};
+    // Collection of seed nodes, populated by connOptions.vSeedNodes
+    std::vector<std::string> m_seed_nodes;
 
     // Stores number of full-tx connections (outbound and manual) per network
     std::array<unsigned int, Network::NET_MAX> m_network_conn_counts GUARDED_BY(m_nodes_mutex) = {};

test/functional/p2p_seednode.py

@@ -0,0 +1,69 @@
+#!/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 time
+import random
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.messages import (CAddress, msg_addr)
+from test_framework.p2p import (
+    P2PInterface,
+    P2P_SERVICES,
+)
+from test_framework.wallet import MiniWallet
+
+def setup_addr_msg(num, time):
+    addrs = []
+    for i in range(num):
+        addr = CAddress()
+        addr.time = time + random.randrange(-100, 100)
+        addr.nServices = P2P_SERVICES
+        addr.ip = f"{random.randrange(128,169)}.{random.randrange(1,255)}.{random.randrange(1,255)}.{random.randrange(1,255)}"
+        addr.port = 8333 + i
+        addrs.append(addr)
+
+    msg = msg_addr()
+    msg.addrs = addrs
+    return msg
+
+
+class P2PSeedNodes(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+        self.disable_autoconnect = False
+
+    def test_no_seednode(self):
+        # 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=[], unexpected_msgs=["Empty addrman, adding seednode", "Couldn't connect to peers from addrman after 10 seconds. Adding seednode"], timeout=10):
+            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=10):
+            self.restart_node(0, extra_args=[f'-seednode={seed_node}'])
+
+    def test_seednode_addrman_unreachable_peers(self):
+        seed_node = "0.0.0.2"
+        # Fill the addrman with unreachable nodes
+        addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
+        addr_source.send_and_ping(setup_addr_msg(1, int(time.time())))
+
+        # Restart the node so seednode is processed again
+        with self.nodes[0].assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after 10 seconds. Adding seednode ({seed_node}) to addrfetch"], timeout=15):
+            self.restart_node(0, extra_args=[f'-seednode={seed_node}'])
+
+    def run_test(self):
+        self.test_no_seednode()
+        self.test_seednode_empty_addrman()
+        self.test_seednode_addrman_unreachable_peers()
+
+
+if __name__ == '__main__':
+    P2PSeedNodes().main()
+

test/functional/test_runner.py

@@ -397,6 +397,7 @@
     '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
 ]