From 58b2fb9de304d39744523ddd274d4d3b31b9b6d8 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Sun, 18 Oct 2026 00:08:16 -0500 Subject: [PATCH 1/6] Merge bitcoin/bitcoin#25806: wallet: group outputs only once, decouple it from Coin Selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44 wallet: single output groups filtering and grouping process (furszy) bd91ed1cb2cc804f824d1b204513ac2afb7d5e28 wallet: unify outputs grouping process (furszy) 55962001da4f8467e52da502b05f5c0a85128fb0 test: coinselector_tests refactor, use CoinsResult instead of plain std::vector (furszy) 34f54a0a3a54c59d3f062de69550ab3a3b077ea0 wallet: decouple outputs grouping process from each ChooseSelectionResult (furszy) 461f0821a2dff0a27f755464b0eb92314fba1acd refactor: make OutputGroup::m_outputs field a vector of shared_ptr (furszy) d8e749bb840cf65065ed00561998255156126278 test: wallet, add coverage for outputs grouping process (furszy) 06ec8f992890cac69cd0fd20224aa51fa311a181 wallet: make OutputGroup "positive_only" filter explicit (furszy) Pull request description: The idea originates from https://github.com/bitcoin/bitcoin/pull/24845#issuecomment-1130310321. Note: For clarity, it's recommended to start reviewing from the end result to understand the structure of the flow. #### GroupOutputs function rationale: If "Avoid Partial Spends" is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them. #### How the Inputs Fetch + Selection process roughly works: ``` 1. Fetch user’s manually selected inputs. 2. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type). 3. Coin Selection Process: Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds. Each `AttemptSelection` call performs the following actions: - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them): Call ‘ChooseSelectionResult’ providing the respective, filtered by type, coins vector. Which: I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well). - GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups. II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD. III. Then returns the best solution out of them. ``` We perform the general operation of gathering outputs, with the same script, into a single container inside: Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them). So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the `GroupOutputs` function is called 80 times (8 * 5 * 2). #### Improvements: This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process. The new process is as follows: ``` 1. Fetch user’s manually selected inputs. 2. Fetch wallet available coins. 3. Group outputs by each coin eligibility filter and each different output type found. 4. Coin Selection Process: Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds. Each ‘AttemptSelection’ call performs the following actions: - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them): A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which: I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD. II. Then returns the best solution out of them. ``` Extra Note: The next steps after this PR will be to: 1) Merge `AvailableCoins` and `GroupOutputs` processes. 2) Skip entire coin selection rounds if no new coins are added into the subsequent round. 3) Remove global feerates from the OutputGroup class. 4) Remove secondary "grouped" tx creation from `CreateTransactionInternal` by running Coin Selection results over the aps grouped outputs vs non-aps ones. ACKs for top commit: S3RK: ReACK 6a302d4 achow101: ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44 theStack: re-ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44 :coconut: Tree-SHA512: dff849063be328e7d9c358ec80239a6db2cd6131963b511b83699b95b337d3106263507eaba0119eaac63e6ac21c6c42d187ae23d79d9220b90c323d44b01d24 Dash adaptations: - src/wallet/spend.cpp: GroupOutputs keeps Dash's InstantSend check, calling EligibleForSpending(filter, isISLocked) per filter; isGroupISLocked now reads output->outpoint - src/wallet/spend.cpp, src/wallet/spend.h: AttemptSelection/ChooseSelectionResult keep the `const CWallet& wallet` parameter because Dash's KnapsackSolver needs wallet.m_default_max_tx_fee - src/wallet/spend.cpp: ChooseSelectionResult still empties groups.positive_group to skip BnB/SRD (Dash's existing behaviour) and keeps Dash's max_inputs_weight/fully-mixed/max-fee solver arguments - src/wallet/spend.cpp: CreateTransactionInternal keeps Dash's GetInputSet() + BIP69/shuffle vin path, updated to coin->outpoint, instead of upstream's GetShuffledInputVector/RBF nSequence - src/wallet/coinselection.cpp: no WITNESS_SCALE_FACTOR in OutputGroup::Insert/AddInputs (Dash weight is in bytes); Dash-only GetSelectionWeight helpers switched to shared_ptr - src/wallet/coinselection.h: adds only , not upstream's ; EligibleForSpending keeps Dash's isISLocked argument - src/bench/coin_selection.cpp: AttemptSelection call passes the wallet, matching Dash's kept parameter - src/wallet/test/coinselector_tests.cpp: Dash's SelectCoinsSRDResult (from bitcoin#26720) uses the new GroupOutputs(...)[filter].all_groups.positive_group form, as upstream's post-25806 version does - src/wallet/test/coinselector_tests.cpp: Dash's own check_max_weight/has_coin and the Dash-only knapsack max-weight checks dereference shared_ptr; upstream's relocated check_max_weight copy is dropped - src/wallet/test/group_outputs_tests.cpp: OutputType::BECH32 becomes LEGACY, OutputTypeFromDestination becomes LEGACY, GetNewDestination(label), and SetupDescriptorScriptPubKeyMans("", "") (Dash has no segwit; its API has a mnemonic) --- src/Makefile.test.include | 3 +- src/bench/coin_selection.cpp | 5 +- src/wallet/coinselection.cpp | 60 ++++-- src/wallet/coinselection.h | 46 ++++- src/wallet/spend.cpp | 165 ++++++++++------- src/wallet/spend.h | 28 ++- src/wallet/test/coinselector_tests.cpp | 101 +++++----- src/wallet/test/fuzz/coinselection.cpp | 6 +- src/wallet/test/group_outputs_tests.cpp | 234 ++++++++++++++++++++++++ 9 files changed, 487 insertions(+), 161 deletions(-) create mode 100644 src/wallet/test/group_outputs_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 7fa85505e0b2..c053ee9021be 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -243,7 +243,8 @@ BITCOIN_TESTS += \ wallet/test/ismine_tests.cpp \ wallet/test/rpc_util_tests.cpp \ wallet/test/scriptpubkeyman_tests.cpp \ - wallet/test/walletload_tests.cpp + wallet/test/walletload_tests.cpp \ + wallet/test/group_outputs_tests.cpp FUZZ_SUITE_LD_COMMON +=\ $(SQLITE_LIBS) \ diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 634afe91696d..d9a8f0d357e3 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -73,8 +73,9 @@ static void CoinSelection(benchmark::Bench& bench) /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; + auto group = wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard]; bench.run([&] { - auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true); + auto result = AttemptSelection(wallet, 1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true); assert(result); assert(result->GetSelectedValue() == 1003 * COIN); assert(result->GetInputSet().size() == 2); @@ -89,7 +90,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector tx.vout[nInput].nValue = nValue; COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true, /*fees=*/ 0); set.emplace_back(); - set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); + set.back().Insert(std::make_shared(output), /*ancestors=*/ 0, /*descendants=*/ 0); } // Copied from src/wallet/test/coinselector_tests.cpp static CAmount make_hard_case(int utxos, std::vector& utxo_pool) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index bc6a3f4187fc..cb356b6ff4a4 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -26,15 +26,15 @@ static util::Result ErrorMaxWeightExceeded() static int GetSelectionWeight(const OutputGroup& group) { - return std::accumulate(group.m_outputs.cbegin(), group.m_outputs.cend(), 0, [](int sum, const COutput& coin) { - return sum + std::max(coin.input_bytes, 0); + return std::accumulate(group.m_outputs.cbegin(), group.m_outputs.cend(), 0, [](int sum, const std::shared_ptr& coin) { + return sum + std::max(coin->input_bytes, 0); }); } static int GetSelectionWeight(const SelectionResult& result) { - return std::accumulate(result.GetInputSet().cbegin(), result.GetInputSet().cend(), 0, [](int sum, const COutput& coin) { - return sum + std::max(coin.input_bytes, 0); + return std::accumulate(result.GetInputSet().cbegin(), result.GetInputSet().cend(), 0, [](int sum, const std::shared_ptr& coin) { + return sum + std::max(coin->input_bytes, 0); }); } @@ -478,12 +478,9 @@ util::Result KnapsackSolver(std::vector& groups, c ******************************************************************************/ -void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only) { - // Filter for positive only here before adding the coin - if (positive_only && output.GetEffectiveValue() <= 0) return; - +void OutputGroup::Insert(const std::shared_ptr& output, size_t ancestors, size_t descendants) { m_outputs.push_back(output); - COutput& coin = m_outputs.back(); + auto& coin = *m_outputs.back(); fee += coin.GetFee(); @@ -503,8 +500,8 @@ void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descend // coin itself; thus, this value is counted as the max, not the sum m_descendants = std::max(m_descendants, descendants); - if (output.input_bytes > 0) { - m_weight += output.input_bytes; + if (output->input_bytes > 0) { + m_weight += output->input_bytes; } } @@ -520,7 +517,29 @@ CAmount OutputGroup::GetSelectionAmount() const return m_subtract_fee_outputs ? m_value : effective_value; } -CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value) +void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed) +{ + if (group.m_outputs.empty()) return; + + Groups& groups = groups_by_type[type]; + if (insert_positive && group.GetSelectionAmount() > 0) { + groups.positive_group.emplace_back(group); + all_groups.positive_group.emplace_back(group); + } + if (insert_mixed) { + groups.mixed_group.emplace_back(group); + all_groups.mixed_group.emplace_back(group); + } +} + +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 assert(!inputs.empty()); @@ -528,7 +547,8 @@ CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, // Always consider the cost of spending an input now vs in the future. CAmount waste = 0; CAmount selected_effective_value = 0; - for (const COutput& coin : inputs) { + for (const auto& coin_ptr : inputs) { + const COutput& coin = *coin_ptr; waste += coin.GetFee() - coin.long_term_fee; selected_effective_value += use_effective_value ? coin.GetEffectiveValue() : coin.txout.nValue; } @@ -576,12 +596,12 @@ CAmount SelectionResult::GetWaste() const 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; }); + 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(); }); + 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() @@ -600,14 +620,14 @@ void SelectionResult::AddInput(const OutputGroup& group) m_weight += group.m_weight; } -void SelectionResult::AddInputs(const std::set& inputs, bool subtract_fee_outputs) +void SelectionResult::AddInputs(const std::set>& inputs, bool subtract_fee_outputs) { // As it can fail, combine inputs first InsertInputs(inputs); m_use_effective = !subtract_fee_outputs; m_weight += std::accumulate(inputs.cbegin(), inputs.cend(), 0, [](int sum, const auto& coin) { - return sum + std::max(coin.input_bytes, 0); + return sum + std::max(coin->input_bytes, 0); }); } @@ -625,14 +645,14 @@ void SelectionResult::Merge(const SelectionResult& other) m_weight += other.m_weight; } -const std::set& SelectionResult::GetInputSet() const +const std::set>& SelectionResult::GetInputSet() const { return m_selected_inputs; } -std::vector SelectionResult::GetShuffledInputVector() const +std::vector> SelectionResult::GetShuffledInputVector() const { - std::vector coins(m_selected_inputs.begin(), m_selected_inputs.end()); + std::vector> coins(m_selected_inputs.begin(), m_selected_inputs.end()); Shuffle(coins.begin(), coins.end(), FastRandomContext()); return coins; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 64a6784ad8dc..dd5e8156df30 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -6,6 +6,7 @@ #define BITCOIN_WALLET_COINSELECTION_H #include +#include #include #include #include @@ -206,13 +207,18 @@ struct CoinEligibilityFilter CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {} + + bool operator<(const CoinEligibilityFilter& other) const { + return std::tie(conf_mine, conf_theirs, max_ancestors, max_descendants, m_include_partial_groups) + < std::tie(other.conf_mine, other.conf_theirs, other.max_ancestors, other.max_descendants, other.m_include_partial_groups); + } }; /** A group of UTXOs paid to the same output script. */ struct OutputGroup { /** The list of UTXOs contained in this output group. */ - std::vector m_outputs; + std::vector> m_outputs; /** Whether the UTXOs were sent by the wallet to itself. This is relevant because we may want at * least a certain number of confirmations on UTXOs received from outside wallets while trusting * our own UTXOs more. */ @@ -251,11 +257,37 @@ struct OutputGroup m_subtract_fee_outputs(params.m_subtract_fee_outputs) {} - void Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only); + void Insert(const std::shared_ptr& output, size_t ancestors, size_t descendants); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter, bool isISLocked) const; CAmount GetSelectionAmount() const; }; +struct Groups { + // Stores 'OutputGroup' containing only positive UTXOs (value > 0). + std::vector positive_group; + // Stores 'OutputGroup' which may contain both positive and negative UTXOs. + std::vector mixed_group; +}; + +/** Stores several 'Groups' whose were mapped by output type. */ +struct OutputGroupTypeMap +{ + // Maps output type to output groups. + std::map groups_by_type; + // All inserted groups, no type distinction. + Groups all_groups; + + // 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(); } +}; + +typedef std::map FilteredOutputGroups; + /** Compute the waste for this result given the cost of change * and the opportunity cost of spending these inputs now vs in the future. * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) @@ -273,7 +305,7 @@ struct OutputGroup * @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false). * @return The waste */ -[[nodiscard]] CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); +[[nodiscard]] CAmount GetSelectionWaste(const std::set>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); /** Choose a random change target for each transaction to make it harder to fingerprint the Core @@ -306,7 +338,7 @@ struct SelectionResult { private: /** Set of inputs selected by the algorithm to use in the transaction */ - std::set m_selected_inputs; + 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 */ @@ -343,7 +375,7 @@ struct SelectionResult void Clear(); void AddInput(const OutputGroup& group); - void AddInputs(const std::set& inputs, bool subtract_fee_outputs); + 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); @@ -358,9 +390,9 @@ struct SelectionResult void Merge(const SelectionResult& other); /** Get m_selected_inputs */ - const std::set& GetInputSet() const; + 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; + std::vector> GetShuffledInputVector() const; bool operator<(SelectionResult other) const; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e3721a2c8238..b5a0677350cd 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -403,33 +403,42 @@ std::map> ListCoins(const CWallet& wallet) static bool isGroupISLocked(const OutputGroup& group, interfaces::Chain& chain) { return std::all_of(group.m_outputs.begin(), group.m_outputs.end(), [&chain](const auto& output) { - return chain.isInstantSendLockedTx(output.outpoint.hash); + return chain.isInstantSendLockedTx(output->outpoint.hash); }); } -std::vector GroupOutputs(const CWallet& wallet, const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) +FilteredOutputGroups GroupOutputs(const CWallet& wallet, + const CoinsResult& coins, + const CoinSelectionParams& coin_sel_params, + const std::vector& filters) { - std::vector groups_out; + FilteredOutputGroups filtered_groups; if (!coin_sel_params.m_avoid_partial_spends) { - // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup. - for (const COutput& output : outputs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; - - size_t ancestors, descendants; - wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - - // Make an OutputGroup containing just this output - OutputGroup group{coin_sel_params}; - group.Insert(output, ancestors, descendants, positive_only); - - // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.GetSelectionAmount() <= 0) continue; - bool isISLocked = isGroupISLocked(group, wallet.chain()); - if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter, isISLocked)) groups_out.push_back(group); + // 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); + + // Create a new group per output and add it to the all groups vector + OutputGroup group(coin_sel_params); + group.Insert(std::make_shared(output), ancestors, descendants); + + bool isISLocked = isGroupISLocked(group, wallet.chain()); + // Each filter maps to a different set of groups + for (const auto& sel_filter : filters) { + const auto& filter = sel_filter.filter; + if (!group.EligibleForSpending(filter, isISLocked)) continue; + filtered_groups[filter].Push(group, type, /*insert_positive=*/true, /*insert_mixed=*/true); + } + } } - return groups_out; + return filtered_groups; } // We want to combine COutputs that have the same scriptPubKey into single OutputGroups @@ -438,16 +447,12 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector> spk_to_groups_map; - for (const auto& output : outputs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; - - size_t ancestors, descendants; - wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - CScript spk = output.txout.scriptPubKey; - - std::vector& groups = spk_to_groups_map[spk]; + typedef std::map, std::vector> ScriptPubKeyToOutgroup; + const auto& group_outputs = []( + const COutput& 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)]; if (groups.size() == 0) { // No OutputGroups for this scriptPubKey yet, add one @@ -466,43 +471,70 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vectorInsert(output, ancestors, descendants, positive_only); + // Filter for positive only before adding the output to group + if (!positive_only || output.GetEffectiveValue() > 0) { + group->Insert(std::make_shared(output), ancestors, descendants); + } + }; + + ScriptPubKeyToOutgroup spk_to_groups_map; + 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); + + 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, + coin_sel_params, /*positive_only=*/ true); + } } - // Now we go through the entire map and pull out the OutputGroups - for (const auto& spk_and_groups_pair: spk_to_groups_map) { - const std::vector& groups_per_spk= spk_and_groups_pair.second; + // Now we go through the entire maps and pull out the OutputGroups + const auto& push_output_groups = [&](const ScriptPubKeyToOutgroup& groups_map, bool positive_only) { + for (const auto& [script, groups] : groups_map) { + // Go through the vector backwards. This allows for the first item we deal with being the partial group. + for (auto group_it = groups.rbegin(); group_it != groups.rend(); group_it++) { + const OutputGroup& group = *group_it; - // Go through the vector backwards. This allows for the first item we deal with being the partial group. - for (auto group_it = groups_per_spk.rbegin(); group_it != groups_per_spk.rend(); group_it++) { - const OutputGroup& group = *group_it; + bool isISLocked = isGroupISLocked(group, wallet.chain()); + // Each filter maps to a different set of groups + for (const auto& sel_filter : filters) { + const auto& filter = sel_filter.filter; + if (!group.EligibleForSpending(filter, isISLocked)) continue; - // Don't include partial groups if there are full groups too and we don't want partial groups - if (group_it == groups_per_spk.rbegin() && groups_per_spk.size() > 1 && !filter.m_include_partial_groups) { - continue; - } + // Don't include partial groups if there are full groups too and we don't want partial groups + if (group_it == groups.rbegin() && groups.size() > 1 && !filter.m_include_partial_groups) { + continue; + } - // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.GetSelectionAmount() <= 0) continue; - bool isISLocked = isGroupISLocked(group, wallet.chain()); - if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter, isISLocked)) groups_out.push_back(group); + OutputType type = script.second; + // Either insert the group into the positive-only groups or the mixed ones. + filtered_groups[filter].Push(group, type, positive_only, /*insert_mixed=*/!positive_only); + } + } } - } + }; - return groups_out; + push_output_groups(spk_to_groups_map, /*positive_only=*/ false); + push_output_groups(spk_to_positive_groups_map, /*positive_only=*/ true); + + return filtered_groups; } // Returns true if the result contains an error and the message is not empty static bool HasErrorMsg(const util::Result& res) { return !util::ErrorString(res).empty(); } -util::Result AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +util::Result AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, OutputGroupTypeMap& groups, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types) { // Run coin selection on each OutputType and compute the Waste Metric std::vector results; - for (const auto& it : available_coins.coins) { - auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params)}; + for (auto& [type, group] : groups.groups_by_type) { + auto result{ChooseSelectionResult(wallet, nTargetValue, group, coin_selection_params)}; // If any specific error message appears here, then something particularly wrong happened. if (HasErrorMsg(result)) return result; // So let's return the specific error. // Append the favorable result. @@ -515,16 +547,15 @@ util::Result AttemptSelection(const CWallet& wallet, const CAmo // If we can't fund the transaction from any individual OutputType, run coin selection one last time // over all available coins, which would allow mixing. // If TypesCount() <= 1, there is nothing to mix. - if (allow_mixed_output_types && available_coins.TypesCount() > 1) { - return ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params); + if (allow_mixed_output_types && groups.TypesCount() > 1) { + return ChooseSelectionResult(wallet, nTargetValue, groups.all_groups, coin_selection_params); } // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't // find a solution using all available coins return util::Error(); }; -util::Result ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector& available_coins, - const CoinSelectionParams& coin_selection_params) +util::Result ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params) { // Vector of results. We will choose the best one based on waste. std::vector results; @@ -539,17 +570,15 @@ util::Result ChooseSelectionResult(const CWallet& wallet, cons int max_inputs_weight = MAX_STANDARD_TX_SIZE - coin_selection_params.tx_noinputs_size; - std::vector positive_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/true); - positive_groups.clear(); // Cleared to skip BnB and SRD as they're unaware of mixed coins - if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { + groups.positive_group.clear(); // Cleared to skip BnB and SRD as they're unaware of mixed coins + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { results.push_back(*bnb_result); } else append_error(bnb_result); max_inputs_weight -= coin_selection_params.change_output_size; // 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, /*positive_only=*/false); - if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue, coin_selection_params.m_min_change_target, + if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast, max_inputs_weight, coin_selection_params.m_coin_type == CoinType::ONLY_FULLY_MIXED, wallet.m_default_max_tx_fee)}) { @@ -557,7 +586,7 @@ util::Result ChooseSelectionResult(const CWallet& wallet, cons results.push_back(*knapsack_result); } else append_error(knapsack_result); - if (auto srd_result{SelectCoinsSRD(positive_groups, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight, + if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight, coin_selection_params.m_coin_type == CoinType::ONLY_FULLY_MIXED)}) { 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); @@ -620,11 +649,6 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av return op_selection_result; } -struct SelectionFilter { - CoinEligibilityFilter filter; - bool allow_mixed_output_types{true}; -}; - util::Result 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; @@ -679,12 +703,17 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin } } + // Group outputs and map them by coin eligibility filter + FilteredOutputGroups filtered_groups = GroupOutputs(wallet, available_coins, coin_selection_params, ordered_filters); + // Walk-through the filters until the solution gets found. // If no solution is found, return the first detailed error (if any). // future: add "error level" so the worst one can be picked instead. std::vector> res_detailed_errors; for (const auto& select_filter : ordered_filters) { - if (auto res{AttemptSelection(wallet, value_to_select, select_filter.filter, available_coins, + auto it = filtered_groups.find(select_filter.filter); + if (it == filtered_groups.end()) continue; + if (auto res{AttemptSelection(wallet, value_to_select, it->second, coin_selection_params, select_filter.allow_mixed_output_types)}) { return res; // result found } else { @@ -1026,7 +1055,7 @@ static util::Result CreateTransactionInternal( // works. const uint32_t nSequence{CTxIn::SEQUENCE_FINAL - 1}; for (const auto& coin : result.GetInputSet()) { - txNew.vin.emplace_back(coin.outpoint, CScript(), nSequence); + txNew.vin.emplace_back(coin->outpoint, CScript(), nSequence); } DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 73dbac9f49fd..92217f0e3b10 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -103,7 +103,18 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& */ std::map> ListCoins(const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -std::vector GroupOutputs(const CWallet& wallet, const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only); +struct SelectionFilter { + CoinEligibilityFilter filter; + bool allow_mixed_output_types{true}; +}; + +/** +* Group coins by the provided filters. +*/ +FilteredOutputGroups GroupOutputs(const CWallet& wallet, + const CoinsResult& coins, + const CoinSelectionParams& coin_sel_params, + const std::vector& filters); /** * Attempt to find a valid input set that preserves privacy by not mixing OutputTypes. @@ -113,8 +124,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector GroupOutputs(const CWallet& wallet, const std::vector AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +util::Result AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, OutputGroupTypeMap& groups, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types); /** @@ -132,21 +142,19 @@ util::Result AttemptSelection(const CWallet& wallet, const CAmo * * param@[in] wallet The wallet which provides solving data for the coins * param@[in] nTargetValue The target value - * param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection - * param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering + * param@[in] groups The struct containing the outputs grouped by script and divided by (1) positive only outputs and (2) all outputs (positive + negative). * param@[in] coin_selection_params Parameters for the coin selection * returns If successful, a SelectionResult containing the input set * If failed, returns (1) an empty error message if the target was not reached (general "Insufficient funds") * 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 ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector& available_coins, - const CoinSelectionParams& coin_selection_params); +util::Result ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params); // User manually selected inputs that must be part of the transaction struct PreSelectedInputs { - std::set coins; + 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 @@ -159,7 +167,7 @@ struct PreSelectedInputs } else { total_amount += output.GetEffectiveValue(); } - coins.insert(output); + coins.insert(std::make_shared(output)); } }; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 38d3cfb8c3e7..83ee597a0b2f 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -31,7 +31,7 @@ BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup) // we repeat those tests this many times and only complain if all iterations of the test fail #define RANDOM_REPEATS 5 -typedef std::set CoinSet; +typedef std::set> CoinSet; static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); @@ -55,7 +55,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result) tx.nLockTime = nextLockTime++; // so all transactions get different hashes COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0); OutputGroup group; - group.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ true); + group.Insert(std::make_shared(output), /*ancestors=*/ 0, /*descendants=*/ 0); result.AddInput(group); } @@ -67,7 +67,7 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fe tx.nLockTime = nextLockTime++; // so all transactions get different hashes COutput coin(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ 148, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, fee); coin.long_term_fee = long_term_fee; - set.insert(coin); + set.insert(std::make_shared(coin)); } static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmount& nValue, CFeeRate feerate = CFeeRate(0), int nAge = 6*24, bool fIsFromMe = false, int nInput =0, bool spendable = false, int custom_size = 0) @@ -102,10 +102,10 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) std::vector a_amts; std::vector b_amts; for (const auto& coin : a.GetInputSet()) { - a_amts.push_back(coin.txout.nValue); + a_amts.push_back(coin->txout.nValue); } for (const auto& coin : b.GetInputSet()) { - b_amts.push_back(coin.txout.nValue); + b_amts.push_back(coin->txout.nValue); } std::sort(a_amts.begin(), a_amts.end()); std::sort(b_amts.begin(), b_amts.end()); @@ -118,16 +118,16 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) static bool EqualResult(const SelectionResult& a, const SelectionResult& b) { std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), - [](const COutput& a, const COutput& b) { - return a.outpoint == b.outpoint; + [](const std::shared_ptr& a, const std::shared_ptr& b) { + return a->outpoint == b->outpoint; }); return ret.first == a.GetInputSet().end() && ret.second == b.GetInputSet().end(); } static int GetSelectionWeight(const SelectionResult& result) { - return std::accumulate(result.GetInputSet().cbegin(), result.GetInputSet().cend(), 0, [](int sum, const COutput& coin) { - return sum + std::max(coin.input_bytes, 0); + return std::accumulate(result.GetInputSet().cbegin(), result.GetInputSet().cend(), 0, [](int sum, const std::shared_ptr& coin) { + return sum + std::max(coin->input_bytes, 0); }); } @@ -149,7 +149,7 @@ inline std::vector& GroupCoins(const std::vector& availabl static_groups.clear(); for (auto& coin : available_coins) { static_groups.emplace_back(); - static_groups.back().Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); + static_groups.back().Insert(std::make_shared(coin), /*ancestors=*/ 0, /*descendants=*/ 0); } return static_groups; } @@ -160,7 +160,7 @@ inline std::optional KnapsackSolver(std::vector& g return res ? std::optional(*res) : std::nullopt; } -inline std::vector& KnapsackGroupOutputs(const std::vector& available_coins, CWallet& wallet, const CoinEligibilityFilter& filter) +inline std::vector& KnapsackGroupOutputs(const CoinsResult& available_coins, CWallet& wallet, const CoinEligibilityFilter& filter) { FastRandomContext rand{}; CoinSelectionParams coin_selection_params{ @@ -174,9 +174,9 @@ inline std::vector& KnapsackGroupOutputs(const std::vector /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; - static std::vector static_groups; - static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, filter, /*positive_only=*/false); - return static_groups; + static OutputGroupTypeMap static_groups; + static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, {{filter}})[filter]; + return static_groups.all_groups.mixed_group; } // Branch and bound coin selection tests @@ -440,25 +440,25 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) available_coins.Clear(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT)); add_coin(available_coins, *wallet, 1*CENT, CFeeRate(0), 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT)); // but we can find a new 1 cent - const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result1); BOOST_CHECK_EQUAL(result1->GetSelectedValue(), 1 * CENT); add_coin(available_coins, *wallet, 2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 3 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 3 * CENT, CENT)); // we can make 3 cents of new coins - const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 3 * CENT, CENT); + const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 3 * CENT, CENT); BOOST_CHECK(result2); BOOST_CHECK_EQUAL(result2->GetSelectedValue(), 3 * CENT); @@ -469,38 +469,38 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 38 * CENT, CENT)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard_extra), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard_extra), 38 * CENT, CENT)); // but we can make 37 cents if we accept new coins from ourself - const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 37 * CENT, CENT); + const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 37 * CENT, CENT); BOOST_CHECK(result3); BOOST_CHECK_EQUAL(result3->GetSelectedValue(), 37 * CENT); // and we can make 38 cents if we accept all new coins - const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 38 * CENT, CENT); + const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 38 * CENT, CENT); BOOST_CHECK(result4); BOOST_CHECK_EQUAL(result4->GetSelectedValue(), 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 34 * CENT, CENT); + const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 34 * CENT, CENT); BOOST_CHECK(result5); BOOST_CHECK_EQUAL(result5->GetSelectedValue(), 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(result5->GetInputSet().size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 7 * CENT, CENT); + const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 7 * CENT, CENT); BOOST_CHECK(result6); BOOST_CHECK_EQUAL(result6->GetSelectedValue(), 7 * CENT); BOOST_CHECK_EQUAL(result6->GetInputSet().size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 8 * CENT, CENT); + const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 8 * CENT, CENT); BOOST_CHECK(result7); BOOST_CHECK(result7->GetSelectedValue() == 8 * CENT); BOOST_CHECK_EQUAL(result7->GetInputSet().size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 9 * CENT, CENT); + const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 9 * CENT, CENT); BOOST_CHECK(result8); BOOST_CHECK_EQUAL(result8->GetSelectedValue(), 10 * CENT); BOOST_CHECK_EQUAL(result8->GetInputSet().size(), 1U); @@ -515,12 +515,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 71 * CENT, CENT); + const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 71 * CENT, CENT); BOOST_CHECK(result9); - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 72 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 72 * CENT, CENT)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result10); BOOST_CHECK_EQUAL(result10->GetSelectedValue(), 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(result10->GetInputSet().size(), 1U); @@ -528,7 +528,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result11); BOOST_CHECK_EQUAL(result11->GetSelectedValue(), 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(result11->GetInputSet().size(), 3U); @@ -536,13 +536,13 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result12); BOOST_CHECK_EQUAL(result12->GetSelectedValue(), 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(result12->GetInputSet().size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 11 * CENT, CENT); + const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 11 * CENT, CENT); BOOST_CHECK(result13); BOOST_CHECK_EQUAL(result13->GetSelectedValue(), 11 * CENT); BOOST_CHECK_EQUAL(result13->GetInputSet().size(), 2U); @@ -552,12 +552,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 2*COIN); add_coin(available_coins, *wallet, 3*COIN); add_coin(available_coins, *wallet, 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 95 * CENT, CENT); + const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 95 * CENT, CENT); BOOST_CHECK(result14); BOOST_CHECK_EQUAL(result14->GetSelectedValue(), 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(result14->GetInputSet().size(), 1U); - const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 195 * CENT, CENT); + const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 195 * CENT, CENT); BOOST_CHECK(result15); BOOST_CHECK_EQUAL(result15->GetSelectedValue(), 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(result15->GetInputSet().size(), 1U); @@ -573,7 +573,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * CENT from the 1.5 * CENT // we'll get change smaller than CENT whatever happens, so can expect CENT exactly - const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); + const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result16); BOOST_CHECK_EQUAL(result16->GetSelectedValue(), CENT); @@ -581,7 +581,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 1111*CENT); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result17); BOOST_CHECK_EQUAL(result17->GetSelectedValue(), 1 * CENT); // we should get the exact amount @@ -590,7 +590,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 7 / 10); // and try again to make 1.0 * CENT - const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result18); BOOST_CHECK_EQUAL(result18->GetSelectedValue(), 1 * CENT); // we should get the exact amount @@ -600,7 +600,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(available_coins, *wallet, 50000 * COIN); - const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 500000 * COIN, CENT); + const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 500000 * COIN, CENT); BOOST_CHECK(result19); BOOST_CHECK_EQUAL(result19->GetSelectedValue(), 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(result19->GetInputSet().size(), 10U); // in ten coins @@ -614,7 +614,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 7 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result20); BOOST_CHECK_EQUAL(result20->GetSelectedValue(), 1111 * CENT); // we get the bigger coin BOOST_CHECK_EQUAL(result20->GetInputSet().size(), 1U); @@ -625,7 +625,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 8 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); + const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result21); BOOST_CHECK_EQUAL(result21->GetSelectedValue(), CENT); // we should get the exact amount BOOST_CHECK_EQUAL(result21->GetInputSet().size(), 2U); // in two coins 0.4+0.6 @@ -637,13 +637,13 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 100); // trying to make 100.01 from these three coins - const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 10001 / 100, CENT); + const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT * 10001 / 100, CENT); BOOST_CHECK(result22); BOOST_CHECK_EQUAL(result22->GetSelectedValue(), CENT * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(result22->GetInputSet().size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 9990 / 100, CENT); + const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT * 9990 / 100, CENT); BOOST_CHECK(result23); BOOST_CHECK_EQUAL(result23->GetSelectedValue(), 101 * CENT); BOOST_CHECK_EQUAL(result23->GetInputSet().size(), 2U); @@ -658,7 +658,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 2000, CENT); + const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 2000, CENT); BOOST_CHECK(result24); if (amt - 2000 < CENT) { @@ -749,7 +749,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(available_coins, *wallet, 1000 * COIN); add_coin(available_coins, *wallet, 3 * COIN); - const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1003 * COIN, CENT, rand); + const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1003 * COIN, CENT, rand); BOOST_CHECK(result); BOOST_CHECK_EQUAL(result->GetSelectedValue(), 1003 * COIN); BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U); @@ -1046,9 +1046,8 @@ static util::Result SelectCoinsSRDResult(const CAmount& target, wallet->SetupDescriptorScriptPubKeyMans("", ""); CoinEligibilityFilter filter(0, 0, 0); - auto available_coins = coin_setup(*wallet); - auto groups = GroupOutputs(*wallet, available_coins.All(), cs_params, filter, /*positive_only=*/true); - return wallet::SelectCoinsSRD(groups, target, cs_params.rng_fast, max_weight, false); + Groups group = GroupOutputs(*wallet, coin_setup(*wallet), cs_params, {{filter}})[filter].all_groups; + return wallet::SelectCoinsSRD(group.positive_group, target, cs_params.rng_fast, max_weight, false); } BOOST_AUTO_TEST_CASE(srd_tests) @@ -1126,7 +1125,7 @@ static util::Result select_coins(const CAmount& target, const C static bool has_coin(const CoinSet& set, CAmount amount) { - return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin.GetEffectiveValue() == amount; }); + return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin->GetEffectiveValue() == amount; }); } BOOST_AUTO_TEST_CASE(check_max_weight) @@ -1159,7 +1158,7 @@ BOOST_AUTO_TEST_CASE(check_max_weight) BOOST_CHECK(result); BOOST_CHECK_EQUAL(result->GetInputSet().size(), 1U); - BOOST_CHECK_EQUAL(result->GetInputSet().begin()->GetEffectiveValue(), 50 * COIN); + BOOST_CHECK_EQUAL((*result->GetInputSet().begin())->GetEffectiveValue(), 50 * COIN); } { @@ -1206,7 +1205,7 @@ BOOST_AUTO_TEST_CASE(check_max_weight) BOOST_REQUIRE(result); BOOST_CHECK_EQUAL(result->GetInputSet().size(), 1U); - BOOST_CHECK_EQUAL(result->GetInputSet().begin()->GetEffectiveValue(), 50 * COIN); + BOOST_CHECK_EQUAL((*result->GetInputSet().begin())->GetEffectiveValue(), 50 * COIN); } } diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index f8d8769ea900..4903b5477574 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -32,8 +32,10 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect auto output_group = OutputGroup(coin_params); bool valid_outputgroup{false}; for (auto& coin : coins) { - output_group.Insert(coin, /*ancestors=*/0, /*descendants=*/0, positive_only); - // If positive_only was specified, nothing may have been inserted, leading to an empty output group + if (!positive_only || (positive_only && coin.GetEffectiveValue() > 0)) { + output_group.Insert(std::make_shared(coin), /*ancestors=*/0, /*descendants=*/0); + } + // If positive_only was specified, nothing was inserted, leading to an empty output group // that would be invalid for the BnB algorithm valid_outputgroup = !positive_only || output_group.GetSelectionAmount() > 0; if (valid_outputgroup && fuzzed_data_provider.ConsumeBool()) { diff --git a/src/wallet/test/group_outputs_tests.cpp b/src/wallet/test/group_outputs_tests.cpp new file mode 100644 index 000000000000..bb959e25d681 --- /dev/null +++ b/src/wallet/test/group_outputs_tests.cpp @@ -0,0 +1,234 @@ +// 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 + +namespace wallet { +BOOST_FIXTURE_TEST_SUITE(group_outputs_tests, TestingSetup) + +static int nextLockTime = 0; + +static std::shared_ptr NewWallet(const node::NodeContext& m_node) +{ + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + wallet->LoadWallet(); + LOCK(wallet->cs_wallet); + wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet->SetupDescriptorScriptPubKeyMans("", ""); + return wallet; +} + +static void addCoin(CoinsResult& coins, + CWallet& wallet, + const CTxDestination& dest, + const CAmount& nValue, + bool is_from_me, + CFeeRate fee_rate = CFeeRate(0), + int depth = 6) +{ + CMutableTransaction tx; + tx.nLockTime = nextLockTime++; // so all transactions get different hashes + tx.vout.resize(1); + tx.vout[0].nValue = nValue; + tx.vout[0].scriptPubKey = GetScriptForDestination(dest); + + const uint256& txid = tx.GetHash(); + LOCK(wallet.cs_wallet); + auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{})); + assert(ret.second); + CWalletTx& wtx = (*ret.first).second; + const auto& txout = wtx.tx->vout.at(0); + coins.Add(OutputType::LEGACY, + {COutPoint(wtx.GetHash(), 0), + txout, + depth, + CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), + /*spendable=*/ true, + /*solvable=*/ true, + /*safe=*/ true, + wtx.GetTxTime(), + is_from_me, + fee_rate}); +} + + CoinSelectionParams makeSelectionParams(FastRandomContext& rand, bool avoid_partial_spends) +{ + return CoinSelectionParams{ + rand, + /*change_output_size=*/ 0, + /*change_spend_size=*/ 0, + /*min_change_target=*/ CENT, + /*effective_feerate=*/ CFeeRate(0), + /*long_term_feerate=*/ CFeeRate(0), + /*discard_feerate=*/ CFeeRate(0), + /*tx_noinputs_size=*/ 0, + /*avoid_partial=*/ avoid_partial_spends, + }; +} + +class GroupVerifier +{ +public: + std::shared_ptr wallet{nullptr}; + CoinsResult coins_pool; + FastRandomContext rand; + + void GroupVerify(const OutputType type, + const CoinEligibilityFilter& filter, + bool avoid_partial_spends, + bool positive_only, + int expected_size) + { + OutputGroupTypeMap groups = GroupOutputs(*wallet, coins_pool, makeSelectionParams(rand, avoid_partial_spends), {{filter}})[filter]; + std::vector& groups_out = positive_only ? groups.groups_by_type[type].positive_group : + groups.groups_by_type[type].mixed_group; + BOOST_CHECK_EQUAL(groups_out.size(), expected_size); + } + + void GroupAndVerify(const OutputType type, + const CoinEligibilityFilter& filter, + int expected_with_partial_spends_size, + int expected_without_partial_spends_size, + bool positive_only) + { + // First avoid partial spends + GroupVerify(type, filter, /*avoid_partial_spends=*/false, positive_only, expected_with_partial_spends_size); + // Second don't avoid partial spends + GroupVerify(type, filter, /*avoid_partial_spends=*/true, positive_only, expected_without_partial_spends_size); + } +}; + +BOOST_AUTO_TEST_CASE(outputs_grouping_tests) +{ + const auto& wallet = NewWallet(m_node); + GroupVerifier group_verifier; + group_verifier.wallet = wallet; + + const CoinEligibilityFilter& BASIC_FILTER{1, 6, 0}; + + // ################################################################################# + // 10 outputs from different txs going to the same script + // 1) if partial spends is enabled --> must not be grouped + // 2) if partial spends is not enabled --> must be grouped into a single OutputGroup + // ################################################################################# + + unsigned long GROUP_SIZE = 10; + const CTxDestination dest = *Assert(wallet->GetNewDestination("")); + for (unsigned long i = 0; i < GROUP_SIZE; i++) { + addCoin(group_verifier.coins_pool, *wallet, dest, 10 * COIN, /*is_from_me=*/true); + } + + group_verifier.GroupAndVerify(OutputType::LEGACY, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE, + /*expected_without_partial_spends_size=*/ 1, + /*positive_only=*/ true); + + // #################################################################################### + // 3) 10 more UTXO are added with a different script --> must be grouped into a single + // group for avoid partial spends and 10 different output groups for partial spends + // #################################################################################### + + const CTxDestination dest2 = *Assert(wallet->GetNewDestination("")); + for (unsigned long i = 0; i < GROUP_SIZE; i++) { + addCoin(group_verifier.coins_pool, *wallet, dest2, 5 * COIN, /*is_from_me=*/true); + } + + group_verifier.GroupAndVerify(OutputType::LEGACY, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, + /*expected_without_partial_spends_size=*/ 2, + /*positive_only=*/ true); + + // ################################################################################ + // 4) Now add a negative output --> which will be skipped if "positive_only" is set + // ################################################################################ + + const CTxDestination dest3 = *Assert(wallet->GetNewDestination("")); + addCoin(group_verifier.coins_pool, *wallet, dest3, 1, true, CFeeRate(100)); + BOOST_CHECK(group_verifier.coins_pool.coins[OutputType::LEGACY].back().GetEffectiveValue() <= 0); + + // First expect no changes with "positive_only" enabled + group_verifier.GroupAndVerify(OutputType::LEGACY, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, + /*expected_without_partial_spends_size=*/ 2, + /*positive_only=*/ true); + + // Then expect changes with "positive_only" disabled + group_verifier.GroupAndVerify(OutputType::LEGACY, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + + // ############################################################################## + // 5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for + // "not mine" UTXOs) --> it must not be added to any group + // ############################################################################## + + const CTxDestination dest4 = *Assert(wallet->GetNewDestination("")); + addCoin(group_verifier.coins_pool, *wallet, dest4, 6 * COIN, + /*is_from_me=*/false, CFeeRate(0), /*depth=*/5); + + // Expect no changes from this round and the previous one (point 4) + group_verifier.GroupAndVerify(OutputType::LEGACY, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + + // ############################################################################## + // 6) Try to add a non-eligible UTXO (due not fulfilling the min depth target for + // "mine" UTXOs) --> it must not be added to any group + // ############################################################################## + + const CTxDestination dest5 = *Assert(wallet->GetNewDestination("")); + addCoin(group_verifier.coins_pool, *wallet, dest5, 6 * COIN, + /*is_from_me=*/true, CFeeRate(0), /*depth=*/0); + + // Expect no changes from this round and the previous one (point 5) + group_verifier.GroupAndVerify(OutputType::LEGACY, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + // ########################################################################################### + // 7) Surpass the OUTPUT_GROUP_MAX_ENTRIES and verify that a second partial group gets created + // ########################################################################################### + + const CTxDestination dest7 = *Assert(wallet->GetNewDestination("")); + uint16_t NUM_SINGLE_ENTRIES = 101; + for (unsigned long i = 0; i < NUM_SINGLE_ENTRIES; i++) { // OUTPUT_GROUP_MAX_ENTRIES{100} + addCoin(group_verifier.coins_pool, *wallet, dest7, 9 * COIN, /*is_from_me=*/true); + } + + // Exclude partial groups only adds one more group to the previous test case (point 6) + int PREVIOUS_ROUND_COUNT = GROUP_SIZE * 2 + 1; + group_verifier.GroupAndVerify(OutputType::LEGACY, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, + /*expected_without_partial_spends_size=*/ 4, + /*positive_only=*/ false); + + // Include partial groups should add one more group inside the "avoid partial spends" count + const CoinEligibilityFilter& avoid_partial_groups_filter{1, 6, 0, 0, /*include_partial=*/ true}; + group_verifier.GroupAndVerify(OutputType::LEGACY, + avoid_partial_groups_filter, + /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, + /*expected_without_partial_spends_size=*/ 5, + /*positive_only=*/ false); +} + +BOOST_AUTO_TEST_SUITE_END() +} // end namespace wallet From ea7d5ecfb414a39fe79c174781bd9f7681bf7d85 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Sun, 18 Oct 2026 00:13:09 -0500 Subject: [PATCH 2/6] Merge bitcoin/bitcoin#26715: Introduce `MockableDatabase` for wallet unit tests 33e2b82a4fc990253ff77655f437c7aed336bc55 wallet, bench: Remove unused database options from WalletBenchLoading (Andrew Chow) 80ace042d8fece9be50bfef1be64c6e5720e87e6 tests: Modify records directly in wallet ckey loading test (Andrew Chow) b3bb17d5d07f51ac2e501e4a7a3bbcd17144070f tests: Update DuplicateMockDatabase for MockableDatabase (Andrew Chow) f0eecf5e408238c64b77b0a4974ba2b9edb17487 scripted-diff: Replace CreateMockWalletDB with CreateMockableWalletDB (Andrew Chow) 075962bc25a90661612fe4613cd50ea1cae21f52 wallet, tests: Include wallet/test/util.h (Andrew Chow) 14aa4cb1e44f089a6022a2b14a98bca4a7dd9a01 wallet: Move DummyDatabase to salvage (Andrew Chow) f67a385556c60b2e4788a378196a395fca0539f5 wallet, tests: Replace usage of dummy db with mockable db (Andrew Chow) 33c6245ac1ecdfe25b1ee4fd9e93c43393634ae3 Introduce MockableDatabase for wallet unit tests (Andrew Chow) Pull request description: For the wallet's unit tests, we currently use either `DummyDatabase` or memory-only versions of either BDB or SQLite. The tests that use `DummyDatabase` just need a `WalletDatabase` so that the `CWallet` can be constructed, while the tests using the memory-only databases just need a backing data store. There is also a `FailDatabase` that is similar to `DummyDatabase` except it fails be default or can have a configured return value. Having all of these different database types can make it difficult to write tests, particularly tests that work when either BDB or SQLite is disabled. This PR unifies all of these different unit test database classes into a single `MockableDatabase`. Like `DummyDatabase`, most functions do nothing and just return true. Like `FailDatabase`, the return value of some functions can be configured on the fly to test various failure cases. Like the memory-only databases, records can actually be written to the `MockableDatabase` and be retrieved later, but all of this is still held in memory. Using `MockableDatabase` completely removes the need for having BDB or SQLite backed wallets in the unit tests for the tests that are not actually testing specific database behaviors. Because `MockableDatabase`s can be created by each unit test, we can also control what records are stored in the database. Records can be added and removed externally from the typical database modification functions. This will give us greater ability to test failure conditions, particularly those involving corrupted records. Possible alternative to #26644 ACKs for top commit: furszy: ACK 33e2b82 TheCharlatan: ACK 33e2b82a4fc990253ff77655f437c7aed336bc55 Tree-SHA512: c2b09eff9728d063d2d4aea28a0f0e64e40b76483e75dc53f08667df23bd25834d52656cd4eafb02e552db0b9e619cfdb1b1c65b26b5436ee2c971d804768bcc Dash adaptations: - src/wallet/salvage.cpp: DummyDatabase replaces CreateDummyWalletDatabase() but keeps Dash's DB_TXN_WRITE_NOSYNC and dummyWallet.SetupLegacyScriptPubKeyMan() - src/wallet/test/coinselector_tests.cpp: Dash has no NewWallet() (its bitcoin#26720 backport skipped 25ab14712b), so the 11 inline wallet creations it replaces upstream use CreateMockableWalletDatabase - src/wallet/test/wallet_tests.cpp: removes Dash's relocated FailDatabase/FailBatch/FailCursor and ports Dash-only interface_coin_lock_failed_persist to CreateMockableWalletDatabase + GetMockableDatabase().m_pass - src/wallet/test/wallet_tests.cpp: keeps Dash-only SetupLegacyScriptPubKeyMan() and tip lookups around the renamed wallet constructions - src/wallet/test/coinjoin_tests.cpp: fixture wallet uses CreateMockableWalletDatabase so DuplicateMockDatabase(db) works; persist-retry test fails the write via m_pass, not an open SQLite txn, and drops its USE_SQLITE guard - src/qt/test/wallettests.cpp: Dash's own wallet setup is renamed to CreateMockableWalletDatabase; upstream's SetupLegacyWatchOnlyWallet/SetupDescriptorsWallet/MiniGUI do not exist in Dash - src/wallet/test/walletload_tests.cpp: takes upstream's comment but keeps Dash's GetNewDestination() without an OutputType argument - src/wallet/test/util.cpp, wallet_test_fixture.cpp, scriptpubkeyman_tests.cpp: rename only; Dash's ChainInfo snapshot, string chain name/loader args and local NodeContext chain are kept - build_msvc: Dash's autotools build already links wallet/test/util.cpp via libtest_util.a into bench, test_dash and test_dash-qt Not applicable to Dash (intentionally omitted): - build_msvc/libtest_util/libtest_util.vcxproj.in: Dash has no build_msvc; its counterpart libtest_util.a (Makefile.test_util.include) already contains wallet/test/util.cpp - build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj: Dash has no build_msvc; test_dash-qt already links $(LIBTEST_UTIL) in Makefile.qttest.include - src/wallet/test/ismine_tests.cpp: renames inside P2WPKH/P2WSH/P2TR/witness test blocks, which Dash dropped because it has no segwit/taproot; all 19 remaining Dash cases are renamed --- src/bench/coin_selection.cpp | 5 +- src/bench/wallet_balance.cpp | 4 +- src/bench/wallet_create_tx.cpp | 6 +- src/bench/wallet_loading.cpp | 27 +++-- src/qt/test/addressbooktests.cpp | 5 +- src/qt/test/wallettests.cpp | 5 +- src/wallet/db.h | 45 --------- src/wallet/salvage.cpp | 47 ++++++++- src/wallet/test/coinjoin_tests.cpp | 20 ++-- src/wallet/test/coinselector_tests.cpp | 23 ++--- src/wallet/test/group_outputs_tests.cpp | 3 +- src/wallet/test/ismine_tests.cpp | 39 ++++---- src/wallet/test/scriptpubkeyman_tests.cpp | 3 +- src/wallet/test/util.cpp | 115 +++++++++++++++++----- src/wallet/test/util.h | 77 ++++++++++++++- src/wallet/test/wallet_test_fixture.cpp | 3 +- src/wallet/test/wallet_tests.cpp | 87 +++------------- src/wallet/test/walletload_tests.cpp | 97 +++++++++--------- src/wallet/walletdb.cpp | 6 -- src/wallet/walletdb.h | 3 - 20 files changed, 342 insertions(+), 278 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index d9a8f0d357e3..4999b0cb8def 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include @@ -17,7 +18,7 @@ using wallet::CHANGE_LOWER; using wallet::CoinEligibilityFilter; using wallet::CoinSelectionParams; using wallet::COutput; -using wallet::CreateDummyWalletDatabase; +using wallet::CreateMockableWalletDatabase; using wallet::CWallet; using wallet::CWalletTx; using wallet::OutputGroup; @@ -44,7 +45,7 @@ static void CoinSelection(benchmark::Bench& bench) { NodeContext node; auto chain = interfaces::MakeChain(node); - CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateMockableWalletDatabase()); std::vector> wtxs; LOCK(wallet.cs_wallet); diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index 578dae5137dd..21357dd44323 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -15,7 +15,7 @@ #include -using wallet::CreateMockWalletDatabase; +using wallet::CreateMockableWalletDatabase; using wallet::CWallet; using wallet::DBErrors; using wallet::WALLET_FLAG_DESCRIPTORS; @@ -31,7 +31,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b // Set clock to genesis block, so the descriptors/keys creation time don't interfere with the blocks scanning process. // The reason is 'generatetoaddress', which creates a chain with deterministic timestamps in the past. SetMockTime(test_setup->m_node.chainman->GetParams().GenesisBlock().nTime); - CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockWalletDatabase()}; + CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase()}; { LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 022234c40298..17c249641518 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -15,7 +15,7 @@ #include using wallet::CWallet; -using wallet::CreateMockWalletDatabase; +using wallet::CreateMockableWalletDatabase; using wallet::DBErrors; using wallet::WALLET_FLAG_DESCRIPTORS; @@ -84,7 +84,7 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type { const auto test_setup = MakeNoLogFileContext(); - CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockWalletDatabase()}; + CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase()}; { LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -137,7 +137,7 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type static void AvailableCoins(benchmark::Bench& bench, const std::vector& output_type) { const auto test_setup = MakeNoLogFileContext(); - CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockWalletDatabase()}; + CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase()}; { LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/bench/wallet_loading.cpp b/src/bench/wallet_loading.cpp index 0b0574eceaa5..fd165d072596 100644 --- a/src/bench/wallet_loading.cpp +++ b/src/bench/wallet_loading.cpp @@ -21,18 +21,17 @@ #include using wallet::CWallet; -using wallet::DatabaseFormat; -using wallet::DatabaseOptions; +using wallet::CreateMockableWalletDatabase; using wallet::TxStateInactive; using wallet::WALLET_FLAG_DESCRIPTORS; using wallet::WalletContext; using wallet::WalletDatabase; -static std::shared_ptr BenchLoadWallet(std::unique_ptr database, WalletContext& context, DatabaseOptions& options) +static std::shared_ptr BenchLoadWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags) { bilingual_str error; std::vector warnings; - auto wallet = CWallet::Create(context, "", std::move(database), options.create_flags, error, warnings); + auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings); NotifyWalletLoaded(context, wallet); if (context.chain) { wallet->postInitProcess(); @@ -59,7 +58,6 @@ static void AddTx(CWallet& wallet) static void WalletLoading(benchmark::Bench& bench, bool legacy_wallet) { const auto test_setup = MakeNoLogFileContext(); - test_setup->m_args.ForceSetArg("-unsafesqlitesync", "1"); WalletContext context; context.args = &test_setup->m_args; @@ -67,31 +65,28 @@ static void WalletLoading(benchmark::Bench& bench, bool legacy_wallet) // Setup the wallet // Loading the wallet will also create it - DatabaseOptions options; - if (legacy_wallet) { - options.require_format = DatabaseFormat::BERKELEY; - } else { - options.create_flags = WALLET_FLAG_DESCRIPTORS; - options.require_format = DatabaseFormat::SQLITE; + uint64_t create_flags = 0; + if (!legacy_wallet) { + create_flags = WALLET_FLAG_DESCRIPTORS; } - auto database = CreateMockWalletDatabase(options); - auto wallet = BenchLoadWallet(std::move(database), context, options); + auto database = CreateMockableWalletDatabase(); + auto wallet = BenchLoadWallet(std::move(database), context, create_flags); // Generate a bunch of transactions and addresses to put into the wallet for (int i = 0; i < 1000; ++i) { AddTx(*wallet); } - database = DuplicateMockDatabase(wallet->GetDatabase(), options); + database = DuplicateMockDatabase(wallet->GetDatabase()); // reload the wallet for the actual benchmark BenchUnloadWallet(std::move(wallet)); bench.epochs(5).run([&] { - wallet = BenchLoadWallet(std::move(database), context, options); + wallet = BenchLoadWallet(std::move(database), context, create_flags); // Cleanup - database = DuplicateMockDatabase(wallet->GetDatabase(), options); + database = DuplicateMockDatabase(wallet->GetDatabase()); BenchUnloadWallet(std::move(wallet)); }); } diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 5143de883569..b038bc842598 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -29,7 +30,7 @@ #include using wallet::AddWallet; -using wallet::CreateMockWalletDatabase; +using wallet::CreateMockableWalletDatabase; using wallet::CWallet; using wallet::RemoveWallet; using wallet::WALLET_FLAG_DESCRIPTORS; @@ -72,7 +73,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) { TestChain100Setup test; node.setContext(&test.m_node); - const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); { diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index f31adf43f1d9..3133b0dc1798 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -45,7 +46,7 @@ using wallet::AddWallet; using wallet::CWallet; -using wallet::CreateMockWalletDatabase; +using wallet::CreateMockableWalletDatabase; using wallet::RemoveWallet; using wallet::WALLET_FLAG_DESCRIPTORS; using wallet::WalletContext; @@ -132,7 +133,7 @@ void TestGUI(interfaces::Node& node) } node.setContext(&test.m_node); WalletContext& context = *node.walletLoader().context(); - const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockableWalletDatabase()); AddWallet(context, wallet); wallet->LoadWallet(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/wallet/db.h b/src/wallet/db.h index dc89ed9497c3..323c506970cb 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -175,51 +175,6 @@ class WalletDatabase virtual bool SupportsAutoBackup() { return false; } }; -class DummyCursor : public DatabaseCursor -{ - Status Next(DataStream& key, DataStream& value) override { return Status::FAIL; } -}; - -/** RAII class that provides access to a DummyDatabase. Never fails. */ -class DummyBatch : public DatabaseBatch -{ -private: - bool ReadKey(DataStream&& key, DataStream& value) override { return true; } - bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite=true) override { return true; } - bool EraseKey(DataStream&& key) override { return true; } - bool HasKey(DataStream&& key) override { return true; } - bool ErasePrefix(Span prefix) override { return true; } - -public: - void Flush() override {} - void Close() override {} - - std::unique_ptr GetNewCursor() override { return std::make_unique(); } - bool TxnBegin() override { return true; } - bool TxnCommit() override { return true; } - bool TxnAbort() override { return true; } -}; - -/** A dummy WalletDatabase that does nothing and never fails. Only used by unit tests. - **/ -class DummyDatabase : public WalletDatabase -{ -public: - void Open() override {}; - void AddRef() override {} - void RemoveRef() override {} - bool Rewrite(const char* pszSkip=nullptr) override { return true; } - bool Backup(const std::string& strDest) const override { return true; } - void Close() override {} - void Flush() override {} - bool PeriodicFlush() override { return true; } - void IncrementUpdateCounter() override { ++nUpdateCounter; } - void ReloadDbEnv() override {} - std::string Filename() override { return "dummy"; } - std::string Format() override { return "dummy"; } - std::unique_ptr MakeBatch(bool flush_on_close = true) override { return std::make_unique(); } -}; - enum class DatabaseFormat { BERKELEY, SQLITE, diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index b25d83a33a31..8bd79b5fbd9d 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -25,6 +25,51 @@ static bool KeyFilter(const std::string& type) return WalletBatch::IsKeyType(type) || type == DBKeys::HDCHAIN; } +class DummyCursor : public DatabaseCursor +{ + Status Next(DataStream& key, DataStream& value) override { return Status::FAIL; } +}; + +/** RAII class that provides access to a DummyDatabase. Never fails. */ +class DummyBatch : public DatabaseBatch +{ +private: + bool ReadKey(DataStream&& key, DataStream& value) override { return true; } + bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite=true) override { return true; } + bool EraseKey(DataStream&& key) override { return true; } + bool HasKey(DataStream&& key) override { return true; } + bool ErasePrefix(Span prefix) override { return true; } + +public: + void Flush() override {} + void Close() override {} + + std::unique_ptr GetNewCursor() override { return std::make_unique(); } + bool TxnBegin() override { return true; } + bool TxnCommit() override { return true; } + bool TxnAbort() override { return true; } +}; + +/** A dummy WalletDatabase that does nothing and never fails. Only used by salvage. + **/ +class DummyDatabase : public WalletDatabase +{ +public: + void Open() override {}; + void AddRef() override {} + void RemoveRef() override {} + bool Rewrite(const char* pszSkip=nullptr) override { return true; } + bool Backup(const std::string& strDest) const override { return true; } + void Close() override {} + void Flush() override {} + bool PeriodicFlush() override { return true; } + void IncrementUpdateCounter() override { ++nUpdateCounter; } + void ReloadDbEnv() override {} + std::string Filename() override { return "dummy"; } + std::string Format() override { return "dummy"; } + std::unique_ptr MakeBatch(bool flush_on_close = true) override { return std::make_unique(); } +}; + bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bilingual_str& error, std::vector& warnings) { DatabaseOptions options; @@ -137,7 +182,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil } DbTxn* ptxn = env->TxnBegin(DB_TXN_WRITE_NOSYNC); - CWallet dummyWallet(/*chain=*/nullptr, "", CreateDummyWalletDatabase()); + CWallet dummyWallet(/*chain=*/nullptr, "", std::make_unique()); dummyWallet.SetupLegacyScriptPubKeyMan(); for (KeyValPair& row : salvagedData) { diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index ffac18054bda..ed25629ecd1d 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -147,7 +147,7 @@ class CTransactionBuilderTestSetup : public TestChain100Setup { public: CTransactionBuilderTestSetup() : - wallet{std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase())} + wallet{std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase())} { context.args = &m_args; context.chain = m_node.chain.get(); @@ -474,9 +474,6 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_unreadable_tests, CTransact SetMockTime(0); } -// The failed write is provoked with a transaction left open on the database, which only blocks -// further transactions on SQLite: BerkeleyBatch::TxnBegin() starts an independent one. -#ifdef USE_SQLITE BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_persist_retry_tests, CTransactionBuilderTestSetup) { // 0.100001 DASH, a valid CoinJoin denomination @@ -486,13 +483,10 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_persist_retry_tests, CTrans BOOST_CHECK(m_node.cj_walletman->doForClient("", [&](CCoinJoinClientManager& cj_man) { // Load the (missing) record first so that only the write itself fails below cj_man.CheckPendingObservations(*m_node.mempool); - { - // A transaction left open on the database keeps the manager from starting its own - WalletBatch blocker(wallet->GetDatabase()); - BOOST_REQUIRE(blocker.TxnBegin()); - cj_man.AddPendingObservation({outpoint}); - BOOST_REQUIRE(blocker.TxnAbort()); - } + // A failing database keeps the manager from starting its own transaction + GetMockableDatabase(*wallet).m_pass = false; + cj_man.AddPendingObservation({outpoint}); + GetMockableDatabase(*wallet).m_pass = true; BOOST_CHECK(cj_man.IsPendingObservation(outpoint)); BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpoint))); @@ -508,16 +502,14 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_persist_retry_tests, CTrans // Load a copy of the wallet database, the way a restart would: the input must still // be locked and tracked - DatabaseOptions options; const auto reloaded = std::make_shared(m_node.chain.get(), "", - DuplicateMockDatabase(wallet->GetDatabase(), options)); + DuplicateMockDatabase(wallet->GetDatabase())); BOOST_REQUIRE_EQUAL(reloaded->LoadWallet(), DBErrors::LOAD_OK); BOOST_CHECK(WITH_LOCK(reloaded->cs_wallet, return reloaded->IsLockedCoin(outpoint))); std::map persisted; BOOST_CHECK(WalletBatch(reloaded->GetDatabase()).ReadCoinJoinPendingObs(persisted)); BOOST_CHECK(persisted.count(outpoint) > 0); } -#endif // USE_SQLITE BOOST_FIXTURE_TEST_CASE(coinjoin_manager_start_stop_tests, CTransactionBuilderTestSetup) { diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 83ee597a0b2f..5d521c364eff 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -315,7 +316,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) 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); { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -337,7 +338,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -360,7 +361,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) BOOST_CHECK(result10); } { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -426,7 +427,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) FastRandomContext rand{}; const auto temp1{[&rand](std::vector& g, const CAmount& v, CAmount c) { return KnapsackSolver(g, v, c, rand); }}; const auto KnapsackSolver{temp1}; - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -736,7 +737,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) BOOST_AUTO_TEST_CASE(ApproximateBestSubset) { FastRandomContext rand{}; - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -758,7 +759,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) // Tests that with the ideal conditions, the coin selector will always be able to find a solution that can pay the target value BOOST_AUTO_TEST_CASE(SelectCoins_test) { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -949,7 +950,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) // 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(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -957,7 +958,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) CoinsResult available_coins; { - std::unique_ptr dummyWallet = std::make_unique(m_node.chain.get(), "dummy", CreateMockWalletDatabase()); + std::unique_ptr dummyWallet = std::make_unique(m_node.chain.get(), "dummy", CreateMockableWalletDatabase()); dummyWallet->LoadWallet(); LOCK(dummyWallet->cs_wallet); dummyWallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -998,7 +999,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_coinsresult_test, TestingSetup) // Test case to verify CoinsResult object sanity. CoinsResult available_coins; { - std::unique_ptr dummyWallet = std::make_unique(m_node.chain.get(), "dummy", CreateMockWalletDatabase()); + std::unique_ptr dummyWallet = std::make_unique(m_node.chain.get(), "dummy", CreateMockableWalletDatabase()); BOOST_CHECK_EQUAL(dummyWallet->LoadWallet(), DBErrors::LOAD_OK); LOCK(dummyWallet->cs_wallet); dummyWallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -1039,7 +1040,7 @@ static util::Result SelectCoinsSRDResult(const CAmount& target, int max_weight, std::function coin_setup) { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -1108,7 +1109,7 @@ BOOST_AUTO_TEST_CASE(srd_tests) static util::Result select_coins(const CAmount& target, const CoinSelectionParams& cs_params, const CCoinControl& cc, std::function coin_setup, const node::NodeContext& m_node) { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/wallet/test/group_outputs_tests.cpp b/src/wallet/test/group_outputs_tests.cpp index bb959e25d681..e25ea5c2b0b5 100644 --- a/src/wallet/test/group_outputs_tests.cpp +++ b/src/wallet/test/group_outputs_tests.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -17,7 +18,7 @@ static int nextLockTime = 0; static std::shared_ptr NewWallet(const node::NodeContext& m_node) { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockableWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp index c0c4e06ab633..77a70a7c76b5 100644 --- a/src/wallet/test/ismine_tests.cpp +++ b/src/wallet/test/ismine_tests.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include @@ -56,7 +57,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK compressed - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForRawPubKey(pubkeys[0]); @@ -75,7 +76,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK compressed - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "pk(" + EncodeSecret(keys[0]) + ")"; auto spk_manager = CreateDescriptor(keystore, desc_str, true); @@ -87,7 +88,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK uncompressed - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForRawPubKey(uncompressedPubkey); @@ -106,7 +107,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK uncompressed - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "pk(" + EncodeSecret(uncompressedKey) + ")"; auto spk_manager = CreateDescriptor(keystore, desc_str, true); @@ -118,7 +119,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH compressed - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForDestination(PKHash(pubkeys[0])); @@ -137,7 +138,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH compressed - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "pkh(" + EncodeSecret(keys[0]) + ")"; auto spk_manager = CreateDescriptor(keystore, desc_str, true); @@ -149,7 +150,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH uncompressed - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForDestination(PKHash(uncompressedPubkey)); @@ -168,7 +169,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH uncompressed - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "pkh(" + EncodeSecret(uncompressedKey) + ")"; auto spk_manager = CreateDescriptor(keystore, desc_str, true); @@ -180,7 +181,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -207,7 +208,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "sh(pkh(" + EncodeSecret(keys[0]) + "))"; auto spk_manager = CreateDescriptor(keystore, desc_str, true); @@ -220,7 +221,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // (P2PKH inside) P2SH inside P2SH (invalid) - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -239,7 +240,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // (P2PKH inside) P2SH inside P2SH (invalid) - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "sh(sh(" + EncodeSecret(keys[0]) + "))"; auto spk_manager = CreateDescriptor(keystore, desc_str, false); @@ -251,7 +252,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // scriptPubKey multisig - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -286,7 +287,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // scriptPubKey multisig - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "multi(2, " + EncodeSecret(uncompressedKey) + ", " + EncodeSecret(keys[1]) + ")"; auto spk_manager = CreateDescriptor(keystore, desc_str, true); @@ -298,7 +299,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH multisig - Legacy { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(uncompressedKey)); @@ -321,7 +322,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH multisig - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "sh(multi(2, " + EncodeSecret(uncompressedKey) + ", " + EncodeSecret(keys[1]) + "))"; @@ -335,7 +336,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // Combo - Descriptor { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); std::string desc_str = "combo(" + EncodeSecret(keys[0]) + ")"; @@ -358,7 +359,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // OP_RETURN { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); @@ -373,7 +374,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // Nonstandard { - CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateMockableWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 6a2ea01c9acf..b1877e93652b 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include @@ -22,7 +23,7 @@ BOOST_AUTO_TEST_CASE(CanProvide) // Set up wallet and keyman variables. NodeContext node; std::unique_ptr chain = interfaces::MakeChain(node); - CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateMockableWalletDatabase()); LegacyScriptPubKeyMan& keyman = *wallet.GetOrCreateLegacyScriptPubKeyMan(); // Make a 1 of 2 multisig script diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index 4f4dc8c11fe5..337ef997e98b 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -29,7 +30,7 @@ std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, Chainstate chainman.ActiveChain().Tip()->GetBlockHash(), chainman.ActiveChain().Genesis()->GetBlockHash()}))}; - auto wallet = std::make_unique(&chain, "", CreateMockWalletDatabase()); + auto wallet = std::make_unique(&chain, "", CreateMockableWalletDatabase()); { LOCK(wallet->cs_wallet); wallet->SetLastBlockProcessed(chain_info.height, chain_info.tip_hash); @@ -57,28 +58,9 @@ std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, Chainstate return wallet; } -std::unique_ptr DuplicateMockDatabase(WalletDatabase& database, DatabaseOptions& options) +std::unique_ptr DuplicateMockDatabase(WalletDatabase& database) { - auto new_database = CreateMockWalletDatabase(options); - - // Get a cursor to the original database - auto batch = database.MakeBatch(); - std::unique_ptr cursor = batch->GetNewCursor(); - - // Get a batch for the new database - auto new_batch = new_database->MakeBatch(); - - // Read all records from the original database and write them to the new one - while (true) { - DataStream key{}; - DataStream value{}; - DatabaseCursor::Status status = cursor->Next(key, value); - assert(status != DatabaseCursor::Status::FAIL); - if (status == DatabaseCursor::Status::DONE) break; - new_batch->Write(Span{key}, Span{value}); - } - - return new_database; + return std::make_unique(dynamic_cast(database).m_records); } std::string getnewaddress(CWallet& w) @@ -93,4 +75,93 @@ CTxDestination getNewDestination(CWallet& w, OutputType output_type) return *Assert(w.GetNewDestination("")); } +DatabaseCursor::Status MockableCursor::Next(DataStream& key, DataStream& value) +{ + if (!m_pass) { + return Status::FAIL; + } + if (m_cursor == m_cursor_end) { + return Status::DONE; + } + const auto& [key_data, value_data] = *m_cursor; + key.write(key_data); + value.write(value_data); + m_cursor++; + return Status::MORE; +} + +bool MockableBatch::ReadKey(DataStream&& key, DataStream& value) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + const auto& it = m_records.find(key_data); + if (it == m_records.end()) { + return false; + } + value.write(it->second); + return true; +} + +bool MockableBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + SerializeData value_data{value.begin(), value.end()}; + auto [it, inserted] = m_records.emplace(key_data, value_data); + if (!inserted && overwrite) { // Overwrite if requested + it->second = value_data; + inserted = true; + } + return inserted; +} + +bool MockableBatch::EraseKey(DataStream&& key) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + m_records.erase(key_data); + return true; +} + +bool MockableBatch::HasKey(DataStream&& key) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + return m_records.count(key_data) > 0; +} + +bool MockableBatch::ErasePrefix(Span prefix) +{ + if (!m_pass) { + return false; + } + auto it = m_records.begin(); + while (it != m_records.end()) { + auto& key = it->first; + if (key.size() < prefix.size() || std::search(key.begin(), key.end(), prefix.begin(), prefix.end()) != key.begin()) { + it++; + continue; + } + it = m_records.erase(it); + } + return true; +} + +std::unique_ptr CreateMockableWalletDatabase(std::map records) +{ + return std::make_unique(records); +} + +MockableDatabase& GetMockableDatabase(CWallet& wallet) +{ + return dynamic_cast(wallet.GetDatabase()); +} } // namespace wallet diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index 9621d043485e..8c0596122055 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -10,6 +10,8 @@ #endif #include