net: improve code documentation for dns seed behaviour #19084
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
This was referenced May 28, 2020
laanwj
reviewed
May 28, 2020
laanwj
reviewed
May 28, 2020
laanwj
reviewed
May 28, 2020
829d9a8 to
ecc86b9
Compare
fanquake
added a commit
that referenced
this pull request
May 29, 2020
96954d1 DNS seeds: don't query DNS while network is inactive (Anthony Towns) fa5894f DNS seeds: wait for 5m instead of 11s if 1000+ peers are known (Anthony Towns) Pull request description: Changes the logic for querying DNS seeds: after this PR, if there's less than 1000 entries in addrman, it will still usually query DNS seeds after 11s (unless the first few peers tried mostly succeed), but if there's more than 1000 entries it won't try DNS seeds until 5 minutes have passed without getting multiple outbound peers. (If there's 0 entries in addrman, it will still immediately query the DNS seeds). Additionally, delays querying DNS seeds while the p2p network is not active. Fixes #15434 ACKs for top commit: fanquake: ACK 96954d1 - Ran some tests of different scenarios. More documentation is being added in #19084. ariard: Tested ACK 96954d1, on Debian 9.1. Both MANY_PEERS/FEW_PEERS cases work. Sjors: tACK 96954d1 (rebased on master) on macOS 10.15.4. It found it useful to run with `-debug=addrman` and change `DNSSEEDS_DELAY_MANY_PEERS` to something lower to test the behaviour, as well as renaming `peers.dat` to test the peer threshold. naumenkogs: utACK 96954d1 Tree-SHA512: 73693db3da73bf8e76c3df9e9c82f0a7fb08049187356eac2575c4ffa455f76548dd1c86a11fc6beea8a3baf0ba020e047bebe927883c731383ec72442356005
Member
|
Concept ACK. ecc86b9d514c6d67766eb2dd76f745ebfa36238a looks pretty good. Can you rebase and mark as ready for review? |
Member
|
Concept ACK. |
maflcko
reviewed
May 29, 2020
Contributor
|
ACK ecc86b9 |
ecc86b9 to
5cb7ee6
Compare
Contributor
Author
|
Rebased, un-drafted PR, added some comments reflecting review by @Sjors |
fanquake
approved these changes
May 30, 2020
| 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 | ||
| static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11}; | ||
| static constexpr std::chrono::minutes DNSSEEDS_DELAY_MANY_PEERS{5}; | ||
| static constexpr int DNSSEEDS_DELAY_PEER_THRESHOLD = 1000; // "many" vs "few" peers |
Member
There was a problem hiding this comment.
nit: "many" vs "few" peers seems redundant now that there is an explainer above.
hebasto
reviewed
May 30, 2020
|
|
||
| /** How long to delay before querying DNS seeds | ||
| * | ||
| * If we have more than THRESHOLD entries in addrman, then it's likely |
Member
There was a problem hiding this comment.
nit: maybe
Suggested change
| * If we have more than THRESHOLD entries in addrman, then it's likely | |
| * If we have more than PEER_THRESHOLD entries in addrman, then it's likely |
?
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 31, 2020
96954d1 DNS seeds: don't query DNS while network is inactive (Anthony Towns) fa5894f DNS seeds: wait for 5m instead of 11s if 1000+ peers are known (Anthony Towns) Pull request description: Changes the logic for querying DNS seeds: after this PR, if there's less than 1000 entries in addrman, it will still usually query DNS seeds after 11s (unless the first few peers tried mostly succeed), but if there's more than 1000 entries it won't try DNS seeds until 5 minutes have passed without getting multiple outbound peers. (If there's 0 entries in addrman, it will still immediately query the DNS seeds). Additionally, delays querying DNS seeds while the p2p network is not active. Fixes bitcoin#15434 ACKs for top commit: fanquake: ACK 96954d1 - Ran some tests of different scenarios. More documentation is being added in bitcoin#19084. ariard: Tested ACK 96954d1, on Debian 9.1. Both MANY_PEERS/FEW_PEERS cases work. Sjors: tACK 96954d1 (rebased on master) on macOS 10.15.4. It found it useful to run with `-debug=addrman` and change `DNSSEEDS_DELAY_MANY_PEERS` to something lower to test the behaviour, as well as renaming `peers.dat` to test the peer threshold. naumenkogs: utACK 96954d1 Tree-SHA512: 73693db3da73bf8e76c3df9e9c82f0a7fb08049187356eac2575c4ffa455f76548dd1c86a11fc6beea8a3baf0ba020e047bebe927883c731383ec72442356005
ariard
reviewed
Jun 1, 2020
| // Add seed nodes if DNS seeds are all down (an infrastructure attack?). | ||
| // Note that we only do this if we started with an empty peers.dat, | ||
| // (in which case we will query DNS seeds immediately) *and* the DNS | ||
| // seeds have not returned any results. |
There was a problem hiding this comment.
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 ?
Contributor
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes I'm suggesting renaming them with a different names to mark difference of mechanism in a separate PR.
Member
There was a problem hiding this comment.
I'm going to merge this now. @ariard please open a followup for this (and if you want the other nits).
Contributor
|
ACK 5cb7ee6 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 3, 2020
…aviour 5cb7ee6 net: improve code documentation for dns seed behaviour (Anthony Towns) Pull request description: Some better internal documentation post bitcoin#16939 ACKs for top commit: hebasto: ACK 5cb7ee6 naumenkogs: ACK 5cb7ee6 ariard: ACK 5cb7ee6 fanquake: ACK 5cb7ee6 - thanks for following up. Tree-SHA512: 5a12680651cd1e0436aed1cadcbf50047ffeabfdc9f7f2f81fa176c30b10673fc960154c73ec34ed08ace1a000a81cedd44d67587883152654dee46065991b45
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 24, 2020
Summary: Backport of core [[bitcoin/bitcoin#19084 | PR19084]]. There is no change in behavior. Depends on D8512. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8513
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some better internal documentation post #16939