From b288d9ec481878f5082e54760a2014a758ee4358 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 11 Jul 2026 21:26:34 -0500 Subject: [PATCH 01/16] Merge bitcoin/bitcoin#25647: wallet: return change from SelectionResult 4fef5344288e454460b80db0316294e1ec1ad8ad wallet: use GetChange() when computing waste (S3RK) 87e0ef903133492e76b7c7556209554d4a0c3d66 wallet: use GetChange() in tx building (S3RK) 15e97a6886902ebb378829993a972dc52558aa92 wallet: add SelectionResult::GetChange (S3RK) 72cad28da05cfce9e4950f2dc5a709da41d251f4 wallet: calculate and store min_viable_change (S3RK) e3210a722542a9cb5f7e4be72470dbe488c281fd wallet: account for preselected inputs in target (S3RK) f8e796348b644c011ad9a8312356d4426c16cc4b wallet: add SelectionResult::Merge (S3RK) 06f558e4e2164d1916f258c731efe4586728a23b wallet: accurate SelectionResult::m_target (S3RK) c8cf08ea743e430c2bf3fe46439594257b0937e5 wallet: ensure m_min_change_target always covers change fee (S3RK) Pull request description: Benefits: 1. more accurate waste calculation for knapsack. Waste calculation is now consistent with tx building code. Before we always assumed change for knapsack even when the solution is changeless4. 2. simpler tx building code. Only create change output when it's needed 3. makes it easier to correctly account for fees for CPFP inputs (should be done in a follow up) In the first three commits we fix the code to accurately track selection target in `SelectionResult::m_target` Then we introduce new variable `min_change` that represents the minimum viable change amount Then we introduce `SelectionResult::GetChange()` which incapsulates dropping change for fee logic and uses correct values of `SelectionResult::m_target` Then we use `SelectionResult::GetChange()` in both tx building and waste calculation code This PR is a refactoring and shouldn't change the behaviour. There is only one known small change (arguably a bug fix). Before we dropped change output if it's smaller than `cost_of_change` after paying change fees. This is incorrect as `cost_of_change` already includes `change_fee`. ACKs for top commit: achow101: ACK 4fef5344288e454460b80db0316294e1ec1ad8ad Xekyo: crACK 4fef5344288e454460b80db0316294e1ec1ad8ad furszy: Code review ACK 4fef5344 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25647/commits/4fef5344288e454460b80db0316294e1ec1ad8ad Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941 (cherry picked from commit 2bd9aa5a44b88c866c4d98f8a7bf7154049cba31) --- src/wallet/coinselection.cpp | 60 ++++++++++-- src/wallet/coinselection.h | 46 +++++++-- src/wallet/spend.cpp | 124 ++++++++++++------------- src/wallet/test/coinselector_tests.cpp | 13 ++- src/wallet/test/fuzz/coinselection.cpp | 8 +- 5 files changed, 164 insertions(+), 87 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index eff3a31e8322..25d2999a3c29 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -190,7 +190,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool for (const size_t& i : best_selection) { result.AddInput(utxo_pool.at(i)); } - result.ComputeAndSetWaste(CAmount{0}); + result.ComputeAndSetWaste(cost_of_change, cost_of_change, CAmount{0}); assert(best_waste == result.GetWaste()); return result; @@ -211,6 +211,13 @@ util::Result SelectCoinsSRD(const std::vector& utx if (utxo_pool.empty()) return util::Error(); SelectionResult result(target_value, SelectionAlgorithm::SRD); + + // Include change for SRD as we want to avoid making really small change if the selection just + // barely meets the target. Just use the lower bound change target instead of the randomly + // generated one, since SRD will result in a random change amount anyway; avoid making the + // target needlessly large. + target_value += CHANGE_LOWER; + std::priority_queue, MinOutputGroupComparator> heap; std::vector indexes; @@ -530,20 +537,26 @@ CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, return waste; } -CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng) +CAmount GenerateChangeTarget(const CAmount payment_value, const CAmount change_fee, FastRandomContext& rng) { if (payment_value <= CHANGE_LOWER / 2) { - return CHANGE_LOWER; + return change_fee + CHANGE_LOWER; } else { // random value between 50ksat and min (payment_value * 2, 1milsat) const auto upper_bound = std::min(payment_value * 2, CHANGE_UPPER); - return rng.randrange(upper_bound - CHANGE_LOWER) + CHANGE_LOWER; + return change_fee + rng.randrange(upper_bound - CHANGE_LOWER) + CHANGE_LOWER; } } -void SelectionResult::ComputeAndSetWaste(CAmount change_cost) +void SelectionResult::ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee) { - m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective); + const CAmount change = GetChange(min_viable_change, change_fee); + + if (change > 0) { + m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective); + } else { + m_waste = GetSelectionWaste(m_selected_inputs, 0, m_target, m_use_effective); + } } CAmount SelectionResult::GetWaste() const @@ -556,6 +569,11 @@ CAmount SelectionResult::GetSelectedValue() const return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.txout.nValue; }); } +CAmount SelectionResult::GetSelectedEffectiveValue() const +{ + return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.GetEffectiveValue(); }); +} + void SelectionResult::Clear() { m_selected_inputs.clear(); @@ -568,6 +586,16 @@ void SelectionResult::AddInput(const OutputGroup& group) m_use_effective = !group.m_subtract_fee_outputs; } +void SelectionResult::Merge(const SelectionResult& other) +{ + m_target += other.m_target; + m_use_effective |= other.m_use_effective; + if (m_algo == SelectionAlgorithm::MANUAL) { + m_algo = other.m_algo; + } + util::insert(m_selected_inputs, other.m_selected_inputs); +} + const std::set& SelectionResult::GetInputSet() const { return m_selected_inputs; @@ -605,4 +633,24 @@ std::string GetAlgorithmName(const SelectionAlgorithm algo) } assert(false); } + +CAmount SelectionResult::GetChange(const CAmount min_viable_change, const CAmount change_fee) const +{ + // change = SUM(inputs) - SUM(outputs) - fees + // 1) With SFFO we don't pay any fees + // 2) Otherwise we pay all the fees: + // - input fees are covered by GetSelectedEffectiveValue() + // - non_input_fee is included in m_target + // - change_fee + const CAmount change = m_use_effective + ? GetSelectedEffectiveValue() - m_target - change_fee + : GetSelectedValue() - m_target; + + if (change < min_viable_change) { + return 0; + } + + return change; +} + } // namespace wallet diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 7d3456297054..6166786aeedc 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -124,6 +124,10 @@ struct CoinSelectionParams { /** Mininmum change to target in Knapsack solver: select coins to cover the payment and * at least this value of change. */ CAmount m_min_change_target{0}; + /** Minimum amount for creating a change output. + * If change budget is smaller than min_change then we forgo creation of change output. + */ + CAmount min_viable_change{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. */ @@ -251,6 +255,7 @@ struct OutputGroup /** Choose a random change target for each transaction to make it harder to fingerprint the Core * wallet based on the change output values of transactions it creates. + * Change target covers at least change fees and adds a random value on top of it. * The random value is between 50ksat and min(2 * payment_value, 1milsat) * When payment_value <= 25ksat, the value is just 50ksat. * @@ -260,8 +265,9 @@ struct OutputGroup * coins selected are just sufficient to cover the payment amount ("unnecessary input" heuristic). * * @param[in] payment_value Average payment value of the transaction output(s). + * @param[in] change_fee Fee for creating a change output. */ -[[nodiscard]] CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng); +[[nodiscard]] CAmount GenerateChangeTarget(const CAmount payment_value, const CAmount change_fee, FastRandomContext& rng); enum class SelectionAlgorithm : uint8_t { @@ -278,17 +284,16 @@ struct SelectionResult private: /** Set of inputs selected by the algorithm to use in the transaction */ std::set m_selected_inputs; + /** The target the algorithm selected for. Equal to the recipient amount plus non-input fees */ + CAmount m_target; + /** The algorithm used to produce this result */ + SelectionAlgorithm m_algo; /** Whether the input values for calculations should be the effective value (true) or normal value (false) */ bool m_use_effective{false}; /** The computed waste */ std::optional m_waste; public: - /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */ - const CAmount m_target; - /** The algorithm used to produce this result */ - const SelectionAlgorithm m_algo; - explicit SelectionResult(const CAmount target, SelectionAlgorithm algo) : m_target(target), m_algo(algo) {} @@ -297,20 +302,47 @@ struct SelectionResult /** Get the sum of the input values */ [[nodiscard]] CAmount GetSelectedValue() const; + [[nodiscard]] CAmount GetSelectedEffectiveValue() const; + void Clear(); void AddInput(const OutputGroup& group); /** Calculates and stores the waste for this selection via GetSelectionWaste */ - void ComputeAndSetWaste(CAmount change_cost); + void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); [[nodiscard]] CAmount GetWaste() const; + void Merge(const SelectionResult& other); + /** Get m_selected_inputs */ const std::set& GetInputSet() const; /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */ std::vector GetShuffledInputVector() const; bool operator<(SelectionResult other) const; + + /** Get the amount for the change output after paying needed fees. + * + * The change amount is not 100% precise due to discrepancies in fee calculation. + * The final change amount (if any) should be corrected after calculating the final tx fees. + * When there is a discrepancy, most of the time the final change would be slightly bigger than estimated. + * + * Following are the possible factors of discrepancy: + * + non-input fees always include segwit flags + * + input fee estimation always include segwit stack size + * + input fees are rounded individually and not collectively, which leads to small rounding errors + * - input counter size is always assumed to be 1vbyte + * + * @param[in] min_viable_change Minimum amount for change output, if change would be less then we forgo change + * @param[in] change_fee Fees to include change output in the tx + * @returns Amount for change output, 0 when there is no change. + * + */ + CAmount GetChange(const CAmount min_viable_change, const CAmount change_fee) const; + + CAmount GetTarget() const { return m_target; } + + SelectionAlgorithm GetAlgo() const { return m_algo; } }; util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4ca823c4ab5b..ca819789a5e0 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -481,27 +481,15 @@ std::optional ChooseSelectionResult(const CWallet& wallet, cons // 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(wallet, available_coins, coin_selection_params, eligibility_filter, false /* positive_only */); - CAmount target_with_change = nTargetValue; - // 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. - // There is also no change output when spending fully mixed coins. - if (!coin_selection_params.m_subtract_fee_outputs && nCoinType != CoinType::ONLY_FULLY_MIXED) { - target_with_change += coin_selection_params.m_change_fee; - } - if (auto knapsack_result{KnapsackSolver(all_groups, target_with_change, coin_selection_params.m_min_change_target, + if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast, max_inputs_weight, nCoinType == CoinType::ONLY_FULLY_MIXED, wallet.m_default_max_tx_fee)}) { - knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + knapsack_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*knapsack_result); } - // Include change for SRD as we want to avoid making really small change if the selection just - // barely meets the target. Just use the lower bound change target instead of the randomly - // generated one, since SRD will result in a random change amount anyway; avoid making the - // target needlessly large. - const CAmount srd_target = target_with_change + CHANGE_LOWER; - if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast, max_inputs_weight)}) { - srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + if (auto srd_result{SelectCoinsSRD(positive_groups, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight)}) { + srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*srd_result); } @@ -602,7 +590,7 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a result.AddInput(preset_inputs); } if (result.GetSelectedValue() < nTargetValue) return std::nullopt; - result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); return result; } @@ -628,12 +616,16 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a Shuffle(available_coins.legacy.begin(), available_coins.legacy.end(), coin_selection_params.rng_fast); Shuffle(available_coins.other.begin(), available_coins.other.end(), coin_selection_params.rng_fast); } + + SelectionResult preselected(preset_inputs.GetSelectionAmount(), SelectionAlgorithm::MANUAL); + preselected.AddInput(preset_inputs); + // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the // transaction at a target feerate. If an attempt fails, more attempts may be made using a more // permissive CoinEligibilityFilter. std::optional res = [&] { // Pre-selected inputs already cover the target amount. - if (value_to_select <= 0) return std::make_optional(SelectionResult(nTargetValue, SelectionAlgorithm::MANUAL)); + if (value_to_select <= 0) return std::make_optional(SelectionResult(value_to_select, SelectionAlgorithm::MANUAL)); // 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. @@ -687,9 +679,9 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a if (!res) return std::nullopt; // Add preset inputs to result - res->AddInput(preset_inputs); - if (res->m_algo == SelectionAlgorithm::MANUAL) { - res->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + res->Merge(preselected); + if (res->GetAlgo() == SelectionAlgorithm::MANUAL) { + res->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); } return res; @@ -802,7 +794,6 @@ static util::Result CreateTransactionInternal( coin_selection_params.m_subtract_fee_outputs = true; } } - coin_selection_params.m_min_change_target = GenerateChangeTarget(std::floor(recipients_sum / vecSend.size()), rng_fast); // Create change script that will be used if we need change CScript scriptChange; @@ -871,10 +862,25 @@ static util::Result 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; + coin_selection_params.m_min_change_target = GenerateChangeTarget(std::floor(recipients_sum / vecSend.size()), coin_selection_params.m_change_fee, rng_fast); + + // The smallest change amount should be: + // 1. at least equal to dust threshold + // 2. at least 1 sat greater than fees to spend it at m_discard_feerate + const auto dust = GetDustThreshold(change_prototype_txout, coin_selection_params.m_discard_feerate); + const auto change_spend_fee = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size); + coin_selection_params.min_viable_change = std::max(change_spend_fee + 1, dust); + // 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 + if (nExtraPayloadSize != 0) { + // Special txes carry an extra payload which is not part of txNew, but is accounted for + // in the final size below. Coin Selection now derives the change amount from this + // target, so the payload has to be included here or we would underpay the fee. + coin_selection_params.tx_noinputs_size += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; + } } for (const auto& recipient : vecSend) { @@ -921,25 +927,26 @@ static util::Result CreateTransactionInternal( } return util::Error{_("Insufficient funds.")}; } - TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->m_algo).c_str(), result->m_target, result->GetWaste(), result->GetSelectedValue()); - - // 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 = result->GetSelectedValue() - recipients_sum; - assert(change_and_fee >= 0); - CTxOut newTxOut(change_and_fee, scriptChange); - - if (nChangePosInOut == -1) { - // Insert change txn at random position: - nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1); - } - else if ((unsigned int)nChangePosInOut > txNew.vout.size()) { - return util::Error{_("Transaction change output index out of range")}; + TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->GetAlgo()).c_str(), result->GetTarget(), result->GetWaste(), result->GetSelectedValue()); + + // Dash: fully mixed CoinJoin sends spend exact denominated amounts and must never create a + // change output; any excess is dropped to fees instead. + const CAmount change_amount = coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED + ? 0 + : result->GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee); + CTxOut newTxOut(change_amount, scriptChange); + if (change_amount > 0) { + if (nChangePosInOut == -1) { + // Insert change txn at random position: + nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1); + } else if ((unsigned int)nChangePosInOut > txNew.vout.size()) { + return util::Error{_("Transaction change output index out of range")}; + } + txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + } else { + nChangePosInOut = -1; } - 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}; @@ -953,9 +960,10 @@ static util::Result CreateTransactionInternal( }); // 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); + if (nChangePosInOut != -1) { + const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); + assert(it != txNew.vout.end()); + nChangePosInOut = std::distance(txNew.vout.begin(), it); } }; @@ -982,27 +990,7 @@ static util::Result CreateTransactionInternal( nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; } - CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - - if (!coin_selection_params.m_subtract_fee_outputs) { - 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); - - nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control); - fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - } - + const CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); nFeeRet = result->GetSelectedValue() - recipients_sum - change_amount; // The only time that fee_needed should be less than the amount available for fees is when @@ -1011,14 +999,16 @@ static util::Result CreateTransactionInternal( return util::Error{Untranslated(STR_INTERNAL_BUG("Fee needed > fee paid"))}; } - // 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 there is a change output and we overpay the fees then increase the change to match the fee needed + if (nChangePosInOut != -1 && fee_needed < nFeeRet) { + auto& change = txNew.vout.at(nChangePosInOut); + change.nValue += nFeeRet - fee_needed; + nFeeRet = fee_needed; } // Reduce output values for subtractFeeFromAmount if (coin_selection_params.m_subtract_fee_outputs) { - CAmount to_reduce = fee_needed + change_amount - change_and_fee; + CAmount to_reduce = fee_needed - nFeeRet; int i = 0; bool fFirst = true; for (const auto& recipient : sortedVecSend) diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 0b940dcb7635..e47f94ecf42b 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -258,9 +258,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Iteration exhaustion test CAmount target = make_hard_case(17, utxo_pool); - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0)); // Should exhaust + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 1)); // Should exhaust target = make_hard_case(14, utxo_pool); - const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 0); // Should not exhaust + const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 1); // Should not exhaust BOOST_CHECK(result7); // Test same value early bailout optimization @@ -299,8 +299,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Make sure that effective value is working in AttemptSelection when BnB is used CoinSelectionParams coin_selection_params_bnb{ rand, - /*change_output_size=*/ 0, - /*change_spend_size=*/ 0, + /*change_output_size=*/ 31, + /*change_spend_size=*/ 68, /*min_change_target=*/ 0, /*effective_feerate=*/ CFeeRate(3000), /*long_term_feerate=*/ CFeeRate(1000), @@ -308,6 +308,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; + coin_selection_params_bnb.m_change_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_output_size); + coin_selection_params_bnb.m_cost_of_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee; + coin_selection_params_bnb.min_viable_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size); coin_selection_params_bnb.m_subtract_fee_outputs = true; { @@ -789,6 +792,8 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; + cs_params.m_cost_of_change = 1; + cs_params.min_viable_change = 1; CCoinControl cc; const auto result = SelectCoins(*wallet, available_coins, target, cc, cs_params); BOOST_CHECK(result); diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index e10a8abbf1d4..a5a9eb19bb7c 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -61,6 +61,8 @@ FUZZ_TARGET(coinselection) coin_params.m_subtract_fee_outputs = subtract_fee_outputs; coin_params.m_long_term_feerate = long_term_fee_rate; coin_params.m_effective_feerate = effective_fee_rate; + coin_params.change_output_size = fuzzed_data_provider.ConsumeIntegralInRange(10, 1000); + coin_params.m_change_fee = effective_fee_rate.GetFee(coin_params.change_output_size); // Create some coins CAmount total_balance{0}; @@ -86,12 +88,12 @@ FUZZ_TARGET(coinselection) const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_SIZE); auto result_srd = SelectCoinsSRD(group_pos, target, fast_random_context, MAX_STANDARD_TX_SIZE); - if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change); + if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); - CAmount change_target{GenerateChangeTarget(target, fast_random_context)}; + CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)}; auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_SIZE, /*fFullyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE); - if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change); + if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); // If the total balance is sufficient for the target and we are not using // effective values, Knapsack should always find a solution unless the selection exceeds max tx weight. From 79cb403d3e3b3939339ea005a312c5097e12659f Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sun, 12 Jul 2026 08:55:31 -0500 Subject: [PATCH 02/16] Merge bitcoin/bitcoin#26021: wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error e06676377d935c69f0ee51fc18eb0772d524aba5 wallet: coverage for loading an unknown descriptor (furszy) d26c3cc44438ecb9e4f618a2427c3c92a292aa16 wallet: bugfix, load wallet with an unknown descriptor cause fatal error (furszy) Pull request description: Fixes #26015 If the descriptor entry is unrecognized (due a soft downgrade) or corrupt, the unserialization fails and `LoadWallet`, instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized or corrupt descriptor are scanned, a fatal error is being thrown. This fixes it by catching the descriptor parse failure and return which wallet failed. Logging its name/path, so the user can remove it from the settings file, to prevent its load at startup. Note: added the test in a separate file intentionally. Will continue adding coverage for the wallet load process in follow-up PRs. ACKs for top commit: achow101: ACK e06676377d935c69f0ee51fc18eb0772d524aba5 Sjors: re-utACK e06676377d935c69f0ee51fc18eb0772d524aba5 Tree-SHA512: d1f1a5d7e944c89c97a33b25b4411a36a11edae172c22f8524f69c84a035f84c570b284679f901fe60f1300f781b76a6c17b015a8e7ad44ebd25a0c295ef260f (cherry picked from commit c85688347eac1a79099ae61dfc37e69e3fa3b220) --- src/Makefile.test.include | 3 +- src/wallet/test/walletload_tests.cpp | 54 ++++++++++++++++++++++++++++ src/wallet/wallet.cpp | 5 +++ src/wallet/walletdb.cpp | 27 ++++++++++---- src/wallet/walletdb.h | 3 +- 5 files changed, 84 insertions(+), 8 deletions(-) create mode 100644 src/wallet/test/walletload_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index e98111bed99c..d1bda4be549f 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -224,7 +224,8 @@ BITCOIN_TESTS += \ wallet/test/init_tests.cpp \ wallet/test/ismine_tests.cpp \ wallet/test/rpc_util_tests.cpp \ - wallet/test/scriptpubkeyman_tests.cpp + wallet/test/scriptpubkeyman_tests.cpp \ + wallet/test/walletload_tests.cpp FUZZ_SUITE_LD_COMMON +=\ $(SQLITE_LIBS) \ diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp new file mode 100644 index 000000000000..c77768a20564 --- /dev/null +++ b/src/wallet/test/walletload_tests.cpp @@ -0,0 +1,54 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include +#include + +#include + +namespace wallet { + +BOOST_AUTO_TEST_SUITE(walletload_tests) + +class DummyDescriptor final : public Descriptor { +private: + std::string desc; +public: + explicit DummyDescriptor(const std::string& descriptor) : desc(descriptor) {}; + ~DummyDescriptor() = default; + + std::string ToString() const override { return desc; } + std::optional GetOutputType() const override { return OutputType::UNKNOWN; } + + bool IsRange() const override { return false; } + bool IsSolvable() const override { return false; } + bool IsSingleType() const override { return true; } + bool ToPrivateString(const SigningProvider& provider, std::string& out) const override { return false; } + bool ToNormalizedString(const SigningProvider& provider, std::string& out, const DescriptorCache* cache = nullptr) const override { return false; } + bool Expand(int pos, const SigningProvider& provider, std::vector& output_scripts, FlatSigningProvider& out, DescriptorCache* write_cache = nullptr) const override { return false; }; + bool ExpandFromCache(int pos, const DescriptorCache& read_cache, std::vector& output_scripts, FlatSigningProvider& out) const override { return false; } + void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const override {} +}; + +BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup) +{ + std::unique_ptr database = CreateMockWalletDatabase(); + { + // Write unknown active descriptor + WalletBatch batch(*database, false); + std::string unknown_desc = "trx(tpubD6NzVbkrYhZ4Y4S7m6Y5s9GD8FqEMBy56AGphZXuagajudVZEnYyBahZMgHNCTJc2at82YX6s8JiL1Lohu5A3v1Ur76qguNH4QVQ7qYrBQx/86'/1'/0'/0/*)#8pn8tzdt"; + WalletDescriptor wallet_descriptor(std::make_shared(unknown_desc), 0, 0, 0, 0); + BOOST_CHECK(batch.WriteDescriptor(uint256(), wallet_descriptor)); + BOOST_CHECK(batch.WriteActiveScriptPubKeyMan(uint256(), false)); + } + + { + // Now try to load the wallet and verify the error. + const std::shared_ptr wallet(new CWallet(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, std::move(database))); + BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::UNKNOWN_DESCRIPTOR); + } +} + +BOOST_AUTO_TEST_SUITE_END() +} // namespace wallet diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 94dd93781d2c..fd1ee885885b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3135,6 +3135,11 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." " Rescanning wallet."), walletFile)); rescan_required = true; + } else if (nLoadWalletRet == DBErrors::UNKNOWN_DESCRIPTOR) { + error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" + "The wallet might had been created on a newer version.\n" + "Please try running the latest software version.\n"), walletFile); + return nullptr; } else { error = strprintf(_("Error loading %s"), walletFile); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index dd7366ca30b7..e1c6548a1c82 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -340,6 +340,7 @@ class CWalletScanState { std::map, std::pair> mnemonics; std::map, std::pair, std::vector>> crypted_mnemonics; bool tx_corrupt{false}; + bool descriptor_unknown{false}; CWalletScanState() = default; }; @@ -644,7 +645,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, uint256 id; ssKey >> id; WalletDescriptor desc; - ssValue >> desc; + try { + ssValue >> desc; + } catch (const std::ios_base::failure& e) { + strErr = e.what(); + wss.descriptor_unknown = true; + return false; + } if (wss.m_descriptor_caches.count(id) == 0) { wss.m_descriptor_caches[id] = DescriptorCache(); } @@ -817,6 +824,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) DBErrors result = DBErrors::LOAD_OK; LOCK(pwallet->cs_wallet); + + // Last client version to open this wallet + int last_client = CLIENT_VERSION; + bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client); + pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client); + try { int nMinVersion = 0; if (m_batch->Read(DBKeys::MINVERSION, nMinVersion)) { @@ -882,6 +895,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx) wss.tx_corrupt = false; result = DBErrors::CORRUPT; + } else if (wss.descriptor_unknown) { + strErr = strprintf("Error: Unrecognized descriptor found in wallet %s. ", pwallet->GetName()); + strErr += (last_client > CLIENT_VERSION) ? "The wallet might had been created on a newer version. " : + "The database might be corrupted or the software version is not compatible with one of your wallet descriptors. "; + strErr += "Please try running the latest software version"; + pwallet->WalletLogPrintf("%s\n", strErr); + return DBErrors::UNKNOWN_DESCRIPTOR; } else { // Leave other errors alone, if we try to fix them we might make things worse. fNoncriticalErrors = true; // ... but do warn the user there is something wrong. @@ -966,11 +986,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) pwallet->nKeysLeftSinceAutoBackup = pwallet->KeypoolCountExternalKeys(); pwallet->WalletLogPrintf("nKeysLeftSinceAutoBackup: %d\n", pwallet->nKeysLeftSinceAutoBackup); - // Last client version to open this wallet - int last_client = CLIENT_VERSION; - bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client); - pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client); - pwallet->WalletLogPrintf("Keys: %u plaintext, %u encrypted, %u total; Watch scripts: %u; HD PubKeys: %u; Metadata: %u; Unknown wallet records: %u\n", wss.nKeys, wss.nCKeys, wss.nKeys + wss.nCKeys, wss.nWatchKeys, wss.nHDPubKeys, wss.nKeyMeta, wss.m_unknown_records); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index eb5ce4b37d9a..b93f947de96e 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -57,7 +57,8 @@ enum class DBErrors EXTERNAL_SIGNER_SUPPORT_REQUIRED, LOAD_FAIL, NEED_REWRITE, - NEED_RESCAN + NEED_RESCAN, + UNKNOWN_DESCRIPTOR }; namespace DBKeys { From 2b81fa12f059109c1d64a53bf7a2eed74c31e346 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 11 Jul 2026 21:28:03 -0500 Subject: [PATCH 03/16] Merge bitcoin/bitcoin#26203: wallet: Use correct effective value when checking target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9 test: Check external coin effective value is used in CoinSelection (Aurèle Oulès) 76b79c1a177afa006184d716bd3d5b22ebadb168 wallet: Use correct effective value when checking target (Aurèle Oulès) Pull request description: Fixes #26185. The following assert failed because it was not checked in the parent function. https://github.com/bitcoin/bitcoin/blob/2bd9aa5a44b88c866c4d98f8a7bf7154049cba31/src/wallet/coinselection.cpp#L391 ACKs for top commit: glozow: reACK d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9 furszy: ACK d0d9cf7a Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9 (cherry picked from commit cda6c791903c10ec20d3a6c4619bfe78ff02f3b6) --- src/wallet/spend.cpp | 8 ++++- src/wallet/test/coinselector_tests.cpp | 47 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index ca819789a5e0..f8dbec27fa92 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -589,7 +589,13 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a if (all_inputs) { result.AddInput(preset_inputs); } - if (result.GetSelectedValue() < nTargetValue) return std::nullopt; + + if (!coin_selection_params.m_subtract_fee_outputs && result.GetSelectedEffectiveValue() < nTargetValue) { + return std::nullopt; + } else if (result.GetSelectedValue() < nTargetValue) { + return std::nullopt; + } + result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); return result; } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index e47f94ecf42b..f556af8693e8 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -934,6 +934,53 @@ BOOST_AUTO_TEST_CASE(effective_value_test) BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1 } +BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) +{ + // Test that the effective value is used to check whether preset inputs provide sufficient funds when subtract_fee_outputs is not used. + // This test creates a coin whose value is higher than the target but whose effective value is lower than the target. + // The coin is selected using coin control, with m_allow_other_inputs = false. SelectCoins should fail due to insufficient funds. + + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase()); + wallet->LoadWallet(); + LOCK(wallet->cs_wallet); + wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet->SetupDescriptorScriptPubKeyMans("", ""); + + CoinsResult available_coins; + { + std::unique_ptr dummyWallet = std::make_unique(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "dummy", m_args, CreateMockWalletDatabase()); + dummyWallet->LoadWallet(); + LOCK(dummyWallet->cs_wallet); + dummyWallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + dummyWallet->SetupDescriptorScriptPubKeyMans("", ""); + + add_coin(available_coins, *dummyWallet, 100000); // 0.001 DASH + } + + CAmount target{99900}; // 0.000999 DASH + + FastRandomContext rand; + CoinSelectionParams cs_params{ + rand, + /*change_output_size=*/34, + /*change_spend_size=*/148, + /*min_change_target=*/1000, + /*effective_feerate=*/CFeeRate(3000), + /*long_term_feerate=*/CFeeRate(1000), + /*discard_feerate=*/CFeeRate(1000), + /*tx_noinputs_size=*/0, + /*avoid_partial=*/false, + }; + CCoinControl cc; + cc.m_allow_other_inputs = false; + COutput output = available_coins.all().at(0); + cc.SetInputWeight(output.outpoint, 148); + cc.SelectExternal(output.outpoint, output.txout); + + const auto result = SelectCoins(*wallet, available_coins, target, cc, cs_params); + BOOST_CHECK(!result); +} + static util::Result SelectCoinsSRDResult(const CAmount& target, const CoinSelectionParams& cs_params, const node::NodeContext& m_node, From cfbfa6edeb446009f7e50d3bcc6bdb298960d3f4 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 11 Jul 2026 22:01:21 -0500 Subject: [PATCH 04/16] Merge bitcoin/bitcoin#25685: wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3fcb545ab26be3e785b5e5654be0bdc77099d827 bench: benchmark transaction creation process (furszy) a8a75346d7e7247596c8a580d65ceaad49c97b97 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy) f41712a734dc119f8a5e053a9cfa1f0411b5e8f1 wallet: simplify preset inputs selection target check (furszy) 5baedc33519661af9d19efcefd23dca8998d2547 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy) 295852f61998a025b0b28a0671e6e1cf0dc08d0d wallet: encapsulate pre-selected-inputs lookup into its own function (furszy) 37e7887cb4bfd7db6eb462ed0741c45aea22a990 wallet: skip manually selected coins from 'AvailableCoins' result (furszy) 94c0766b0cd1990c1399a745c88c2ba4c685d8d1 wallet: skip available coins fetch if "other inputs" are disallowed (furszy) Pull request description: #### # Context (Current Flow on Master) In the transaction creation process, in order to select which coins the new transaction will spend, we first obtain all the available coins known by the wallet, which means walking-through the wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector. This coins vector is then provided to the Coin Selection process, which first checks if the user has manually selected any input (which could be internal, aka known by the wallet, or external), and if it does, it fetches them by searching each of them inside the wallet and/or inside the Coin Control external tx data. Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection process walks-through the entire available coins vector once more just to erase coins that are in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside the same transaction). #### # Process Workflow Changes Now, a new method, `FetchCoins` will be responsible for: 1) Lookup the user pre-selected-inputs (which can be internal or external). 2) And, fetch the available coins in the wallet (excluding the already fetched ones). Which will occur prior to the Coin Selection process. Which allows us to never include the pre-selected-inputs inside the available coins vector in the first place, as well as doing other nice improvements (written below). So, Coin Selection can perform its main responsibility without mixing it with having to fetch internal/external coins nor any slow and unneeded duplicate coins verification. #### # Summarizing the Improvements: 1) If any pre-selected-input lookup fail, the process will return the error right away. (before, the wallet was fetching all the wallet available coins, walking through the entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins) 2) The pre-selected-inputs lookup failure causes are properly described on the return error. (before, we were returning an "Insufficient Funds" error for everything, even if the failure was due a not solvable external input) 3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected). Now, the available coins vector, which is built after the pre-selected-inputs fetching, doesn’t include the already selected inputs in the first place. 4) **Faster transaction creation** for transactions that only use manually selected inputs. We now will return early, as soon as we finish fetching the pre-selected-inputs and not perform the resources expensive calculation of walking-through the entire wallet txes map to obtain the available coins (coins that we will not use). --------------------------- Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically. #### Result on this PR (tip f6d0bb2d): | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction` vs #### Result on master (tip 4a4289e2): | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction` The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 . ACKs for top commit: S3RK: Code Review ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827 achow101: ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827 aureleoules: reACK 3fcb545ab26be3e785b5e5654be0bdc77099d827 Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731 (cherry picked from commit f37bd15d472fdc7dd3d40cafaba9e8dfddd6b530) --- src/Makefile.bench.include | 1 + src/bench/wallet_create_tx.cpp | 141 +++++++++++ src/qt/sendcoinsdialog.cpp | 4 +- src/rpc/evo.cpp | 2 + src/test/util/wallet.cpp | 7 +- src/test/util/wallet.h | 6 +- src/wallet/coincontrol.h | 2 +- src/wallet/coinselection.cpp | 6 + src/wallet/coinselection.h | 1 + src/wallet/spend.cpp | 244 ++++++++++--------- src/wallet/spend.h | 40 ++- src/wallet/test/coinselector_tests.cpp | 38 ++- src/wallet/test/wallet_tests.cpp | 10 + test/functional/rpc_psbt.py | 2 +- test/functional/wallet_fundrawtransaction.py | 16 +- test/functional/wallet_send.py | 2 +- 16 files changed, 384 insertions(+), 138 deletions(-) create mode 100644 src/bench/wallet_create_tx.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index be7bfe2d2dcc..2cd403b48297 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -91,6 +91,7 @@ if ENABLE_WALLET bench_bench_dash_SOURCES += bench/coin_selection.cpp bench_bench_dash_SOURCES += bench/wallet_balance.cpp bench_bench_dash_SOURCES += bench/wallet_create.cpp +bench_bench_dash_SOURCES += bench/wallet_create_tx.cpp bench_bench_dash_SOURCES += bench/wallet_loading.cpp bench_bench_dash_LDADD += $(BDB_LIBS) $(SQLITE_LIBS) endif diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp new file mode 100644 index 000000000000..df826338d934 --- /dev/null +++ b/src/bench/wallet_create_tx.cpp @@ -0,0 +1,141 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using wallet::CWallet; +using wallet::CreateMockWalletDatabase; +using wallet::DBErrors; +using wallet::WALLET_FLAG_DESCRIPTORS; + +struct TipBlock +{ + uint256 prev_block_hash; + int64_t prev_block_time; + int tip_height; +}; + +TipBlock getTip(const CChainParams& params, const node::NodeContext& context) +{ + auto tip = WITH_LOCK(::cs_main, return context.chainman->ActiveTip()); + return (tip) ? TipBlock{tip->GetBlockHash(), tip->GetBlockTime(), tip->nHeight} : + TipBlock{params.GenesisBlock().GetHash(), params.GenesisBlock().GetBlockTime(), 0}; +} + +void generateFakeBlock(const CChainParams& params, + const node::NodeContext& context, + CWallet& wallet, + const CScript& coinbase_out_script) +{ + TipBlock tip{getTip(params, context)}; + + // Create block + CBlock block; + CMutableTransaction coinbase_tx; + coinbase_tx.vin.resize(1); + coinbase_tx.vin[0].prevout.SetNull(); + coinbase_tx.vout.resize(2); + coinbase_tx.vout[0].scriptPubKey = coinbase_out_script; + coinbase_tx.vout[0].nValue = 49 * COIN; + coinbase_tx.vin[0].scriptSig = CScript() << ++tip.tip_height << OP_0; + coinbase_tx.vout[1].scriptPubKey = coinbase_out_script; // extra output + coinbase_tx.vout[1].nValue = 1 * COIN; + block.vtx = {MakeTransactionRef(std::move(coinbase_tx))}; + + block.nVersion = VERSIONBITS_LAST_OLD_BLOCK_VERSION; + block.hashPrevBlock = tip.prev_block_hash; + block.hashMerkleRoot = BlockMerkleRoot(block); + block.nTime = ++tip.prev_block_time; + block.nBits = params.GenesisBlock().nBits; + block.nNonce = 0; + + { + LOCK(::cs_main); + // Add it to the index + CBlockIndex* pindex{context.chainman->m_blockman.AddToBlockIndex(block, block.GetHash(), context.chainman->m_best_header)}; + // add it to the chain + context.chainman->ActiveChain().SetTip(*pindex); + } + + // notify wallet + const auto& pindex = WITH_LOCK(::cs_main, return context.chainman->ActiveChain().Tip()); + wallet.blockConnected(block, pindex->nHeight); +} + +struct PreSelectInputs { + // How many coins from the wallet the process should select + int num_of_internal_inputs; + // future: this could have external inputs as well. +}; + +static void WalletCreateTx(benchmark::Bench& bench, bool allow_other_inputs, std::optional preset_inputs) +{ + const auto test_setup = MakeNoLogFileContext(); + + CWallet wallet{test_setup->m_node.chain.get(), test_setup->m_node.coinjoin_loader.get(), "", gArgs, CreateMockWalletDatabase()}; + { + LOCK(wallet.cs_wallet); + wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet.SetupDescriptorScriptPubKeyMans("", ""); + if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false); + } + + // Generate destinations + CScript dest = GetScriptForDestination(getNewDestination(wallet)); + + // Generate chain; each coinbase will have two outputs to fill-up the wallet + const auto& params = Params(); + unsigned int chain_size = 5000; // 5k blocks means 10k UTXO for the wallet (minus 100 due COINBASE_MATURITY) + for (unsigned int i = 0; i < chain_size; ++i) { + generateFakeBlock(params, test_setup->m_node, wallet, dest); + } + + // Check available balance + auto bal = wallet::GetAvailableBalance(wallet); // Cache + assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY)); + + wallet::CCoinControl coin_control; + coin_control.m_allow_other_inputs = allow_other_inputs; + + CAmount target = 0; + if (preset_inputs) { + // Select inputs, each has 49 DASH + const auto& res = WITH_LOCK(wallet.cs_wallet, + return wallet::AvailableCoins(wallet, nullptr, std::nullopt, 1, MAX_MONEY, + MAX_MONEY, preset_inputs->num_of_internal_inputs)); + for (int i = 0; i < preset_inputs->num_of_internal_inputs; i++) { + const auto& coin{res.legacy[i]}; + target += coin.txout.nValue; + coin_control.Select(coin.outpoint); + } + } + + // If automatic coin selection is enabled, add the value of another UTXO to the target + if (coin_control.m_allow_other_inputs) target += 50 * COIN; + std::vector recipients = {{dest, target, true}}; + + bench.epochIterations(5).run([&] { + LOCK(wallet.cs_wallet); + const auto& tx_res = CreateTransaction(wallet, recipients, wallet::RANDOM_CHANGE_POSITION, coin_control); + assert(tx_res); + }); +} + +static void WalletCreateTxUseOnlyPresetInputs(benchmark::Bench& bench) { WalletCreateTx(bench, /*allow_other_inputs=*/false, + {{/*num_of_internal_inputs=*/4}}); } + +static void WalletCreateTxUsePresetInputsAndCoinSelection(benchmark::Bench& bench) { WalletCreateTx(bench, /*allow_other_inputs=*/true, + {{/*num_of_internal_inputs=*/4}}); } + +BENCHMARK(WalletCreateTxUseOnlyPresetInputs, benchmark::PriorityLevel::LOW) +BENCHMARK(WalletCreateTxUsePresetInputsAndCoinSelection, benchmark::PriorityLevel::LOW) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index db2a304b2410..11b7104de2e0 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -324,7 +324,9 @@ bool SendCoinsDialog::send(const QList& recipients, QString& updateCoinControlState(); - prepareStatus = model->prepareTransaction(*m_current_transaction, *m_coin_control); + CCoinControl coin_control = *m_coin_control; + coin_control.m_allow_other_inputs = !coin_control.HasSelected(); // future, could introduce a checkbox to customize this value. + prepareStatus = model->prepareTransaction(*m_current_transaction, coin_control); // process prepareStatus and on error generate message shown to user processSendCoinsReturn(prepareStatus, diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index 59ff1c0e7cc4..f8d1c29e8ae6 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -366,6 +366,8 @@ static void FundSpecialTx(CWallet& wallet, CMutableTransaction& tx, const Specia CCoinControl coinControl; coinControl.destChange = fundDest; + // Fund from the given address only, spending no more of its coins than necessary. + coinControl.m_allow_other_inputs = false; coinControl.fRequireAllInputs = false; for (const auto& out : AvailableCoinsListUnspent(wallet).all()) { diff --git a/src/test/util/wallet.cpp b/src/test/util/wallet.cpp index e1d59b3bc76e..d610fe314428 100644 --- a/src/test/util/wallet.cpp +++ b/src/test/util/wallet.cpp @@ -25,7 +25,12 @@ const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqq #ifdef ENABLE_WALLET std::string getnewaddress(CWallet& w) { - return EncodeDestination(*Assert(w.GetNewDestination(""))); + return EncodeDestination(getNewDestination(w)); +} + +CTxDestination getNewDestination(CWallet& w) +{ + return *Assert(w.GetNewDestination("")); } // void importaddress(CWallet& wallet, const std::string& address) diff --git a/src/test/util/wallet.h b/src/test/util/wallet.h index d8a81c2d8224..9a96f663daec 100644 --- a/src/test/util/wallet.h +++ b/src/test/util/wallet.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_TEST_UTIL_WALLET_H #define BITCOIN_TEST_UTIL_WALLET_H +#include