From 94c0766b0cd1990c1399a745c88c2ba4c685d8d1 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 5 Aug 2022 11:58:08 -0300 Subject: wallet: skip available coins fetch if "other inputs" are disallowed no need to waste resources calculating the wallet available coins if they are not going to be used. The 'm_allow_other_inputs=true` default value change is to correct an ugly misleading behavior: The tx creation process was having a workaround patch to automatically fall back to select coins from the wallet if `m_allow_other_inputs=false` (previous default value) and no manual inputs were selected. This could be seen in master in flows like `sendtoaddress`, `sendmany` and even the GUI, where the `m_allow_other_inputs` value isn't customized and the wallet still selects and adds coins to the tx internally. --- src/wallet/coincontrol.h | 2 +- src/wallet/spend.cpp | 22 +++++++++++++--------- 2 files changed, 14 insertions(+), 10 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index d08d3664c4..b56a6d3aee 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -37,7 +37,7 @@ public: bool m_include_unsafe_inputs = false; //! If true, the selection process can add extra unselected inputs from the wallet //! while requires all selected inputs be used - bool m_allow_other_inputs = false; + bool m_allow_other_inputs = true; //! Includes watch only addresses which are solvable bool fAllowWatchOnly = false; //! Override automatic min/max checks on fee, m_feerate must be set if true diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 6833f9a095..e65644d8e4 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -579,7 +579,7 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a } // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) - if (coin_control.HasSelected() && !coin_control.m_allow_other_inputs) { + if (!coin_control.m_allow_other_inputs) { SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInput(preset_inputs); @@ -893,14 +893,18 @@ static util::Result CreateTransactionInternal( const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); CAmount selection_target = recipients_sum + not_input_fees; - // Get available coins - auto available_coins = AvailableCoins(wallet, - &coin_control, - coin_selection_params.m_effective_feerate, - 1, /*nMinimumAmount*/ - MAX_MONEY, /*nMaximumAmount*/ - MAX_MONEY, /*nMinimumSumAmount*/ - 0); /*nMaximumCount*/ + // Fetch wallet available coins if "other inputs" are + // allowed (coins automatically selected by the wallet) + CoinsResult available_coins; + if (coin_control.m_allow_other_inputs) { + available_coins = AvailableCoins(wallet, + &coin_control, + coin_selection_params.m_effective_feerate, + 1, /*nMinimumAmount*/ + MAX_MONEY, /*nMaximumAmount*/ + MAX_MONEY, /*nMinimumSumAmount*/ + 0); /*nMaximumCount*/ + } // Choose coins to use std::optional result = SelectCoins(wallet, available_coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); -- cgit v1.2.3 From 37e7887cb4bfd7db6eb462ed0741c45aea22a990 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 5 Aug 2022 12:08:18 -0300 Subject: wallet: skip manually selected coins from 'AvailableCoins' result No need to walk through the entire wallet's txes map just to get coins that we could have gotten by just doing a simple map.find(out.hash). (Which is what we are doing inside `SelectCoins` anyway) --- src/wallet/spend.cpp | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e65644d8e4..cd0c7c4a09 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -230,7 +230,8 @@ CoinsResult AvailableCoins(const CWallet& wallet, if (output.nValue < nMinimumAmount || output.nValue > nMaximumAmount) continue; - if (coinControl && coinControl->HasSelected() && !coinControl->m_allow_other_inputs && !coinControl->IsSelected(outpoint)) + // Skip manually selected coins (the caller can fetch them directly) + if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint)) continue; if (wallet.IsLockedCoin(outpoint)) @@ -528,9 +529,6 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a OutputGroup preset_inputs(coin_selection_params); - // calculate value from preset inputs and store them - std::set preset_coins; - std::vector vPresetInputs; coin_control.ListSelected(vPresetInputs); for (const COutPoint& outpoint : vPresetInputs) { @@ -571,7 +569,6 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a } else { value_to_select -= output.GetEffectiveValue(); } - preset_coins.insert(outpoint); /* Set ancestors and descendants to 0 as they don't matter for preset inputs since no actual selection is being done. * positive_only is set to false because we want to include all preset inputs, even if they are dust. */ @@ -593,11 +590,6 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a return result; } - // remove preset inputs from coins so that Coin Selection doesn't pick them. - if (coin_control.HasSelected()) { - available_coins.Erase(preset_coins); - } - unsigned int limit_ancestor_count = 0; unsigned int limit_descendant_count = 0; wallet.chain().getPackageLimits(limit_ancestor_count, limit_descendant_count); -- cgit v1.2.3 From 295852f61998a025b0b28a0671e6e1cf0dc08d0d Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 22 Jul 2022 16:16:44 -0300 Subject: wallet: encapsulate pre-selected-inputs lookup into its own function First step towards decoupling the pre-selected-inputs fetching functionality from `SelectCoins`. Which, will let us not waste resources calculating the available coins if one of the pre-set inputs has an error. (right now, if one of the pre-set inputs is invalid, we first walk through the entire wallet txes map just to end up failing right after it finish) --- src/wallet/coinselection.cpp | 6 +++ src/wallet/coinselection.h | 1 + src/wallet/spend.cpp | 106 ++++++++++++++++++++++--------------------- src/wallet/spend.h | 20 ++++++++ 4 files changed, 82 insertions(+), 51 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index b568e90998..a8be6cd83a 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -444,6 +444,12 @@ void SelectionResult::AddInput(const OutputGroup& group) m_use_effective = !group.m_subtract_fee_outputs; } +void SelectionResult::AddInputs(const std::set& inputs, bool subtract_fee_outputs) +{ + util::insert(m_selected_inputs, inputs); + m_use_effective = !subtract_fee_outputs; +} + void SelectionResult::Merge(const SelectionResult& other) { m_target += other.m_target; diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 761c2be0b3..b23dd10867 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -308,6 +308,7 @@ public: void Clear(); void AddInput(const OutputGroup& group); + void AddInputs(const std::set& inputs, bool subtract_fee_outputs); /** Calculates and stores the waste for this selection via GetSelectionWaste */ void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index cd0c7c4a09..0b7cb7faa4 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -143,6 +143,51 @@ static OutputType GetOutputType(TxoutType type, bool is_from_p2sh) } } +// Fetch and validate the coin control selected inputs. +// Coins could be internal (from the wallet) or external. +util::Result FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control, + const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +{ + PreSelectedInputs result; + std::vector vPresetInputs; + coin_control.ListSelected(vPresetInputs); + for (const COutPoint& outpoint : vPresetInputs) { + int input_bytes = -1; + CTxOut txout; + if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) { + // Clearly invalid input, fail + if (ptr_wtx->tx->vout.size() <= outpoint.n) { + return util::Error{strprintf(_("Invalid pre-selected input %s"), outpoint.ToString())}; + } + txout = ptr_wtx->tx->vout.at(outpoint.n); + input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control); + } else { + // The input is external. We did not find the tx in mapWallet. + if (!coin_control.GetExternalOutput(outpoint, txout)) { + return util::Error{strprintf(_("Not found pre-selected input %s"), outpoint.ToString())}; + } + } + + if (input_bytes == -1) { + input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, &coin_control); + } + + // If available, override calculated size with coin control specified size + if (coin_control.HasInputWeight(outpoint)) { + input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0); + } + + if (input_bytes == -1) { + return util::Error{strprintf(_("Not solvable pre-selected input %s"), outpoint.ToString())}; // Not solvable, can't estimate size for fee + } + + /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */ + COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, coin_selection_params.m_effective_feerate); + result.Insert(output, coin_selection_params.m_subtract_fee_outputs); + } + return result; +} + CoinsResult AvailableCoins(const CWallet& wallet, const CCoinControl* coinControl, std::optional feerate, @@ -527,58 +572,14 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a { CAmount value_to_select = nTargetValue; - OutputGroup preset_inputs(coin_selection_params); + util::Result pre_selected_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params); + if (!pre_selected_inputs) return std::nullopt; + PreSelectedInputs inputs = *pre_selected_inputs; - std::vector vPresetInputs; - coin_control.ListSelected(vPresetInputs); - for (const COutPoint& outpoint : vPresetInputs) { - int input_bytes = -1; - CTxOut txout; - auto ptr_wtx = wallet.GetWalletTx(outpoint.hash); - if (ptr_wtx) { - // Clearly invalid input, fail - if (ptr_wtx->tx->vout.size() <= outpoint.n) { - return std::nullopt; - } - txout = ptr_wtx->tx->vout.at(outpoint.n); - input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control); - } else { - // The input is external. We did not find the tx in mapWallet. - if (!coin_control.GetExternalOutput(outpoint, txout)) { - return std::nullopt; - } - } - - if (input_bytes == -1) { - input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, &coin_control); - } - - // If available, override calculated size with coin control specified size - if (coin_control.HasInputWeight(outpoint)) { - input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0); - } - - if (input_bytes == -1) { - return std::nullopt; // Not solvable, can't estimate size for fee - } - - /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */ - COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, coin_selection_params.m_effective_feerate); - if (coin_selection_params.m_subtract_fee_outputs) { - value_to_select -= output.txout.nValue; - } else { - value_to_select -= output.GetEffectiveValue(); - } - /* Set ancestors and descendants to 0 as they don't matter for preset inputs since no actual selection is being done. - * positive_only is set to false because we want to include all preset inputs, even if they are dust. - */ - preset_inputs.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); - } - - // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) + // If automatic coin selection was disabled, we just want to return the preset inputs result if (!coin_control.m_allow_other_inputs) { SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); - result.AddInput(preset_inputs); + result.AddInputs(inputs.coins, coin_selection_params.m_subtract_fee_outputs); if (!coin_selection_params.m_subtract_fee_outputs && result.GetSelectedEffectiveValue() < nTargetValue) { return std::nullopt; @@ -590,6 +591,9 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a return result; } + // Decrease the already selected amount + value_to_select -= inputs.total_amount; + unsigned int limit_ancestor_count = 0; unsigned int limit_descendant_count = 0; wallet.chain().getPackageLimits(limit_ancestor_count, limit_descendant_count); @@ -606,8 +610,8 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a available_coins.Shuffle(coin_selection_params.rng_fast); } - SelectionResult preselected(preset_inputs.GetSelectionAmount(), SelectionAlgorithm::MANUAL); - preselected.AddInput(preset_inputs); + SelectionResult preselected(inputs.total_amount, SelectionAlgorithm::MANUAL); + preselected.AddInputs(inputs.coins, coin_selection_params.m_subtract_fee_outputs); // 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 diff --git a/src/wallet/spend.h b/src/wallet/spend.h index c29e5be5c7..39832f363f 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -121,6 +121,26 @@ std::optional AttemptSelection(const CWallet& wallet, const CAm std::optional ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector& available_coins, const CoinSelectionParams& coin_selection_params); +// User manually selected inputs that must be part of the transaction +struct PreSelectedInputs +{ + std::set coins; + // If subtract fee from outputs is disabled, the 'total_amount' + // will be the sum of each output effective value + // instead of the sum of the outputs amount + CAmount total_amount{0}; + + void Insert(const COutput& output, bool subtract_fee_outputs) + { + if (subtract_fee_outputs) { + total_amount += output.txout.nValue; + } else { + total_amount += output.GetEffectiveValue(); + } + coins.insert(output); + } +}; + /** * Select a set of coins such that nTargetValue is met and at least * all coins from coin_control are selected; never select unconfirmed coins if they are not ours -- cgit v1.2.3 From 5baedc33519661af9d19efcefd23dca8998d2547 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 5 Aug 2022 12:51:44 -0300 Subject: wallet: remove fetch pre-selected-inputs responsibility from SelectCoins so if there is an error in any of the pre-set coins, we can fail right away without computing the wallet available coins set (calling `AvailableCoins`) which is a slow operation as it goes through the entire wallet's txes map. ---------------------- And to make the Coin Selection flow cleared, have decoupled SelectCoins in two functions: 1) AutomaticCoinSelection. 2) SelectCoins. 1) AutomaticCoinSelection: Receives a set of coins and selects the best subset of them to cover the target amount. 2) SelectCoins In charge of select all the user manually selected coins first ("pre-set inputs"), and if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to select a subset of coins owned by the wallet to cover for the target - preset_inputs.total_amount remaining value. --- src/wallet/spend.cpp | 51 +++++++++++++++++++--------------- src/wallet/spend.h | 20 +++++++++++-- src/wallet/test/coinselector_tests.cpp | 27 ++++++++++++------ 3 files changed, 65 insertions(+), 33 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 0b7cb7faa4..84c96b041f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -568,18 +568,14 @@ std::optional ChooseSelectionResult(const CWallet& wallet, cons return best_result; } -std::optional SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) +std::optional SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, + const CAmount& nTargetValue, const CCoinControl& coin_control, + const CoinSelectionParams& coin_selection_params) { - CAmount value_to_select = nTargetValue; - - util::Result pre_selected_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params); - if (!pre_selected_inputs) return std::nullopt; - PreSelectedInputs inputs = *pre_selected_inputs; - // If automatic coin selection was disabled, we just want to return the preset inputs result if (!coin_control.m_allow_other_inputs) { SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); - result.AddInputs(inputs.coins, coin_selection_params.m_subtract_fee_outputs); + result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); if (!coin_selection_params.m_subtract_fee_outputs && result.GetSelectedEffectiveValue() < nTargetValue) { return std::nullopt; @@ -591,9 +587,23 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a return result; } - // Decrease the already selected amount - value_to_select -= inputs.total_amount; + // Decrease the selection target before start the automatic coin selection + CAmount selection_target = nTargetValue - pre_set_inputs.total_amount; + auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_control, coin_selection_params); + if (!op_selection_result) return op_selection_result; + + // Add preset inputs to the automatic coin selection result + SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL); + preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); + op_selection_result->Merge(preselected); + if (op_selection_result->GetAlgo() == SelectionAlgorithm::MANUAL) { + op_selection_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); + } + return op_selection_result; +} +std::optional AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) +{ unsigned int limit_ancestor_count = 0; unsigned int limit_descendant_count = 0; wallet.chain().getPackageLimits(limit_ancestor_count, limit_descendant_count); @@ -610,9 +620,6 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a available_coins.Shuffle(coin_selection_params.rng_fast); } - SelectionResult preselected(inputs.total_amount, SelectionAlgorithm::MANUAL); - preselected.AddInputs(inputs.coins, coin_selection_params.m_subtract_fee_outputs); - // 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. @@ -669,14 +676,6 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a return std::optional(); }(); - if (!res) return std::nullopt; - - // Add preset inputs to result - 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; } @@ -889,6 +888,14 @@ static util::Result CreateTransactionInternal( const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); CAmount selection_target = recipients_sum + not_input_fees; + // Fetch manually selected coins + PreSelectedInputs preset_inputs; + if (coin_control.HasSelected()) { + auto res_fetch_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params); + if (!res_fetch_inputs) return util::Error{util::ErrorString(res_fetch_inputs)}; + preset_inputs = *res_fetch_inputs; + } + // Fetch wallet available coins if "other inputs" are // allowed (coins automatically selected by the wallet) CoinsResult available_coins; @@ -903,7 +910,7 @@ static util::Result CreateTransactionInternal( } // Choose coins to use - std::optional result = SelectCoins(wallet, available_coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); + std::optional result = SelectCoins(wallet, available_coins, preset_inputs, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); if (!result) { return util::Error{_("Insufficient funds")}; } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 39832f363f..b66bb3797c 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -142,8 +142,14 @@ struct PreSelectedInputs }; /** - * Select a set of coins such that nTargetValue is met and at least - * all coins from coin_control are selected; never select unconfirmed coins if they are not ours + * Fetch and validate coin control selected inputs. + * Coins could be internal (from the wallet) or external. +*/ +util::Result FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control, + const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); + +/** + * Select a set of coins such that nTargetValue is met; never select unconfirmed coins if they are not ours * param@[in] wallet The wallet which provides data necessary to spend the selected coins * param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering * param@[in] nTargetValue The target value @@ -152,9 +158,17 @@ struct PreSelectedInputs * returns If successful, a SelectionResult containing the selected coins * If failed, a nullopt. */ -std::optional SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, +std::optional AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +/** + * Select all coins from coin_control, and if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to + * select a set of coins such that nTargetValue - pre_set_inputs.total_amount is met. + */ +std::optional SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, + const CAmount& nTargetValue, const CCoinControl& coin_control, + const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); + struct CreatedTransactionResult { CTransactionRef tx; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 23f024247d..f9c8c8ee9d 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -338,9 +338,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(available_coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); CCoinControl coin_control; coin_control.m_allow_other_inputs = true; - coin_control.Select(available_coins.All().at(0).outpoint); + COutput select_coin = available_coins.All().at(0); + coin_control.Select(select_coin.outpoint); + PreSelectedInputs selected_input; + selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs); + available_coins.coins[OutputType::BECH32].erase(available_coins.coins[OutputType::BECH32].begin()); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); - const auto result10 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); + const auto result10 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(result10); } { @@ -363,7 +367,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) expected_result.Clear(); add_coin(10 * CENT, 2, expected_result); CCoinControl coin_control; - const auto result11 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); + const auto result11 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result11)); available_coins.Clear(); @@ -378,7 +382,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) expected_result.Clear(); add_coin(9 * CENT, 2, expected_result); add_coin(1 * CENT, 2, expected_result); - const auto result12 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); + const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result12)); available_coins.Clear(); @@ -394,8 +398,12 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(9 * CENT, 2, expected_result); add_coin(1 * CENT, 2, expected_result); coin_control.m_allow_other_inputs = true; - coin_control.Select(available_coins.All().at(1).outpoint); // pre select 9 coin - const auto result13 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); + COutput select_coin = available_coins.All().at(1); // pre select 9 coin + coin_control.Select(select_coin.outpoint); + PreSelectedInputs selected_input; + selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs); + available_coins.coins[OutputType::BECH32].erase(++available_coins.coins[OutputType::BECH32].begin()); + const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result13)); } } @@ -783,7 +791,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) 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); + const auto result = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, target, cc, cs_params); BOOST_CHECK(result); BOOST_CHECK_GE(result->GetSelectedValue(), target); } @@ -965,7 +973,10 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) cc.SetInputWeight(output.outpoint, 148); cc.SelectExternal(output.outpoint, output.txout); - const auto result = SelectCoins(*wallet, available_coins, target, cc, cs_params); + const auto preset_inputs = *Assert(FetchSelectedInputs(*wallet, cc, cs_params)); + available_coins.coins[OutputType::BECH32].erase(available_coins.coins[OutputType::BECH32].begin()); + + const auto result = SelectCoins(*wallet, available_coins, preset_inputs, target, cc, cs_params); BOOST_CHECK(!result); } -- cgit v1.2.3 From f41712a734dc119f8a5e053a9cfa1f0411b5e8f1 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 4 Oct 2022 11:04:48 -0300 Subject: wallet: simplify preset inputs selection target check we are already computing the preset inputs total amount inside `PreSelectedInputs::Insert`, which internally decides whether to use the effective value or the raw output value based on the 'subtract_fee_outputs' flag. --- src/wallet/spend.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 84c96b041f..f17a53348b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -572,23 +572,21 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) { + // Deduct preset inputs amount from the search target + CAmount selection_target = nTargetValue - pre_set_inputs.total_amount; + // If automatic coin selection was disabled, we just want to return the preset inputs result if (!coin_control.m_allow_other_inputs) { + // 'selection_target' is computed on `PreSelectedInputs::Insert` which decides whether to use the effective value + // or the raw output value based on the 'subtract_fee_outputs' flag. + if (selection_target > 0) return std::nullopt; SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); - - 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; } - // Decrease the selection target before start the automatic coin selection - CAmount selection_target = nTargetValue - pre_set_inputs.total_amount; + // Start wallet Coin Selection procedure auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_control, coin_selection_params); if (!op_selection_result) return op_selection_result; -- cgit v1.2.3 From a8a75346d7e7247596c8a580d65ceaad49c97b97 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 24 Aug 2022 15:03:13 -0300 Subject: wallet: SelectCoins, return early if target is covered by preset-inputs --- src/wallet/spend.cpp | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index f17a53348b..644b2b587c 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -575,11 +575,11 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a // Deduct preset inputs amount from the search target CAmount selection_target = nTargetValue - pre_set_inputs.total_amount; - // If automatic coin selection was disabled, we just want to return the preset inputs result - if (!coin_control.m_allow_other_inputs) { - // 'selection_target' is computed on `PreSelectedInputs::Insert` which decides whether to use the effective value - // or the raw output value based on the 'subtract_fee_outputs' flag. - if (selection_target > 0) return std::nullopt; + // Return if automatic coin selection is disabled, and we don't cover the selection target + if (!coin_control.m_allow_other_inputs && selection_target > 0) return std::nullopt; + + // Return if we can cover the target only with the preset inputs + if (selection_target <= 0) { SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); @@ -590,12 +590,14 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_control, coin_selection_params); if (!op_selection_result) return op_selection_result; - // Add preset inputs to the automatic coin selection result - SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL); - preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); - op_selection_result->Merge(preselected); - if (op_selection_result->GetAlgo() == SelectionAlgorithm::MANUAL) { - op_selection_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); + // If needed, add preset inputs to the automatic coin selection result + if (!pre_set_inputs.coins.empty()) { + SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL); + preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); + op_selection_result->Merge(preselected); + op_selection_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, + coin_selection_params.m_cost_of_change, + coin_selection_params.m_change_fee); } return op_selection_result; } @@ -622,9 +624,6 @@ std::optional AutomaticCoinSelection(const CWallet& wallet, Coi // 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(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. if (auto r1{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 6, 0), available_coins, coin_selection_params, /*allow_mixed_output_types=*/false)}) return r1; -- cgit v1.2.3