p2p: gives seednode priority over dnsseed if both are provided #28016
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
I wanted to have an exit early strategy that will hand the logic back to |
src/net.cpp
Outdated
| // Abort if we have spent enough time without reaching our target. | ||
| // Giving seed nodes 30 seconds so this does not become a race against fixedseeds (which | ||
| // triggers after a minute) | ||
| if (GetTime<std::chrono::seconds>() > start + std::chrono::seconds{30}) { |
There was a problem hiding this comment.
nit: Could use NodeClock::now() for new code? Also 30s should work for the constant.
There was a problem hiding this comment.
Should be fixed in ac7ce24
|
If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds... |
Kindly add some rationale for that? As I see it, if the user is changing a default (adding a |
sr-gi
force-pushed
the
dnsseed-priority
branch
from
ac7ce24
to
91efd59
Compare
If the user has specified both, presumably he wants both to be used... |
And they are, just not at the same time. The same rationale applies to #27577, yet seednodes are prioritized over fixed seeds |
sr-gi
force-pushed
the
dnsseed-priority
branch
from
91efd59
to
0f1c629
Compare
sr-gi
force-pushed
the
dnsseed-priority
branch
from
0f1c629
to
5843cbd
Compare
|
Rebased to fix CI |
|
ACK 78482a0 I ran into this issue with the existing behavior when testing Testing with a fresh signet node: $ bitcoind -debug=net -signet -seednode=bitcoin.achow101.com:38333 -datadir=/tmp/28016/On master
$ bitcoind -debug=net -signet -seednode=bitcoin.achow101.com:38333 -datadir=/tmp/28016/2024-03-15T21:22:57Z init message: Starting network threads…
2024-03-15T21:22:57Z net thread start
2024-03-15T21:22:57Z addcon thread start
2024-03-15T21:22:57Z dnsseed thread start
2024-03-15T21:22:57Z Loading addresses from DNS seed seed.signet.bitcoin.sprovoost.nl.
2024-03-15T21:22:57Z opencon thread start
2024-03-15T21:22:57Z [net] trying v2 connection bitcoin.achow101.com:38333 lastseen=0.0hrs
2024-03-15T21:22:57Z init message: Done loading
2024-03-15T21:22:57Z msghand thread start
2024-03-15T21:22:57Z Loading addresses from DNS seed 178.128.221.177
2024-03-15T21:22:57Z Loading addresses from DNS seed v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion:38333
2024-03-15T21:22:57Z 34 addresses found from DNS seeds
2024-03-15T21:22:57Z dnsseed thread exit
2024-03-15T21:22:57Z [net] Added connection peer=0
2024-03-15T21:22:57Z [net] sending version (103 bytes) peer=0
2024-03-15T21:22:57Z [net] send version message: version 70016, blocks=0, txrelay=1, peer=0
2024-03-15T21:22:57Z [net] start sending v2 handshake to peer=0
2024-03-15T21:22:57Z [net] received: version (103 bytes) peer=0
2024-03-15T21:22:57Z [net] sending wtxidrelay (0 bytes) peer=0
2024-03-15T21:22:57Z [net] sending sendaddrv2 (0 bytes) peer=0
2024-03-15T21:22:57Z [net] sending verack (0 bytes) peer=0
2024-03-15T21:22:57Z [net] sending getaddr (0 bytes) peer=0
2024-03-15T21:22:57Z [net] receive version message: /Satoshi:27.99.0/: version 70016, blocks=186938, us=192.12.14.3:16482, txrelay=1, peer=0
2024-03-15T21:22:57Z [net] added time data, samples 2, offset +0 (+0 minutes)
2024-03-15T21:22:57Z [net] received: wtxidrelay (0 bytes) peer=0
2024-03-15T21:22:57Z [net] received: sendaddrv2 (0 bytes) peer=0
2024-03-15T21:22:57Z [net] received: verack (0 bytes) peer=0
2024-03-15T21:22:57Z New addr-fetch v2 peer connected: version: 70016, blocks=186938, peer=0
2024-03-15T21:22:57Z [net] sending sendcmpct (9 bytes) peer=0
2024-03-15T21:22:57Z [net] sending ping (8 bytes) peer=0
2024-03-15T21:22:57Z [net] sending feefilter (8 bytes) peer=0
2024-03-15T21:22:57Z [net] received: sendcmpct (9 bytes) peer=0
2024-03-15T21:22:57Z [net] received: ping (8 bytes) peer=0
2024-03-15T21:22:57Z [net] sending pong (8 bytes) peer=0
2024-03-15T21:22:57Z [net] received: getheaders (965 bytes) peer=0
2024-03-15T21:22:57Z [net] Ignoring getheaders from peer=0 because active chain has too little work; sending empty response
2024-03-15T21:22:57Z [net] sending headers (1 bytes) peer=0
2024-03-15T21:22:57Z [net] received: feefilter (8 bytes) peer=0
2024-03-15T21:22:57Z [net] received: feefilter of 0.00001000 BTC/kvB from peer=0
2024-03-15T21:22:57Z [net] received: addrv2 (19609 bytes) peer=0
2024-03-15T21:22:57Z [net] Received addr: 1000 addresses (1000 processed, 0 rate-limited) from peer=0
2024-03-15T21:22:57Z [net] addrfetch connection completed peer=0; disconnecting
2024-03-15T21:22:57Z [net] disconnecting peer=0
2024-03-15T21:22:57Z [net] Cleared nodestate for peer=0On this branch:
$ bitcoind -signet -seednode=bitcoin.achow101.com:38333 -datadir=/tmp/28016/2024-03-15T21:31:46Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
2024-03-15T21:31:46Z addcon thread start
2024-03-15T21:31:46Z opencon thread start
2024-03-15T21:31:46Z init message: Done loading
2024-03-15T21:31:46Z [net] trying v2 connection bitcoin.achow101.com:38333 lastseen=0.0hrs
2024-03-15T21:31:46Z msghand thread start
2024-03-15T21:31:46Z [net] Added connection peer=0
2024-03-15T21:31:46Z [net] sending version (103 bytes) peer=0
2024-03-15T21:31:46Z [net] send version message: version 70016, blocks=0, txrelay=1, peer=0
2024-03-15T21:31:46Z [net] start sending v2 handshake to peer=0
2024-03-15T21:31:46Z [net] received: version (103 bytes) peer=0
2024-03-15T21:31:46Z [net] sending wtxidrelay (0 bytes) peer=0
2024-03-15T21:31:46Z [net] sending sendaddrv2 (0 bytes) peer=0
2024-03-15T21:31:46Z [net] sending verack (0 bytes) peer=0
2024-03-15T21:31:46Z [net] sending getaddr (0 bytes) peer=0
2024-03-15T21:31:46Z [net] receive version message: /Satoshi:27.99.0/: version 70016, blocks=186940, us=192.12.14.3:44748, txrelay=1, peer=0
2024-03-15T21:31:46Z [net] added time data, samples 2, offset +0 (+0 minutes)
2024-03-15T21:31:46Z [net] received: wtxidrelay (0 bytes) peer=0
2024-03-15T21:31:46Z [net] received: sendaddrv2 (0 bytes) peer=0
2024-03-15T21:31:46Z [net] received: verack (0 bytes) peer=0
2024-03-15T21:31:46Z New addr-fetch v2 peer connected: version: 70016, blocks=186940, peer=0
2024-03-15T21:31:46Z [net] sending sendcmpct (9 bytes) peer=0
2024-03-15T21:31:46Z [net] sending ping (8 bytes) peer=0
2024-03-15T21:31:46Z [net] sending feefilter (8 bytes) peer=0
2024-03-15T21:31:46Z [net] received: sendcmpct (9 bytes) peer=0
2024-03-15T21:31:46Z [net] received: ping (8 bytes) peer=0
2024-03-15T21:31:46Z [net] sending pong (8 bytes) peer=0
2024-03-15T21:31:46Z [net] received: getheaders (965 bytes) peer=0
2024-03-15T21:31:46Z [net] Ignoring getheaders from peer=0 because active chain has too little work; sending empty response
2024-03-15T21:31:46Z [net] sending headers (1 bytes) peer=0
2024-03-15T21:31:46Z [net] received: feefilter (8 bytes) peer=0
2024-03-15T21:31:46Z [net] received: feefilter of 0.00001000 BTC/kvB from peer=0
2024-03-15T21:31:46Z [net] received: addrv2 (19594 bytes) peer=0
2024-03-15T21:31:46Z [net] Received addr: 999 addresses (999 processed, 0 rate-limited) from peer=0
2024-03-15T21:31:46Z [net] addrfetch connection completed peer=0; disconnecting
2024-03-15T21:31:46Z [net] disconnecting peer=0
2024-03-15T21:31:46Z [net] Cleared nodestate for peer=0
2024-03-15T21:31:47Z [net] trying v1 connection [2620:0:e00:400f::2c5]:38333 lastseen=196.1hrs
2024-03-15T21:31:47Z [net] connect() to [2620:0:e00:400f::2c5]:38333 failed: Network is unreachable (101)
2024-03-15T21:31:47Z [net] trying v1 connection [2a01:4f9:3a:2dd2::2]:38333 lastseen=15.0hrs
# [Later...]
2024-03-15T21:31:55Z P2P peers available. Finished fetching data from seed nodes.
2024-03-15T21:31:55Z Skipping DNS seeds. Enough peers have been found
2024-03-15T21:31:55Z dnsseed thread exit |
I'm not too familiar with the underlying logic for making the actual connection, but the higher level logic is designed around always having a max number of connections, that is: there is logic for filling the connections slots, and the logic to maintain, evict, and favor different kind of connections once the limit is reached. I don't think there is a real rush to fill the outbound connection slots, it happens rather quickly under normal conditions (with a populated addrman), so the case that it may take slightly longer on the first bootstrap shouldn't be an issue.
I don't think this would be a good idea. The collection of addresses that other peers feed you when requested is purposely "untested" (as in, not giving you information of whether they are good, or not giving you only good addresses is a feature). The reason for that is that if a seed only provides valid/tested addresses, you could tell what peers the node is connected to/has been connected to, which is undesirable |
|
Addresses review comments by @davidgumberg |
|
tested ACK 2842e51 Standard tests
Manual functional testRepeated a similar manual functional testing methodology to @davidgumberg and @cbergqvist on both testnet and signet, and again the expected behavior described in this PR is confirmed. For brevity I only include the testnet examples. Test case 1: Valid seed node
The seed node provided is used upfront and subsequent connections to peers follow suitTest case 2: Invalid seed node
Connection to the seed node is given 30 sec to connect but it fails as it is invalid. Upon the expiration of this delay control is handed over to DNS seeds |
|
I wonder if it wouldn't be simpler to just disable dns seeds in InitParameterInteraction if My main use case for In general, results from working dns seeds will be of better quality (in terms of percentage of peers you can actually connect to), because those specifically return peers that were reachable during the last minutes / hours, whereas I'm not sure about other people's use case though, so I wonder if there is a common use case for DNS seeds being backup to |
Yeah, me neither to be honest. My intuition is that if a non-default config is provided (and a default is not disabled), the user may want to give priority over the manual default, but still use the default as a last resort. Given dns seeds are usually faster feeding addresses than seednodes are (due to a dns query vs a p2p oneshot) I leaned towards the current approach. |
|
reACK 2842e51 I think the 30 second dns seed fallback condition here is more likely than I assumed because of the low quality of P2P gossip addresses mentioned by mzumsande above. I experienced it on my first test of this branch on signet and decided to investigate: In my testing, 6 out of 12 attempts to bootstrap a signet node with a It's a bit difficult for me to imagine a use case where someone would want to use Balancing the surprise of using |
|
re-ACK for 2842e51 The code adjustments made to bound scope and increase declaration/usage proximity look great to me (cleaner, more readable). Overall, I think the approach of this PR (prioritize seednodes before dnsseeds, then use dnsseeds if seednode usage was unsuccessful) is appropriate. Although subjective, overall I would consider node network reachability/health a bit more important than privacy. The parameter descriptions seem clear enough to enable a user to enhance privacy (e.g. disable dnsseed). Built and ran all functionals (all passed). Cleared datadir (with the exception of bitcoin.conf, chainstate, and blocks). Performed >1000 block download on signet to a reachable public node specified as a seednode. dnsseeds seemed to be avoided as expected. Cleared datadir again and performed test with a known invalid seednode specified (i.e. purposefully specified an invalid port node for the previous public node used). |
|
ACK 2842e51 Diffed top 2 commits in that and 78482a0 which I previously tested & acked. Only scope of |
|
Rebased to address merge conflicts |
|
untested reACK 82f41d7 |
There was a problem hiding this comment.
ACK 82f41d7
After checking out ran: git range-diff 2842e51~2..2842e51 HEAD~2..HEAD
Functional including --extended tests passed (skipped unrelated feature_dbcrash).
|
tested re-ACK 82f41d7 This PR not only adds meaningful functionality by enabling seeds that are provided by the user to take precedence over the default DNS seeds, but also enhances user experience with relevant reporting on what exactly happens through the process of connecting to the P2P network and which type of nodes are effectively used. Built this PR from a cleaned up environment using I also run all unit, functional, extended and manual tests exactly as I described them in my previous #28016 (comment) and all pass. |
|
ACK 82f41d7 |
This is a follow-up of #27577
If both
seednodeanddnsseedare provided, the node will start a race between them in order to fetch data to feed theaddrman.This PR gives priority to
seednodeoverdnsseedso if some nodes are provided as seeds, they can be tried before defaulting to thednsseeds