From 9c0871977839c28636eff975748182888299cd2b Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 28 May 2021 13:45:15 -0700 Subject: [PATCH 1/8] [test] Introduce test logic to query DNS seeds This commit introduces a DNS seed to the regest chain params in order to add coverage to the DNS querying logic. The first test checks that we do not query DNS seeds if we are able to succesfully connect to 2 outbound connections. Since we participate in ADDR relay with those connections, including sending a GETADDR message during the VERSION handshake, querying the DNS seeds is unnecessary. Co-authored-by: Martin Zumsande --- src/chainparams.cpp | 3 ++- test/functional/feature_config_args.py | 5 ++-- test/functional/p2p_dns_seeds.py | 34 ++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 4 files changed, 40 insertions(+), 3 deletions(-) create mode 100755 test/functional/p2p_dns_seeds.py diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 1b71c4db430..4f1ff1d5121 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/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index de9d0d2e803..07b74e65de4 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -158,8 +158,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 00000000000..254f9af4458 --- /dev/null +++ b/test/functional/p2p_dns_seeds.py @@ -0,0 +1,34 @@ +#!/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.""" + +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.existing_outbound_connections_test() + + 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") + + +if __name__ == '__main__': + P2PDNSSeeds().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 00527e78f15..1a59cfc7550 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', 'rpc_rawtransaction.py --legacy-wallet', From 75c05af361552eeecd100cee8cc40d4cd5a3aa27 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 28 May 2021 13:49:29 -0700 Subject: [PATCH 2/8] [test] Test logic to query DNS seeds with block-relay-only connections When a node is able to properly shutdown, it will persist its block-relay-only connections to the addrman. On startup, it will attempt to reconnect to these anchors. Since block-relay-only connections do not participate in ADDR relay, succesful connections are insufficient to skip querying the DNS seeds. This test fails prior to the changes in #22013. Co-authored-by: Martin Zumsande --- test/functional/p2p_dns_seeds.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/functional/p2p_dns_seeds.py b/test/functional/p2p_dns_seeds.py index 254f9af4458..b58607a5c9b 100755 --- a/test/functional/p2p_dns_seeds.py +++ b/test/functional/p2p_dns_seeds.py @@ -16,6 +16,7 @@ class P2PDNSSeeds(BitcoinTestFramework): def run_test(self): self.existing_outbound_connections_test() + self.existing_block_relay_connections_test() def existing_outbound_connections_test(self): # Make sure addrman is populated to enter the conditional where we @@ -29,6 +30,23 @@ class P2PDNSSeeds(BitcoinTestFramework): 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") + if __name__ == '__main__': P2PDNSSeeds().main() From 35851450a928ffacca240fadbf1747a42d5ba256 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 24 May 2021 15:56:32 -0700 Subject: [PATCH 3/8] [test] Test the interactions between -connect and -dnsseed --- test/functional/p2p_dns_seeds.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/functional/p2p_dns_seeds.py b/test/functional/p2p_dns_seeds.py index b58607a5c9b..213741f3a8a 100755 --- a/test/functional/p2p_dns_seeds.py +++ b/test/functional/p2p_dns_seeds.py @@ -15,9 +15,22 @@ class P2PDNSSeeds(BitcoinTestFramework): self.extra_args = [["-dnsseed=1"]] def run_test(self): + self.init_arg_tests() self.existing_outbound_connections_test() self.existing_block_relay_connections_test() + 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"]) + def existing_outbound_connections_test(self): # Make sure addrman is populated to enter the conditional where we # delay and potentially skip DNS seeding. From 26d0ffe4f2573e0297c9b0e095c2a0868929b08b Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 24 May 2021 17:08:54 -0700 Subject: [PATCH 4/8] [test] Test -forcednsseed causes querying DNS seeds --- test/functional/p2p_dns_seeds.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/functional/p2p_dns_seeds.py b/test/functional/p2p_dns_seeds.py index 213741f3a8a..4714de64dc8 100755 --- a/test/functional/p2p_dns_seeds.py +++ b/test/functional/p2p_dns_seeds.py @@ -18,6 +18,7 @@ class P2PDNSSeeds(BitcoinTestFramework): self.init_arg_tests() self.existing_outbound_connections_test() self.existing_block_relay_connections_test() + self.force_dns_test() def init_arg_tests(self): fakeaddr = "fakenodeaddr.fakedomain.invalid." @@ -60,6 +61,17 @@ class P2PDNSSeeds(BitcoinTestFramework): 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) + if __name__ == '__main__': P2PDNSSeeds().main() From 6f6b7df6bdcf863af160c0426e3a22028ca8259a Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 28 May 2021 13:30:35 -0700 Subject: [PATCH 5/8] [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed -dnsseed determines whether we run ThreadDNSAddressSeed and potentially query the DNS seeds for addresses. -forcednsseed tells the node to force querying the DNS seeds even if we have sufficient addresses or current connections. This commit disallows starting up with explicitly conflicting parameters. --- src/init.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 593128747e9..11a87dd4417 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -853,6 +853,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)) { From 6395c8ed5689ea72e9a1618f14551775246f6361 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 28 May 2021 13:38:46 -0700 Subject: [PATCH 6/8] [test] Test the interactions between -forcednsseed and -dnsseed Test that passing conflicting parameters for the two causes a startup error. This logic also impacts -connect, which soft sets -dnsseed, so add a test for that too. --- test/functional/p2p_dns_seeds.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/functional/p2p_dns_seeds.py b/test/functional/p2p_dns_seeds.py index 4714de64dc8..d9977dccee2 100755 --- a/test/functional/p2p_dns_seeds.py +++ b/test/functional/p2p_dns_seeds.py @@ -32,6 +32,24 @@ class P2PDNSSeeds(BitcoinTestFramework): 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. From 4c89e24f64c1dc1a56a3bcb6b5e2b4fb95e8b29f Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 24 May 2021 17:56:22 -0700 Subject: [PATCH 7/8] [test] Test the delay before querying DNS seeds When starting up with a populated addrman, ThreadDNSAddressSeed adds a delay during which time the node may be able to connect to some peers. This commit tests the delay changes based on the number of addresses in the addrman. --- test/functional/p2p_dns_seeds.py | 34 ++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/functional/p2p_dns_seeds.py b/test/functional/p2p_dns_seeds.py index d9977dccee2..e58ad8e0fc8 100755 --- a/test/functional/p2p_dns_seeds.py +++ b/test/functional/p2p_dns_seeds.py @@ -4,6 +4,8 @@ # 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 @@ -19,6 +21,7 @@ class P2PDNSSeeds(BitcoinTestFramework): 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." @@ -90,6 +93,37 @@ class P2PDNSSeeds(BitcoinTestFramework): # 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() From 82b6f89819e55af26f5264678e0f93052934bcb3 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 24 May 2021 12:57:50 -0700 Subject: [PATCH 8/8] [style] Small style improvements to DNS parameters --- src/init.cpp | 2 +- src/net.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 11a87dd4417..1cf7dce8f7c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -431,7 +431,7 @@ void SetupServerArgs(NodeContext& node) 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=", "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=", strprintf("Maintain at most 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); diff --git a/src/net.h b/src/net.h index b43916c55ec..bacce326fe0 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;