net: improve code documentation for dns seed behaviour

ajtowns commented at 0:15 am on May 28, 2020: member

Some better internal documentation post #16939

fanquake added the label P2P on May 28, 2020

DrahtBot commented at 4:59 am on May 28, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in src/net.cpp:57 in 829d9a8ea8 outdated

50@@ -51,6 +51,19 @@ static constexpr int DUMP_PEERS_INTERVAL = 15 * 60;
51 /** Number of DNS seeds to query when the number of connections is low. */
52 static constexpr int DNSSEEDS_TO_QUERY_AT_ONCE = 3;
53 
54+/** How long to delay before querying DNS seeds
55+ *
56+ * If we have more than THRESHOLD entries in addrman, then it's likely
57+ * that we got those addresses from having previously connected to the p2p

laanwj commented at 11:03 am on May 28, 2020:

capitalize P2P please

in src/net.cpp:63 in 829d9a8ea8 outdated

58+ * network, and that we'll be able to successfully reconnect to the p2p
59+ * network via contacting one of them. So if that's the case, spend a
60+ * little longer trying to connect to known peers before querying the
61+ * DNS seeds.
62+ */
63+static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11}; // 11sec

laanwj commented at 11:03 am on May 28, 2020:

the 11sec comment is needless here, the constant (using std::chrono type is self-documenting)

in src/net.cpp:64 in 829d9a8ea8 outdated

59+ * network via contacting one of them. So if that's the case, spend a
60+ * little longer trying to connect to known peers before querying the
61+ * DNS seeds.
62+ */
63+static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11}; // 11sec
64+static constexpr std::chrono::seconds DNSSEEDS_DELAY_MANY_PEERS{300}; // 5min

laanwj commented at 11:04 am on May 28, 2020:

same but std::chrono::minutes … {5}` ?

ajtowns force-pushed on May 28, 2020

fanquake referenced this in commit 73407ff65c on May 29, 2020

fanquake commented at 10:27 am on May 29, 2020: member

Concept ACK. ecc86b9d514c6d67766eb2dd76f745ebfa36238a looks pretty good. Can you rebase and mark as ready for review?

hebasto commented at 10:28 am on May 29, 2020: member

Concept ACK.

in src/net.cpp:65 in ecc86b9d51 outdated

62  */
63-static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11}; // 11sec
64-static constexpr std::chrono::seconds DNSSEEDS_DELAY_MANY_PEERS{300}; // 5min
65-static constexpr int DNSSEEDS_DELAY_PEER_THRESHOLD = 1000; // "many" vs "few" peers -- you should only get this many if you've been on the live network
66+static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11};
67+static constexpr std::chrono::minutes DNSSEEDS_DELAY_MANY_PEERS{5};

MarcoFalke commented at 11:45 am on May 29, 2020:

ACK on this change

naumenkogs commented at 2:28 pm on May 29, 2020: member

ACK ecc86b9

net: improve code documentation for dns seed behaviour 5cb7ee67a5

ajtowns force-pushed on May 29, 2020

ajtowns renamed this:
~~net: add comments on dns seed behaviour~~
net: improve code documentation for dns seed behaviour
on May 29, 2020

ajtowns marked this as ready for review on May 29, 2020

ajtowns commented at 4:30 pm on May 29, 2020: member

Rebased, un-drafted PR, added some comments reflecting [review by @Sjors](/bitcoin-bitcoin/16939/#pullrequestreview-420310676)

in src/net.cpp:66 in 5cb7ee67a5

64-static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11}; // 11sec
65-static constexpr std::chrono::seconds DNSSEEDS_DELAY_MANY_PEERS{300}; // 5min
66-static constexpr int DNSSEEDS_DELAY_PEER_THRESHOLD = 1000; // "many" vs "few" peers -- you should only get this many if you've been on the live network
67+static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11};
68+static constexpr std::chrono::minutes DNSSEEDS_DELAY_MANY_PEERS{5};
69+static constexpr int DNSSEEDS_DELAY_PEER_THRESHOLD = 1000; // "many" vs "few" peers

fanquake commented at 8:24 am on May 30, 2020:

nit: "many" vs "few" peers seems redundant now that there is an explainer above.

fanquake approved

fanquake commented at 8:25 am on May 30, 2020: member

ACK 5cb7ee67a5c0e5da39eb698b64d23722fb2f7b3e - thanks for following up.

in src/net.cpp:57 in 5cb7ee67a5

52@@ -53,10 +53,17 @@ static constexpr std::chrono::minutes DUMP_PEERS_INTERVAL{15};
53 static constexpr int DNSSEEDS_TO_QUERY_AT_ONCE = 3;
54 
55 /** How long to delay before querying DNS seeds
56+ *
57+ * If we have more than THRESHOLD entries in addrman, then it's likely

hebasto commented at 9:01 am on May 30, 2020:

nit: maybe

0 * If we have more than PEER_THRESHOLD entries in addrman, then it's likely

?

hebasto commented at 9:02 am on May 30, 2020: member

ACK 5cb7ee67a5c0e5da39eb698b64d23722fb2f7b3e

sidhujag referenced this in commit 4f7ea77c81 on May 31, 2020

in src/net.cpp:1814 in 5cb7ee67a5

1808@@ -1797,6 +1809,9 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1809             return;
1810 
1811         // Add seed nodes if DNS seeds are all down (an infrastructure attack?).
1812+        // Note that we only do this if we started with an empty peers.dat,
1813+        // (in which case we will query DNS seeds immediately) *and* the DNS
1814+        // seeds have not returned any results.

ariard commented at 11:28 pm on June 1, 2020:

I don’t think calling those peers “fixed seeds” matches with DNS seeds meaning. Contrary to DNS seeds open as fOneShot=true, we open regular connections to them, and not querying a ADDR and then disconnecting ?

naumenkogs commented at 6:58 am on June 2, 2020:

I agree it’s a bit confusing that we call both of them seeds, but the problem is not with this PR. This PR just adapts to the existing naming. Are you suggesting renaming fixed seeds to hardcoded nodes or something? That alone would require a separate PR.

ariard commented at 10:16 pm on June 2, 2020:

Yes I’m suggesting renaming them with a different names to mark difference of mechanism in a separate PR.

fanquake commented at 3:03 am on June 3, 2020:

I’m going to merge this now. @ariard please open a followup for this (and if you want the other nits).

ariard commented at 11:29 pm on June 1, 2020: member

ACK 5cb7ee6

naumenkogs commented at 7:00 am on June 2, 2020: member

ACK 5cb7ee6

fanquake merged this on Jun 3, 2020

fanquake closed this on Jun 3, 2020

sidhujag referenced this in commit dd925a31c5 on Jun 3, 2020

deadalnix referenced this in commit fefbdfffde on Nov 24, 2020

DrahtBot locked this on Feb 15, 2022

net: improve code documentation for dns seed behaviour #19084

Conflicts