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

pull ajtowns wants to merge 1 commits into bitcoin:master from ajtowns:202105-ignoreblockrelayfordnsskip changing 1 files +1 −1
  1. ajtowns commented at 5:34 am on May 21, 2021: member

    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.

  2. net: ignore block-relay-only peers when skipping DNS seed fe3d17df04
  3. ajtowns requested review from amitiuttarwar on May 21, 2021
  4. fanquake added the label P2P on May 21, 2021
  5. Sjors commented at 1:37 pm on May 21, 2021: member

    utACK fe3d17d

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

    https://github.com/bitcoin/bitcoin/blob/eb4df9a628bdcdd1333c7fa4fb23c73df9642902/src/net.h#L468-L485

  6. practicalswift commented at 7:57 pm on May 21, 2021: contributor

    Oh good catch.

    Concept ACK

  7. amitiuttarwar commented at 11:22 pm on May 21, 2021: contributor

    ACK fe3d17df04decc4e856121eb311636977d60f80f, 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.

  8. Sjors commented at 12:07 pm on May 22, 2021: member
    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.
  9. mzumsande commented at 2:43 pm on May 22, 2021: member

    ACK fe3d17df04decc4e856121eb311636977d60f80f

    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.

  10. ghost commented at 3:52 pm on May 22, 2021: none

    Concept ACK

    Change in https://github.com/bitcoin/bitcoin/commit/fe3d17df04decc4e856121eb311636977d60f80f makes sense however I was not able to reproduce the issue by compiling master branch. Not sure if I am testing it correctly.

    1. DNS seeding is skipped if I have 0 block relay connections in anchors.dat but few full relay connections in peers.dat
    02021-05-22T15:04:15Z New outbound peer connected: version: 70015, blocks=1976272, peer=0 (outbound-full-relay)
12021-05-22T15:04:19Z Synchronizing blockheaders, height: 19999 (~4.15%)
22021-05-22T15:04:21Z New outbound peer connected: version: 70015, blocks=1976272, peer=1 (outbound-full-relay)
32021-05-22T15:04:22Z Synchronizing blockheaders, height: 21999 (~4.58%)
42021-05-22T15:04:25Z P2P peers available. Skipped DNS seeding.
    1. Deleted everything from testnet3 directory, compiled master branch and run bitcoind with testnet=1
    2. 2 outbound block-relay-only peer addresses saved in anchors.dat
     02021-05-22T14:54:04Z init message: Starting network threads…
 12021-05-22T14:54:04Z init message: Done loading
 22021-05-22T14:54:04Z msghand thread start
 32021-05-22T14:54:04Z opencon thread start
 42021-05-22T14:54:04Z addcon thread start
 52021-05-22T14:54:04Z dnsseed thread start
 62021-05-22T14:54:04Z Loading addresses from DNS seed testnet-seed.bluematt.me
 72021-05-22T14:54:04Z net thread start
 82021-05-22T14:54:05Z Loading addresses from DNS seed testnet-seed.bitcoin.jonasschnelli.ch
 92021-05-22T14:54:06Z Loading addresses from DNS seed seed.testnet.bitcoin.sprovoost.nl
102021-05-22T14:54:08Z Loading addresses from DNS seed seed.tbtc.petertodd.org
112021-05-22T14:54:08Z 112 addresses found from DNS seeds
122021-05-22T14:54:08Z dnsseed thread exit
132021-05-22T14:54:27Z New outbound peer connected: version: 70015, blocks=1976271, peer=0 (outbound-full-relay)
14
15
16
172021-05-22T15:14:33Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
    1. Delete peers.dat run bitcoind again
     02021-05-22T15:19:35Z Loaded 2 addresses from "anchors.dat"
 12021-05-22T15:19:35Z 2 block-relay-only anchors will be tried for connections.
 22021-05-22T15:19:35Z init message: Starting network threads…
 32021-05-22T15:19:35Z net thread start
 42021-05-22T15:19:35Z dnsseed thread start
 52021-05-22T15:19:35Z Loading addresses from DNS seed seed.tbtc.petertodd.org
 62021-05-22T15:19:36Z addcon thread start
 72021-05-22T15:19:36Z opencon thread start
 82021-05-22T15:19:36Z init message: Done loading
 92021-05-22T15:19:36Z msghand thread start
102021-05-22T15:19:37Z Loading addresses from DNS seed testnet-seed.bluematt.me
112021-05-22T15:19:38Z Loading addresses from DNS seed seed.testnet.bitcoin.sprovoost.nl
122021-05-22T15:19:38Z Loading addresses from DNS seed testnet-seed.bitcoin.jonasschnelli.ch
132021-05-22T15:19:38Z New outbound peer connected: version: 70016, blocks=1976272, peer=0 (block-relay-only)
142021-05-22T15:19:38Z 112 addresses found from DNS seeds
152021-05-22T15:19:38Z dnsseed thread exit
162021-05-22T15:19:40Z Synchronizing blockheaders, height: 507998 (~62.33%)
172021-05-22T15:19:40Z New outbound peer connected: version: 70016, blocks=1976272, peer=1 (block-relay-only)
    1. Restart with 2 block-relay-only anchors and few outbound full relay in peers.dat
     02021-05-22T15:44:26Z 2 block-relay-only anchors will be tried for connections.
 12021-05-22T15:44:26Z init message: Starting network threads…
 22021-05-22T15:44:26Z init message: Done loading
 32021-05-22T15:44:26Z addcon thread start
 42021-05-22T15:44:26Z dnsseed thread start
 52021-05-22T15:44:26Z Waiting 300 seconds before querying DNS seeds.
 62021-05-22T15:44:26Z opencon thread start
 72021-05-22T15:44:26Z msghand thread start
 82021-05-22T15:44:26Z net thread start
 92021-05-22T15:44:29Z New outbound peer connected: version: 70016, blocks=1976274, peer=1 (block-relay-only)
102021-05-22T15:44:29Z New outbound peer connected: version: 70016, blocks=1976274, peer=0 (block-relay-only)
112021-05-22T15:44:29Z New outbound peer connected: version: 70015, blocks=1976274, peer=2 (outbound-full-relay)
122021-05-22T15:44:32Z Synchronizing blockheaders, height: 929997 (~79.16%)
132021-05-22T15:44:36Z Synchronizing blockheaders, height: 931997 (~79.19%)
142021-05-22T15:44:37Z P2P peers available. Skipped DNS seeding.
  11. mzumsande commented at 4:51 pm on May 22, 2021: member

    @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:

    0bitcoind --testnet -dnsseed=0 -fixedseeds=0
1bitcoin-cli --testnet addpeeraddress <INVALID ADDRESS> 18333
2bitcoin-cli --testnet stop
3bitcoind --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.

  12. amitiuttarwar commented at 6:25 pm on May 22, 2021: contributor

    @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!

  13. Sjors commented at 7:10 pm on May 22, 2021: member
    @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.
  14. amitiuttarwar commented at 7:24 pm on May 22, 2021: contributor
    @Sjors did you mean to tag @ajtowns to update OP? It’s not my PR 😛
  15. mzumsande commented at 7:58 pm on May 22, 2021: member
    @amitiuttarwar See https://github.com/mzumsande/bitcoin/commit/237d75472ba1f8b90c286519e48f58d07a50b431 for what I did to test this PR. But I have no idea if adding dummy seeds to regtest is viable or breaks anything…
  16. ghost commented at 4:15 pm on May 23, 2021: none

    Thanks @mzumsande I was able to reproduce the issue.

    1. Run bitcoind for few minutes and get 2 block only peers in anchors.dat
    02021-05-23T14:13:31Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
    1. Delete peers.dat
    2. bitcoind -dnsseed=0 -fixedseeds=0
    3. bitcoin-cli --testnet addpeeraddress <INVALID ADDRESS> 18333

    Things that I tried in INVALID ADDRESS:

     0prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "i11\/^£||>|pλ<>r€ss" 18333
 1{
 2  "success": false
 3}
 4prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "invalidip" 18333
 5{
 6  "success": false
 7}
 8prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "8.8.8.8.8.8.8" 18333
 9{
10  "success": false
11}
12prayank@ubuntu:~$ bitcoin-cli --testnet addpeeraddress "8.8.8.8" 18333
13{
14  "success": true
15}
    1. bitcoin-cli stop
    2. bitcoind (run for few minutes)
    3. bitcoin-cli getpeerfinfo: 2 block-relay-only connections
    4. bitcoin-cli stop

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

    02021-05-23T14:29:24Z New outbound peer connected: version: 70016, blocks=1976375, peer=1 (block-relay-only)
1
22021-05-23T14:29:25Z sending getdata (577 bytes) peer=1
32021-05-23T14:29:28Z connection attempt to 8.8.8.8:18333 timed out
42021-05-23T14:29:29Z trying connection 8.8.8.8:18333 lastseen=0.0hrs
52021-05-23T14:29:32Z received: block (745874 bytes) peer=1
62021-05-23T14:29:32Z P2P peers available. Skipped DNS seeding.
72021-05-23T14:29:32Z dnsseed thread exit

    Logs had lot of connections attempts for invalid peer:

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

    bitcoin.conf

    0testnet=1
1debug=net
  17. ghost commented at 5:18 pm on May 23, 2021: none

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

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

     02021-05-23T17:04:39Z DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat started
 12021-05-23T17:04:39Z DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
 22021-05-23T17:04:39Z disconnecting peer=0
 32021-05-23T17:04:39Z Cleared nodestate for peer=0
 42021-05-23T17:04:39Z disconnecting peer=1
 52021-05-23T17:04:39Z Cleared nodestate for peer=1
 62021-05-23T17:04:39Z disconnecting peer=2
 72021-05-23T17:04:39Z Cleared nodestate for peer=2
 82021-05-23T17:04:39Z disconnecting peer=3
 92021-05-23T17:04:39Z Cleared nodestate for peer=3
102021-05-23T17:04:39Z disconnecting peer=4
112021-05-23T17:04:39Z Cleared nodestate for peer=4
  18. jonatack commented at 12:20 pm on May 24, 2021: member
    ACK fe3d17df04decc4e856121eb311636977d60f80f
  19. unknown approved
  21. fanquake merged this on May 24, 2021
  22. fanquake closed this on May 24, 2021
  23. sidhujag referenced this in commit fd31a503bf on May 25, 2021
  24. amitiuttarwar referenced this in commit 3548b57152 on May 28, 2021
  25. amitiuttarwar commented at 10:13 pm on May 28, 2021: contributor

    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.

  26. amitiuttarwar referenced this in commit d02da04860 on May 28, 2021
  27. luke-jr referenced this in commit bc4ae53900 on Jun 27, 2021
  28. amitiuttarwar referenced this in commit f0ce80a47f on Jul 28, 2021
  29. amitiuttarwar referenced this in commit 166626a101 on Jul 28, 2021
  30. amitiuttarwar referenced this in commit 75c05af361 on Jul 30, 2021
  31. fanquake referenced this in commit 10fbb37268 on Aug 3, 2021
  32. sidhujag referenced this in commit ffcd9568b8 on Aug 4, 2021
  33. gwillen referenced this in commit 4ccd7f4e36 on Jun 1, 2022
  34. DrahtBot locked this on Aug 16, 2022


ajtowns Sjors practicalswift amitiuttarwar mzumsande ghost jonatack


amitiuttarwar

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me