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

ajtowns · 2021-05-21T05:34:47Z

Since #17428 bitcoind will attempt to reconnect to two block-relay-only anchors before doing any other outbound connections. When determining whether to use DNS seeds, it will currently see these two peers and decide "we're connected to the p2p network, so no need to lookup DNS" -- but block-relay-only peers don't do address relay, so if your address book is full of invalid addresses (apart from your anchors) this behaviour will prevent you from recovering from that situation.

This patch changes it so that it only skips use of DNS seeds when there are two full-outbound peers, not just block-relay-only peers.

Sjors · 2021-05-21T13:37:58Z

utACK fe3d17d

IsFullOutboundConn() indeed seems the right replacement for IsOutboundOrBlockRelayConn(), given their respective definitions:

bitcoin/src/net.h

Lines 468 to 485 in eb4df9a

    
           bool IsOutboundOrBlockRelayConn() const { 
        
               switch (m_conn_type) { 
        
                   case ConnectionType::OUTBOUND_FULL_RELAY: 
        
                   case ConnectionType::BLOCK_RELAY: 
        
                       return true; 
        
                   case ConnectionType::INBOUND: 
        
                   case ConnectionType::MANUAL: 
        
                   case ConnectionType::ADDR_FETCH: 
        
                   case ConnectionType::FEELER: 
        
                       return false; 
        
               } // no default case, so the compiler can warn about missing cases 
        
               assert(false); 
        
           } 
        
           bool IsFullOutboundConn() const { 
        
               return m_conn_type == ConnectionType::OUTBOUND_FULL_RELAY; 
        
           }

practicalswift · 2021-05-21T19:57:55Z

Oh good catch.

Concept ACK

amitiuttarwar · 2021-05-21T23:22:46Z

ACK fe3d17d, this impacts the very common case where we stop/start a node, persisting anchors & have a non-empty addrman (although, to be clear, wouldn't be particularly problematic in the common cases where the addrman has valid addresses)

I tried to figure out if I could write a functional test to cover this code path, but seems like a no go- I can add outbound-block-relay only connections & then shut down the node to persist them to anchors, but they can't reconnect before hitting ThreadDNSAddressSeed on startup unless we do something more invasive to the test framework, which doesn't seem worth it.

Sjors · 2021-05-22T12:07:07Z

This is not the first bug in p2p bootstrap behaviour (#19795, #15434), so in due time having a functional test for this would be nice.

mzumsande · 2021-05-22T14:43:10Z

ACK fe3d17d

I tried to figure out if I could write a functional test to cover this code path, but seems like a no go- I can add outbound-block-relay only connections & then shut down the node to persist them to anchors, but they can't reconnect before hitting ThreadDNSAddressSeed on startup unless we do something more invasive to the test framework, which doesn't seem worth it.

I think the main issue is that this code is inside of a loop over the DNS seeds, and regtest doesn't have any. If there was a unreachable dummy dns seed for regtest, this could be tested:
ThreadDNSAddressSeed sleeps for 11s/5min depending on addrman size before querying, during this time one could add block-relay-only or full outbound peers and test that the behavior is different e.g. by looking at log messages.

ghost · 2021-05-22T15:52:39Z

Concept ACK

Change in fe3d17d makes sense however I was not able to reproduce the issue by compiling master branch. Not sure if I am testing it correctly.

DNS seeding is skipped if I have 0 block relay connections in anchors.dat but few full relay connections in peers.dat

2021-05-22T15:04:15Z New outbound peer connected: version: 70015, blocks=1976272, peer=0 (outbound-full-relay)
2021-05-22T15:04:19Z Synchronizing blockheaders, height: 19999 (~4.15%)
2021-05-22T15:04:21Z New outbound peer connected: version: 70015, blocks=1976272, peer=1 (outbound-full-relay)
2021-05-22T15:04:22Z Synchronizing blockheaders, height: 21999 (~4.58%)
2021-05-22T15:04:25Z P2P peers available. Skipped DNS seeding.

Deleted everything from testnet3 directory, compiled master branch and run bitcoind with testnet=1
2 outbound block-relay-only peer addresses saved in anchors.dat

2021-05-22T14:54:04Z init message: Starting network threads…
2021-05-22T14:54:04Z init message: Done loading
2021-05-22T14:54:04Z msghand thread start
2021-05-22T14:54:04Z opencon thread start
2021-05-22T14:54:04Z addcon thread start
2021-05-22T14:54:04Z dnsseed thread start
2021-05-22T14:54:04Z Loading addresses from DNS seed testnet-seed.bluematt.me
2021-05-22T14:54:04Z net thread start
2021-05-22T14:54:05Z Loading addresses from DNS seed testnet-seed.bitcoin.jonasschnelli.ch
2021-05-22T14:54:06Z Loading addresses from DNS seed seed.testnet.bitcoin.sprovoost.nl
2021-05-22T14:54:08Z Loading addresses from DNS seed seed.tbtc.petertodd.org
2021-05-22T14:54:08Z 112 addresses found from DNS seeds
2021-05-22T14:54:08Z dnsseed thread exit
2021-05-22T14:54:27Z New outbound peer connected: version: 70015, blocks=1976271, peer=0 (outbound-full-relay)



2021-05-22T15:14:33Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)

Delete peers.dat run bitcoind again

2021-05-22T15:19:35Z Loaded 2 addresses from "anchors.dat"
2021-05-22T15:19:35Z 2 block-relay-only anchors will be tried for connections.
2021-05-22T15:19:35Z init message: Starting network threads…
2021-05-22T15:19:35Z net thread start
2021-05-22T15:19:35Z dnsseed thread start
2021-05-22T15:19:35Z Loading addresses from DNS seed seed.tbtc.petertodd.org
2021-05-22T15:19:36Z addcon thread start
2021-05-22T15:19:36Z opencon thread start
2021-05-22T15:19:36Z init message: Done loading
2021-05-22T15:19:36Z msghand thread start
2021-05-22T15:19:37Z Loading addresses from DNS seed testnet-seed.bluematt.me
2021-05-22T15:19:38Z Loading addresses from DNS seed seed.testnet.bitcoin.sprovoost.nl
2021-05-22T15:19:38Z Loading addresses from DNS seed testnet-seed.bitcoin.jonasschnelli.ch
2021-05-22T15:19:38Z New outbound peer connected: version: 70016, blocks=1976272, peer=0 (block-relay-only)
2021-05-22T15:19:38Z 112 addresses found from DNS seeds
2021-05-22T15:19:38Z dnsseed thread exit
2021-05-22T15:19:40Z Synchronizing blockheaders, height: 507998 (~62.33%)
2021-05-22T15:19:40Z New outbound peer connected: version: 70016, blocks=1976272, peer=1 (block-relay-only)

Restart with 2 block-relay-only anchors and few outbound full relay in peers.dat

2021-05-22T15:44:26Z 2 block-relay-only anchors will be tried for connections.
2021-05-22T15:44:26Z init message: Starting network threads…
2021-05-22T15:44:26Z init message: Done loading
2021-05-22T15:44:26Z addcon thread start
2021-05-22T15:44:26Z dnsseed thread start
2021-05-22T15:44:26Z Waiting 300 seconds before querying DNS seeds.
2021-05-22T15:44:26Z opencon thread start
2021-05-22T15:44:26Z msghand thread start
2021-05-22T15:44:26Z net thread start
2021-05-22T15:44:29Z New outbound peer connected: version: 70016, blocks=1976274, peer=1 (block-relay-only)
2021-05-22T15:44:29Z New outbound peer connected: version: 70016, blocks=1976274, peer=0 (block-relay-only)
2021-05-22T15:44:29Z New outbound peer connected: version: 70015, blocks=1976274, peer=2 (outbound-full-relay)
2021-05-22T15:44:32Z Synchronizing blockheaders, height: 929997 (~79.16%)
2021-05-22T15:44:36Z Synchronizing blockheaders, height: 931997 (~79.19%)
2021-05-22T15:44:37Z P2P peers available. Skipped DNS seeding.

mzumsande · 2021-05-22T16:51:49Z

@prayank23 In order to hit this on testnet, you need to have a non-empty addrman, but with no valid addresses - the following should work if you start with empty peers.dat and two good anchors:

bitcoind --testnet -dnsseed=0 -fixedseeds=0
bitcoin-cli --testnet addpeeraddress <INVALID ADDRESS> 18333
bitcoin-cli --testnet stop
bitcoind --testnet

In the second run, you'll have two block-only peers but will never query a DNS seeds on master - whereas on this branch you will.

amitiuttarwar · 2021-05-22T18:25:50Z

@mzumsande

I think the main issue is that this code is inside of a loop over the DNS seeds, and regtest doesn't have any. If there was a unreachable dummy dns seed for regtest, this could be tested:
ThreadDNSAddressSeed sleeps for 11s/5min depending on addrman size before querying, during this time one could add block-relay-only or full outbound peers and test that the behavior is different e.g. by looking at log messages.

yup, this was the approach I took- added dummy seeds to regtest chain params & from the test made sure addrman has addresses & was asserting against log messages. the step I haven't been able to figure out is adding the connections during the sleep, with the test framework timing playing nicely with bitcoind timing. the branch is a bit of a mess, but lmk if you'd like me to clean it up and share- very possible I'm missing a viable solution!

Sjors · 2021-05-22T19:10:10Z

@mzumsande these manual test instructions are quite useful; I might test it next week.

@amitiuttarwar maybe mention those in the PR description, so it's a bit easier to find with git blame later. I often use the "blame" feature in Github to figure out when a line was last changed, which links to the commit, which in turn links to the pull request.

amitiuttarwar · 2021-05-22T19:24:01Z

@Sjors did you mean to tag @ajtowns to update OP? It's not my PR 😛

mzumsande · 2021-05-22T19:58:22Z

@amitiuttarwar See mzumsande@237d754 for what I did to test this PR. But I have no idea if adding dummy seeds to regtest is viable or breaks anything...

ghost · 2021-05-23T16:15:25Z

Thanks @mzumsande I was able to reproduce the issue.

Run bitcoind for few minutes and get 2 block only peers in anchors.dat

2021-05-23T14:13:31Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)

Delete peers.dat
bitcoind -dnsseed=0 -fixedseeds=0
bitcoin-cli --testnet addpeeraddress <INVALID ADDRESS> 18333

Things that I tried in INVALID ADDRESS:

prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "i11\/^£||>|pλ<>r€ss" 18333
{
  "success": false
}
prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "invalidip" 18333
{
  "success": false
}
prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "8.8.8.8.8.8.8" 18333
{
  "success": false
}
prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "8.8.8.8" 18333
{
  "success": true
}

bitcoin-cli stop
bitcoind (run for few minutes)
bitcoin-cli getpeerfinfo: 2 block-relay-only connections
bitcoin-cli stop

debug.log (DNS seeding was skipped because of peer=1 which is a block-relay-only connection)

2021-05-23T14:29:24Z New outbound peer connected: version: 70016, blocks=1976375, peer=1 (block-relay-only)

2021-05-23T14:29:25Z sending getdata (577 bytes) peer=1
2021-05-23T14:29:28Z connection attempt to 8.8.8.8:18333 timed out
2021-05-23T14:29:29Z trying connection 8.8.8.8:18333 lastseen=0.0hrs
2021-05-23T14:29:32Z received: block (745874 bytes) peer=1
2021-05-23T14:29:32Z P2P peers available. Skipped DNS seeding.
2021-05-23T14:29:32Z dnsseed thread exit

Logs had lot of connections attempts for invalid peer:

2021-05-23T14:37:05Z connection attempt to 8.8.8.8:18333 timed out
2021-05-23T14:37:06Z trying connection 8.8.8.8:18333 lastseen=0.1hrs

bitcoin.conf

testnet=1
debug=net

ghost · 2021-05-23T17:18:11Z

I retarted bitcoind few times to see if something changes. It recovered, used DNS seed and had outbound full relay connections.

2021-05-23T17:03:38Z connection attempt to 8.8.8.8:18333 timed out
2021-05-23T17:03:39Z trying connection 8.8.8.8:18333 lastseen=1.0hrs
2021-05-23T17:03:41Z Loading addresses from DNS seed testnet-seed.bluematt.me

2021-05-23T17:04:39Z DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat started
2021-05-23T17:04:39Z DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
2021-05-23T17:04:39Z disconnecting peer=0
2021-05-23T17:04:39Z Cleared nodestate for peer=0
2021-05-23T17:04:39Z disconnecting peer=1
2021-05-23T17:04:39Z Cleared nodestate for peer=1
2021-05-23T17:04:39Z disconnecting peer=2
2021-05-23T17:04:39Z Cleared nodestate for peer=2
2021-05-23T17:04:39Z disconnecting peer=3
2021-05-23T17:04:39Z Cleared nodestate for peer=3
2021-05-23T17:04:39Z disconnecting peer=4
2021-05-23T17:04:39Z Cleared nodestate for peer=4

jonatack

ACK fe3d17d

ghost

tACK fe3d17d

… DNS seed

amitiuttarwar · 2021-05-28T22:13:15Z

opened #22098 based on the convo here. huge props @mzumsande for figuring out the technique for timing around DNS logic with block-relay-only vs full-outbound connections 👏

the second commit on that PR 3548b571521b3f0693bbba39677344a626a406c5 fails prior to these changes.

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]>

Github-Pull: bitcoin#22013 Rebased-From: fe3d17d

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]>

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 #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

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

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

fe3d17d

ajtowns requested a review from amitiuttarwar May 21, 2021 05:34

fanquake added the P2P label May 21, 2021

jonatack reviewed May 24, 2021

View reviewed changes

ghost approved these changes May 24, 2021

View reviewed changes

fanquake merged commit 2968417 into bitcoin:master May 24, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 25, 2021

Merge bitcoin#22013: net: ignore block-relay-only peers when skipping…

fd31a50

… DNS seed

amitiuttarwar mentioned this pull request May 28, 2021

[test, init] DNS seed querying logic #22098

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

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

bc4ae53

Github-Pull: bitcoin#22013 Rebased-From: fe3d17d

JaredTate mentioned this pull request Oct 30, 2021

WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code DigiByte-Core/digibyte#77

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 2968417 into merged_master (Bitcoin PR bitcoin/bitcoin#22013)

4ccd7f4

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

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

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

Uh oh!

Conversation

ajtowns commented May 21, 2021

Uh oh!

Sjors commented May 21, 2021

Uh oh!

practicalswift commented May 21, 2021

Uh oh!

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

Uh oh!

Uh oh!

Sjors commented May 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

ghost commented May 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

amitiuttarwar commented May 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

amitiuttarwar commented May 22, 2021

Uh oh!

mzumsande commented May 22, 2021

Uh oh!

ghost commented May 23, 2021

Uh oh!

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

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar commented May 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

amitiuttarwar commented May 21, 2021 •

edited

Loading

mzumsande commented May 22, 2021 •

edited

Loading

mzumsande commented May 22, 2021 •

edited

Loading

Sjors commented May 22, 2021 •

edited

Loading

ghost commented May 23, 2021 •

edited by ghost

Loading