[test, init] DNS seed querying logic #22098

amitiuttarwar · 2021-05-28T22:10:49Z

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

lsilva01

Tested ACK 75ee018 on Ubuntu 20.04.

willcl-ark

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 ea0ad08 is exercised by the new test test/functional/p2p_dns_seeds.py

laanwj · 2021-06-02T16:55:18Z

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

Code review ACK 75ee018

mzumsande

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

test/functional/p2p_dns_seeds.py

jnewbery

ACK 75ee018

Thanks for adding these tests!

test/functional/p2p_dns_seeds.py

src/net.h

amitiuttarwar · 2021-07-28T21:59:26Z

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 · 2021-07-29T14:17:57Z

Code review ACK a8834fa

kristapsk

ACK a8834fa0513d6bfcd4dfc759bbb841c02fcf8517

mzumsande · 2021-07-29T18:40:35Z

Code Review ACK a8834fa0513d6bfcd4dfc759bbb841c02fcf8517

practicalswift

Concept ACK

Thanks for improving testing!

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

test/functional/p2p_dns_seeds.py

src/chainparams.cpp

mzumsande · 2021-07-30T11:55:23Z

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

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 <[email protected]>

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 bitcoin#22013. Co-authored-by: Martin Zumsande <[email protected]>

…nd -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.

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.

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.

amitiuttarwar · 2021-07-30T18:36:38Z

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 · 2021-07-30T22:56:45Z

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 …

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

… instead of the current …

    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:

IP <src> > <resolver>.53: A? seed.bitcoin.sipa.be.megacorp.com.
IP <resolver>.53 > <src>: NXDomain
IP <src> > <resolver>.53: A? seed.bitcoin.sipa.be.
IP <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:

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

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

The BSD Unix implementation of the name resolver established numerous
conventions that have become defacto standards for Unix and Linux, but are
not formally Internet standards, and are not commonly found on other 
platforms, such as Windows.  These include such things as:

…

b) a trailing dot tells BSD's DNS name resolver not to try appending domain 
name suffixes.  E.g. for a client in the mozilla.com domain, the DNS name 
foo.bar can be interpreted as foo.bar.mozilla.com as well as foo.bar, but 
foo.bar. is only interpreted as foo.bar and not as foo.bar.mozilla.com.  
These are only conventions of some OSes, and not Internet standards.

amitiuttarwar · 2021-07-31T23:26:16Z

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 · 2021-08-01T19:47:28Z

ACK 82b6f89

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 · 2021-08-01T20:36:07Z

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!):

search localtest.me
options ndots:15
nameserver 8.8.8.8
nameserver 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 · 2021-08-02T06:38:55Z

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:

$ 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 · 2021-08-02T09:46:19Z

reACK 82b6f89

fanquake · 2021-08-03T03:20:58Z

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 · 2021-08-03T03:25:43Z

cc @ajtowns

82b6f89 [style] Small style improvements to DNS parameters (Amiti Uttarwar) 4c89e24 [test] Test the delay before querying DNS seeds (Amiti Uttarwar) 6395c8e [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar) 6f6b7df [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar) 26d0ffe [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar) 3585145 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar) 75c05af [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar) 9c08719 [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 bitcoin#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 82b6f89 jnewbery: reACK 82b6f89 Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d

amitiuttarwar mentioned this pull request May 28, 2021

net: ignore block-relay-only peers when skipping DNS seed #22013

Merged

DrahtBot added P2P Validation labels May 28, 2021

amitiuttarwar force-pushed the 2021-05-dns-tests branch from 5664a2f to 75ee018 Compare May 28, 2021 22:40

amitiuttarwar added Tests and removed Validation labels May 28, 2021

lsilva01 approved these changes May 29, 2021

View reviewed changes

willcl-ark approved these changes Jun 2, 2021

View reviewed changes

mzumsande reviewed Jun 2, 2021

View reviewed changes

test/functional/p2p_dns_seeds.py Show resolved Hide resolved

test/functional/p2p_dns_seeds.py Outdated Show resolved Hide resolved

test/functional/p2p_dns_seeds.py Outdated Show resolved Hide resolved

jnewbery reviewed Jun 3, 2021

View reviewed changes

test/functional/p2p_dns_seeds.py Outdated Show resolved Hide resolved

test/functional/p2p_dns_seeds.py Outdated Show resolved Hide resolved

test/functional/p2p_dns_seeds.py Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

amitiuttarwar force-pushed the 2021-05-dns-tests branch 2 times, most recently from 6108630 to a8834fa Compare July 28, 2021 21:50

kristapsk approved these changes Jul 29, 2021

View reviewed changes

practicalswift suggested changes Jul 29, 2021

View reviewed changes

amitiuttarwar mentioned this pull request Jul 30, 2021

test: Disable automatic connections per default in the functional tests #22490

Merged

amitiuttarwar and others added 8 commits July 30, 2021 11:15

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

3585145

[test] Test -forcednsseed causes querying DNS seeds

26d0ffe

[test] Test the interactions between -forcednsseed and -dnsseed

6395c8e

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] Test the delay before querying DNS seeds

4c89e24

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.

[style] Small style improvements to DNS parameters

82b6f89

amitiuttarwar force-pushed the 2021-05-dns-tests branch from a8834fa to 82b6f89 Compare July 30, 2021 18:23

fanquake merged commit 10fbb37 into bitcoin:master Aug 3, 2021

practicalswift mentioned this pull request Oct 5, 2021

DNS seed node lookup is ambiguous in some (rare) scenarios #23193

Closed

mzumsande mentioned this pull request Aug 1, 2022

feature_config_args.py failure #25767

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

[test, init] DNS seed querying logic #22098

[test, init] DNS seed querying logic #22098

Uh oh!

Conversation

amitiuttarwar commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 2, 2021

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amitiuttarwar commented Jul 28, 2021

Uh oh!

jnewbery commented Jul 29, 2021

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Jul 29, 2021

Uh oh!

practicalswift left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mzumsande commented Jul 30, 2021

Uh oh!

amitiuttarwar commented Jul 30, 2021

Uh oh!

practicalswift commented Jul 30, 2021

Uh oh!

amitiuttarwar commented Jul 31, 2021

Uh oh!

mzumsande commented Aug 1, 2021

Uh oh!

practicalswift commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Aug 2, 2021

Uh oh!

jnewbery commented Aug 2, 2021

Uh oh!

fanquake commented Aug 3, 2021

Uh oh!

fanquake commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

amitiuttarwar commented May 28, 2021 •

edited

Loading

practicalswift commented Aug 1, 2021 •

edited

Loading