net: Make AddrFetch connections to fixed seeds #26114
Conversation
15908a7 to
3c74b6c
Compare
|
Concept ACK. |
|
Concept ACK, will review |
96471c4 to
9b09fc7
Compare
9b09fc7 to
e438878
Compare
|
9b09fc7 to e438878: |
|
Concept ACK |
|
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. |
|
utACK e4388787870ac2b42903cde47152e4735eb1be86 |
|
Concept ACK |
e438878 to
7e95f8f
Compare
|
utACK 7e95f8f |
I would very much appreciate that. As I started reviewing this PR, the first thing I did (before reading this comment actually) was see if I could refactor this function into something a bit more easily digestible just as a PoC. Below the diff for a very rough suggestion which probably suffers from incompleteness, poor naming, etc. Compiles fine but I do get some warnings re git diffdiff --git a/src/net.cpp b/src/net.cpp
index 925334db1..f4d475315 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1388,6 +1388,132 @@ void CConnman::WakeMessageHandler()
condMsgProc.notify_one();
}
+void CConnman::WaitForDNSSeeding(int already_found)
+{
+ const std::chrono::seconds seeds_wait_time = (addrman.size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);
+ LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
+ std::chrono::seconds to_wait = seeds_wait_time;
+ while (to_wait.count() > 0) {
+ // if sleeping for the MANY_PEERS interval, wake up
+ // early to see if we have enough peers and can stop
+ // this thread entirely freeing up its resources
+ std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait);
+ if (!interruptNet.sleep_for(w)) return;
+ to_wait -= w;
+
+ int nRelevant = 0;
+ {
+ LOCK(m_nodes_mutex);
+ for (const CNode* pnode : m_nodes) {
+ if (pnode->fSuccessfullyConnected && pnode->IsFullOutboundConn()) ++nRelevant;
+ }
+ }
+ if (nRelevant >= 2) {
+ if (already_found > 0) {
+ LogPrintf("%d addresses found from DNS seeds\n", already_found);
+ LogPrintf("P2P peers available. Finished DNS seeding.\n");
+ } else {
+ LogPrintf("P2P peers available. Skipped DNS seeding.\n");
+ }
+ return;
+ }
+ }
+}
+
+bool CConnman::AddFixedSeeds()
+{
+ std::vector<CAddress> seed_addrs{ConvertSeeds(Params().FixedSeeds())};
+ // We will not make outgoing connections to peers that are unreachable
+ // (e.g. because of -onlynet configuration).
+ // Therefore, we do not add them to addrman in the first place.
+ // Note that if you change -onlynet setting from one network to another,
+ // peers.dat will contain only peers of unreachable networks and
+ // manual intervention will be needed (either delete peers.dat after
+ // configuration change or manually add some reachable peer using addnode),
+ // see <https://github.com/bitcoin/bitcoin/issues/26035> for details.
+ seed_addrs.erase(std::remove_if(seed_addrs.begin(), seed_addrs.end(),
+ [](const CAddress& addr) { return !IsReachable(addr); }),
+ seed_addrs.end());
+ Shuffle(seed_addrs.begin(), seed_addrs.end(), FastRandomContext());
+
+ // Make AddrFetch connections to fixed seeds first. This reduces the
+ // load on the fixed seeds that would otherwise be serving blocks for
+ // many new peers requiring IBD. Try this with multiple fixed seeds for
+ // a better diversity of received addrs and because some may be offline.
+ LogPrintf("Initiating AddrFetch connections to fixed seeds.\n");
+ for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) {
+ if (addr_pos >= seed_addrs.size()) {
+ break;
+ }
+ AddAddrFetch(seed_addrs.at(addr_pos).ToString());
+ }
+ // Give AddrFetch peers some time to provide us with addresses
+ // before adding the fixed seeds to AddrMan
+ if (!interruptNet.sleep_for(std::chrono::minutes(2))) {
+ return false;
+ }
+ // The fixed seeds queried in the previous steps might have been offline,
+ // failed to send us any addresses or sent us fake ones. Therefore,
+ // we now add all reachable fixed seeds to AddrMan as a fallback.
+ CNetAddr local;
+ local.SetInternal("fixedseeds");
+ addrman.Add(seed_addrs, local);
+ LogPrintf("Added %d fixed seeds from reachable networks.\n", seed_addrs.size());
+ return true;
+}
+
+bool CConnman::ShouldAddFixedSeedsNow(std::chrono::microseconds start, bool dns_seeds_requested)
+{
+ // When the node starts with an empty peers.dat, there are a few other sources of peers before
+ // we fallback on to fixed seeds: -dnsseed, -seednode, -addnode
+ // If none of those are available, we fallback on to fixed seeds immediately, else we allow
+ // 60 seconds for any of those sources to populate addrman.
+ // It is cheapest to check if enough time has passed first.
+ if (GetTime<std::chrono::seconds>() > start + std::chrono::minutes{1}) {
+ LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty\n");
+ return true;
+ }
+
+ // Checking !dnsseed is cheaper before locking 2 mutexes.
+ if (!dns_seeds_requested) {
+ LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
+ if (m_addr_fetches.empty() && m_added_nodes.empty()) {
+ LogPrintf("Adding fixed seeds as -dnsseed=0 (or IPv4/IPv6 connections are disabled via -onlynet), -addnode is not provided and all -seednode(s) attempted\n");
+ return true;
+ }
+ }
+ return false;
+}
+
+int CConnman::AddAddressesFromSeed(const std::string& seed, FastRandomContext& rng)
+{
+ std::vector<CNetAddr> vIPs;
+ std::vector<CAddress> vAdd;
+ ServiceFlags requiredServiceBits = GetDesirableServiceFlags(NODE_NONE);
+ std::string host = strprintf("x%x.%s", requiredServiceBits, seed);
+ CNetAddr resolveSource;
+ if (!resolveSource.SetInternal(host)) {
+ return 0;
+ }
+ unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed
+ int found{0};
+ if (LookupHost(host, vIPs, nMaxIPs, true)) {
+ for (const CNetAddr& ip : vIPs) {
+ CAddress addr = CAddress(CService(ip, Params().GetDefaultPort()), requiredServiceBits);
+ addr.nTime = rng.rand_uniform_delay(Now<NodeSeconds>() - 3 * 24h, -4 * 24h); // use a random age between 3 and 7 days old
+ vAdd.push_back(addr);
+ found++;
+ }
+ addrman.Add(vAdd, resolveSource);
+ } else {
+ // We now avoid directly using results from DNS Seeds which do not support service bit filtering,
+ // instead using them as a addrfetch to get nodes with our desired service bits.
+ AddAddrFetch(seed);
+ }
+
+ return found;
+}
+
void CConnman::ThreadAddressSeed()
{
SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION_ADDR_SEED);
@@ -1422,40 +1548,13 @@ void CConnman::ThreadAddressSeed()
// * If we continue having problems, eventually query all the
// DNS seeds, and if that fails too, also try the fixed seeds.
// (done in ThreadOpenConnections)
- const std::chrono::seconds seeds_wait_time = (addrman.size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);
for (const std::string& seed : seeds) {
if (seeds_right_now == 0) {
seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;
if (addrman.size() > 0) {
- LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
- std::chrono::seconds to_wait = seeds_wait_time;
- while (to_wait.count() > 0) {
- // if sleeping for the MANY_PEERS interval, wake up
- // early to see if we have enough peers and can stop
- // this thread entirely freeing up its resources
- std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait);
- if (!interruptNet.sleep_for(w)) return;
- to_wait -= w;
-
- int nRelevant = 0;
- {
- LOCK(m_nodes_mutex);
- for (const CNode* pnode : m_nodes) {
- if (pnode->fSuccessfullyConnected && pnode->IsFullOutboundConn()) ++nRelevant;
- }
- }
- if (nRelevant >= 2) {
- if (found > 0) {
- LogPrintf("%d addresses found from DNS seeds\n", found);
- LogPrintf("P2P peers available. Finished DNS seeding.\n");
- } else {
- LogPrintf("P2P peers available. Skipped DNS seeding.\n");
- }
- return;
- }
- }
+ WaitForDNSSeeding(found);
}
}
@@ -1473,28 +1572,7 @@ void CConnman::ThreadAddressSeed()
if (HaveNameProxy()) {
AddAddrFetch(seed);
} else {
- std::vector<CNetAddr> vIPs;
- std::vector<CAddress> vAdd;
- ServiceFlags requiredServiceBits = GetDesirableServiceFlags(NODE_NONE);
- std::string host = strprintf("x%x.%s", requiredServiceBits, seed);
- CNetAddr resolveSource;
- if (!resolveSource.SetInternal(host)) {
- continue;
- }
- unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed
- if (LookupHost(host, vIPs, nMaxIPs, true)) {
- for (const CNetAddr& ip : vIPs) {
- CAddress addr = CAddress(CService(ip, Params().GetDefaultPort()), requiredServiceBits);
- addr.nTime = rng.rand_uniform_delay(Now<NodeSeconds>() - 3 * 24h, -4 * 24h); // use a random age between 3 and 7 days old
- vAdd.push_back(addr);
- found++;
- }
- addrman.Add(vAdd, resolveSource);
- } else {
- // We now avoid directly using results from DNS Seeds which do not support service bit filtering,
- // instead using them as a addrfetch to get nodes with our desired service bits.
- AddAddrFetch(seed);
- }
+ found += AddAddressesFromSeed(seed, rng);
}
--seeds_right_now;
}
@@ -1509,65 +1587,11 @@ void CConnman::ThreadAddressSeed()
return;
}
while (addrman.size() == 0) {
- // When the node starts with an empty peers.dat, there are a few other sources of peers before
- // we fallback on to fixed seeds: -dnsseed, -seednode, -addnode
- // If none of those are available, we fallback on to fixed seeds immediately, else we allow
- // 60 seconds for any of those sources to populate addrman.
- bool add_fixed_seeds_now = false;
- // It is cheapest to check if enough time has passed first.
- if (GetTime<std::chrono::seconds>() > start + std::chrono::minutes{1}) {
- add_fixed_seeds_now = true;
- LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty\n");
- }
-
- // Checking !dnsseed is cheaper before locking 2 mutexes.
- if (!add_fixed_seeds_now && !dns_seeds_requested) {
- LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
- if (m_addr_fetches.empty() && m_added_nodes.empty()) {
- add_fixed_seeds_now = true;
- LogPrintf("Adding fixed seeds as -dnsseed=0 (or IPv4/IPv6 connections are disabled via -onlynet), -addnode is not provided and all -seednode(s) attempted\n");
- }
- }
+ auto add_fixed_seeds_now{ShouldAddFixedSeedsNow(start, dns_seeds_requested)};
if (add_fixed_seeds_now) {
- std::vector<CAddress> seed_addrs{ConvertSeeds(Params().FixedSeeds())};
- // We will not make outgoing connections to peers that are unreachable
- // (e.g. because of -onlynet configuration).
- // Therefore, we do not add them to addrman in the first place.
- // Note that if you change -onlynet setting from one network to another,
- // peers.dat will contain only peers of unreachable networks and
- // manual intervention will be needed (either delete peers.dat after
- // configuration change or manually add some reachable peer using addnode),
- // see <https://github.com/bitcoin/bitcoin/issues/26035> for details.
- seed_addrs.erase(std::remove_if(seed_addrs.begin(), seed_addrs.end(),
- [](const CAddress& addr) { return !IsReachable(addr); }),
- seed_addrs.end());
- Shuffle(seed_addrs.begin(), seed_addrs.end(), FastRandomContext());
-
- // Make AddrFetch connections to fixed seeds first. This reduces the
- // load on the fixed seeds that would otherwise be serving blocks for
- // many new peers requiring IBD. Try this with multiple fixed seeds for
- // a better diversity of received addrs and because some may be offline.
- LogPrintf("Initiating AddrFetch connections to fixed seeds.\n");
- for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) {
- if (addr_pos >= seed_addrs.size()) {
- break;
- }
- AddAddrFetch(seed_addrs.at(addr_pos).ToString());
- }
- // Give AddrFetch peers some time to provide us with addresses
- // before adding the fixed seeds to AddrMan
- if (!interruptNet.sleep_for(std::chrono::minutes(2))) {
- return;
- }
- // The fixed seeds queried in the previous steps might have been offline,
- // failed to send us any addresses or sent us fake ones. Therefore,
- // we now add all reachable fixed seeds to AddrMan as a fallback.
- CNetAddr local;
- local.SetInternal("fixedseeds");
- addrman.Add(seed_addrs, local);
- LogPrintf("Added %d fixed seeds from reachable networks.\n", seed_addrs.size());
- break;
+ if (AddFixedSeeds()) break;
+ return; // interrupted
}
if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
return;
diff --git a/src/net.h b/src/net.h
index 1a92392fd..1fec113af 100644
--- a/src/net.h
+++ b/src/net.h
@@ -694,6 +694,12 @@ public:
bool m_i2p_accept_incoming;
};
+ void WaitForDNSSeeding(int already_found);
+ int AddAddressesFromSeed(const std::string& seed, FastRandomContext& rng);
+ bool ShouldAddFixedSeedsNow(std::chrono::microseconds start, bool dns_seeds_requested);
+ bool AddFixedSeeds();
+
+
void Init(const Options& connOptions) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_total_bytes_sent_mutex)
{
AssertLockNotHeld(m_total_bytes_sent_mutex); |
Awesome, I'll get to that soon! |
|
While implementing and testing the |
brunoerg
left a comment
There was a problem hiding this comment.
ACK d8df9f94f933c9d5fd19a58a4b1473ea9becd362
d8df9f9 to
c07bfae
Compare
bc74135 to
6430bea
Compare
There was a problem hiding this comment.
Light code review.
I feel like I'm missing something here though. Before the changes of this pull, the fixed seeds would be pulled after a minute of trying DNS seeds and not reaching the desired peer goal. However, after this pull ProcessFixedSeeds is called sequentially after QueryDNSSeeds. And the latter has no exit logic based on how long that has been running, meaning that if both are set ProcessFixedSeeds will never trigger (?).
PS: Well, technically that's not completely accurate. This will finish after querying all the DNS seeds and not achieving the target connection count, but that would potentially make the fixed seeds trigger later than before
src/net.cpp
Outdated
There was a problem hiding this comment.
In c1be7952cff9669b87e5002dcbc990dbf726c06f
A timer is used both for QueryDNSSeeds and ProcessFixedSeeds (to wait for 30 secs on the former, and 1 min on the latter). The timer is implicitly shared, we are setting it before entering QueryDNSSeeds and passing it to ProcessFixedSeeds. However, for QueryDNSSeeds we parse the time again inside the method. This should not be necessary. start_time could also be passed here.
There was a problem hiding this comment.
Good point! The 30s timer in QueryDNSSeeds was introduced recently, I hadn't really thought about this during the last rebase.
I'll add this to the cleanup commit after the move.
I also updated the earlier commit to use NodeClock instead of the deprecated GetTime.
Yes, that's true. I think that the timing will more likely change in unusual situations, here is one I could think of: |
6430bea to
c97bd16
Compare
This is in preparation of adding the fixed seed logic to this thread.
Always start ThreadAddressSeed() and move the criterion of skipping the DNS seeds into this thread. This is in preparation of moving the fixed seed logic into this thread. This commit does not change external behavior. Can be reviewed with "git diff -b" to ignore whitespace changes.
Adding fixed seeds to addrman needs to be done only once. As such, it makes more sense to have it in ThreadAddrSeed as opposed to ThreadOpenConnections. This does not change external behavior, however it can slightly change the timing between fixed and dns seeds. can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Since ProcessFixedSeeds() is only entered if fixed seeds are enabled, it has become unnecessary.
In order to reduce the load on fixed seeds that will receive potentially many peers requiring IBD, just do an AddrFetch (so that we disconnect them after receiving addresses from them). Do that with up to 10 fixed seeds for diversity. The fixed seeds continue to be added to AddrMan afterwards, which at this point should contain multiple other addresses received from the AddrFetch peers.
c97bd16 to
b7b1a3c
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
The refactoring commits are pretty annoying to rebase (and probably also to review) - maybe I'll open another version without the refactoring (although I think it makes sense to get the fixed seeds logic out of ThreadOpenConnections). |
This proposes two things:
Make AddrFetch connections to fixed seeds instead of just adding them to AddrMan (suggested in p2p: skip querying dns seeds if
-onlynetdisables IPv4 and IPv6 #25678 (comment))When adding fixed seeds, we currently add them to AddrMan and then make regular full outbound connections to them. If many new nodes use the fixed seeds for this, it will result in them getting a lot of peers requiring IBD, which will create a lot of traffic for them. With an AddrFetch connection, we will only use them to get addresses from other peers and disconnect afterwards.
This PR proposes to attempt making AddrFetch connections to 10 peers for better diversity (some may be offline anyway). The fixed seeds will still be added to Addrman as before, but only after a delay of 2 minutes, after which they will hopefully no longer be the only entries in AddrMan.
Move the logic of adding fixed seeds out of
ThreadOpenConnectionsintoThreadDNSAddressSeed(which is being renamed toThreadAddressSeed).I think this makes sense in general because adding the fixed seeds is in many ways analogous to querying the DNS seeds, and ThreadOpenConnections, which is there to actually open the connections is already quite complex.
Also, it makes the changes from 1) easier if we don't have to worry about interfering with the automatic connection making logic.
One way to test this is to start without
peers.datand run with-nodnsseed -blocksonly -debug=netand monitor the debug log andbitcoin-cli -netinfobehavior.