diff options
Diffstat (limited to 'src/wallet')
36 files changed, 1513 insertions, 347 deletions
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index cbf6c9b1ea..38cca32f80 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -887,7 +887,12 @@ bool BerkeleyBatch::HasKey(DataStream&& key) bool BerkeleyBatch::ErasePrefix(Span<const std::byte> prefix) { - if (!TxnBegin()) return false; + // Because this function erases records one by one, ensure that it is executed within a txn context. + // Otherwise, consistency is at risk; it's possible that certain records are removed while others + // remain due to an internal failure during the procedure. + // Additionally, the Dbc::del() cursor delete call below would fail without an active transaction. + if (!Assume(activeTxn)) return false; + auto cursor{std::make_unique<BerkeleyCursor>(m_database, *this)}; // const_cast is safe below even though prefix_key is an in/out parameter, // because we are not using the DB_DBT_USERMEM flag, so BDB will allocate @@ -901,7 +906,7 @@ bool BerkeleyBatch::ErasePrefix(Span<const std::byte> prefix) ret = cursor->dbc()->del(0); } cursor.reset(); - return TxnCommit() && (ret == 0 || ret == DB_NOTFOUND); + return ret == 0 || ret == DB_NOTFOUND; } void BerkeleyDatabase::AddRef() diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 391e120932..42615b5d42 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -25,14 +25,30 @@ static util::Result<SelectionResult> ErrorMaxWeightExceeded() "Please try sending a smaller amount or manually consolidating your wallet's UTXOs")}; } -// Descending order comparator +// Sort by descending (effective) value prefer lower waste on tie struct { bool operator()(const OutputGroup& a, const OutputGroup& b) const { + if (a.GetSelectionAmount() == b.GetSelectionAmount()) { + // Lower waste is better when effective_values are tied + return (a.fee - a.long_term_fee) < (b.fee - b.long_term_fee); + } return a.GetSelectionAmount() > b.GetSelectionAmount(); } } descending; +// Sort by descending (effective) value prefer lower weight on tie +struct { + bool operator()(const OutputGroup& a, const OutputGroup& b) const + { + if (a.GetSelectionAmount() == b.GetSelectionAmount()) { + // Sort lower weight to front on tied effective_value + return a.m_weight < b.m_weight; + } + return a.GetSelectionAmount() > b.GetSelectionAmount(); + } +} descending_effval_weight; + /* * This is the Branch and Bound Coin Selection algorithm designed by Murch. It searches for an input * set that can pay for the spending target and does not exceed the spending target by more than the @@ -68,6 +84,7 @@ struct { * bound of the range. * @param const CAmount& cost_of_change This is the cost of creating and spending a change output. * This plus selection_target is the upper bound of the range. + * @param int max_weight The maximum weight available for the input set. * @returns The result of this coin selection algorithm, or std::nullopt */ @@ -183,6 +200,331 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool return result; } +/* + * TL;DR: Coin Grinder is a DFS-based algorithm that deterministically searches for the minimum-weight input set to fund + * the transaction. The algorithm is similar to the Branch and Bound algorithm, but will produce a transaction _with_ a + * change output instead of a changeless transaction. + * + * Full description: CoinGrinder can be thought of as a graph walking algorithm. It explores a binary tree + * representation of the powerset of the UTXO pool. Each node in the tree represents a candidate input set. The tree’s + * root is the empty set. Each node in the tree has two children which are formed by either adding or skipping the next + * UTXO ("inclusion/omission branch"). Each level in the tree after the root corresponds to a decision about one UTXO in + * the UTXO pool. + * + * Example: + * We represent UTXOs as _alias=[effective_value/weight]_ and indicate omitted UTXOs with an underscore. Given a UTXO + * pool {A=[10/2], B=[7/1], C=[5/1], D=[4/2]} sorted by descending effective value, our search tree looks as follows: + * + * _______________________ {} ________________________ + * / \ + * A=[10/2] __________ {A} _________ __________ {_} _________ + * / \ / \ + * B=[7/1] {AB} _ {A_} _ {_B} _ {__} _ + * / \ / \ / \ / \ + * C=[5/1] {ABC} {AB_} {A_C} {A__} {_BC} {_B_} {__C} {___} + * / \ / \ / \ / \ / \ / \ / \ / \ + * D=[4/2] {ABCD} {ABC_} {AB_D} {AB__} {A_CD} {A_C_} {A__D} {A___} {_BCD} {_BC_} {_B_D} {_B__} {__CD} {__C_} {___D} {____} + * + * + * CoinGrinder uses a depth-first search to walk this tree. It first tries inclusion branches, then omission branches. A + * naive exploration of a tree with four UTXOs requires visiting all 31 nodes: + * + * {} {A} {AB} {ABC} {ABCD} {ABC_} {AB_} {AB_D} {AB__} {A_} {A_C} {A_CD} {A_C_} {A__} {A__D} {A___} {_} {_B} {_BC} + * {_BCD} {_BC_} {_B_} {_B_D} {_B__} {__} {__C} {__CD} {__C} {___} {___D} {____} + * + * As powersets grow exponentially with the set size, walking the entire tree would quickly get computationally + * infeasible with growing UTXO pools. Thanks to traversing the tree in a deterministic order, we can keep track of the + * progress of the search solely on basis of the current selection (and the best selection so far). We visit as few + * nodes as possible by recognizing and skipping any branches that can only contain solutions worse than the best + * solution so far. This makes CoinGrinder a branch-and-bound algorithm + * (https://en.wikipedia.org/wiki/Branch_and_bound). + * CoinGrinder is searching for the input set with lowest weight that can fund a transaction, so for example we can only + * ever find a _better_ candidate input set in a node that adds a UTXO, but never in a node that skips a UTXO. After + * visiting {A} and exploring the inclusion branch {AB} and its descendants, the candidate input set in the omission + * branch {A_} is equivalent to the parent {A} in effective value and weight. While CoinGrinder does need to visit the + * descendants of the omission branch {A_}, it is unnecessary to evaluate the candidate input set in the omission branch + * itself. By skipping evaluation of all nodes on an omission branch we reduce the visited nodes to 15: + * + * {A} {AB} {ABC} {ABCD} {AB_D} {A_C} {A_CD} {A__D} {_B} {_BC} {_BCD} {_B_D} {__C} {__CD} {___D} + * + * _______________________ {} ________________________ + * / \ + * A=[10/2] __________ {A} _________ ___________\____________ + * / \ / \ + * B=[7/1] {AB} __ __\_____ {_B} __ __\_____ + * / \ / \ / \ / \ + * C=[5/1] {ABC} \ {A_C} \ {_BC} \ {__C} \ + * / / / / / / / / + * D=[4/2] {ABCD} {AB_D} {A_CD} {A__D} {_BCD} {_B_D} {__CD} {___D} + * + * + * We refer to the move from the inclusion branch {AB} via the omission branch {A_} to its inclusion-branch child {A_C} + * as _shifting to the omission branch_ or just _SHIFT_. (The index of the ultimate element in the candidate input set + * shifts right by one: {AB} ⇒ {A_C}.) + * When we reach a leaf node in the last level of the tree, shifting to the omission branch is not possible. Instead we + * go to the omission branch of the node’s last ancestor on an inclusion branch: from {ABCD}, we go to {AB_D}. From + * {AB_D}, we go to {A_C}. We refer to this operation as a _CUT_. (The ultimate element in + * the input set is deselected, and the penultimate element is shifted right by one: {AB_D} ⇒ {A_C}.) + * If a candidate input set in a node has not selected sufficient funds to build the transaction, we continue directly + * along the next inclusion branch. We call this operation _EXPLORE_. (We go from one inclusion branch to the next + * inclusion branch: {_B} ⇒ {_BC}.) + * Further, any prefix that already has selected sufficient effective value to fund the transaction cannot be improved + * by adding more UTXOs. If for example the candidate input set in {AB} is a valid solution, all potential descendant + * solutions {ABC}, {ABCD}, and {AB_D} must have a higher weight, thus instead of exploring the descendants of {AB}, we + * can SHIFT from {AB} to {A_C}. + * + * Given the above UTXO set, using a target of 11, and following these initial observations, the basic implementation of + * CoinGrinder visits the following 10 nodes: + * + * Node [eff_val/weight] Evaluation + * --------------------------------------------------------------- + * {A} [10/2] Insufficient funds: EXPLORE + * {AB} [17/3] Solution: SHIFT to omission branch + * {A_C} [15/3] Better solution: SHIFT to omission branch + * {A__D} [14/4] Worse solution, shift impossible due to leaf node: CUT to omission branch of {A__D}, + * i.e. SHIFT to omission branch of {A} + * {_B} [7/1] Insufficient funds: EXPLORE + * {_BC} [12/2] Better solution: SHIFT to omission branch + * {_B_D} [11/3] Worse solution, shift impossible due to leaf node: CUT to omission branch of {_B_D}, + * i.e. SHIFT to omission branch of {_B} + * {__C} [5/1] Insufficient funds: EXPLORE + * {__CD} [9/3] Insufficient funds, leaf node: CUT + * {___D} [4/2] Insufficient funds, leaf node, cannot CUT since only one UTXO selected: done. + * + * _______________________ {} ________________________ + * / \ + * A=[10/2] __________ {A} _________ ___________\____________ + * / \ / \ + * B=[7/1] {AB} __\_____ {_B} __ __\_____ + * / \ / \ / \ + * C=[5/1] {A_C} \ {_BC} \ {__C} \ + * / / / / + * D=[4/2] {A__D} {_B_D} {__CD} {___D} + * + * + * We implement this tree walk in the following algorithm: + * 1. Add `next_utxo` + * 2. Evaluate candidate input set + * 3. Determine `next_utxo` by deciding whether to + * a) EXPLORE: Add next inclusion branch, e.g. {_B} ⇒ {_B} + `next_uxto`: C + * b) SHIFT: Replace last selected UTXO by next higher index, e.g. {A_C} ⇒ {A__} + `next_utxo`: D + * c) CUT: deselect last selected UTXO and shift to omission branch of penultimate UTXO, e.g. {AB_D} ⇒ {A_} + `next_utxo: C + * + * The implementation then adds further optimizations by discovering further situations in which either the inclusion + * branch can be skipped, or both the inclusion and omission branch can be skipped after evaluating the candidate input + * set in the node. + * + * @param std::vector<OutputGroup>& utxo_pool The UTXOs that we are choosing from. These UTXOs will be sorted in + * descending order by effective value, with lower weight preferred as a tie-breaker. (We can think of an output + * group with multiple as a heavier UTXO with the combined amount here.) + * @param const CAmount& selection_target This is the minimum amount that we need for the transaction without considering change. + * @param const CAmount& change_target The minimum budget for creating a change output, by which we increase the selection_target. + * @param int max_weight The maximum permitted weight for the input set. + * @returns The result of this coin selection algorithm, or std::nullopt + */ +util::Result<SelectionResult> CoinGrinder(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_weight) +{ + std::sort(utxo_pool.begin(), utxo_pool.end(), descending_effval_weight); + // The sum of UTXO amounts after this UTXO index, e.g. lookahead[5] = Σ(UTXO[6+].amount) + std::vector<CAmount> lookahead(utxo_pool.size()); + // The minimum UTXO weight among the remaining UTXOs after this UTXO index, e.g. min_tail_weight[5] = min(UTXO[6+].weight) + std::vector<int> min_tail_weight(utxo_pool.size()); + + // Calculate lookahead values, min_tail_weights, and check that there are sufficient funds + CAmount total_available = 0; + int min_group_weight = std::numeric_limits<int>::max(); + for (size_t i = 0; i < utxo_pool.size(); ++i) { + size_t index = utxo_pool.size() - 1 - i; // Loop over every element in reverse order + lookahead[index] = total_available; + min_tail_weight[index] = min_group_weight; + // UTXOs with non-positive effective value must have been filtered + Assume(utxo_pool[index].GetSelectionAmount() > 0); + total_available += utxo_pool[index].GetSelectionAmount(); + min_group_weight = std::min(min_group_weight, utxo_pool[index].m_weight); + } + + const CAmount total_target = selection_target + change_target; + if (total_available < total_target) { + // Insufficient funds + return util::Error(); + } + + // The current selection and the best input set found so far, stored as the utxo_pool indices of the UTXOs forming them + std::vector<size_t> curr_selection; + std::vector<size_t> best_selection; + + // The currently selected effective amount, and the effective amount of the best selection so far + CAmount curr_amount = 0; + CAmount best_selection_amount = MAX_MONEY; + + // The weight of the currently selected input set, and the weight of the best selection + int curr_weight = 0; + int best_selection_weight = max_weight; // Tie is fine, because we prefer lower selection amount + + // Whether the input sets generated during this search have exceeded the maximum transaction weight at any point + bool max_tx_weight_exceeded = false; + + // Index of the next UTXO to consider in utxo_pool + size_t next_utxo = 0; + + /* + * You can think of the current selection as a vector of booleans that has decided inclusion or exclusion of all + * UTXOs before `next_utxo`. When we consider the next UTXO, we extend this hypothetical boolean vector either with + * a true value if the UTXO is included or a false value if it is omitted. The equivalent state is stored more + * compactly as the list of indices of the included UTXOs and the `next_utxo` index. + * + * We can never find a new solution by deselecting a UTXO, because we then revisit a previously evaluated + * selection. Therefore, we only need to check whether we found a new solution _after adding_ a new UTXO. + * + * Each iteration of CoinGrinder starts by selecting the `next_utxo` and evaluating the current selection. We + * use three state transitions to progress from the current selection to the next promising selection: + * + * - EXPLORE inclusion branch: We do not have sufficient funds, yet. Add `next_utxo` to the current selection, then + * nominate the direct successor of the just selected UTXO as our `next_utxo` for the + * following iteration. + * + * Example: + * Current Selection: {0, 5, 7} + * Evaluation: EXPLORE, next_utxo: 8 + * Next Selection: {0, 5, 7, 8} + * + * - SHIFT to omission branch: Adding more UTXOs to the current selection cannot produce a solution that is better + * than the current best, e.g. the current selection weight exceeds the max weight or + * the current selection amount is equal to or greater than the target. + * We designate our `next_utxo` the one after the tail of our current selection, then + * deselect the tail of our current selection. + * + * Example: + * Current Selection: {0, 5, 7} + * Evaluation: SHIFT, next_utxo: 8, omit last selected: {0, 5} + * Next Selection: {0, 5, 8} + * + * - CUT entire subtree: We have exhausted the inclusion branch for the penultimately selected UTXO, both the + * inclusion and the omission branch of the current prefix are barren. E.g. we have + * reached the end of the UTXO pool, so neither further EXPLORING nor SHIFTING can find + * any solutions. We designate our `next_utxo` the one after our penultimate selected, + * then deselect both the last and penultimate selected. + * + * Example: + * Current Selection: {0, 5, 7} + * Evaluation: CUT, next_utxo: 6, omit two last selected: {0} + * Next Selection: {0, 6} + */ + auto deselect_last = [&]() { + OutputGroup& utxo = utxo_pool[curr_selection.back()]; + curr_amount -= utxo.GetSelectionAmount(); + curr_weight -= utxo.m_weight; + curr_selection.pop_back(); + }; + + SelectionResult result(selection_target, SelectionAlgorithm::CG); + bool is_done = false; + size_t curr_try = 0; + while (!is_done) { + bool should_shift{false}, should_cut{false}; + // Select `next_utxo` + OutputGroup& utxo = utxo_pool[next_utxo]; + curr_amount += utxo.GetSelectionAmount(); + curr_weight += utxo.m_weight; + curr_selection.push_back(next_utxo); + ++next_utxo; + ++curr_try; + + // EVALUATE current selection: check for solutions and see whether we can CUT or SHIFT before EXPLORING further + auto curr_tail = curr_selection.back(); + if (curr_amount + lookahead[curr_tail] < total_target) { + // Insufficient funds with lookahead: CUT + should_cut = true; + } else if (curr_weight > best_selection_weight) { + // best_selection_weight is initialized to max_weight + if (curr_weight > max_weight) max_tx_weight_exceeded = true; + // Worse weight than best solution. More UTXOs only increase weight: + // CUT if last selected group had minimal weight, else SHIFT + if (utxo_pool[curr_tail].m_weight <= min_tail_weight[curr_tail]) { + should_cut = true; + } else { + should_shift = true; + } + } else if (curr_amount >= total_target) { + // Success, adding more weight cannot be better: SHIFT + should_shift = true; + if (curr_weight < best_selection_weight || (curr_weight == best_selection_weight && curr_amount < best_selection_amount)) { + // New lowest weight, or same weight with fewer funds tied up + best_selection = curr_selection; + best_selection_weight = curr_weight; + best_selection_amount = curr_amount; + } + } else if (!best_selection.empty() && curr_weight + int64_t{min_tail_weight[curr_tail]} * ((total_target - curr_amount + utxo_pool[curr_tail].GetSelectionAmount() - 1) / utxo_pool[curr_tail].GetSelectionAmount()) > best_selection_weight) { + // Compare minimal tail weight and last selected amount with the amount missing to gauge whether a better weight is still possible. + if (utxo_pool[curr_tail].m_weight <= min_tail_weight[curr_tail]) { + should_cut = true; + } else { + should_shift = true; + } + } + + if (curr_try >= TOTAL_TRIES) { + // Solution is not guaranteed to be optimal if `curr_try` hit TOTAL_TRIES + result.SetAlgoCompleted(false); + break; + } + + if (next_utxo == utxo_pool.size()) { + // Last added UTXO was end of UTXO pool, nothing left to add on inclusion or omission branch: CUT + should_cut = true; + } + + if (should_cut) { + // Neither adding to the current selection nor exploring the omission branch of the last selected UTXO can + // find any solutions. Redirect to exploring the Omission branch of the penultimate selected UTXO (i.e. + // set `next_utxo` to one after the penultimate selected, then deselect the last two selected UTXOs) + should_cut = false; + deselect_last(); + should_shift = true; + } + + while (should_shift) { + // Set `next_utxo` to one after last selected, then deselect last selected UTXO + if (curr_selection.empty()) { + // Exhausted search space before running into attempt limit + is_done = true; + result.SetAlgoCompleted(true); + break; + } + next_utxo = curr_selection.back() + 1; + deselect_last(); + should_shift = false; + + // After SHIFTing to an omission branch, the `next_utxo` might have the same effective value as the UTXO we + // just omitted. Since lower weight is our tiebreaker on UTXOs with equal effective value for sorting, if it + // ties on the effective value, it _must_ have the same weight (i.e. be a "clone" of the prior UTXO) or a + // higher weight. If so, selecting `next_utxo` would produce an equivalent or worse selection as one we + // previously evaluated. In that case, increment `next_utxo` until we find a UTXO with a differing amount. + while (utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_pool[next_utxo].GetSelectionAmount()) { + if (next_utxo >= utxo_pool.size() - 1) { + // Reached end of UTXO pool skipping clones: SHIFT instead + should_shift = true; + break; + } + // Skip clone: previous UTXO is equivalent and unselected + ++next_utxo; + } + } + } + + result.SetSelectionsEvaluated(curr_try); + + if (best_selection.empty()) { + return max_tx_weight_exceeded ? ErrorMaxWeightExceeded() : util::Error(); + } + + for (const size_t& i : best_selection) { + result.AddInput(utxo_pool[i]); + } + + return result; +} + class MinOutputGroupComparator { public: @@ -509,6 +851,26 @@ void SelectionResult::ComputeAndSetWaste(const CAmount min_viable_change, const } } +void SelectionResult::SetAlgoCompleted(bool algo_completed) +{ + m_algo_completed = algo_completed; +} + +bool SelectionResult::GetAlgoCompleted() const +{ + return m_algo_completed; +} + +void SelectionResult::SetSelectionsEvaluated(size_t attempts) +{ + m_selections_evaluated = attempts; +} + +size_t SelectionResult::GetSelectionsEvaluated() const +{ + return m_selections_evaluated; +} + CAmount SelectionResult::GetWaste() const { return *Assert(m_waste); @@ -602,6 +964,7 @@ std::string GetAlgorithmName(const SelectionAlgorithm algo) case SelectionAlgorithm::BNB: return "bnb"; case SelectionAlgorithm::KNAPSACK: return "knapsack"; case SelectionAlgorithm::SRD: return "srd"; + case SelectionAlgorithm::CG: return "cg"; case SelectionAlgorithm::MANUAL: return "manual"; // No default case to allow for compiler to warn } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 20b2461c04..80c92e1b0e 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -142,8 +142,8 @@ struct CoinSelectionParams { size_t change_output_size = 0; /** Size of the input to spend a change output in virtual bytes. */ size_t change_spend_size = 0; - /** Mininmum change to target in Knapsack solver: select coins to cover the payment and - * at least this value of change. */ + /** Mininmum change to target in Knapsack solver and CoinGrinder: + * select coins to cover the payment and at least this value of change. */ CAmount m_min_change_target{0}; /** Minimum amount for creating a change output. * If change budget is smaller than min_change then we forgo creation of change output. @@ -311,7 +311,8 @@ enum class SelectionAlgorithm : uint8_t BNB = 0, KNAPSACK = 1, SRD = 2, - MANUAL = 3, + CG = 3, + MANUAL = 4, }; std::string GetAlgorithmName(const SelectionAlgorithm algo); @@ -329,6 +330,10 @@ private: bool m_use_effective{false}; /** The computed waste */ std::optional<CAmount> m_waste; + /** False if algorithm was cut short by hitting limit of attempts and solution is non-optimal */ + bool m_algo_completed{true}; + /** The count of selections that were evaluated by this coin selection attempt */ + size_t m_selections_evaluated; /** Total weight of the selected inputs */ int m_weight{0}; /** How much individual inputs overestimated the bump fees for the shared ancestry */ @@ -386,6 +391,18 @@ public: void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); [[nodiscard]] CAmount GetWaste() const; + /** Tracks that algorithm was able to exhaustively search the entire combination space before hitting limit of tries */ + void SetAlgoCompleted(bool algo_completed); + + /** Get m_algo_completed */ + bool GetAlgoCompleted() const; + + /** Record the number of selections that were evaluated */ + void SetSelectionsEvaluated(size_t attempts); + + /** Get selections_evaluated */ + size_t GetSelectionsEvaluated() const ; + /** * Combines the @param[in] other selection result into 'this' selection result. * @@ -430,6 +447,8 @@ public: util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, int max_weight); +util::Result<SelectionResult> CoinGrinder(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_weight); + /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible * outputs until the target is satisfied * diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index a71f8f9fbc..b5703fa54a 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -9,9 +9,11 @@ #include <wallet/external_signer_scriptpubkeyman.h> #include <iostream> +#include <key_io.h> #include <memory> #include <stdexcept> #include <string> +#include <univalue.h> #include <utility> #include <vector> @@ -51,15 +53,26 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() { return signers[0]; } -bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const +util::Result<void> ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const { // TODO: avoid the need to infer a descriptor from inside a descriptor wallet + const CScript& scriptPubKey = GetScriptForDestination(dest); auto provider = GetSolvingProvider(scriptPubKey); auto descriptor = InferDescriptor(scriptPubKey, *provider); - signer.DisplayAddress(descriptor->ToString()); - // TODO inspect result - return true; + const UniValue& result = signer.DisplayAddress(descriptor->ToString()); + + const UniValue& error = result.find_value("error"); + if (error.isStr()) return util::Error{strprintf(_("Signer returned error: %s"), error.getValStr())}; + + const UniValue& ret_address = result.find_value("address"); + if (!ret_address.isStr()) return util::Error{_("Signer did not echo address")}; + + if (ret_address.getValStr() != EncodeDestination(dest)) { + return util::Error{strprintf(_("Signer echoed unexpected address %s"), ret_address.getValStr())}; + } + + return util::Result<void>(); } // If sign is true, transaction must previously have been filled diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index c052ce6129..44286456b6 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -9,6 +9,8 @@ #include <memory> +struct bilingual_str; + namespace wallet { class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { @@ -27,7 +29,11 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan static ExternalSigner GetExternalSigner(); - bool DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const; + /** + * Display address on the device and verify that the returned value matches. + * @returns nothing or an error message + */ + util::Result<void> DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const; TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; }; diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 088343458c..14e988ec1a 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -3,6 +3,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <common/args.h> #include <init.h> #include <interfaces/chain.h> diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d15273dfc9..0c1cae7253 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -92,7 +92,7 @@ WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx) WalletTxStatus result; result.block_height = wtx.state<TxStateConfirmed>() ? wtx.state<TxStateConfirmed>()->confirmed_block_height : - wtx.state<TxStateConflicted>() ? wtx.state<TxStateConflicted>()->conflicting_block_height : + wtx.state<TxStateBlockConflicted>() ? wtx.state<TxStateBlockConflicted>()->conflicting_block_height : std::numeric_limits<int>::max(); result.blocks_to_maturity = wallet.GetTxBlocksToMaturity(wtx); result.depth_in_main_chain = wallet.GetTxDepthInMainChain(wtx); @@ -101,7 +101,7 @@ WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx) result.is_trusted = CachedTxIsTrusted(wallet, wtx); result.is_abandoned = wtx.isAbandoned(); result.is_coinbase = wtx.IsCoinBase(); - result.is_in_main_chain = wallet.IsTxInMainChain(wtx); + result.is_in_main_chain = wtx.isConfirmed(); return result; } @@ -247,7 +247,7 @@ public: return value.empty() ? m_wallet->EraseAddressReceiveRequest(batch, dest, id) : m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } - bool displayAddress(const CTxDestination& dest) override + util::Result<void> displayAddress(const CTxDestination& dest) override { LOCK(m_wallet->cs_wallet); return m_wallet->DisplayAddress(dest); @@ -286,7 +286,7 @@ public: if (!res) return util::Error{util::ErrorString(res)}; const auto& txr = *res; fee = txr.fee; - change_pos = txr.change_pos ? *txr.change_pos : -1; + change_pos = txr.change_pos ? int(*txr.change_pos) : -1; return txr.tx; } diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index b9d8d9abc9..c164266f80 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -149,7 +149,7 @@ CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, c { AssertLockHeld(wallet.cs_wallet); - if (wallet.IsTxImmatureCoinBase(wtx) && wallet.IsTxInMainChain(wtx)) { + if (wallet.IsTxImmatureCoinBase(wtx) && wtx.isConfirmed()) { return GetCachableAmount(wallet, wtx, CWalletTx::IMMATURE_CREDIT, filter); } @@ -253,12 +253,12 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef return (CachedTxGetDebit(wallet, wtx, filter) > 0); } +// NOLINTNEXTLINE(misc-no-recursion) bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents) { AssertLockHeld(wallet.cs_wallet); - int nDepth = wallet.GetTxDepthInMainChain(wtx); - if (nDepth >= 1) return true; - if (nDepth < 0) return false; + if (wtx.isConfirmed()) return true; + if (wtx.isBlockConflicted()) return false; // using wtx's cached debit if (!wallet.m_spend_zero_conf_change || !CachedTxIsFromMe(wallet, wtx, ISMINE_ALL)) return false; diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index e9b93afc30..65587f0b18 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -2,6 +2,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <core_io.h> #include <key_io.h> #include <rpc/util.h> @@ -391,6 +393,7 @@ class DescribeWalletAddressVisitor public: const SigningProvider * const provider; + // NOLINTNEXTLINE(misc-no-recursion) void ProcessSubScript(const CScript& subscript, UniValue& obj) const { // Always present: script type and redeemscript @@ -441,6 +444,7 @@ public: return obj; } + // NOLINTNEXTLINE(misc-no-recursion) UniValue operator()(const ScriptHash& scripthash) const { UniValue obj(UniValue::VOBJ); @@ -461,6 +465,7 @@ public: return obj; } + // NOLINTNEXTLINE(misc-no-recursion) UniValue operator()(const WitnessV0ScriptHash& id) const { UniValue obj(UniValue::VOBJ); @@ -782,9 +787,8 @@ RPCHelpMan walletdisplayaddress() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); } - if (!pwallet->DisplayAddress(dest)) { - throw JSONRPCError(RPC_MISC_ERROR, "Failed to display address"); - } + util::Result<void> res = pwallet->DisplayAddress(dest); + if (!res) throw JSONRPCError(RPC_MISC_ERROR, util::ErrorString(res).original); UniValue result(UniValue::VOBJ); result.pushKV("address", request.params[0].get_str()); diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index d17a220603..8d3eea59ee 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -2,6 +2,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <chain.h> #include <clientversion.h> #include <core_io.h> @@ -394,14 +396,8 @@ RPCHelpMan removeprunedfunds() uint256 hash(ParseHashV(request.params[0], "txid")); std::vector<uint256> vHash; vHash.push_back(hash); - std::vector<uint256> vHashOut; - - if (pwallet->ZapSelectTx(vHash, vHashOut) != DBErrors::LOAD_OK) { - throw JSONRPCError(RPC_WALLET_ERROR, "Could not properly delete the transaction."); - } - - if(vHashOut.empty()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction does not exist in wallet."); + if (auto res = pwallet->RemoveTxs(vHash); !res) { + throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original); } return UniValue::VNULL; @@ -851,6 +847,7 @@ enum class ScriptContext // Analyse the provided scriptPubKey, determining which keys and which redeem scripts from the ImportData struct are needed to spend it, and mark them as used. // Returns an error string, or the empty string for success. +// NOLINTNEXTLINE(misc-no-recursion) static std::string RecurseImportData(const CScript& script, ImportData& import_data, const ScriptContext script_ctx) { // Use Solver to obtain script type and parsed pubkeys or hashes: diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 0cb0891141..b6c7396f4b 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -194,15 +194,12 @@ RPCHelpMan getbalance() LOCK(pwallet->cs_wallet); - const auto dummy_value{self.MaybeArg<std::string>(0)}; + const auto dummy_value{self.MaybeArg<std::string>("dummy")}; if (dummy_value && *dummy_value != "*") { throw JSONRPCError(RPC_METHOD_DEPRECATED, "dummy first argument must be excluded or set to \"*\"."); } - int min_depth = 0; - if (!request.params[1].isNull()) { - min_depth = request.params[1].getInt<int>(); - } + const auto min_depth{self.Arg<int>("minconf")}; bool include_watchonly = ParseIncludeWatchonly(request.params[2], *pwallet); diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index e6c021d426..05b340995d 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -40,6 +40,10 @@ static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue for (const uint256& conflict : wallet.GetTxConflicts(wtx)) conflicts.push_back(conflict.GetHex()); entry.pushKV("walletconflicts", conflicts); + UniValue mempool_conflicts(UniValue::VARR); + for (const Txid& mempool_conflict : wtx.mempool_conflicts) + mempool_conflicts.push_back(mempool_conflict.GetHex()); + entry.pushKV("mempoolconflicts", mempool_conflicts); entry.pushKV("time", wtx.GetTxTime()); entry.pushKV("timereceived", int64_t{wtx.nTimeReceived}); @@ -417,6 +421,10 @@ static std::vector<RPCResult> TransactionDescriptionString() }}, {RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx was replaced."}, {RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx replaces another."}, + {RPCResult::Type::ARR, "mempoolconflicts", "Transactions that directly conflict with either this transaction or an ancestor transaction", + { + {RPCResult::Type::STR_HEX, "txid", "The transaction id."}, + }}, {RPCResult::Type::STR, "to", /*optional=*/true, "If a comment to is associated with the transaction."}, {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."}, {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."}, diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 06ec7db1bc..1252843e9d 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -11,6 +11,7 @@ #include <wallet/context.h> #include <wallet/wallet.h> +#include <string_view> #include <univalue.h> #include <boost/date_time/posix_time/posix_time.hpp> @@ -61,9 +62,9 @@ bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wal bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name) { - if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) { + if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) { // wallet endpoint was used - wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size())); + wallet_name = UrlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); return true; } return false; diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 391153a2a1..f1cb595271 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -3,6 +3,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <core_io.h> #include <key_io.h> #include <rpc/server.h> @@ -748,13 +750,13 @@ RPCHelpMan simulaterawtransaction() static RPCHelpMan migratewallet() { return RPCHelpMan{"migratewallet", - "EXPERIMENTAL warning: This call may not work as expected and may be changed in future releases\n" "\nMigrate the wallet to a descriptor wallet.\n" "A new wallet backup will need to be made.\n" "\nThe migration process will create a backup of the wallet before migrating. This backup\n" "file will be named <wallet name>-<timestamp>.legacy.bak and can be found in the directory\n" "for this wallet. In the event of an incorrect migration, the backup can be restored using restorewallet." - "\nEncrypted wallets must have the passphrase provided as an argument to this call.", + "\nEncrypted wallets must have the passphrase provided as an argument to this call.\n" + "\nThis RPC may take a long time to complete. Increasing the RPC client timeout is recommended.", { {"wallet_name", RPCArg::Type::STR, RPCArg::DefaultHint{"the wallet name from the RPC endpoint"}, "The name of the wallet to migrate. If provided both here and in the RPC endpoint, the two must be identical."}, {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The wallet passphrase"}, @@ -813,6 +815,217 @@ static RPCHelpMan migratewallet() }; } +RPCHelpMan gethdkeys() +{ + return RPCHelpMan{ + "gethdkeys", + "\nList all BIP 32 HD keys in the wallet and which descriptors use them.\n", + { + {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", { + {"active_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show the keys for only active descriptors"}, + {"private", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show private keys"} + }}, + }, + RPCResult{RPCResult::Type::ARR, "", "", { + { + {RPCResult::Type::OBJ, "", "", { + {RPCResult::Type::STR, "xpub", "The extended public key"}, + {RPCResult::Type::BOOL, "has_private", "Whether the wallet has the private key for this xpub"}, + {RPCResult::Type::STR, "xprv", /*optional=*/true, "The extended private key if \"private\" is true"}, + {RPCResult::Type::ARR, "descriptors", "Array of descriptor objects that use this HD key", + { + {RPCResult::Type::OBJ, "", "", { + {RPCResult::Type::STR, "desc", "Descriptor string representation"}, + {RPCResult::Type::BOOL, "active", "Whether this descriptor is currently used to generate new addresses"}, + }}, + }}, + }}, + } + }}, + RPCExamples{ + HelpExampleCli("gethdkeys", "") + HelpExampleRpc("gethdkeys", "") + + HelpExampleCliNamed("gethdkeys", {{"active_only", "true"}, {"private", "true"}}) + HelpExampleRpcNamed("gethdkeys", {{"active_only", "true"}, {"private", "true"}}) + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue + { + const std::shared_ptr<const CWallet> wallet = GetWalletForJSONRPCRequest(request); + if (!wallet) return UniValue::VNULL; + + if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "gethdkeys is not available for non-descriptor wallets"); + } + + LOCK(wallet->cs_wallet); + + UniValue options{request.params[0].isNull() ? UniValue::VOBJ : request.params[0]}; + const bool active_only{options.exists("active_only") ? options["active_only"].get_bool() : false}; + const bool priv{options.exists("private") ? options["private"].get_bool() : false}; + if (priv) { + EnsureWalletIsUnlocked(*wallet); + } + + + std::set<ScriptPubKeyMan*> spkms; + if (active_only) { + spkms = wallet->GetActiveScriptPubKeyMans(); + } else { + spkms = wallet->GetAllScriptPubKeyMans(); + } + + std::map<CExtPubKey, std::set<std::tuple<std::string, bool, bool>>> wallet_xpubs; + std::map<CExtPubKey, CExtKey> wallet_xprvs; + for (auto* spkm : spkms) { + auto* desc_spkm{dynamic_cast<DescriptorScriptPubKeyMan*>(spkm)}; + CHECK_NONFATAL(desc_spkm); + LOCK(desc_spkm->cs_desc_man); + WalletDescriptor w_desc = desc_spkm->GetWalletDescriptor(); + + // Retrieve the pubkeys from the descriptor + std::set<CPubKey> desc_pubkeys; + std::set<CExtPubKey> desc_xpubs; + w_desc.descriptor->GetPubKeys(desc_pubkeys, desc_xpubs); + for (const CExtPubKey& xpub : desc_xpubs) { + std::string desc_str; + bool ok = desc_spkm->GetDescriptorString(desc_str, false); + CHECK_NONFATAL(ok); + wallet_xpubs[xpub].emplace(desc_str, wallet->IsActiveScriptPubKeyMan(*spkm), desc_spkm->HasPrivKey(xpub.pubkey.GetID())); + if (std::optional<CKey> key = priv ? desc_spkm->GetKey(xpub.pubkey.GetID()) : std::nullopt) { + wallet_xprvs[xpub] = CExtKey(xpub, *key); + } + } + } + + UniValue response(UniValue::VARR); + for (const auto& [xpub, descs] : wallet_xpubs) { + bool has_xprv = false; + UniValue descriptors(UniValue::VARR); + for (const auto& [desc, active, has_priv] : descs) { + UniValue d(UniValue::VOBJ); + d.pushKV("desc", desc); + d.pushKV("active", active); + has_xprv |= has_priv; + + descriptors.push_back(std::move(d)); + } + UniValue xpub_info(UniValue::VOBJ); + xpub_info.pushKV("xpub", EncodeExtPubKey(xpub)); + xpub_info.pushKV("has_private", has_xprv); + if (priv) { + xpub_info.pushKV("xprv", EncodeExtKey(wallet_xprvs.at(xpub))); + } + xpub_info.pushKV("descriptors", std::move(descriptors)); + + response.push_back(std::move(xpub_info)); + } + + return response; + }, + }; +} + +static RPCHelpMan createwalletdescriptor() +{ + return RPCHelpMan{"createwalletdescriptor", + "Creates the wallet's descriptor for the given address type. " + "The address type must be one that the wallet does not already have a descriptor for." + + HELP_REQUIRING_PASSPHRASE, + { + {"type", RPCArg::Type::STR, RPCArg::Optional::NO, "The address type the descriptor will produce. Options are \"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"."}, + {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", { + {"internal", RPCArg::Type::BOOL, RPCArg::DefaultHint{"Both external and internal will be generated unless this parameter is specified"}, "Whether to only make one descriptor that is internal (if parameter is true) or external (if parameter is false)"}, + {"hdkey", RPCArg::Type::STR, RPCArg::DefaultHint{"The HD key used by all other active descriptors"}, "The HD key that the wallet knows the private key of, listed using 'gethdkeys', to use for this descriptor's key"}, + }}, + }, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + {RPCResult::Type::ARR, "descs", "The public descriptors that were added to the wallet", + {{RPCResult::Type::STR, "", ""}} + } + }, + }, + RPCExamples{ + HelpExampleCli("createwalletdescriptor", "bech32m") + + HelpExampleRpc("createwalletdescriptor", "bech32m") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue + { + std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request); + if (!pwallet) return UniValue::VNULL; + + // Make sure wallet is a descriptor wallet + if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "createwalletdescriptor is not available for non-descriptor wallets"); + } + + std::optional<OutputType> output_type = ParseOutputType(request.params[0].get_str()); + if (!output_type) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str())); + } + + UniValue options{request.params[1].isNull() ? UniValue::VOBJ : request.params[1]}; + UniValue internal_only{options["internal"]}; + UniValue hdkey{options["hdkey"]}; + + std::vector<bool> internals; + if (internal_only.isNull()) { + internals.push_back(false); + internals.push_back(true); + } else { + internals.push_back(internal_only.get_bool()); + } + + LOCK(pwallet->cs_wallet); + EnsureWalletIsUnlocked(*pwallet); + + CExtPubKey xpub; + if (hdkey.isNull()) { + std::set<CExtPubKey> active_xpubs = pwallet->GetActiveHDPubKeys(); + if (active_xpubs.size() != 1) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to determine which HD key to use from active descriptors. Please specify with 'hdkey'"); + } + xpub = *active_xpubs.begin(); + } else { + xpub = DecodeExtPubKey(hdkey.get_str()); + if (!xpub.pubkey.IsValid()) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to parse HD key. Please provide a valid xpub"); + } + } + + std::optional<CKey> key = pwallet->GetKey(xpub.pubkey.GetID()); + if (!key) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Private key for %s is not known", EncodeExtPubKey(xpub))); + } + CExtKey active_hdkey(xpub, *key); + + std::vector<std::reference_wrapper<DescriptorScriptPubKeyMan>> spkms; + WalletBatch batch{pwallet->GetDatabase()}; + for (bool internal : internals) { + WalletDescriptor w_desc = GenerateWalletDescriptor(xpub, *output_type, internal); + uint256 w_id = DescriptorID(*w_desc.descriptor); + if (!pwallet->GetScriptPubKeyMan(w_id)) { + spkms.emplace_back(pwallet->SetupDescriptorScriptPubKeyMan(batch, active_hdkey, *output_type, internal)); + } + } + if (spkms.empty()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Descriptor already exists"); + } + + // Fetch each descspkm from the wallet in order to get the descriptor strings + UniValue descs{UniValue::VARR}; + for (const auto& spkm : spkms) { + std::string desc_str; + bool ok = spkm.get().GetDescriptorString(desc_str, false); + CHECK_NONFATAL(ok); + descs.push_back(desc_str); + } + UniValue out{UniValue::VOBJ}; + out.pushKV("descs", std::move(descs)); + return out; + } + }; +} + // addresses RPCHelpMan getaddressinfo(); RPCHelpMan getnewaddress(); @@ -896,6 +1109,7 @@ Span<const CRPCCommand> GetWalletRPCCommands() {"wallet", &bumpfee}, {"wallet", &psbtbumpfee}, {"wallet", &createwallet}, + {"wallet", &createwalletdescriptor}, {"wallet", &restorewallet}, {"wallet", &dumpprivkey}, {"wallet", &dumpwallet}, @@ -903,6 +1117,7 @@ Span<const CRPCCommand> GetWalletRPCCommands() {"wallet", &getaddressesbylabel}, {"wallet", &getaddressinfo}, {"wallet", &getbalance}, + {"wallet", &gethdkeys}, {"wallet", &getnewaddress}, {"wallet", &getrawchangeaddress}, {"wallet", &getreceivedbyaddress}, diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index e07771b17f..b42275fe4b 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -11,6 +11,7 @@ #include <script/sign.h> #include <script/solver.h> #include <util/bip32.h> +#include <util/check.h> #include <util/strencodings.h> #include <util/string.h> #include <util/time.h> @@ -96,6 +97,7 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyScriptPubKeyMan& //! @param recurse_scripthash whether to recurse into nested p2sh and p2wsh //! scripts or simply treat any script that has been //! stored in the keystore as spendable +// NOLINTNEXTLINE(misc-no-recursion) IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true) { IsMineResult ret = IsMineResult::NO; @@ -1288,6 +1290,9 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize) } if (!batch.TxnCommit()) throw std::runtime_error(strprintf("Error during keypool top up. Cannot commit changes for wallet %s", m_storage.GetDisplayName())); NotifyCanGetAddressesChanged(); + // Note: Unlike with DescriptorSPKM, LegacySPKM does not need to call + // m_storage.TopUpCallback() as we do not know what new scripts the LegacySPKM is + // watching for. CWallet's scriptPubKey cache is not used for LegacySPKMs. return true; } @@ -2140,6 +2145,36 @@ std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const return m_map_keys; } +bool DescriptorScriptPubKeyMan::HasPrivKey(const CKeyID& keyid) const +{ + AssertLockHeld(cs_desc_man); + return m_map_keys.contains(keyid) || m_map_crypted_keys.contains(keyid); +} + +std::optional<CKey> DescriptorScriptPubKeyMan::GetKey(const CKeyID& keyid) const +{ + AssertLockHeld(cs_desc_man); + if (m_storage.HasEncryptionKeys() && !m_storage.IsLocked()) { + const auto& it = m_map_crypted_keys.find(keyid); + if (it == m_map_crypted_keys.end()) { + return std::nullopt; + } + const std::vector<unsigned char>& crypted_secret = it->second.second; + CKey key; + if (!Assume(m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, crypted_secret, it->second.first, key); + }))) { + return std::nullopt; + } + return key; + } + const auto& it = m_map_keys.find(keyid); + if (it == m_map_keys.end()) { + return std::nullopt; + } + return it->second; +} + bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) { WalletBatch batch(m_storage.GetDatabase()); @@ -2152,6 +2187,7 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int size) { LOCK(cs_desc_man); + std::set<CScript> new_spks; unsigned int target_size; if (size > 0) { target_size = size; @@ -2182,6 +2218,7 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz if (!m_wallet_descriptor.descriptor->Expand(i, provider, scripts_temp, out_keys, &temp_cache)) return false; } // Add all of the scriptPubKeys to the scriptPubKey set + new_spks.insert(scripts_temp.begin(), scripts_temp.end()); for (const CScript& script : scripts_temp) { m_map_script_pub_keys[script] = i; } @@ -2207,6 +2244,7 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz // By this point, the cache size should be the size of the entire range assert(m_wallet_descriptor.range_end - 1 == m_max_cached_index); + m_storage.TopUpCallback(new_spks, this); NotifyCanGetAddressesChanged(); return true; } @@ -2290,55 +2328,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, co return false; } - int64_t creation_time = GetTime(); - - std::string xpub = EncodeExtPubKey(master_key.Neuter()); - - // Build descriptor string - std::string desc_prefix; - std::string desc_suffix = "/*)"; - switch (addr_type) { - case OutputType::LEGACY: { - desc_prefix = "pkh(" + xpub + "/44h"; - break; - } - case OutputType::P2SH_SEGWIT: { - desc_prefix = "sh(wpkh(" + xpub + "/49h"; - desc_suffix += ")"; - break; - } - case OutputType::BECH32: { - desc_prefix = "wpkh(" + xpub + "/84h"; - break; - } - case OutputType::BECH32M: { - desc_prefix = "tr(" + xpub + "/86h"; - break; - } - case OutputType::UNKNOWN: { - // We should never have a DescriptorScriptPubKeyMan for an UNKNOWN OutputType, - // so if we get to this point something is wrong - assert(false); - } - } // no default case, so the compiler can warn about missing cases - assert(!desc_prefix.empty()); - - // Mainnet derives at 0', testnet and regtest derive at 1' - if (Params().IsTestChain()) { - desc_prefix += "/1h"; - } else { - desc_prefix += "/0h"; - } - - std::string internal_path = internal ? "/1" : "/0"; - std::string desc_str = desc_prefix + "/0h" + internal_path + desc_suffix; - - // Make the descriptor - FlatSigningProvider keys; - std::string error; - std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false); - WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); - m_wallet_descriptor = w_desc; + m_wallet_descriptor = GenerateWalletDescriptor(master_key.Neuter(), addr_type, internal); // Store the master private key, and descriptor if (!AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey())) { @@ -2620,6 +2610,7 @@ uint256 DescriptorScriptPubKeyMan::GetID() const void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) { LOCK(cs_desc_man); + std::set<CScript> new_spks; m_wallet_descriptor.cache = cache; for (int32_t i = m_wallet_descriptor.range_start; i < m_wallet_descriptor.range_end; ++i) { FlatSigningProvider out_keys; @@ -2628,6 +2619,7 @@ void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) throw std::runtime_error("Error: Unable to expand wallet descriptor from cache"); } // Add all of the scriptPubKeys to the scriptPubKey set + new_spks.insert(scripts_temp.begin(), scripts_temp.end()); for (const CScript& script : scripts_temp) { if (m_map_script_pub_keys.count(script) != 0) { throw std::runtime_error(strprintf("Error: Already loaded script at index %d as being at index %d", i, m_map_script_pub_keys[script])); @@ -2645,6 +2637,8 @@ void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) } m_max_cached_index++; } + // Make sure the wallet knows about our new spks + m_storage.TopUpCallback(new_spks, this); } bool DescriptorScriptPubKeyMan::AddKey(const CKeyID& key_id, const CKey& key) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 1c99e5ffcd..2c1ab8d44a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -27,10 +27,10 @@ #include <unordered_map> enum class OutputType; -struct bilingual_str; namespace wallet { struct MigrationData; +class ScriptPubKeyMan; // Wallet storage things that ScriptPubKeyMans need in order to be able to store things to the wallet database. // It provides access to things that are part of the entire wallet and not specific to a ScriptPubKeyMan such as @@ -51,6 +51,8 @@ public: virtual bool WithEncryptionKey(std::function<bool (const CKeyingMaterial&)> cb) const = 0; virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked() const = 0; + //! Callback function for after TopUp completes containing any scripts that were added by a SPKMan + virtual void TopUpCallback(const std::set<CScript>&, ScriptPubKeyMan*) = 0; }; //! Constant representing an unknown spkm creation time @@ -630,6 +632,9 @@ public: bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal); bool HavePrivateKeys() const override; + bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + //! Retrieve the particular key if it is available. Returns nullopt if the key is not in the wallet, or if the wallet is locked. + std::optional<CKey> GetKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); std::optional<int64_t> GetOldestKeyPoolTime() const override; unsigned int GetKeyPoolSize() const override; @@ -666,7 +671,7 @@ public: std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys(int32_t minimum_index) const; int32_t GetEndRange() const; - bool GetDescriptorString(std::string& out, const bool priv) const; + [[nodiscard]] bool GetDescriptorString(std::string& out, const bool priv) const; void UpgradeDescriptorCache(); }; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e229962ca5..9a7e166e68 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -683,11 +683,11 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co // Vector of results. We will choose the best one based on waste. std::vector<SelectionResult> results; std::vector<util::Result<SelectionResult>> errors; - auto append_error = [&] (const util::Result<SelectionResult>& result) { + auto append_error = [&] (util::Result<SelectionResult>&& result) { // If any specific error message appears here, then something different from a simple "no selection found" happened. // Let's save it, so it can be retrieved to the user if no other selection algorithm succeeded. if (HasErrorMsg(result)) { - errors.emplace_back(result); + errors.emplace_back(std::move(result)); } }; @@ -698,7 +698,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co if (!coin_selection_params.m_subtract_fee_outputs) { 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); + } else append_error(std::move(bnb_result)); } // As Knapsack and SRD can create change, also deduce change weight. @@ -707,16 +707,25 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co // 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. if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast, max_inputs_weight)}) { results.push_back(*knapsack_result); - } else append_error(knapsack_result); + } else append_error(std::move(knapsack_result)); + + if (coin_selection_params.m_effective_feerate > CFeeRate{3 * coin_selection_params.m_long_term_feerate}) { // Minimize input set for feerates of at least 3×LTFRE (default: 30 ṩ/vB+) + if (auto cg_result{CoinGrinder(groups.positive_group, nTargetValue, coin_selection_params.m_min_change_target, max_inputs_weight)}) { + cg_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); + results.push_back(*cg_result); + } else { + append_error(std::move(cg_result)); + } + } if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.m_change_fee, coin_selection_params.rng_fast, max_inputs_weight)}) { results.push_back(*srd_result); - } else append_error(srd_result); + } else append_error(std::move(srd_result)); if (results.empty()) { // No solution found, retrieve the first explicit error (if any). // future: add 'severity level' to errors so the worst one can be retrieved instead of the first one. - return errors.empty() ? util::Error() : errors.front(); + return errors.empty() ? util::Error() : std::move(errors.front()); } // If the chosen input set has unconfirmed inputs, check for synergies from overlapping ancestry @@ -809,7 +818,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the // transaction at a target feerate. If an attempt fails, more attempts may be made using a more // permissive CoinEligibilityFilter. - util::Result<SelectionResult> res = [&] { + { // Place coins eligibility filters on a scope increasing order. std::vector<SelectionFilter> ordered_filters{ // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six @@ -857,9 +866,9 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select) { // Special case, too-long-mempool cluster. if (total_amount + total_unconf_long_chain > value_to_select) { - return util::Result<SelectionResult>({_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")}); + return util::Error{_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")}; } - return util::Result<SelectionResult>(util::Error()); // General "Insufficient Funds" + return util::Error{}; // General "Insufficient Funds" } // Walk-through the filters until the solution gets found. @@ -876,19 +885,17 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin // If any specific error message appears here, then something particularly wrong might have happened. // Save the error and continue the selection process. So if no solutions gets found, we can return // the detailed error to the upper layers. - if (HasErrorMsg(res)) res_detailed_errors.emplace_back(res); + if (HasErrorMsg(res)) res_detailed_errors.emplace_back(std::move(res)); } } // Return right away if we have a detailed error - if (!res_detailed_errors.empty()) return res_detailed_errors.front(); + if (!res_detailed_errors.empty()) return std::move(res_detailed_errors.front()); // General "Insufficient Funds" - return util::Result<SelectionResult>(util::Error()); - }(); - - return res; + return util::Error{}; + } } static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256& block_hash) diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 2cbeedbde6..5e3a8179a2 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -2,6 +2,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <wallet/sqlite.h> #include <chainparams.h> diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 9fea14145f..9a349f0992 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -1090,6 +1090,216 @@ BOOST_AUTO_TEST_CASE(effective_value_test) BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1 } +static util::Result<SelectionResult> CoinGrinder(const CAmount& target, + const CoinSelectionParams& cs_params, + const node::NodeContext& m_node, + int max_weight, + std::function<CoinsResult(CWallet&)> coin_setup) +{ + std::unique_ptr<CWallet> wallet = NewWallet(m_node); + CoinEligibilityFilter filter(0, 0, 0); // accept all coins without ancestors + Groups group = GroupOutputs(*wallet, coin_setup(*wallet), cs_params, {{filter}})[filter].all_groups; + return CoinGrinder(group.positive_group, target, cs_params.m_min_change_target, max_weight); +} + +BOOST_AUTO_TEST_CASE(coin_grinder_tests) +{ + // Test Coin Grinder: + // 1) Insufficient funds, select all provided coins and fail. + // 2) Exceeded max weight, coin selection always surpasses the max allowed weight. + // 3) Select coins without surpassing the max weight (some coins surpasses the max allowed weight, some others not) + // 4) Test that two less valuable UTXOs with a combined lower weight are preferred over a more valuable heavier UTXO + // 5) Test finding a solution in a UTXO pool with mixed weights + // 6) Test that the lightest solution among many clones is found + // 7) Test that lots of tiny UTXOs can be skipped if they are too heavy while there are enough funds in lookahead + + FastRandomContext rand; + CoinSelectionParams dummy_params{ // Only used to provide the 'avoid_partial' flag. + rand, + /*change_output_size=*/34, + /*change_spend_size=*/68, + /*min_change_target=*/CENT, + /*effective_feerate=*/CFeeRate(5000), + /*long_term_feerate=*/CFeeRate(2000), + /*discard_feerate=*/CFeeRate(1000), + /*tx_noinputs_size=*/10 + 34, // static header size + output size + /*avoid_partial=*/false, + }; + + { + // ######################################################### + // 1) Insufficient funds, select all provided coins and fail + // ######################################################### + CAmount target = 49.5L * COIN; + int max_weight = 10'000; // high enough to not fail for this reason. + const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + for (int j = 0; j < 10; ++j) { + add_coin(available_coins, wallet, CAmount(1 * COIN)); + add_coin(available_coins, wallet, CAmount(2 * COIN)); + } + return available_coins; + }); + BOOST_CHECK(!res); + BOOST_CHECK(util::ErrorString(res).empty()); // empty means "insufficient funds" + } + + { + // ########################### + // 2) Test max weight exceeded + // ########################### + CAmount target = 29.5L * COIN; + int max_weight = 3000; + const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + for (int j = 0; j < 10; ++j) { + add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true); + add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(5000), 144, false, 0, true); + } + return available_coins; + }); + BOOST_CHECK(!res); + BOOST_CHECK(util::ErrorString(res).original.find("The inputs size exceeds the maximum weight") != std::string::npos); + } + + { + // ############################################################################################################### + // 3) Test selection when some coins surpass the max allowed weight while others not. --> must find a good solution + // ################################################################################################################ + CAmount target = 25.33L * COIN; + int max_weight = 10'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + for (int j = 0; j < 60; ++j) { // 60 UTXO --> 19,8 BTC total --> 60 × 272 WU = 16320 WU + add_coin(available_coins, wallet, CAmount(0.33 * COIN), CFeeRate(5000), 144, false, 0, true); + } + for (int i = 0; i < 10; i++) { // 10 UTXO --> 20 BTC total --> 10 × 272 WU = 2720 WU + add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(5000), 144, false, 0, true); + } + return available_coins; + }); + BOOST_CHECK(res); + // Demonstrate how following improvements reduce iteration count and catch any regressions in the future. + size_t expected_attempts = 37; + BOOST_CHECK_MESSAGE(res->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, res->GetSelectionsEvaluated())); + } + + { + // ################################################################################################################# + // 4) Test that two less valuable UTXOs with a combined lower weight are preferred over a more valuable heavier UTXO + // ################################################################################################################# + CAmount target = 1.9L * COIN; + int max_weight = 400'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(5000), 144, false, 0, true, 148); + add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true, 68); + add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true, 68); + return available_coins; + }); + SelectionResult expected_result(CAmount(0), SelectionAlgorithm::CG); + add_coin(1 * COIN, 1, expected_result); + add_coin(1 * COIN, 2, expected_result); + BOOST_CHECK(EquivalentResult(expected_result, *res)); + // Demonstrate how following improvements reduce iteration count and catch any regressions in the future. + size_t expected_attempts = 3; + BOOST_CHECK_MESSAGE(res->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, res->GetSelectionsEvaluated())); + } + + { + // ############################################################################################################### + // 5) Test finding a solution in a UTXO pool with mixed weights + // ################################################################################################################ + CAmount target = 30L * COIN; + int max_weight = 400'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + for (int j = 0; j < 5; ++j) { + // Add heavy coins {3, 6, 9, 12, 15} + add_coin(available_coins, wallet, CAmount((3 + 3 * j) * COIN), CFeeRate(5000), 144, false, 0, true, 350); + // Add medium coins {2, 5, 8, 11, 14} + add_coin(available_coins, wallet, CAmount((2 + 3 * j) * COIN), CFeeRate(5000), 144, false, 0, true, 250); + // Add light coins {1, 4, 7, 10, 13} + add_coin(available_coins, wallet, CAmount((1 + 3 * j) * COIN), CFeeRate(5000), 144, false, 0, true, 150); + } + return available_coins; + }); + BOOST_CHECK(res); + SelectionResult expected_result(CAmount(0), SelectionAlgorithm::CG); + add_coin(14 * COIN, 1, expected_result); + add_coin(13 * COIN, 2, expected_result); + add_coin(4 * COIN, 3, expected_result); + BOOST_CHECK(EquivalentResult(expected_result, *res)); + // Demonstrate how following improvements reduce iteration count and catch any regressions in the future. + size_t expected_attempts = 92; + BOOST_CHECK_MESSAGE(res->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, res->GetSelectionsEvaluated())); + } + + { + // ################################################################################################################# + // 6) Test that the lightest solution among many clones is found + // ################################################################################################################# + CAmount target = 9.9L * COIN; + int max_weight = 400'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + // Expected Result: 4 + 3 + 2 + 1 = 10 BTC at 400 vB + add_coin(available_coins, wallet, CAmount(4 * COIN), CFeeRate(5000), 144, false, 0, true, 100); + add_coin(available_coins, wallet, CAmount(3 * COIN), CFeeRate(5000), 144, false, 0, true, 100); + add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(5000), 144, false, 0, true, 100); + add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true, 100); + // Distracting clones: + for (int j = 0; j < 100; ++j) { + add_coin(available_coins, wallet, CAmount(8 * COIN), CFeeRate(5000), 144, false, 0, true, 1000); + } + for (int j = 0; j < 100; ++j) { + add_coin(available_coins, wallet, CAmount(7 * COIN), CFeeRate(5000), 144, false, 0, true, 800); + } + for (int j = 0; j < 100; ++j) { + add_coin(available_coins, wallet, CAmount(6 * COIN), CFeeRate(5000), 144, false, 0, true, 600); + } + for (int j = 0; j < 100; ++j) { + add_coin(available_coins, wallet, CAmount(5 * COIN), CFeeRate(5000), 144, false, 0, true, 400); + } + return available_coins; + }); + SelectionResult expected_result(CAmount(0), SelectionAlgorithm::CG); + add_coin(4 * COIN, 0, expected_result); + add_coin(3 * COIN, 0, expected_result); + add_coin(2 * COIN, 0, expected_result); + add_coin(1 * COIN, 0, expected_result); + BOOST_CHECK(EquivalentResult(expected_result, *res)); + // Demonstrate how following improvements reduce iteration count and catch any regressions in the future. + size_t expected_attempts = 38; + BOOST_CHECK_MESSAGE(res->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, res->GetSelectionsEvaluated())); + } + + { + // ################################################################################################################# + // 7) Test that lots of tiny UTXOs can be skipped if they are too heavy while there are enough funds in lookahead + // ################################################################################################################# + CAmount target = 1.9L * COIN; + int max_weight = 40000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + add_coin(available_coins, wallet, CAmount(1.8 * COIN), CFeeRate(5000), 144, false, 0, true, 2500); + add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true, 1000); + add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true, 1000); + for (int j = 0; j < 100; ++j) { + // make a 100 unique coins only differing by one sat + add_coin(available_coins, wallet, CAmount(0.01 * COIN + j), CFeeRate(5000), 144, false, 0, true, 110); + } + return available_coins; + }); + SelectionResult expected_result(CAmount(0), SelectionAlgorithm::CG); + add_coin(1 * COIN, 1, expected_result); + add_coin(1 * COIN, 2, expected_result); + BOOST_CHECK(EquivalentResult(expected_result, *res)); + // Demonstrate how following improvements reduce iteration count and catch any regressions in the future. + size_t expected_attempts = 7; + BOOST_CHECK_MESSAGE(res->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, res->GetSelectionsEvaluated())); + } +} static util::Result<SelectionResult> SelectCoinsSRD(const CAmount& target, const CoinSelectionParams& cs_params, @@ -1150,6 +1360,7 @@ BOOST_AUTO_TEST_CASE(srd_tests) const auto& res = SelectCoinsSRD(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { CoinsResult available_coins; for (int j = 0; j < 10; ++j) { + /* 10 × 1 BTC + 10 × 2 BTC = 30 BTC. 20 × 272 WU = 5440 WU */ add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(0), 144, false, 0, true); add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(0), 144, false, 0, true); } diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index adbbb94318..438dfceb7f 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -2,6 +2,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <boost/test/unit_test.hpp> #include <test/util/setup_common.h> @@ -228,6 +230,39 @@ BOOST_AUTO_TEST_CASE(db_availability_after_write_error) } } +// Verify 'ErasePrefix' functionality using db keys similar to the ones used by the wallet. +// Keys are in the form of std::pair<TYPE, ENTRY_ID> +BOOST_AUTO_TEST_CASE(erase_prefix) +{ + const std::string key = "key"; + const std::string key2 = "key2"; + const std::string value = "value"; + const std::string value2 = "value_2"; + auto make_key = [](std::string type, std::string id) { return std::make_pair(type, id); }; + + for (const auto& database : TestDatabases(m_path_root)) { + std::unique_ptr<DatabaseBatch> batch = database->MakeBatch(); + + // Write two entries with the same key type prefix, a third one with a different prefix + // and a fourth one with the type-id values inverted + BOOST_CHECK(batch->Write(make_key(key, value), value)); + BOOST_CHECK(batch->Write(make_key(key, value2), value2)); + BOOST_CHECK(batch->Write(make_key(key2, value), value)); + BOOST_CHECK(batch->Write(make_key(value, key), value)); + + // Erase the ones with the same prefix and verify result + BOOST_CHECK(batch->TxnBegin()); + BOOST_CHECK(batch->ErasePrefix(DataStream() << key)); + BOOST_CHECK(batch->TxnCommit()); + + BOOST_CHECK(!batch->Exists(make_key(key, value))); + BOOST_CHECK(!batch->Exists(make_key(key, value2))); + // Also verify that entries with a different prefix were not erased + BOOST_CHECK(batch->Exists(make_key(key2, value))); + BOOST_CHECK(batch->Exists(make_key(value, key))); + } +} + #ifdef USE_SQLITE // Test-only statement execution error diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 87d419493b..331590df7f 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -11,6 +11,7 @@ #include <test/util/setup_common.h> #include <wallet/coinselection.h> +#include <numeric> #include <vector> namespace wallet { @@ -77,6 +78,144 @@ static SelectionResult ManualSelection(std::vector<COutput>& utxos, const CAmoun // Returns true if the result contains an error and the message is not empty static bool HasErrorMsg(const util::Result<SelectionResult>& res) { return !util::ErrorString(res).empty(); } +FUZZ_TARGET(coin_grinder) +{ + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + std::vector<COutput> utxo_pool; + + const CAmount target{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY)}; + + FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)}; + CoinSelectionParams coin_params{fast_random_context}; + coin_params.m_subtract_fee_outputs = fuzzed_data_provider.ConsumeBool(); + coin_params.m_long_term_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; + coin_params.m_effective_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; + coin_params.change_output_size = fuzzed_data_provider.ConsumeIntegralInRange<int>(10, 1000); + coin_params.change_spend_size = fuzzed_data_provider.ConsumeIntegralInRange<int>(10, 1000); + coin_params.m_cost_of_change= coin_params.m_effective_feerate.GetFee(coin_params.change_output_size) + coin_params.m_long_term_feerate.GetFee(coin_params.change_spend_size); + coin_params.m_change_fee = coin_params.m_effective_feerate.GetFee(coin_params.change_output_size); + // For other results to be comparable to SRD, we must align the change_target with SRD’s hardcoded behavior + coin_params.m_min_change_target = CHANGE_LOWER + coin_params.m_change_fee; + + // Create some coins + CAmount total_balance{0}; + CAmount max_spendable{0}; + int next_locktime{0}; + LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) + { + const int n_input{fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 10)}; + const int n_input_bytes{fuzzed_data_provider.ConsumeIntegralInRange<int>(41, 10000)}; + const CAmount amount{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY)}; + if (total_balance + amount >= MAX_MONEY) { + break; + } + AddCoin(amount, n_input, n_input_bytes, ++next_locktime, utxo_pool, coin_params.m_effective_feerate); + total_balance += amount; + CAmount eff_value = amount - coin_params.m_effective_feerate.GetFee(n_input_bytes); + max_spendable += eff_value; + } + + std::vector<OutputGroup> group_pos; + GroupCoins(fuzzed_data_provider, utxo_pool, coin_params, /*positive_only=*/true, group_pos); + + // Run coinselection algorithms + auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, MAX_STANDARD_TX_WEIGHT); + if (target + coin_params.m_min_change_target > max_spendable || HasErrorMsg(result_cg)) return; // We only need to compare algorithms if CoinGrinder has a solution + assert(result_cg); + if (!result_cg->GetAlgoCompleted()) return; // Bail out if CoinGrinder solution is not optimal + + auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); + if (result_srd && result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0) { // exclude any srd solutions that don’t have change, err on excluding + assert(result_srd->GetWeight() >= result_cg->GetWeight()); + } + + auto result_knapsack = KnapsackSolver(group_pos, target, coin_params.m_min_change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT); + if (result_knapsack && result_knapsack->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0) { // exclude any knapsack solutions that don’t have change, err on excluding + assert(result_knapsack->GetWeight() >= result_cg->GetWeight()); + } +} + +FUZZ_TARGET(coin_grinder_is_optimal) +{ + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + + FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)}; + CoinSelectionParams coin_params{fast_random_context}; + coin_params.m_subtract_fee_outputs = false; + // Set effective feerate up to MAX_MONEY sats per 1'000'000 vB (2'100'000'000 sat/vB = 21'000 BTC/kvB). + coin_params.m_effective_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, MAX_MONEY), 1'000'000}; + coin_params.m_min_change_target = ConsumeMoney(fuzzed_data_provider); + + // Create some coins + CAmount max_spendable{0}; + int next_locktime{0}; + static constexpr unsigned max_output_groups{16}; + std::vector<OutputGroup> group_pos; + LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), max_output_groups) + { + // With maximum m_effective_feerate and n_input_bytes = 1'000'000, input_fee <= MAX_MONEY. + const int n_input_bytes{fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 1'000'000)}; + // Only make UTXOs with positive effective value + const CAmount input_fee = coin_params.m_effective_feerate.GetFee(n_input_bytes); + // Ensure that each UTXO has at least an effective value of 1 sat + const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY + group_pos.size() - max_spendable - max_output_groups)}; + const CAmount amount{eff_value + input_fee}; + std::vector<COutput> temp_utxo_pool; + + AddCoin(amount, /*n_input=*/0, n_input_bytes, ++next_locktime, temp_utxo_pool, coin_params.m_effective_feerate); + max_spendable += eff_value; + + auto output_group = OutputGroup(coin_params); + output_group.Insert(std::make_shared<COutput>(temp_utxo_pool.at(0)), /*ancestors=*/0, /*descendants=*/0); + group_pos.push_back(output_group); + } + size_t num_groups = group_pos.size(); + assert(num_groups <= max_output_groups); + + // Only choose targets below max_spendable + const CAmount target{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, std::max(CAmount{1}, max_spendable - coin_params.m_min_change_target))}; + + // Brute force optimal solution + CAmount best_amount{MAX_MONEY}; + int best_weight{std::numeric_limits<int>::max()}; + for (uint32_t pattern = 1; (pattern >> num_groups) == 0; ++pattern) { + CAmount subset_amount{0}; + int subset_weight{0}; + for (unsigned i = 0; i < num_groups; ++i) { + if ((pattern >> i) & 1) { + subset_amount += group_pos[i].GetSelectionAmount(); + subset_weight += group_pos[i].m_weight; + } + } + if ((subset_amount >= target + coin_params.m_min_change_target) && (subset_weight < best_weight || (subset_weight == best_weight && subset_amount < best_amount))) { + best_weight = subset_weight; + best_amount = subset_amount; + } + } + + if (best_weight < std::numeric_limits<int>::max()) { + // Sufficient funds and acceptable weight: CoinGrinder should find at least one solution + int high_max_weight = fuzzed_data_provider.ConsumeIntegralInRange<int>(best_weight, std::numeric_limits<int>::max()); + + auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, high_max_weight); + assert(result_cg); + assert(result_cg->GetWeight() <= high_max_weight); + assert(result_cg->GetSelectedEffectiveValue() >= target + coin_params.m_min_change_target); + assert(best_weight < result_cg->GetWeight() || (best_weight == result_cg->GetWeight() && best_amount <= result_cg->GetSelectedEffectiveValue())); + if (result_cg->GetAlgoCompleted()) { + // If CoinGrinder exhausted the search space, it must return the optimal solution + assert(best_weight == result_cg->GetWeight()); + assert(best_amount == result_cg->GetSelectedEffectiveValue()); + } + } + + // CoinGrinder cannot ever find a better solution than the brute-forced best, or there is none in the first place + int low_max_weight = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, best_weight - 1); + auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, low_max_weight); + // Max_weight should have been exceeded, or there were insufficient funds + assert(!result_cg); +} + FUZZ_TARGET(coinselection) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; @@ -152,7 +291,10 @@ FUZZ_TARGET(coinselection) } std::vector<COutput> utxos; - std::vector<util::Result<SelectionResult>> results{result_srd, result_knapsack, result_bnb}; + std::vector<util::Result<SelectionResult>> results; + results.emplace_back(std::move(result_srd)); + results.emplace_back(std::move(result_knapsack)); + results.emplace_back(std::move(result_bnb)); CAmount new_total_balance{CreateCoins(fuzzed_data_provider, utxos, coin_params, next_locktime)}; if (new_total_balance > 0) { std::set<std::shared_ptr<COutput>> new_utxo_pool; diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 9a515828fe..792079e6c6 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -106,13 +106,11 @@ struct FuzzedWallet { CTxDestination GetDestination(FuzzedDataProvider& fuzzed_data_provider) { auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)}; - util::Result<CTxDestination> op_dest{util::Error{}}; if (fuzzed_data_provider.ConsumeBool()) { - op_dest = wallet->GetNewDestination(type, ""); + return *Assert(wallet->GetNewDestination(type, "")); } else { - op_dest = wallet->GetNewChangeDestination(type); + return *Assert(wallet->GetNewChangeDestination(type)); } - return *Assert(op_dest); } CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider) { return GetScriptForDestination(GetDestination(fuzzed_data_provider)); } void FundTx(FuzzedDataProvider& fuzzed_data_provider, CMutableTransaction tx) diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 3509bc116f..963c0f838b 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -97,13 +97,11 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup) // so that the recipient's amount is no longer equal to the user's selected target of 299 BTC. // First case, use 'subtract_fee_from_outputs=true' - util::Result<CreatedTransactionResult> res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control); - BOOST_CHECK(!res_tx.has_value()); + BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control)); // Second case, don't use 'subtract_fee_from_outputs'. recipients[0].fSubtractFeeFromAmount = false; - res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control); - BOOST_CHECK(!res_tx.has_value()); + BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control)); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index cbf3ccd1ec..49d206f409 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -71,7 +71,8 @@ std::shared_ptr<CWallet> TestLoadWallet(WalletContext& context) void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet) { - SyncWithValidationInterfaceQueue(); + // Calls SyncWithValidationInterfaceQueue + wallet->chain().waitForNotificationsIfTipChanged({}); wallet->m_chain_notifications_handler.reset(); UnloadWallet(std::move(wallet)); } diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index 8bd238648f..a3e6ede81e 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_WALLET_TEST_UTIL_H #define BITCOIN_WALLET_TEST_UTIL_H +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <addresstype.h> #include <wallet/db.h> diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 65297054df..3a67b9a433 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -445,9 +445,11 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) auto requests = wallet->GetAddressReceiveRequests(); auto erequests = {"val_rr11", "val_rr20"}; BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests)); - WalletBatch batch{wallet->GetDatabase()}; - BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false)); - BOOST_CHECK(batch.EraseAddressData(ScriptHash())); + RunWithinTxn(wallet->GetDatabase(), /*process_desc*/"test", [](WalletBatch& batch){ + BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false)); + BOOST_CHECK(batch.EraseAddressData(ScriptHash())); + return true; + }); }); TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) { BOOST_CHECK(!wallet->IsAddressPreviouslySpent(PKHash())); @@ -812,7 +814,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) // transactionAddedToMempool notifications, and create block and mempool // transactions paying to the wallet std::promise<void> promise; - CallFunctionInValidationInterfaceQueue([&promise] { + m_node.validation_signals->CallFunctionInValidationInterfaceQueue([&promise] { promise.get_future().wait(); }); std::string error; @@ -840,7 +842,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) // Unblock notification queue and make sure stale blockConnected and // transactionAddedToMempool events are processed promise.set_value(); - SyncWithValidationInterfaceQueue(); + m_node.validation_signals->SyncWithValidationInterfaceQueue(); // AddToWallet events for block_tx and mempool_tx events are counted a // second time as the notification queue is processed BOOST_CHECK_EQUAL(addtx_count, 5); @@ -863,7 +865,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); - SyncWithValidationInterfaceQueue(); + m_node.validation_signals->SyncWithValidationInterfaceQueue(); }); wallet = TestLoadWallet(context); // Since mempool transactions are requested at the end of loading, there will @@ -888,7 +890,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup) UnloadWallet(std::move(wallet)); } -BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) +BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup) { m_args.ForceSetArg("-unsafesqlitesync", "1"); WalletContext context; @@ -903,7 +905,7 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - SyncWithValidationInterfaceQueue(); + m_node.validation_signals->SyncWithValidationInterfaceQueue(); { auto block_hash = block_tx.GetHash(); @@ -913,8 +915,8 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) BOOST_CHECK(wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); - std::vector<uint256> vHashIn{ block_hash }, vHashOut; - BOOST_CHECK_EQUAL(wallet->ZapSelectTx(vHashIn, vHashOut), DBErrors::LOAD_OK); + std::vector<uint256> vHashIn{ block_hash }; + BOOST_CHECK(wallet->RemoveTxs(vHashIn)); BOOST_CHECK(!wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 0u); diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index 3dba2231f0..2e43eda582 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -34,6 +34,7 @@ public: std::optional<int64_t> ScriptSize() const override { return {}; } std::optional<int64_t> MaxSatisfactionWeight(bool) const override { return {}; } std::optional<int64_t> MaxSatisfactionElems() const override { return {}; } + void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const override {} }; BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup) diff --git a/src/wallet/transaction.cpp b/src/wallet/transaction.cpp index 6777257e53..561880482f 100644 --- a/src/wallet/transaction.cpp +++ b/src/wallet/transaction.cpp @@ -45,7 +45,7 @@ void CWalletTx::updateState(interfaces::Chain& chain) }; if (auto* conf = state<TxStateConfirmed>()) { lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, m_state); - } else if (auto* conf = state<TxStateConflicted>()) { + } else if (auto* conf = state<TxStateBlockConflicted>()) { lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, m_state); } } diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index ddeb931112..9c27574103 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -43,12 +43,12 @@ struct TxStateInMempool { }; //! State of rejected transaction that conflicts with a confirmed block. -struct TxStateConflicted { +struct TxStateBlockConflicted { uint256 conflicting_block_hash; int conflicting_block_height; - explicit TxStateConflicted(const uint256& block_hash, int height) : conflicting_block_hash(block_hash), conflicting_block_height(height) {} - std::string toString() const { return strprintf("Conflicted (block=%s, height=%i)", conflicting_block_hash.ToString(), conflicting_block_height); } + explicit TxStateBlockConflicted(const uint256& block_hash, int height) : conflicting_block_hash(block_hash), conflicting_block_height(height) {} + std::string toString() const { return strprintf("BlockConflicted (block=%s, height=%i)", conflicting_block_hash.ToString(), conflicting_block_height); } }; //! State of transaction not confirmed or conflicting with a known block and @@ -75,7 +75,7 @@ struct TxStateUnrecognized { }; //! All possible CWalletTx states -using TxState = std::variant<TxStateConfirmed, TxStateInMempool, TxStateConflicted, TxStateInactive, TxStateUnrecognized>; +using TxState = std::variant<TxStateConfirmed, TxStateInMempool, TxStateBlockConflicted, TxStateInactive, TxStateUnrecognized>; //! Subset of states transaction sync logic is implemented to handle. using SyncTxState = std::variant<TxStateConfirmed, TxStateInMempool, TxStateInactive>; @@ -90,7 +90,7 @@ static inline TxState TxStateInterpretSerialized(TxStateUnrecognized data) } else if (data.index >= 0) { return TxStateConfirmed{data.block_hash, /*height=*/-1, data.index}; } else if (data.index == -1) { - return TxStateConflicted{data.block_hash, /*height=*/-1}; + return TxStateBlockConflicted{data.block_hash, /*height=*/-1}; } return data; } @@ -102,7 +102,7 @@ static inline uint256 TxStateSerializedBlockHash(const TxState& state) [](const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; }, [](const TxStateInMempool& in_mempool) { return uint256::ZERO; }, [](const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; }, - [](const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; }, + [](const TxStateBlockConflicted& conflicted) { return conflicted.conflicting_block_hash; }, [](const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; } }, state); } @@ -114,7 +114,7 @@ static inline int TxStateSerializedIndex(const TxState& state) [](const TxStateInactive& inactive) { return inactive.abandoned ? -1 : 0; }, [](const TxStateInMempool& in_mempool) { return 0; }, [](const TxStateConfirmed& confirmed) { return confirmed.position_in_block; }, - [](const TxStateConflicted& conflicted) { return -1; }, + [](const TxStateBlockConflicted& conflicted) { return -1; }, [](const TxStateUnrecognized& unrecognized) { return unrecognized.index; } }, state); } @@ -258,6 +258,14 @@ public: CTransactionRef tx; TxState m_state; + // Set of mempool transactions that conflict + // directly with the transaction, or that conflict + // with an ancestor transaction. This set will be + // empty if state is InMempool or Confirmed, but + // can be nonempty if state is Inactive or + // BlockConflicted. + std::set<Txid> mempool_conflicts; + template<typename Stream> void Serialize(Stream& s) const { @@ -335,9 +343,10 @@ public: void updateState(interfaces::Chain& chain); bool isAbandoned() const { return state<TxStateInactive>() && state<TxStateInactive>()->abandoned; } - bool isConflicted() const { return state<TxStateConflicted>(); } + bool isMempoolConflicted() const { return !mempool_conflicts.empty(); } + bool isBlockConflicted() const { return state<TxStateBlockConflicted>(); } bool isInactive() const { return state<TxStateInactive>(); } - bool isUnconfirmed() const { return !isAbandoned() && !isConflicted() && !isConfirmed(); } + bool isUnconfirmed() const { return !isAbandoned() && !isBlockConflicted() && !isMempoolConflicted() && !isConfirmed(); } bool isConfirmed() const { return state<TxStateConfirmed>(); } const Txid& GetHash() const LIFETIMEBOUND { return tx->GetHash(); } const Wtxid& GetWitnessHash() const LIFETIMEBOUND { return tx->GetWitnessHash(); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fdf610955b..45f69f52d1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5,9 +5,7 @@ #include <wallet/wallet.h> -#if defined(HAVE_CONFIG_H) -#include <config/bitcoin-config.h> -#endif +#include <config/bitcoin-config.h> // IWYU pragma: keep #include <addresstype.h> #include <blockfilter.h> #include <chain.h> @@ -752,8 +750,8 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const const uint256& wtxid = it->second; const auto mit = mapWallet.find(wtxid); if (mit != mapWallet.end()) { - int depth = GetTxDepthInMainChain(mit->second); - if (depth > 0 || (depth == 0 && !mit->second.isAbandoned())) + const auto& wtx = mit->second; + if (!wtx.isAbandoned() && !wtx.isBlockConflicted() && !wtx.isMempoolConflicted()) return true; // Spent } } @@ -1197,7 +1195,7 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx auto it = mapWallet.find(txin.prevout.hash); if (it != mapWallet.end()) { CWalletTx& prevtx = it->second; - if (auto* prev = prevtx.state<TxStateConflicted>()) { + if (auto* prev = prevtx.state<TxStateBlockConflicted>()) { MarkConflicted(prev->conflicting_block_hash, prev->conflicting_block_height, wtx.GetHash()); } } @@ -1309,7 +1307,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) assert(!wtx.isConfirmed()); assert(!wtx.InMempool()); // If already conflicted or abandoned, no need to set abandoned - if (!wtx.isConflicted() && !wtx.isAbandoned()) { + if (!wtx.isBlockConflicted() && !wtx.isAbandoned()) { wtx.m_state = TxStateInactive{/*abandoned=*/true}; return TxUpdate::NOTIFY_CHANGED; } @@ -1346,7 +1344,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c if (conflictconfirms < GetTxDepthInMainChain(wtx)) { // Block is 'more conflicted' than current confirm; update. // Mark transaction as conflicted with this block. - wtx.m_state = TxStateConflicted{hashBlock, conflicting_height}; + wtx.m_state = TxStateBlockConflicted{hashBlock, conflicting_height}; return TxUpdate::CHANGED; } return TxUpdate::UNCHANGED; @@ -1360,7 +1358,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { // Do not flush the wallet here for performance reasons WalletBatch batch(GetDatabase(), false); + RecursiveUpdateTxState(&batch, tx_hash, try_updating_state); +} +void CWallet::RecursiveUpdateTxState(WalletBatch* batch, const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { std::set<uint256> todo; std::set<uint256> done; @@ -1377,7 +1378,7 @@ void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingSt TxUpdate update_state = try_updating_state(wtx); if (update_state != TxUpdate::UNCHANGED) { wtx.MarkDirty(); - batch.WriteTx(wtx); + if (batch) batch->WriteTx(wtx); // Iterate over all its outputs, and update those tx states as well (if applicable) for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range = mapTxSpends.equal_range(COutPoint(Txid::FromUint256(now), i)); @@ -1418,6 +1419,20 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) { if (it != mapWallet.end()) { RefreshMempoolStatus(it->second, chain()); } + + const Txid& txid = tx->GetHash(); + + for (const CTxIn& tx_in : tx->vin) { + // For each wallet transaction spending this prevout.. + for (auto range = mapTxSpends.equal_range(tx_in.prevout); range.first != range.second; range.first++) { + const uint256& spent_id = range.first->second; + // Skip the recently added tx + if (spent_id == txid) continue; + RecursiveUpdateTxState(/*batch=*/nullptr, spent_id, [&txid](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + return wtx.mempool_conflicts.insert(txid).second ? TxUpdate::CHANGED : TxUpdate::UNCHANGED; + }); + } + } } void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) { @@ -1455,6 +1470,21 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking SyncTransaction(tx, TxStateInactive{}); } + + const Txid& txid = tx->GetHash(); + + for (const CTxIn& tx_in : tx->vin) { + // Iterate over all wallet transactions spending txin.prev + // and recursively mark them as no longer conflicting with + // txid + for (auto range = mapTxSpends.equal_range(tx_in.prevout); range.first != range.second; range.first++) { + const uint256& spent_id = range.first->second; + + RecursiveUpdateTxState(/*batch=*/nullptr, spent_id, [&txid](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + return wtx.mempool_conflicts.erase(txid) ? TxUpdate::CHANGED : TxUpdate::UNCHANGED; + }); + } + } } void CWallet::blockConnected(ChainstateRole role, const interfaces::BlockInfo& block) @@ -1506,11 +1536,11 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) for (TxSpends::const_iterator _it = range.first; _it != range.second; ++_it) { CWalletTx& wtx = mapWallet.find(_it->second)->second; - if (!wtx.isConflicted()) continue; + if (!wtx.isBlockConflicted()) continue; auto try_updating_state = [&](CWalletTx& tx) { - if (!tx.isConflicted()) return TxUpdate::UNCHANGED; - if (tx.state<TxStateConflicted>()->conflicting_block_height >= disconnect_height) { + if (!tx.isBlockConflicted()) return TxUpdate::UNCHANGED; + if (tx.state<TxStateBlockConflicted>()->conflicting_block_height >= disconnect_height) { tx.m_state = TxStateInactive{}; return TxUpdate::CHANGED; } @@ -1571,11 +1601,22 @@ isminetype CWallet::IsMine(const CTxDestination& dest) const isminetype CWallet::IsMine(const CScript& script) const { AssertLockHeld(cs_wallet); - isminetype result = ISMINE_NO; - for (const auto& spk_man_pair : m_spk_managers) { - result = std::max(result, spk_man_pair.second->IsMine(script)); + + // Search the cache so that IsMine is called only on the relevant SPKMs instead of on everything in m_spk_managers + const auto& it = m_cached_spks.find(script); + if (it != m_cached_spks.end()) { + isminetype res = ISMINE_NO; + for (const auto& spkm : it->second) { + res = std::max(res, spkm->IsMine(script)); + } + Assume(res == ISMINE_SPENDABLE); + return res; } - return result; + + // Legacy wallet + if (IsLegacy()) return GetLegacyScriptPubKeyMan()->IsMine(script); + + return ISMINE_NO; } bool CWallet::IsMine(const CTransaction& tx) const @@ -2320,12 +2361,41 @@ DBErrors CWallet::LoadWallet() return nLoadWalletRet; } -DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut) +util::Result<void> CWallet::RemoveTxs(std::vector<uint256>& txs_to_remove) { AssertLockHeld(cs_wallet); - DBErrors nZapSelectTxRet = WalletBatch(GetDatabase()).ZapSelectTx(vHashIn, vHashOut); - for (const uint256& hash : vHashOut) { - const auto& it = mapWallet.find(hash); + WalletBatch batch(GetDatabase()); + if (!batch.TxnBegin()) return util::Error{_("Error starting db txn for wallet transactions removal")}; + + // Check for transaction existence and remove entries from disk + using TxIterator = std::unordered_map<uint256, CWalletTx, SaltedTxidHasher>::const_iterator; + std::vector<TxIterator> erased_txs; + bilingual_str str_err; + for (const uint256& hash : txs_to_remove) { + auto it_wtx = mapWallet.find(hash); + if (it_wtx == mapWallet.end()) { + str_err = strprintf(_("Transaction %s does not belong to this wallet"), hash.GetHex()); + break; + } + if (!batch.EraseTx(hash)) { + str_err = strprintf(_("Failure removing transaction: %s"), hash.GetHex()); + break; + } + erased_txs.emplace_back(it_wtx); + } + + // Roll back removals in case of an error + if (!str_err.empty()) { + batch.TxnAbort(); + return util::Error{str_err}; + } + + // Dump changes to disk + if (!batch.TxnCommit()) return util::Error{_("Error committing db txn for wallet transactions removal")}; + + // Update the in-memory state and notify upper layers about the removals + for (const auto& it : erased_txs) { + const uint256 hash{it->first}; wtxOrdered.erase(it->second.m_it_wtxOrdered); for (const auto& txin : it->second.tx->vin) mapTxSpends.erase(txin.prevout); @@ -2333,22 +2403,9 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256 NotifyTransactionChanged(hash, CT_DELETED); } - if (nZapSelectTxRet == DBErrors::NEED_REWRITE) - { - if (GetDatabase().Rewrite("\x04pool")) - { - for (const auto& spk_man_pair : m_spk_managers) { - spk_man_pair.second->RewriteDB(); - } - } - } - - if (nZapSelectTxRet != DBErrors::LOAD_OK) - return nZapSelectTxRet; - MarkDirty(); - return DBErrors::LOAD_OK; + return {}; // all good } bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& new_purpose) @@ -2395,8 +2452,9 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s bool CWallet::DelAddressBook(const CTxDestination& address) { - WalletBatch batch(GetDatabase()); - return DelAddressBookWithDB(batch, address); + return RunWithinTxn(GetDatabase(), /*process_desc=*/"address book entry removal", [&](WalletBatch& batch){ + return DelAddressBookWithDB(batch, address); + }); } bool CWallet::DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address) @@ -2579,8 +2637,10 @@ util::Result<CTxDestination> ReserveDestination::GetReservedDestination(bool int if (nIndex == -1) { CKeyPool keypool; - auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool); + int64_t index; + auto op_address = m_spk_man->GetReservedDestination(type, internal, index, keypool); if (!op_address) return op_address; + nIndex = index; address = *op_address; fInternal = keypool.fInternal; } @@ -2605,7 +2665,7 @@ void ReserveDestination::ReturnDestination() address = CNoDestination(); } -bool CWallet::DisplayAddress(const CTxDestination& dest) +util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest) { CScript scriptPubKey = GetScriptForDestination(dest); for (const auto& spk_man : GetScriptPubKeyMans(scriptPubKey)) { @@ -2614,9 +2674,9 @@ bool CWallet::DisplayAddress(const CTxDestination& dest) continue; } ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); - return signer_spk_man->DisplayAddress(scriptPubKey, signer); + return signer_spk_man->DisplayAddress(dest, signer); } - return false; + return util::Error{_("There is no ScriptPubKeyManager for this address")}; } bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch) @@ -2757,7 +2817,7 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old std::optional<uint256> block_hash; if (auto* conf = wtx.state<TxStateConfirmed>()) { block_hash = conf->confirmed_block_hash; - } else if (auto* conf = wtx.state<TxStateConflicted>()) { + } else if (auto* conf = wtx.state<TxStateBlockConflicted>()) { block_hash = conf->conflicting_block_hash; } @@ -3347,7 +3407,7 @@ int CWallet::GetTxDepthInMainChain(const CWalletTx& wtx) const if (auto* conf = wtx.state<TxStateConfirmed>()) { assert(conf->confirmed_block_height >= 0); return GetLastBlockHeight() - conf->confirmed_block_height + 1; - } else if (auto* conf = wtx.state<TxStateConflicted>()) { + } else if (auto* conf = wtx.state<TxStateBlockConflicted>()) { assert(conf->conflicting_block_height >= 0); return -1 * (GetLastBlockHeight() - conf->conflicting_block_height + 1); } else { @@ -3435,6 +3495,17 @@ std::set<ScriptPubKeyMan*> CWallet::GetActiveScriptPubKeyMans() const return spk_mans; } +bool CWallet::IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const +{ + for (const auto& [_, ext_spkm] : m_external_spk_managers) { + if (ext_spkm == &spkm) return true; + } + for (const auto& [_, int_spkm] : m_internal_spk_managers) { + if (int_spkm == &spkm) return true; + } + return false; +} + std::set<ScriptPubKeyMan*> CWallet::GetAllScriptPubKeyMans() const { std::set<ScriptPubKeyMan*> spk_mans; @@ -3457,12 +3528,18 @@ ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const OutputType& type, bool intern std::set<ScriptPubKeyMan*> CWallet::GetScriptPubKeyMans(const CScript& script) const { std::set<ScriptPubKeyMan*> spk_mans; - SignatureData sigdata; - for (const auto& spk_man_pair : m_spk_managers) { - if (spk_man_pair.second->CanProvide(script, sigdata)) { - spk_mans.insert(spk_man_pair.second.get()); - } + + // Search the cache for relevant SPKMs instead of iterating m_spk_managers + const auto& it = m_cached_spks.find(script); + if (it != m_cached_spks.end()) { + spk_mans.insert(it->second.begin(), it->second.end()); } + SignatureData sigdata; + Assume(std::all_of(spk_mans.begin(), spk_mans.end(), [&script, &sigdata](ScriptPubKeyMan* spkm) { return spkm->CanProvide(script, sigdata); })); + + // Legacy wallet + if (IsLegacy() && GetLegacyScriptPubKeyMan()->CanProvide(script, sigdata)) spk_mans.insert(GetLegacyScriptPubKeyMan()); + return spk_mans; } @@ -3482,11 +3559,17 @@ std::unique_ptr<SigningProvider> CWallet::GetSolvingProvider(const CScript& scri std::unique_ptr<SigningProvider> CWallet::GetSolvingProvider(const CScript& script, SignatureData& sigdata) const { - for (const auto& spk_man_pair : m_spk_managers) { - if (spk_man_pair.second->CanProvide(script, sigdata)) { - return spk_man_pair.second->GetSolvingProvider(script); - } + // Search the cache for relevant SPKMs instead of iterating m_spk_managers + const auto& it = m_cached_spks.find(script); + if (it != m_cached_spks.end()) { + // All spkms for a given script must already be able to make a SigningProvider for the script, so just return the first one. + Assume(it->second.at(0)->CanProvide(script, sigdata)); + return it->second.at(0)->GetSolvingProvider(script); } + + // Legacy wallet + if (IsLegacy() && GetLegacyScriptPubKeyMan()->CanProvide(script, sigdata)) return GetLegacyScriptPubKeyMan()->GetSolvingProvider(script); + return nullptr; } @@ -3565,15 +3648,36 @@ void CWallet::ConnectScriptPubKeyManNotifiers() } } -void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) +DescriptorScriptPubKeyMan& CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) { + DescriptorScriptPubKeyMan* spk_manager; if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { - auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size)); - AddScriptPubKeyMan(id, std::move(spk_manager)); + spk_manager = new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size); } else { - auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size)); - AddScriptPubKeyMan(id, std::move(spk_manager)); + spk_manager = new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size); } + AddScriptPubKeyMan(id, std::unique_ptr<ScriptPubKeyMan>(spk_manager)); + return *spk_manager; +} + +DescriptorScriptPubKeyMan& CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal) +{ + AssertLockHeld(cs_wallet); + auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, m_keypool_size)); + if (IsCrypted()) { + if (IsLocked()) { + throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); + } + if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, &batch)) { + throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); + } + } + spk_manager->SetupDescriptorGeneration(batch, master_key, output_type, internal); + DescriptorScriptPubKeyMan* out = spk_manager.get(); + uint256 id = spk_manager->GetID(); + AddScriptPubKeyMan(id, std::move(spk_manager)); + AddActiveScriptPubKeyManWithDb(batch, id, output_type, internal); + return *out; } void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) @@ -3586,19 +3690,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) for (bool internal : {false, true}) { for (OutputType t : OUTPUT_TYPES) { - auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, m_keypool_size)); - if (IsCrypted()) { - if (IsLocked()) { - throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); - } - if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, &batch)) { - throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); - } - } - spk_manager->SetupDescriptorGeneration(batch, master_key, t, internal); - uint256 id = spk_manager->GetID(); - AddScriptPubKeyMan(id, std::move(spk_manager)); - AddActiveScriptPubKeyManWithDb(batch, id, t, internal); + SetupDescriptorScriptPubKeyMan(batch, master_key, t, internal); } } @@ -3928,6 +4020,8 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest); } + Assume(!m_cached_spks.empty()); + for (auto& desc_spkm : data.desc_spkms) { if (m_spk_managers.count(desc_spkm->GetID()) > 0) { error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted."); @@ -4025,19 +4119,10 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { - std::vector<uint256> deleted_txids; - if (ZapSelectTx(txids_to_delete, deleted_txids) != DBErrors::LOAD_OK) { - error = _("Error: Could not delete watchonly transactions"); - return false; - } - if (deleted_txids != txids_to_delete) { - error = _("Error: Not all watchonly txs could be deleted"); + if (auto res = RemoveTxs(txids_to_delete); !res) { + error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); return false; } - // Tell the GUI of each tx - for (const uint256& txid : deleted_txids) { - NotifyTransactionChanged(txid, CT_UPDATED); - } } // Pair external wallets with their corresponding db handler @@ -4300,7 +4385,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle // Make a backup of the DB fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path(); - fs::path backup_filename = fs::PathFromString(strprintf("%s-%d.legacy.bak", wallet_name, GetTime())); + fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime())); fs::path backup_path = this_wallet_dir / backup_filename; if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) { if (was_loaded) { @@ -4420,4 +4505,53 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle } return res; } + +void CWallet::CacheNewScriptPubKeys(const std::set<CScript>& spks, ScriptPubKeyMan* spkm) +{ + for (const auto& script : spks) { + m_cached_spks[script].push_back(spkm); + } +} + +void CWallet::TopUpCallback(const std::set<CScript>& spks, ScriptPubKeyMan* spkm) +{ + // Update scriptPubKey cache + CacheNewScriptPubKeys(spks, spkm); +} + +std::set<CExtPubKey> CWallet::GetActiveHDPubKeys() const +{ + AssertLockHeld(cs_wallet); + + Assert(IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + + std::set<CExtPubKey> active_xpubs; + for (const auto& spkm : GetActiveScriptPubKeyMans()) { + const DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(spkm); + assert(desc_spkm); + LOCK(desc_spkm->cs_desc_man); + WalletDescriptor w_desc = desc_spkm->GetWalletDescriptor(); + + std::set<CPubKey> desc_pubkeys; + std::set<CExtPubKey> desc_xpubs; + w_desc.descriptor->GetPubKeys(desc_pubkeys, desc_xpubs); + active_xpubs.merge(std::move(desc_xpubs)); + } + return active_xpubs; +} + +std::optional<CKey> CWallet::GetKey(const CKeyID& keyid) const +{ + Assert(IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + + for (const auto& spkm : GetAllScriptPubKeyMans()) { + const DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(spkm); + assert(desc_spkm); + LOCK(desc_spkm->cs_desc_man); + if (std::optional<CKey> key = desc_spkm->GetKey(keyid)) { + return key; + } + } + return std::nullopt; +} } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c0d10ab7fc..6a998fa398 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -364,6 +364,7 @@ private: /** Mark a transaction (and its in-wallet descendants) as a particular tx state. */ void RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void RecursiveUpdateTxState(WalletBatch* batch, const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Mark a transaction's inputs dirty, thus forcing the outputs to be recomputed */ void MarkInputsDirty(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -422,6 +423,9 @@ private: // Same as 'AddActiveScriptPubKeyMan' but designed for use within a batch transaction context void AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal); + //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms + std::unordered_map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks; + /** * Catch wallet up to current chain, scanning new blocks, updating the best * block locator and m_last_block_processed, and registering for @@ -515,11 +519,6 @@ public: * referenced in transaction, and might cause assert failures. */ int GetTxDepthInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool IsTxInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) - { - AssertLockHeld(cs_wallet); - return GetTxDepthInMainChain(wtx) > 0; - } /** * @return number of blocks to maturity for this transaction: @@ -538,8 +537,8 @@ public: bool IsSpentKey(const CScript& scriptPubKey) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** Display address on an external signer. Returns false if external signer support is not compiled */ - bool DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Display address on an external signer. */ + util::Result<void> DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -790,7 +789,9 @@ public: void chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) override; DBErrors LoadWallet(); - DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** Erases the provided transactions from the wallet. */ + util::Result<void> RemoveTxs(std::vector<uint256>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose); @@ -937,6 +938,7 @@ public: //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const; + bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const; //! Returns all unique ScriptPubKeyMans std::set<ScriptPubKeyMan*> GetAllScriptPubKeyMans() const; @@ -992,7 +994,7 @@ public: void ConnectScriptPubKeyManNotifiers(); //! Instantiate a descriptor ScriptPubKeyMan from the WalletDescriptor and load it - void LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc); + DescriptorScriptPubKeyMan& LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc); //! Adds the active ScriptPubKeyMan for the specified type and internal. Writes it to the wallet file //! @param[in] id The unique id for the ScriptPubKeyMan @@ -1012,6 +1014,8 @@ public: //! @param[in] internal Whether this ScriptPubKeyMan provides change addresses void DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool internal); + //! Create new DescriptorScriptPubKeyMan and add it to the wallet + DescriptorScriptPubKeyMan& SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Create new DescriptorScriptPubKeyMans and add them to the wallet void SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -1043,6 +1047,18 @@ public: //! Whether the (external) signer performs R-value signature grinding bool CanGrindR() const; + + //! Add scriptPubKeys for this ScriptPubKeyMan into the scriptPubKey cache + void CacheNewScriptPubKeys(const std::set<CScript>& spks, ScriptPubKeyMan* spkm); + + void TopUpCallback(const std::set<CScript>& spks, ScriptPubKeyMan* spkm) override; + + //! Retrieve the xpubs in use by the active descriptors + std::set<CExtPubKey> GetActiveHDPubKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + //! Find the private key for the given key id from the wallet's descriptors, if available + //! Returns nullopt when no descriptor has the key or if the wallet is locked. + std::optional<CKey> GetKey(const CKeyID& keyid) const; }; /** diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index f3dd5b328e..3ba43cdb73 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -3,6 +3,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <config/bitcoin-config.h> // IWYU pragma: keep + #include <wallet/walletdb.h> #include <common/system.h> @@ -804,10 +806,10 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat strErr = strprintf("%s\nDetails: %s", strErr, e.what()); return DBErrors::UNKNOWN_DESCRIPTOR; } - pwallet->LoadDescriptorScriptPubKeyMan(id, desc); + DescriptorScriptPubKeyMan& spkm = pwallet->LoadDescriptorScriptPubKeyMan(id, desc); // Prior to doing anything with this spkm, verify ID compatibility - if (id != pwallet->GetDescriptorScriptPubKeyMan(desc)->GetID()) { + if (id != spkm.GetID()) { strErr = "The descriptor ID calculated by the wallet differs from the one in DB"; return DBErrors::CORRUPT; } @@ -1230,88 +1232,33 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } -DBErrors WalletBatch::FindWalletTxHashes(std::vector<uint256>& tx_hashes) +static bool RunWithinTxn(WalletBatch& batch, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func) { - DBErrors result = DBErrors::LOAD_OK; - - try { - int nMinVersion = 0; - if (m_batch->Read(DBKeys::MINVERSION, nMinVersion)) { - if (nMinVersion > FEATURE_LATEST) - return DBErrors::TOO_NEW; - } - - // Get cursor - std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor(); - if (!cursor) - { - LogPrintf("Error getting wallet database cursor\n"); - return DBErrors::CORRUPT; - } + if (!batch.TxnBegin()) { + LogPrint(BCLog::WALLETDB, "Error: cannot create db txn for %s\n", process_desc); + return false; + } - while (true) - { - // Read next record - DataStream ssKey{}; - DataStream ssValue{}; - DatabaseCursor::Status status = cursor->Next(ssKey, ssValue); - if (status == DatabaseCursor::Status::DONE) { - break; - } else if (status == DatabaseCursor::Status::FAIL) { - LogPrintf("Error reading next record from wallet database\n"); - return DBErrors::CORRUPT; - } + // Run procedure + if (!func(batch)) { + LogPrint(BCLog::WALLETDB, "Error: %s failed\n", process_desc); + batch.TxnAbort(); + return false; + } - std::string strType; - ssKey >> strType; - if (strType == DBKeys::TX) { - uint256 hash; - ssKey >> hash; - tx_hashes.push_back(hash); - } - } - } catch (...) { - result = DBErrors::CORRUPT; + if (!batch.TxnCommit()) { + LogPrint(BCLog::WALLETDB, "Error: cannot commit db txn for %s\n", process_desc); + return false; } - return result; + // All good + return true; } -DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<uint256>& vTxHashOut) +bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func) { - // build list of wallet TX hashes - std::vector<uint256> vTxHash; - DBErrors err = FindWalletTxHashes(vTxHash); - if (err != DBErrors::LOAD_OK) { - return err; - } - - std::sort(vTxHash.begin(), vTxHash.end()); - std::sort(vTxHashIn.begin(), vTxHashIn.end()); - - // erase each matching wallet TX - bool delerror = false; - std::vector<uint256>::iterator it = vTxHashIn.begin(); - for (const uint256& hash : vTxHash) { - while (it < vTxHashIn.end() && (*it) < hash) { - it++; - } - if (it == vTxHashIn.end()) { - break; - } - else if ((*it) == hash) { - if(!EraseTx(hash)) { - LogPrint(BCLog::WALLETDB, "Transaction was found for deletion but returned database error: %s\n", hash.GetHex()); - delerror = true; - } - vTxHashOut.push_back(hash); - } - } - - if (delerror) { - return DBErrors::CORRUPT; - } - return DBErrors::LOAD_OK; + WalletBatch batch(database); + return RunWithinTxn(batch, process_desc, func); } void MaybeCompactWalletDB(WalletContext& context) @@ -1376,47 +1323,11 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags) bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types) { - // Begin db txn - if (!m_batch->TxnBegin()) return false; - - // Get cursor - std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor(); - if (!cursor) - { - return false; - } - - // Iterate the DB and look for any records that have the type prefixes - while (true) { - // Read next record - DataStream key{}; - DataStream value{}; - DatabaseCursor::Status status = cursor->Next(key, value); - if (status == DatabaseCursor::Status::DONE) { - break; - } else if (status == DatabaseCursor::Status::FAIL) { - cursor.reset(nullptr); - m_batch->TxnAbort(); // abort db txn - return false; - } - - // Make a copy of key to avoid data being deleted by the following read of the type - const SerializeData key_data{key.begin(), key.end()}; - - std::string type; - key >> type; - - if (types.count(type) > 0) { - if (!m_batch->Erase(Span{key_data})) { - cursor.reset(nullptr); - m_batch->TxnAbort(); - return false; // erase failed - } - } - } - // Finish db txn - cursor.reset(nullptr); - return m_batch->TxnCommit(); + return RunWithinTxn(*this, "erase records", [&types](WalletBatch& self) { + return std::all_of(types.begin(), types.end(), [&self](const std::string& type) { + return self.m_batch->ErasePrefix(DataStream() << type); + }); + }); } bool WalletBatch::TxnBegin() diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index dad0b18a78..9474a59660 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -275,8 +275,6 @@ public: bool EraseActiveScriptPubKeyMan(uint8_t type, bool internal); DBErrors LoadWallet(CWallet* pwallet); - DBErrors FindWalletTxHashes(std::vector<uint256>& tx_hashes); - DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut); //! write the hdchain model (external chain child index counter) bool WriteHDChain(const CHDChain& chain); @@ -296,6 +294,20 @@ private: WalletDatabase& m_database; }; +/** + * Executes the provided function 'func' within a database transaction context. + * + * This function ensures that all db modifications performed within 'func()' are + * atomically committed to the db at the end of the process. And, in case of a + * failure during execution, all performed changes are rolled back. + * + * @param database The db connection instance to perform the transaction on. + * @param process_desc A description of the process being executed, used for logging purposes in the event of a failure. + * @param func The function to be executed within the db txn context. It returns a boolean indicating whether to commit or roll back the txn. + * @return true if the db txn executed successfully, false otherwise. + */ +bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func); + //! Compacts BDB state so that wallet.dat is self-contained (if there are changes) void MaybeCompactWalletDB(WalletContext& context); diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index cda344ab19..7a1930fd31 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -2,9 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#if defined(HAVE_CONFIG_H) -#include <config/bitcoin-config.h> -#endif +#include <config/bitcoin-config.h> // IWYU pragma: keep #include <wallet/wallettool.h> diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp index fdd5bc36d8..0de2617d45 100644 --- a/src/wallet/walletutil.cpp +++ b/src/wallet/walletutil.cpp @@ -4,7 +4,9 @@ #include <wallet/walletutil.h> +#include <chainparams.h> #include <common/args.h> +#include <key_io.h> #include <logging.h> namespace wallet { @@ -43,4 +45,58 @@ WalletFeature GetClosestWalletFeature(int version) } return static_cast<WalletFeature>(0); } + +WalletDescriptor GenerateWalletDescriptor(const CExtPubKey& master_key, const OutputType& addr_type, bool internal) +{ + int64_t creation_time = GetTime(); + + std::string xpub = EncodeExtPubKey(master_key); + + // Build descriptor string + std::string desc_prefix; + std::string desc_suffix = "/*)"; + switch (addr_type) { + case OutputType::LEGACY: { + desc_prefix = "pkh(" + xpub + "/44h"; + break; + } + case OutputType::P2SH_SEGWIT: { + desc_prefix = "sh(wpkh(" + xpub + "/49h"; + desc_suffix += ")"; + break; + } + case OutputType::BECH32: { + desc_prefix = "wpkh(" + xpub + "/84h"; + break; + } + case OutputType::BECH32M: { + desc_prefix = "tr(" + xpub + "/86h"; + break; + } + case OutputType::UNKNOWN: { + // We should never have a DescriptorScriptPubKeyMan for an UNKNOWN OutputType, + // so if we get to this point something is wrong + assert(false); + } + } // no default case, so the compiler can warn about missing cases + assert(!desc_prefix.empty()); + + // Mainnet derives at 0', testnet and regtest derive at 1' + if (Params().IsTestChain()) { + desc_prefix += "/1h"; + } else { + desc_prefix += "/0h"; + } + + std::string internal_path = internal ? "/1" : "/0"; + std::string desc_str = desc_prefix + "/0h" + internal_path + desc_suffix; + + // Make the descriptor + FlatSigningProvider keys; + std::string error; + std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false); + WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); + return w_desc; +} + } // namespace wallet diff --git a/src/wallet/walletutil.h b/src/wallet/walletutil.h index 7ad3ffe9e4..38926c1eb8 100644 --- a/src/wallet/walletutil.h +++ b/src/wallet/walletutil.h @@ -114,6 +114,8 @@ public: WalletDescriptor() {} WalletDescriptor(std::shared_ptr<Descriptor> descriptor, uint64_t creation_time, int32_t range_start, int32_t range_end, int32_t next_index) : descriptor(descriptor), id(DescriptorID(*descriptor)), creation_time(creation_time), range_start(range_start), range_end(range_end), next_index(next_index) { } }; + +WalletDescriptor GenerateWalletDescriptor(const CExtPubKey& master_key, const OutputType& output_type, bool internal); } // namespace wallet #endif // BITCOIN_WALLET_WALLETUTIL_H |