From 445f489452b914d81b90d0b0ced1f75d2b0cd806 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 9 Feb 2025 09:21:44 +0000 Subject: [PATCH] merge bitcoin#17331: Use effective values throughout coin selection excludes: - 1bf4a62cb61bd4b91d9cd4e379fea2b914786342 - af5867c89688b06173b295b7c32a42845ea455da - d97d25d95006725e705635530b27643363d6b2a4 - cc3f14b27c06b7a0da1472f5c7100c3f0b76fd98 --- src/bench/coin_selection.cpp | 8 +- src/wallet/coinselection.cpp | 40 +-- src/wallet/coinselection.h | 52 +++- src/wallet/spend.cpp | 361 +++++++++++-------------- src/wallet/test/coinselector_tests.cpp | 148 +++++----- src/wallet/wallet.h | 48 +--- 6 files changed, 305 insertions(+), 352 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index e53755c046c19..da29c8bddbb92 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -48,15 +48,14 @@ static void CoinSelection(benchmark::Bench& bench) coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */); } const CoinEligibilityFilter filter_standard(1, 6, 0); - const CoinSelectionParams coin_selection_params(/* use_bnb= */ true, /* change_output_size= */ 34, + const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34, /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); bench.run([&] { std::set setCoinsRet; CAmount nValueRet; - bool bnb_used; - bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used); + bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params); assert(success); assert(nValueRet == 1003 * COIN); assert(setCoinsRet.size() == 2); @@ -94,12 +93,11 @@ static void BnBExhaustion(benchmark::Bench& bench) std::vector utxo_pool; CoinSet selection; CAmount value_ret = 0; - CAmount not_input_fees = 0; bench.run([&] { // Benchmark CAmount target = make_hard_case(17, utxo_pool); - SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret, not_input_fees); // Should exhaust + SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret); // Should exhaust // Cleanup utxo_pool.clear(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index e3919105023fc..a657b282c3b07 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -16,7 +16,7 @@ struct { bool operator()(const OutputGroup& a, const OutputGroup& b) const { - return a.effective_value > b.effective_value; + return a.GetSelectionAmount() > b.GetSelectionAmount(); } } descending; @@ -51,35 +51,32 @@ struct { * @param const std::vector& utxo_pool The set of UTXOs that we are choosing from. * These UTXOs will be sorted in descending order by effective value and the CInputCoins' * values are their effective values. - * @param const CAmount& target_value This is the value that we want to select. It is the lower + * @param const CAmount& selection_target This is the value that we want to select. It is the lower * bound of the range. * @param const CAmount& cost_of_change This is the cost of creating and spending a change output. - * This plus target_value is the upper bound of the range. + * This plus selection_target is the upper bound of the range. * @param std::set& out_set -> This is an output parameter for the set of CInputCoins * that have been selected. * @param CAmount& value_ret -> This is an output parameter for the total value of the CInputCoins * that were selected. - * @param CAmount not_input_fees -> The fees that need to be paid for the outputs and fixed size - * overhead (version, locktime, marker and flag) */ static const size_t TOTAL_TRIES = 100000; -bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret, CAmount not_input_fees) +bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret) { out_set.clear(); CAmount curr_value = 0; std::vector curr_selection; // select the utxo at this index curr_selection.reserve(utxo_pool.size()); - CAmount selection_target = not_input_fees + target_value; // 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.effective_value > 0); - curr_available_value += utxo.effective_value; + assert(utxo.GetSelectionAmount() > 0); + curr_available_value += utxo.GetSelectionAmount(); } if (curr_available_value < selection_target) { return false; @@ -123,7 +120,7 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v // Walk backwards to find the last included UTXO that still needs to have its omission branch traversed. while (!curr_selection.empty() && !curr_selection.back()) { curr_selection.pop_back(); - curr_available_value += utxo_pool.at(curr_selection.size()).effective_value; + curr_available_value += utxo_pool.at(curr_selection.size()).GetSelectionAmount(); } if (curr_selection.empty()) { // We have walked back to the first utxo and no branch is untraversed. All solutions searched @@ -133,24 +130,24 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v // Output was included on previous iterations, try excluding now. curr_selection.back() = false; OutputGroup& utxo = utxo_pool.at(curr_selection.size() - 1); - curr_value -= utxo.effective_value; + curr_value -= utxo.GetSelectionAmount(); curr_waste -= utxo.fee - utxo.long_term_fee; } else { // Moving forwards, continuing down this branch OutputGroup& utxo = utxo_pool.at(curr_selection.size()); // Remove this utxo from the curr_available_value utxo amount - curr_available_value -= utxo.effective_value; + curr_available_value -= utxo.GetSelectionAmount(); // 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. if (!curr_selection.empty() && !curr_selection.back() && - utxo.effective_value == utxo_pool.at(curr_selection.size() - 1).effective_value && + utxo.GetSelectionAmount() == utxo_pool.at(curr_selection.size() - 1).GetSelectionAmount() && utxo.fee == utxo_pool.at(curr_selection.size() - 1).fee) { curr_selection.push_back(false); } else { // Inclusion branch first (Largest First Exploration) curr_selection.push_back(true); - curr_value += utxo.effective_value; + curr_value += utxo.GetSelectionAmount(); curr_waste += utxo.fee - utxo.long_term_fee; } } @@ -283,14 +280,14 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group if (tryDenom == 0 && CoinJoin::IsDenominatedAmount(group.m_value)) { continue; // we don't want denom values on first run } - if (group.m_value == nTargetValue) { + if (group.GetSelectionAmount() == nTargetValue) { util::insert(setCoinsRet, group.m_outputs); nValueRet += group.m_value; return true; - } else if (group.m_value < nTargetValue + nMinChange) { + } else if (group.GetSelectionAmount() < nTargetValue + nMinChange) { applicable_groups.push_back(group); - nTotalLower += group.m_value; - } else if (!lowest_larger || group.m_value < lowest_larger->m_value) { + nTotalLower += group.GetSelectionAmount(); + } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) { lowest_larger = group; } } @@ -336,7 +333,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group // If we have a bigger coin and (either the stochastic approximation didn't find a good solution, // or the next bigger coin is closer), return the bigger coin if (lowest_larger && - ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->m_value <= nBest)) { + ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->GetSelectionAmount() <= nBest)) { util::insert(setCoinsRet, lowest_larger->m_outputs); nValueRet += lowest_larger->m_value; } else { @@ -400,3 +397,8 @@ bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_f && m_ancestors <= eligibility_filter.max_ancestors && m_descendants <= eligibility_filter.max_descendants; } + +CAmount OutputGroup::GetSelectionAmount() const +{ + return m_subtract_fee_outputs ? m_value : effective_value; +} diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 487636ffa413c..cbe80d87c682b 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -57,6 +57,45 @@ class CInputCoin { } }; +/** Parameters for one iteration of Coin Selection. */ +struct CoinSelectionParams +{ + /** Size of a change output in bytes, determined by the output type. */ + size_t change_output_size = 0; + /** Size of the input to spend a change output in virtual bytes. */ + size_t change_spend_size = 0; + /** Cost of creating the change output. */ + CAmount m_change_fee{0}; + /** Cost of creating the change output + cost of spending the change output in the future. */ + CAmount m_cost_of_change{0}; + /** The fee to spend these UTXOs at the long term feerate. */ + CFeeRate m_effective_feerate; + /** The feerate estimate used to estimate an upper bound on what should be sufficient to spend + * the change output sometime in the future. */ + CFeeRate m_long_term_feerate; + /** If the cost to spend a change output at the discard feerate exceeds its value, drop it to fees. */ + CFeeRate m_discard_feerate; + size_t tx_noinputs_size = 0; + /** Indicate that we are subtracting the fee from outputs */ + bool m_subtract_fee_outputs = false; + /** When true, always spend all (up to OUTPUT_GROUP_MAX_ENTRIES) or none of the outputs + * associated with the same address. This helps reduce privacy leaks resulting from address + * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */ + bool m_avoid_partial_spends = false; + + CoinSelectionParams(size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate, + CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) : + change_output_size(change_output_size), + change_spend_size(change_spend_size), + m_effective_feerate(effective_feerate), + m_long_term_feerate(long_term_feerate), + m_discard_feerate(discard_feerate), + tx_noinputs_size(tx_noinputs_size), + m_avoid_partial_spends(avoid_partial) + {} + CoinSelectionParams() {} +}; + /** Parameters for filtering which OutputGroups we may use in coin selection. * We start by being very selective and requiring multiple confirmations and * then get more permissive if we cannot fund the transaction. */ @@ -109,18 +148,23 @@ struct OutputGroup * a lower feerate). Calculated using long term fee estimate. This is used to decide whether * it could be economical to create a change output. */ CFeeRate m_long_term_feerate{0}; + /** Indicate that we are subtracting the fee from outputs. + * When true, the value that is used for coin selection is the UTXO's real value rather than effective value */ + bool m_subtract_fee_outputs{false}; OutputGroup() {} - OutputGroup(const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) : - m_effective_feerate(effective_feerate), - m_long_term_feerate(long_term_feerate) + OutputGroup(const CoinSelectionParams& params) : + m_effective_feerate(params.m_effective_feerate), + m_long_term_feerate(params.m_long_term_feerate), + m_subtract_fee_outputs(params.m_subtract_fee_outputs) {} void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter, bool isISLocked) const; + CAmount GetSelectionAmount() const; }; -bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret, CAmount not_input_fees); +bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret); // Original coin selection algorithm as a fallback bool KnapsackSolver(const CAmount& nTargetValue, std::vector& groups, std::set& setCoinsRet, CAmount& nValueRet, bool fFulyMixedOnly, CAmount maxTxFee); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 2d42e9232a015..61d5ab1e8f0bd 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -271,12 +271,12 @@ static bool isGroupISLocked(const OutputGroup& group, interfaces::Chain& chain) }); } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const +std::vector CWallet::GroupOutputs(const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) const { std::vector groups_out; - if (separate_coins) { - // Single coin means no grouping. Each COutput gets its own OutputGroup. + if (!coin_sel_params.m_avoid_partial_spends) { + // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup. for (const COutput& output : outputs) { // Skip outputs we cannot spend if (!output.fSpendable) continue; @@ -286,11 +286,11 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu CInputCoin input_coin = output.GetInputCoin(); // Make an OutputGroup containing just this output - OutputGroup group{effective_feerate, long_term_feerate}; + OutputGroup group{coin_sel_params}; group.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.effective_value <= 0) continue; + if (positive_only && group.GetSelectionAmount() <= 0) continue; bool isISLocked = isGroupISLocked(group, chain()); if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter, isISLocked)) groups_out.push_back(group); } @@ -317,7 +317,7 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu if (groups.size() == 0) { // No OutputGroups for this scriptPubKey yet, add one - groups.emplace_back(effective_feerate, long_term_feerate); + groups.emplace_back(coin_sel_params); } // Get the last OutputGroup in the vector so that we can add the CInputCoin to it @@ -328,7 +328,7 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu // to avoid surprising users with very high fees. if (group->m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { // The last output group is full, add a new group to the vector and use that group for the insertion - groups.emplace_back(effective_feerate, long_term_feerate); + groups.emplace_back(coin_sel_params); group = &groups.back(); } @@ -350,7 +350,7 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu } // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.effective_value <= 0) continue; + if (positive_only && group.GetSelectionAmount() <= 0) continue; bool isISLocked = isGroupISLocked(group, chain()); if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter, isISLocked)) groups_out.push_back(group); } @@ -360,35 +360,24 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu } bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, - std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used, CoinType nCoinType) const + std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType) const { setCoinsRet.clear(); nValueRet = 0; - // Calculate the fees for things that aren't inputs, excluding the change output - const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); - - // Get the feerate for effective value. - // When subtracting the fee from the outputs, we want the effective feerate to be 0 - CFeeRate effective_feerate{0}; - if (!coin_selection_params.m_subtract_fee_outputs) { - effective_feerate = coin_selection_params.m_effective_feerate; - } - - if (coin_selection_params.use_bnb) { - std::vector positive_groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */); - bnb_used = true; - return SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet, not_input_fees); - } else { - // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. - // The knapsack solver currently does not use effective values, so we give GroupOutputs feerates of 0 so it sets the effective values to be the same as the real value. - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */); - bnb_used = false; - return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet, nCoinType == CoinType::ONLY_FULLY_MIXED, m_default_max_tx_fee); + // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output. + std::vector positive_groups = GroupOutputs(coins, coin_selection_params, eligibility_filter, true /* positive_only */); + if (SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet)) { + return true; } + // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. + std::vector all_groups = GroupOutputs(coins, coin_selection_params, eligibility_filter, false /* positive_only */); + // While nTargetValue includes the transaction fees for non-input things, it does not include the fee for creating a change output. + // So we need to include that for KnapsackSolver as well, as we are expecting to create a change output. + return KnapsackSolver(nTargetValue + coin_selection_params.m_change_fee, all_groups, setCoinsRet, nValueRet, nCoinType == CoinType::ONLY_FULLY_MIXED, m_default_max_tx_fee); } -bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const +bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params) const { // Note: this function should never be used for "always free" tx types like dstx @@ -396,9 +385,6 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm CoinType nCoinType = coin_control.nCoinType; CAmount value_to_select = nTargetValue; - // Default to bnb was not used. If we use it, we set it later - bnb_used = false; - // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs) { @@ -447,10 +433,10 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm return false; // Not solvable, can't estimate size for fee } coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); - if (coin_selection_params.use_bnb) { - value_to_select -= coin.effective_value; - } else { + if (coin_selection_params.m_subtract_fee_outputs) { value_to_select -= coin.txout.nValue; + } else { + value_to_select -= coin.effective_value; } setPresetCoins.insert(coin); } else { @@ -491,26 +477,26 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six // confirmations on outputs received from other wallets and only spend confirmed change. - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; + if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; + if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; // Fall back to using zero confirmation change (but with as few ancestors in the mempool as // possible) if we cannot fund the transaction otherwise. if (m_spend_zero_conf_change) { - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; + if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } // If partial groups are allowed, relax the requirement of spending OutputGroups (groups // of UTXOs sent to the same address, which are obviously controlled by a single wallet) // in their entirety. if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs @@ -518,7 +504,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (coin_control.m_include_unsafe_inputs && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0 /* conf_mine */, 0 /* conf_theirs */, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } // Try with unlimited ancestors/descendants. The transaction will still need to meet @@ -526,7 +512,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // OutputGroups use heuristics that may overestimate ancestor/descendant counts. if (!fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max(), std::numeric_limits::max(), true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } } @@ -616,8 +602,7 @@ bool CWallet::CreateTransactionInternal( { CAmount nValue = 0; ReserveDestination reservedest(this); - int nChangePosRequest = nChangePosInOut; - const bool sort_bip69{nChangePosRequest == -1}; + const bool sort_bip69{nChangePosInOut == -1}; unsigned int nSubtractFeeFromAmount = 0; for (const auto& recipient : vecSend) { @@ -721,189 +706,167 @@ bool CWallet::CreateTransactionInternal( coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee; - nFeeRet = 0; - CAmount nValueIn = 0; - - // BnB selector is the only selector used when this is true. - coin_selection_params.use_bnb = false; // Dash: never use BnB coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values - // Start with no fee and loop until there is enough fee, try it 500 times. - int nMaxTries = 500; - while (--nMaxTries > 0) + // vouts to the payees + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size = 9; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count + coin_selection_params.tx_noinputs_size += GetSizeOfCompactSize(vecSend.size()); // bytes for output count + } + for (const auto& recipient : vecSend) { - nChangePosInOut = nChangePosRequest; - txNew.vin.clear(); - txNew.vout.clear(); + CTxOut txout(recipient.nAmount, recipient.scriptPubKey); - CAmount nValueToSelect = nValue; - if (nSubtractFeeFromAmount == 0) - nValueToSelect += nFeeRet; - - // vouts to the payees + // Include the fee cost for outputs. if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size = 9; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count - coin_selection_params.tx_noinputs_size += GetSizeOfCompactSize(vecSend.size()); // bytes for output count + coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); } - for (const auto& recipient : vecSend) - { - CTxOut txout(recipient.nAmount, recipient.scriptPubKey); - // Include the fee cost for outputs. - if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); - } - - if (IsDust(txout, chain().relayDustFee())) - { - error = _("Transaction amount too small"); - return false; - } - txNew.vout.push_back(txout); + if (IsDust(txout, chain().relayDustFee())) + { + error = _("Transaction amount too small"); + return false; } + txNew.vout.push_back(txout); + } - // Choose coins to use - bool bnb_used = false; - nValueIn = 0; - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) { - if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { - error = _("Unable to locate enough non-denominated funds for this transaction."); - } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { - error = _("Unable to locate enough mixed funds for this transaction."); - error = error + Untranslated(" ") + strprintf(_("%s uses exact denominated amounts to send funds, you might simply need to mix some more coins."), gCoinJoinName); - } else { - error = _("Insufficient funds."); - } - return false; + // Include the fees for things that aren't inputs, excluding the change output + const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); + CAmount nValueToSelect = nValue + not_input_fees; + + // Choose coins to use + CAmount inputs_sum = 0; + setCoins.clear(); + if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, coin_control, coin_selection_params)) { + if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { + error = _("Unable to locate enough non-denominated funds for this transaction."); + } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { + error = _("Unable to locate enough mixed funds for this transaction."); + error = error + Untranslated(" ") + strprintf(_("%s uses exact denominated amounts to send funds, you might simply need to mix some more coins."), gCoinJoinName); + } else { + error = _("Insufficient funds."); } + return false; + } - // Always make a change output - // We will reduce the fee from this change output later, and remove the output if it is too small. - const CAmount change_and_fee = nValueIn - nValue; - assert(change_and_fee >= 0); - CTxOut newTxOut(change_and_fee, scriptChange); + // Always make a change output + // We will reduce the fee from this change output later, and remove the output if it is too small. + const CAmount change_and_fee = inputs_sum - nValue; + assert(change_and_fee >= 0); + CTxOut newTxOut(change_and_fee, scriptChange); - if (nChangePosInOut == -1) - { - // Insert change txn at random position: - nChangePosInOut = GetRandInt(txNew.vout.size()+1); - } - else if ((unsigned int)nChangePosInOut > txNew.vout.size()) - { - error = _("Transaction change output index out of range"); - return false; + if (nChangePosInOut == -1) + { + // Insert change txn at random position: + nChangePosInOut = GetRandInt(txNew.vout.size()+1); + } + else if ((unsigned int)nChangePosInOut > txNew.vout.size()) + { + error = _("Transaction change output index out of range"); + return false; + } + + assert(nChangePosInOut != -1); + auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + + // We're making a copy of vecSend because it's const, sortedVecSend should be used + // in place of vecSend in all subsequent usage. + std::vector sortedVecSend{vecSend}; + if (sort_bip69) { + std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); + // The output reduction loop uses vecSend to map to txNew.vout, we need to + // shuffle them both to ensure this mapping remains consistent + std::sort(sortedVecSend.begin(), sortedVecSend.end(), + [](const CRecipient& a, const CRecipient& b) { + return a.nAmount < b.nAmount || (a.nAmount == b.nAmount && a.scriptPubKey < b.scriptPubKey); + }); + + // If there was a change output added before, we must update its position now + if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) { + change_position = it; + nChangePosInOut = std::distance(txNew.vout.begin(), change_position); } + }; - assert(nChangePosInOut != -1); - auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); - - // We're making a copy of vecSend because it's const, sortedVecSend should be used - // in place of vecSend in all subsequent usage. - std::vector sortedVecSend{vecSend}; - if (sort_bip69) { - std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); - // The output reduction loop uses vecSend to map to txNew.vout, we need to - // shuffle them both to ensure this mapping remains consistent - std::sort(sortedVecSend.begin(), sortedVecSend.end(), - [](const CRecipient& a, const CRecipient& b) { - return a.nAmount < b.nAmount || (a.nAmount == b.nAmount && a.scriptPubKey < b.scriptPubKey); - }); - - // If there was a change output added before, we must update its position now - if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) { - change_position = it; - nChangePosInOut = std::distance(txNew.vout.begin(), change_position); - } - }; + // Dummy fill vin for maximum size estimation + // + for (const auto& coin : setCoins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); + } - // Dummy fill vin for maximum size estimation - // - for (const auto& coin : setCoins) { - txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); - } + // Calculate the transaction fee + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + if (nBytes < 0) { + error = _("Signing transaction failed"); + return false; + } - // Calculate the transaction fee - nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - if (nBytes < 0) { - error = _("Signing transaction failed"); - return false; - } + if (nExtraPayloadSize != 0) { + // account for extra payload in fee calculation + nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; + } - if (nExtraPayloadSize != 0) { - // account for extra payload in fee calculation - nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; - } + nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); - nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); + CAmount fee_needed = nFeeRet; + if (nSubtractFeeFromAmount == 0) { + change_position->nValue -= fee_needed; + } - CAmount fee_needed = nFeeRet; - if (nSubtractFeeFromAmount == 0) { - change_position->nValue -= fee_needed; - } + // We want to drop the change to fees if: + // 1. The change output would be dust + // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) + // 3. We are working with fully mixed CoinJoin denominations + CAmount change_amount = change_position->nValue; + if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change || coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) + { + nChangePosInOut = -1; + change_amount = 0; + txNew.vout.erase(change_position); - // We want to drop the change to fees if: - // 1. The change output would be dust - // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) - // 3. We are working with fully mixed CoinJoin denominations - CAmount change_amount = change_position->nValue; - if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change || coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) - { - nChangePosInOut = -1; - change_amount = 0; - txNew.vout.erase(change_position); + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); + } - nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - } + // Update nFeeRet in case fee_needed changed due to dropping the change output + if (fee_needed <= change_and_fee - change_amount) { + nFeeRet = change_and_fee - change_amount; + } - // If the fee is covered, there's no need to loop or subtract from recipients - if (fee_needed <= change_and_fee - change_amount) { - nFeeRet = change_and_fee - change_amount; - break; - } + // Reduce output values for subtractFeeFromAmount + if (nSubtractFeeFromAmount != 0) { + CAmount to_reduce = fee_needed + change_amount - change_and_fee; + int i = 0; + bool fFirst = true; + for (const auto& recipient : sortedVecSend) + { + if (i == nChangePosInOut) { + ++i; + } + CTxOut& txout = txNew.vout[i]; - // Reduce output values for subtractFeeFromAmount - if (nSubtractFeeFromAmount != 0) { - CAmount to_reduce = fee_needed + change_amount - change_and_fee; - int i = 0; - bool fFirst = true; - for (const auto& recipient : sortedVecSend) + if (recipient.fSubtractFeeFromAmount) { - if (i == nChangePosInOut) { - ++i; - } - CTxOut& txout = txNew.vout[i]; + txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient - if (recipient.fSubtractFeeFromAmount) + if (fFirst) // first receiver pays the remainder not divisible by output count { - txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient - - if (fFirst) // first receiver pays the remainder not divisible by output count - { - fFirst = false; - txout.nValue -= to_reduce % nSubtractFeeFromAmount; - } - // Error if this output is reduced to be below dust - if (IsDust(txout, chain().relayDustFee())) { - if (txout.nValue < 0) { - error = _("The transaction amount is too small to pay the fee"); - } else { - error = _("The transaction amount is too small to send after the fee has been deducted"); - } - return false; + fFirst = false; + txout.nValue -= to_reduce % nSubtractFeeFromAmount; + } + // Error if this output is reduced to be below dust + if (IsDust(txout, chain().relayDustFee())) { + if (txout.nValue < 0) { + error = _("The transaction amount is too small to pay the fee"); + } else { + error = _("The transaction amount is too small to send after the fee has been deducted"); } + return false; } - ++i; } - nFeeRet = fee_needed; - break; // The fee has been deducted from the recipients, nothing left to do here + ++i; } - } - - if (nMaxTries == 0) { - error = _("Exceeded max tries."); - return false; + nFeeRet = fee_needed; } // Give up if change keypool ran out and change is required diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index e7731a9c4db3c..a6d26482464a6 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -34,10 +34,10 @@ static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); static const CoinEligibilityFilter filter_standard_extra(6, 6, 0); -CoinSelectionParams coin_selection_params(/* use_bnb= */ false, /* change_output_size= */ 0, +CoinSelectionParams coin_selection_params(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { @@ -132,14 +132,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CoinSet selection; CoinSet actual_selection; CAmount value_ret = 0; - CAmount not_input_fees = 0; ///////////////////////// // Known Outcome tests // ///////////////////////// // Empty utxo pool - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret)); selection.clear(); // Add utxos @@ -150,7 +149,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Select 1 Cent add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 1 * CENT); actual_selection.clear(); @@ -158,7 +157,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Select 2 Cent add_coin(2 * CENT, 2, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 2 * CENT); actual_selection.clear(); @@ -167,27 +166,27 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Select 5 Cent add_coin(4 * CENT, 4, actual_selection); add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 5 * CENT); actual_selection.clear(); selection.clear(); // Select 11 Cent, not possible - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT, selection, value_ret)); actual_selection.clear(); selection.clear(); // Cost of change is greater than the difference between target value and utxo sum add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK_EQUAL(value_ret, 1 * CENT); BOOST_CHECK(equal_sets(selection, actual_selection)); actual_selection.clear(); selection.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, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0, selection, value_ret)); actual_selection.clear(); selection.clear(); @@ -196,7 +195,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(5 * CENT, 5, actual_selection); add_coin(4 * CENT, 4, actual_selection); add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 10 * CENT); actual_selection.clear(); @@ -207,21 +206,21 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(5 * CENT, 5, actual_selection); add_coin(3 * CENT, 3, actual_selection); add_coin(2 * CENT, 2, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000, selection, value_ret)); BOOST_CHECK_EQUAL(value_ret, 10 * CENT); // FIXME: this test is redundant with the above, because 1 Cent is selected, not "too small" // BOOST_CHECK(equal_sets(selection, actual_selection)); // Select 0.25 Cent, not possible - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT, selection, value_ret)); actual_selection.clear(); selection.clear(); // Iteration exhaustion test CAmount target = make_hard_case(17, utxo_pool); - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret, not_input_fees)); // Should exhaust + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should exhaust target = make_hard_case(14, utxo_pool); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret, not_input_fees)); // Should not exhaust + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should not exhaust // Test same value early bailout optimization utxo_pool.clear(); @@ -238,7 +237,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) for (int i = 0; i < 50000; ++i) { add_coin(5 * CENT, 7, utxo_pool); } - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000, selection, value_ret)); BOOST_CHECK_EQUAL(value_ret, 30 * CENT); BOOST_CHECK(equal_sets(selection, actual_selection)); @@ -252,35 +251,34 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } // 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, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret)); } // Make sure that effective value is working in SelectCoinsMinConf when BnB is used - CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 0, + CoinSelectionParams coin_selection_params_bnb(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000), /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); { std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); wallet->LoadWallet(); LOCK(wallet->cs_wallet); - bool bnb_used; std::vector coins; CoinSet setCoinsRet; CAmount nValueRet; add_coin(coins, *wallet, 1); coins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail - BOOST_CHECK(!wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb)); // Test fees subtracted from output: coins.clear(); add_coin(coins, *wallet, 1 * CENT); coins.at(0).nInputBytes = 40; coin_selection_params_bnb.m_subtract_fee_outputs = true; - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); } @@ -290,7 +288,6 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) wallet->SetupLegacyScriptPubKeyMan(); LOCK(wallet->cs_wallet); - bool bnb_used; std::vector coins; CoinSet setCoinsRet; CAmount nValueRet; @@ -302,9 +299,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_control.fAllowOtherInputs = true; coin_control.Select(COutPoint(coins.at(0).tx->GetHash(), coins.at(0).i)); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); - BOOST_CHECK(wallet->SelectCoins(coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used)); - BOOST_CHECK(bnb_used); - BOOST_CHECK(coin_selection_params_bnb.use_bnb); + BOOST_CHECK(wallet->SelectCoins(coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb)); } } @@ -317,7 +312,6 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) CoinSet setCoinsRet, setCoinsRet2; CAmount nValueRet; - bool bnb_used; std::vector coins; // test multiple times to allow for differences in the shuffle order @@ -326,24 +320,24 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) coins.clear(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); add_coin(coins, *wallet, 1*CENT, 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); // but we can find a new 1 cent - BOOST_CHECK(wallet->SelectCoinsMinConf( 1 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 1 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); add_coin(coins, *wallet, 2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!wallet->SelectCoinsMinConf( 3 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf( 3 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); // we can make 3 cents of new coins - BOOST_CHECK(wallet->SelectCoinsMinConf( 3 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 3 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 3 * CENT); add_coin(coins, *wallet, 5*CENT); // add a mature 5 cent coin, @@ -353,33 +347,33 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard_extra, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard_extra, coins, setCoinsRet, nValueRet, coin_selection_params)); // but we can make 37 cents if we accept new coins from ourself - BOOST_CHECK(wallet->SelectCoinsMinConf(37 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(37 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 37 * CENT); // and we can make 38 cents if we accept all new coins - BOOST_CHECK(wallet->SelectCoinsMinConf(38 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(38 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - BOOST_CHECK(wallet->SelectCoinsMinConf(34 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(34 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - BOOST_CHECK(wallet->SelectCoinsMinConf( 7 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 7 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 7 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - BOOST_CHECK(wallet->SelectCoinsMinConf( 8 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 8 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK(nValueRet == 8 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - BOOST_CHECK(wallet->SelectCoinsMinConf( 9 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 9 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 10 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -393,30 +387,30 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, 30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - BOOST_CHECK(wallet->SelectCoinsMinConf(71 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(!wallet->SelectCoinsMinConf(72 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(71 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(72 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); add_coin(coins, *wallet, 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); add_coin(coins, *wallet, 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - BOOST_CHECK(wallet->SelectCoinsMinConf(11 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(11 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 11 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); @@ -425,11 +419,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, 2*COIN); add_coin(coins, *wallet, 3*COIN); add_coin(coins, *wallet, 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - BOOST_CHECK(wallet->SelectCoinsMinConf(95 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(95 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); - BOOST_CHECK(wallet->SelectCoinsMinConf(195 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(195 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -444,14 +438,14 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * MIN_CHANGE from the 1.5 * MIN_CHANGE // we'll get change smaller than MIN_CHANGE whatever happens, so can expect MIN_CHANGE exactly - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // but if we add a bigger coin, small change is avoided add_coin(coins, *wallet, 1111*MIN_CHANGE); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // if we add more small coins: @@ -459,7 +453,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 7 / 10); // and try again to make 1.0 * MIN_CHANGE - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) @@ -468,7 +462,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(coins, *wallet, 50000 * COIN); - BOOST_CHECK(wallet->SelectCoinsMinConf(500000 * COIN, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(500000 * COIN, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 10U); // in ten coins @@ -481,7 +475,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 6 / 10); add_coin(coins, *wallet, MIN_CHANGE * 7 / 10); add_coin(coins, *wallet, 1111 * MIN_CHANGE); - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1111 * MIN_CHANGE); // we get the bigger coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -491,7 +485,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 6 / 10); add_coin(coins, *wallet, MIN_CHANGE * 8 / 10); add_coin(coins, *wallet, 1111 * MIN_CHANGE); - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // in two coins 0.4+0.6 @@ -502,12 +496,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 100); // trying to make 100.01 from these three coins - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 101 * MIN_CHANGE); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); } @@ -521,7 +515,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - BOOST_CHECK(wallet->SelectCoinsMinConf(2000, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(2000, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); if (amt - 2000 < MIN_CHANGE) { // needs more than one input: @@ -547,17 +541,19 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int i = 0; i < RUN_TESTS; i++) { // picking 50 from 100 coins doesn't depend on the shuffle, // but does depend on randomness in the stochastic approximation code - BOOST_CHECK(wallet->SelectCoinsMinConf(50 * COIN, filter_standard, coins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(wallet->SelectCoinsMinConf(50 * COIN, filter_standard, coins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(KnapsackSolver(50 * COIN, GroupCoins(coins), setCoinsRet, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); + BOOST_CHECK(KnapsackSolver(50 * COIN, GroupCoins(coins), setCoinsRet2, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); BOOST_CHECK(!equal_sets(setCoinsRet, setCoinsRet2)); int fails = 0; for (int j = 0; j < RANDOM_REPEATS; j++) { - // selecting 1 from 100 identical coins depends on the shuffle; this test will fail 1% of the time - // run the test RANDOM_REPEATS times and only complain if all of them fail - BOOST_CHECK(wallet->SelectCoinsMinConf(COIN, filter_standard, coins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(wallet->SelectCoinsMinConf(COIN, filter_standard, coins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + // Test that the KnapsackSolver selects randomly from equivalent coins (same value and same input size). + // When choosing 1 from 100 identical coins, 1% of the time, this test will choose the same coin twice + // which will cause it to fail. + // To avoid that issue, run the test RANDOM_REPEATS times and only complain if all of them fail + BOOST_CHECK(KnapsackSolver(COIN, GroupCoins(coins), setCoinsRet, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); + BOOST_CHECK(KnapsackSolver(COIN, GroupCoins(coins), setCoinsRet2, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); if (equal_sets(setCoinsRet, setCoinsRet2)) fails++; } @@ -577,10 +573,8 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) int fails = 0; for (int j = 0; j < RANDOM_REPEATS; j++) { - // selecting 1 from 100 identical coins depends on the shuffle; this test will fail 1% of the time - // run the test RANDOM_REPEATS times and only complain if all of them fail - BOOST_CHECK(wallet->SelectCoinsMinConf(90*CENT, filter_standard, coins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(wallet->SelectCoinsMinConf(90*CENT, filter_standard, coins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(KnapsackSolver(90*CENT, GroupCoins(coins), setCoinsRet, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); + BOOST_CHECK(KnapsackSolver(90*CENT, GroupCoins(coins), setCoinsRet2, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); if (equal_sets(setCoinsRet, setCoinsRet2)) fails++; } @@ -598,7 +592,6 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) CoinSet setCoinsRet; CAmount nValueRet; - bool bnb_used; std::vector coins; // Test vValue sort order @@ -606,7 +599,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(coins, *wallet, 1000 * COIN); add_coin(coins, *wallet, 3 * COIN); - BOOST_CHECK(wallet->SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1003 * COIN); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); } @@ -645,19 +638,14 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) CAmount target = rand.randrange(balance - 1000) + 1000; // Perform selection - CoinSelectionParams coin_selection_params_knapsack(/* use_bnb= */ false, /* change_output_size= */ 34, - /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), - /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); - CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 34, - /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), - /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + CoinSelectionParams cs_params(/* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); CoinSet out_set; CAmount out_value = 0; - bool bnb_used = false; - BOOST_CHECK(wallet->SelectCoinsMinConf(target, filter_standard, coins, out_set, out_value, coin_selection_params_bnb, bnb_used) || - wallet->SelectCoinsMinConf(target, filter_standard, coins, out_set, out_value, coin_selection_params_knapsack, bnb_used)); + CCoinControl cc; + BOOST_CHECK(wallet->SelectCoins(coins, target, out_set, out_value, cc, cs_params)); BOOST_CHECK_GE(out_value, target); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1d03d8f4f1473..77483eeddcdc5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -253,48 +253,6 @@ struct WalletTxHasher } }; -/** Parameters for one iteration of Coin Selection. */ -struct CoinSelectionParams -{ - /** Toggles use of Branch and Bound instead of Knapsack solver. */ - bool use_bnb = true; - /** Size of a change output in bytes, determined by the output type. */ - size_t change_output_size = 0; - /** Size of the input to spend a change output in virtual bytes. */ - size_t change_spend_size = 0; - /** Cost of creating the change output. */ - CAmount m_change_fee{0}; - /** Cost of creating the change output + cost of spending the change output in the future. */ - CAmount m_cost_of_change{0}; - /** The fee to spend these UTXOs at the long term feerate. */ - CFeeRate m_effective_feerate; - /** The feerate estimate used to estimate an upper bound on what should be sufficient to spend - * the change output sometime in the future. */ - CFeeRate m_long_term_feerate; - /** If the cost to spend a change output at the discard feerate exceeds its value, drop it to fees. */ - CFeeRate m_discard_feerate; - size_t tx_noinputs_size = 0; - /** Indicate that we are subtracting the fee from outputs */ - bool m_subtract_fee_outputs = false; - /** When true, always spend all (up to OUTPUT_GROUP_MAX_ENTRIES) or none of the outputs - * associated with the same address. This helps reduce privacy leaks resulting from address - * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */ - bool m_avoid_partial_spends = false; - - CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate, - CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) : - use_bnb(use_bnb), - change_output_size(change_output_size), - change_spend_size(change_spend_size), - m_effective_feerate(effective_feerate), - m_long_term_feerate(long_term_feerate), - m_discard_feerate(discard_feerate), - tx_noinputs_size(tx_noinputs_size), - m_avoid_partial_spends(avoid_partial) - {} - CoinSelectionParams() {} -}; - class WalletRescanReserver; //forward declarations for ScanForWalletTransactions/RescanFromTime /** * A CWallet maintains a set of transactions and balances, and provides the ability to create new transactions. @@ -474,7 +432,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * from coin_control and Coin Selection if successful. */ bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, - const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Get a name for this wallet for logging/debugging purposes. */ @@ -588,7 +546,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * param@[out] setCoinsRet Populated with the coins selected if successful. * param@[out] nValueRet Used to return the total value of selected coins. */ - bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used, CoinType nCoinType = CoinType::ALL_COINS) const; + bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS) const; // Coin selection bool SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector& vecTxDSInRet); @@ -615,7 +573,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; + std::vector GroupOutputs(const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) const; void RecalculateMixedCredit(const uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);