net: Make AddrFetch connections to fixed seeds #26114
Conversation
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
from
15908a7
to
3c74b6c
Compare
|
Concept ACK. |
|
Concept ACK, will review |
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
2 times, most recently
from
96471c4
to
9b09fc7
Compare
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
from
9b09fc7
to
e438878
Compare
|
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 e438878 |
|
Concept ACK |
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
from
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 |
| // 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. This might take up to 2 minutes.\n"); | ||
| for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) { |
There was a problem hiding this comment.
In dfd635b "net: Make AddrFetch connections to fixed seeds": When running multiple networks, wouldn't it be good to ensure that we're covering all networks? I mean, having at least one per network of the 10 ones.
There was a problem hiding this comment.
I agree that could be good! Though I think it would add more complexity to this pull, so I would prefer not to do it here - could be a good follow-up though.
Just noting that we do add all fixed seeds to addrman though, so with the connection logic from #27213 we'd eventually still make a connection to the network.
There was a problem hiding this comment.
I agree that could be good! Though I think it would add more complexity to this pull, so I would prefer not to do it here - could be a good follow-up though.
Sgtm!
|
Are you still working on this? |
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
2 times, most recently
from
63ab51c
to
d8df9f9
Compare
src/net.cpp
Outdated
| } | ||
| // 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))) { |
There was a problem hiding this comment.
I agree. I've tested it following
contrib/seeds/README.mdto generate the seeds and the worst scenario was connecting with 5 of the 10.
Yes, I see similar results. If older versions are used, it could become a bit worse though.
nit: the log might use a variable 2, so that it's defined in a single place.
I have now used a variable for the 2 minutes (sorry, I missed this comment before).
| // 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. This might take up to 2 minutes.\n"); | ||
| for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) { |
There was a problem hiding this comment.
I agree that could be good! Though I think it would add more complexity to this pull, so I would prefer not to do it here - could be a good follow-up though.
Just noting that we do add all fixed seeds to addrman though, so with the connection logic from #27213 we'd eventually still make a connection to the network.
src/net.cpp
Outdated
| } | ||
| // 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. |
There was a problem hiding this comment.
I have reordered the sentence now so that it is clearer now hopefully!
There was a problem hiding this comment.
Just tested with -nodnsseed -blocksonly -debug=net with an empty peers.dat and checked the behaviour is as expected. Some logs:
- Adding fixed seeds
2024-04-25T13:02:59Z Adding fixed seeds as -dnsseed=0 (or IPv4/IPv6 connections are disabled via -onlynet) and neither -addnode nor -seednode are provided
2024-04-25T13:02:59Z [net] Added hardcoded seed: 1.253.159.19:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 2.152.74.211:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 5.2.23.226:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 5.128.87.126:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 5.157.103.202:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 5.188.62.18:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 5.253.18.218:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 5.255.109.160:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 8.129.184.255:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 8.209.70.77:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 8.210.18.56:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 12.34.98.148:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 18.27.79.17:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 23.93.170.118:8333
...
- Establishing the addr-fetch connections.
2024-04-25T13:02:59Z Initiating AddrFetch connections to fixed seeds. This might take up to 2 minutes.
2024-04-25T13:02:59Z Progress loading mempool transactions from file: 10% (tried 120, 1072 remaining)
2024-04-25T13:02:59Z Progress loading mempool transactions from file: 20% (tried 239, 953 remaining)
2024-04-25T13:02:59Z [net] trying v2 connection 2001:4dd0:af0e:3564::69:90 lastseen=0.0hrs
2024-04-25T13:03:00Z [net] Added connection peer=0
2024-04-25T13:03:00Z [net] sending version (103 bytes) peer=0
2024-04-25T13:03:00Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=0
2024-04-25T13:03:00Z [net] start sending v2 handshake to peer=0
2024-04-25T13:03:00Z Progress loading mempool transactions from file: 30% (tried 358, 834 remaining)
2024-04-25T13:03:00Z [net] trying v2 connection 69.112.103.124 lastseen=0.0hrs
2024-04-25T13:03:00Z [net] received: version (102 bytes) peer=0
2024-04-25T13:03:00Z [net] sending wtxidrelay (0 bytes) peer=0
2024-04-25T13:03:00Z [net] sending sendaddrv2 (0 bytes) peer=0
2024-04-25T13:03:00Z [net] sending verack (0 bytes) peer=0
2024-04-25T13:03:00Z [net] Added connection peer=1
2024-04-25T13:03:00Z [net] sending version (103 bytes) peer=1
2024-04-25T13:03:00Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=1
2024-04-25T13:03:00Z [net] sending getaddr (0 bytes) peer=0
2024-04-25T13:03:00Z [net] receive version message: /Satoshi:27.0.0/: version 70016, blocks=840823, us=[2804:14c:3bfb:37:e55d:6a9c:bb51:8079]:50636, txrelay=1, peer=0
2024-04-25T13:03:00Z [net] added time data, samples 2, offset +0 (+0 minutes)
2024-04-25T13:03:00Z [net] start sending v2 handshake to peer=1
2024-04-25T13:03:00Z [net] received: wtxidrelay (0 bytes) peer=0
2024-04-25T13:03:00Z [net] received: sendaddrv2 (0 bytes) peer=0
2024-04-25T13:03:00Z [net] received: verack (0 bytes) peer=0
2024-04-25T13:03:00Z New addr-fetch v2 peer connected: version: 70016, blocks=840823, peer=0
2024-04-25T13:03:00Z [net] sending sendcmpct (9 bytes) peer=0
2024-04-25T13:03:00Z [net] sending ping (8 bytes) peer=0
2024-04-25T13:03:00Z [net] Advertising address [2804:14c:3bfb:37:e55d:6a9c:bb51:8079]:8333 to peer=0
2024-04-25T13:03:00Z [net] sending addrv2 (28 bytes) peer=0
2024-04-25T13:03:00Z [net] sending getheaders (1029 bytes) peer=0
2024-04-25T13:03:00Z [net] initial getheaders (840821) to peer=0 (startheight:840823)
2024-04-25T13:03:00Z [net] received: sendcmpct (9 bytes) peer=0
2024-04-25T13:03:00Z [net] received: ping (8 bytes) peer=0
2024-04-25T13:03:00Z [net] sending pong (8 bytes) peer=0
2024-04-25T13:03:00Z [net] received: getheaders (1029 bytes) peer=0
2024-04-25T13:03:00Z [net] getheaders -1 to end from peer=0
2024-04-25T13:03:00Z [net] sending headers (1 bytes) peer=0
2024-04-25T13:03:00Z [net] received: feefilter (8 bytes) peer=0
2024-04-25T13:03:00Z [net] received: feefilter of 0.00004924 BTC/kvB from peer=0
2024-04-25T13:03:00Z [net] socket closed for peer=1
2024-04-25T13:03:00Z [net] disconnecting peer=1
2024-04-25T13:03:00Z [net] retrying with v1 transport protocol for peer=1
024-04-25T13:03:01Z [net] Cleared nodestate for peer=1
2024-04-25T13:03:01Z Progress loading mempool transactions from file: 40% (tried 477, 715 remaining)
2024-04-25T13:03:01Z Progress loading mempool transactions from file: 50% (tried 596, 596 remaining)
2024-04-25T13:03:01Z Progress loading mempool transactions from file: 60% (tried 716, 476 remaining)
2024-04-25T13:03:01Z [net] trying v1 connection 69.112.103.124 lastseen=0.0hrs
2024-04-25T13:03:01Z [net] Added connection peer=2
2024-04-25T13:03:01Z [net] received: addrv2 (17295 bytes) peer=0
2024-04-25T13:03:01Z [net] sending version (103 bytes) peer=2
2024-04-25T13:03:01Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=2
2024-04-25T13:03:01Z [net] trying v2 connection 27.124.108.19 lastseen=0.0hrs
2024-04-25T13:03:01Z [net] Received addr: 1000 addresses (1000 processed, 0 rate-limited) from peer=0
2024-04-25T13:03:01Z [net] addrfetch connection completed peer=0; disconnecting
2024-04-25T13:03:01Z [net] disconnecting peer=0
2024-04-25T13:03:01Z [net] Cleared nodestate for peer=0
2024-04-25T13:03:01Z Progress loading mempool transactions from file: 70% (tried 835, 357 remaining)
2024-04-25T13:03:01Z Progress loading mempool transactions from file: 80% (tried 954, 238 remaining)
2024-04-25T13:03:01Z Progress loading mempool transactions from file: 90% (tried 1073, 119 remaining)
2024-04-25T13:03:01Z Imported mempool transactions from file: 1192 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2024-04-25T13:03:01Z initload thread exit
2024-04-25T13:03:01Z [net] socket closed for peer=2
2024-04-25T13:03:01Z [net] disconnecting peer=2
2024-04-25T13:03:01Z [net] Cleared nodestate for peer=2
2024-04-25T13:03:02Z [net] Added connection peer=3
2024-04-25T13:03:02Z [net] sending version (103 bytes) peer=3
2024-04-25T13:03:02Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=3
2024-04-25T13:03:02Z [net] start sending v2 handshake to peer=3
2024-04-25T13:03:02Z [net] socket closed for peer=3
2024-04-25T13:03:02Z [net] disconnecting peer=3
2024-04-25T13:03:02Z [net] retrying with v1 transport protocol for peer=3
2024-04-25T13:03:02Z [net] Cleared nodestate for peer=3
2024-04-25T13:03:02Z [net] trying v1 connection 27.124.108.19 lastseen=0.0hrs
2024-04-25T13:03:02Z [net] Added connection peer=4
2024-04-25T13:03:02Z [net] sending version (103 bytes) peer=4
2024-04-25T13:03:02Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=4
2024-04-25T13:03:02Z [net] trying v1 connection [2804:14c:7d87:88bd::1003]:8333 lastseen=317.1hrs
2024-04-25T13:03:03Z [net] received: version (102 bytes) peer=4
2024-04-25T13:03:03Z [net] sending wtxidrelay (0 bytes) peer=4
2024-04-25T13:03:03Z [net] sending sendaddrv2 (0 bytes) peer=4
2024-04-25T13:03:03Z [net] sending verack (0 bytes) peer=4
2024-04-25T13:03:03Z [net] sending getaddr (0 bytes) peer=4
2024-04-25T13:03:03Z [net] receive version message: /Satoshi:25.0.0/: version 70016, blocks=840823, us=187.183.43.117:1077, txrelay=1, peer=4
2024-04-25T13:03:03Z [net] added time data, samples 3, offset +0 (+0 minutes)
2024-04-25T13:03:03Z [net] received: wtxidrelay (0 bytes) peer=4
2024-04-25T13:03:03Z [net] received: sendaddrv2 (0 bytes) peer=4
2024-04-25T13:03:03Z [net] received: verack (0 bytes) peer=4
2024-04-25T13:03:03Z New addr-fetch v1 peer connected: version: 70016, blocks=840823, peer=4
...
This is in preparation of adding the fixed seed logic to this thread.
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
from
d8df9f9
to
c07bfae
Compare
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
from
c07bfae
to
bc74135
Compare
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.
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
from
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
| void CConnman::ProcessFixedSeeds(std::chrono::microseconds start) | ||
| { | ||
| const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED); | ||
| bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS); |
There was a problem hiding this comment.
In c1be795
Isn't add_fixed_seeds unconditionally true now? This is called only by:
if (gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS)) {
ProcessFixedSeeds(start_time);
}
There was a problem hiding this comment.
Yes! That's why the next commit removes it. The reason for the split up into 2 commits is that I wanted to make the previous commit as "move-only" as possible so it is easier to review (especially with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space).
| @@ -2321,13 +2321,78 @@ void CConnman::QueryDNSSeeds() | |||
| } | |||
| } | |||
|
|
|||
| void CConnman::ProcessFixedSeeds(std::chrono::microseconds start) | |||
| { | |||
| const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED); | |||
There was a problem hiding this comment.
In c1be795
Given this is called right after QueryDNSSeeds() conditionally to -dnsseed being set, we could make this method take it instead of re-parsing it here.
There was a problem hiding this comment.
True - on one hand this should make it minimally faster - on the other hand having simpler function signatures is also nice. I don't think it really matters much either way, parsing an arg once more can't be very expensive and this is not on any critical path anyway, so I kinda like the current way more.
| { | ||
| const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED); | ||
| bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS); | ||
| const bool use_seednodes{gArgs.IsArgSet("-seednode")}; |
There was a problem hiding this comment.
In c1be795
This can also be passed as a param. We are parsing it both here and in CConnman::QueryDNSSeeds()
There was a problem hiding this comment.
see previous comment
src/net.cpp
Outdated
| void CConnman::ThreadAddressSeed() | ||
| { | ||
| auto start_time = GetTime<std::chrono::microseconds>(); |
There was a problem hiding this comment.
In c1be795
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: |
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
- remove add_fixed_seeds from ProcessFixedSeeds Since ProcessFixedSeeds() is only entered if fixed seeds are enabled, it has become unnecessary. - use a common start time for QueryDNSSeeds and ProcessFixedSeeds - remove outdated comment from QueryDNSSeeds
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.
mzumsande
force-pushed
the
202209_addrfetch_fixedseeds
branch
from
6430bea
to
c97bd16
Compare
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.