net: Make AddrFetch connections to fixed seeds

mzumsande commented at 9:01 pm on September 16, 2022: contributor

This proposes two things:

Make AddrFetch connections to fixed seeds instead of just adding them to AddrMan (suggested in #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 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 force-pushed on Sep 16, 2022

DrahtBot added the label P2P on Sep 16, 2022

mzumsande marked this as ready for review on Sep 19, 2022

in src/net.cpp:1564 in 3c74b6cb25 outdated

1559+            // Give AddrFetch peers some time to provide us with addresses
1560+            // before adding the fixed seeds to AddrMan
1561+            if (!interruptNet.sleep_for(std::chrono::minutes(2))) {
1562+                return;
1563+            }
1564+            // As an additional precaution against malicious fixed seeds, add all

naumenkogs commented at 7:00 am on September 20, 2022:

Let’s specify what exactly it protects us from?

Them not giving us anything
Them giving us a lot of fake addrs (I assume our addrman will bucket them together and we won’t have to go through every single of them, according to GetNewBucket)

mzumsande commented at 4:00 pm on September 20, 2022:

Yes, and 3. them just not being not online. I changed the comment to reflect this better.

in src/net.cpp:1553 in 3c74b6cb25 outdated

1548+
1549+            // Make AddrFetch connections to fixed seeds first. This reduces the
1550+            // load on the fixed seeds that would otherwise be serving blocks for
1551+            // many new peers requiring IBD.
1552+            LogPrintf("Initiating AddrFetch connections to fixed seeds.\n");
1553+            for(size_t addr_pos = 0; addr_pos < 10; addr_pos++) {

naumenkogs commented at 7:02 am on September 20, 2022:

nits according to developer notes:

++xyz is better than xyz++ :P
one space right after for and if

mzumsande commented at 4:00 pm on September 20, 2022:

Fixed, thanks!

naumenkogs commented at 7:03 am on September 20, 2022: member

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

jonatack commented at 9:50 am on September 20, 2022: member

Concept ACK, will review

mzumsande force-pushed on Sep 20, 2022

in src/net.cpp:1522 in 3c370ba615 outdated

1502@@ -1502,6 +1503,57 @@ void CConnman::ThreadAddressSeed()
1503     else{
1504         LogPrintf("DNS seeding disabled\n");
1505     }
1506+    // Fixed seeds
1507+    bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);
1508+
1509+    if (!add_fixed_seeds) {

naumenkogs commented at 9:18 am on September 21, 2022:

Can you just inline GetBoolArg here? You do update this variable later, but i think the updated version is not used at all?

mzumsande commented at 7:50 pm on September 21, 2022:

Good catch! It was needed in ThreadOpenConnections, but no longer after the move.

in src/net.cpp:1513 in 3c370ba615 outdated

1508+
1509+    if (!add_fixed_seeds) {
1510+        LogPrintf("Fixed seeds are disabled\n");
1511+        return;
1512+    }
1513+    while ( addrman.size() == 0) {

naumenkogs commented at 9:18 am on September 21, 2022:

nit: remove space before addrman

jonatack commented at 9:23 am on September 21, 2022:

(Friendly tip, one can avoid nitty formatting issues by running clang format on commits before pushing, see /contrib/devtools/clang-format-diff.py for details.)

mzumsande commented at 7:51 pm on September 21, 2022:

Done, thanks!

mzumsande force-pushed on Sep 21, 2022

mzumsande commented at 7:52 pm on September 21, 2022: contributor

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

brunoerg commented at 2:03 pm on September 22, 2022: contributor

Concept ACK

DrahtBot commented at 3:29 am on September 23, 2022: contributor

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 commented at 8:13 am on September 23, 2022: member

utACK e4388787870ac2b42903cde47152e4735eb1be86

in src/net.cpp:1396 in e438878787 outdated

1442-                    {
1443-                        LOCK(m_nodes_mutex);
1444-                        for (const CNode* pnode : m_nodes) {
1445-                            if (pnode->fSuccessfullyConnected && pnode->IsFullOutboundConn()) ++nRelevant;
1446+    auto start = GetTime<std::chrono::microseconds>();
1447+    const bool dnsseed{gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)};

jonatack commented at 8:45 am on September 23, 2022:

5989fc1bf861aa2777f2515ddc9281569d2d87b4 naming nit, mind making this variable name more specific so that grepping on it returns clearer results? for example:

0@@ -1393,8 +1393,8 @@ void CConnman::ThreadAddressSeed()
1-    const bool dnsseed{gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)};
2-    if (dnsseed) {
3+    const bool dns_seeds_requested{gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)};
4+    if (dns_seeds_requested) {
5@@ -1521,7 +1521,7 @@ void CConnman::ThreadAddressSeed()
6-        if (!add_fixed_seeds_now && !dnsseed) {
7+        if (!add_fixed_seeds_now && !dns_seeds_requested) {

0$ git grep dns_seeds_requested
1src/net.cpp:1396:    const bool dns_seeds_requested{gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)};
2src/net.cpp:1397:    if (dns_seeds_requested) {
3src/net.cpp:1524:        if (!add_fixed_seeds_now && !dns_seeds_requested) {

mzumsande commented at 2:30 pm on September 26, 2022:

renamed to dns_seeds_requested as suggested

in src/net.h:1326 in e438878787 outdated

938@@ -939,7 +939,7 @@ class CConnman
939     void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock);
940 
941     void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
942-    void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex);
943+    void ThreadAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex);

jonatack commented at 9:04 am on September 23, 2022:

fc1a3719b0cd733985b1e6d9234819823c3bfab8 It looks like this thread safety annotation added here can be correspondingly removed from ThreadOpenConnections(), as it no longer accesses m_added_nodes.

0--- a/src/net.h
1+++ b/src/net.h
2@@ -888,7 +888,7 @@ private:
3-    void ThreadOpenConnections(std::vector<std::string> connect) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex);
4+    void ThreadOpenConnections(std::vector<std::string> connect) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex);

mzumsande commented at 2:30 pm on September 26, 2022:

Done, thanks!

in src/net.cpp:1565 in e438878787 outdated

1631+
1632+            // Make AddrFetch connections to fixed seeds first. This reduces the
1633+            // load on the fixed seeds that would otherwise be serving blocks for
1634+            // many new peers requiring IBD.
1635+            LogPrintf("Initiating AddrFetch connections to fixed seeds.\n");
1636+            for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) {

jonatack commented at 9:10 am on September 23, 2022:

e4388787870ac2b42903cde47152e4735eb1be8 it might be helpful to explain/document the magic value of 10, similar to the commit message

mzumsande commented at 2:30 pm on September 26, 2022:

Done.

jonatack commented at 9:14 am on September 23, 2022: member

WIP review

Rspigler commented at 1:18 am on September 24, 2022: contributor

Concept ACK

mzumsande force-pushed on Sep 26, 2022

mzumsande commented at 2:35 pm on September 26, 2022: contributor

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 commented at 8:55 am on October 3, 2022: member

utACK 7e95f8f6bb0e06b0676db3da05b2a55a011c9668

amovfx commented at 10:32 pm on October 4, 2022: none

Reviewed for pr club.

stickies-v commented at 5:00 pm on October 5, 2022: 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.

  0diff --git a/src/net.cpp b/src/net.cpp
  1index 925334db1..f4d475315 100644
  2--- a/src/net.cpp
  3+++ b/src/net.cpp
  4@@ -1388,6 +1388,132 @@ void CConnman::WakeMessageHandler()
  5     condMsgProc.notify_one();
  6 }
  7 
  8+void CConnman::WaitForDNSSeeding(int already_found)
  9+{
 10+    const std::chrono::seconds seeds_wait_time = (addrman.size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);
 11+    LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
 12+    std::chrono::seconds to_wait = seeds_wait_time;
 13+    while (to_wait.count() > 0) {
 14+        // if sleeping for the MANY_PEERS interval, wake up
 15+        // early to see if we have enough peers and can stop
 16+        // this thread entirely freeing up its resources
 17+        std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait);
 18+        if (!interruptNet.sleep_for(w)) return;
 19+        to_wait -= w;
 20+
 21+        int nRelevant = 0;
 22+        {
 23+            LOCK(m_nodes_mutex);
 24+            for (const CNode* pnode : m_nodes) {
 25+                if (pnode->fSuccessfullyConnected && pnode->IsFullOutboundConn()) ++nRelevant;
 26+            }
 27+        }
 28+        if (nRelevant >= 2) {
 29+            if (already_found > 0) {
 30+                LogPrintf("%d addresses found from DNS seeds\n", already_found);
 31+                LogPrintf("P2P peers available. Finished DNS seeding.\n");
 32+            } else {
 33+                LogPrintf("P2P peers available. Skipped DNS seeding.\n");
 34+            }
 35+            return;
 36+        }
 37+    }
 38+}
 39+
 40+bool CConnman::AddFixedSeeds()
 41+{
 42+    std::vector<CAddress> seed_addrs{ConvertSeeds(Params().FixedSeeds())};
 43+    // We will not make outgoing connections to peers that are unreachable
 44+    // (e.g. because of -onlynet configuration).
 45+    // Therefore, we do not add them to addrman in the first place.
 46+    // Note that if you change -onlynet setting from one network to another,
 47+    // peers.dat will contain only peers of unreachable networks and
 48+    // manual intervention will be needed (either delete peers.dat after
 49+    // configuration change or manually add some reachable peer using addnode),
 50+    // see <https://github.com/bitcoin/bitcoin/issues/26035> for details.
 51+    seed_addrs.erase(std::remove_if(seed_addrs.begin(), seed_addrs.end(),
 52+                                    [](const CAddress& addr) { return !IsReachable(addr); }),
 53+                        seed_addrs.end());
 54+    Shuffle(seed_addrs.begin(), seed_addrs.end(), FastRandomContext());
 55+
 56+    // Make AddrFetch connections to fixed seeds first. This reduces the
 57+    // load on the fixed seeds that would otherwise be serving blocks for
 58+    // many new peers requiring IBD. Try this with multiple fixed seeds for
 59+    // a better diversity of received addrs and because some may be offline.
 60+    LogPrintf("Initiating AddrFetch connections to fixed seeds.\n");
 61+    for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) {
 62+        if (addr_pos >= seed_addrs.size()) {
 63+            break;
 64+        }
 65+        AddAddrFetch(seed_addrs.at(addr_pos).ToString());
 66+    }
 67+    // Give AddrFetch peers some time to provide us with addresses
 68+    // before adding the fixed seeds to AddrMan
 69+    if (!interruptNet.sleep_for(std::chrono::minutes(2))) {
 70+        return false;
 71+    }
 72+    // The fixed seeds queried in the previous steps might have been offline,
 73+    // failed to send us any addresses or sent us fake ones. Therefore,
 74+    // we now add all reachable fixed seeds to AddrMan as a fallback.
 75+    CNetAddr local;
 76+    local.SetInternal("fixedseeds");
 77+    addrman.Add(seed_addrs, local);
 78+    LogPrintf("Added %d fixed seeds from reachable networks.\n", seed_addrs.size());
 79+    return true;
 80+}
 81+
 82+bool CConnman::ShouldAddFixedSeedsNow(std::chrono::microseconds start, bool dns_seeds_requested)
 83+{
 84+    // When the node starts with an empty peers.dat, there are a few other sources of peers before
 85+    // we fallback on to fixed seeds: -dnsseed, -seednode, -addnode
 86+    // If none of those are available, we fallback on to fixed seeds immediately, else we allow
 87+    // 60 seconds for any of those sources to populate addrman.
 88+    // It is cheapest to check if enough time has passed first.
 89+    if (GetTime<std::chrono::seconds>() > start + std::chrono::minutes{1}) {
 90+        LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty\n");
 91+        return true;
 92+    }
 93+
 94+    // Checking !dnsseed is cheaper before locking 2 mutexes.
 95+    if (!dns_seeds_requested) {
 96+        LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
 97+        if (m_addr_fetches.empty() && m_added_nodes.empty()) {
 98+            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");
 99+            return true;
100+        }
101+    }
102+    return false;
103+}
104+
105+int CConnman::AddAddressesFromSeed(const std::string& seed, FastRandomContext& rng)
106+{
107+    std::vector<CNetAddr> vIPs;
108+    std::vector<CAddress> vAdd;
109+    ServiceFlags requiredServiceBits = GetDesirableServiceFlags(NODE_NONE);
110+    std::string host = strprintf("x%x.%s", requiredServiceBits, seed);
111+    CNetAddr resolveSource;
112+    if (!resolveSource.SetInternal(host)) {
113+        return 0;
114+    }
115+    unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed
116+    int found{0};
117+    if (LookupHost(host, vIPs, nMaxIPs, true)) {
118+        for (const CNetAddr& ip : vIPs) {
119+            CAddress addr = CAddress(CService(ip, Params().GetDefaultPort()), requiredServiceBits);
120+            addr.nTime = rng.rand_uniform_delay(Now<NodeSeconds>() - 3 * 24h, -4 * 24h); // use a random age between 3 and 7 days old
121+            vAdd.push_back(addr);
122+            found++;
123+        }
124+        addrman.Add(vAdd, resolveSource);
125+    } else {
126+        // We now avoid directly using results from DNS Seeds which do not support service bit filtering,
127+        // instead using them as a addrfetch to get nodes with our desired service bits.
128+        AddAddrFetch(seed);
129+    }
130+
131+    return found;
132+}
133+
134 void CConnman::ThreadAddressSeed()
135 {
136     SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION_ADDR_SEED);
137@@ -1422,40 +1548,13 @@ void CConnman::ThreadAddressSeed()
138         // * If we continue having problems, eventually query all the
139         //   DNS seeds, and if that fails too, also try the fixed seeds.
140         //   (done in ThreadOpenConnections)
141-        const std::chrono::seconds seeds_wait_time = (addrman.size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);
142 
143         for (const std::string& seed : seeds) {
144             if (seeds_right_now == 0) {
145                 seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;
146 
147                 if (addrman.size() > 0) {
148-                    LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
149-                    std::chrono::seconds to_wait = seeds_wait_time;
150-                    while (to_wait.count() > 0) {
151-                        // if sleeping for the MANY_PEERS interval, wake up
152-                        // early to see if we have enough peers and can stop
153-                        // this thread entirely freeing up its resources
154-                        std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait);
155-                        if (!interruptNet.sleep_for(w)) return;
156-                        to_wait -= w;
157-
158-                        int nRelevant = 0;
159-                        {
160-                            LOCK(m_nodes_mutex);
161-                            for (const CNode* pnode : m_nodes) {
162-                                if (pnode->fSuccessfullyConnected && pnode->IsFullOutboundConn()) ++nRelevant;
163-                            }
164-                        }
165-                        if (nRelevant >= 2) {
166-                            if (found > 0) {
167-                                LogPrintf("%d addresses found from DNS seeds\n", found);
168-                                LogPrintf("P2P peers available. Finished DNS seeding.\n");
169-                            } else {
170-                                LogPrintf("P2P peers available. Skipped DNS seeding.\n");
171-                            }
172-                            return;
173-                        }
174-                    }
175+                    WaitForDNSSeeding(found);
176                 }
177             }
178 
179@@ -1473,28 +1572,7 @@ void CConnman::ThreadAddressSeed()
180             if (HaveNameProxy()) {
181                 AddAddrFetch(seed);
182             } else {
183-                std::vector<CNetAddr> vIPs;
184-                std::vector<CAddress> vAdd;
185-                ServiceFlags requiredServiceBits = GetDesirableServiceFlags(NODE_NONE);
186-                std::string host = strprintf("x%x.%s", requiredServiceBits, seed);
187-                CNetAddr resolveSource;
188-                if (!resolveSource.SetInternal(host)) {
189-                    continue;
190-                }
191-                unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed
192-                if (LookupHost(host, vIPs, nMaxIPs, true)) {
193-                    for (const CNetAddr& ip : vIPs) {
194-                        CAddress addr = CAddress(CService(ip, Params().GetDefaultPort()), requiredServiceBits);
195-                        addr.nTime = rng.rand_uniform_delay(Now<NodeSeconds>() - 3 * 24h, -4 * 24h); // use a random age between 3 and 7 days old
196-                        vAdd.push_back(addr);
197-                        found++;
198-                    }
199-                    addrman.Add(vAdd, resolveSource);
200-                } else {
201-                    // We now avoid directly using results from DNS Seeds which do not support service bit filtering,
202-                    // instead using them as a addrfetch to get nodes with our desired service bits.
203-                    AddAddrFetch(seed);
204-                }
205+                found += AddAddressesFromSeed(seed, rng);
206             }
207             --seeds_right_now;
208         }
209@@ -1509,65 +1587,11 @@ void CConnman::ThreadAddressSeed()
210         return;
211     }
212     while (addrman.size() == 0) {
213-        // When the node starts with an empty peers.dat, there are a few other sources of peers before
214-        // we fallback on to fixed seeds: -dnsseed, -seednode, -addnode
215-        // If none of those are available, we fallback on to fixed seeds immediately, else we allow
216-        // 60 seconds for any of those sources to populate addrman.
217-        bool add_fixed_seeds_now = false;
218-        // It is cheapest to check if enough time has passed first.
219-        if (GetTime<std::chrono::seconds>() > start + std::chrono::minutes{1}) {
220-            add_fixed_seeds_now = true;
221-            LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty\n");
222-        }
223-
224-        // Checking !dnsseed is cheaper before locking 2 mutexes.
225-        if (!add_fixed_seeds_now && !dns_seeds_requested) {
226-            LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
227-            if (m_addr_fetches.empty() && m_added_nodes.empty()) {
228-                add_fixed_seeds_now = true;
229-                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");
230-            }
231-        }
232+        auto add_fixed_seeds_now{ShouldAddFixedSeedsNow(start, dns_seeds_requested)};
233 
234         if (add_fixed_seeds_now) {
235-            std::vector<CAddress> seed_addrs{ConvertSeeds(Params().FixedSeeds())};
236-            // We will not make outgoing connections to peers that are unreachable
237-            // (e.g. because of -onlynet configuration).
238-            // Therefore, we do not add them to addrman in the first place.
239-            // Note that if you change -onlynet setting from one network to another,
240-            // peers.dat will contain only peers of unreachable networks and
241-            // manual intervention will be needed (either delete peers.dat after
242-            // configuration change or manually add some reachable peer using addnode),
243-            // see <https://github.com/bitcoin/bitcoin/issues/26035> for details.
244-            seed_addrs.erase(std::remove_if(seed_addrs.begin(), seed_addrs.end(),
245-                                            [](const CAddress& addr) { return !IsReachable(addr); }),
246-                             seed_addrs.end());
247-            Shuffle(seed_addrs.begin(), seed_addrs.end(), FastRandomContext());
248-
249-            // Make AddrFetch connections to fixed seeds first. This reduces the
250-            // load on the fixed seeds that would otherwise be serving blocks for
251-            // many new peers requiring IBD. Try this with multiple fixed seeds for
252-            // a better diversity of received addrs and because some may be offline.
253-            LogPrintf("Initiating AddrFetch connections to fixed seeds.\n");
254-            for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) {
255-                if (addr_pos >= seed_addrs.size()) {
256-                    break;
257-                }
258-                AddAddrFetch(seed_addrs.at(addr_pos).ToString());
259-            }
260-            // Give AddrFetch peers some time to provide us with addresses
261-            // before adding the fixed seeds to AddrMan
262-            if (!interruptNet.sleep_for(std::chrono::minutes(2))) {
263-                return;
264-            }
265-            // The fixed seeds queried in the previous steps might have been offline,
266-            // failed to send us any addresses or sent us fake ones. Therefore,
267-            // we now add all reachable fixed seeds to AddrMan as a fallback.
268-            CNetAddr local;
269-            local.SetInternal("fixedseeds");
270-            addrman.Add(seed_addrs, local);
271-            LogPrintf("Added %d fixed seeds from reachable networks.\n", seed_addrs.size());
272-            break;
273+            if (AddFixedSeeds()) break;
274+            return;  // interrupted
275         }
276         if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
277             return;
278diff --git a/src/net.h b/src/net.h
279index 1a92392fd..1fec113af 100644
280--- a/src/net.h
281+++ b/src/net.h
282@@ -694,6 +694,12 @@ public:
283         bool m_i2p_accept_incoming;
284     };
285 
286+    void WaitForDNSSeeding(int already_found);
287+    int AddAddressesFromSeed(const std::string& seed, FastRandomContext& rng);
288+    bool ShouldAddFixedSeedsNow(std::chrono::microseconds start, bool dns_seeds_requested);
289+    bool AddFixedSeeds();
290+
291+
292     void Init(const Options& connOptions) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_total_bytes_sent_mutex)
293     {
294         AssertLockNotHeld(m_total_bytes_sent_mutex);

mzumsande commented at 11:21 pm on October 12, 2022: 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 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 marked this as a draft on Oct 18, 2022

mzumsande commented at 6:37 pm on October 19, 2022: contributor

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.

DrahtBot added the label Needs rebase on Jan 31, 2023

mzumsande force-pushed on Apr 20, 2023

DrahtBot removed the label Needs rebase on Apr 20, 2023

mzumsande commented at 10:01 pm on April 20, 2023: contributor

Rebased and added a refactor to split up ThreadAddressSeed into separate functions for DNS and fixed seeds - I will keep in draft a bit longer, until the issue below is resolved.

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.

The issue was that if one chooses -seednode=X, the idea is to use the peer X for seeding, and not dns seeds or fixed seeds. However, currently there is a “race” between using this seednode and fixed seeds, because as soon as we make a connection to the seednode, we won’t wait until we have receive addrs from it, but start adding fixed seeds right away (and connecting to them / getting addrs from them might be faster). Moving the fixed seeds out of ThreadOpenConnection would have changed the timing of this race slightly.

However, I think that the current behavior is also not great in the first place and will open a separate PR to give the seednode some time before reverting to fixed seeds, so that this PR doesn’t change behavior.

dergoegge commented at 9:12 am on June 22, 2023: member

Concept ACK

DrahtBot added the label Needs rebase on Jun 23, 2023

mzumsande force-pushed on Jun 28, 2023

mzumsande commented at 6:41 pm on June 28, 2023: contributor

Updated and rebased due to multiple conflicts - in particular #27577 got merged which should’ve fixed the problem mentioned above. I intend to move this out of draft in a few days after some manual testing / possible CI fixes.

DrahtBot removed the label Needs rebase on Jun 28, 2023

mzumsande marked this as ready for review on Jul 5, 2023

mzumsande commented at 9:51 pm on July 5, 2023: contributor

Did some more manual testing and it worked as intended for me - moved out of draft!

jonatack commented at 10:03 pm on July 5, 2023: member

Concept ACK, will have a look at this (and merged #27577 soon).

in test/functional/p2p_dns_seeds.py:5 in 3d06fa372c outdated

1@@ -2,7 +2,7 @@
2 # Copyright (c) 2021 The Bitcoin Core developers
3 # Distributed under the MIT software license, see the accompanying
4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
5-"""Test ThreadDNSAddressSeed logic for querying DNS seeds."""
6+"""Test ThreadAddressSeed logic for querying DNS seeds."""

naumenkogs commented at 9:25 am on July 26, 2023:

3d06fa372c963a0957f54fe80a0ff3462cdb3a2d nit: might want to rename the file too :)

mzumsande commented at 9:03 pm on July 27, 2023:

I think it still makes sense because the functional test only covers the dns seed logic.

in src/net.cpp:2380 in 30b0f57b19 outdated

1561+                    }
1562+                    AddAddrFetch(seed_addrs.at(addr_pos).ToStringAddr());
1563+                }
1564+                // Give AddrFetch peers some time to provide us with addresses
1565+                // before adding the fixed seeds to AddrMan
1566+                if (!interruptNet.sleep_for(std::chrono::minutes(2))) {

naumenkogs commented at 9:46 am on July 27, 2023:

30b0f57b19eddd47fa137d0c20ae39174b54dbad

Doesn’t this substantially change the observations of a user? If they get unlucky with the 10 seeds and gets stuck for 2 minutes, which was not the case before?

Maybe worth expanding a log this might take up to 2 minutes. or something. Ideally we won’t wait anymore once we realize all those attempts failed already, but that might be difficult to code.

mzumsande commented at 9:09 pm on July 27, 2023:

I think that usually it shouldn’t substantially change the observations of a user, because 10 is a pretty high number and if all of those fail, we either have rally bad fixed seeds, or have a connectivity problem. I’ll add a log entry with the next push though. Also, I’ll do some test runs to gather some statistics whether we might want to reduce this to 1 minute…

mzumsande commented at 3:22 pm on August 18, 2023:

added the log.

naumenkogs commented at 10:56 am on August 23, 2023:

nit: the log might use a variable 2, so that it’s defined in a single place.

brunoerg commented at 10:04 am on April 10, 2024:

I think that usually it shouldn’t substantially change the observations of a user, because 10 is a pretty high number and if all of those fail, we either have rally bad fixed seeds, or have a connectivity problem.

I agree. I’ve tested it following contrib/seeds/README.md to generate the seeds and the worst scenario was connecting with 5 of the 10.

mzumsande commented at 6:37 pm on April 24, 2024:

I agree. I’ve tested it following contrib/seeds/README.md to 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).

naumenkogs changes_requested

DrahtBot added the label Needs rebase on Aug 6, 2023

mzumsande force-pushed on Aug 18, 2023

mzumsande commented at 3:22 pm on August 18, 2023: contributor

30b0f57 to 17028c1: Rebased, expanded log entry with this might take up to 2 minutes

DrahtBot removed the label Needs rebase on Aug 18, 2023

in src/net.cpp:2291 in 8b1f718211 outdated

1508@@ -1509,13 +1509,73 @@ void CConnman::QueryDNSSeeds()
1509     LogPrintf("%d addresses found from DNS seeds\n", found);
1510 }
1511 
1512+void CConnman::QueryFixedSeeds(std::chrono::microseconds start)

naumenkogs commented at 10:56 am on August 23, 2023:

8b1f718211b19fcf59b933c0cae7e237309913ad

Nothing is queried in this function, so why does it have this name?

naumenkogs commented at 10:57 am on August 23, 2023:

Ah sure it’s done in the next commit. Maybe a commit message could mention it, or an in-code todo deleted in the next commit :)

mzumsande commented at 9:38 pm on September 25, 2023:

I will rename it to ProcessFixedSeeds() (I did it mostly so that it sounds similar to QueryDNSSeeds): Even with the last commit, we don’t actually query the fixed seeds in this function or thread, we just add them to m_addr_fetches, and ThreadOpenConnection will make the actual addrfetch connections that could be described as “querying”.

naumenkogs commented at 10:58 am on August 23, 2023: member

ACK 17028c1fec3f109b139e5e23f535c86f5a1ac2a4

DrahtBot added the label Needs rebase on Sep 14, 2023

mzumsande force-pushed on Sep 25, 2023

mzumsande commented at 9:33 pm on September 25, 2023: contributor

17028c1 to 10aa6d3: rebased

10aa6d3 to fbbe25e: changed name of QueryFixedSeeds to ProcessFixedSeeds

mzumsande force-pushed on Sep 25, 2023

DrahtBot added the label CI failed on Sep 25, 2023

DrahtBot removed the label Needs rebase on Sep 25, 2023

DrahtBot removed the label CI failed on Sep 25, 2023

DrahtBot added the label Needs rebase on Oct 3, 2023

mzumsande force-pushed on Oct 3, 2023

mzumsande commented at 5:51 pm on October 3, 2023: contributor

fbbe25e to 06cdad4: rebased

DrahtBot removed the label Needs rebase on Oct 3, 2023

darosior commented at 12:46 pm on October 23, 2023: member

Concept ACK

mzumsande force-pushed on Dec 18, 2023

mzumsande commented at 7:42 pm on December 18, 2023: contributor

06cdad4 to 00e5986:

added missing return to ProcessFixedSeeds() in case where we already have addrs for all networks (thanks @stratospher for pointing me towards it)
added a commit to remove add_fixed_seeds from ProcessFixedSeeds(), since the entire function is gated on this anyway.

00e5986 to dfd635b: Rebased due to silent conflict with #28895

mzumsande force-pushed on Dec 18, 2023

DrahtBot added the label CI failed on Dec 18, 2023

DrahtBot removed the label CI failed on Dec 18, 2023

in doc/developer-notes.md:651 in 7cc4633a86 outdated

651@@ -652,7 +652,7 @@ Threads
652     : Application level message handling (sending and receiving). Almost
653     all net_processing and validation logic runs on this thread.
654 
655-  - [ThreadDNSAddressSeed (`b-dnsseed`)](https://doxygen.bitcoincore.org/class_c_connman.html#aa7c6970ed98a4a7bafbc071d24897d13)
656+  - [ThreadAddressSeed (`b-addrseed`)](https://doxygen.bitcoincore.org/class_c_connman.html#aa7c6970ed98a4a7bafbc071d24897d13)

stratospher commented at 6:40 am on December 19, 2023:

7cc4633: could also update the doc comments to Load addresses of peers from the DNS and from fixed seeds.

mzumsande commented at 6:44 pm on April 24, 2024:

updated the doc!

in src/net.cpp:2241 in 611923ef6d outdated

2266+        // * When querying DNS seeds query a few at once, this ensures
2267+        //   that we don't give DNS seeds the ability to eclipse nodes
2268+        //   that query them.
2269+        // * If we continue having problems, eventually query all the
2270+        //   DNS seeds, and if that fails too, also try the fixed seeds.
2271+        //   (done in ThreadOpenConnections)

stratospher commented at 6:43 am on December 19, 2023:

611923e: we could remove this fixed seed logic handled in ThreadOpenConnections comment.

mzumsande commented at 6:45 pm on April 24, 2024:

Good catch! removed the mention of ThreadOpenConnections in the following commit (to keep it out of the already large move commit)

stratospher commented at 7:12 am on December 19, 2023: contributor

ACK dfd635b.

i think this is a good improvement to have! - better peer diversity compared to what’s happening on master where we directly open connections to hardcoded seeds.

tested it using getaddrmaninfo RPC - before fixed seeds were added, new table already had around 1679 addresses from the addrfetch connections - after fixed seeds were added, new table had 2399 addresses

observed 2 addrfetch connections being successfully opened during the 2 minute delay though.

DrahtBot requested review from brunoerg on Dec 19, 2023

DrahtBot requested review from naumenkogs on Dec 19, 2023

DrahtBot requested review from jonatack on Dec 19, 2023

DrahtBot requested review from dergoegge on Dec 19, 2023

DrahtBot requested review from darosior on Dec 19, 2023

in src/net.cpp:2396 in 8d8e0b796d outdated

2351+            if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) {
2352+                return;
2353+            }
2354+        }
2355+        else {
2356+            LogPrint(BCLog::NET, "No fixed seeds were added (sufficient addresses available).\n");

naumenkogs commented at 9:47 am on December 22, 2023:

8d8e0b796dabba601742a9585237be5892c5f05a

maybe expand for every reachable network

mzumsande commented at 6:42 pm on April 24, 2024:

done, thanks!

DrahtBot requested review from naumenkogs on Dec 22, 2023

in src/net.cpp:2385 in dfd635bcc9 outdated

2358+                if (!interruptNet.sleep_for(std::chrono::minutes(2))) {
2359+                    return;
2360+                }
2361+                // The fixed seeds queried in the previous steps might have been offline,
2362+                // failed to send us any addresses or sent us fake ones. Therefore,
2363+                // we now add all reachable fixed seeds to AddrMan as a fallback.

naumenkogs commented at 9:52 am on December 22, 2023:

dfd635bcc9b3f3615cabe115af302df33efba6a1

nit: The first sentence talks about might, while the second makes a reader think this is unconditionally a fallback (which is not true). I’d suggest aligning the two

mzumsande commented at 6:11 pm on December 22, 2023:

I don’t quite follow: The fixed seeds will eventually be added to addrman as a fallback unconditionally, both in the case where we got sufficient addresses from addrfetch peers and in the case where that didn’t work for some reason.

naumenkogs commented at 8:51 am on January 9, 2024:

Ahhh I guess I thought it means “as a fallback to the situation from the previous sentence (poor in-place seeds)”, but you mean “as a fallback for a general failure” (not that related to the thing above).

This is an even more subtle nit now :)

mzumsande commented at 6:43 pm on April 24, 2024:

I have reordered the sentence now so that it is clearer now hopefully!

DrahtBot requested review from naumenkogs on Dec 22, 2023

DrahtBot added the label CI failed on Jan 1, 2024

brunoerg commented at 7:22 pm on January 8, 2024: contributor

WIP review!

Tested with an empty addrman and -nodnsseed -blocksonly -debug=net. I could check the new flow, an example:

2024-01-08T19:08:02Z [net] Added hardcoded seed: 192.146.137.44:8333
2024-01-08T19:08:18Z [net:debug] trying v1 connection 192.146.137.44 lastseen=0.0hrs
2024-01-08T19:08:19Z New addr-fetch v1 peer connected: version: 70016, blocks=824898, peer=2
2024-01-08T19:08:19Z [net] sending getaddr (0 bytes) peer=2
2024-01-08T19:08:19Z [net] Received addr: 1000 addresses (1000 processed, 0 rate-limited) from peer=2
2024-01-08T19:08:19Z [addrman] Added 891 addresses (of 1000) from 192.146.137.44: 0 tried, 890 new
2024-01-08T19:08:19Z [net] addrfetch connection completed peer=2; disconnecting

DrahtBot requested review from brunoerg on Jan 8, 2024

DrahtBot removed the label CI failed on Jan 9, 2024

DrahtBot added the label CI failed on Jan 14, 2024

achow101 requested review from sr-gi on Apr 9, 2024

achow101 requested review from stratospher on Apr 9, 2024

in src/net.cpp:2371 in dfd635bcc9 outdated

2344+
2345+                // Make AddrFetch connections to fixed seeds first. This reduces the
2346+                // load on the fixed seeds that would otherwise be serving blocks for
2347+                // many new peers requiring IBD. Try this with multiple fixed seeds for
2348+                // a better diversity of received addrs and because some may be offline.
2349+                LogPrintf("Initiating AddrFetch connections to fixed seeds. This might take up to 2 minutes.\n");

brunoerg commented at 8:37 am on April 10, 2024:

In dfd635bcc9b3f3615cabe115af302df33efba6a1 “net: Make AddrFetch connections to fixed seeds”: Perhaps it would be interesting to specify in the log it will initiate AddrFetch connections to 10 fixed seeds instead of just saying “to fixed seeds”.

mzumsande commented at 6:42 pm on April 24, 2024:

added the 10

DrahtBot requested review from brunoerg on Apr 10, 2024

in src/net.cpp:2372 in dfd635bcc9 outdated

2345+                // Make AddrFetch connections to fixed seeds first. This reduces the
2346+                // load on the fixed seeds that would otherwise be serving blocks for
2347+                // many new peers requiring IBD. Try this with multiple fixed seeds for
2348+                // a better diversity of received addrs and because some may be offline.
2349+                LogPrintf("Initiating AddrFetch connections to fixed seeds. This might take up to 2 minutes.\n");
2350+                for (size_t addr_pos = 0; addr_pos < 10; ++addr_pos) {

brunoerg commented at 9:55 am on April 10, 2024:

In dfd635bcc9b3f3615cabe115af302df33efba6a1 “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.

mzumsande commented at 6:42 pm on April 24, 2024:

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.

brunoerg commented at 12:33 pm on April 25, 2024:

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!

DrahtBot requested review from brunoerg on Apr 10, 2024

maflcko commented at 6:49 am on April 23, 2024: member

Are you still working on this?

mzumsande force-pushed on Apr 24, 2024

mzumsande commented at 6:54 pm on April 24, 2024: contributor

Thanks for the reviews and sorry for not following up earlier! I hope I have now addressed all comments.

I also rebased wrt master - strangely github doesn’t seem to detect some merge conflicts in this PR, locally I had conflicts with both #28170 and #29850 - different algorithms maybe?

DrahtBot removed the label CI failed on Apr 24, 2024

brunoerg commented at 4:41 pm on April 25, 2024: contributor

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

Adding fixed seeds

 02024-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
 12024-04-25T13:02:59Z [net] Added hardcoded seed: 1.253.159.19:8333
 22024-04-25T13:02:59Z [net] Added hardcoded seed: 2.152.74.211:8333
 32024-04-25T13:02:59Z [net] Added hardcoded seed: 5.2.23.226:8333
 42024-04-25T13:02:59Z [net] Added hardcoded seed: 5.128.87.126:8333
 52024-04-25T13:02:59Z [net] Added hardcoded seed: 5.157.103.202:8333
 62024-04-25T13:02:59Z [net] Added hardcoded seed: 5.188.62.18:8333
 72024-04-25T13:02:59Z [net] Added hardcoded seed: 5.253.18.218:8333
 82024-04-25T13:02:59Z [net] Added hardcoded seed: 5.255.109.160:8333
 92024-04-25T13:02:59Z [net] Added hardcoded seed: 8.129.184.255:8333
102024-04-25T13:02:59Z [net] Added hardcoded seed: 8.209.70.77:8333
112024-04-25T13:02:59Z [net] Added hardcoded seed: 8.210.18.56:8333
122024-04-25T13:02:59Z [net] Added hardcoded seed: 12.34.98.148:8333
132024-04-25T13:02:59Z [net] Added hardcoded seed: 18.27.79.17:8333
142024-04-25T13:02:59Z [net] Added hardcoded seed: 23.93.170.118:8333
15...

Establishing the addr-fetch connections.

 02024-04-25T13:02:59Z Initiating AddrFetch connections to fixed seeds. This might take up to 2 minutes.
 12024-04-25T13:02:59Z Progress loading mempool transactions from file: 10% (tried 120, 1072 remaining)
 22024-04-25T13:02:59Z Progress loading mempool transactions from file: 20% (tried 239, 953 remaining)
 32024-04-25T13:02:59Z [net] trying v2 connection 2001:4dd0:af0e:3564::69:90 lastseen=0.0hrs
 42024-04-25T13:03:00Z [net] Added connection peer=0
 52024-04-25T13:03:00Z [net] sending version (103 bytes) peer=0
 62024-04-25T13:03:00Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=0
 72024-04-25T13:03:00Z [net] start sending v2 handshake to peer=0
 82024-04-25T13:03:00Z Progress loading mempool transactions from file: 30% (tried 358, 834 remaining)
 92024-04-25T13:03:00Z [net] trying v2 connection 69.112.103.124 lastseen=0.0hrs
102024-04-25T13:03:00Z [net] received: version (102 bytes) peer=0
112024-04-25T13:03:00Z [net] sending wtxidrelay (0 bytes) peer=0
122024-04-25T13:03:00Z [net] sending sendaddrv2 (0 bytes) peer=0
132024-04-25T13:03:00Z [net] sending verack (0 bytes) peer=0
142024-04-25T13:03:00Z [net] Added connection peer=1
152024-04-25T13:03:00Z [net] sending version (103 bytes) peer=1
162024-04-25T13:03:00Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=1
172024-04-25T13:03:00Z [net] sending getaddr (0 bytes) peer=0
182024-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
192024-04-25T13:03:00Z [net] added time data, samples 2, offset +0 (+0 minutes)
202024-04-25T13:03:00Z [net] start sending v2 handshake to peer=1
212024-04-25T13:03:00Z [net] received: wtxidrelay (0 bytes) peer=0
222024-04-25T13:03:00Z [net] received: sendaddrv2 (0 bytes) peer=0
232024-04-25T13:03:00Z [net] received: verack (0 bytes) peer=0
242024-04-25T13:03:00Z New addr-fetch v2 peer connected: version: 70016, blocks=840823, peer=0
252024-04-25T13:03:00Z [net] sending sendcmpct (9 bytes) peer=0
262024-04-25T13:03:00Z [net] sending ping (8 bytes) peer=0
272024-04-25T13:03:00Z [net] Advertising address [2804:14c:3bfb:37:e55d:6a9c:bb51:8079]:8333 to peer=0
282024-04-25T13:03:00Z [net] sending addrv2 (28 bytes) peer=0
292024-04-25T13:03:00Z [net] sending getheaders (1029 bytes) peer=0
302024-04-25T13:03:00Z [net] initial getheaders (840821) to peer=0 (startheight:840823)
312024-04-25T13:03:00Z [net] received: sendcmpct (9 bytes) peer=0
322024-04-25T13:03:00Z [net] received: ping (8 bytes) peer=0
332024-04-25T13:03:00Z [net] sending pong (8 bytes) peer=0
342024-04-25T13:03:00Z [net] received: getheaders (1029 bytes) peer=0
352024-04-25T13:03:00Z [net] getheaders -1 to end from peer=0
362024-04-25T13:03:00Z [net] sending headers (1 bytes) peer=0
372024-04-25T13:03:00Z [net] received: feefilter (8 bytes) peer=0
382024-04-25T13:03:00Z [net] received: feefilter of 0.00004924 BTC/kvB from peer=0
392024-04-25T13:03:00Z [net] socket closed for peer=1
402024-04-25T13:03:00Z [net] disconnecting peer=1
412024-04-25T13:03:00Z [net] retrying with v1 transport protocol for peer=1
42024-04-25T13:03:01Z [net] Cleared nodestate for peer=1
432024-04-25T13:03:01Z Progress loading mempool transactions from file: 40% (tried 477, 715 remaining)
442024-04-25T13:03:01Z Progress loading mempool transactions from file: 50% (tried 596, 596 remaining)
452024-04-25T13:03:01Z Progress loading mempool transactions from file: 60% (tried 716, 476 remaining)
462024-04-25T13:03:01Z [net] trying v1 connection 69.112.103.124 lastseen=0.0hrs
472024-04-25T13:03:01Z [net] Added connection peer=2
482024-04-25T13:03:01Z [net] received: addrv2 (17295 bytes) peer=0
492024-04-25T13:03:01Z [net] sending version (103 bytes) peer=2
502024-04-25T13:03:01Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=2
512024-04-25T13:03:01Z [net] trying v2 connection 27.124.108.19 lastseen=0.0hrs
522024-04-25T13:03:01Z [net] Received addr: 1000 addresses (1000 processed, 0 rate-limited) from peer=0
532024-04-25T13:03:01Z [net] addrfetch connection completed peer=0; disconnecting
542024-04-25T13:03:01Z [net] disconnecting peer=0
552024-04-25T13:03:01Z [net] Cleared nodestate for peer=0
562024-04-25T13:03:01Z Progress loading mempool transactions from file: 70% (tried 835, 357 remaining)
572024-04-25T13:03:01Z Progress loading mempool transactions from file: 80% (tried 954, 238 remaining)
582024-04-25T13:03:01Z Progress loading mempool transactions from file: 90% (tried 1073, 119 remaining)
592024-04-25T13:03:01Z Imported mempool transactions from file: 1192 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
602024-04-25T13:03:01Z initload thread exit
612024-04-25T13:03:01Z [net] socket closed for peer=2
622024-04-25T13:03:01Z [net] disconnecting peer=2
632024-04-25T13:03:01Z [net] Cleared nodestate for peer=2
642024-04-25T13:03:02Z [net] Added connection peer=3
652024-04-25T13:03:02Z [net] sending version (103 bytes) peer=3
662024-04-25T13:03:02Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=3
672024-04-25T13:03:02Z [net] start sending v2 handshake to peer=3
682024-04-25T13:03:02Z [net] socket closed for peer=3
692024-04-25T13:03:02Z [net] disconnecting peer=3
702024-04-25T13:03:02Z [net] retrying with v1 transport protocol for peer=3
712024-04-25T13:03:02Z [net] Cleared nodestate for peer=3
722024-04-25T13:03:02Z [net] trying v1 connection 27.124.108.19 lastseen=0.0hrs
732024-04-25T13:03:02Z [net] Added connection peer=4
742024-04-25T13:03:02Z [net] sending version (103 bytes) peer=4
752024-04-25T13:03:02Z [net] send version message: version 70016, blocks=840822, txrelay=0, peer=4
762024-04-25T13:03:02Z [net] trying v1 connection [2804:14c:7d87:88bd::1003]:8333 lastseen=317.1hrs
772024-04-25T13:03:03Z [net] received: version (102 bytes) peer=4
782024-04-25T13:03:03Z [net] sending wtxidrelay (0 bytes) peer=4
792024-04-25T13:03:03Z [net] sending sendaddrv2 (0 bytes) peer=4
802024-04-25T13:03:03Z [net] sending verack (0 bytes) peer=4
812024-04-25T13:03:03Z [net] sending getaddr (0 bytes) peer=4
822024-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
832024-04-25T13:03:03Z [net] added time data, samples 3, offset +0 (+0 minutes)
842024-04-25T13:03:03Z [net] received: wtxidrelay (0 bytes) peer=4
852024-04-25T13:03:03Z [net] received: sendaddrv2 (0 bytes) peer=4
862024-04-25T13:03:03Z [net] received: verack (0 bytes) peer=4
872024-04-25T13:03:03Z New addr-fetch v1 peer connected: version: 70016, blocks=840823, peer=4
88...

brunoerg approved

brunoerg commented at 5:35 pm on April 25, 2024: contributor

ACK d8df9f94f933c9d5fd19a58a4b1473ea9becd362

DrahtBot added the label CI failed on Apr 27, 2024

DrahtBot added the label Needs rebase on May 1, 2024

mzumsande force-pushed on May 1, 2024

mzumsande commented at 4:32 pm on May 1, 2024: contributor

d8df9f9 to 6430bea: Rebased

DrahtBot removed the label Needs rebase on May 1, 2024

mzumsande force-pushed on May 1, 2024

DrahtBot removed the label CI failed on May 1, 2024

in src/net.cpp:2327 in c1be7952cf outdated

2320@@ -2321,13 +2321,78 @@ void CConnman::QueryDNSSeeds()
2321     }
2322 }
2323 
2324+void CConnman::ProcessFixedSeeds(std::chrono::microseconds start)
2325+{
2326+    const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
2327+    bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);

sr-gi commented at 5:27 pm on May 13, 2024:

In c1be7952cff9669b87e5002dcbc990dbf726c06f

Isn’t add_fixed_seeds unconditionally true now? This is called only by:

0if (gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS)) {
1    ProcessFixedSeeds(start_time);    
2}

mzumsande commented at 4:55 pm on May 24, 2024:

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

in src/net.cpp:2327 in c1be7952cf outdated

2320@@ -2321,13 +2321,78 @@ void CConnman::QueryDNSSeeds()
2321     }
2322 }
2323 
2324+void CConnman::ProcessFixedSeeds(std::chrono::microseconds start)
2325+{
2326+    const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);

sr-gi commented at 5:28 pm on May 13, 2024:

In c1be7952cff9669b87e5002dcbc990dbf726c06f

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.

mzumsande commented at 4:58 pm on May 24, 2024:

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.

in src/net.cpp:2328 in c1be7952cf outdated

2320@@ -2321,13 +2321,78 @@ void CConnman::QueryDNSSeeds()
2321     }
2322 }
2323 
2324+void CConnman::ProcessFixedSeeds(std::chrono::microseconds start)
2325+{
2326+    const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
2327+    bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);
2328+    const bool use_seednodes{gArgs.IsArgSet("-seednode")};

sr-gi commented at 5:44 pm on May 13, 2024:

In c1be7952cff9669b87e5002dcbc990dbf726c06f

This can also be passed as a param. We are parsing it both here and in CConnman::QueryDNSSeeds()

mzumsande commented at 4:58 pm on May 24, 2024:

see previous comment

in src/net.cpp:2404 in c1be7952cf outdated

2379+    }
2380+}
2381+
2382 void CConnman::ThreadAddressSeed()
2383 {
2384+    auto start_time = GetTime<std::chrono::microseconds>();

sr-gi commented at 5:48 pm on May 13, 2024:

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.

mzumsande commented at 4:59 pm on May 24, 2024:

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.

sr-gi commented at 6:08 pm on May 13, 2024: member

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

mzumsande commented at 5:08 pm on May 24, 2024: contributor

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 force-pushed on May 24, 2024

mzumsande commented at 5:13 pm on May 24, 2024: contributor

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

DrahtBot added the label Needs rebase on Jul 4, 2024

net: Rename ThreadDNSAddressSeed to ThreadAddressSeed

This is in preparation of adding the fixed seed logic
to this thread.

11a14bc877

net: always start ThreadAddressSeed

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.

6e74c7dbe0

net, refactor: Move DNS seed querying logic into its own function 0e7f457fa6

net: Move fixed seed logic to ThreadAddrSeed

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

d4c956df77

net: remove add_fixed_seeds from ProcessFixedSeeds

Since ProcessFixedSeeds() is only entered if fixed seeds are enabled,
it has become unnecessary.

285038d3c0

net: Make AddrFetch connections to fixed seeds

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.

b7b1a3cb6e

mzumsande force-pushed on Jul 22, 2024

DrahtBot removed the label Needs rebase on Jul 22, 2024

DrahtBot commented at 7:33 pm on September 4, 2024: contributor

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

DrahtBot added the label Needs rebase on Sep 4, 2024

mzumsande commented at 3:25 pm on October 2, 2024: contributor

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 closed this on Oct 2, 2024

net: Make AddrFetch connections to fixed seeds #26114

Code Coverage

Reviews

Conflicts