addrman: Enable selecting addresses by network #27214

amitiuttarwar commented at 7:47 PM on March 6, 2023: contributor

For the full context & motivation of this patch, see #27213

This is joint work with mzumsande.

This PR adds functionality to AddrMan::Select to enable callers to specify a network they are interested in.

Along the way, it refactors the function to deduplicate the logic, updates the local variables to match modern conventions, adds test coverage for both the new and existing Select logic, and adds bench tests for the worst case performance of both the new and existing Select logic.

This functionality is used in the parent PR.

refactor: update Select_ function

Extract the logic that decides whether the new or the tried table is going to
be searched to the beginning of the function.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

9bf078f66c

DrahtBot commented at 7:47 PM on March 6, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, brunoerg, ajtowns, mzumsande

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:

#27213 (p2p: Diversify automatic outbound connections with respect to networks by amitiuttarwar)

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.

DrahtBot renamed this:
~~addrman: Enable selecting addresses by network~~
addrman: Enable selecting addresses by network
on Mar 6, 2023

DrahtBot added the label P2P on Mar 6, 2023

brunoerg commented at 9:36 PM on March 6, 2023: contributor

Concept ACK

going to review soon

in src/addrman.cpp:725 in 9bf078f66c outdated

 718 | @@ -719,12 +719,21 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool newOnly) const
 719 |      AssertLockHeld(cs);
 720 |  
 721 |      if (vRandom.empty()) return {};
 722 | -
 723 |      if (newOnly && nNew == 0) return {};
 724 |  
 725 | +    // Decide if we are going to search the new or tried table
 726 | +    bool search_tried;

brunoerg commented at 9:53 PM on March 6, 2023:

nit (for 9bf078f66c8f286e1ab5e34b8eeed7d80290a897):

diff --git a/src/addrman.cpp b/src/addrman.cpp
index ec5b0213b..f608d60f0 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -722,15 +722,13 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool newOnly) const
     if (newOnly && nNew == 0) return {};
 
     // Decide if we are going to search the new or tried table
-    bool search_tried;
+    bool search_tried{false};
 
     // Use a 50% chance for choosing between tried and new table entries.
     if (!newOnly &&
        (nTried > 0 &&
         (nNew == 0 || insecure_rand.randbool() == 0))) {
         search_tried = true;
-    } else {
-        search_tried = false;
     }
 
     if (search_tried) {

amitiuttarwar commented at 2:42 AM on March 7, 2023:

hm, this suggestion makes sense within the context of the first commit. however, the search_tried logic gets updated over the commits so it would be expanded anyways.

it would be possible to rework the end state to be a bit more nifty like this, but since this logic is pretty crucial to the function working as intended, I'm more inclined to leave the explicit assignment of new vs tried.

brunoerg commented at 11:41 AM on March 7, 2023:

Yes, can leave this way. This suggestion I made while reviewing the first commit but considering the whole context, you can leave it.

in src/addrman.cpp:735 in d2f12a0687 outdated

 732 | +        auto counts = it->second;
 733 | +        new_count = counts.n_new;
 734 | +        tried_count = counts.n_tried;
 735 | +    }
 736 | +
 737 | +    if (newOnly && new_count == 0) return {};

brunoerg commented at 9:59 PM on March 6, 2023:

perhaps rename newOnly to new_only?

amitiuttarwar commented at 3:02 AM on March 7, 2023:

done

in src/addrman.cpp:750 in d2f12a0687 outdated

 756 | -        bucket_count = ADDRMAN_NEW_BUCKET_COUNT;
 757 | +        search_tried = insecure_rand.randbool();
 758 |      }
 759 |  
 760 | +    int bucket_count;
 761 | +    search_tried ? bucket_count = ADDRMAN_TRIED_BUCKET_COUNT : bucket_count = ADDRMAN_NEW_BUCKET_COUNT;

brunoerg commented at 10:03 PM on March 6, 2023:

Suggestion:

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 8c36af13f..66d54d2b4 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -746,8 +746,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool newOnly, std::optiona
         search_tried = insecure_rand.randbool();
     }
 
-    int bucket_count;
-    search_tried ? bucket_count = ADDRMAN_TRIED_BUCKET_COUNT : bucket_count = ADDRMAN_NEW_BUCKET_COUNT;
+    const int bucket_count{search_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT};
 
     //  Loop through the addrman table until we find an appropriate entry
     double chance_factor = 1.0;

amitiuttarwar commented at 3:02 AM on March 7, 2023:

nice, done

amitiuttarwar force-pushed on Mar 7, 2023

amitiuttarwar commented at 3:02 AM on March 7, 2023: contributor

thanks for the review @brunoerg ! responded to all your feedback

in src/bench/addrman.cpp:145 in 3a93327688 outdated

 140 | +    addrman.Add({i2p_address}, source);
 141 | +
 142 | +    FillAddrMan(addrman);
 143 | +
 144 | +    bench.run([&] {
 145 | +        const auto& address = addrman.Select(/*newOnly*/false, NET_I2P);

brunoerg commented at 11:39 AM on March 7, 2023:

nit:

        const auto& address = addrman.Select(/*new_only=*/false, NET_I2P);

amitiuttarwar commented at 10:53 PM on March 7, 2023:

good catch, updated. also realized there were stale comments in the addrman unit test, so updated those too.

in src/bench/addrman.cpp:82 in 3a93327688 outdated

  77 | +    CNetAddr addr;
  78 | +    LookupHost(ip, addr, false);
  79 | +    return addr;
  80 | +}
  81 | +
  82 | +static CService ResolveService(const std::string& ip, uint16_t port = 0)

brunoerg commented at 5:21 PM on March 7, 2023:

Interesting, perhaps #26261 would be a good fit :)

amitiuttarwar commented at 10:52 PM on March 7, 2023:

ah, it looks like those changes would remove the need for this helper?

brunoerg commented at 10:16 AM on March 8, 2023:

Yes, if it makes sense for you I'd appreciate your review there btw.

amitiuttarwar force-pushed on Mar 7, 2023

brunoerg approved

brunoerg commented at 10:16 AM on March 8, 2023: contributor

crACK 25a64a20749f10ce84060f3570ad76d1a4776948

in src/addrman.cpp:798 in 25a64a2074 outdated

 851 | +        // Otherwise start over with a (likely) different bucket, and increased chance factor.
 852 | +        chance_factor *= 1.2;
 853 | +    }
 854 | +}
 855 | +
 856 | +int AddrManImpl::LookupAddrmanEntry(bool use_tried, int bucket, int position) const

vasild commented at 1:32 PM on March 8, 2023:

nit: Addrman in the method name is redundant. Maybe AddrManImpl::LookupEntry() or even AddrManImpl::Lookup() or AddrManImpl::Get().

vasild commented at 1:38 PM on March 8, 2023:

nit: I know it was using int to index the array, but those should be size_t. Now looks like a good time to fix that:

int AddrManImpl::LookupAddrmanEntry(bool use_tried, size_t bucket, size_t position) const

amitiuttarwar commented at 6:31 PM on March 11, 2023:

good point, updated to GetEntry to match the naming style of GetAddr

amitiuttarwar commented at 6:32 PM on March 11, 2023:

updated, my understanding is that size_t is unsigned and better optimizes for different platforms. does that track?

vasild commented at 11:12 AM on March 15, 2023:

Yes. Also std methods that take such "indexes" are usually size_t, e.g. https://en.cppreference.com/w/cpp/container/array/operator_at

in src/addrman.cpp:809 in 25a64a2074 outdated

 859 | +
 860 | +    if (use_tried) {
 861 | +        return vvTried[bucket][position];
 862 | +    } else {
 863 | +        return vvNew[bucket][position];
 864 |      }

vasild commented at 1:36 PM on March 8, 2023:

Would be good to have an assert that checks there is no out-of-bounds access. This method itself does not know what the callers will use for arguments or how they will be derived so better check.

Maybe change the arrays to std::array and use .at() here.

amitiuttarwar commented at 8:20 PM on March 10, 2023:

good point about checking bounds. one way to do that is with simple checks like assert(bucket <= ADDRMAN_TRIED_BUCKET_COUNT) etc.

Maybe change the arrays to std::array and use .at() here.

but I'd like to understand this option better. right now vvTried and vvNew are declared as C-style arrays. is your recommendation to change those AddrManImpl declarations so that we can utilize the bounds checking of the .at() function?

vasild commented at 10:54 AM on March 15, 2023:

Yes, but I am not saying to do it (use std::array and .at()). It is just an option, in case you have not considered it. Up to you.

Btw, a difference between an assert() and .at() is that the former will call abort() and will inevitably cause the program to exit. OTOH .at() will throw std::out_of_range which the caller can catch and continue the execution. I don't have a strong opinion which one is more preferable to use here. Maybe either one is ok. Usually on such "programmer error" we want to really stop the whole program, OTOH .at() is much more elegant to write :) and if the exception is unhandled then it will also cause the program to exit.

amitiuttarwar commented at 1:06 AM on March 18, 2023:

opted for the simple assertions because its less invasive & I didn't feel like there was a big advantage of switching over

in src/addrman.cpp:789 in 25a64a2074 outdated

 842 | +
 843 | +        // With probability GetChance() * chance_factor, return the entry.
 844 | +        if (insecure_rand.randbits(30) < chance_factor * info.GetChance() * (1 << 30)) {
 845 | +            std::string table_name;
 846 | +            search_tried ? table_name = "tried" : table_name = "new";
 847 | +            LogPrint(BCLog::ADDRMAN, "Selected %s from %s\n", info.ToStringAddrPort(), table_name);

vasild commented at 2:26 PM on March 8, 2023:

std::string will do heap allocation. It is an overkill in this case.

            LogPrint(BCLog::ADDRMAN, "Selected %s from %s\n", info.ToStringAddrPort(), search_tried ? "tried" : "new");

amitiuttarwar commented at 11:29 PM on March 10, 2023:

oh interesting, I thought that the compiler optimizes away these sort of named-variables-used-shortly-after patterns, but maybe it's different for std::string? I took it to compiler explorer to see if I could observe the allocation- https://godbolt.org/z/GG78zv3KT. are the calls to std::allocator<char> what you are referring to?

amitiuttarwar commented at 6:32 PM on March 11, 2023:

updated

in src/addrman.h:154 in 25a64a2074 outdated

 151 | +     * @param[in] network  Select only addresses of this network (nullopt = all)
 152 |       * @return    CAddress The record for the selected peer.
 153 |       *            seconds  The last time we attempted to connect to that peer.
 154 |       */
 155 | -    std::pair<CAddress, NodeSeconds> Select(bool newOnly = false) const;
 156 | +    std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<Network> network = {}) const;

vasild commented at 2:37 PM on March 8, 2023:

nit: use std::nullopt instead of {} which I believe is more readable and consistent with the comment that says "nullopt = all". Like here:

https://github.com/bitcoin/bitcoin/blob/8d12127a9c19cb218d661a88ab9b6871c9d853b9/src/addrman.h#L109

amitiuttarwar commented at 6:33 PM on March 11, 2023:

done

in src/addrman.cpp:726 in 25a64a2074 outdated

 754 | -            fChanceFactor *= 1.2;
 755 | -        }
 756 | +    size_t new_count = nNew;
 757 | +    size_t tried_count = nTried;
 758 | +
 759 | +    if (network) {

vasild commented at 2:42 PM on March 8, 2023:

nit: I think it is good to be explicit and use network.has_value() here. Otherwise it can get confusing, especially with booleans (network is not boolean, but anyway). For example:

std::optional<bool> is_odd
...
if (is_odd) { // did the author mean is_odd.has_value() or *is_odd?

amitiuttarwar commented at 6:33 PM on March 11, 2023:

done

in src/addrman.cpp:736 in 25a64a2074 outdated

 764 | +        new_count = counts.n_new;
 765 | +        tried_count = counts.n_tried;
 766 | +    }
 767 | +
 768 | +    if (new_only && new_count == 0) return {};
 769 | +    if ((new_count + tried_count) == 0) return {};

vasild commented at 2:45 PM on March 8, 2023:

nit:

    if (new_count + tried_count == 0) return {};

amitiuttarwar commented at 6:33 PM on March 11, 2023:

done

in src/addrman.h:149 in 25a64a2074 outdated

 145 | @@ -146,11 +146,12 @@ class AddrMan
 146 |      /**
 147 |       * Choose an address to connect to.
 148 |       *
 149 | -     * @param[in] newOnly  Whether to only select addresses from the new table.
 150 | +     * @param[in] new_only Whether to only select addresses from the new table.

vasild commented at 2:53 PM on March 8, 2023:

This maybe warrants a better description:

     * [@param](/bitcoin-bitcoin/contributor/param/)[in] new_only Whether to only select addresses from the new table. Passing `true` guarantees either an address from the new table or an invalid return value. Passing `false` requests 50% chance of new or tried, it does not guarantee an entry from the tried table.

amitiuttarwar commented at 6:34 PM on March 11, 2023:

updated but worded it a bit differently - the 50% is only true if there are matches in both tables (with network interactions). lmk if the new language seems reasonable to you

in src/addrman.cpp:751 in 25a64a2074 outdated

 802 | +        search_tried = insecure_rand.randbool();
 803 | +    }
 804 | +
 805 | +    const int bucket_count{search_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT};
 806 | +
 807 | +    //  Loop through the addrman table until we find an appropriate entry

vasild commented at 2:55 PM on March 8, 2023:

nit:

    // Loop through the addrman table until we find an appropriate entry

amitiuttarwar commented at 6:34 PM on March 11, 2023:

done

in src/addrman.cpp:765 in 25a64a2074 outdated

 816 | +        int i;
 817 | +        for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
 818 | +            int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
 819 | +            int node_id = LookupAddrmanEntry(search_tried, bucket, position);
 820 | +            if (node_id != -1) {
 821 | +                if (network) {

vasild commented at 2:55 PM on March 8, 2023:

nit: same as elsewhere: consider network.has_value().

amitiuttarwar commented at 6:34 PM on March 11, 2023:

done

in src/addrman.cpp:768 in 25a64a2074 outdated

 818 | +            int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
 819 | +            int node_id = LookupAddrmanEntry(search_tried, bucket, position);
 820 | +            if (node_id != -1) {
 821 | +                if (network) {
 822 | +                    const auto it{mapInfo.find(node_id)};
 823 | +                    const auto info{it->second};

vasild commented at 3:00 PM on March 8, 2023:

nit: the code below has an assert after find(), good to have it here too:

                    const auto it{mapInfo.find(node_id)};
                    assert(it != mapInfo.end());
                    const auto info{it->second};

amitiuttarwar commented at 6:35 PM on March 11, 2023:

done

in src/test/addrman_tests.cpp:128 in 25a64a2074 outdated

 126 | @@ -127,45 +127,47 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
 127 |      // the specified port to tried, but not the other.
 128 |      addrman->Good(CAddress(addr1_port, NODE_NONE));

vasild commented at 8:46 AM on March 9, 2023:

nit: in the commit message of tests: add addrman_select_by_network test: s/newOnly/new_only since it was already renamed before that commit

amitiuttarwar commented at 6:35 PM on March 11, 2023:

done

in src/test/addrman_tests.cpp:200 in 25a64a2074 outdated

 197 |  }
 198 |  
 199 | +BOOST_AUTO_TEST_CASE(addrman_select_by_network)
 200 | +{
 201 | +    auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));
 202 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/true, NET_IPV4).first.ToStringAddrPort(), "[::]:0");

vasild commented at 8:49 AM on March 9, 2023:

nit: that should be /*new_only=*/ for clang-tidy to check it and for consistency with the rest of the code base (here and elsewhere).

amitiuttarwar commented at 6:35 PM on March 11, 2023:

ah, thanks. updated lots of comments, I think I caught them all?

vasild commented at 2:32 PM on March 15, 2023:

I think so, thanks!

in src/test/addrman_tests.cpp:201 in 25a64a2074 outdated

 198 |  
 199 | +BOOST_AUTO_TEST_CASE(addrman_select_by_network)
 200 | +{
 201 | +    auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));
 202 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/true, NET_IPV4).first.ToStringAddrPort(), "[::]:0");
 203 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_IPV4).first.ToStringAddrPort(), "[::]:0");

vasild commented at 8:52 AM on March 9, 2023:

It would be better to check that the returned result is !IsValid() and not rely on how an invalid address would be represented by ToStringAddrPort() (maybe even that method should assert that the object is valid).

    BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_IPV4).first.IsValid());

amitiuttarwar commented at 6:35 PM on March 11, 2023:

thanks, updated this in lots of places

in src/test/addrman_tests.cpp:208 in 25a64a2074 outdated

 205 | +    // add ipv4 address to the new table
 206 | +    CNetAddr source = ResolveIP("252.2.2.2");
 207 | +    CService addr1 = ResolveService("250.1.1.1", 8333);
 208 | +    BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source));
 209 | +
 210 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/true, NET_IPV4).first.ToStringAddrPort(), "250.1.1.1:8333");

vasild commented at 9:18 AM on March 9, 2023:

nit: 250.1.1.1 is duplicated here and above. Maybe put it in a variable or avoid comparing through strings altogether (here and elsewhere):

    BOOST_CHECK(addrman->Select(/*new_only=*/true, NET_IPV4).first == addr1);

amitiuttarwar commented at 6:36 PM on March 11, 2023:

incorporated this in many places in the tests I touched. definitely reads better

in src/test/addrman_tests.cpp:226 in 25a64a2074 outdated

 223 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/true, NET_I2P).first.ToStringAddrPort(), "udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p:0");
 224 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_I2P).first.ToStringAddrPort(), "udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p:0");
 225 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_IPV4).first.ToStringAddrPort(), "250.1.1.1:8333");
 226 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_IPV6).first.ToStringAddrPort(), "[::]:0");
 227 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_ONION).first.ToStringAddrPort(), "[::]:0");
 228 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_CJDNS).first.ToStringAddrPort(), "[::]:0");

vasild commented at 9:23 AM on March 9, 2023:

"new_only=true, some_nonexistent_network" seems untested:

BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_CJDNS).first.IsValid());

amitiuttarwar commented at 6:36 PM on March 11, 2023:

added

vasild commented at 1:46 PM on March 15, 2023:

Where?

amitiuttarwar commented at 1:05 AM on March 18, 2023:

maybe I was thinking of BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_I2P).first.IsValid());?

not sure, I added BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_CJDNS).first.IsValid()); for real this time 🙃

in src/test/addrman_tests.cpp:217 in 25a64a2074 outdated

 214 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_I2P).first.ToStringAddrPort(), "[::]:0");
 215 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_CJDNS).first.ToStringAddrPort(), "[::]:0");
 216 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false).first.ToStringAddrPort(), "250.1.1.1:8333");
 217 | +
 218 | +    // add I2P address to the new table
 219 | +    CService i2p_addr;

vasild commented at 9:29 AM on March 9, 2023:

If this is changed to:

CAddress i2p_addr;

and the rest of the code left as it is, then it will still work, but then CAddress(i2p_addr, NODE_NONE) which is duplicated below can be replaced with i2p_addr.

amitiuttarwar commented at 6:36 PM on March 11, 2023:

done

in src/test/addrman_tests.cpp:246 in 25a64a2074 outdated

 243 | +    // ensure that both new and tried table are selected from
 244 | +    std::set<std::string> i2p_addrs;
 245 | +    for (int i = 0; i < 10; ++i) {
 246 | +        i2p_addrs.insert(addrman->Select(/*new_only*/false, NET_I2P).first.ToStringAddrPort());
 247 | +    }
 248 | +    BOOST_CHECK_EQUAL(i2p_addrs.size(), 2U);

vasild commented at 9:45 AM on March 9, 2023:

This test will fail sporadically.

Also, if you are only going to check the size, then a counter suffices. If you are going to collect the strings, then you may as well check that they are as expected (i2p_addr and i2p_addr2).

Consider this:

    CAddress i2p_addr2;
...
    // ensure that both new and tried table are selected from
    bool new_selected{false};
    bool tried_selected{false};
    while (!new_selected || !tried_selected) { 
        const CAddress selected{addrman->Select(/*new_only=*/false, NET_I2P).first};
        BOOST_REQUIRE(selected == i2p_addr || selected == i2p_addr2);
        if (selected == i2p_addr) {
            tried_selected = true;
        } else {
            new_selected = true;
        }
    }

amitiuttarwar commented at 6:36 PM on March 11, 2023:

great idea ! updated to use this while loop. also added you as co-author for this commit btw, thanks for the in depth review :)

in src/test/addrman_tests.cpp:262 in 25a64a2074 outdated

 259 | +    BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source));
 260 | +
 261 | +    // since the only address is on the new table, ensure that the new table
 262 | +    // gets selected even if new_only is false. if the table was being selected
 263 | +    // at random, this test will sporadically fail
 264 | +    BOOST_CHECK_EQUAL(addrman->Select(/*new_only*/false, NET_IPV4).first.ToStringAddrPort(), "250.1.1.3:8333");

vasild commented at 10:03 AM on March 9, 2023:

For this test to be meaningful there must be some address also in the tried table. Otherwise:

740     bool search_tried;
741     if (new_only || tried_count == 0) {
742         search_tried = false;
743     } else if (new_count == 0) {
744         search_tried = true;
745     } else {
746         search_tried = insecure_rand.randbool();
747     }

it will always go to the new table via line 742 because tried_count == 0 and it will never execute line 746 which I guess is the purpose of this test.

amitiuttarwar commented at 3:29 PM on March 11, 2023:

this test is actually covering the tried_count == 0 logic. if it were to hit line 746, it would sometimes select the tried table and sometimes the new table, which means it should fail 50% of the time. this coverage is ensuring that the tried_count has the proper interactions with network, which gets assigned earlier in the function-

    if (network.has_value()) {
        auto it = m_network_counts.find(*network);
        if (it == m_network_counts.end()) return {};

        auto counts = it->second;
        new_count = counts.n_new;
        tried_count = counts.n_tried;
    }

lmk if that makes sense

vasild commented at 11:03 AM on March 15, 2023:

Oh, yes, now it makes sense! I was confused. I should have read the comment like: "ensure that the new table gets selected even if new_only is false (because the tried table is empty)"

amitiuttarwar commented at 4:29 AM on March 16, 2023:

yeah, just to clarify- the tried table doesn't have to be empty, it just needs to have no matches for the specific network. I tried to capture that in the beginning of the comment: "since the only ipv4 address is on the new table", but let me know if there's something else that would help make that more clear

in src/bench/addrman.cpp:146 in 25a64a2074 outdated

 141 | +
 142 | +    FillAddrMan(addrman);
 143 | +
 144 | +    bench.run([&] {
 145 | +        const auto& address = addrman.Select(/*new_only*/false, NET_I2P);
 146 | +        assert(address.first.ToStringAddrPort() == "udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p:0");

vasild commented at 11:05 AM on March 9, 2023:

String comparison is very non-effective and surely has an impact of performance in such a tight-loop cases. This may better be:

        const auto address = addrman.Select(/*new_only*/false, NET_I2P).first;
        assert(address == i2p_address);

I think that this also does not need to check the result - now it is mixing bench + correctness test in such a way that the correctness check may skew the benchmark. We have the unit tests to ensure correctness. Thus, this may as well be:

        (void)addrman.Select(/*new_only*/false, NET_I2P);

to reduce the amount of noise.

amitiuttarwar commented at 6:37 PM on March 11, 2023:

that makes sense, updated to remove correctness tests

in src/bench/addrman.cpp:126 in 25a64a2074 outdated

 121 | +    CService addr = ResolveService("250.3.1.1", 8333);
 122 | +    addrman.Add({CAddress(addr, NODE_NONE)}, ResolveService("250.3.1.1", 8333));
 123 | +
 124 | +    bench.run([&] {
 125 | +        const auto& address = addrman.Select();
 126 | +        assert(address.first.GetPort() > 0);

vasild commented at 11:10 AM on March 9, 2023:

Is checking the port an indirect way to check that Select() returned a valid result? You can check that the returned value IsValid() for that. But same as for the other bench, this may as well be just (void)addrman.Select(); to reduce the noise (and leave correctness checks to the unit tests).

amitiuttarwar commented at 6:37 PM on March 11, 2023:

yeah it was, but updated to remove that assertion.

in src/bench/addrman.cpp:129 in 25a64a2074 outdated

 125 | +        const auto& address = addrman.Select();
 126 | +        assert(address.first.GetPort() > 0);
 127 | +    });
 128 | +}
 129 | +
 130 | +static void AddrManSelectByNetwork(benchmark::Bench& bench)

vasild commented at 11:14 AM on March 9, 2023:

It is excellent to see benchmark tests added! My only concern with this PR is that it will iterate through a lot of entries in addrman before finding one from the requested network. This in theory is O(addrman size). And the benchmarks will help us assess that. So, it would be nice to compare, on the same, full addrman (e.g. 70k addresses):

the speed of a regular Select()
the speed of a Select(network) where the searched for address is ~near the end~ rare. In other words, worst case scenario, when ~70k addresses are iterated before finding the result.

Somehow I don't see that from the added benchmarks. I will play with them to see if that's possible (~how to put an address "at the end"?~, that is nonsense, there is no end).

Btw the results are unstable:

:wavy_dash: `AddrManSelectByNetwork` (Unstable with ~1.2 iters. Increase `minEpochIterations` to e.g. 12)

vasild commented at 5:02 PM on March 10, 2023:

Let me elaborate why I think comparing 1. and 2. above is important (the benchmarks in this PR don't do that):

Right now, on master, when we pick a peer to connect to 1. happens.
With #27213 when we pick a peer to connect to 2. happens.
I assume most of the addrmans out there are full (tens of thousands of addresses, max 80k), thus testing on an empty addrman is not representative.

So, we will add security at the cost of making it a bit slower. To assess how much slower I tweaked the benchmark as this:

<details> <summary>benchmark tweaks</summary>

diff --git i/src/bench/addrman.cpp w/src/bench/addrman.cpp
index 9fe50f4ec2..d1def9e520 100644
--- i/src/bench/addrman.cpp
+++ w/src/bench/addrman.cpp
@@ -12,13 +12,13 @@
 
 #include <optional>
 #include <vector>
 
 /* A "source" is a source address from which we have received a bunch of other addresses. */
 
-static constexpr size_t NUM_SOURCES = 64;
+static constexpr size_t NUM_SOURCES = 512; // fills addrman with ~55k addresses, instead of ~15k
 static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;
 
 static NetGroupManager EMPTY_NETGROUPMAN{std::vector<bool>()};
 static constexpr uint32_t ADDRMAN_CONSISTENCY_CHECK_RATIO{0};
 
 static std::vector<CAddress> g_sources;
@@ -137,16 +137,20 @@ static void AddrManSelectByNetwork(benchmark::Bench& bench)
     CAddress i2p_address(i2p_service, NODE_NONE);
     i2p_address.nTime = Now<NodeSeconds>();
     CNetAddr source = ResolveIP("252.2.2.2");
     addrman.Add({i2p_address}, source);
 
     FillAddrMan(addrman);
-
-    bench.run([&] {
-        const auto& address = addrman.Select(/*new_only*/false, NET_I2P);
-        assert(address.first.ToStringAddrPort() == "udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p:0");
+    fprintf(stderr, "addrman size: %zu\n", addrman.Size());
+
+    bench.minEpochIterations(300).run([&] {
+#if 0
+        addrman.Select(/*new_only=*/false);
+#else
+        addrman.Select(/*new_only=*/false, NET_I2P);
+#endif
     });
 }
 
 static void AddrManGetAddr(benchmark::Bench& bench)
 {
     AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};

</details>

The results are:

Select() takes 0.18 microseconds
Select(network) takes 6000 microseconds

Now, this is a big difference, but maybe that is ok. We don't Select() in a tight loop. However, it can be improved relatively easy. I observed that in 2. we visit between ~100 and ~600k bucket positions before finding a match. Given that there are 80k in addrman it means that we visit some multiple times, which is a waste of resources. This is avoided if we don't visit already visited buckets (but still visit buckets in random order). The patch below does that (on top of this PR):

<details> <summary>don't visit already visited buckets</summary>

diff --git i/src/addrman.cpp w/src/addrman.cpp
index 1023c3cbdb..0e196050b9 100644
--- i/src/addrman.cpp
+++ w/src/addrman.cpp
@@ -16,12 +16,13 @@
 #include <streams.h>
 #include <tinyformat.h>
 #include <uint256.h>
 #include <util/check.h>
 #include <util/time.h>
 
+#include <array>
 #include <cmath>
 #include <optional>
 
 /** Over how many buckets entries with tried addresses from a single group (/16 for IPv4) are spread */
 static constexpr uint32_t ADDRMAN_TRIED_BUCKETS_PER_GROUP{8};
 /** Over how many buckets entries with new addresses originating from a single group are spread */
@@ -745,17 +746,39 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
     } else {
         search_tried = insecure_rand.randbool();
     }
 
     const int bucket_count{search_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT};
 
+    // We want to search buckets in random order and also prefer visiting a
+    // bucket that we have not visited before instead of visiting an already
+    // visited bucket. Thus shuffle the buckets and visit that shuffled list
+    // in order.
+
+    std::array<uint16_t, std::max(ADDRMAN_TRIED_BUCKET_COUNT, ADDRMAN_NEW_BUCKET_COUNT)>
+        shuffled_bucket_indexes;
+    for (uint16_t i = 0; i < bucket_count; ++i) {
+        shuffled_bucket_indexes[i] = i;
+    }
+    Shuffle(shuffled_bucket_indexes.begin(),
+            shuffled_bucket_indexes.begin() + bucket_count,
+            insecure_rand);
+    // If we visit a bucket and find 0 matching addresses in it, mark it with
+    // this and never visit it again.
+    static constexpr uint16_t ALREADY_VISITED_AND_BORING{std::numeric_limits<uint16_t>::max()};
+
+    static_assert(ADDRMAN_TRIED_BUCKET_COUNT < ALREADY_VISITED_AND_BORING);
+    static_assert(ADDRMAN_NEW_BUCKET_COUNT < ALREADY_VISITED_AND_BORING);
+
     //  Loop through the addrman table until we find an appropriate entry
     double chance_factor = 1.0;
-    while (1) {
-        // Pick a bucket, and an initial position in that bucket.
-        int bucket = insecure_rand.randrange(bucket_count);
+    for (uint16_t b = 0;; b = (b + 1) % bucket_count) {
+        if (shuffled_bucket_indexes[b] == ALREADY_VISITED_AND_BORING) {
+            continue;
+        }
+        const size_t bucket{shuffled_bucket_indexes[b]};
         int initial_position = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
 
         // Iterate over the positions of that bucket, starting at the initial one,
         // and looping around.
         int i;
         for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
@@ -769,14 +792,19 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
                 } else {
                     break;
                 }
             }
         }
 
-        // If the bucket is entirely empty, start over with a (likely) different one.
-        if (i == ADDRMAN_BUCKET_SIZE) continue;
+        // Start over with a different bucket if this one is entirely empty or
+        // specific network was requested and it does not contain any addresses
+        // from that network.
+        if (i == ADDRMAN_BUCKET_SIZE) {
+            shuffled_bucket_indexes[b] = ALREADY_VISITED_AND_BORING;
+            continue;
+        }
 
         // Find the entry to return.
         int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
         int nId = LookupAddrmanEntry(search_tried, bucket, position);
         const auto it_found{mapInfo.find(nId)};
         assert(it_found != mapInfo.end());

</details>

It changes Select(network) to about 3000 microseconds (down from 6000), but that result from the benchmark is averaged between executions that take anything between 100 and 600k iterations. What is more important is that with the change above it never takes more than ~60k iterations, that is - anything between 100 and 60k. Worst case lowered ~10 times.

The optimization brings some complexity but IMO it is worth it.

mzumsande commented at 6:37 PM on March 10, 2023:

This looks like a nice speedup in the case where we only have a few addresses - however, due to the constant overhead of building the shuffled list of buckets in the beginning I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark? Why does this need ALREADY_VISITED_AND_BORING? If we do a for-loop through a pre-shuffled list of buckets instead of a while loop, doesn't that already guarantee that we visit each bucket at most once?

amitiuttarwar commented at 3:58 PM on March 11, 2023:

if I am understanding this thread properly, it seems like there are two concepts being discussed:

changes to the bench tests
optimization to AddrManImpl::Select_

RE 1, I'm slightly confused as to the desired coverage. Without the network parameter, I would expect the worst case of the current Select_ function to happen when addrman is practically empty, which is why we introduced AddrManSelectFromAlmostEmpty. On mainnet, this case would be unlikely, which is represented by AddrManSelect. With the network parameter, I would expect worst case to be represented by AddrManSelectByNetwork where theres just 1 I2P address on an addrman filled with other addresses. @vasild can you clarify what feels absent or misrepresentative?

also happy to discuss 2 further, but a question there - does it feel relevant to these patches to improve performance, or is it an orthogonal improvement (since we see similar worst cases right now)?

vasild commented at 10:40 AM on March 15, 2023:

@mzumsande,

I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark?

Yes, that slows down a bit the fast Select() (any network is ok). I observed that and think it is ok because it is still super fast with or without the shuffling.

Why does this need ALREADY_VISITED_AND_BORING?

The for loop could still repeat the whole array from the start if some address was found but was skipped. ALREADY_VISITED_AND_BORING is a further optimization to completely skip that bucket on the second and further passes through the array. @amitiuttarwar, you are right that I modified the newly added AddrManSelectByNetwork() so that, in general, it tests the same thing as AddrManSelect() which already exists in master. I did that in order to be sure that everything else is identical: the way addrman is filled, minEpochIterations(300) and the GetPort() call. Way too many times I have been bitten by chasing noise in benchmarks, so I wanted to be sure that this is the only difference in the two things I am comparing:

#if 0 // flip to 1 to test the other case
        addrman.Select(/*new_only=*/false);
#else
        addrman.Select(/*new_only=*/false, NET_I2P);
#endif

also happy to discuss 2 further, but a question there - does it feel relevant to these patches to improve performance, or is it an orthogonal improvement

I think the performance improvement would be a nice addition to the work done by this PR (or maybe even a "must have"?)

(since we see similar worst cases right now)?

Hmm? Now on master on mainnet it takes 0.18 microseconds and with this PR on mainnet it will take 6000 microseconds. That is the average case (averaged by the benchmark executing Select about 3500 times). The difference in the worst case is even more pronounced.

mzumsande commented at 5:48 PM on March 15, 2023:

Hmm? Now on master on mainnet it takes 0.18 microseconds and with this PR on mainnet it will take 6000 microseconds. That is the average case (averaged by the benchmark executing Select about 3500 times). The difference in the worst case is even more pronounced.

We might be using slightly different terminology here: With "worst case", I mean the edge case where we have just one entry in AddrMan of the kind of address that we want. This is the benchmark AddrManSelectFromAlmostEmpty for unspecific selection and AddrManSelectByNetwork for network-specific selection.

#if 0 // flip to 1 to test the other case addrman.Select(/new_only=/false); #else addrman.Select(/new_only=/false, NET_I2P); #endif

This is not a meaningful comparison, because the first case is picking from an almost full AddrMan ("best case"), and the second case is the "worst case". If you replace NET_I2P by NET_IPV4 in that example, I'd expect the network-specific performance to be basically identical to that of unspecific Select(), because AddrMan contains a single I2P address and ~50k IPV4 addresses in your setup.

Fluctuations due to RNG randomness within a given benchmark shouldn't be called "worst case" in my opinion, especially because there is no lower limit with the way Select_() currently works: for a given RNG seed, we might not visit the required bucket "forever" even if the probability for that will go to zero so quickly that it will never happen - there just is no distinct lower bound we could call "worst case" because it's probabilistic).

Yes, that slows down a bit the fast Select() (any network is ok).

The crucial question is by how much, because situations close to the "worst case" should be rather infrequent, while situations where we have multiple addresses to choose from should me much more common. With all the different methods of getting addresses, after a while of running your node you should at least know a couple dozens of addresses for each reachable network (even for CJDNS?!). So there is the possibility that we might be optimizing for an edge case that is encountered very infrequently in the wild, while slightly slowing down the frequently encountered case at the same time. In this case, the change could be detrimental for performance overall!

I'm not saying this would be the case here, but I want to do some more in-depth benchmarking myself later this week to convince myself a bit more that this would really be a clear performance improvement overall.

mzumsande commented at 9:30 PM on March 17, 2023:

I compared the performance of the PR branch with @vasild's suggested optimization ("VD") a bit more closely:

Test 1: Reasonably full addrman (15k addresses): ./bench_bitcoin -filter=AddrManSelect -min-time=5000 => The PR is much faster, since VD has a constant overhead due to the shuffling.

<details> <summary>Details</summary>

PR: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 142.41 | 7,021,818.47 | 0.2% | 5.50 | AddrManSelect
| 145.75 | 6,860,921.17 | 0.4% | 5.63 | AddrManSelect | 145.91 | 6,853,404.83 | 1.4% | 5.55 | AddrManSelect | 149.78 | 6,676,385.78 | 3.7% | 5.63 | AddrManSelect | 144.37 | 6,926,771.77 | 0.3% | 5.49 | AddrManSelect

VD: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 12,716.43 | 78,638.44 | 0.1% | 5.49 | AddrManSelect | 12,790.84 | 78,180.95 | 0.8% | 5.36 | AddrManSelect | 12,884.37 | 77,613.41 | 0.7% | 5.53 | AddrManSelect | 12,815.30 | 78,031.72 | 0.9% | 5.52 | AddrManSelect | 12,961.91 | 77,149.10 | 1.3% | 5.55 | AddrManSelect

</details>

<br />

Test 2: A single address in AddrMan:
bench_bitcoin -filter=AddrManSelectFromAlmostEmpty -min-time=5000 => VD is faster by a factor of 1.8

<details> <summary>Details</summary>

PR: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 93,286.28 | 10,719.69 | 2.2% | 5.42 | AddrManSelectFromAlmostEmpty | 90,781.42 | 11,015.47 | 1.0% | 5.52 | AddrManSelectFromAlmostEmpty | 92,758.06 | 10,780.73 | 1.2% | 5.57 | AddrManSelectFromAlmostEmpty | 90,026.20 | 11,107.88 | 0.7% | 5.52 | AddrManSelectFromAlmostEmpty | 91,946.89 | 10,875.84 | 1.1% | 5.47 | AddrManSelectFromAlmostEmpty

VD: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 55,306.93 | 18,080.92 | 0.7% | 5.50 | AddrManSelectFromAlmostEmpty | 55,209.86 | 18,112.71 | 0.4% | 5.48 | AddrManSelectFromAlmostEmpty | 54,989.61 | 18,185.25 | 0.5% | 5.51 | AddrManSelectFromAlmostEmpty | 56,727.70 | 17,628.07 | 2.1% | 5.57 | AddrManSelectFromAlmostEmpty | 55,732.54 | 17,942.84 | 0.5% | 5.50 | AddrManSelectFromAlmostEmpty

</details>

Test 3: Three addresses in AddrMan: set NUM_SOURCES = 3, NUM_ADDRESSES_PER_SOURCE=1 then run ./bench_bitcoin -filter=AddrManSelect -min-time=5000 => PR is slightly faster.

<details> <summary>Details</summary>

PR: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 30,034.99 | 33,294.50 | 0.7% | 5.51 | AddrManSelect | 30,370.28 | 32,926.92 | 1.1% | 5.17 | AddrManSelect | 30,574.52 | 32,706.98 | 2.1% | 5.20 | AddrManSelect | 29,883.53 | 33,463.25 | 0.7% | 5.52 | AddrManSelect | 29,944.31 | 33,395.33 | 0.5% | 5.52 | AddrManSelect

VD: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 34,126.18 | 29,303.02 | 0.7% | 5.30 | AddrManSelect | 34,660.26 | 28,851.49 | 0.9% | 5.32 | AddrManSelect | 34,075.81 | 29,346.33 | 0.7% | 5.30 | AddrManSelect | 34,628.06 | 28,878.31 | 1.3% | 5.36 | AddrManSelect | 34,300.57 | 29,154.03 | 0.6% | 5.31 | AddrManSelect

</details>

Test 4: Query for a single network-specific address in an AddrMan filled with 15k IPV4 addresses we don't want ./bench_bitcoin -filter=AddrManSelectByNetwork -min-time=5000 => VD is faster by a factor of ~2.1.

<details> <summary>Details</summary>

PR: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1,256,860.84 | 795.63 | 4.4% | 5.40 | AddrManSelectByNetwork | 1,269,322.70 | 787.82 | 2.6% | 5.50 | AddrManSelectByNetwork | 1,242,639.25 | 804.74 | 1.9% | 5.56 | AddrManSelectByNetwork | 1,246,311.65 | 802.37 | 2.9% | 5.69 | AddrManSelectByNetwork | 1,265,535.68 | 790.18 | 3.4% | 5.59 | AddrManSelectByNetwork

VD | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 560,818.30 | 1,783.11 | 1.0% | 5.56 | AddrManSelectByNetwork | 558,351.91 | 1,790.99 | 0.9% | 5.53 | AddrManSelectByNetwork | 600,536.33 | 1,665.18 | 4.8% | 5.38 | AddrManSelectByNetwork | 569,208.08 | 1,756.83 | 1.7% | 5.45 | AddrManSelectByNetwork | 581,860.03 | 1,718.63 | 2.3% | 5.47 | AddrManSelectByNetwork

</details>

Test 5: Query for 20 network-specific address in an AddrMan filled with 15k IPV4 addresses we don't want (changing the code a bit) ./bench_bitcoin -filter=AddrManSelectByNetwork -min-time=5000 => Performance is approximately the same.

<details> <summary>Details</summary>

PR: | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 109,388.10 | 9,141.76 | 1.6% | 5.40 | AddrManSelectByNetwork | 108,804.63 | 9,190.79 | 2.1% | 5.56 | AddrManSelectByNetwork | 111,333.16 | 8,982.05 | 2.0% | 5.61 | AddrManSelectByNetwork | 110,827.61 | 9,023.02 | 1.4% | 5.55 | AddrManSelectByNetwork | 122,518.06 | 8,162.06 | 1.4% | 5.47 | AddrManSelectByNetwork

VD | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 106,806.78 | 9,362.70 | 2.5% | 5.51 | AddrManSelectByNetwork | 109,221.66 | 9,155.69 | 3.1% | 5.59 | AddrManSelectByNetwork | 107,870.77 | 9,270.35 | 3.8% | 5.41 | AddrManSelectByNetwork | 105,131.17 | 9,511.93 | 0.5% | 5.47 | AddrManSelectByNetwork | 106,080.47 | 9,426.81 | 1.2% | 5.48 | AddrManSelectByNetwork

</details>

In conclusion, my results indicate that the suggested optimization is only faster in some specific scenarios because the pre-shuffling of the buckets adds a constant overhead that is not neglible: In the case of non-network specific queries (which should be the bulk even after #27213 ), the PR is already faster when AddrMan has more than 3 addresses (which should virtually always be the case!) In the case of network-specific queries, the tipping point is (because of the additional cost of looking up addrs in buckets that we can't use) at ~20 network-specific addresses (with 15k IPV4 addresses). This is a bit higher, but I would imagine that we should get more than 20 addresses of most alternative networks rather quickly. To summarize, my results indicate that adding pre-shuffling AddrMan buckets to prevent repeated visits appears to be no clear performance improvement - in most cases, performance would go down. @vasild: Do you see similar numbers?

brunoerg commented at 2:36 PM on March 24, 2023:

About the optimization in #27214 (review), couldn't we simply use a vector to store the visited buckets?

just an example:

  std::vector<int> buckets_visited;
  while (1) {
      // Pick a bucket, and an initial position in that bucket.
      int bucket = insecure_rand.randrange(bucket_count);
      if (std::find(buckets_visited.begin(), buckets_visited.end(), bucket) != buckets_visited.end()) {
          continue;
      } else {
          buckets_visited.push_back(bucket);
      }

Also, I ran same tests that @mzumsande did and got similar results.

vasild commented at 4:51 AM on March 25, 2023:

@mzumsande, I think you are missing my point, let me try to rephrase it: now on master, from ThreadOpenConnections() we call Select(). With the parent of this PR, we will be calling Select(network) which will bring some security (good) + some slowdown (bad).

Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.

What is your "Test 5" doing? Does it call 20 times Select(network)?

Further thoughts if an improvement of performance of Select(network) is deemed necessary:

The shuffling can be done only if network is provided, to avoid slowing down of Select() without a network.
The shuffling can be removed altogether and just marking the buckets as boring to be left. This should still improve the 600k iterations in the worst case I observed.
The ultimate beast solution would be to have an index per network, e.g. std::unordered_set<size_t> positions_in_vRandom_of_i2p_addresses.

I will try to compare on a snapshot of an addrman from my public full node, not on artificially filled addrman from the bench. Last time I checked it had ~70k addresses and also it will have at least a bunch of addresses from each network, not just 1 address. That would be more representative.

vasild commented at 5:22 AM on March 25, 2023:

@brunoerg, yes, that should still bring some improvement. But only push it into the vector if we have visited all addresses in that bucket and found 0 from the requested network because it may happen that we visit a bucket with an address from the requested network, but only pick it up on some of the subsequent iterations, not from the first one. Change it to std::unordered_set so that lookup is fast (O(1)).

brunoerg commented at 5:27 PM on March 27, 2023:

Last time I checked it had ~70k addresses and also it will have at least a bunch of addresses from each network, not just 1 address.

"bunch of addresses from each network", how much? Last time I checked my node (running with multiple networks) I remembered that the number of i2p addresses were so small compared to clearnet, for example.

jonatack commented at 5:51 PM on March 27, 2023:

"bunch of addresses from each network", how much?

If helpful, my node knows 15k Tor, 1.2k I2P and 8 CJDNS recently active peers ATM for a bit more than 16k total non-clearnet peers.

amitiuttarwar commented at 8:05 PM on March 27, 2023:

Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.

agreed that this is the fundamental thing we are trying to evaluate - is the performance difference significant & acceptable

I will try to compare on a snapshot of an addrman from my public full node, not on artificially filled addrman from the bench. Last time I checked it had ~70k addresses and also it will have at least a bunch of addresses from each network, not just 1 address. That would be more representative.

would be awesome if you could compare based on your addrman snapshot from mainnet!

my understanding of martin's benches is its trying to observe "normal" as well as "worst case". so the examples with 1 address are aiming towards "worst case". but of course "normal" would vary widely from node to node.

mzumsande commented at 9:04 PM on March 27, 2023:

What is your "Test 5" doing? Does it call 20 times Select(network)?

No, it calls Select(network) for an addrman that has 20 addresses of the selected network.

I'll try to explain the idea behind my benchmarks better: Starting point is the observation that the suggested change makes Select slower if we already know a lot of addresses of the desired type, while making it faster if addrman has very few addresses of this type. This is true for botch Select() and Select(network), which is why it is not obvious at first sight if the suggestion is an overall performance improvement or a degradation.

So my idea was to find the cutoff - how many addresses do we need such that both algorithms are equally fast - in situations where we have more addrs than this, the change would be a slowdown, when we have less it's a speedup. This cutoff turned out to be very small for Select() (3 addresses), while being a bit larger for Select(network) - it was approximately 20. It would be helpful to know how things change with a full addrman!

So if we used an addrman with counts similar to those reported by @jonatack, my results would suggest that the pre-shufffling change suggested here would be a speedup for CJDNS-specific queries, while being a slowdown for clearnet, onion, i2p and unspecific queries (all compared to this PR).

vasild commented at 4:04 PM on March 30, 2023:

I ran this on a mainnet peers.dat:

addrman size total: 72947
addrman size IPv4:  47252
addrman size IPv6:  11491
addrman size Tor:   13305
addrman size I2P:   893
addrman size CJDNS: 6

<details> <summary>[patch] How to load /tmp/peers.dat from bench_bitcoin</summary>

diff --git i/src/bench/addrman.cpp w/src/bench/addrman.cpp
index 8a5cab443f..b8ec4a4071 100644
--- i/src/bench/addrman.cpp
+++ w/src/bench/addrman.cpp
@@ -1,17 +1,22 @@
 // Copyright (c) 2020-2022 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <addrdb.h>
 #include <addrman.h>
 #include <bench/bench.h>
+#include <chainparams.h>
+#include <chainparamsbase.h>
 #include <netbase.h>
 #include <netgroup.h>
 #include <random.h>
 #include <util/check.h>
+#include <util/system.h>
 #include <util/time.h>
+#include <util/translation.h>
 
 #include <optional>
 #include <vector>
 
 /* A "source" is a source address from which we have received a bunch of other addresses. */
 
@@ -21,12 +26,23 @@ static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;
 static NetGroupManager EMPTY_NETGROUPMAN{std::vector<bool>()};
 static constexpr uint32_t ADDRMAN_CONSISTENCY_CHECK_RATIO{0};
 
 static std::vector<CAddress> g_sources;
 static std::vector<std::vector<CAddress>> g_addresses;
 
+std::unique_ptr<AddrMan> LoadAddrmanFromPeersDat()
+{
+    SelectBaseParams("main");
+    SelectParams("main");
+    gArgs.ForceSetArg("-datadir", "/tmp");
+    std::unique_ptr<AddrMan> addrman;
+    auto err = LoadAddrman(EMPTY_NETGROUPMAN, gArgs, addrman);
+    assert(!err.has_value());
+    return addrman;
+}
+
 static void CreateAddresses()
 {
     if (g_sources.size() > 0) { // already created
         return;
     }
 
@@ -69,19 +85,12 @@ static void FillAddrMan(AddrMan& addrman)
 {
     CreateAddresses();
 
     AddAddressesToAddrMan(addrman);
 }
 
-static CNetAddr ResolveIP(const std::string& ip)
-{
-    CNetAddr addr;
-    LookupHost(ip, addr, false);
-    return addr;
-}
-
 static CService ResolveService(const std::string& ip, uint16_t port = 0)
 {
     CService serv;
     Lookup(ip, serv, port, false);
     return serv;
 }
@@ -125,26 +134,24 @@ static void AddrManSelectFromAlmostEmpty(benchmark::Bench& bench)
         (void)addrman.Select();
     });
 }
 
 static void AddrManSelectByNetwork(benchmark::Bench& bench)
 {
-    AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
-
-    // add single I2P address to new table
-    CService i2p_service;
-    i2p_service.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p");
-    CAddress i2p_address(i2p_service, NODE_NONE);
-    i2p_address.nTime = Now<NodeSeconds>();
-    CNetAddr source = ResolveIP("252.2.2.2");
-    addrman.Add({i2p_address}, source);
-
-    FillAddrMan(addrman);
+    auto addrman = LoadAddrmanFromPeersDat();
+    std::cout << "addrman size total: " << addrman->Size() << std::endl
+              << "addrman size IPv4:  " << addrman->Size(NET_IPV4) << std::endl
+              << "addrman size IPv6:  " << addrman->Size(NET_IPV6) << std::endl
+              << "addrman size Tor:   " << addrman->Size(NET_ONION) << std::endl
+              << "addrman size I2P:   " << addrman->Size(NET_I2P) << std::endl
+              << "addrman size CJDNS: " << addrman->Size(NET_CJDNS) << std::endl;
 
     bench.run([&] {
-        (void)addrman.Select(/*new_only=*/false, NET_I2P);
+        //addrman->Select(/*new_only=*/false);
+        //addrman->Select(/*new_only=*/false, NET_I2P);
+        addrman->Select(/*new_only=*/false, NET_CJDNS);
     });
 }
 
 static void AddrManGetAddr(benchmark::Bench& bench)
 {
     AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};

</details>

Results:

Select(/*new_only=*/false): 0.265 microseconds, this is what master is doing before this PR

After this PR:

Select(/*new_only=*/false, NET_IPV4): 0.5 microseconds
Select(/*new_only=*/false, NET_IPV6): 1.7 microseconds
Select(/*new_only=*/false, NET_ONION): 3.1 microseconds
Select(/*new_only=*/false, NET_I2P): 27 microseconds
Select(/*new_only=*/false, NET_CJDNS): 1600 microseconds

Is this slowdown acceptable? Maybe yes. We don't call Select in a tight loop, so maybe even the 1600 microseconds is ok.

What happens if we mark and subsequently skip buckets we have seen to contain 0 interesting addresses, without the shuffling? I guess that should bring most of the improvement from the above patch without the downside of the shuffling. Here is the change on top of this PR:

<details> <summary>[patch] Skip boring</summary>

diff --git i/src/addrman.cpp w/src/addrman.cpp
index cdfd079fcd..46f4de99be 100644
--- i/src/addrman.cpp
+++ w/src/addrman.cpp
@@ -747,15 +747,19 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
     }
 
     const int bucket_count{search_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT};
 
     // Loop through the addrman table until we find an appropriate entry
     double chance_factor = 1.0;
+    std::unordered_set<int> already_visited_and_boring_buckets;
     while (1) {
         // Pick a bucket, and an initial position in that bucket.
         int bucket = insecure_rand.randrange(bucket_count);
+        if (already_visited_and_boring_buckets.count(bucket) > 0) {
+            continue;
+        }
         int initial_position = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
 
         // Iterate over the positions of that bucket, starting at the initial one,
         // and looping around.
         int i;
         for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
@@ -770,14 +774,19 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
                 } else {
                     break;
                 }
             }
         }
 
-        // If the bucket is entirely empty, start over with a (likely) different one.
-        if (i == ADDRMAN_BUCKET_SIZE) continue;
+        // Start over with a different bucket if this one is entirely empty or
+        // specific network was requested and it does not contain any addresses
+        // from that network.
+        if (i == ADDRMAN_BUCKET_SIZE) {
+            already_visited_and_boring_buckets.insert(bucket);
+            continue;
+        }
 
         // Find the entry to return.
         int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
         int nId = GetEntry(search_tried, bucket, position);
         const auto it_found{mapInfo.find(nId)};
         assert(it_found != mapInfo.end());

</details>

Result for NET_CJDNS where the slowdown is most pronounced: 1200 microseconds (vs 1600). Those numbers are averaged over many runs and they do not tell the whole story. Here are the distributions without "skip boring" (the height of a bar indicates how many addresses were checked before finding the result, e.g. if the bar that spans between 2500 and 3500 is tall 280, it means 280 of the runs checked 2500-3500 addresses before finding the result).

with "skip boring":

ajtowns commented at 2:47 AM on April 6, 2023:

I don't see why 1.6 milliseconds to choose an address would be a concern.

ajtowns commented at 3:16 AM on April 6, 2023:

A simpler approach to skipping already tried buckets might be something like this:

    int n = 1 << 8; // power of 2
    int initial_pos = random(n);
    int step = random(n/2) * 2 + 1; // odd number
    int i = initial_pos;
    do {
        if (check_bucket(i)) break;
        i = (i + step) % n; // oops, try a new bucket
    } while (i != initial_pos);

That doesn't need a full shuffle, still hits every bucket, and only hits each bucket once (provided step and n are co-prime).

Based on vasild's stats above, I think this is fine to merge without any improvement here, but it might be interesting to rebench with this approach -- performance should benefit a little from needing less randomness as well as avoiding repeated work...

If we wanted to making things actually reliably fast for cjdns-like networks lost in a sea of ipv4 addresses, then having separate addrman tables for each network might be worth exploring. That would probably help prevent an attacker from blocking valid ipv4 addresses with a mass of valid-but-sybiled tor addresses, which might be worthwhile.

vasild commented at 11:22 AM on March 9, 2023: contributor

Approach ACK 25a64a20749f10ce84060f3570ad76d1a4776948

Feel free to ignore the nits below. I will try to assess the performance of the new Select(network).

Thanks for working on this!

amitiuttarwar force-pushed on Mar 11, 2023

amitiuttarwar commented at 6:38 PM on March 11, 2023: contributor

thank you for the in-depth review @vasild ! I have addressed all your review comments (incorporated most, left questions on a couple)

vasild approved

vasild commented at 2:36 PM on March 15, 2023: contributor

ACK 09d514583f15860f3bc7ae0c89e640c94fae3c71

Suggestions remaining (non-blocker):

Check out-of-bounds array access in AddrManImpl::GetEntry().
I can't find where the suggested test was added.
Performance wise I think the change in #27214 (review) would be very nice to have.

Thanks!

DrahtBot requested review from brunoerg on Mar 15, 2023

addrman: Introduce helper to generalize looking up an addrman entry

Unused until later commit.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

052fbcd5a7

refactor: generalize select logic

in preparation for consolidating the logic for searching the new and tried
tables, generalize the call paths for both

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

ca2a9c5f8f

refactor: consolidate select logic for new and tried tables

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

48806412e2

scripted-diff: rename local variables to match modern conventions

-BEGIN VERIFY SCRIPT-
sed -i 's/fChanceFactor/chance_factor/g' src/addrman.cpp
sed -i 's/nBucketPos/initial_position/g' src/addrman.cpp
sed -i 's/nBucket/bucket/g' src/addrman.cpp src/addrman_impl.h
sed -i 's/newOnly/new_only/g' src/addrman.cpp src/addrman_impl.h src/addrman.h src/test/addrman_tests.cpp
-END VERIFY SCRIPT-

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

26c3bf11e2

addrman: add functionality to select by network

Add an optional parameter to the addrman Select function that allows callers to
specify which network the returned address should be on. Ensure that the proper
table is selected with different cases of whether the new or tried table has
network addresses that match.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

6b229284fd

tests: add addrman_select_by_network test

this adds coverage for the 7 different cases of which table should be selected
when the network is specified. the different cases are the result of new_only
being true or false and whether there are network addresses on both, neither,
or one of new vs tried tables. the only case not covered is when new_only is
false and the only network addresses are on the new table.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

5c8b4baff2

test: add addrman test for special case

if an addr matching the network requirements is only on the new table and
select is invoked with new_only = false, ensure that the code selects the new
table.

in order to test this case, we use a non deterministic addrman. this means we
cannot have more than one address in any addrman table, or risk sporadic
failures when the second address happens to conflict.

if the code chose a table at random, the test would fail 50% of the time

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

a98e542e0c

test: increase coverage of addrman select (without network)

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

22a4d1489c

bench: add coverage for addrman select with network parameter

to evaluate the worst case performance with the network parameter passed
through, fill the new table with addresses then add a singular I2P address to
retrieve

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

9b91aae085

bench: test select for a new table with only one address

the addrman select function will demonstrate it's worst case performance when
it is almost empty, because it might have to linearly search several buckets.
add a bench test to cover this case

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

b0010c83a1

doc: clarify new_only param for Select function

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

17e705428d

amitiuttarwar force-pushed on Mar 18, 2023

amitiuttarwar commented at 1:10 AM on March 18, 2023: contributor

two small updates:

added assertions for bounds checking in GetEntry
added the suggested test

in src/addrman.cpp:760 in 17e705428d

 811 | +        int bucket = insecure_rand.randrange(bucket_count);
 812 | +        int initial_position = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
 813 | +
 814 | +        // Iterate over the positions of that bucket, starting at the initial one,
 815 | +        // and looping around.
 816 | +        int i;

brunoerg commented at 2:11 PM on March 24, 2023:

Just a suggestion to make the code a lit bit cleaner in case you have to touch it again, perhaps we don't need to create position and nId again after the loop.

diff --git a/src/addrman.cpp b/src/addrman.cpp
index cdfd079fc..30ce2cadc 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -757,10 +757,10 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
 
         // Iterate over the positions of that bucket, starting at the initial one,
         // and looping around.
-        int i;
+        int i, position, node_id;
         for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
-            int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
-            int node_id = GetEntry(search_tried, bucket, position);
+            position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
+            node_id = GetEntry(search_tried, bucket, position);
             if (node_id != -1) {
                 if (network.has_value()) {
                     const auto it{mapInfo.find(node_id)};
@@ -777,9 +777,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
         if (i == ADDRMAN_BUCKET_SIZE) continue;
 
         // Find the entry to return.
-        int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
-        int nId = GetEntry(search_tried, bucket, position);
-        const auto it_found{mapInfo.find(nId)};
+        const auto it_found{mapInfo.find(node_id)};
         assert(it_found != mapInfo.end());
         const AddrInfo& info{it_found->second};

amitiuttarwar commented at 7:23 PM on May 24, 2023:

done in #27745

vasild approved

vasild commented at 5:15 AM on March 25, 2023: contributor

ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6

Maybe other reviewers would be interested in the performance discussion: #27214 (review)

DrahtBot requested review from brunoerg on Mar 25, 2023

brunoerg approved

brunoerg commented at 3:36 PM on March 29, 2023: contributor

re-ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6

in src/addrman.h:152 in 17e705428d

 145 | @@ -146,11 +146,14 @@ class AddrMan
 146 |      /**
 147 |       * Choose an address to connect to.
 148 |       *
 149 | -     * @param[in] newOnly  Whether to only select addresses from the new table.
 150 | +     * @param[in] new_only Whether to only select addresses from the new table. Passing `true` returns
 151 | +     *                     an address from the new table or an empty pair. Passing `false` will return an
 152 | +     *                     address from either the new or tried table (it does not guarantee a tried entry).
 153 | +     * @param[in] network  Select only addresses of this network (nullopt = all)

ajtowns commented at 1:13 AM on April 13, 2023:

Probably worth documenting that passing in a network here rather than nullopt can make the search significantly slower (though still fast)?

amitiuttarwar commented at 7:23 PM on May 24, 2023:

done in #27745

in src/addrman.cpp:767 in 17e705428d

 818 | +            int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
 819 | +            int node_id = GetEntry(search_tried, bucket, position);
 820 | +            if (node_id != -1) {
 821 | +                if (network.has_value()) {
 822 | +                    const auto it{mapInfo.find(node_id)};
 823 | +                    assert(it != mapInfo.end());

ajtowns commented at 1:25 AM on April 13, 2023:

Do we really want an unconditional assertion failure here, vs say:

if (Assume(it != mapInfo.end()) && it->second.GetNetwork() == *network) break;

(maybe a similar question for GetEntry)

amitiuttarwar commented at 7:23 PM on May 24, 2023:

done in #27745

in src/test/addrman_tests.cpp:243 in 17e705428d

 260 | +
 261 | +    // ensure that both new and tried table are selected from
 262 | +    bool new_selected{false};
 263 | +    bool tried_selected{false};
 264 | +
 265 | +    while (!new_selected || !tried_selected) {

ajtowns commented at 1:32 AM on April 13, 2023:

Seems like having:

int counter = 256;
while (--counter > 0 && (!new_selected || !tried_selected)) { ... }
BOOST_CHECK(new_selected);
BOOST_CHECK(tried_selected);

would be better here than an infinite loop leading to CI timeout on error. A one-in-2**256 chance of failure is already the security of bitcoin as a whole, after all; and if our randomness generator is biassed enough that it fails more than that, it's probably better that the test case fails anyway? That we use the deterministic addrman presumably means any failures here will always be deterministic, no?

amitiuttarwar commented at 7:23 PM on May 24, 2023:

done in #27745

ajtowns commented at 1:52 AM on April 13, 2023: contributor

ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6

in src/test/addrman_tests.cpp:268 in a98e542e0c outdated

 263 | +    BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source));
 264 | +
 265 | +    // add I2P address to the tried table
 266 | +    CAddress i2p_addr;
 267 | +    i2p_addr.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p");
 268 | +    BOOST_CHECK(addrman->Add({i2p_addr}, source));

mzumsande commented at 7:08 PM on April 17, 2023:

This non-deterministic test could fail intermittently, if i2p_addr collides with addr1 (I tried it, and it happened to me after ~4000 runs). It would be good to change the order, such that we first add an address to tried (via new), and only then add another addr to new when the first has been moved over to tried.

amitiuttarwar commented at 9:52 PM on April 17, 2023:

🤦‍♀️ yup, agreed. will do in follow-up

in src/addrman.h:151 in 17e705428d

 145 | @@ -146,7 +146,9 @@ class AddrMan
 146 |      /**
 147 |       * Choose an address to connect to.
 148 |       *
 149 | -     * @param[in] new_only Whether to only select addresses from the new table.
 150 | +     * @param[in] new_only Whether to only select addresses from the new table. Passing `true` returns
 151 | +     *                     an address from the new table or an empty pair. Passing `false` will return an
 152 | +     *                     address from either the new or tried table (it does not guarantee a tried entry).

mzumsande commented at 7:17 PM on April 17, 2023:

nit: mentioning the "empty pair" only in the first case makes it sound as if this wasn't one of the possible outcomes in the second case (but it is).

amitiuttarwar commented at 7:23 PM on May 24, 2023:

done in #27745

mzumsande commented at 7:29 PM on April 17, 2023: contributor

Code Review ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6

I reviewed the PR again - although I'm a coauthor, so not sure about the etiquette of ack'ing.

I think there is a small bug in the non-deterministic test that could make it fail on rare occasions and should be fixed - here or in a follow-up.

amitiuttarwar commented at 9:51 PM on April 17, 2023: contributor

thanks for the reviews! I'll address the outstanding comments in a followup

fanquake added this to the milestone 26.0 on Apr 18, 2023

fanquake commented at 8:27 AM on April 18, 2023: member

This will be merged after branch-off. It would be good to either get the follow up opened in advance, so we can avoid any time between merging, and the potential for intermittent CI failures, or, if that change is straight-forward enough, you could push an additional commit here, leaving the rest of the branch as-is.

amitiuttarwar force-pushed on Apr 19, 2023

amitiuttarwar commented at 2:11 AM on April 19, 2023: contributor

added an extra commit to ensure the non-deterministic test will not fail intermittently. will incorporate the outstanding feedback separately

amitiuttarwar force-pushed on Apr 19, 2023

DrahtBot added the label CI failed on Apr 19, 2023

achow101 merged this on Apr 20, 2023

achow101 closed this on Apr 20, 2023

amitiuttarwar commented at 2:55 AM on April 21, 2023: contributor

opened #27506 for the test fix

maflcko removed the label CI failed on Apr 21, 2023

fanquake referenced this in commit 49d07ea9a1 on Apr 21, 2023

sidhujag referenced this in commit 6004bb5ebe on Apr 21, 2023

sidhujag referenced this in commit 6b90637559 on Apr 21, 2023

achow101 referenced this in commit 6744d840df on Jun 30, 2023

sidhujag referenced this in commit f70e00d132 on Jul 1, 2023

bitcoin locked this on May 23, 2024