net: call Select with reachable networks in ThreadOpenConnections #29436

pull brunoerg wants to merge 3 commits into bitcoin:master from brunoerg:2024-02-addrman-select-networks changing 8 files +72 −43
  1. brunoerg commented at 6:49 pm on February 14, 2024: contributor

    This PR changes addrman’s Select to support multiple networks and change ThreadOpenConnections to call it with reachable networks. It can avoid unnecessary Select calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is “less aggresive”. It does not add a new init flag and does not impact address relay.

    I did an experiment of calling Select without passing a network until it finds an address from a network that compose 20% ~ 25% of the addrman (limited to 100 tries).

    Screenshot 2024-02-14 at 14 37 58

  2. DrahtBot commented at 6:49 pm on February 14, 2024: 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
    ACK vasild, naumenkogs, achow101
    Stale ACK Empact

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

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label P2P on Feb 14, 2024
  4. brunoerg commented at 6:52 pm on February 14, 2024: contributor
  5. in src/addrman.h:162 in 2096aeabec outdated
    159      *                     slow down the search.
    160      * @return    CAddress The record for the selected peer.
    161      *            seconds  The last time we attempted to connect to that peer.
    162      */
    163-    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<Network> network = std::nullopt) const;
    164+    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<std::vector<Network>> networks = std::nullopt) const;
    


    amitiuttarwar commented at 5:04 am on February 15, 2024:
    instead of an optional of a vector, you could have the default value explicitly be all networks, or an empty vector be interpreted as all networks. probably the first one is clearer.

    vasild commented at 11:22 am on February 15, 2024:

    With this PR all production code passes both arguments. No need anymore for defaults. This compiles:

    0-    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<std::vector<Network>> networks = std::nullopt) const;
    1+    std::pair<CAddress, NodeSeconds> Select(bool new_only, std::vector<Network> networks) const;
    

    Some tests use Select() and I adjusted them to do Select(false, {}) just to check that the compilation will succeed. They can be adjusted to Select(false, g_reachable_nets.All()) or Select(false, {NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS}) if you decide to drop the defaults.

  6. in src/addrman.cpp:725 in 2096aeabec outdated
    723+    if (networks.has_value()) {
    724+        new_count = 0;
    725+        tried_count = 0;
    726+        for (auto& network : *networks) {
    727+            auto it = m_network_counts.find(network);
    728+            if (it == m_network_counts.end()) return {};
    


    vasild commented at 10:11 am on February 15, 2024:

    This should be continue;? If addrman has 0 addresses from some of the requested networks, it can still return an address from another requested network.

    0            if (it == m_network_counts.end()) {
    1                continue;
    2            }
    

    brunoerg commented at 12:06 pm on February 15, 2024:
    Yes, nice catch. I’ll address it.
  7. in src/addrman.cpp:766 in 2096aeabec outdated
    759@@ -756,9 +760,10 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
    760             position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
    761             node_id = GetEntry(search_tried, bucket, position);
    762             if (node_id != -1) {
    763-                if (network.has_value()) {
    764+                if (networks.has_value()) {
    765                     const auto it{mapInfo.find(node_id)};
    766-                    if (Assume(it != mapInfo.end()) && it->second.GetNetwork() == *network) break;
    767+                    auto& nets = *networks;
    768+                    if (Assume(it != mapInfo.end()) && std::find(nets.begin(), nets.end(), it->second.GetNetwork()) != nets.end()) break;
    


    vasild commented at 10:18 am on February 15, 2024:

    This code is correct, but is doing linear search in the list of networks. For values of up to 6 that is irrelevant, but if it is changed from vector to unordered_set then lookup is also shorter to write, in addition to being O(1):

    0                    if (Assume(it != mapInfo.end()) && networks->contains(it->second.GetNetwork())) break;
    

    brunoerg commented at 1:27 pm on February 15, 2024:
    Nice, I’ll address it.
  8. in src/bench/addrman.cpp:132 in 2096aeabec outdated
    125@@ -126,8 +126,10 @@ static void AddrManSelectByNetwork(benchmark::Bench& bench)
    126 
    127     FillAddrMan(addrman);
    128 
    129+    std::vector<Network> net_i2p = {NET_I2P};
    130+
    131     bench.run([&] {
    132-        (void)addrman.Select(/*new_only=*/false, NET_I2P);
    133+        (void)addrman.Select(/*new_only=*/false, net_i2p);
    


    vasild commented at 10:21 am on February 15, 2024:

    This should work as well (untested):

    0(void)addrman.Select(/*new_only=*/false, {NET_I2P});
    

    Same in src/test/addrman_tests.cpp.


    brunoerg commented at 12:30 pm on February 15, 2024:
    It doesn’t work with vector I guess. But since I’ll change it to unordered_set, it will work.

    vasild commented at 2:18 pm on February 16, 2024:
    Does not matter now, but it should work with vector as well.
  9. in src/net.cpp:2693 in 2096aeabec outdated
    2690                 // ensures that a peer from another network will be evicted.
    2691-                std::tie(addr, addr_last_try) = addrman.Select(false, preferred_net);
    2692+                if (preferred_net.has_value()) {
    2693+                    networks = {*preferred_net};
    2694+                }
    2695+                std::tie(addr, addr_last_try) = addrman.Select(false, networks);
    


    vasild commented at 10:29 am on February 15, 2024:

    This could be shorter (untested):

    0std::tie(addr, addr_last_try) = preferred_net.has_value()
    1    ? addrman.Select(false, {*preferred_net})
    2    : addrman.Select(false, g_reachable_nets.All());
    
  10. in src/test/fuzz/addrman.cpp:295 in 2096aeabec outdated
    282+    Shuffle(all_networks.begin(), all_networks.end(), fast_random_context);
    283+    std::vector<Network> nets;
    284+    for (size_t i = 0; i < all_networks.size() && fuzzed_data_provider.ConsumeBool(); i++) {
    285+        nets.push_back(all_networks[i]);
    286+    }
    287+    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), nets);
    


    vasild commented at 10:39 am on February 15, 2024:

    This could end up passing an empty container (vector or unordered_set if you decide to change it). Maybe this:

    0if (nets.empty()) {
    1    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool());
    2} else {
    3    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), nets);
    4}
    

    brunoerg commented at 1:26 pm on February 15, 2024:
    Empty container and nothing means the same, all networks.
  11. in src/net.cpp:2573 in 2096aeabec outdated
    2568+        for (int i = 0; i != NET_MAX; i++) {
    2569+            Network net = static_cast<Network>(i);
    2570+            if (g_reachable_nets.Contains(net)) {
    2571+                networks.push_back(net);
    2572+            }
    2573+        }
    


    vasild commented at 10:46 am on February 15, 2024:

    If you extend the ReachableNets class like this:

     0--- i/src/netbase.h
     1+++ w/src/netbase.h
     2@@ -102,12 +102,18 @@ public:
     3     [[nodiscard]] bool Contains(const CNetAddr& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     4     {   
     5         AssertLockNotHeld(m_mutex);
     6         return Contains(addr.GetNetwork());
     7     }
     8
     9+    [[nodiscard]] std::unordered_set<Network> All() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
    10+    {
    11+        LOCK(m_mutex);
    12+        return m_reachable;
    13+    }
    14+
    15 private:
    16     mutable Mutex m_mutex;
    17
    18     std::unordered_set<Network> m_reachable GUARDED_BY(m_mutex){
    

    Then you can pass directly g_reachable_nets.All() to Select() without an intermediate variable.


    brunoerg commented at 1:27 pm on February 15, 2024:
    Nice idea! I’ll address it.
  12. vasild commented at 10:52 am on February 15, 2024: contributor

    Approach ACK 2096aeabec288bd2852593f0c965a2a9d22ad0c8

    Some non-blockers below (feel free to ignore them) plus one return vs continue that probably needs to be addressed.

  13. brunoerg force-pushed on Feb 15, 2024
  14. brunoerg commented at 1:45 pm on February 15, 2024: contributor
  15. vasild approved
  16. vasild commented at 2:17 pm on February 16, 2024: contributor

    ACK 7edb07ca800991f7c88d582b880a392efe17f31d

    About the graph in the OP: “… until it finds an address from a network that compose 20% …”. The chance of not getting the result after N tries is (8/10)^N. For example for N=25 that is less than 0.004. What is the explanation of having so many dots so high? Uneven distribution within addrman?

  17. brunoerg commented at 7:45 pm on February 16, 2024: contributor

    About the graph in the OP: “… until it finds an address from a network that compose 20% …”. The chance of not getting the result after N tries is (8/10)^N. For example for N=25 that is less than 0.004. What is the explanation of having so many dots so high? Uneven distribution within addrman?

    That was what I was expecting too. I’ll investigate but yes I think it might be an uneven distribution. Btw, this is not deterministic.

  18. in src/addrman.cpp:731 in 3f99f5a6e5 outdated
    731+            auto it = m_network_counts.find(network);
    732+            if (it == m_network_counts.end()) {
    733+                continue;
    734+            }
    735+            auto counts = it->second;
    736+            new_count += counts.n_new;
    


    naumenkogs commented at 9:02 am on February 19, 2024:

    3f99f5a6e58fae4981228929045ed7be32463381

    Might be worth making this overflow-safe?


    naumenkogs commented at 10:25 am on February 19, 2024:
    Might as well fix if (new_count + tried_count == 0) return {}; above. While it seems like a bug in the existing code, it’s really hard to trigger…. feeding addrman with that much garbage would be a failure probably before the size_t limit is hit :)

    brunoerg commented at 6:10 pm on February 19, 2024:

    Might be worth making this overflow-safe?

    I don’t know, I don’t see a viable way of reaching that.

    it’s really hard to trigger…. feeding addrman with that much garbage would be a failure probably before the size_t limit is hit :)

    I believe that’s impractical due to the limited size of addrman.


    naumenkogs commented at 8:38 am on February 20, 2024:

    I believe that’s impractical due to the limited size of addrman.

    Right, probably fine to keep it this way for both cases.

  19. naumenkogs commented at 8:39 am on February 20, 2024: member
    ACK 7edb07ca800991f7c88d582b880a392efe17f31d
  20. mzumsande commented at 5:48 pm on February 20, 2024: contributor
    I tried to test the performance impact on the “normal” case of an addrman that only has addrs for reachable nets: If I change the AddrManSelect benchmark to use addrman.Select(false, g_reachable_nets.All()); the performance goes down by a factor of ~5 for me (from ~160ns/op to ~770 ns/op). Do you see a similar effect?
  21. in src/addrman.h:163 in 3f99f5a6e5 outdated
    160      *                     slow down the search.
    161      * @return    CAddress The record for the selected peer.
    162      *            seconds  The last time we attempted to connect to that peer.
    163      */
    164-    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<Network> network = std::nullopt) const;
    165+    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::unordered_set<Network> networks = {}) const;
    


    mzumsande commented at 6:07 pm on February 20, 2024:
    Looks like the case of empty networks is no longer used outside of tests after this PR. In that case, should we drop the default args and simplify the implementation?

    brunoerg commented at 1:59 pm on February 22, 2024:
    Yes, sure! I’ll address it.
  22. in src/test/addrman_tests.cpp:282 in 7edb07ca80 outdated
    278@@ -275,7 +279,8 @@ BOOST_AUTO_TEST_CASE(addrman_select_special)
    279     // since the only ipv4 address is on the new table, ensure that the new
    280     // table gets selected even if new_only is false. if the table was being
    281     // selected at random, this test will sporadically fail
    282-    BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_IPV4).first == addr1);
    283+    std::unordered_set<Network> {NET_IPV4} = {NET_IPV4};
    


    luke-jr commented at 3:02 pm on February 21, 2024:
    What is this supposed to do? :/

    brunoerg commented at 1:48 pm on February 22, 2024:
    Leftover from previous changes, thanks, I’ll remove it.
  23. mzumsande commented at 4:09 pm on February 22, 2024: contributor

    the performance goes down by a factor of ~5 for me

    If this is correct and can be reproduced by others, I think that something should be done about it. If this PR would increase performance for a rare and transient case (right after the users switched their onlynet parameters) but decrease performance for each Select() call in the typical scenario (addrman has mostly addrs from reachable networks), the overall impact on performance of this PR would be clearly negative. One possibility might be to track the percentage of non-reachable addrs while loading AddrMan (we can’t get more unreachable addrs afterwards) and only execute the slower logic if that is larger than some threshold (for example, 50%).

  24. brunoerg commented at 4:46 pm on February 22, 2024: contributor

    If this is correct and can be reproduced by others, I think that something should be done about it. If this PR would increase performance for a rare and transient case (right after the users switched their onlynet parameters) but decrease performance for each Select() call in the typical scenario (addrman has mostly addrs from reachable networks), the overall impact on performance of this PR would be clearly negative. One possibility might be to track the percentage of non-reachable addrs while loading AddrMan (we can’t get more unreachable addrs afterwards) and only execute the slower logic if that is larger than some threshold (for example, 50%).

    I’m reproducing it to check it.

  25. brunoerg force-pushed on Feb 26, 2024
  26. brunoerg force-pushed on Feb 26, 2024
  27. DrahtBot commented at 11:02 am on February 26, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/21977629586

  28. DrahtBot added the label CI failed on Feb 26, 2024
  29. brunoerg commented at 11:06 am on February 26, 2024: contributor

    @mzumsande you’re right!

    I checked the performance goes down when calling Select when addrman only has addrs for reachable nets. So, I changed the approach to call Select with networks only whether the number of addresses from reachable networks in the addrman is less than 50%. This should be enough to cover the cases where this is an issue.

    So, when initializing CConnman, we check whether the number of addresses from reachable networks in the addrman is less than 50%, if so, we call Select passing the networks.

    cc: @vasild @naumenkogs (due previous ACK)

  30. DrahtBot removed the label CI failed on Feb 26, 2024
  31. in src/addrman.cpp:1041 in 7b4b9535a7 outdated
    1036+        const auto& net{item.first};
    1037+        const auto& count{item.second};
    1038+
    1039+        total_size += (count.n_new + count.n_tried);
    1040+
    1041+        if (networks.count(net)) {
    


    Empact commented at 5:31 am on February 27, 2024:
    nit: contains is equivalent but more semantically correct here
  32. in src/net.cpp:3293 in 7b4b9535a7 outdated
    3287@@ -3285,6 +3288,12 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
    3288         scheduler.scheduleEvery([this] { ASMapHealthCheck(); }, ASMAP_HEALTH_CHECK_INTERVAL);
    3289     }
    3290 
    3291+    auto reachable_nets{g_reachable_nets.All()};
    3292+    const bool has_few_addresses{addrman.HasFewReachableAddresses(reachable_nets)};
    3293+    if (has_few_addresses) {
    


    Empact commented at 5:45 am on February 27, 2024:
    nit: could inline these
  33. in src/addrman.cpp:1046 in 7b4b9535a7 outdated
    1041+        if (networks.count(net)) {
    1042+            reachable_size += (count.n_new + count.n_tried);
    1043+        }
    1044+    }
    1045+
    1046+    if (reachable_size < total_size / 2) {
    


    Empact commented at 5:50 am on February 27, 2024:
    nit: could return this directly
  34. in src/net.h:1400 in 7b4b9535a7 outdated
    1395@@ -1396,6 +1396,9 @@ class CConnman
    1396     // P2P timeout in seconds
    1397     std::chrono::seconds m_peer_connect_timeout;
    1398 
    1399+    // Reachable networks that have addresses in the addrman
    1400+    std::unordered_set<Network> networks_with_addrs;
    


    Empact commented at 5:56 am on February 27, 2024:
  35. in src/addrman.cpp:715 in 7b4b9535a7 outdated
    711@@ -712,7 +712,7 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds
    712     }
    713 }
    714 
    715-std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::optional<Network> network) const
    716+std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::unordered_set<Network> networks) const
    


    Empact commented at 6:06 am on February 27, 2024:
    networks could be a const reference throughout, no?

    brunoerg commented at 12:09 pm on February 27, 2024:
    Yes, I’ll address it.
  36. in src/net.cpp:3294 in 7b4b9535a7 outdated
    3287@@ -3285,6 +3288,12 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
    3288         scheduler.scheduleEvery([this] { ASMapHealthCheck(); }, ASMAP_HEALTH_CHECK_INTERVAL);
    3289     }
    3290 
    3291+    auto reachable_nets{g_reachable_nets.All()};
    3292+    const bool has_few_addresses{addrman.HasFewReachableAddresses(reachable_nets)};
    3293+    if (has_few_addresses) {
    3294+        networks_with_addrs = reachable_nets;
    


    Empact commented at 6:32 am on February 27, 2024:

    So, to be explicit, this uses g_reachable_nets if reachable_size < total_size / 2 otherwise all nets. The switch is to avoid the cost of the filtering if the filtering doesn’t much reduce the set size, yes?

    A comment to that effect might be helpful, as I had to puzzle out a bit why the value was set only in this case.


    brunoerg commented at 11:07 am on February 27, 2024:

    The switch is to avoid the cost of the filtering if the filtering doesn’t much reduce the set size, yes?

    Yes, I’ll put a comment explaining that.

  37. in src/addrman.cpp:1019 in 44aba6cbff outdated
    1015@@ -1016,6 +1016,35 @@ std::optional<AddressPosition> AddrManImpl::FindAddressEntry_(const CAddress& ad
    1016     }
    1017 }
    1018 
    1019+bool AddrManImpl::HasFewReachableAddresses_(const std::unordered_set<Network>& networks)
    


    naumenkogs commented at 10:13 am on February 27, 2024:
    An outside node easily influences the outcome by feeding garbage, right? Then I’d suggest commenting along the following lines: “Should not be used for anything critical since it’s easily […]”

    naumenkogs commented at 10:14 am on February 27, 2024:
    Another improvement could be differentiating new/tried here, since tried is harder to influence.

    brunoerg commented at 11:34 am on February 27, 2024:

    An outside node easily influences the outcome by feeding garbage, right?

    I don’t think so, since we’re checking by network and we do not store addresses from unreachable networks.


    brunoerg commented at 11:35 am on February 27, 2024:

    Another improvement could be differentiating new/tried here, since tried is harder to influence.

    Sure!

  38. brunoerg force-pushed on Feb 27, 2024
  39. brunoerg commented at 5:31 pm on February 27, 2024: contributor

    Force-pushed addressing: #29436 (review) #29436 (review) #29436 (review) #29436 (review) #29436 (review) #29436 (review) #29436 (review)

    I changed the approach to know verify new and tried table isolated, so if there is less than 50% of reachable addresses than we filter the selection (selecting from new table will consider data from new table only, for example).

    Thanks, @empact and @naumenkogs.

  40. in src/net.h:1404 in ef1cd394bb outdated
    1395@@ -1396,6 +1396,13 @@ class CConnman
    1396     // P2P timeout in seconds
    1397     std::chrono::seconds m_peer_connect_timeout;
    1398 
    1399+    /**
    1400+     * Reachable networks that have addresses in the addrman (new, tried). It is filled when there
    1401+     * are few reachable addresses in addrman and used to filter the addrman's selection.
    1402+     * Otherwise, we avoid the cost of the filtering.
    1403+     */
    1404+    std::pair<std::unordered_set<Network>, std::unordered_set<Network>>  m_networks_with_addrs;
    


    Empact commented at 6:08 pm on February 27, 2024:

    nit: IMO the code will be more self-documenting if you use 2 separate variables for new vs tried, rather than a pair here, because to make sense of the pair you have to refer back to the definition, while new vs tried in the variable name will be immediately apparent.

    Could still return a pair from NetworksWithAddresses, and unpack it for assignment.


    brunoerg commented at 6:25 pm on February 27, 2024:
    Sounds good, I’ll address it.
  41. brunoerg force-pushed on Feb 27, 2024
  42. brunoerg commented at 6:25 pm on February 27, 2024: contributor
    Force-pushed addressing #29436 (review).
  43. Empact approved
  44. DrahtBot requested review from vasild on Feb 27, 2024
  45. DrahtBot requested review from naumenkogs on Feb 27, 2024
  46. in src/addrman.cpp:713 in b85893ca02 outdated
    711@@ -712,7 +712,7 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds
    712     }
    713 }
    714 
    715-std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::optional<Network> network) const
    716+std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, const std::unordered_set<Network>& networks) const
    


    ajtowns commented at 2:26 am on February 29, 2024:

    Having an unordered_set to choose a subset of 7 values from an enum seems a bit overkill. Doing:

    0    std::bitset<NET_MAX> network_bitset;
    1    for (net : networks) { if (0 <= net && net < NET_MAX) network_bitset.set(net); }
    2
    3    ...
    4
    5    if (network_bitset.test(it->second.GetNetwork())) { /* this is one of the networks i wanted!! */ }
    

    seems like it would be better. The network_bitset value could be set as part of initializing addrman, and the test should be fast enough to do it always, I think?


    vasild commented at 2:41 pm on February 29, 2024:
    unordered_set is the most semantically sound container for this. It is also the most convenient and easy to use. For the same reasons we use std::string to store fixed strings or string literals. IMO a deviation from that with a more verbose alternative is justified only if it provides a noticeable performance improvement.

    brunoerg commented at 12:13 pm on March 4, 2024:
    I’ll leave as it for now, but free to change if other reviewers agree.
  47. in src/addrman.cpp:722 in b85893ca02 outdated
    726-        if (it == m_network_counts.end()) return {};
    727-
    728-        auto counts = it->second;
    729-        new_count = counts.n_new;
    730-        tried_count = counts.n_tried;
    731+    if (!networks.empty()) {
    


    vasild commented at 5:36 am on March 4, 2024:

    I am trying to understand better the performance of this. Posting here to avoid cluttering the PR’s main discussion thread.

    #29436#pullrequestreview-1891096814:

    I tried to test the performance impact on the “normal” case of an addrman that only has addrs for reachable nets: If I change the AddrManSelect benchmark to use addrman.Select(false, g_reachable_nets.All()); the performance goes down by a factor of ~5 for me

    One thing to note here is that the benchmark fills addrman solely with IPv6 addresses. That’s not representative. So I borrowed the peers.dat from my mainnet node and ran the tests below.

    As a start I began from 7edb07ca80 (this PR before the performance related tweaks). My aim was to 1. confirm that there is indeed a performance issue and 2. to understand where exactly it comes from so it can be addressed in the best way. Then I compared with the PR and then I compared with the PR + a change to make the last argument of Select() const ref.

    addrman size:

    0IPV4: 49123
    1IPV6: 11071
    2ONION:15096
    3I2P:   1863
    4CJDNS:    2
    

    Numbers in ns/op (bigger number => slower).

    \ Baseline (master @ 7edb07ca80~3) PR (7edb07ca80) PR (7edb07ca80) + const ref argument
    any (empty optional or empty map) 130 140 A 135
    IPV4 or {IPV4} 190 320 200
    IPV6 or {IPV6} 490 640 510
    ONION or {ONION} 1200 1370 1270
    I2P or {I2P} 2500 2650 2550
    CJDNS or {CJDNS} 1500000 1700000 1600000
    {IPV4, IPV6, ONION, I2P, CJDNS} - 550 B 150 C

    A vs B is probably what @mzumsande observed in #29436#pullrequestreview-1891096814. But fixing the arg changes that to A vs C.

    Based on the above, I think that it is best to go back to 7edb07ca80 and only fix the Select() argument.

     0diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
     1index f044feebba..dd45b9e69b 100644
     2--- a/src/bench/addrman.cpp
     3+++ b/src/bench/addrman.cpp
     4@@ -1,12 +1,15 @@
     5 // Copyright (c) 2020-2022 The Bitcoin Core developers
     6 // Distributed under the MIT software license, see the accompanying
     7 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
     8 
     9+#include <addrdb.h>
    10 #include <addrman.h>
    11 #include <bench/bench.h>
    12+#include <chainparams.h>
    13+#include <common/args.h>
    14 #include <netbase.h>
    15 #include <netgroup.h>
    16 #include <random.h>
    17 #include <util/check.h>
    18 #include <util/time.h>
    19 
    20@@ -81,12 +84,37 @@ static void AddrManAdd(benchmark::Bench& bench)
    21     bench.run([&] {
    22         AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
    23         AddAddressesToAddrMan(addrman);
    24     });
    25 }
    26 
    27+static void AddrManSelectMain(benchmark::Bench& bench)
    28+{
    29+    SelectParams(ChainType::MAIN);
    30+    gArgs.ForceSetArg("checkaddrman", "0");
    31+    gArgs.ForceSetArg("datadir", "/tmp");
    32+    auto result = LoadAddrman(EMPTY_NETGROUPMAN, gArgs);
    33+    AddrMan& addrman = *(*result).get();
    34+
    35+    std::cout << "addrman size:\n";
    36+    std::cout << "IPv4: " << addrman.Size(NET_IPV4) << "\n";
    37+    std::cout << "IPv6: " << addrman.Size(NET_IPV6) << "\n";
    38+    std::cout << "Tor:  " << addrman.Size(NET_ONION) << "\n";
    39+    std::cout << "I2P:  " << addrman.Size(NET_I2P) << "\n";
    40+    std::cout << "CJDNS:" << addrman.Size(NET_CJDNS) << "\n";
    41+
    42+    //const std::optional<Network> n{NET_CJDNS};
    43+    //const std::unordered_set<Network> n{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS};
    44+    //const std::unordered_set<Network> n{};
    45+    //const std::unordered_set<Network> n{g_reachable_nets.All()};
    46+    bench.run([&] {
    47+        const auto& address = addrman.Select(/*new_only=*/false, n);
    48+        assert(address.first.IsValid());
    49+    });
    50+}
    51+
    52 static void AddrManSelect(benchmark::Bench& bench)
    53 {
    54     AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
    55 
    56     FillAddrMan(addrman);
    57 
    58@@ -167,11 +195,12 @@ static void AddrManAddThenGood(benchmark::Bench& bench)
    59 
    60         markSomeAsGood(addrman);
    61     });
    62 }
    63 
    64 BENCHMARK(AddrManAdd, benchmark::PriorityLevel::HIGH);
    65+BENCHMARK(AddrManSelectMain, benchmark::PriorityLevel::HIGH);
    66 BENCHMARK(AddrManSelect, benchmark::PriorityLevel::HIGH);
    67 BENCHMARK(AddrManSelectFromAlmostEmpty, benchmark::PriorityLevel::HIGH);
    68 BENCHMARK(AddrManSelectByNetwork, benchmark::PriorityLevel::HIGH);
    69 BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH);
    70 BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::HIGH);
    
      0diff --git i/src/addrman.cpp w/src/addrman.cpp
      1index 4e551e81c3..bde2d90ef2 100644
      2--- i/src/addrman.cpp
      3+++ w/src/addrman.cpp
      4@@ -705,13 +705,13 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds
      5     if (fCountFailure && info.m_last_count_attempt < m_last_good) {
      6         info.m_last_count_attempt = time;
      7         info.nAttempts++;
      8     }
      9 }
     10 
     11-std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::unordered_set<Network> networks) const
     12+std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, const std::unordered_set<Network>& networks) const
     13 {
     14     AssertLockHeld(cs);
     15 
     16     if (vRandom.empty()) return {};
     17 
     18     size_t new_count = nNew;
     19@@ -1208,13 +1208,13 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision()
     20     Check();
     21     auto ret = SelectTriedCollision_();
     22     Check();
     23     return ret;
     24 }
     25 
     26-std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::unordered_set<Network> networks) const
     27+std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, const std::unordered_set<Network>& networks) const
     28 {
     29     LOCK(cs);
     30     Check();
     31     auto addrRet = Select_(new_only, networks);
     32     Check();
     33     return addrRet;
     34@@ -1315,13 +1315,13 @@ void AddrMan::ResolveCollisions()
     35 
     36 std::pair<CAddress, NodeSeconds> AddrMan::SelectTriedCollision()
     37 {
     38     return m_impl->SelectTriedCollision();
     39 }
     40 
     41-std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, std::unordered_set<Network> networks) const
     42+std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, const std::unordered_set<Network>& networks) const
     43 {
     44     return m_impl->Select(new_only, networks);
     45 }
     46 
     47 std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
     48 {
     49diff --git i/src/addrman.h w/src/addrman.h
     50index 2d2846993a..b6fdb07f82 100644
     51--- i/src/addrman.h
     52+++ w/src/addrman.h
     53@@ -157,13 +157,13 @@ public:
     54      *                     guarantee a tried entry).
     55      * [@param](/bitcoin-bitcoin/contributor/param/)[in] networks Select only addresses of these networks (empty = all). Passing networks may
     56      *                     slow down the search.
     57      * [@return](/bitcoin-bitcoin/contributor/return/)    CAddress The record for the selected peer.
     58      *            seconds  The last time we attempted to connect to that peer.
     59      */
     60-    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::unordered_set<Network> networks = {}) const;
     61+    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, const std::unordered_set<Network>& networks = {}) const;
     62 
     63     /**
     64      * Return all or many randomly selected addresses, optionally by network.
     65      *
     66      * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses  Maximum number of addresses to return (0 = all).
     67      * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct        Maximum percentage of addresses to return (0 = all).
     68diff --git i/src/addrman_impl.h w/src/addrman_impl.h
     69index bb89213bb7..2a4596e81f 100644
     70--- i/src/addrman_impl.h
     71+++ w/src/addrman_impl.h
     72@@ -123,13 +123,13 @@ public:
     73         EXCLUSIVE_LOCKS_REQUIRED(!cs);
     74 
     75     void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!cs);
     76 
     77     std::pair<CAddress, NodeSeconds> SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);
     78 
     79-    std::pair<CAddress, NodeSeconds> Select(bool new_only, std::unordered_set<Network> networks) const
     80+    std::pair<CAddress, NodeSeconds> Select(bool new_only, const std::unordered_set<Network>& networks) const
     81         EXCLUSIVE_LOCKS_REQUIRED(!cs);
     82 
     83     std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const
     84         EXCLUSIVE_LOCKS_REQUIRED(!cs);
     85 
     86     std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const
     87@@ -250,13 +250,13 @@ private:
     88     bool Good_(const CService& addr, bool test_before_evict, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
     89 
     90     bool Add_(const std::vector<CAddress>& vAddr, const CNetAddr& source, std::chrono::seconds time_penalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
     91 
     92     void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
     93 
     94-    std::pair<CAddress, NodeSeconds> Select_(bool new_only, std::unordered_set<Network> networks) const EXCLUSIVE_LOCKS_REQUIRED(cs);
     95+    std::pair<CAddress, NodeSeconds> Select_(bool new_only, const std::unordered_set<Network>& networks) const EXCLUSIVE_LOCKS_REQUIRED(cs);
     96 
     97     /** Helper to generalize looking up an addrman entry from either table.
     98      *
     99      * [@return](/bitcoin-bitcoin/contributor/return/)  int The nid of the entry. If the addrman position is empty or not found, returns -1.
    100      * */
    101     int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
    

    brunoerg commented at 2:24 pm on March 4, 2024:
    Thanks for it, @vasild

    brunoerg commented at 2:24 pm on March 4, 2024:

    About:

    I tried to test the performance impact on the “normal” case of an addrman that only has addrs for reachable nets: If I change the AddrManSelect benchmark to use addrman.Select(false, g_reachable_nets.All()); the performance goes down by a factor of ~5 for me (from ~160ns/op to ~770 ns/op). Do you see a similar effect?

    In fact, I just realized that the performance issue is related to g_reachable_nets.All(). With the following diff I realized the performance is not relevantly affected. cc: @mzumsande

     0diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
     1index ee4070b07a..8760f134ca 100644
     2--- a/src/bench/addrman.cpp
     3+++ b/src/bench/addrman.cpp
     4@@ -96,6 +96,31 @@ static void AddrManSelect(benchmark::Bench& bench)
     5     });
     6 }
     7 
     8+static void AddrManSelect1(benchmark::Bench& bench)
     9+{
    10+    AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
    11+
    12+    FillAddrMan(addrman);
    13+    const auto nets{g_reachable_nets.All()};
    14+
    15+    bench.run([&] {
    16+        const auto& address = addrman.Select(false, nets);
    17+        assert(address.first.GetPort() > 0);
    18+    });
    19+}
    20+
    21+static void AddrManSelect2(benchmark::Bench& bench)
    22+{
    23+    AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
    24+
    25+    FillAddrMan(addrman);
    26+
    27+    bench.run([&] {
    28+        const auto& address = addrman.Select(false, g_reachable_nets.All());
    29+        assert(address.first.GetPort() > 0);
    30+    });
    31+}
    32+
    33 // The worst case performance of the Select() function is when there is only
    34 // one address on the table, because it linearly searches every position of
    35 // several buckets before identifying the correct bucket
    36@@ -171,6 +196,8 @@ static void AddrManAddThenGood(benchmark::Bench& bench)
    37 
    38 BENCHMARK(AddrManAdd, benchmark::PriorityLevel::HIGH);
    39 BENCHMARK(AddrManSelect, benchmark::PriorityLevel::HIGH);
    40+BENCHMARK(AddrManSelect1, benchmark::PriorityLevel::HIGH);
    41+BENCHMARK(AddrManSelect2, benchmark::PriorityLevel::HIGH);
    42 BENCHMARK(AddrManSelectFromAlmostEmpty, benchmark::PriorityLevel::HIGH);
    43 BENCHMARK(AddrManSelectByNetwork, benchmark::PriorityLevel::HIGH);
    44 BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH);
    
    0|               84.73 |       11,801,666.44 |    5.2% |      0.01 | :wavy_dash: `AddrManSelect` (Unstable with ~11,349.7 iters. Increase `minEpochIterations` to e.g. 113497)
    1|              110.53 |        9,047,580.50 |    3.0% |      0.01 | `AddrManSelect1`
    2|              558.40 |        1,790,829.47 |    6.7% |      0.01 | :wavy_dash: `AddrManSelect2` (Unstable with ~1,690.3 iters. Increase `minEpochIterations` to e.g. 16903)
    

    brunoerg commented at 9:43 am on March 5, 2024:

    Based on the above, I think that it is best to go back to 7edb07ca80 and only fix the Select() argument.

    Since it is simpler and does not impact the performance, I just addressed this suggestion. Thanks.

  48. DrahtBot requested review from vasild on Mar 4, 2024
  49. brunoerg force-pushed on Mar 5, 2024
  50. brunoerg commented at 9:42 am on March 5, 2024: contributor

    Force-pushed simplifying the approach. I changed the last argument of Select() to be a const ref and now we just call Select with reachable networks since it does not significantly impact the performance (see #29436 (review)).

    Thanks @vasild for the benchmarking and review.

  51. vasild approved
  52. vasild commented at 1:21 pm on March 6, 2024: contributor
    ACK e66b32b2f49b71b37f48191afb761538a52e8f08
  53. DrahtBot requested review from Empact on Mar 6, 2024
  54. DrahtBot commented at 0:30 am on July 14, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/22286694905

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  55. DrahtBot added the label CI failed on Jul 14, 2024
  56. brunoerg force-pushed on Jul 15, 2024
  57. brunoerg commented at 1:58 pm on July 15, 2024: contributor
    Rebased
  58. brunoerg commented at 3:06 pm on July 15, 2024: contributor
    CI failure is unrelated.
  59. DrahtBot removed the label CI failed on Jul 15, 2024
  60. brunoerg force-pushed on Jul 16, 2024
  61. vasild approved
  62. vasild commented at 9:16 am on July 24, 2024: contributor
    ACK ef525de104ef6116b4532801ef3fcbc1ad5e5552
  63. DrahtBot added the label Needs rebase on Sep 4, 2024
  64. brunoerg force-pushed on Sep 5, 2024
  65. brunoerg commented at 3:26 pm on September 5, 2024: contributor
    Rebased
  66. DrahtBot removed the label Needs rebase on Sep 5, 2024
  67. brunoerg requested review from vasild on Sep 6, 2024
  68. vasild approved
  69. vasild commented at 7:39 am on September 9, 2024: contributor
    ACK f1704c021e615f740130fff7cd47093d708a3028
  70. in src/net.cpp:2742 in 8a82ab9840 outdated
    2738@@ -2739,7 +2739,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, Spa
    2739                 // If preferred_net has a value set, pick an extra outbound
    2740                 // peer from that network. The eviction logic in net_processing
    2741                 // ensures that a peer from another network will be evicted.
    2742-                std::tie(addr, addr_last_try) = addrman.Select(false, preferred_net);
    2743+                std::unordered_set<Network> preferred_nets;
    


    naumenkogs commented at 1:06 pm on September 10, 2024:

    8a82ab9840a46f6382fca92b4f6bb60e5fc5ac6d

    nit Deserves commenting what if preferred_net is not set (Well I guess there is already a comment “empty = all”).

  71. naumenkogs approved
  72. naumenkogs commented at 1:09 pm on September 10, 2024: member
    ACK f1704c021e615f740130fff7cd47093d708a3028
  73. in src/test/fuzz/addrman.cpp:289 in f1704c021e outdated
    284@@ -285,7 +285,16 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
    285     auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
    286     auto filtered = fuzzed_data_provider.ConsumeBool();
    287     (void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered);
    288-    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
    289+
    290+    auto all_networks = ALL_NETWORKS;
    291+    FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)};
    292+    std::shuffle(all_networks.begin(), all_networks.end(), fast_random_context);
    293+    std::unordered_set<Network> nets;
    


    maflcko commented at 1:43 pm on September 10, 2024:
    Is there a reason why the entries are shuffled before inserting them into an unordered set? if there is a reason, it seems that shuffling of a handful of values can be done more efficiently than consuming a full 256 bits.

    brunoerg commented at 3:01 pm on September 10, 2024:
    Nevermind, I just force-pushed a better solution. No need to shuffle anymore.
  74. net: add `All()` in `ReachableNets`
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    f698636ec8
  75. brunoerg force-pushed on Sep 10, 2024
  76. brunoerg commented at 3:02 pm on September 10, 2024: contributor
    Force-pushed with a small improvement on fuzz target (https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1752007850)
  77. in src/test/fuzz/addrman.cpp:291 in be41baf0b7 outdated
    284@@ -285,7 +285,14 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
    285     auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
    286     auto filtered = fuzzed_data_provider.ConsumeBool();
    287     (void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered);
    288-    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
    289+
    290+    std::unordered_set<Network> nets;
    291+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), ALL_NETWORKS.size()) {
    292+        auto net{fuzzed_data_provider.PickValueInArray(ALL_NETWORKS)};
    


    maflcko commented at 3:34 pm on September 10, 2024:

    This still reads a bit odd, because a set shouldn’t hold duplicate values, but the loop may produce duplicate values?

    Not sure what you are trying to achieve, but for (const auto& net : ALL_NETWORKS) { if (fdp.Bool()) nets.insert(net); } may be what you are looking for?


    brunoerg commented at 3:56 pm on September 10, 2024:

    This still reads a bit odd, because a set shouldn’t hold duplicate values, but the loop may produce duplicate values?

    Yes, it may produce but would be “ignored” when trying to add.

    Not sure what you are trying to achieve, but for (const auto& net : ALL_NETWORKS) { if (fdp.Bool()) nets.insert(net); } may be what you are looking for?

    Yes, will address it.


    brunoerg commented at 3:59 pm on September 10, 2024:
    Done, thanks.
  78. addrman: change `Select` to support multiple networks 829becd990
  79. net: call `Select` with reachable networks in `ThreadOpenConnections`
    Calling `Select` with reachable networks can avoid unecessary
    calls and avoid exceed the max number of tries.
    e4e3b44e9c
  80. brunoerg force-pushed on Sep 10, 2024
  81. vasild approved
  82. vasild commented at 7:12 am on September 11, 2024: contributor
    ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
  83. DrahtBot requested review from naumenkogs on Sep 11, 2024
  84. naumenkogs approved
  85. naumenkogs commented at 8:08 am on September 11, 2024: member
    ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
  86. DrahtBot added the label CI failed on Sep 11, 2024
  87. DrahtBot removed the label CI failed on Sep 15, 2024
  88. achow101 commented at 8:49 pm on September 16, 2024: member
    ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
  89. achow101 merged this on Sep 16, 2024
  90. achow101 closed this on Sep 16, 2024

  91. in src/addrman.cpp:1216 in e4e3b44e9c
    1212@@ -1208,11 +1213,11 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision()
    1213     return ret;
    1214 }
    1215 
    1216-std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::optional<Network> network) const
    1217+std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, const std::unordered_set<Network>& networks) const
    


    jonatack commented at 8:58 pm on September 16, 2024:

    Missing #include <unordered_set> in this file (include what you use)

    0/src/addrman.cpp should add these lines:
    1#include <algorithm>                // for max, min
    2#include <compare>                  // for operator<, strong_ordering, operator>, operator<=
    3#include <iterator>                 // for advance
    4#include <ratio>                    // for ratio
    5#include <set>                      // for set, _Rb_tree_const_iterator, operator==
    6#include <unordered_map>            // for unordered_map
    7#include "netgroup.h"               // for NetGroupManager
    8#include "sync.h"                   // for AssertLockHeldInternal, AssertLockHeld, LOCK, UniqueLock
    
  92. in src/addrman.cpp:725 in e4e3b44e9c
    727-        new_count = counts.n_new;
    728-        tried_count = counts.n_tried;
    729+    if (!networks.empty()) {
    730+        new_count = 0;
    731+        tried_count = 0;
    732+        for (auto& network : networks) {
    


    jonatack commented at 9:01 pm on September 16, 2024:

    A Network is a cheaply-copied int enum; no need to use it by reference, unless I’m missing something.

    0        for (Network network : networks) {
    
  93. in src/test/fuzz/addrman.cpp:290 in e4e3b44e9c
    284@@ -285,7 +285,15 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
    285     auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
    286     auto filtered = fuzzed_data_provider.ConsumeBool();
    287     (void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered);
    288-    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
    289+
    290+    std::unordered_set<Network> nets;
    291+    for (const auto& net : ALL_NETWORKS) {
    


    jonatack commented at 9:03 pm on September 16, 2024:
    0    for (Network net : ALL_NETWORKS) {
    
  94. jonatack commented at 9:19 pm on September 16, 2024: member
    (Happy to re-review if you squash the minor comments below into #30183.)
  95. brunoerg deleted the branch on Sep 23, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me