From 805f399b174ef129ffabc07a87024be1ea1bfc35 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 13 Feb 2023 19:56:26 -0300 Subject: [PATCH 1/6] wallet: do not make two COutputs, use shared_ptr --- src/wallet/spend.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index a8ecce119a..0b58f99ce9 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -445,10 +445,10 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, // OUTPUT_GROUP_MAX_ENTRIES COutputs, a new OutputGroup is added to the end of the vector. typedef std::map, std::vector> ScriptPubKeyToOutgroup; const auto& group_outputs = []( - const COutput& output, OutputType type, size_t ancestors, size_t descendants, + const std::shared_ptr& output, OutputType type, size_t ancestors, size_t descendants, ScriptPubKeyToOutgroup& groups_map, const CoinSelectionParams& coin_sel_params, bool positive_only) { - std::vector& groups = groups_map[std::make_pair(output.txout.scriptPubKey,type)]; + std::vector& groups = groups_map[std::make_pair(output->txout.scriptPubKey,type)]; if (groups.size() == 0) { // No OutputGroups for this scriptPubKey yet, add one @@ -468,8 +468,8 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, } // Filter for positive only before adding the output to group - if (!positive_only || output.GetEffectiveValue() > 0) { - group->Insert(std::make_shared(output), ancestors, descendants); + if (!positive_only || output->GetEffectiveValue() > 0) { + group->Insert(output, ancestors, descendants); } }; @@ -483,8 +483,9 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, size_t ancestors, descendants; wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - group_outputs(output, type, ancestors, descendants, spk_to_groups_map, coin_sel_params, /*positive_only=*/ false); - group_outputs(output, type, ancestors, descendants, spk_to_positive_groups_map, + const auto& shared_output = std::make_shared(output); + group_outputs(shared_output, type, ancestors, descendants, spk_to_groups_map, coin_sel_params, /*positive_only=*/ false); + group_outputs(shared_output, type, ancestors, descendants, spk_to_positive_groups_map, coin_sel_params, /*positive_only=*/ true); } } From 99034b2b72728def57204abe518e0360d9675437 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 13 Feb 2023 20:03:28 -0300 Subject: [PATCH 2/6] wallet: APS, don't create empty groups By moving the "positive-only" flag out of the lambda function. --- src/wallet/spend.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 0b58f99ce9..238422dc89 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -444,10 +444,9 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, // to the last OutputGroup in the vector for the scriptPubKey. When the last OutputGroup has // OUTPUT_GROUP_MAX_ENTRIES COutputs, a new OutputGroup is added to the end of the vector. typedef std::map, std::vector> ScriptPubKeyToOutgroup; - const auto& group_outputs = []( + const auto& insert_output = [&]( const std::shared_ptr& output, OutputType type, size_t ancestors, size_t descendants, - ScriptPubKeyToOutgroup& groups_map, const CoinSelectionParams& coin_sel_params, - bool positive_only) { + ScriptPubKeyToOutgroup& groups_map) { std::vector& groups = groups_map[std::make_pair(output->txout.scriptPubKey,type)]; if (groups.size() == 0) { @@ -467,10 +466,7 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, group = &groups.back(); } - // Filter for positive only before adding the output to group - if (!positive_only || output->GetEffectiveValue() > 0) { - group->Insert(output, ancestors, descendants); - } + group->Insert(output, ancestors, descendants); }; ScriptPubKeyToOutgroup spk_to_groups_map; @@ -484,9 +480,13 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); const auto& shared_output = std::make_shared(output); - group_outputs(shared_output, type, ancestors, descendants, spk_to_groups_map, coin_sel_params, /*positive_only=*/ false); - group_outputs(shared_output, type, ancestors, descendants, spk_to_positive_groups_map, - coin_sel_params, /*positive_only=*/ true); + // Filter for positive only before adding the output + if (output.GetEffectiveValue() > 0) { + insert_output(shared_output, type, ancestors, descendants, spk_to_positive_groups_map); + } + + // 'All' groups + insert_output(shared_output, type, ancestors, descendants, spk_to_groups_map); } } From a9aa04183c4502c23ec924e9c8944b7c90f9f5c2 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 13 Feb 2023 20:08:38 -0300 Subject: [PATCH 3/6] wallet: OutputGroup, remove unused effective_feerate member --- src/wallet/coinselection.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 3abd22c207..cd03b7d224 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -222,8 +222,6 @@ struct OutputGroup CAmount effective_value{0}; /** The fee to spend these UTXOs at the effective feerate. */ CAmount fee{0}; - /** The target feerate of the transaction we're trying to build. */ - CFeeRate m_effective_feerate{0}; /** The fee to spend these UTXOs at the long term feerate. */ CAmount long_term_fee{0}; /** The feerate for spending a created change output eventually (i.e. not urgently, and thus at @@ -238,7 +236,6 @@ struct OutputGroup OutputGroup() {} 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) {} From 8471967d7be0804665fdf94a9a841b1c56598b3c Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 4 Mar 2023 11:23:11 -0300 Subject: [PATCH 4/6] wallet: GroupOutput, remove unneeded "spendable" check `AvailableCoins` already filters non-spendable coins. --- src/wallet/spend.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 238422dc89..359fc78df1 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -415,9 +415,6 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup for (const auto& [type, outputs] : coins.coins) { for (const COutput& output : outputs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; - // Get mempool info size_t ancestors, descendants; wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); @@ -473,9 +470,6 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, ScriptPubKeyToOutgroup spk_to_positive_groups_map; for (const auto& [type, outs] : coins.coins) { for (const COutput& output : outs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; - size_t ancestors, descendants; wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); From 8a5583131ce78cd748e2279eb9331321e4ef2aa3 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 19 Aug 2022 22:31:49 -0300 Subject: [PATCH 5/6] wallet: remove unused methods CWallet::DummySignTx, OutputGroupTypeMap::find --- src/wallet/coinselection.cpp | 7 ------- src/wallet/coinselection.h | 2 -- src/wallet/wallet.h | 6 ------ 3 files changed, 15 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 3fd0280b8b..9cf61dbe51 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -387,13 +387,6 @@ void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool in } } -std::optional OutputGroupTypeMap::Find(OutputType type) -{ - auto it_by_type = groups_by_type.find(type); - if (it_by_type == groups_by_type.end()) return std::nullopt; - return it_by_type->second; -} - CAmount GetSelectionWaste(const std::set>& inputs, CAmount change_cost, CAmount target, bool use_effective_value) { // This function should not be called with empty inputs as that would mean the selection failed diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index cd03b7d224..dbe01a08fe 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -263,8 +263,6 @@ struct OutputGroupTypeMap // Based on the insert flag; appends group to the 'mixed_group' and, if value > 0, to the 'positive_group'. // This affects both; the groups filtered by type and the overall groups container. void Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed); - // Retrieves 'Groups' filtered by type - std::optional Find(OutputType type); // Different output types count size_t TypesCount() { return groups_by_type.size(); } }; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 16eee1e050..e8c18dbb67 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -592,12 +592,6 @@ public: bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool DummySignTx(CMutableTransaction &txNew, const std::set &txouts, const CCoinControl* coin_control = nullptr) const - { - std::vector v_txouts(txouts.size()); - std::copy(txouts.begin(), txouts.end(), v_txouts.begin()); - return DummySignTx(txNew, v_txouts, coin_control); - } bool DummySignTx(CMutableTransaction &txNew, const std::vector &txouts, const CCoinControl* coin_control = nullptr) const; bool ImportScripts(const std::set scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 475c20aa568d597c7850c784058596ae26f37496 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 8 Mar 2023 19:03:40 -0300 Subject: [PATCH 6/6] wallet: remove coin control arg from AutomaticCoinSelection we only need the "include unsafe" flag, not all what coin control stores. --- src/wallet/coinselection.h | 5 +++++ src/wallet/spend.cpp | 17 ++++++++--------- src/wallet/spend.h | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index dbe01a08fe..5a7b748be1 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -153,6 +153,11 @@ struct CoinSelectionParams { * 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; + /** + * When true, allow unsafe coins to be selected during Coin Selection. This may spend unconfirmed outputs: + * 1) Received from other wallets, 2) replacing other txs, 3) that have been replaced. + */ + bool m_include_unsafe_inputs = false; CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size, CAmount min_change_target, CFeeRate effective_feerate, diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 359fc78df1..5771d33b7a 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -617,7 +617,7 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av } // Start wallet Coin Selection procedure - auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_control, coin_selection_params); + auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_selection_params); if (!op_selection_result) return op_selection_result; // If needed, add preset inputs to the automatic coin selection result @@ -632,7 +632,7 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av return op_selection_result; } -util::Result AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) +util::Result AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CoinSelectionParams& coin_selection_params) { unsigned int limit_ancestor_count = 0; unsigned int limit_descendant_count = 0; @@ -641,12 +641,10 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin const size_t max_descendants = (size_t)std::max(1, limit_descendant_count); const bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS); - // form groups from remaining coins; note that preset coins will not - // automatically have their associated (same address) coins included - if (coin_control.m_avoid_partial_spends && available_coins.Size() > OUTPUT_GROUP_MAX_ENTRIES) { - // Cases where we have 101+ outputs all pointing to the same destination may result in - // privacy leaks as they will potentially be deterministically sorted. We solve that by - // explicitly shuffling the outputs before processing + // Cases where we have 101+ outputs all pointing to the same destination may result in + // privacy leaks as they will potentially be deterministically sorted. We solve that by + // explicitly shuffling the outputs before processing + if (coin_selection_params.m_avoid_partial_spends && available_coins.Size() > OUTPUT_GROUP_MAX_ENTRIES) { available_coins.Shuffle(coin_selection_params.rng_fast); } @@ -673,7 +671,7 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, /*include_partial=*/true)}); // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs // received from other wallets. - if (coin_control.m_include_unsafe_inputs) { + if (coin_selection_params.m_include_unsafe_inputs) { ordered_filters.push_back({CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true)}); } // Try with unlimited ancestors/descendants. The transaction will still need to meet @@ -804,6 +802,7 @@ static util::Result CreateTransactionInternal( CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; + coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs; // Set the long term feerate estimate to the wallet's consolidate feerate coin_selection_params.m_long_term_feerate = wallet.m_consolidate_feerate; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index b8bc82db7a..78c2c5f22b 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -191,7 +191,7 @@ util::Result FetchSelectedInputs(const CWallet& wallet, const * or (2) an specific error message if there was something particularly wrong (e.g. a selection * result that surpassed the tx max weight size). */ -util::Result AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, +util::Result AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); /**