Merge 44bab80284 into c5e44a0435

32d55e28af test: Use the correct node for doubled keypath test (Ava Chow)

Pull request description:

  #29124 had a silent merge conflict with #32350 which resulted in it using the wrong node. Fix the test to use the correct v22 node.

ACKs for top commit:
  maflcko:
    lgtm ACK 32d55e28af
  rkrux:
    ACK 32d55e28af
  BrandonOdiwuor:
    Code Review ACK 32d55e28af

Tree-SHA512: 1e0231985beb382b16e1d608c874750423d0502388db0c8ad450b22d17f9d96f5e16a6b44948ebda5efc750f62b60d0de8dd20131f449427426a36caf374af92

src/wallet/coinselection.cpp

@@ -55,11 +55,11 @@ struct {
  * cost of creating and spending a change output. The algorithm uses a depth-first search on a binary
  * tree. In the binary tree, each node corresponds to the inclusion or the omission of a UTXO. UTXOs
  * are sorted by their effective values and the tree is explored deterministically per the inclusion
- * branch first. At each node, the algorithm checks whether the selection is within the target range.
+ * branch first. For each new input set candidate, the algorithm checks whether the selection is within the target range.
  * While the selection has not reached the target range, more UTXOs are included. When a selection's
  * value exceeds the target range, the complete subtree deriving from this selection can be omitted.
  * At that point, the last included UTXO is deselected and the corresponding omission branch explored
- * instead. The search ends after the complete tree has been searched or after a limited number of tries.
+ * instead starting by adding the subsequent UTXO. The search ends after the complete tree has been searched or after a limited number of tries.
  *
  * The search continues to search for better solutions after one solution has been found. The best
  * solution is chosen by minimizing the waste metric. The waste metric is defined as the cost to
@@ -93,113 +93,161 @@ static const size_t TOTAL_TRIES = 100000;
 util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change,
                                              int max_selection_weight)
 {
-    SelectionResult result(selection_target, SelectionAlgorithm::BNB);
-    CAmount curr_value = 0;
-    std::vector<size_t> curr_selection; // selected utxo indexes
-    int curr_selection_weight = 0; // sum of selected utxo weight
+    std::sort(utxo_pool.begin(), utxo_pool.end(), descending);
+    // The sum of UTXO amounts after this UTXO index, e.g. lookahead[5] = Σ(UTXO[6+].amount)
+    std::vector<CAmount> lookahead(utxo_pool.size());
 
-    // Calculate curr_available_value
-    CAmount curr_available_value = 0;
-    for (const OutputGroup& utxo : utxo_pool) {
-        // Assert that this utxo is not negative. It should never be negative,
-        // effective value calculation should have removed it
-        assert(utxo.GetSelectionAmount() > 0);
-        curr_available_value += utxo.GetSelectionAmount();
+    // Calculate lookahead values, and check that there are sufficient funds
+    CAmount total_available = 0;
+    for (size_t i = 0; i < utxo_pool.size(); ++i) {
+        size_t index = utxo_pool.size() - 1 - i; // Loop over every element in reverse order
+        lookahead[index] = total_available;
+        // UTXOs with non-positive effective value must have been filtered
+        Assume(utxo_pool[index].GetSelectionAmount() > 0);
+        total_available += utxo_pool[index].GetSelectionAmount();
     }
-    if (curr_available_value < selection_target) {
+
+    if (total_available < selection_target) {
+        // Insufficient funds
         return util::Error();
     }
 
-    // Sort the utxo_pool
-    std::sort(utxo_pool.begin(), utxo_pool.end(), descending);
 
-    CAmount curr_waste = 0;
+    // The current selection and the best input set found so far, stored as the utxo_pool indices of the UTXOs forming them
+    std::vector<size_t> curr_selection;
     std::vector<size_t> best_selection;
+
+    // The currently selected effective amount
+    CAmount curr_amount = 0;
+
+    // The waste score of the current section, and the best waste score so far
+    CAmount curr_selection_waste = 0;
     CAmount best_waste = MAX_MONEY;
 
-    bool is_feerate_high = utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee;
+    // The weight of the currently selected input set
+    int curr_weight = 0;
+
+    // Whether the input sets generated during this search have exceeded the maximum transaction weight at any point
     bool max_tx_weight_exceeded = false;
 
-    // Depth First search loop for choosing the UTXOs
-    for (size_t curr_try = 0, utxo_pool_index = 0; curr_try < TOTAL_TRIES; ++curr_try, ++utxo_pool_index) {
-        // Conditions for starting a backtrack
-        bool backtrack = false;
-        if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value.
-            curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch
-            (curr_waste > best_waste && is_feerate_high)) { // Don't select things which we know will be more wasteful if the waste is increasing
-            backtrack = true;
-        } else if (curr_selection_weight > max_selection_weight) { // Selected UTXOs weight exceeds the maximum weight allowed, cannot find more solutions by adding more inputs
-            max_tx_weight_exceeded = true; // at least one selection attempt exceeded the max weight
-            backtrack = true;
-        } else if (curr_value >= selection_target) {       // Selected value is within range
-            curr_waste += (curr_value - selection_target); // This is the excess value which is added to the waste for the below comparison
-            // Adding another UTXO after this check could bring the waste down if the long term fee is higher than the current fee.
-            // However we are not going to explore that because this optimization for the waste is only done when we have hit our target
-            // value. Adding any more UTXOs will be just burning the UTXO; it will go entirely to fees. Thus we aren't going to
-            // explore any more UTXOs to avoid burning money like that.
+    // Index of the next UTXO to consider in utxo_pool
+    size_t next_utxo = 0;
+
+    auto deselect_last = [&]() {
+        OutputGroup& utxo = utxo_pool[curr_selection.back()];
+        curr_amount -= utxo.GetSelectionAmount();
+        curr_weight -= utxo.m_weight;
+        curr_selection_waste -= utxo.fee - utxo.long_term_fee;
+        curr_selection.pop_back();
+    };
+
+    size_t curr_try = 0;
+    SelectionResult result(selection_target, SelectionAlgorithm::BNB);
+    bool is_done = false;
+    bool is_feerate_high = utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee;
+    while (!is_done) {
+        bool should_shift{false}, should_cut{false};
+        // Select `next_utxo`
+        OutputGroup& utxo = utxo_pool[next_utxo];
+        curr_amount += utxo.GetSelectionAmount();
+        curr_weight += utxo.m_weight;
+        curr_selection_waste += utxo.fee - utxo.long_term_fee;
+        curr_selection.push_back(next_utxo);
+        ++next_utxo;
+        ++curr_try;
+
+        // EVALUATE current selection: check for solutions and see whether we can CUT or SHIFT before EXPLORING further
+        if (curr_amount + lookahead[curr_selection.back()] < selection_target) {
+            // Insufficient funds with lookahead: CUT
+            should_cut = true;
+        } else if (curr_weight > max_selection_weight) {
+            // max_weight exceeded: SHIFT
+            max_tx_weight_exceeded = true;
+            should_shift  = true;
+        } else if (curr_amount > selection_target + cost_of_change) {
+            // Overshot target range: SHIFT
+            should_shift = true;
+        } else if (is_feerate_high && curr_selection_waste > best_waste) {
+            // Waste is already worse than best selection and adding more inputs will not improve it: SHIFT
+            should_shift = true;
+        } else if (curr_amount >= selection_target) {
+            // Selection is within target window: potential solution
+            // Adding more UTXOs only increases fees and cannot be better: SHIFT
+            should_shift  = true;
+            // The amount exceeding the selection_target (the "excess"), would be dropped to the fees: it is waste.
+            CAmount curr_excess = curr_amount - selection_target;
+            CAmount curr_waste = curr_selection_waste + curr_excess;
             if (curr_waste <= best_waste) {
+                // New best solution
                 best_selection = curr_selection;
                 best_waste = curr_waste;
             }
-            curr_waste -= (curr_value - selection_target); // Remove the excess value as we will be selecting different coins now
-            backtrack = true;
         }
 
-        if (backtrack) { // Backtracking, moving backwards
-            if (curr_selection.empty()) { // We have walked back to the first utxo and no branch is untraversed. All solutions searched
+        if (curr_try >= TOTAL_TRIES) {
+            // Solution is not guaranteed to be optimal if `curr_try` hit TOTAL_TRIES
+            result.SetAlgoCompleted(false);
+            break;
+        }
+
+        if (next_utxo == utxo_pool.size()) {
+            // Last added UTXO was end of UTXO pool, nothing left to add on inclusion or omission branch: CUT
+            should_cut = true;
+        }
+
+        if (should_cut) {
+            // Neither adding to the current selection nor exploring the omission branch of the last selected UTXO can
+            // find any solutions. Redirect to exploring the Omission branch of the penultimate selected UTXO (i.e.
+            // set `next_utxo` to one after the penultimate selected, then deselect the last two selected UTXOs)
+            should_cut = false;
+            deselect_last();
+            should_shift  = true;
+        }
+
+        while (should_shift) {
+            // Set `next_utxo` to one after last selected, then deselect last selected UTXO
+            if (curr_selection.empty()) {
+                // Exhausted search space before running into attempt limit
+                is_done = true;
+                result.SetAlgoCompleted(true);
                 break;
             }
+            next_utxo = curr_selection.back() + 1;
+            deselect_last();
+            should_shift  = false;
 
-            // Add omitted UTXOs back to lookahead before traversing the omission branch of last included UTXO.
-            for (--utxo_pool_index; utxo_pool_index > curr_selection.back(); --utxo_pool_index) {
-                curr_available_value += utxo_pool.at(utxo_pool_index).GetSelectionAmount();
-            }
-
-            // Output was included on previous iterations, try excluding now.
-            assert(utxo_pool_index == curr_selection.back());
-            OutputGroup& utxo = utxo_pool.at(utxo_pool_index);
-            curr_value -= utxo.GetSelectionAmount();
-            curr_waste -= utxo.fee - utxo.long_term_fee;
-            curr_selection_weight -= utxo.m_weight;
-            curr_selection.pop_back();
-        } else { // Moving forwards, continuing down this branch
-            OutputGroup& utxo = utxo_pool.at(utxo_pool_index);
-
-            // Remove this utxo from the curr_available_value utxo amount
-            curr_available_value -= utxo.GetSelectionAmount();
-
-            if (curr_selection.empty() ||
-                // The previous index is included and therefore not relevant for exclusion shortcut
-                (utxo_pool_index - 1) == curr_selection.back() ||
-                // Avoid searching a branch if the previous UTXO has the same value and same waste and was excluded.
-                // Since the ratio of fee to long term fee is the same, we only need to check if one of those values match in order to know that the waste is the same.
-                utxo.GetSelectionAmount() != utxo_pool.at(utxo_pool_index - 1).GetSelectionAmount() ||
-                utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee)
-            {
-                // Inclusion branch first (Largest First Exploration)
-                curr_selection.push_back(utxo_pool_index);
-                curr_value += utxo.GetSelectionAmount();
-                curr_waste += utxo.fee - utxo.long_term_fee;
-                curr_selection_weight += utxo.m_weight;
+            // After SHIFTing to an omission branch, the `next_utxo` might have the same effective value as the
+            // UTXO we just omitted. Since lower waste is our tiebreaker on UTXOs with equal effective value for sorting, if it
+            // ties on the effective value, it _must_ have the same waste (i.e. be a "clone" of the prior UTXO) or a
+            // higher waste.  If so, selecting `next_utxo` would produce an equivalent or worse
+            // selection as one we previously evaluated. In that case, increment `next_utxo` until we find a UTXO with a
+            // differing amount.
+            while (utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_pool[next_utxo].GetSelectionAmount()) {
+                if (next_utxo >= utxo_pool.size() - 1) {
+                    // Reached end of UTXO pool skipping clones: SHIFT instead
+                    should_shift = true;
+                    break;
+                }
+                // Skip clone: previous UTXO is equivalent and unselected
+                ++next_utxo;
             }
         }
     }
 
-    // Check for solution
+    result.SetSelectionsEvaluated(curr_try);
+
     if (best_selection.empty()) {
         return max_tx_weight_exceeded ? ErrorMaxWeightExceeded() : util::Error();
     }
 
-    // Set output set
     for (const size_t& i : best_selection) {
         result.AddInput(utxo_pool.at(i));
     }
-    result.RecalculateWaste(cost_of_change, cost_of_change, CAmount{0});
-    assert(best_waste == result.GetWaste());
 
     return result;
 }
 
+
 /*
  * TL;DR: Coin Grinder is a DFS-based algorithm that deterministically searches for the minimum-weight input set to fund
  * the transaction. The algorithm is similar to the Branch and Bound algorithm, but will produce a transaction _with_ a

src/wallet/test/CMakeLists.txt

@@ -10,6 +10,7 @@ target_sources(test_bitcoin
     wallet_test_fixture.cpp
     db_tests.cpp
     coinselector_tests.cpp
+    coinselection_tests.cpp
     feebumper_tests.cpp
     group_outputs_tests.cpp
     init_tests.cpp

src/wallet/test/coinselection_tests.cpp

@@ -0,0 +1,220 @@
+// Copyright (c) 2024 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 <consensus/amount.h>
+#include <policy/policy.h>
+#include <wallet/coinselection.h>
+#include <wallet/test/wallet_test_fixture.h>
+
+#include <boost/test/unit_test.hpp>
+
+namespace wallet {
+BOOST_FIXTURE_TEST_SUITE(coinselection_tests, TestingSetup)
+
+static int next_lock_time = 0;
+static FastRandomContext default_rand;
+
+/** Default coin selection parameters (dcsp) allow us to only explicitly set
+ * parameters when a diverging value is relevant in the context of a test.
+ * We use P2WPKH input and output weights for the change weights. */
+static CoinSelectionParams init_default_params()
+{
+    CoinSelectionParams dcsp{
+        /*rng_fast*/default_rand,
+        /*change_output_size=*/31,
+        /*change_spend_size=*/68,
+        /*min_change_target=*/50'000,
+        /*effective_feerate=*/CFeeRate(5000),
+        /*long_term_feerate=*/CFeeRate(10'000),
+        /*discard_feerate=*/CFeeRate(3000),
+        /*tx_noinputs_size=*/11 + 31, //static header size + output size
+        /*avoid_partial=*/false,
+    };
+    dcsp.m_change_fee = /*155 sats=*/dcsp.m_effective_feerate.GetFee(dcsp.change_output_size);
+    dcsp.min_viable_change = /*204 sats=*/dcsp.m_discard_feerate.GetFee(dcsp.change_spend_size);
+    dcsp.m_cost_of_change = /*204 + 155 sats=*/dcsp.min_viable_change + dcsp.m_change_fee;
+    dcsp.m_subtract_fee_outputs = false;
+    return dcsp;
+}
+
+static const CoinSelectionParams default_cs_params = init_default_params();
+
+/** Make one OutputGroup with a single UTXO that either has a given effective value (default) or a given amount (`is_eff_value = false`). */
+static OutputGroup MakeCoin(const CAmount& amount, bool is_eff_value = true, CoinSelectionParams cs_params = default_cs_params, int custom_spending_vsize = 68)
+{
+    // Always assume that we only have one input
+    CMutableTransaction tx;
+    tx.vout.resize(1);
+    CAmount fees = cs_params.m_effective_feerate.GetFee(custom_spending_vsize);
+    tx.vout[0].nValue = amount + int(is_eff_value) * fees;
+    tx.nLockTime = next_lock_time++;        // so all transactions get different hashes
+    OutputGroup group(cs_params);
+    group.Insert(std::make_shared<COutput>(COutPoint(tx.GetHash(), 0), tx.vout.at(0), /*depth=*/1, /*input_bytes=*/custom_spending_vsize, /*spendable=*/true, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/fees), /*ancestors=*/0, /*descendants=*/0);
+    return group;
+}
+
+/** Make multiple OutputGroups with the given values as their effective value */
+static void AddCoins(std::vector<OutputGroup>& utxo_pool, std::vector<CAmount> coins, CoinSelectionParams cs_params = default_cs_params)
+{
+    for (CAmount c : coins) {
+        utxo_pool.push_back(MakeCoin(c, true, cs_params));
+    }
+}
+
+/** Make multiple coins that share the same effective value */
+static void AddDuplicateCoins(std::vector<OutputGroup>& utxo_pool, int count, int amount) {
+    for (int i = 0 ; i < count; ++i) {
+        utxo_pool.push_back(MakeCoin(amount));
+    }
+}
+
+/** Check if SelectionResult a is equivalent to SelectionResult b.
+ * Two results are equivalent if they are composed of the same input values, even if they have different inputs (i.e., same value, different prevout) */
+static bool HaveEquivalentValues(const SelectionResult& a, const SelectionResult& b)
+{
+    std::vector<CAmount> a_amts;
+    std::vector<CAmount> b_amts;
+    for (const auto& coin : a.GetInputSet()) {
+        a_amts.push_back(coin->txout.nValue);
+    }
+    for (const auto& coin : b.GetInputSet()) {
+        b_amts.push_back(coin->txout.nValue);
+    }
+    std::sort(a_amts.begin(), a_amts.end());
+    std::sort(b_amts.begin(), b_amts.end());
+
+    auto ret = std::mismatch(a_amts.begin(), a_amts.end(), b_amts.begin());
+    return ret.first == a_amts.end() && ret.second == b_amts.end();
+}
+
+static std::string InputsToString(const SelectionResult& selection)
+{
+    std::string res = "[ ";
+    for (const auto& input : selection.GetInputSet()) {
+        res += util::ToString(input->txout.nValue);
+        res += " ";
+    }
+    return res + "]";
+}
+
+static void TestBnBSuccess(std::string test_title, std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const std::vector<CAmount>& expected_input_amounts, size_t expected_attempts, const CoinSelectionParams& cs_params = default_cs_params, int custom_spending_vsize = 68)
+{
+    SelectionResult expected_result(CAmount(0), SelectionAlgorithm::BNB);
+    CAmount expected_amount = 0;
+    for (CAmount input_amount : expected_input_amounts) {
+        OutputGroup group = MakeCoin(input_amount, true, cs_params, custom_spending_vsize);
+        expected_amount += group.m_value;
+        expected_result.AddInput(group);
+    }
+
+    const auto result = SelectCoinsBnB(utxo_pool, selection_target, /*cost_of_change=*/default_cs_params.m_cost_of_change, /*max_selection_weight=*/MAX_STANDARD_TX_WEIGHT);
+    BOOST_CHECK_MESSAGE(result, "Falsy result in BnB-Success: " + test_title);
+    BOOST_CHECK_MESSAGE(HaveEquivalentValues(expected_result, *result), strprintf("Result mismatch in BnB-Success: %s. Expected %s, but got %s", test_title, InputsToString(expected_result), InputsToString(*result)));
+    BOOST_CHECK_MESSAGE(result->GetSelectedValue() == expected_amount, strprintf("Selected amount mismatch in BnB-Success: %s. Expected %d, but got %d", test_title, expected_amount, result->GetSelectedValue()));
+    BOOST_CHECK_MESSAGE(result->GetSelectionsEvaluated() == expected_attempts, strprintf("Unexpected number of attempts in BnB-Success: %s. Expected %i attempts, but got %i", test_title, expected_attempts, result->GetSelectionsEvaluated()));
+}
+
+static void TestBnBFail(std::string test_title, std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target)
+{
+    BOOST_CHECK_MESSAGE(!SelectCoinsBnB(utxo_pool, selection_target, /*cost_of_change=*/default_cs_params.m_cost_of_change, /*max_selection_weight=*/MAX_STANDARD_TX_WEIGHT), "BnB-Fail: " + test_title);
+}
+
+BOOST_AUTO_TEST_CASE(bnb_test)
+{
+    std::vector<OutputGroup> utxo_pool;
+
+    // Fail for empty UTXO pool
+    TestBnBFail("Empty UTXO pool", utxo_pool, /*selection_target=*/1 * CENT);
+
+    AddCoins(utxo_pool, {1 * CENT, 3 * CENT, 5 * CENT});
+
+    // Simple success cases
+    TestBnBSuccess("Select smallest UTXO", utxo_pool, /*selection_target=*/1 * CENT, /*expected_input_amounts=*/{1 * CENT}, /*expected_attempts=*/3);
+    TestBnBSuccess("Select middle UTXO", utxo_pool, /*selection_target=*/3 * CENT, /*expected_input_amounts=*/{3 * CENT}, /*expected_attempts=*/3);
+    TestBnBSuccess("Select biggest UTXO", utxo_pool, /*selection_target=*/5 * CENT, /*expected_input_amounts=*/{5 * CENT}, /*expected_attempts=*/2);
+    TestBnBSuccess("Select two UTXOs", utxo_pool, /*selection_target=*/4 * CENT, /*expected_input_amounts=*/{1 * CENT, 3 * CENT}, /*expected_attempts=*/4);
+    TestBnBSuccess("Select all UTXOs", utxo_pool, /*selection_target=*/9 * CENT, /*expected_input_amounts=*/{1 * CENT, 3 * CENT, 5 * CENT}, /*expected_attempts=*/5);
+
+    // BnB finds changeless solution while overshooting by up to cost_of_change
+    TestBnBSuccess("Select upper bound", utxo_pool, /*selection_target=*/4 * CENT - default_cs_params.m_cost_of_change, /*expected_input_amounts=*/{1 * CENT, 3 * CENT}, /*expected_attempts=*/4);
+
+    // BnB fails to find changeless solution when overshooting by cost_of_change + 1 sat
+    TestBnBFail("Overshoot upper bound", utxo_pool, /*selection_target=*/4 * CENT - default_cs_params.m_cost_of_change - 1);
+
+    // Simple cases without BnB solution
+    TestBnBFail("Smallest combination too big", utxo_pool, /*selection_target=*/0.5 * CENT);
+    TestBnBFail("No UTXO combination in target window", utxo_pool, /*selection_target=*/7 * CENT);
+    TestBnBFail("Select more than available", utxo_pool, /*selection_target=*/10 * CENT);
+
+    // Test skipping of equivalent input sets
+    std::vector<OutputGroup> clone_pool;
+    AddCoins(clone_pool, {2 * CENT, 7 * CENT, 7 * CENT});
+    AddDuplicateCoins(clone_pool, 50'000, 5 * CENT);
+    TestBnBSuccess("Skip equivalent input sets", clone_pool, /*selection_target=*/16 * CENT, /*expected_input_amounts=*/{2 * CENT, 7 * CENT, 7 * CENT}, /*expected_attempts=*/16);
+
+    /* Test BnB attempt limit (`TOTAL_TRIES`)
+     *
+     * Generally, on a diverse UTXO pool BnB will quickly pass over UTXOs bigger than the target and then start
+     * combining small counts of UTXOs that in sum remain under the selection_target+cost_of_change. When there are
+     * multiple UTXOs that have matching amount and cost, combinations with equivalent input sets are skipped. The UTXO
+     * pool for this test is specifically crafted to create as much branching as possible. The selection target is
+     * 8 CENT while all UTXOs are slightly bigger than 1 CENT. The smallest eight are 100,000…100,007 sats, while the larger
+     * nine are 100,368…100,375 (i.e., 100,008…100,016 sats plus cost_of_change (359 sats)).
+     *
+     * Because BnB will only select input sets that fall between selection_target and selection_target + cost_of_change,
+     * and the search traverses the UTXO pool from large to small amounts, the search will visit every single
+     * combination of eight inputs. All except the last combination will overshoot by more than cost_of_change on the eighth input, because the larger nine inputs each exceed 1 CENT by more than cost_of_change.
+     * Only the last combination consisting of the eight smallest UTXOs falls into the target window.
+     */
+    std::vector<OutputGroup> doppelganger_pool;
+    std::vector<CAmount> doppelgangers;
+    std::vector<CAmount> expected_inputs;
+    for (int i = 0; i < 17; ++i) {
+        if (i < 8) {
+            // The eight smallest UTXOs can be combined to create expected_result
+            doppelgangers.push_back(1 * CENT + i);
+            expected_inputs.push_back(doppelgangers[i]);
+        } else {
+            // Any eight UTXOs including at least one UTXO with the added cost_of_change will exceed target window
+            doppelgangers.push_back(1 * CENT + default_cs_params.m_cost_of_change + i);
+        }
+    }
+    AddCoins(doppelganger_pool, doppelgangers);
+    // Among up to 17 unique UTXOs of similar effective value we will find a solution composed of the eight smallest UTXOs
+    TestBnBSuccess("Combine smallest 8 of 17 unique UTXOs", doppelganger_pool, /*selection_target=*/8 * CENT, /*expected_input_amounts=*/expected_inputs, /*expected_attempts=*/51'765);
+
+    // Among up to 18 unique UTXOs of similar effective value we will find a solution composed of the eight smallest UTXOs
+    AddCoins(doppelganger_pool, {1 * CENT + default_cs_params.m_cost_of_change + 17});
+    TestBnBSuccess("Combine smallest 8 of 18 unique UTXOs", doppelganger_pool, /*selection_target=*/8 * CENT, /*expected_input_amounts=*/expected_inputs, /*expected_attempts=*/87'957);
+
+    // Starting with 19 unique UTXOs of similar effective value we will not find the solution due to exceeding the attempt limit
+    AddCoins(doppelganger_pool, {1 * CENT + default_cs_params.m_cost_of_change + 18});
+    TestBnBFail("Exhaust looking for smallest 8 of 19 unique UTXOs", doppelganger_pool, /*selection_target=*/8 * CENT);
+}
+
+BOOST_AUTO_TEST_CASE(bnb_feerate_sensitivity_test)
+{
+    // Create sets of UTXOs with the same effective amounts at different feerates (but different absolute amounts)
+    std::vector<OutputGroup> low_feerate_pool; // 5 sat/vB (default, and lower than long_term_feerate of 10 sat/vB)
+    AddCoins(low_feerate_pool, {2 * CENT, 3 * CENT, 5 * CENT, 10 * CENT});
+    TestBnBSuccess("Select many inputs at low feerates", low_feerate_pool, /*selection_target=*/10 * CENT, /*expected_input_amounts=*/{2 * CENT, 3 * CENT, 5 * CENT}, /*expected_attempts=*/6);
+
+    CoinSelectionParams high_feerate_params = init_default_params();
+    high_feerate_params.m_effective_feerate = CFeeRate{25'000};
+    std::vector<OutputGroup> high_feerate_pool; // 25 sat/vB (greater than long_term_feerate of 10 sat/vB)
+    AddCoins(high_feerate_pool, {2 * CENT, 3 * CENT, 5 * CENT, 10 * CENT}, high_feerate_params);
+    TestBnBSuccess("Select one input at high feerates", high_feerate_pool, /*selection_target=*/10 * CENT, /*expected_input_amounts=*/{10 * CENT}, /*expected_attempts=*/5, high_feerate_params);
+
+    // Add heavy inputs {6, 7} to existing {2, 3, 5, 10}
+    low_feerate_pool.push_back(MakeCoin(6 * CENT, true, default_cs_params, /*custom_spending_vsize=*/500));
+    low_feerate_pool.push_back(MakeCoin(7 * CENT, true, default_cs_params, /*custom_spending_vsize=*/500));
+    TestBnBSuccess("Prefer two heavy inputs over two light inputs at low feerates", low_feerate_pool, /*selection_target=*/13 * CENT, /*expected_input_amounts=*/{6 * CENT, 7 * CENT}, /*expected_attempts=*/18, default_cs_params, /*custom_spending_vsize=*/500);
+
+    high_feerate_pool.push_back(MakeCoin(6 * CENT, true, high_feerate_params, /*custom_spending_vsize=*/500));
+    high_feerate_pool.push_back(MakeCoin(7 * CENT, true, high_feerate_params, /*custom_spending_vsize=*/500));
+    TestBnBSuccess("Prefer two light inputs over two heavy inputs at high feerates", high_feerate_pool, /*selection_target=*/13 * CENT, /*expected_input_amounts=*/{3 * CENT, 10 * CENT}, /*expected_attempts=*/9, high_feerate_params);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+} // namespace wallet

src/wallet/test/coinselector_tests.cpp

@@ -37,15 +37,6 @@ static const CoinEligibilityFilter filter_confirmed(1, 1, 0);
 static const CoinEligibilityFilter filter_standard_extra(6, 6, 0);
 static int nextLockTime = 0;
 
-static void add_coin(const CAmount& nValue, int nInput, std::vector<COutput>& set)
-{
-    CMutableTransaction tx;
-    tx.vout.resize(nInput + 1);
-    tx.vout[nInput].nValue = nValue;
-    tx.nLockTime = nextLockTime++;        // so all transactions get different hashes
-    set.emplace_back(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0);
-}
-
 static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result)
 {
     CMutableTransaction tx;
@@ -133,18 +124,6 @@ static bool EqualResult(const SelectionResult& a, const SelectionResult& b)
     return ret.first == a.GetInputSet().end() && ret.second == b.GetInputSet().end();
 }
 
-static CAmount make_hard_case(int utxos, std::vector<COutput>& utxo_pool)
-{
-    utxo_pool.clear();
-    CAmount target = 0;
-    for (int i = 0; i < utxos; ++i) {
-        target += CAmount{1} << (utxos+i);
-        add_coin(CAmount{1} << (utxos+i), 2*i, utxo_pool);
-        add_coin((CAmount{1} << (utxos+i)) + (CAmount{1} << (utxos-1-i)), 2*i + 1, utxo_pool);
-    }
-    return target;
-}
-
 inline std::vector<OutputGroup>& GroupCoins(const std::vector<COutput>& available_coins, bool subtract_fee_outputs = false)
 {
     static std::vector<OutputGroup> static_groups;
@@ -194,116 +173,11 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
     // Setup
     std::vector<COutput> utxo_pool;
     SelectionResult expected_result(CAmount(0), SelectionAlgorithm::BNB);
-
-    /////////////////////////
-    // Known Outcome tests //
-    /////////////////////////
-
-    // Empty utxo pool
-    BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT));
-
-    // Add utxos
-    add_coin(1 * CENT, 1, utxo_pool);
-    add_coin(2 * CENT, 2, utxo_pool);
-    add_coin(3 * CENT, 3, utxo_pool);
-    add_coin(4 * CENT, 4, utxo_pool);
-
-    // Select 1 Cent
-    add_coin(1 * CENT, 1, expected_result);
-    const auto result1 = SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT);
-    BOOST_CHECK(result1);
-    BOOST_CHECK(EquivalentResult(expected_result, *result1));
-    BOOST_CHECK_EQUAL(result1->GetSelectedValue(), 1 * CENT);
-    expected_result.Clear();
-
-    // Select 2 Cent
-    add_coin(2 * CENT, 2, expected_result);
-    const auto result2 = SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT);
-    BOOST_CHECK(result2);
-    BOOST_CHECK(EquivalentResult(expected_result, *result2));
-    BOOST_CHECK_EQUAL(result2->GetSelectedValue(), 2 * CENT);
-    expected_result.Clear();
-
-    // Select 5 Cent
-    add_coin(3 * CENT, 3, expected_result);
-    add_coin(2 * CENT, 2, expected_result);
-    const auto result3 = SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT);
-    BOOST_CHECK(result3);
-    BOOST_CHECK(EquivalentResult(expected_result, *result3));
-    BOOST_CHECK_EQUAL(result3->GetSelectedValue(), 5 * CENT);
-    expected_result.Clear();
-
-    // Select 11 Cent, not possible
-    BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT));
-    expected_result.Clear();
-
-    // Cost of change is greater than the difference between target value and utxo sum
-    add_coin(1 * CENT, 1, expected_result);
-    const auto result4 = SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT);
-    BOOST_CHECK(result4);
-    BOOST_CHECK_EQUAL(result4->GetSelectedValue(), 1 * CENT);
-    BOOST_CHECK(EquivalentResult(expected_result, *result4));
-    expected_result.Clear();
-
-    // Cost of change is less than the difference between target value and utxo sum
-    BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0));
-    expected_result.Clear();
-
-    // Select 10 Cent
-    add_coin(5 * CENT, 5, utxo_pool);
-    add_coin(4 * CENT, 4, expected_result);
-    add_coin(3 * CENT, 3, expected_result);
-    add_coin(2 * CENT, 2, expected_result);
-    add_coin(1 * CENT, 1, expected_result);
-    const auto result5 = SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT);
-    BOOST_CHECK(result5);
-    BOOST_CHECK(EquivalentResult(expected_result, *result5));
-    BOOST_CHECK_EQUAL(result5->GetSelectedValue(), 10 * CENT);
-    expected_result.Clear();
-
-    // Select 0.25 Cent, not possible
-    BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT));
-    expected_result.Clear();
-
-    // Iteration exhaustion test
-    CAmount target = make_hard_case(17, utxo_pool);
-    BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 1)); // Should exhaust
-    target = make_hard_case(14, utxo_pool);
-    const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 1); // Should not exhaust
-    BOOST_CHECK(result7);
-
-    // Test same value early bailout optimization
-    utxo_pool.clear();
-    add_coin(7 * CENT, 7, expected_result);
-    add_coin(7 * CENT, 7, expected_result);
-    add_coin(7 * CENT, 7, expected_result);
-    add_coin(7 * CENT, 7, expected_result);
-    add_coin(2 * CENT, 7, expected_result);
-    add_coin(7 * CENT, 7, utxo_pool);
-    add_coin(7 * CENT, 7, utxo_pool);
-    add_coin(7 * CENT, 7, utxo_pool);
-    add_coin(7 * CENT, 7, utxo_pool);
-    add_coin(2 * CENT, 7, utxo_pool);
-    for (int i = 0; i < 50000; ++i) {
-        add_coin(5 * CENT, 7, utxo_pool);
-    }
-    const auto result8 = SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000);
-    BOOST_CHECK(result8);
-    BOOST_CHECK_EQUAL(result8->GetSelectedValue(), 30 * CENT);
-    BOOST_CHECK(EquivalentResult(expected_result, *result8));
+    size_t expected_attempts;
 
     ////////////////////
     // Behavior tests //
     ////////////////////
-    // Select 1 Cent with pool of only greater than 5 Cent
-    utxo_pool.clear();
-    for (int i = 5; i <= 20; ++i) {
-        add_coin(i * CENT, i, utxo_pool);
-    }
-    // Run 100 times, to make sure it is never finding a solution
-    for (int i = 0; i < 100; ++i) {
-        BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT));
-    }
 
     // Make sure that effective value is working in AttemptSelection when BnB is used
     CoinSelectionParams coin_selection_params_bnb{
@@ -337,6 +211,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         const auto result9 = SelectCoinsBnB(GroupCoins(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change);
         BOOST_CHECK(result9);
         BOOST_CHECK_EQUAL(result9->GetSelectedValue(), 1 * CENT);
+        // Demonstrate how following improvements reduce iteration count and catch any regressions in the future.
+        expected_attempts = 1;
+        BOOST_CHECK_MESSAGE(result9->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, result9->GetSelectionsEvaluated()));
     }
 
     {
@@ -359,6 +236,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         LOCK(wallet->cs_wallet);
         const auto result10 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(result10);
+        // Demonstrate how following improvements reduce iteration count and catch any regressions in the future.
+        expected_attempts = 3;
+        BOOST_CHECK_MESSAGE(result10->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, result10->GetSelectionsEvaluated()));
     }
     {
         std::unique_ptr<CWallet> wallet = NewWallet(m_node);
@@ -366,7 +246,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
 
         CoinsResult available_coins;
 
-        // single coin should be selected when effective fee > long term fee
+        // pre selected coin should be selected even if disadvantageous
         coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000);
         coin_selection_params_bnb.m_long_term_feerate = CFeeRate(3000);
 
@@ -377,42 +257,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
 
         expected_result.Clear();
-        add_coin(10 * CENT + input_fee, 2, expected_result);
+        add_coin(9 * CENT + input_fee, 2, expected_result);
+        add_coin(1 * CENT + input_fee, 2, expected_result);
         CCoinControl coin_control;
-        const auto result11 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb);
-        BOOST_CHECK(EquivalentResult(expected_result, *result11));
-        available_coins.Clear();
-
-        // more coins should be selected when effective fee < long term fee
-        coin_selection_params_bnb.m_effective_feerate = CFeeRate(3000);
-        coin_selection_params_bnb.m_long_term_feerate = CFeeRate(5000);
-
-        // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount
-        input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type)
-        add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
-        add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
-        add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
-
-        expected_result.Clear();
-        add_coin(9 * CENT + input_fee, 2, expected_result);
-        add_coin(1 * CENT + input_fee, 2, expected_result);
-        const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb);
-        BOOST_CHECK(EquivalentResult(expected_result, *result12));
-        available_coins.Clear();
-
-        // pre selected coin should be selected even if disadvantageous
-        coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000);
-        coin_selection_params_bnb.m_long_term_feerate = CFeeRate(3000);
-
-        // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount
-        input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type)
-        add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
-        add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
-        add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
-
-        expected_result.Clear();
-        add_coin(9 * CENT + input_fee, 2, expected_result);
-        add_coin(1 * CENT + input_fee, 2, expected_result);
         coin_control.m_allow_other_inputs = true;
         COutput select_coin = available_coins.All().at(1); // pre select 9 coin
         coin_control.Select(select_coin.outpoint);
@@ -421,6 +268,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         available_coins.Erase({(++available_coins.coins[OutputType::BECH32].begin())->outpoint});
         const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(EquivalentResult(expected_result, *result13));
+        // Demonstrate how following improvements reduce iteration count and catch any regressions in the future.
+        expected_attempts = 2;
+        BOOST_CHECK_MESSAGE(result13->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, result13->GetSelectionsEvaluated()));
     }
 
     {
@@ -452,6 +302,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         add_coin(5 * CENT, 2, expected_result);
         add_coin(3 * CENT, 2, expected_result);
         BOOST_CHECK(EquivalentResult(expected_result, *res));
+        // Demonstrate how following improvements reduce iteration count and catch any regressions in the future.
+        expected_attempts = 22;
+        BOOST_CHECK_MESSAGE(res->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, res->GetSelectionsEvaluated()));
     }
 }
 

test/functional/wallet_backwards_compatibility.py

@@ -87,7 +87,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
         # Make sure that this is being automatically cleaned up by migration
         node_master = self.nodes[1]
-        node_v22 = self.nodes[self.num_nodes - 5]
+        node_v22 = self.nodes[self.num_nodes - 3]
         wallet_name = "bad_deriv_path"
         node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
         bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)