From 7aa7e30441fe77bf8e8092916e36b004bbbfe2a7 Mon Sep 17 00:00:00 2001 From: Murch Date: Mon, 21 Aug 2023 15:27:23 -0400 Subject: Fold GetSelectionWaste() into ComputeAndSetWaste() Both `GetSelectionWaste()` and `ComputeAndSetWaste()` now are part of `SelectionResult`. Instead of `ComputeAndSetWaste()` being a wrapper for `GetSelectionWaste()`, we combine them to a new function `RecalculateWaste()`. As I was combining the logic of the two functions, I noticed that `GetSelectionWaste()` was making the odd assumption that the `change_cost` being set to zero means that no change is created. However, if we build transactions at a feerate of zero with the `discard_feerate` also set to zero, we'd organically have a `change_cost` of zero, even when we create change on a transaction. This commit cleans up this duplicate meaning of `change_cost` and relies on `GetChange()` to figure out whether there is change on basis of the `min_viable_change` and whatever is left after deducting fees. Since this broke a bunch of tests that relied on the double-meaning of `change_cost` a bunch of tests had to be fixed. --- src/bench/coin_selection.cpp | 8 ++-- src/wallet/coinselection.cpp | 59 +++++++++++----------------- src/wallet/coinselection.h | 31 +++++++-------- src/wallet/spend.cpp | 8 ++-- src/wallet/test/coinselector_tests.cpp | 70 +++++++++++++++++----------------- src/wallet/test/fuzz/coinselection.cpp | 4 +- 6 files changed, 82 insertions(+), 98 deletions(-) (limited to 'src') diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 249b76ee85..171c61c46f 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -71,15 +71,15 @@ static void CoinSelection(benchmark::Bench& bench) /*change_output_size=*/ 34, /*change_spend_size=*/ 148, /*min_change_target=*/ CHANGE_LOWER, - /*effective_feerate=*/ CFeeRate(0), - /*long_term_feerate=*/ CFeeRate(0), - /*discard_feerate=*/ CFeeRate(0), + /*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(), 1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true); + auto result = AttemptSelection(wallet.chain(), 1002.99 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true); assert(result); assert(result->GetSelectedValue() == 1003 * COIN); assert(result->GetInputSet().size() == 2); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 42615b5d42..f1706b6800 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -194,7 +194,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool for (const size_t& i : best_selection) { result.AddInput(utxo_pool.at(i)); } - result.ComputeAndSetWaste(cost_of_change, cost_of_change, CAmount{0}); + result.RecalculateWaste(cost_of_change, cost_of_change, CAmount{0}); assert(best_waste == result.GetWaste()); return result; @@ -792,35 +792,6 @@ void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool in } } -CAmount SelectionResult::GetSelectionWaste(CAmount change_cost, CAmount target, bool use_effective_value) -{ - // This function should not be called with empty inputs as that would mean the selection failed - assert(!m_selected_inputs.empty()); - - // Always consider the cost of spending an input now vs in the future. - CAmount waste = 0; - for (const auto& coin_ptr : m_selected_inputs) { - const COutput& coin = *coin_ptr; - waste += coin.GetFee() - coin.long_term_fee; - } - // Bump fee of whole selection may diverge from sum of individual bump fees - waste -= bump_fee_group_discount; - - if (change_cost) { - // Consider the cost of making change and spending it in the future - // If we aren't making change, the caller should've set change_cost to 0 - assert(change_cost > 0); - waste += change_cost; - } else { - // When we are not making change (change_cost == 0), consider the excess we are throwing away to fees - CAmount selected_effective_value = use_effective_value ? GetSelectedEffectiveValue() : GetSelectedValue(); - assert(selected_effective_value >= target); - waste += selected_effective_value - target; - } - - return waste; -} - CAmount GenerateChangeTarget(const CAmount payment_value, const CAmount change_fee, FastRandomContext& rng) { if (payment_value <= CHANGE_LOWER / 2) { @@ -839,16 +810,32 @@ void SelectionResult::SetBumpFeeDiscount(const CAmount discount) bump_fee_group_discount = discount; } - -void SelectionResult::ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee) +void SelectionResult::RecalculateWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee) { - const CAmount change = GetChange(min_viable_change, change_fee); + // This function should not be called with empty inputs as that would mean the selection failed + assert(!m_selected_inputs.empty()); + + // Always consider the cost of spending an input now vs in the future. + CAmount waste = 0; + for (const auto& coin_ptr : m_selected_inputs) { + const COutput& coin = *coin_ptr; + waste += coin.GetFee() - coin.long_term_fee; + } + // Bump fee of whole selection may diverge from sum of individual bump fees + waste -= bump_fee_group_discount; - if (change > 0) { - m_waste = GetSelectionWaste(change_cost, m_target, m_use_effective); + if (GetChange(min_viable_change, change_fee)) { + // if we have a minimum viable amount after deducting fees, account for + // cost of creating and spending change + waste += change_cost; } else { - m_waste = GetSelectionWaste(0, m_target, m_use_effective); + // When we are not making change (GetChange(…) == 0), consider the excess we are throwing away to fees + CAmount selected_effective_value = m_use_effective ? GetSelectedEffectiveValue() : GetSelectedValue(); + assert(selected_effective_value >= m_target); + waste += selected_effective_value - m_target; } + + m_waste = waste; } void SelectionResult::SetAlgoCompleted(bool algo_completed) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 80c92e1b0e..9fb000422c 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -350,22 +350,6 @@ private: } } - /** Compute the waste for this result given the cost of change - * and the opportunity cost of spending these inputs now vs in the future. - * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) - * If no change, waste = excess + inputs * (effective_feerate - long_term_feerate) - * where excess = selected_effective_value - target - * change_cost = effective_feerate * change_output_size + long_term_feerate * change_spend_size - * - * @param[in] change_cost The cost of creating change and spending it in the future. - * Only used if there is change, in which case it must be positive. - * Must be 0 if there is no change. - * @param[in] target The amount targeted by the coin selection algorithm. - * @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false). - * @return The waste - */ - [[nodiscard]] CAmount GetSelectionWaste(CAmount change_cost, CAmount target, bool use_effective_value = true); - public: explicit SelectionResult(const CAmount target, SelectionAlgorithm algo) : m_target(target), m_algo(algo) {} @@ -387,8 +371,19 @@ public: /** How much individual inputs overestimated the bump fees for shared ancestries */ void SetBumpFeeDiscount(const CAmount discount); - /** Calculates and stores the waste for this selection via GetSelectionWaste */ - void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); + /** Calculates and stores the waste for this result given the cost of change + * and the opportunity cost of spending these inputs now vs in the future. + * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) - bump_fee_group_discount + * If no change, waste = excess + inputs * (effective_feerate - long_term_feerate) - bump_fee_group_discount + * where excess = selected_effective_value - target + * change_cost = effective_feerate * change_output_size + long_term_feerate * change_spend_size + * + * @param[in] min_viable_change The minimum amount necessary to make a change output economic + * @param[in] change_cost The cost of creating a change output and spending it in the future. Only + * used if there is change, in which case it must be non-negative. + * @param[in] change_fee The fee for creating a change output + */ + void RecalculateWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); [[nodiscard]] CAmount GetWaste() const; /** Tracks that algorithm was able to exhaustively search the entire combination space before hitting limit of tries */ diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 5d23ebd35a..89317b7656 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -711,7 +711,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co if (coin_selection_params.m_effective_feerate > CFeeRate{3 * coin_selection_params.m_long_term_feerate}) { // Minimize input set for feerates of at least 3×LTFRE (default: 30 ṩ/vB+) if (auto cg_result{CoinGrinder(groups.positive_group, nTargetValue, coin_selection_params.m_min_change_target, max_inputs_weight)}) { - cg_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); + cg_result->RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*cg_result); } else { append_error(cg_result); @@ -746,7 +746,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co if (bump_fee_overestimate) { result.SetBumpFeeDiscount(bump_fee_overestimate); } - result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); + result.RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); } // Choose the result with the least waste @@ -771,7 +771,7 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av if (selection_target <= 0) { SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); - result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); + result.RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); return result; } @@ -792,7 +792,7 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL); preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); op_selection_result->Merge(preselected); - op_selection_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, + op_selection_result->RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 9a349f0992..979100d7b2 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -874,29 +874,31 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) BOOST_AUTO_TEST_CASE(waste_test) { const CAmount fee{100}; + const CAmount min_viable_change{300}; const CAmount change_cost{125}; + const CAmount change_fee{30}; const CAmount fee_diff{40}; const CAmount in_amt{3 * COIN}; const CAmount target{2 * COIN}; - const CAmount excess{in_amt - fee * 2 - target}; + const CAmount excess{80}; - // The following tests that the waste is calculated correctly in various scenarios. - // ComputeAndSetWaste will first determine the size of the change output. We don't really - // care about the change and just want to use the variant that always includes the change_cost, - // so min_viable_change and change_fee are set to 0 to ensure that. + // In the following, we test that the waste is calculated correctly in various scenarios. + // Usually, RecalculateWaste would compute change_fee and change_cost on basis of the + // change output type, current feerate, and discard_feerate, but we use fixed values + // across this test to make the test easier to understand. { // Waste with change is the change cost and difference between fee and long term fee SelectionResult selection1{target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection1, fee, fee - fee_diff); add_coin(2 * COIN, 2, selection1, fee, fee - fee_diff); - selection1.ComputeAndSetWaste(/*min_viable_change=*/0, change_cost, /*change_fee=*/0); + selection1.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(fee_diff * 2 + change_cost, selection1.GetWaste()); // Waste will be greater when fee is greater, but long term fee is the same SelectionResult selection2{target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection2, fee * 2, fee - fee_diff); add_coin(2 * COIN, 2, selection2, fee * 2, fee - fee_diff); - selection2.ComputeAndSetWaste(/*min_viable_change=*/0, change_cost, /*change_fee=*/0); + selection2.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_GT(selection2.GetWaste(), selection1.GetWaste()); // Waste with change is the change cost and difference between fee and long term fee @@ -904,25 +906,25 @@ BOOST_AUTO_TEST_CASE(waste_test) SelectionResult selection3{target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection3, fee, fee + fee_diff); add_coin(2 * COIN, 2, selection3, fee, fee + fee_diff); - selection3.ComputeAndSetWaste(/*min_viable_change=*/0, change_cost, /*change_fee=*/0); + selection3.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(fee_diff * -2 + change_cost, selection3.GetWaste()); BOOST_CHECK_LT(selection3.GetWaste(), selection1.GetWaste()); } { // Waste without change is the excess and difference between fee and long term fee - SelectionResult selection_nochange1{target, SelectionAlgorithm::MANUAL}; + SelectionResult selection_nochange1{/*target creates no change*/in_amt - 2 * fee - excess, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection_nochange1, fee, fee - fee_diff); add_coin(2 * COIN, 2, selection_nochange1, fee, fee - fee_diff); - selection_nochange1.ComputeAndSetWaste(/*min_viable_change=*/0, /*change_cost=*/0, /*change_fee=*/0); + selection_nochange1.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(fee_diff * 2 + excess, selection_nochange1.GetWaste()); // Waste without change is the excess and difference between fee and long term fee // With long term fee greater than fee, waste should be less than when long term fee is less than fee - SelectionResult selection_nochange2{target, SelectionAlgorithm::MANUAL}; + SelectionResult selection_nochange2{/*target creates no change*/in_amt - 2 * fee - excess, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection_nochange2, fee, fee + fee_diff); add_coin(2 * COIN, 2, selection_nochange2, fee, fee + fee_diff); - selection_nochange2.ComputeAndSetWaste(/*min_viable_change=*/0, /*change_cost=*/0, /*change_fee=*/0); + selection_nochange2.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(fee_diff * -2 + excess, selection_nochange2.GetWaste()); BOOST_CHECK_LT(selection_nochange2.GetWaste(), selection_nochange1.GetWaste()); } @@ -932,57 +934,56 @@ BOOST_AUTO_TEST_CASE(waste_test) SelectionResult selection{target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection, fee, fee); add_coin(2 * COIN, 2, selection, fee, fee); - selection.ComputeAndSetWaste(/*min_viable_change=*/0, change_cost, /*change_fee=*/0); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(change_cost, selection.GetWaste()); } { // Waste without change and fee == long term fee is just the excess - SelectionResult selection{target, SelectionAlgorithm::MANUAL}; + SelectionResult selection{/*target creates no change*/in_amt - 2 * fee - excess, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection, fee, fee); add_coin(2 * COIN, 2, selection, fee, fee); - selection.ComputeAndSetWaste(/*min_viable_change=*/0, /*change_cost=*/0, /*change_fee=*/0); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(excess, selection.GetWaste()); } { - // No Waste when fee == long_term_fee, no change, and no excess + // Waste is 0 when fee == long_term_fee, no change, and no excess const CAmount exact_target{in_amt - fee * 2}; SelectionResult selection{exact_target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection, fee, fee); add_coin(2 * COIN, 2, selection, fee, fee); - selection.ComputeAndSetWaste(/*min_viable_change=*/0, /*change_cost=*/0, /*change_fee=*/0); + selection.RecalculateWaste(min_viable_change, change_cost , change_fee); BOOST_CHECK_EQUAL(0, selection.GetWaste()); } { - // No Waste when (fee - long_term_fee) == (-cost_of_change), and no excess + // Waste is 0 when (fee - long_term_fee) == (-cost_of_change), and no excess SelectionResult selection{target, SelectionAlgorithm::MANUAL}; - const CAmount new_change_cost{fee_diff * 2}; add_coin(1 * COIN, 1, selection, fee, fee + fee_diff); add_coin(2 * COIN, 2, selection, fee, fee + fee_diff); - selection.ComputeAndSetWaste(/*min_viable_change=*/0, new_change_cost, /*change_fee=*/0); + selection.RecalculateWaste(min_viable_change, /*change_cost=*/fee_diff * 2, change_fee); BOOST_CHECK_EQUAL(0, selection.GetWaste()); } { - // No Waste when (fee - long_term_fee) == (-excess), no change cost - const CAmount new_target{in_amt - fee * 2 - fee_diff * 2}; + // Waste is 0 when (fee - long_term_fee) == (-excess), no change cost + const CAmount new_target{in_amt - fee * 2 - /*excess=*/fee_diff * 2}; SelectionResult selection{new_target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection, fee, fee + fee_diff); add_coin(2 * COIN, 2, selection, fee, fee + fee_diff); - selection.ComputeAndSetWaste(/*min_viable_change=*/0, /*change_cost=*/0, /*change_fee=*/0); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(0, selection.GetWaste()); } { // Negative waste when the long term fee is greater than the current fee and the selected value == target - const CAmount exact_target{3 * COIN - 2 * fee}; + const CAmount exact_target{in_amt - 2 * fee}; SelectionResult selection{exact_target, SelectionAlgorithm::MANUAL}; const CAmount target_waste1{-2 * fee_diff}; // = (2 * fee) - (2 * (fee + fee_diff)) add_coin(1 * COIN, 1, selection, fee, fee + fee_diff); add_coin(2 * COIN, 2, selection, fee, fee + fee_diff); - selection.ComputeAndSetWaste(/*min_viable_change=*/0, /*change_cost=*/0, /*change_fee=*/0); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(target_waste1, selection.GetWaste()); } @@ -993,7 +994,7 @@ BOOST_AUTO_TEST_CASE(waste_test) const CAmount target_waste2{-2 * large_fee_diff + change_cost}; // = (2 * fee) - (2 * (fee + large_fee_diff)) + change_cost add_coin(1 * COIN, 1, selection, fee, fee + large_fee_diff); add_coin(2 * COIN, 2, selection, fee, fee + large_fee_diff); - selection.ComputeAndSetWaste(/*min_viable_change=*/0, change_cost, /*change_fee=*/0); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(target_waste2, selection.GetWaste()); } } @@ -1018,12 +1019,12 @@ BOOST_AUTO_TEST_CASE(bump_fee_test) inputs[i]->ApplyBumpFee(20*(i+1)); } - selection.ComputeAndSetWaste(min_viable_change, change_cost, change_fee); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); CAmount expected_waste = fee_diff * -2 + change_cost + /*bump_fees=*/60; BOOST_CHECK_EQUAL(expected_waste, selection.GetWaste()); selection.SetBumpFeeDiscount(30); - selection.ComputeAndSetWaste(min_viable_change, change_cost, change_fee); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); expected_waste = fee_diff * -2 + change_cost + /*bump_fees=*/60 - /*group_discount=*/30; BOOST_CHECK_EQUAL(expected_waste, selection.GetWaste()); } @@ -1044,12 +1045,12 @@ BOOST_AUTO_TEST_CASE(bump_fee_test) inputs[i]->ApplyBumpFee(20*(i+1)); } - selection.ComputeAndSetWaste(min_viable_change, change_cost, change_fee); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); CAmount expected_waste = fee_diff * -2 + /*bump_fees=*/60 + /*excess = 100 - bump_fees*/40; BOOST_CHECK_EQUAL(expected_waste, selection.GetWaste()); selection.SetBumpFeeDiscount(30); - selection.ComputeAndSetWaste(min_viable_change, change_cost, change_fee); + selection.RecalculateWaste(min_viable_change, change_cost, change_fee); expected_waste = fee_diff * -2 + /*bump_fees=*/60 - /*group_discount=*/30 + /*excess = 100 - bump_fees + group_discount*/70; BOOST_CHECK_EQUAL(expected_waste, selection.GetWaste()); } @@ -1429,6 +1430,7 @@ BOOST_AUTO_TEST_CASE(check_max_weight) /*avoid_partial=*/false, }; + int max_weight = MAX_STANDARD_TX_WEIGHT - WITNESS_SCALE_FACTOR * (cs_params.tx_noinputs_size + cs_params.change_output_size); { // Scenario 1: // The actor starts with 1x 50.0 BTC and 1515x 0.033 BTC (~100.0 BTC total) unspent outputs @@ -1450,10 +1452,9 @@ BOOST_AUTO_TEST_CASE(check_max_weight) m_node); BOOST_CHECK(result); - // Verify that only the 50 BTC UTXO was selected - const auto& selection_res = result->GetInputSet(); - BOOST_CHECK(selection_res.size() == 1); - BOOST_CHECK((*selection_res.begin())->GetEffectiveValue() == 50 * COIN); + // Verify that the 50 BTC UTXO was selected, and result is below max_weight + BOOST_CHECK(has_coin(result->GetInputSet(), CAmount(50 * COIN))); + BOOST_CHECK_LE(result->GetWeight(), max_weight); } { @@ -1479,6 +1480,7 @@ BOOST_AUTO_TEST_CASE(check_max_weight) BOOST_CHECK(has_coin(result->GetInputSet(), CAmount(0.0625 * COIN))); BOOST_CHECK(has_coin(result->GetInputSet(), CAmount(0.025 * COIN))); + BOOST_CHECK_LE(result->GetWeight(), max_weight); } { diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 297432de9e..4244c6397f 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -270,7 +270,7 @@ FUZZ_TARGET(coinselection) if (result_srd) { assert(result_srd->GetSelectedValue() >= target); assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER - result_srd->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); + result_srd->RecalculateWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_srd->GetShuffledInputVector(); (void)result_srd->GetInputSet(); } @@ -279,7 +279,7 @@ FUZZ_TARGET(coinselection) auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT); if (result_knapsack) { assert(result_knapsack->GetSelectedValue() >= target); - result_knapsack->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); + result_knapsack->RecalculateWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_knapsack->GetShuffledInputVector(); (void)result_knapsack->GetInputSet(); } -- cgit v1.2.3 From bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624 Mon Sep 17 00:00:00 2001 From: Murch Date: Fri, 24 May 2024 14:31:37 -0400 Subject: Use `exact_target` shorthand in coinselector_tests --- src/wallet/test/coinselector_tests.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 979100d7b2..7bd92b471c 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -881,6 +881,7 @@ BOOST_AUTO_TEST_CASE(waste_test) const CAmount in_amt{3 * COIN}; const CAmount target{2 * COIN}; const CAmount excess{80}; + const CAmount exact_target{in_amt - fee * 2}; // Maximum spendable amount after fees: no change, no excess // In the following, we test that the waste is calculated correctly in various scenarios. // Usually, RecalculateWaste would compute change_fee and change_cost on basis of the @@ -889,7 +890,7 @@ BOOST_AUTO_TEST_CASE(waste_test) { // Waste with change is the change cost and difference between fee and long term fee SelectionResult selection1{target, SelectionAlgorithm::MANUAL}; - add_coin(1 * COIN, 1, selection1, fee, fee - fee_diff); + add_coin(1 * COIN, 1, selection1, /*fee=*/fee, /*long_term_fee=*/fee - fee_diff); add_coin(2 * COIN, 2, selection1, fee, fee - fee_diff); selection1.RecalculateWaste(min_viable_change, change_cost, change_fee); BOOST_CHECK_EQUAL(fee_diff * 2 + change_cost, selection1.GetWaste()); @@ -913,7 +914,7 @@ BOOST_AUTO_TEST_CASE(waste_test) { // Waste without change is the excess and difference between fee and long term fee - SelectionResult selection_nochange1{/*target creates no change*/in_amt - 2 * fee - excess, SelectionAlgorithm::MANUAL}; + SelectionResult selection_nochange1{exact_target - excess, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection_nochange1, fee, fee - fee_diff); add_coin(2 * COIN, 2, selection_nochange1, fee, fee - fee_diff); selection_nochange1.RecalculateWaste(min_viable_change, change_cost, change_fee); @@ -921,7 +922,7 @@ BOOST_AUTO_TEST_CASE(waste_test) // Waste without change is the excess and difference between fee and long term fee // With long term fee greater than fee, waste should be less than when long term fee is less than fee - SelectionResult selection_nochange2{/*target creates no change*/in_amt - 2 * fee - excess, SelectionAlgorithm::MANUAL}; + SelectionResult selection_nochange2{exact_target - excess, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection_nochange2, fee, fee + fee_diff); add_coin(2 * COIN, 2, selection_nochange2, fee, fee + fee_diff); selection_nochange2.RecalculateWaste(min_viable_change, change_cost, change_fee); @@ -940,7 +941,7 @@ BOOST_AUTO_TEST_CASE(waste_test) { // Waste without change and fee == long term fee is just the excess - SelectionResult selection{/*target creates no change*/in_amt - 2 * fee - excess, SelectionAlgorithm::MANUAL}; + SelectionResult selection{exact_target - excess, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection, fee, fee); add_coin(2 * COIN, 2, selection, fee, fee); selection.RecalculateWaste(min_viable_change, change_cost, change_fee); @@ -949,7 +950,6 @@ BOOST_AUTO_TEST_CASE(waste_test) { // Waste is 0 when fee == long_term_fee, no change, and no excess - const CAmount exact_target{in_amt - fee * 2}; SelectionResult selection{exact_target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection, fee, fee); add_coin(2 * COIN, 2, selection, fee, fee); @@ -968,7 +968,7 @@ BOOST_AUTO_TEST_CASE(waste_test) { // Waste is 0 when (fee - long_term_fee) == (-excess), no change cost - const CAmount new_target{in_amt - fee * 2 - /*excess=*/fee_diff * 2}; + const CAmount new_target{exact_target - /*excess=*/fee_diff * 2}; SelectionResult selection{new_target, SelectionAlgorithm::MANUAL}; add_coin(1 * COIN, 1, selection, fee, fee + fee_diff); add_coin(2 * COIN, 2, selection, fee, fee + fee_diff); @@ -978,7 +978,6 @@ BOOST_AUTO_TEST_CASE(waste_test) { // Negative waste when the long term fee is greater than the current fee and the selected value == target - const CAmount exact_target{in_amt - 2 * fee}; SelectionResult selection{exact_target, SelectionAlgorithm::MANUAL}; const CAmount target_waste1{-2 * fee_diff}; // = (2 * fee) - (2 * (fee + fee_diff)) add_coin(1 * COIN, 1, selection, fee, fee + fee_diff); @@ -991,7 +990,11 @@ BOOST_AUTO_TEST_CASE(waste_test) // Negative waste when the long term fee is greater than the current fee and change_cost < - (inputs * (fee - long_term_fee)) SelectionResult selection{target, SelectionAlgorithm::MANUAL}; const CAmount large_fee_diff{90}; - const CAmount target_waste2{-2 * large_fee_diff + change_cost}; // = (2 * fee) - (2 * (fee + large_fee_diff)) + change_cost + const CAmount target_waste2{-2 * large_fee_diff + change_cost}; + // = (2 * fee) - (2 * (fee + large_fee_diff)) + change_cost + // = (2 * 100) - (2 * (100 + 90)) + 125 + // = 200 - 380 + 125 = -55 + assert(target_waste2 == -55); add_coin(1 * COIN, 1, selection, fee, fee + large_fee_diff); add_coin(2 * COIN, 2, selection, fee, fee + large_fee_diff); selection.RecalculateWaste(min_viable_change, change_cost, change_fee); -- cgit v1.2.3