Add notes and questions for n16939

_posts/2019-10-09-#16939.md

@@ -1,18 +1,76 @@
 ---
 layout: pr
 title: "#16939 Delay querying DNS seeds if addrman is populated"
-components: [p2p]
 pr: 16939
-host: tba
+components: [p2p]
+authors: [ajtowns]
+host: jnewbery
 ---
 
-## Notes
+We will discuss both [PR 15558](https://github.com/bitcoin/bitcoin/pull/15558)
+(merged) and [PR 16939](https://github.com/bitcoin/bitcoin/pull/16939) (open)
+in this week's PR review club. Both are small code changes, so should be quick
+to review.
 
-This PR by ajtowns (Anthony Towns) is a proposed bug fix for merged [PR
-#15558](https://github.com/bitcoin/bitcoin/pull/15558) by sipa (Pieter
-Wuille). Is there indeed a bug to fix?
+## Notes
 
-Review will involve looking at both PRs, but they are both small in terms of
-LOC.
+- 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](https://github.com/bitcoin/bitcoin/issues/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
+  perspective"_.
+- [PR 15558](https://github.com/bitcoin/bitcoin/pull/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
+  comment](https://github.com/bitcoin/bitcoin/pull/15558#discussion_r327421987):
+  _"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
+  exit."_.
+- [PR 16939](https://github.com/bitcoin/bitcoin/pull/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.
 
 ## Questions
+
+1. Did you review the PR? [Concept ACK, approach ACK, tested ACK, or
+NACK?](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review)
+(Don't forget to put your PR review on GitHub.)
+
+2. What steps did you take, beyond reading the code?
+
+3. How would you test this manually? Could any automated tests be added?
+
+4. Do you agree with [AJ's
+   comment](https://github.com/bitcoin/bitcoin/pull/15558#discussion_r327421987)
+   that this is a bug?
+
+5. What implications do these PRs have for privacy? Robustness/redundancy?
+   Usability?
+
+6. Why was three DNS seeds chosen in [PR
+   15558](*https://github.com/bitcoin/bitcoin/pull/15558)?