Skip to content

Conversation

mzumsande
Copy link
Contributor

@mzumsande mzumsande commented Sep 16, 2022

This proposes two things:

  1. Make AddrFetch connections to fixed seeds instead of just adding them to AddrMan (suggested in p2p: skip querying dns seeds if -onlynet disables 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.

  2. Move the logic of adding fixed seeds out of ThreadOpenConnections into ThreadDNSAddressSeed (which is being renamed to ThreadAddressSeed).
    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.dat and run with -nodnsseed -blocksonly -debug=net and monitor the debug log and bitcoin-cli -netinfo behavior.

@mzumsande mzumsande force-pushed the 202209_addrfetch_fixedseeds branch from 15908a7 to 3c74b6c Compare September 16, 2022 21:16
@DrahtBot DrahtBot added the P2P label Sep 16, 2022
@mzumsande mzumsande marked this pull request as ready for review September 19, 2022 17:48
@naumenkogs
Copy link
Member

Concept ACK.
The motivation makes perfect sense and it's indeed an improvement.

@jonatack
Copy link
Member

Concept ACK, will review

@mzumsande mzumsande force-pushed the 202209_addrfetch_fixedseeds branch 2 times, most recently from 96471c4 to 9b09fc7 Compare September 20, 2022 16:02
@mzumsande mzumsande force-pushed the 202209_addrfetch_fixedseeds branch from 9b09fc7 to e438878 Compare September 21, 2022 19:50
@mzumsande
Copy link
Contributor Author

9b09fc7 to e438878:
Addressed review feedback, small doc change.

@brunoerg
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 23, 2022

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK Rspigler, dergoegge, jonatack, darosior
Stale ACK naumenkogs, stratospher, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30529 (Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
  • #29641 (scripted-diff: Use LogInfo over LogPrintf by maflcko)
  • #29605 (net: Favor peers from addrman over fetching seednodes by sr-gi)

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.

@naumenkogs
Copy link
Member

utACK e438878

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP review

@Rspigler
Copy link
Contributor

Concept ACK

@mzumsande mzumsande force-pushed the 202209_addrfetch_fixedseeds branch from e438878 to 7e95f8f Compare September 26, 2022 14:29
@mzumsande
Copy link
Contributor Author

e438878 to 7e95f8f:

Addressed review feedback by @jonatack (thanks!).

One additional refactor I thought about but wasn't sure if it is worth it, is moving most of the logic currently inside ThreadAddressSeed into their own functions (ProcessDNSSeeds() and ProcessFixedSeeds()).

@naumenkogs
Copy link
Member

utACK 7e95f8f

Copy link

@amovfx amovfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed for pr club.

@stickies-v
Copy link
Contributor

One additional refactor I thought about but wasn't sure if it is worth it, is moving most of the logic currently inside ThreadAddressSeed into their own functions (ProcessDNSSeeds() and ProcessFixedSeeds()).

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 warning: calling function 'AddAddrFetch' requires negative capability '!m_addr_fetches_mutex' [-Wthread-safety-analysis] that I haven't resolved in this PoC.

git diff
diff --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);

@mzumsande
Copy link
Contributor Author

One additional refactor I thought about but wasn't sure if it is worth it, is moving most of the logic currently inside ThreadAddressSeed
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. (...)

Awesome, I'll get to that soon!

@mzumsande mzumsande marked this pull request as draft October 18, 2022 21:34
@mzumsande
Copy link
Contributor Author

While implementing and testing the ThreadAddressSeed code organization changes, I ran into a small issue/ behavior change caused by the movement of the fixed seeding into another thread - I need to analyze this closer, marking as Draft for now.

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested with -nodnsseed -blocksonly -debug=net with an empty peers.dat and checked the behaviour is as expected. Some logs:

  1. 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
...
  1. 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
...

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d8df9f9

@mzumsande
Copy link
Contributor Author

mzumsande commented May 1, 2024

d8df9f9 to 6430bea: Rebased

@mzumsande mzumsande force-pushed the 202209_addrfetch_fixedseeds branch 2 times, most recently from bc74135 to 6430bea Compare May 1, 2024 17:46
@DrahtBot DrahtBot removed the CI failed label May 1, 2024
Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

void CConnman::ThreadAddressSeed()
{
auto start_time = GetTime<std::chrono::microseconds>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mzumsande mzumsande May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mzumsande
Copy link
Contributor Author

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

Yes, that's true.
I think under normal circumstances querying all DNS seeds shouldn't take more than 60s:
If DNSSEEDS_DELAY_FEW_PEERS=11s applies, it should be sufficient because we process 3 (DNSSEEDS_TO_QUERY_AT_ONCE) seeds at a time and we only have 9 seeds (soon 10) - even if we don't make a successful outbound connection during that time. Also note that as soon as if we have an address from a network in addrman, we don't query fixed seeds from it anymore.

I think that the timing will more likely change in unusual situations, here is one I could think of:
If we have an addrman full of bad peers (so that DNSSEEDS_DELAY_MANY_PEERS is used, but we make no successful connections) - maybe an extremely old peers.dat then we spend a lot of time waiting in QueryDNSSeeds. In that case, fixed seed loading of other networks for which addrman is empty, would be delayed. I don't think this is a very typical scenario though.
I tried to address this with "This does not change external behavior, however it can slightly change the timing
between fixed and dns seeds." in the commit message.

@mzumsande mzumsande force-pushed the 202209_addrfetch_fixedseeds branch from 6430bea to c97bd16 Compare May 24, 2024 17:11
@mzumsande
Copy link
Contributor Author

6430bea to c97bd16:
Addressed feedback by @sr-gi, thanks!

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.
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 4, 2024

🐙 This pull request conflicts with the target branch and needs rebase.

@mzumsande
Copy link
Contributor Author

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).

@mzumsande mzumsande closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.