Skip to content

Conversation

@murchandamus
Copy link
Contributor

@murchandamus murchandamus commented Aug 9, 2025

First draft for a Coin Selection Benchmark that doesn’t just test a worst case of one of the algorithms but tries to select inputs for a variety of different targets.

@DrahtBot DrahtBot added the Tests label Aug 9, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 9, 2025

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33160.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK l0rinc

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:

  • #34006 (Add util::Expected (std::expected) by maflcko)
  • #33657 (rest: allow reading partial block data from storage by romanz)

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
Copy link
Contributor

DrahtBot commented Aug 9, 2025

🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/runs/47721081968
LLM reason (✨ experimental): clang-tidy detected a performance-inefficient vector operation error, causing the CI to fail.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

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

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

  • An intermittent issue.

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

@murchandamus murchandamus force-pushed the 2025-08-improve-coinselection-bench branch from 6001a67 to 2dfb6d5 Compare August 18, 2025 18:52
Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

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

Concept ACK, the existing test were all quite trivial compared to this new one.
I left a ton of comments, for simplicity here's the final code that I'm suggesting, feel free to pick and choose from it.

Details
// Copyright (c) 2012-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 <bench/bench.h>
#include <consensus/amount.h>
#include <interfaces/chain.h>
#include <node/context.h>
#include <outputtype.h>
#include <policy/feerate.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <random.h>
#include <sync.h>
#include <util/result.h>
#include <wallet/coinselection.h>
#include <wallet/spend.h>
#include <wallet/test/util.h>
#include <wallet/transaction.h>
#include <wallet/wallet.h>

#include <cassert>
#include <map>
#include <memory>
#include <set>
#include <utility>
#include <vector>
#include <cmath>

using node::NodeContext;
using wallet::AttemptSelection;
using wallet::CHANGE_LOWER;
using wallet::COutput;
using wallet::CWallet;
using wallet::CWalletTx;
using wallet::CoinEligibilityFilter;
using wallet::CoinSelectionParams;
using wallet::CreateMockableWalletDatabase;
using wallet::OutputGroup;
using wallet::SelectCoinsBnB;
using wallet::TxStateInactive;

namespace {
constexpr CAmount operator""_sat(uint64_t v) { return v; }
constexpr CAmount operator""_ksat(uint64_t v) { return v * 1'000; }
constexpr CAmount operator""_btc(uint64_t v) { return v * COIN; }
constexpr CAmount operator""_btc(long double v) { return std::llround(v * COIN); }

int rand_percentage(FastRandomContext& rng) { return rng.randrange(100); }
CAmount rand_range(FastRandomContext& rng, CAmount lo, CAmount hi) { return lo + rng.randrange(hi - lo + 1); }
} // namespace

static void addCoin(CAmount nValue, std::vector<std::unique_ptr<CWalletTx>>& wtxs)
{
    static int nextLockTime = 0;
    CMutableTransaction tx;
    tx.nLockTime = nextLockTime++; // so all transactions get different hashes
    tx.vout.resize(1);
    tx.vout[0].nValue = nValue;
    wtxs.push_back(std::make_unique<CWalletTx>(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
}

// Simple benchmark for wallet coin selection that exercises a worst-case
// scenario for Knapsack: All UTXOs are necessary, but it is not an exact
// match, so the only eligible input set is only discovered on the second pass
// after all random walks fail to produce a solution.
static void KnapsackWorstCase(benchmark::Bench& bench)
{
    NodeContext node;
    auto chain{interfaces::MakeChain(node)};
    CWallet wallet(chain.get(), "", CreateMockableWalletDatabase());
    std::vector<std::unique_ptr<CWalletTx>> wtxs;
    LOCK(wallet.cs_wallet);

    for (int i{0}; i < 1000; ++i) {
        addCoin(1000_btc, wtxs);
    }
    addCoin(3_btc, wtxs);

    // Create coins
    wallet::CoinsResult available_coins;
    for (const auto& wtx : wtxs) {
        const auto txout{wtx->tx->vout.at(0)};
        const int input_bytes{CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr)};
        const COutput output{COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, input_bytes, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/0};
        available_coins.coins[OutputType::BECH32].emplace_back(output);
    }

    const CoinEligibilityFilter filter_standard(1, 6, 0);
    FastRandomContext rand{/*fDeterministic=*/true};
    const CoinSelectionParams coin_selection_params{
        rand,
        /*change_output_size=*/34,
        /*change_spend_size=*/148,
        /*min_change_target=*/CHANGE_LOWER,
        /*effective_feerate=*/CFeeRate(20'000),
        /*long_term_feerate=*/CFeeRate(10'000),
        /*discard_feerate=*/CFeeRate(3000),
        /*tx_noinputs_size=*/0,
        /*avoid_partial=*/false,
    };
    auto group{wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard]};
    bench.run([&] {
        auto result{AttemptSelection(wallet.chain(), 1002.99_btc, group, coin_selection_params, /*allow_mixed_output_types=*/true)};
        assert(result);
        assert(result->GetSelectedValue() == 1003_btc);
        assert(result->GetInputSet().size() == 2);
    });
}

// This benchmark is based on a UTXO pool composed of 400 UTXOs. The UTXOs are
// pseudorandomly generated to be of the four relevant output types P2PKH,
// P2SH-P2WPKH, P2WPKH, and P2TR UTXOs, and fall in the range of 10'000 sats to
// 1₿ with larger amounts being more likely.
// This UTXO pool is used to run coin selection for 100 pseudorandom selection
// targets from 0.1–2₿. Altogether, this gives us a deterministic benchmark
// with a hopefully somewhat representative coin selection scenario.
static void CoinSelectionOnDiverseWallet(benchmark::Bench& bench)
{
    FastRandomContext rng{/*fDeterministic=*/true};

    NodeContext node;
    auto chain{interfaces::MakeChain(node)};
    CWallet wallet(chain.get(), "", CreateMockableWalletDatabase());
    LOCK(wallet.cs_wallet);

    std::vector<std::unique_ptr<CWalletTx>> wtxs;
    wtxs.reserve(400);
    for (size_t i{0}; i < wtxs.capacity(); ++i) {
        const int p{rand_percentage(rng)};
        const auto val{p < 50 ? rand_range(rng, 10_ksat, 100_ksat) :
                       p < 75 ? rand_range(rng, 100_ksat, 1000_ksat) :
                       p < 95 ? rand_range(rng, 1000_ksat, 1_btc) :
                                rand_range(rng, 0.1_btc, 1_btc)};
        addCoin(val, wtxs);
    }

    // Create coins
    wallet::CoinsResult available_coins;
    for (const auto& wtx : wtxs) {
        const int p{rand_percentage(rng)};
        auto val{p < 35 ? OutputType::LEGACY :
                 p < 55 ? OutputType::P2SH_SEGWIT :
                 p < 90 ? OutputType::BECH32 :
                          OutputType::BECH32M};

        const auto txout{wtx->tx->vout.at(0)};
        const int input_bytes{CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr)};
        const COutput output{COutPoint{wtx->GetHash(), 0}, txout, /*depth=*/6 * 24, input_bytes, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/0};
        available_coins.coins[val].emplace_back(output);
    }

    std::vector<CAmount> targets;
    targets.reserve(10);
    for (size_t i{0}; i < targets.capacity(); ++i) {
        targets.push_back(rand_range(rng, 0.1_btc, 1_btc));
    }

    const CoinSelectionParams coin_selection_params{
        rng,
        /*change_output_size=*/31,
        /*change_spend_size=*/68,
        /*min_change_target=*/CHANGE_LOWER,
        /*effective_feerate=*/CFeeRate(20'000),
        /*long_term_feerate=*/CFeeRate(10'000),
        /*discard_feerate=*/CFeeRate(3000),
        /*tx_noinputs_size=*/72,
        /*avoid_partial=*/false,
    };
    const CoinEligibilityFilter filter_standard(1, 6, 0);
    auto group{wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard]};

    bench.run([&] {
        for (const auto& target : targets) {
            auto result{AttemptSelection(wallet.chain(), target, group, coin_selection_params, /*allow_mixed_output_types=*/true)};
            assert(result && result->GetSelectedValue() >= target);
        }
    });
}

static void add_coin(CAmount nValue, uint32_t nInput, std::vector<OutputGroup>& set)
{
    CMutableTransaction tx;
    tx.vout.resize(nInput + 1);
    tx.vout[nInput].nValue = nValue;
    COutput output(COutPoint{tx.GetHash(), nInput}, tx.vout.at(nInput), /*depth=*/0, /*input_bytes=*/-1, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/true, /*fees=*/0);
    set.emplace_back();
    set.back().Insert(std::make_shared<COutput>(output), /*ancestors=*/0, /*descendants=*/0);
}

static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)
{
    utxo_pool.clear();
    CAmount target{0};
    for (int i{0}; i < utxos; ++i) {
        target += 1_sat << (utxos + i);
        add_coin(1_sat << (utxos + i), 2 * i, utxo_pool);
        add_coin((1_sat << (utxos + i)) + (1_sat << (utxos - 1 - i)), 2 * i + 1, utxo_pool);
    }
    return target;
}

static void BnBExhaustion(benchmark::Bench& bench)
{
    std::vector<OutputGroup> utxo_pool;
    auto target{make_hard_case(17, utxo_pool)};
    bench.run([&] {
        SelectCoinsBnB(utxo_pool, target, 0, MAX_STANDARD_TX_WEIGHT); // Should exhaust
    });
}

BENCHMARK(KnapsackWorstCase, benchmark::PriorityLevel::HIGH);
BENCHMARK(CoinSelectionOnDiverseWallet, benchmark::PriorityLevel::HIGH);
BENCHMARK(BnBExhaustion, benchmark::PriorityLevel::HIGH);

int x{det_rand.randrange(100)};
if (x < 50) {
// 0.0001–0.001 COIN
addCoin(det_rand.randrange(90'000) + 10'000, wallet, wtxs);
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't actually need the wallet here, right?

}

// Allow actual randomness for selection
FastRandomContext rand{};
Copy link
Contributor

Choose a reason for hiding this comment

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

we want deterministic randomness for benchmarks, otherwise it's hard to know what we're actually measuring:

Suggested change
FastRandomContext rand{};
FastRandomContext rand{/*fDeterministic=*/true};

}

std::vector<CAmount> targets;
for (int i = 0; i < 100; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we could reserve this to avoid the build failure:

std::vector<CAmount> targets;
targets.reserve(10);
for (size_t i{0}; i < targets.capacity(); ++i) {
    targets.push_back(rand_range(rng, 0.1_btc, 1_btc));
}

Note that I have reduced the target count to 10 since the benchmark was very slow otherwise

// This benchmark is based on a UTXO pool composed of 400 UTXOs. The UTXOs are
// pseudorandomly generated to be of the four relevant output types P2PKH,
// P2SH-P2WPKH, P2WPKH, and P2TR UTXOs, and fall in the range of 10'000 sats to
// 1 ₿ with larger amounts being more likely.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid the Narrow No-Break Space chars from docs if possible, they're rendered differently on different mediums

Comment on lines +114 to +116
// Use arbitrary static seed for generating a pseudorandom scenario
uint256 arb_seed = uint256("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
FastRandomContext det_rand{arb_seed};
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, seems simpler to just leave it to deterministic instead of hard-coding a confusing seed

Suggested change
// Use arbitrary static seed for generating a pseudorandom scenario
uint256 arb_seed = uint256("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
FastRandomContext det_rand{arb_seed};
FastRandomContext det_rand{/*fDeterministic=*/true};

note: can we reuse this for the random source below as well?


CAmount target = make_hard_case(17, utxo_pool);
bench.run([&] {
// Benchmark
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't have to keep this comment either in 2dfb6d5

bench.run([&] {
// Benchmark
CAmount target = make_hard_case(17, utxo_pool);
SelectCoinsBnB(utxo_pool, target, 0, MAX_STANDARD_TX_WEIGHT); // Should exhaust
Copy link
Contributor

@l0rinc l0rinc Aug 26, 2025

Choose a reason for hiding this comment

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

Should exhaust - can we assert that instead of adding a dead comment?

// P2SH-P2WPKH, P2WPKH, and P2TR UTXOs, and fall in the range of 10'000 sats to
// 1 ₿ with larger amounts being more likely.
// This UTXO pool is used to run coin selection for 100 pseudorandom selection
// targets from 0.1–2 ₿. Altogether, this gives us a deterministic benchmark
Copy link
Contributor

Choose a reason for hiding this comment

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

where are we adding 2 btc? Could we avoid values from the comments, they're just adding a maintenance burden when we're forgetting updating them


bench.run([&] {
for (const auto& target : targets) {
auto result = AttemptSelection(wallet.chain(), target, group, coin_selection_params, /*allow_mixed_output_types=*/true);
Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, Knapsack and CoinsBnB are really slow (with debug build to avoid all the inlining), making this whole benchmark one of our heaviest - could we reduce some of the iterations?
image

const CoinEligibilityFilter filter_standard(1, 6, 0);
const CoinSelectionParams coin_selection_params{
rand,
/*change_output_size=*/ 31,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's unify the comment styles across the file

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 9, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants