We will discuss both PR 15558
(merged) and PR 16939 (open)
in this week’s PR review club. Both are small code changes, so should be quick
When bitcoind is started, it tries to connect to peers from its addrman
database. Those new connections are opened in
CConnman::ThreadOpenConnections(), which is called in CConnman::Start().
If the addrman database is empty (for example, the first time that bitcoind
is run), or if bitcoind is unable to successfully connect to two peers, then
it will query DNS seeds to get addresses of candidate peers to connect to.
Querying DNS seeds is done in a separate thread, threadDNSAddressSeed, which
is also started in CConman::Start().
For completeness, if the DNS seed queries are unsuccesful, then bitcoind
will fall back to connecting to a hard-coded list of seed nodes. This
fall back functionality is only run as a last resort if all DNS seeds are
down (eg if there is an attack on the DNS seed infrastructure).
In February, Issue 15434 was
opened, which reported that DNS seeds were being queried too soon and too
frequently. From that issue: “Despite being running most of the time, and
having a database of tens of thousands of peers, my node seems to query the DNS
seeds each time I restart it, which doesn’t seem ideal from a privacy
PR 15558 (which was merged
in time for V0.19) changed the ThreadOpenConnections() behaviour to only
query three DNS seeds at a time (rather than all at once). According to the
author: “This reduces the amount of information DNS seeds can observe about the
requesters by spreading the load over all of them.”.
After that PR was merged, AJ Towns left a
“I think there’s a bug here: if this loop is hit with addrman.size()==0 and
seeds_right_now==0 it will then query the first seed, and set
seeds_right_now==-1, at which point this condition will keep failing on future
loops, preventing a delay between batches of 3 seeds, and preventing early
PR 16939 changes the DNS
seed behaviour once again:
if there are 0 entries in addrman or -forcedns is set, it will query
the DNS seeds immediately;
if there are fewer than 1000 entries in addrman, it will query DNS seeds
after 11 seconds;
if there are more than 1000 entries in addrman, it will query DNS seeds
after 5 minutes;
if there are still no entries in addrman 6 minutes after start up, then
bitcoind will try to connect to the hard-coded seed nodes.
<jnewbery> one of the challenges for this PR is that we don't have any way to do integration testing of these changes - we can't manipulate the peers.dat file or set up DNS servers in our functional test framework
<fanquake> Now that there's been more some PR review coming from the review club, this sort of "how did you actually review PRs" discussion is definitely the most interesting to me. Especially from anyone that has been ACKing PRs.
<lightlike> aj: the 9th peer was the second one that worked. I think the issue is that the first peer I connected to sent me some new blocks (my node was still in final stages of IBD), and while they were being validates there were some ~30s breaks in which no new connections were attempted
<jnewbery> zenogais: one potential difficulty with using a debugger with this functionality is that there are a bunch of threads with timers interacting with each other. Stepping through one thread doesn't capture all the behaviour
<jnewbery> lightlike has already mentioned that not querying DNS seeds could potentially be bad for robustness/redundancy (because reducing the use of DNS seeds could *potentially* make us more vulnerable to having our addrman taken over)
<jnewbery> sprawl-unsolved: I think 3 was simply chosen to mitigate against a single rogue DNS seed being able to eclipse new nodes. Issue 16938 is a very interesting discussion, but it's not the only reason you'd want redundancy in the DNS seeds you query