Merge bitcoin/bitcoin#22098: [test, init] DNS seed querying logic

82b6f89819e55af26f5264678e0f93052934bcb3 [style] Small style improvements to DNS parameters (Amiti Uttarwar)
4c89e24f64c1dc1a56a3bcb6b5e2b4fb95e8b29f [test] Test the delay before querying DNS seeds (Amiti Uttarwar)
6395c8ed5689ea72e9a1618f14551775246f6361 [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar)
6f6b7df6bdcf863af160c0426e3a22028ca8259a [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar)
26d0ffe4f2573e0297c9b0e095c2a0868929b08b [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar)
35851450a928ffacca240fadbf1747a42d5ba256 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar)
75c05af361552eeecd100cee8cc40d4cd5a3aa27 [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar)
9c0871977839c28636eff975748182888299cd2b [test] Introduce test logic to query DNS seeds (Amiti Uttarwar)

Pull request description:

  This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in #22013 (and then some).

  The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`.

  The tests include:
  * parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed`
  * the delay before querying DNS seeds depending on how many addresses are in the addrman
  * the behavior of `-forcednsseed`
  * skipping DNS querying if we have outbound full relay connections & not block-relay-only connections

  Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽

ACKs for top commit:
  mzumsande:
    ACK 82b6f89819e55af26f5264678e0f93052934bcb3
  jnewbery:
    reACK 82b6f89819e55af26f5264678e0f93052934bcb3

Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d

6 files changed, 144 insertions, 7 deletions

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index a8b45685d0..74463f347d 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -435,7 +435,8 @@ public:
         assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));
 
         vFixedSeeds.clear(); //!< Regtest mode doesn't have any fixed seeds.
-        vSeeds.clear();      //!< Regtest mode doesn't have any DNS seeds.
+        vSeeds.clear();
+        vSeeds.emplace_back("dummySeed.invalid.");
 
         fDefaultConsistencyChecks = true;
         fRequireStandard = true;
diff --git a/src/init.cpp b/src/init.cpp
index 75394d96b1..aee8b78999 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -426,7 +426,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used)", DEFAULT_DNSSEED), ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
     argsman.AddArg("-externalip=<ip>", "Specify your own public address", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-fixedseeds", strprintf("Allow fixed seeds if DNS seeds don't provide peers (default: %u)", DEFAULT_FIXEDSEEDS), ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
-    argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
     argsman.AddArg("-listen", "Accept connections from outside (default: 1 if no -proxy or -connect)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-listenonion", strprintf("Automatically create Tor onion service (default: %d)", DEFAULT_LISTEN_ONION), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> connections to peers (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -848,6 +848,11 @@ bool AppInitParameterInteraction(const ArgsManager& args)
             return InitError(_("Prune mode is incompatible with -coinstatsindex."));
     }
 
+    // If -forcednsseed is set to true, ensure -dnsseed has not been set to false
+    if (args.GetBoolArg("-forcednsseed", DEFAULT_FORCEDNSSEED) && !args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)){
+        return InitError(_("Cannot set -forcednsseed to true when setting -dnsseed to false."));
+    }
+
     // -bind and -whitebind can't be set when not listening
     size_t nUserBind = args.GetArgs("-bind").size() + args.GetArgs("-whitebind").size();
     if (nUserBind != 0 && !args.GetBoolArg("-listen", DEFAULT_LISTEN)) {
diff --git a/src/net.h b/src/net.h
index a8836dfcb4..889d57b74c 100644
--- a/src/net.h
+++ b/src/net.h
@@ -79,9 +79,9 @@ static const int64_t DEFAULT_PEER_CONNECT_TIMEOUT = 60;
 /** Number of file descriptors required for message capture **/
 static const int NUM_FDS_MESSAGE_CAPTURE = 1;
 
-static const bool DEFAULT_FORCEDNSSEED = false;
-static const bool DEFAULT_DNSSEED = true;
-static const bool DEFAULT_FIXEDSEEDS = true;
+static constexpr bool DEFAULT_FORCEDNSSEED{false};
+static constexpr bool DEFAULT_DNSSEED{true};
+static constexpr bool DEFAULT_FIXEDSEEDS{true};
 static const size_t DEFAULT_MAXRECEIVEBUFFER = 5 * 1000;
 static const size_t DEFAULT_MAXSENDBUFFER    = 1 * 1000;
 
diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index 5a23df8a13..24c8a8987a 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -159,8 +159,9 @@ class ConfArgsTest(BitcoinTestFramework):
         self.stop_node(0)
 
         # No peers.dat exists and -dnsseed=1
-        # We expect the node will use DNS Seeds, but Regtest mode has 0 DNS seeds
-        # So after 60 seconds, the node should fallback to fixed seeds (this is a slow test)
+        # We expect the node will use DNS Seeds, but Regtest mode does not have
+        # any valid DNS seeds. So after 60 seconds, the node should fallback to
+        # fixed seeds
         assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
         start = int(time.time())
         with self.nodes[0].assert_debug_log(expected_msgs=[
diff --git a/test/functional/p2p_dns_seeds.py b/test/functional/p2p_dns_seeds.py
new file mode 100755
index 0000000000..e58ad8e0fc
--- /dev/null
+++ b/test/functional/p2p_dns_seeds.py
@@ -0,0 +1,129 @@
+#!/usr/bin/env python3
+# Copyright (c) 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 ThreadDNSAddressSeed logic for querying DNS seeds."""
+
+import itertools
+
+from test_framework.p2p import P2PInterface
+from test_framework.test_framework import BitcoinTestFramework
+
+
+class P2PDNSSeeds(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+        self.extra_args = [["-dnsseed=1"]]
+
+    def run_test(self):
+        self.init_arg_tests()
+        self.existing_outbound_connections_test()
+        self.existing_block_relay_connections_test()
+        self.force_dns_test()
+        self.wait_time_tests()
+
+    def init_arg_tests(self):
+        fakeaddr = "fakenodeaddr.fakedomain.invalid."
+
+        self.log.info("Check that setting -connect disables -dnsseed by default")
+        self.nodes[0].stop_node()
+        with self.nodes[0].assert_debug_log(expected_msgs=["DNS seeding disabled"]):
+            self.start_node(0, [f"-connect={fakeaddr}"])
+
+        self.log.info("Check that running -connect and -dnsseed means DNS logic runs.")
+        with self.nodes[0].assert_debug_log(expected_msgs=["Loading addresses from DNS seed"], timeout=12):
+            self.restart_node(0, [f"-connect={fakeaddr}", "-dnsseed=1"])
+
+        self.log.info("Check that running -forcednsseed and -dnsseed=0 throws an error.")
+        self.nodes[0].stop_node()
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg="Error: Cannot set -forcednsseed to true when setting -dnsseed to false.",
+            extra_args=["-forcednsseed=1", "-dnsseed=0"],
+        )
+
+        self.log.info("Check that running -forcednsseed and -connect throws an error.")
+        # -connect soft sets -dnsseed to false, so throws the same error
+        self.nodes[0].stop_node()
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg="Error: Cannot set -forcednsseed to true when setting -dnsseed to false.",
+            extra_args=["-forcednsseed=1", f"-connect={fakeaddr}"],
+        )
+
+        # Restore default bitcoind settings
+        self.restart_node(0)
+
+    def existing_outbound_connections_test(self):
+        # Make sure addrman is populated to enter the conditional where we
+        # delay and potentially skip DNS seeding.
+        self.nodes[0].addpeeraddress("192.0.0.8", 8333)
+
+        self.log.info("Check that we *do not* query DNS seeds if we have 2 outbound connections")
+
+        self.restart_node(0)
+        with self.nodes[0].assert_debug_log(expected_msgs=["P2P peers available. Skipped DNS seeding."], timeout=12):
+            for i in range(2):
+                self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i, connection_type="outbound-full-relay")
+
+    def existing_block_relay_connections_test(self):
+        # Make sure addrman is populated to enter the conditional where we
+        # delay and potentially skip DNS seeding. No-op when run after
+        # existing_outbound_connections_test.
+        self.nodes[0].addpeeraddress("192.0.0.8", 8333)
+
+        self.log.info("Check that we *do* query DNS seeds if we only have 2 block-relay-only connections")
+
+        self.restart_node(0)
+        with self.nodes[0].assert_debug_log(expected_msgs=["Loading addresses from DNS seed"], timeout=12):
+            # This mimics the "anchors" logic where nodes are likely to
+            # reconnect to block-relay-only connections on startup.
+            # Since we do not participate in addr relay with these connections,
+            # we still want to query the DNS seeds.
+            for i in range(2):
+                self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i, connection_type="block-relay-only")
+
+    def force_dns_test(self):
+        self.log.info("Check that we query DNS seeds if -forcednsseed param is set")
+
+        with self.nodes[0].assert_debug_log(expected_msgs=["Loading addresses from DNS seed"], timeout=12):
+            # -dnsseed defaults to 1 in bitcoind, but 0 in the test framework,
+            # so pass it explicitly here
+            self.restart_node(0, ["-forcednsseed", "-dnsseed=1"])
+
+        # Restore default for subsequent tests
+        self.restart_node(0)
+
+    def wait_time_tests(self):
+        self.log.info("Check the delay before querying DNS seeds")
+
+        # Populate addrman with < 1000 addresses
+        for i in range(5):
+            a = f"192.0.0.{i}"
+            self.nodes[0].addpeeraddress(a, 8333)
+
+        # The delay should be 11 seconds
+        with self.nodes[0].assert_debug_log(expected_msgs=["Waiting 11 seconds before querying DNS seeds.\n"]):
+            self.restart_node(0)
+
+        # Populate addrman with > 1000 addresses
+        for i in itertools.count():
+            first_octet = i % 2 + 1
+            second_octet = i % 256
+            third_octet = i % 100
+            a = f"{first_octet}.{second_octet}.{third_octet}.1"
+            self.nodes[0].addpeeraddress(a, 8333)
+            if (i > 1000 and i % 100 == 0):
+                # The addrman size is non-deterministic because new addresses
+                # are sorted into buckets, potentially displacing existing
+                # addresses. Periodically check if we have met the desired
+                # threshold.
+                if len(self.nodes[0].getnodeaddresses(0)) > 1000:
+                    break
+
+        # The delay should be 5 mins
+        with self.nodes[0].assert_debug_log(expected_msgs=["Waiting 300 seconds before querying DNS seeds.\n"]):
+            self.restart_node(0)
+
+
+if __name__ == '__main__':
+    P2PDNSSeeds().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 0a73891f2a..fecf52d53a 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -121,6 +121,7 @@ BASE_SCRIPTS = [
     'wallet_listreceivedby.py --legacy-wallet',
     'wallet_listreceivedby.py --descriptors',
     'wallet_abandonconflict.py --legacy-wallet',
+    'p2p_dns_seeds.py',
     'wallet_abandonconflict.py --descriptors',
     'feature_csv_activation.py',
     'wallet_address_types.py --legacy-wallet',