[test, init] DNS seed querying logic

amitiuttarwar commented at 10:10 pm on May 28, 2021: contributor

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 🙌🏽

DrahtBot added the label P2P on May 28, 2021

DrahtBot added the label Validation on May 28, 2021

amitiuttarwar force-pushed on May 28, 2021

amitiuttarwar added the label Tests on May 28, 2021

amitiuttarwar removed the label Validation on May 28, 2021

lsilva01 approved

lsilva01 commented at 5:32 am on May 29, 2021: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/22098/commits/75ee018f9b8371cfb16bd3ad8f9491d386d9c238 on Ubuntu 20.04.

in test/functional/p2p_dns_seeds.py:72 in 75ee018f9b outdated

34+                self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i, connection_type="outbound-full-relay")
35+
36+    def existing_block_relay_connections_test(self):
37+        # Make sure addrman is populated to enter the conditional where we
38+        # delay and potentially skip DNS seeding.
39+        self.nodes[0].addpeeraddress("192.0.0.8", 8333)

mzumsande commented at 8:58 pm on May 30, 2021:

Not that it matters much, but at this point, the address is already in addrman, so this returns success=false.

amitiuttarwar commented at 9:23 pm on June 2, 2021:

ah, totally. this line is unnecessary when running the tests together, but I added it so the existing_block_relay_connections_test setup would not be dependent on existing_outbound_connections_test. would it better if I change the IP address to be something new?

mzumsande commented at 10:11 pm on June 2, 2021:

oh, I see. No, I don’t think that is necessary.

amitiuttarwar commented at 9:49 pm on July 28, 2021:

didn’t change it, but added a comment to clarify.

in test/functional/p2p_dns_seeds.py:67 in 75ee018f9b outdated

62+
63+    def init_arg_tests(self):
64+        self.log.info("Check that setting -connect disables -dnsseed by default")
65+        self.nodes[0].stop_node()
66+        with(self.nodes[0].assert_debug_log(expected_msgs=["DNS seeding disabled"])):
67+            self.start_node(0, ["-connect"])

mzumsande commented at 9:18 pm on May 30, 2021:

Not sure why this use of -connect is even allowed considering that -connect without an argument leads to the node trying to connect to nothingness, but maybe add some dummy argument so that it is not confused with a boolean option.

amitiuttarwar commented at 2:00 am on August 3, 2021:

oops, forgot to comment / resolve, but this has been fixed for all the -connect calls

willcl-ark approved

willcl-ark commented at 3:33 pm on June 2, 2021: member

This looks like a good change which clears up the behaviour of some contradictory flag combinations and more thoroughly tests the dns fallback behaviour. tACK each commit in the series.

Logic added in ea0ad085e80106aee305628f53d4b7409ed9e44c is exercised by the new test test/functional/p2p_dns_seeds.py

laanwj commented at 4:55 pm on June 2, 2021: member

Thanks for adding tests for this functionality. This goes as far as can be reasonably expected without emulating a DNS server in the tests :sweat_smile:

Code review ACK 75ee018f9b8371cfb16bd3ad8f9491d386d9c238

in test/functional/p2p_dns_seeds.py:20 in 1e03a9e7f3 outdated

16@@ -17,6 +17,7 @@ def set_test_params(self):
17     def run_test(self):
18         self.existing_outbound_connections_test()
19         self.existing_block_relay_connections_test()
20+        self.init_arg_tests()

mzumsande commented at 5:45 pm on June 2, 2021:

The test runs quite long (much waiting). I don’t think it’s possible to prevent this (mocking time doesn’t seem possible) but running init_arg_tests before existing_outbound_connections_test would save one 11 second wait because addrman would still be empty.

amitiuttarwar commented at 9:49 pm on July 28, 2021:

good call! changed the ordering

mzumsande commented at 5:59 pm on June 2, 2021: member

Concept ACK. Some non-critical suggestions below, although maybe it’s worthwile speeding the test up a bit.

in test/functional/p2p_dns_seeds.py:61 in 75ee018f9b outdated

56+            # -dnsseed defaults to 1 in bitcoind, but 0 in the test framework,
57+            # so pass it explicitly here
58+            self.restart_node(0, ["-forcednsseed", "-dnsseed=1"])
59+
60+        # Restore default for subsequent tests
61+        self.restart_node(0, ["-forcednsseed=0"])

jnewbery commented at 3:57 pm on June 3, 2021:

You don’t need to specify the default value for this arg:

0        self.restart_node(0)

will just restart with the default extra args.

amitiuttarwar commented at 9:51 pm on July 28, 2021:

ah I see! I thought the args forwarded between restarts, but I see now its just the initial setup args that get retained. updated these calls.

in test/functional/p2p_dns_seeds.py:89 in 75ee018f9b outdated

84+            expected_msg="Error: Cannot set -forcednsseed to true when setting -dnsseed to false.",
85+            extra_args=["-forcednsseed=1", "-connect"],
86+        )
87+
88+        # Restore default bitcoind settings
89+        self.restart_node(0, ["-forcednsseed=0", "-dnsseed=1", "-connect=0"])

jnewbery commented at 3:57 pm on June 3, 2021:

As above:

0        self.restart_node(0)

amitiuttarwar commented at 9:51 pm on July 28, 2021:

done

in test/functional/p2p_dns_seeds.py:104 in 75ee018f9b outdated

 99+        # The delay should be 11 seconds
100+        with(self.nodes[0].assert_debug_log(expected_msgs=["Waiting 11 seconds before querying DNS seeds.\n"])):
101+            self.restart_node(0)
102+
103+        # Populate addrman with > 1000 addresses
104+        for i in range(1500):

jnewbery commented at 3:58 pm on June 3, 2021:

Consider:

0        for i in itertools.count():

To avoid the magic 1500 constant. You’ll need to import itertools.

amitiuttarwar commented at 9:53 pm on July 28, 2021:

nice! in addition to avoiding the magic number, it means the test has the flexibility to end earlier if we meet the required number of addresses in addrman.

in src/net.h:84 in 75ee018f9b outdated

78@@ -79,9 +79,9 @@ static const int64_t DEFAULT_PEER_CONNECT_TIMEOUT = 60;
79 /** Number of file descriptors required for message capture **/
80 static const int NUM_FDS_MESSAGE_CAPTURE = 1;
81 
82-static const bool DEFAULT_FORCEDNSSEED = false;
83-static const bool DEFAULT_DNSSEED = true;
84-static const bool DEFAULT_FIXEDSEEDS = true;
85+static constexpr bool DEFAULT_FORCEDNSSEED = false;
86+static constexpr bool DEFAULT_DNSSEED = true;
87+static constexpr bool DEFAULT_FIXEDSEEDS = true;

jnewbery commented at 4:03 pm on June 3, 2021:

If you’re touching these anyway…

0static constexpr bool DEFAULT_FORCEDNSSEED{false};
1static constexpr bool DEFAULT_DNSSEED{true};
2static constexpr bool DEFAULT_FIXEDSEEDS{true};

amitiuttarwar commented at 9:53 pm on July 28, 2021:

done

jnewbery commented at 4:06 pm on June 3, 2021: member

ACK 75ee018f9b8371cfb16bd3ad8f9491d386d9c238

Thanks for adding these tests!

amitiuttarwar force-pushed on Jul 28, 2021

amitiuttarwar commented at 9:59 pm on July 28, 2021: contributor

thank you for the reviews @lsilva01, @willcl-ark, @laanwj, @mzumsande & @jnewbery!

I’ve incorporated all the review comments (diff).

I also changed the regtest dns seed to be dummySeed.invalid (diff). Since .invalid is reserved to ensure no real results will be returned (wiki, IETF RFC), it seems like a good fit here.

jnewbery commented at 2:17 pm on July 29, 2021: member

Code review ACK a8834fa

kristapsk approved

kristapsk commented at 2:24 pm on July 29, 2021: contributor

ACK a8834fa0513d6bfcd4dfc759bbb841c02fcf8517

in test/functional/p2p_dns_seeds.py:30 in a8834fa051 outdated

25+
26+    def init_arg_tests(self):
27+        self.log.info("Check that setting -connect disables -dnsseed by default")
28+        self.nodes[0].stop_node()
29+        with(self.nodes[0].assert_debug_log(expected_msgs=["DNS seeding disabled"])):
30+            self.start_node(0, ["-connect=fakenodeaddr"])

practicalswift commented at 6:37 pm on July 29, 2021:

Could use a FQDN such as fakenodeaddr.fakedomain.invalid. (note the ending dot) be used here to make it unambiguous and thus make sure we don’t risk causing a DNS lookup for fakenodeaddr.<local domain name as specified in /etc/resolv.conf>?

amitiuttarwar commented at 6:24 pm on July 30, 2021:

ah cool, thanks for the suggestion! I updated all 3 uses of -connect= to use this FQDN.

in test/functional/p2p_dns_seeds.py:48 in a8834fa051 outdated

43+        self.log.info("Check that running -forcednsseed and -connect throws an error.")
44+        # -connect soft sets -dnsseed to false, so throws the same error
45+        self.nodes[0].stop_node()
46+        self.nodes[0].assert_start_raises_init_error(
47+            expected_msg="Error: Cannot set -forcednsseed to true when setting -dnsseed to false.",
48+            extra_args=["-forcednsseed=1", "-connect=fakenodeaddr"],

practicalswift commented at 6:37 pm on July 29, 2021:

Same here? :)

amitiuttarwar commented at 6:24 pm on July 30, 2021:

done

mzumsande commented at 6:40 pm on July 29, 2021: member

Code Review ACK a8834fa0513d6bfcd4dfc759bbb841c02fcf8517

in test/functional/p2p_dns_seeds.py:59 in a8834fa051 outdated

52+        self.restart_node(0)
53+
54+    def existing_outbound_connections_test(self):
55+        # Make sure addrman is populated to enter the conditional where we
56+        # delay and potentially skip DNS seeding.
57+        self.nodes[0].addpeeraddress("192.0.0.8", 8333)

practicalswift commented at 6:45 pm on July 29, 2021:

I think the recommendation is to use addresses in the block 192.0.2.0/24 (TEST-NET-1) for testing purposes (RFC 5735).

I think the block 192.0.0.0/24 is reserved for future IETF protocol assignments.

amitiuttarwar commented at 6:07 pm on July 30, 2021:

hmmm, I tried this out but unfortunately since the CNetAddr code specifically recognizes these as invalid addresses (link), it prevents being able to test the functionality.

practicalswift commented at 9:44 pm on July 30, 2021:

Got it! Then 192.0.0.8 (the IPv4 dummy address per RFC 7600) is probably a good alternative to 192.0.2.0/24.

I checked the IANA IPv4 Special-Purpose Address Registry and 192.0.0.8 (like 192.0.2.0/24) is guaranteed to not be in use as valid destination address,.

amitiuttarwar commented at 11:21 pm on July 31, 2021:

awesome, thanks!

in test/functional/p2p_dns_seeds.py:73 in a8834fa051 outdated

66+    def existing_block_relay_connections_test(self):
67+        # Make sure addrman is populated to enter the conditional where we
68+        # delay and potentially skip DNS seeding. No-op when run after
69+        # existing_outbound_connections_test.
70+        self.nodes[0].addpeeraddress("192.0.0.8", 8333)
71+

practicalswift commented at 6:45 pm on July 29, 2021:

Same here :)

amitiuttarwar commented at 6:25 pm on July 30, 2021:

responded #22098 (review). let’s continue the convo there

in test/functional/p2p_dns_seeds.py:29 in a8834fa051 outdated

24+        self.wait_time_tests()
25+
26+    def init_arg_tests(self):
27+        self.log.info("Check that setting -connect disables -dnsseed by default")
28+        self.nodes[0].stop_node()
29+        with(self.nodes[0].assert_debug_log(expected_msgs=["DNS seeding disabled"])):

practicalswift commented at 6:55 pm on July 29, 2021:

Non-blocking style nit: I think with …: is favoured overwith(…): in the rest of the code base.

amitiuttarwar commented at 6:28 pm on July 30, 2021:

updated all with(...): to be with ...:

in src/chainparams.cpp:439 in a8834fa051 outdated

434@@ -435,7 +435,8 @@ class CRegTestParams : public CChainParams {
435         assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));
436 
437         vFixedSeeds.clear(); //!< Regtest mode doesn't have any fixed seeds.
438-        vSeeds.clear();      //!< Regtest mode doesn't have any DNS seeds.
439+        vSeeds.clear();
440+        vSeeds.emplace_back("dummySeed.invalid");

practicalswift commented at 7:08 pm on July 29, 2021:

Could use a FQDN such as say dummy-seed.invalid. (note the ending dot) be used here to make it unambiguous and thus make sure we don’t risk causing a DNS lookup for dummySeed.invalid.<local domain name as specified in /etc/resolv.conf>)?

amitiuttarwar commented at 6:28 pm on July 30, 2021:

good call, updated to dummySeed.invalid.

practicalswift changes_requested

practicalswift commented at 7:14 pm on July 29, 2021: contributor

Concept ACK

Thanks for improving testing!

I left some review comments on small changes that could further reduce the risk of accidental external communication :)

mzumsande commented at 11:55 am on July 30, 2021: member

This probably needs a rebase / exception added for the new test after #22490 was merged.

[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 <mzumsande@gmail.com>

9c08719778

[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 <mzumsande@gmail.com>

75c05af361

[test] Test the interactions between -connect and -dnsseed 35851450a9

[test] Test -forcednsseed causes querying DNS seeds 26d0ffe4f2

[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.

6f6b7df6bd

[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.

6395c8ed56

[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.

4c89e24f64

[style] Small style improvements to DNS parameters 82b6f89819

amitiuttarwar force-pushed on Jul 30, 2021

amitiuttarwar commented at 6:36 pm on July 30, 2021: contributor

thanks for the reviews @kristapsk & @practicalswift! & for the re-reviews @jnewbery & @mzumsande ☺️ @practicalswift I took all your suggestions to further reduce changes of external communication except for being able to apply the IP addresses reserved for testing. Thanks again for these! I learned about how FQDNs work. @mzumsande as I mentioned over here #22490 (comment), I surprisingly don’t need to rebase or add an exception!

I realized that test doesn’t actually need rebase or an exception added, because passing in something like self.start_node(0, ["-connect=fakenodeaddr"]) from the individual test appends the -connect arg as a command-line argument, which overrules what’s been set in the .conf file.

I locally rebased & confirmed that the tests still pass, but opted not to push the rebase here to make it easier to re-review. But lmk if you think it would be better to rebase & set self.disable_autoconnect and I’m happy to do so.

practicalswift commented at 10:56 pm on July 30, 2021: contributor

I took all your suggestions to further reduce changes of external communication except for being able to apply the IP addresses reserved for testing. Thanks again for these! I learned about how FQDNs work.

Excellent!

It is outside of the scope of this PR but I think it would make sense to err on the safe side and specify all seed domain names with an ending dot to make them unambiguous.

In other words …

0    vSeeds.emplace_back("seed.bitcoin.sipa.be."); // Pieter Wuille, only supports x1, x5, x9, and xd

… instead of the current …

0    vSeeds.emplace_back("seed.bitcoin.sipa.be"); // Pieter Wuille, only supports x1, x5, x9, and xd

The former is guaranteed to be interpreted as seed.bitcoin.sipa.be. whereas the latter may be interpreted as sayseed.bitcoin.sipa.be.megacorp.com. depending on the content of the user’s /etc/resolv.conf.

The case I’m thinking about is if search megacorp.com is specified in /etc/resolv.conf and say options ndots:5 is used.

Looking up seed.bitcoin.sipa.be would then result in the following DNS traffic:

0IP <src> > <resolver>.53: A? seed.bitcoin.sipa.be.megacorp.com.
1IP <resolver>.53 > <src>: NXDomain
2IP <src> > <resolver>.53: A? seed.bitcoin.sipa.be.
3IP <resolver>.53 > <src>: Some valid response.

In other words seed.bitcoin.sipa.be.megacorp.com. would be tried before seed.bitcoin.sipa.be. :(

I haven’t tried it myself but if I’m thinking correctly here then adding search localtest.me and options ndots:15 to /etc/resolv.conf should break DNS seeding since the DNS server for localtest.me will return 127.0.0.1 for *.localtest.me.

I guess an alternative way to fix this could be to add the ending dot (if missing) when building the host string:

 0diff --git a/src/net.cpp b/src/net.cpp
 1index 073643fb7..c68c497ea 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -1714,7 +1714,7 @@ void CConnman::ThreadDNSAddressSeed()
 5             std::vector<CNetAddr> vIPs;
 6             std::vector<CAddress> vAdd;
 7             ServiceFlags requiredServiceBits = GetDesirableServiceFlags(NODE_NONE);
 8-            std::string host = strprintf("x%x.%s", requiredServiceBits, seed);
 9+            std::string host = strprintf("x%x.%s.", requiredServiceBits, seed);
10             CNetAddr resolveSource;
11             if (!resolveSource.SetInternal(host)) {
12                 continue;

Some background context from https://bugzilla.mozilla.org/show_bug.cgi?id=134402:

 0The BSD Unix implementation of the name resolver established numerous
 1conventions that have become defacto standards for Unix and Linux, but are
 2not formally Internet standards, and are not commonly found on other 
 3platforms, such as Windows.  These include such things as:
 4
 5…
 6
 7b) a trailing dot tells BSD's DNS name resolver not to try appending domain 
 8name suffixes.  E.g. for a client in the mozilla.com domain, the DNS name 
 9foo.bar can be interpreted as foo.bar.mozilla.com as well as foo.bar, but 
10foo.bar. is only interpreted as foo.bar and not as foo.bar.mozilla.com.  
11These are only conventions of some OSes, and not Internet standards.

amitiuttarwar commented at 11:26 pm on July 31, 2021: contributor

all review comments have been addressed.

specify all seed domain names with an ending dot to make them unambiguous.. @practicalswift, very interesting! thanks for digging in & sharing your findings! I’ll have to learn more myself but this seems like a good idea. Shall we open an issue?

mzumsande commented at 7:47 pm on August 1, 2021: member

ACK 82b6f89819e55af26f5264678e0f93052934bcb3

I locally rebased & confirmed that the tests still pass, but opted not to push the rebase here to make it easier to re-review. But lmk if you think it would be better to rebase & set self.disable_autoconnect and I’m happy to do so.

not necessary, I didn’t realize that it would work, but your explanation makes sense.

practicalswift commented at 8:36 pm on August 1, 2021: contributor

The former is guaranteed to be interpreted as seed.bitcoin.sipa.be. whereas the latter may be interpreted as sayseed.bitcoin.sipa.be.megacorp.com. depending on the content of the user’s /etc/resolv.conf.

@practicalswift, very interesting! thanks for digging in & sharing your findings! I’ll have to learn more myself but this seems like a good idea. Shall we open an issue?

Feel free to open an issue if you feel for it: I’m afraid I won’t be able to find time to work on this any time soon, but I’d be glad to help out with review :)

An /etc/resolv.conf file with this content should be sufficient to reproduce the issue (if my theory holds: I haven’t had time to check beyond reading the source!):

0search localtest.me
1options ndots:15
2nameserver 8.8.8.8
3nameserver 8.8.4.4

8.8.8.8 and 8.8.4.4 are Google Public DNS, and localtest.me is a domain name setup with a DNS server that will respond with 127.0.0.1 for all queries including a lookup of say seed.bitcoin.sipa.be.localtest.me.

practicalswift commented at 6:38 am on August 2, 2021: contributor

The former is guaranteed to be interpreted as seed.bitcoin.sipa.be. whereas the latter may be interpreted as sayseed.bitcoin.sipa.be.megacorp.com. depending on the content of the user’s /etc/resolv.conf.

@practicalswift, very interesting! thanks for digging in & sharing your findings! I’ll have to learn more myself but this seems like a good idea. Shall we open an issue?

Feel free to open an issue if you feel for it: I’m afraid I won’t be able to find time to work on this any time soon, but I’d be glad to help out with review :)

An /etc/resolv.conf file with this content should be sufficient to reproduce the issue (if my theory holds: I haven’t had time to check beyond reading the source!):

A simpler way that should reproduce the issue without having to make any changes to /etc/resolv.conf:

0$ RES_OPTIONS="options ndots:15" LOCALDOMAIN="localtest.me" src/bitcoind

If my thinking is correct that should make the DNS seeding queries return 127.0.0.1 due to seed.bitcoin.sipa.be being interpreted as seed.bitcoin.sipa.be.localtest.me, etc.

jnewbery commented at 9:46 am on August 2, 2021: member

reACK 82b6f89819e55af26f5264678e0f93052934bcb3

fanquake commented at 3:20 am on August 3, 2021: member

The DNS querying logic and related config options certainly are somewhat convoluted / confusing. This is not helped by the fact that certain options (soft)-set other options, or sometimes behave un-intuitively. I think there is definitely scope to improve this code further going forward.

fanquake merged this on Aug 3, 2021

fanquake closed this on Aug 3, 2021

fanquake commented at 3:25 am on August 3, 2021: member

cc @ajtowns

sidhujag referenced this in commit ffcd9568b8 on Aug 4, 2021

DrahtBot locked this on Aug 18, 2022

[test, init] DNS seed querying logic #22098