ronantakizawa/codereview-bench huggingface dataset:
Repository: bitcoin/bitcoin
File: src/node/interfaces.cpp (C++)

Code before:
#include <policy/rbf.h>
#include <policy/settings.h>
#include <primitives/block.h>
#include <primitives/transaction.h>
#include <rpc/protocol.h>
#include <rpc/server.h>
#include <support/allocators/secure.h>
#include <sync.h>
#include <txmempool.h>
#include <uint256.h>
#include <univalue.h>
#include <util/check.h>
#include <util/signalinterrupt.h>
#include <util/translation.h>
#include <validation.h>
#include <validationinterface.h>
#include <warnings.h>

#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif

#include <any>
#include <memory>
#include <optional>
#include <utility>

#include <boost/signals2/signal.hpp>

using interfaces::BlockTip;
using interfaces::Chain;
using interfaces::FoundBlock;
using interfaces::Handler;
using interfaces::MakeSignalHandler;
using interfaces::Node;
using interfaces::WalletLoader;

namespace node {
// All members of the classes in this namespace are intentionally public, as the
// classes themselves are private.
namespace {
#ifdef ENABLE_EXTERNAL_SIGNER
class ExternalSignerImpl : public interfaces::ExternalSigner
{
public:
    ExternalSignerImpl(::ExternalSigner signer) : m_signer(std::move(signer)) {}
    std::string getName() override { return m_signer.m_name; }
    ::ExternalSigner m_signer;
};
#endif


Review comment (suggestion):
Nit: I believe this addition might be unnecessary, considering that other instances of `std::string` in the file seem to work fine without it

Code after:
#include <policy/rbf.h>
#include <policy/settings.h>
#include <primitives/block.h>
#include <primitives/transaction.h>
#include <rpc/protocol.h>
#include <rpc/server.h>
#include <support/allocators/secure.h>
#include <sync.h>
#include <txmempool.h>
#include <uint256.h>
#include <univalue.h>
#include <util/check.h>
#include <util/result.h>
#include <util/signalinterrupt.h>
#include <util/translation.h>
#include <validation.h>
#include <validationinterface.h>
#include <warnings.h>

#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif

#include <any>
#include <memory>
#include <optional>
#include <utility>

#include <boost/signals2/signal.hpp>

using interfaces::BlockTip;
using interfaces::Chain;
using interfaces::FoundBlock;
using interfaces::Handler;
using interfaces::MakeSignalHandler;
using interfaces::Node;
using interfaces::WalletLoader;

namespace node {
// All members of the classes in this namespace are intentionally public, as the
// classes themselves are private.
namespace {
#ifdef ENABLE_EXTERNAL_SIGNER
class ExternalSignerImpl : public interfaces::ExternalSigner
{
public:
    ExternalSignerImpl(::ExternalSigner signer) : m_signer(std::move(signer)) {}
    std::string getName() override { return m_signer.m_name; }
    ::ExternalSigner m_signer;
};
#endif
Repository: bitcoin/bitcoin
File: src/kernel/context.h (C/C++)

Code before:
// Copyright (c) 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.

#ifndef BITCOIN_KERNEL_CONTEXT_H
#define BITCOIN_KERNEL_CONTEXT_H

#include <util/signalinterrupt.h>

#include <memory>

namespace kernel {
//! Context struct holding the kernel library's logically global state, and
//! passed to external libbitcoin_kernel functions which need access to this
//! state. The kernel library API is a work in progress, so state organization
//! and member list will evolve over time.
//!
//! State stored directly in this struct should be simple. More complex state
//! should be stored to std::unique_ptr members pointing to opaque types.
struct Context {
    Context();
    ~Context(); // NOLINT(performance-trivially-destructible)
};
} // namespace kernel

#endif // BITCOIN_KERNEL_CONTEXT_H


Review comment (suggestion):
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)

Is there a reason to suppress the lint warning instead of just deleting the destructor, since the destructor does not do anything? If the destructor is needed for some reason, it might be good to clarify in a comment.

Code after:
// Copyright (c) 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.

#ifndef BITCOIN_KERNEL_CONTEXT_H
#define BITCOIN_KERNEL_CONTEXT_H

namespace kernel {
//! Context struct holding the kernel library's logically global state, and
//! passed to external libbitcoin_kernel functions which need access to this
//! state. The kernel library API is a work in progress, so state organization
//! and member list will evolve over time.
//!
//! State stored directly in this struct should be simple. More complex state
//! should be stored to std::unique_ptr members pointing to opaque types.
struct Context {
    Context();
};
} // namespace kernel

#endif // BITCOIN_KERNEL_CONTEXT_H
Repository: bitcoin/bitcoin
File: src/node/context.cpp (C++)

Code before:
// Copyright (c) 2019-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 <node/context.h>

#include <addrman.h>
#include <banman.h>
#include <interfaces/chain.h>
#include <kernel/context.h>
#include <key.h>
#include <net.h>
#include <net_processing.h>
#include <netgroup.h>
#include <node/kernel_notifications.h>
#include <policy/fees.h>
#include <scheduler.h>
#include <txmempool.h>
#include <validation.h>

namespace node {
NodeContext::NodeContext()
{
    ECC_Start();
}

NodeContext::~NodeContext()
{
    ECC_Stop();
}

} // namespace node


Review comment (suggestion):
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)

I don't think it's a great idea to add these ECC_Start / ECC_Stop calls here, because this prevents writing lighter-weight tests that may need to pass a `NodeContext` argument to RPC or Init functions, but don't need ECC. Also calling ECC_Start in the constructor makes it not possible for different NodeContext instances to be constructed at the same time. So for example you couldn't write a test calling RPC or Init functions with different context objects. It also clashes a little with the `NodeContext` documentation which says it "isn't intended to have any member functions", just to be "a collection of references."

 I think using RAII to initialize ECC state it good, but it should happen in separate class, rather than NodeContext. 

Code after:
// Copyright (c) 2019-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 <node/context.h>

#include <addrman.h>
#include <banman.h>
#include <interfaces/chain.h>
#include <kernel/context.h>
#include <key.h>
#include <net.h>
#include <net_processing.h>
#include <netgroup.h>
#include <node/kernel_notifications.h>
#include <policy/fees.h>
#include <scheduler.h>
#include <txmempool.h>
#include <validation.h>
#include <validationinterface.h>

namespace node {
NodeContext::NodeContext() = default;
NodeContext::~NodeContext() = default;
} // namespace node
Repository: bitcoin/bitcoin
File: src/bench/streams_findbyte.cpp (C++)

Code before:

#include <streams.h>
#include <util/fs.h>

#include <cstddef>
#include <cstdint>
#include <cstdio>

static void FindByte(benchmark::Bench& bench)
{
    // Setup
    AutoFile file{fsbridge::fopen("streams_tmp", "w+b")};
    const size_t file_size = 200;
    uint8_t data[file_size] = {0};
    data[file_size-1] = 1;
    file << data;
    file.seek(0, SEEK_SET);
    BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size};

    bench.run([&] {
        bf.SetPos(0);
        bf.FindByte(std::byte(1));
    });

    // Cleanup
    file.fclose();
    fs::remove("streams_tmp");
}

BENCHMARK(FindByte, benchmark::PriorityLevel::HIGH);


Review comment (suggestion):
Rather than silence errors, it might be good to at least assert against errors here and elsewhere in benchmarks/tests as we want to ensure we are benchmarking/testing properly? (Not for some fuzz-cases though [as you already found out](https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1465997866)).
```suggestion
    assert(file.fclose() != 0);
```
That's already how you handled *src/wallet/test/fuzz/wallet_bdb_parser.cpp*.

Code after:

#include <streams.h>
#include <util/fs.h>

#include <cstddef>
#include <cstdint>
#include <cstdio>

static void FindByte(benchmark::Bench& bench)
{
    // Setup
    AutoFile file{fsbridge::fopen("streams_tmp", "w+b")};
    const size_t file_size = 200;
    uint8_t data[file_size] = {0};
    data[file_size-1] = 1;
    file << data;
    file.seek(0, SEEK_SET);
    BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size};

    bench.run([&] {
        bf.SetPos(0);
        bf.FindByte(std::byte(1));
    });

    // Cleanup
    assert(file.fclose() == 0);
    fs::remove("streams_tmp");
}

BENCHMARK(FindByte, benchmark::PriorityLevel::HIGH);
Repository: bitcoin/bitcoin
File: src/rpc/blockchain.cpp (C++)

Code before:
#include <net.h>
#include <net_processing.h>
#include <node/blockstorage.h>
#include <node/context.h>
#include <node/transaction.h>
#include <node/utxo_snapshot.h>
#include <node/warnings.h>
#include <primitives/transaction.h>
#include <rpc/server.h>
#include <rpc/server_util.h>
#include <rpc/util.h>
#include <script/descriptor.h>
#include <serialize.h>
#include <streams.h>
#include <sync.h>
#include <txdb.h>
#include <txmempool.h>
#include <undo.h>
#include <univalue.h>
#include <util/check.h>
#include <util/fs.h>
#include <util/strencodings.h>
#include <util/syserror.h>
#include <util/translation.h>
#include <validation.h>
#include <validationinterface.h>
#include <versionbits.h>

#include <stdint.h>

#include <condition_variable>
#include <memory>
#include <mutex>
#include <optional>

using kernel::CCoinsStats;
using kernel::CoinStatsHashType;

using interfaces::Mining;
using node::BlockManager;
using node::NodeContext;
using node::SnapshotMetadata;
using util::MakeUnorderedList;

std::tuple<std::unique_ptr<CCoinsViewCursor>, CCoinsStats, const CBlockIndex*>
PrepareUTXOSnapshot(
    Chainstate& chainstate,
    const std::function<void()>& interruption_point = {})
    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

UniValue WriteUTXOSnapshot(

Review comment (suggestion):
Should also explicitly `fclose()` `afile` from line 2790/2791.

https://github.com/bitcoin/bitcoin/blob/dba783538683cfc6af209c640c2d019648493f31/src/rpc/blockchain.cpp#L2790-L2791

Code after:
#include <net.h>
#include <net_processing.h>
#include <node/blockstorage.h>
#include <node/context.h>
#include <node/transaction.h>
#include <node/utxo_snapshot.h>
#include <node/warnings.h>
#include <primitives/transaction.h>
#include <rpc/server.h>
#include <rpc/server_util.h>
#include <rpc/util.h>
#include <script/descriptor.h>
#include <serialize.h>
#include <streams.h>
#include <sync.h>
#include <txdb.h>
#include <txmempool.h>
#include <undo.h>
#include <univalue.h>
#include <util/check.h>
#include <util/fs.h>
#include <util/strencodings.h>
#include <util/syserror.h>
#include <util/translation.h>
#include <validation.h>
#include <validationinterface.h>
#include <versionbits.h>

#include <cstdint>

#include <condition_variable>
#include <iterator>
#include <memory>
#include <mutex>
#include <optional>
#include <vector>

using kernel::CCoinsStats;
using kernel::CoinStatsHashType;

using interfaces::BlockRef;
using interfaces::Mining;
using node::BlockManager;
using node::NodeContext;
using node::SnapshotMetadata;
using util::MakeUnorderedList;

std::tuple<std::unique_ptr<CCoinsViewCursor>, CCoinsStats, const CBlockIndex*>
PrepareUTXOSnapshot(
    Chainstate& chainstate,
    const std::function<void()>& interruption_point = {})
Repository: bitcoin/bitcoin
File: contrib/seeds/generate-seeds.py (Python)

Code before:
    if match: # ipv6
        host = match.group(1)
        port = match.group(2)
    elif s.count(':') > 1: # ipv6, no port
        host = s
        port = ''
    else:
        (host,_,port) = s.partition(':')

    if not port:
        port = 0
    else:
        port = int(port)

    host = name_to_bip155(host)

    if host[0] == BIP155Network.TORV2:
        return None  # TORV2 is no longer supported, so we ignore it
    else:
        return host + (port, )

def ser_compact_size(l):
    r = b""
    if l < 253:
        r = struct.pack("B", l)
    elif l < 0x10000:
        r = struct.pack("<BH", 253, l)
    elif l < 0x100000000:
        r = struct.pack("<BI", 254, l)
    else:
        r = struct.pack("<BQ", 255, l)
    return r

def bip155_serialize(spec):
    '''
    Serialize (networkID, addr, port) tuple to BIP155 binary format.
    '''
    r = b""
    r += struct.pack('B', spec[0].value)
    r += ser_compact_size(len(spec[1]))
    r += spec[1]
    r += struct.pack('>H', spec[2])
    return r

def process_nodes(g, f, structname):
    g.write('static const uint8_t %s[] = {\n' % structname)
    for line in f:
        comment = line.find('#')
        if comment != -1:
            line = line[0:comment]
        line = line.strip()

Review comment (suggestion):
nit: for serializing single bytes I personally prefer `bytes([value])` over the `.to_bytes` method, e.g.
```suggestion
        r = bytes([253]) + l.to_bytes(2, "little")
```
(but no blocker, feel free to ignore)

Code after:
        host = match.group(1)
        port = match.group(2)
    elif s.count(':') > 1: # ipv6, no port
        host = s
        port = ''
    else:
        (host,_,port) = s.partition(':')

    if not port:
        port = 0
    else:
        port = int(port)

    host = name_to_bip155(host)

    if host[0] == BIP155Network.TORV2:
        return None  # TORV2 is no longer supported, so we ignore it
    else:
        return host + (port, )

def ser_compact_size(l):
    r = b""
    if l < 253:
        r = l.to_bytes(1, "little")
    elif l < 0x10000:
        r = (253).to_bytes(1, "little") + l.to_bytes(2, "little")
    elif l < 0x100000000:
        r = (254).to_bytes(1, "little") + l.to_bytes(4, "little")
    else:
        r = (255).to_bytes(1, "little") + l.to_bytes(8, "little")
    return r

def bip155_serialize(spec):
    '''
    Serialize (networkID, addr, port) tuple to BIP155 binary format.
    '''
    r = b""
    r += spec[0].value.to_bytes(1, "little")
    r += ser_compact_size(len(spec[1]))
    r += spec[1]
    r += spec[2].to_bytes(2, "big")
    return r

def process_nodes(g, f, structname):
    g.write('static const uint8_t %s[] = {\n' % structname)
    for line in f:
        comment = line.find('#')
        if comment != -1:
            line = line[0:comment]
        line = line.strip()
        if not line:
Repository: bitcoin/bitcoin
File: src/addrman.cpp (C++)

Code before:

size_t AddrManImpl::Size_(std::optional<Network> net, std::optional<bool> in_new) const
{
    AssertLockHeld(cs);

    if (!net.has_value()) {
        if (in_new.has_value()) {
            return *in_new ? nNew : nTried;
        } else {
            return vRandom.size();
        }
    }
    if (auto it = m_network_counts.find(*net); it != m_network_counts.end()) {
        auto net_count = it->second;
        if (in_new.has_value()) {
            return *in_new ? net_count.n_new : net_count.n_tried;
        } else {
            return net_count.n_new + net_count.n_tried;
        }
    }
    return 0;
}

void AddrManImpl::Check() const
{
    AssertLockHeld(cs);

    // Run consistency checks 1 in m_consistency_check_ratio times if enabled
    if (m_consistency_check_ratio == 0) return;
    if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return;

    const int err{CheckAddrman()};
    if (err) {
        LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
        assert(false);
    }
}

int AddrManImpl::CheckAddrman() const
{
    AssertLockHeld(cs);

    LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(
        strprintf("new %i, tried %i, total %u", nNew, nTried, vRandom.size()), BCLog::ADDRMAN);

    std::unordered_set<int> setTried;
    std::unordered_map<int, int> mapNew;
    std::unordered_map<Network, NewTriedCount> local_counts;

    if (vRandom.size() != (size_t)(nTried + nNew))
        return -7;

Review comment (suggestion):
nit: [`contains`](https://en.cppreference.com/w/cpp/container/unordered_set/contains) is equivalent but more semantically correct here

Code after:
                               /*multiplicity_in=*/addr_info->nRefCount,
                               /*bucket_in=*/bucket,
                               /*position_in=*/addr_info->GetBucketPosition(nKey, true, bucket));
    }
}

size_t AddrManImpl::Size_(std::optional<Network> net, std::optional<bool> in_new) const
{
    AssertLockHeld(cs);

    if (!net.has_value()) {
        if (in_new.has_value()) {
            return *in_new ? nNew : nTried;
        } else {
            return vRandom.size();
        }
    }
    if (auto it = m_network_counts.find(*net); it != m_network_counts.end()) {
        auto net_count = it->second;
        if (in_new.has_value()) {
            return *in_new ? net_count.n_new : net_count.n_tried;
        } else {
            return net_count.n_new + net_count.n_tried;
        }
    }
    return 0;
}

void AddrManImpl::Check() const
{
    AssertLockHeld(cs);

    // Run consistency checks 1 in m_consistency_check_ratio times if enabled
    if (m_consistency_check_ratio == 0) return;
    if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return;

    const int err{CheckAddrman()};
    if (err) {
        LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
        assert(false);
    }
}

int AddrManImpl::CheckAddrman() const
{
    AssertLockHeld(cs);

    LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(
        strprintf("new %i, tried %i, total %u", nNew, nTried, vRandom.size()), BCLog::ADDRMAN);

    std::unordered_set<int> setTried;
Repository: bitcoin/bitcoin
File: src/addrman.cpp (C++)

Code before:
    Check();
}

void AddrManImpl::ResolveCollisions()
{
    LOCK(cs);
    Check();
    ResolveCollisions_();
    Check();
}

std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision()
{
    LOCK(cs);
    Check();
    auto ret = SelectTriedCollision_();
    Check();
    return ret;
}

std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::optional<Network> network) const
{
    LOCK(cs);
    Check();
    auto addrRet = Select_(new_only, network);
    Check();
    return addrRet;
}

std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
{
    LOCK(cs);
    Check();
    auto addresses = GetAddr_(max_addresses, max_pct, network, filtered);
    Check();
    return addresses;
}

std::vector<std::pair<AddrInfo, AddressPosition>> AddrManImpl::GetEntries(bool from_tried) const
{
    LOCK(cs);
    Check();
    auto addrInfos = GetEntries_(from_tried);
    Check();
    return addrInfos;
}

void AddrManImpl::Connected(const CService& addr, NodeSeconds time)
{
    LOCK(cs);
    Check();

Review comment (suggestion):
Missing `#include <unordered_set>` in this file (include what you use)

```
/src/addrman.cpp should add these lines:
#include <algorithm>                // for max, min
#include <compare>                  // for operator<, strong_ordering, operator>, operator<=
#include <iterator>                 // for advance
#include <ratio>                    // for ratio
#include <set>                      // for set, _Rb_tree_const_iterator, operator==
#include <unordered_map>            // for unordered_map
#include "netgroup.h"               // for NetGroupManager
#include "sync.h"                   // for AssertLockHeldInternal, AssertLockHeld, LOCK, UniqueLock
```

Code after:
void AddrManImpl::Attempt(const CService& addr, bool fCountFailure, NodeSeconds time)
{
    LOCK(cs);
    Check();
    Attempt_(addr, fCountFailure, time);
    Check();
}

void AddrManImpl::ResolveCollisions()
{
    LOCK(cs);
    Check();
    ResolveCollisions_();
    Check();
}

std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision()
{
    LOCK(cs);
    Check();
    auto ret = SelectTriedCollision_();
    Check();
    return ret;
}

std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, const std::unordered_set<Network>& networks) const
{
    LOCK(cs);
    Check();
    auto addrRet = Select_(new_only, networks);
    Check();
    return addrRet;
}

std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
{
    LOCK(cs);
    Check();
    auto addresses = GetAddr_(max_addresses, max_pct, network, filtered);
    Check();
    return addresses;
}

std::vector<std::pair<AddrInfo, AddressPosition>> AddrManImpl::GetEntries(bool from_tried) const
{
    LOCK(cs);
    Check();
    auto addrInfos = GetEntries_(from_tried);
    Check();
    return addrInfos;
}
Repository: bitcoin/bitcoin
File: src/addrman.cpp (C++)

Code before:
    if (!pinfo)
        return;

    AddrInfo& info = *pinfo;

    // update info
    info.m_last_try = time;
    if (fCountFailure && info.m_last_count_attempt < m_last_good) {
        info.m_last_count_attempt = time;
        info.nAttempts++;
    }
}

std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::optional<Network> network) const
{
    AssertLockHeld(cs);

    if (vRandom.empty()) return {};

    size_t new_count = nNew;
    size_t tried_count = nTried;

    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;
    }

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

    // Decide if we are going to search the new or tried table
    // If either option is viable, use a 50% chance to choose
    bool search_tried;
    if (new_only || tried_count == 0) {
        search_tried = false;
    } else if (new_count == 0) {
        search_tried = true;
    } else {
        search_tried = insecure_rand.randbool();
    }

    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;
    while (1) {
        // Pick a bucket, and an initial position in that bucket.

Review comment (suggestion):
A `Network` is a cheaply-copied int enum; no need to use it by reference, unless I'm missing something.

```suggestion
        for (Network network : networks) {
```


Code after:
    if (!pinfo)
        return;

    AddrInfo& info = *pinfo;

    // update info
    info.m_last_try = time;
    if (fCountFailure && info.m_last_count_attempt < m_last_good) {
        info.m_last_count_attempt = time;
        info.nAttempts++;
    }
}

std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, const std::unordered_set<Network>& networks) const
{
    AssertLockHeld(cs);

    if (vRandom.empty()) return {};

    size_t new_count = nNew;
    size_t tried_count = nTried;

    if (!networks.empty()) {
        new_count = 0;
        tried_count = 0;
        for (auto& network : networks) {
            auto it = m_network_counts.find(network);
            if (it == m_network_counts.end()) {
                continue;
            }
            auto counts = it->second;
            new_count += counts.n_new;
            tried_count += counts.n_tried;
        }
    }

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

    // Decide if we are going to search the new or tried table
    // If either option is viable, use a 50% chance to choose
    bool search_tried;
    if (new_only || tried_count == 0) {
        search_tried = false;
    } else if (new_count == 0) {
        search_tried = true;
    } else {
        search_tried = insecure_rand.randbool();
    }

    const int bucket_count{search_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT};