diff options
Diffstat (limited to 'src/wallet')
45 files changed, 2151 insertions, 670 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/db.h b/src/wallet/db.h index c263f54144..084fcadc24 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -20,7 +20,6 @@ class ArgsManager; struct bilingual_str; namespace wallet { -void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename); class DatabaseCursor { diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 3ac5cf03b1..7a36910dc1 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -8,6 +8,7 @@ #include <util/fs.h> #include <util/translation.h> #include <wallet/wallet.h> +#include <wallet/walletdb.h> #include <algorithm> #include <fstream> @@ -20,7 +21,7 @@ namespace wallet { static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP"; uint32_t DUMP_VERSION = 1; -bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error) +bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error) { // Get the dumpfile std::string dump_filename = args.GetArg("-dumpfile", ""); @@ -44,7 +45,6 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error) HashWriter hasher{}; - WalletDatabase& db = wallet.GetDatabase(); std::unique_ptr<DatabaseBatch> batch = db.MakeBatch(); bool ret = true; @@ -90,9 +90,6 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error) cursor.reset(); batch.reset(); - // Close the wallet after we're done with it. The caller won't be doing this - wallet.Close(); - if (ret) { // Write the hash tfm::format(dump_file, "checksum,%s\n", HexStr(hasher.GetHash())); diff --git a/src/wallet/dump.h b/src/wallet/dump.h index 5034f95479..9b44af922e 100644 --- a/src/wallet/dump.h +++ b/src/wallet/dump.h @@ -14,8 +14,9 @@ struct bilingual_str; class ArgsManager; namespace wallet { -class CWallet; -bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error); +class WalletDatabase; + +bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error); bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector<bilingual_str>& warnings); } // namespace wallet 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 99c548bf1d..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; @@ -460,12 +456,7 @@ RPCHelpMan importpubkey() throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } - if (!IsHex(request.params[0].get_str())) - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string"); - std::vector<unsigned char> data(ParseHex(request.params[0].get_str())); - CPubKey pubKey(data); - if (!pubKey.IsFullyValid()) - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key"); + CPubKey pubKey = HexToPubKey(request.params[0].get_str()); { LOCK(pwallet->cs_wallet); @@ -856,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: @@ -986,15 +978,7 @@ static UniValue ProcessImportLegacy(ImportData& import_data, std::map<CKeyID, CP import_data.witnessscript = std::make_unique<CScript>(parsed_witnessscript.begin(), parsed_witnessscript.end()); } for (size_t i = 0; i < pubKeys.size(); ++i) { - const auto& str = pubKeys[i].get_str(); - if (!IsHex(str)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + str + "\" must be a hex string"); - } - auto parsed_pubkey = ParseHex(str); - CPubKey pubkey(parsed_pubkey); - if (!pubkey.IsFullyValid()) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + str + "\" is not a valid public key"); - } + CPubKey pubkey = HexToPubKey(pubKeys[i].get_str()); pubkey_map.emplace(pubkey.GetID(), pubkey); ordered_pubkeys.push_back(pubkey.GetID()); } 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/spend.cpp b/src/wallet/rpc/spend.cpp index 5a13b5ac8e..1a364a75ed 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -24,34 +24,15 @@ namespace wallet { -static void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector<CRecipient>& recipients) +std::vector<CRecipient> CreateRecipients(const std::vector<std::pair<CTxDestination, CAmount>>& outputs, const std::set<int>& subtract_fee_outputs) { - std::set<CTxDestination> destinations; - int i = 0; - for (const std::string& address: address_amounts.getKeys()) { - CTxDestination dest = DecodeDestination(address); - if (!IsValidDestination(dest)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + address); - } - - if (destinations.count(dest)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); - } - destinations.insert(dest); - - CAmount amount = AmountFromValue(address_amounts[i++]); - - bool subtract_fee = false; - for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) { - const UniValue& addr = subtract_fee_outputs[idx]; - if (addr.get_str() == address) { - subtract_fee = true; - } - } - - CRecipient recipient = {dest, amount, subtract_fee}; + std::vector<CRecipient> recipients; + for (size_t i = 0; i < outputs.size(); ++i) { + const auto& [destination, amount] = outputs.at(i); + CRecipient recipient{destination, amount, subtract_fee_outputs.contains(i)}; recipients.push_back(recipient); } + return recipients; } static void InterpretFeeEstimationInstructions(const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, UniValue& options) @@ -76,6 +57,37 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons } } +std::set<int> InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_instructions, const std::vector<std::string>& destinations) +{ + std::set<int> sffo_set; + if (sffo_instructions.isNull()) return sffo_set; + if (sffo_instructions.isBool()) { + if (sffo_instructions.get_bool()) sffo_set.insert(0); + return sffo_set; + } + for (const auto& sffo : sffo_instructions.getValues()) { + if (sffo.isStr()) { + for (size_t i = 0; i < destinations.size(); ++i) { + if (sffo.get_str() == destinations.at(i)) { + sffo_set.insert(i); + break; + } + } + } + if (sffo.isNum()) { + int pos = sffo.getInt<int>(); + if (sffo_set.contains(pos)) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, duplicated position: %d", pos)); + if (pos < 0) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, negative position: %d", pos)); + if (pos >= int(destinations.size())) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos)); + sffo_set.insert(pos); + } + } + return sffo_set; +} + static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, const CMutableTransaction& rawTx) { // Make a blank psbt @@ -275,11 +287,6 @@ RPCHelpMan sendtoaddress() if (!request.params[3].isNull() && !request.params[3].get_str().empty()) mapValue["to"] = request.params[3].get_str(); - bool fSubtractFeeFromAmount = false; - if (!request.params[4].isNull()) { - fSubtractFeeFromAmount = request.params[4].get_bool(); - } - CCoinControl coin_control; if (!request.params[5].isNull()) { coin_control.m_signal_bip125_rbf = request.params[5].get_bool(); @@ -296,13 +303,10 @@ RPCHelpMan sendtoaddress() UniValue address_amounts(UniValue::VOBJ); const std::string address = request.params[0].get_str(); address_amounts.pushKV(address, request.params[1]); - UniValue subtractFeeFromAmount(UniValue::VARR); - if (fSubtractFeeFromAmount) { - subtractFeeFromAmount.push_back(address); - } - - std::vector<CRecipient> recipients; - ParseRecipients(address_amounts, subtractFeeFromAmount, recipients); + std::vector<CRecipient> recipients = CreateRecipients( + ParseOutputs(address_amounts), + InterpretSubtractFeeFromOutputInstructions(request.params[4], address_amounts.getKeys()) + ); const bool verbose{request.params[10].isNull() ? false : request.params[10].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose); @@ -386,10 +390,6 @@ RPCHelpMan sendmany() if (!request.params[3].isNull() && !request.params[3].get_str().empty()) mapValue["comment"] = request.params[3].get_str(); - UniValue subtractFeeFromAmount(UniValue::VARR); - if (!request.params[4].isNull()) - subtractFeeFromAmount = request.params[4].get_array(); - CCoinControl coin_control; if (!request.params[5].isNull()) { coin_control.m_signal_bip125_rbf = request.params[5].get_bool(); @@ -397,8 +397,10 @@ RPCHelpMan sendmany() SetFeeEstimateMode(*pwallet, coin_control, /*conf_target=*/request.params[6], /*estimate_mode=*/request.params[7], /*fee_rate=*/request.params[8], /*override_min_fee=*/false); - std::vector<CRecipient> recipients; - ParseRecipients(sendTo, subtractFeeFromAmount, recipients); + std::vector<CRecipient> recipients = CreateRecipients( + ParseOutputs(sendTo), + InterpretSubtractFeeFromOutputInstructions(request.params[4], sendTo.getKeys()) + ); const bool verbose{request.params[9].isNull() ? false : request.params[9].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, std::move(mapValue), verbose); @@ -488,17 +490,17 @@ static std::vector<RPCArg> FundTxDoc(bool solving_data = true) return args; } -CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) +CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector<CRecipient>& recipients, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) { + // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients. + // This sets us up to remove tx completely in a future PR in favor of passing the inputs directly. + CHECK_NONFATAL(tx.vout.empty()); // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); std::optional<unsigned int> change_position; bool lockUnspents = false; - UniValue subtractFeeFromOutputs; - std::set<int> setSubtractFeeFromOutputs; - if (!options.isNull()) { if (options.type() == UniValue::VBOOL) { // backward compatibility bool only fallback @@ -553,7 +555,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact if (options.exists("changePosition") || options.exists("change_position")) { int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>(); - if (pos < 0 || (unsigned int)pos > tx.vout.size()) { + if (pos < 0 || (unsigned int)pos > recipients.size()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); } change_position = (unsigned int)pos; @@ -595,9 +597,6 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact coinControl.fOverrideFeeRate = true; } - if (options.exists("subtractFeeFromOutputs") || options.exists("subtract_fee_from_outputs") ) - subtractFeeFromOutputs = (options.exists("subtract_fee_from_outputs") ? options["subtract_fee_from_outputs"] : options["subtractFeeFromOutputs"]).get_array(); - if (options.exists("replaceable")) { coinControl.m_signal_bip125_rbf = options["replaceable"].get_bool(); } @@ -628,15 +627,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact const UniValue solving_data = options["solving_data"].get_obj(); if (solving_data.exists("pubkeys")) { for (const UniValue& pk_univ : solving_data["pubkeys"].get_array().getValues()) { - const std::string& pk_str = pk_univ.get_str(); - if (!IsHex(pk_str)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", pk_str)); - } - const std::vector<unsigned char> data(ParseHex(pk_str)); - const CPubKey pubkey(data.begin(), data.end()); - if (!pubkey.IsFullyValid()) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not a valid public key", pk_str)); - } + const CPubKey pubkey = HexToPubKey(pk_univ.get_str()); coinControl.m_external_provider.pubkeys.emplace(pubkey.GetID(), pubkey); // Add witness script for pubkeys const CScript wit_script = GetScriptForDestination(WitnessV0KeyHash(pubkey)); @@ -703,21 +694,10 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact } } - if (tx.vout.size() == 0) + if (recipients.empty()) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); - for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) { - int pos = subtractFeeFromOutputs[idx].getInt<int>(); - if (setSubtractFeeFromOutputs.count(pos)) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, duplicated position: %d", pos)); - if (pos < 0) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, negative position: %d", pos)); - if (pos >= int(tx.vout.size())) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos)); - setSubtractFeeFromOutputs.insert(pos); - } - - auto txr = FundTransaction(wallet, tx, change_position, lockUnspents, setSubtractFeeFromOutputs, coinControl); + auto txr = FundTransaction(wallet, tx, recipients, change_position, lockUnspents, coinControl); if (!txr) { throw JSONRPCError(RPC_WALLET_ERROR, ErrorString(txr).original); } @@ -843,11 +823,25 @@ RPCHelpMan fundrawtransaction() if (!DecodeHexTx(tx, request.params[0].get_str(), try_no_witness, try_witness)) { throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } - + UniValue options = request.params[1]; + std::vector<std::pair<CTxDestination, CAmount>> destinations; + for (const auto& tx_out : tx.vout) { + CTxDestination dest; + ExtractDestination(tx_out.scriptPubKey, dest); + destinations.emplace_back(dest, tx_out.nValue); + } + std::vector<std::string> dummy(destinations.size(), "dummy"); + std::vector<CRecipient> recipients = CreateRecipients( + destinations, + InterpretSubtractFeeFromOutputInstructions(options["subtractFeeFromOutputs"], dummy) + ); CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. coin_control.m_allow_other_inputs = true; - auto txr = FundTransaction(*pwallet, tx, request.params[1], coin_control, /*override_min_fee=*/true); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + tx.vout.clear(); + auto txr = FundTransaction(*pwallet, tx, recipients, options, coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); result.pushKV("hex", EncodeHexTx(*txr.tx)); @@ -1275,13 +1269,22 @@ RPCHelpMan send() bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf}; + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(request.params[0]); + std::vector<CRecipient> recipients = CreateRecipients( + ParseOutputs(outputs), + InterpretSubtractFeeFromOutputInstructions(options["subtract_fee_from_outputs"], outputs.getKeys()) + ); CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf); CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); - auto txr = FundTransaction(*pwallet, rawTx, options, coin_control, /*override_min_fee=*/false); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + rawTx.vout.clear(); + auto txr = FundTransaction(*pwallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/false); return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); } @@ -1711,12 +1714,21 @@ RPCHelpMan walletcreatefundedpsbt() const UniValue &replaceable_arg = options["replaceable"]; const bool rbf{replaceable_arg.isNull() ? wallet.m_signal_rbf : replaceable_arg.get_bool()}; CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf); + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(request.params[1]); + std::vector<CRecipient> recipients = CreateRecipients( + ParseOutputs(outputs), + InterpretSubtractFeeFromOutputInstructions(options["subtractFeeFromOutputs"], outputs.getKeys()) + ); CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); - auto txr = FundTransaction(wallet, rawTx, options, coin_control, /*override_min_fee=*/true); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + rawTx.vout.clear(); + auto txr = FundTransaction(wallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/true); // Make a blank psbt PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx)); diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index cd7ed461f0..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}); @@ -415,8 +419,12 @@ static std::vector<RPCResult> TransactionDescriptionString() { {RPCResult::Type::STR_HEX, "txid", "The transaction id."}, }}, - {RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "The txid if this tx was replaced."}, - {RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "The txid if the tx replaces one."}, + {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 + "."}, @@ -783,8 +791,7 @@ RPCHelpMan gettransaction() ListTransactions(*pwallet, wtx, 0, false, details, filter, /*filter_label=*/std::nullopt); entry.pushKV("details", details); - std::string strHex = EncodeHexTx(*wtx.tx, pwallet->chain().rpcSerializationWithoutWitness()); - entry.pushKV("hex", strHex); + entry.pushKV("hex", EncodeHexTx(*wtx.tx)); if (verbose) { UniValue decoded(UniValue::VOBJ); 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 70705667b0..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; @@ -225,7 +227,7 @@ isminetype LegacyScriptPubKeyMan::IsMine(const CScript& script) const assert(false); } -bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys) +bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key) { { LOCK(cs_KeyStore); @@ -258,7 +260,7 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key LogPrintf("The wallet is probably corrupted: Some keys decrypt but not all.\n"); throw std::runtime_error("Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt."); } - if (keyFail || (!keyPass && !accept_no_keys)) + if (keyFail || !keyPass) return false; fDecryptionThoroughlyChecked = true; } @@ -280,7 +282,7 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat { const CKey &key = mKey.second; CPubKey vchPubKey = key.GetPubKey(); - CKeyingMaterial vchSecret(key.begin(), key.end()); + CKeyingMaterial vchSecret{UCharCast(key.begin()), UCharCast(key.end())}; std::vector<unsigned char> vchCryptedSecret; if (!EncryptSecret(master_key, vchSecret, vchPubKey.GetHash(), vchCryptedSecret)) { encrypted_batch = nullptr; @@ -652,8 +654,6 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb // There's no UTXO so we can just skip this now continue; } - SignatureData sigdata; - input.FillSignatureData(sigdata); SignPSBTInput(HidingSigningProvider(this, !sign, !bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize); bool signed_one = PSBTInputSigned(input); @@ -810,8 +810,10 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu } std::vector<unsigned char> vchCryptedSecret; - CKeyingMaterial vchSecret(key.begin(), key.end()); - if (!EncryptSecret(m_storage.GetEncryptionKey(), vchSecret, pubkey.GetHash(), vchCryptedSecret)) { + CKeyingMaterial vchSecret{UCharCast(key.begin()), UCharCast(key.end())}; + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, vchSecret, pubkey.GetHash(), vchCryptedSecret); + })) { return false; } @@ -997,7 +999,9 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const { const CPubKey &vchPubKey = (*mi).second.first; const std::vector<unsigned char> &vchCryptedSecret = (*mi).second.second; - return DecryptKey(m_storage.GetEncryptionKey(), vchCryptedSecret, vchPubKey, keyOut); + return m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, vchCryptedSecret, vchPubKey, keyOut); + }); } return false; } @@ -1183,8 +1187,7 @@ bool LegacyScriptPubKeyMan::CanGenerateKeys() const CPubKey LegacyScriptPubKeyMan::GenerateNewSeed() { assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); return DeriveNewSeed(key); } @@ -1287,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; } @@ -2046,7 +2052,7 @@ isminetype DescriptorScriptPubKeyMan::IsMine(const CScript& script) const return ISMINE_NO; } -bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys) +bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key) { LOCK(cs_desc_man); if (!m_map_keys.empty()) { @@ -2071,7 +2077,7 @@ bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master LogPrintf("The wallet is probably corrupted: Some keys decrypt but not all.\n"); throw std::runtime_error("Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt."); } - if (keyFail || (!keyPass && !accept_no_keys)) { + if (keyFail || !keyPass) { return false; } m_decryption_thoroughly_checked = true; @@ -2089,7 +2095,7 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle { const CKey &key = key_in.second; CPubKey pubkey = key.GetPubKey(); - CKeyingMaterial secret(key.begin(), key.end()); + CKeyingMaterial secret{UCharCast(key.begin()), UCharCast(key.end())}; std::vector<unsigned char> crypted_secret; if (!EncryptSecret(master_key, secret, pubkey.GetHash(), crypted_secret)) { return false; @@ -2129,7 +2135,9 @@ std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const const CPubKey& pubkey = key_pair.second.first; const std::vector<unsigned char>& crypted_secret = key_pair.second.second; CKey key; - DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, pubkey, key); + m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, crypted_secret, pubkey, key); + }); keys[pubkey.GetID()] = key; } return keys; @@ -2137,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()); @@ -2149,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; @@ -2179,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; } @@ -2204,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; } @@ -2262,8 +2303,10 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } std::vector<unsigned char> crypted_secret; - CKeyingMaterial secret(key.begin(), key.end()); - if (!EncryptSecret(m_storage.GetEncryptionKey(), secret, pubkey.GetHash(), crypted_secret)) { + CKeyingMaterial secret{UCharCast(key.begin()), UCharCast(key.end())}; + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, secret, pubkey.GetHash(), crypted_secret); + })) { return false; } @@ -2285,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())) { @@ -2521,8 +2516,6 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& // There's no UTXO so we can just skip this now continue; } - SignatureData sigdata; - input.FillSignatureData(sigdata); std::unique_ptr<FlatSigningProvider> keys = std::make_unique<FlatSigningProvider>(); std::unique_ptr<FlatSigningProvider> script_keys = GetSigningProvider(script, /*include_private=*/sign); @@ -2617,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; @@ -2625,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])); @@ -2642,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 449a75eb6b..2c1ab8d44a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -22,14 +22,15 @@ #include <boost/signals2/signal.hpp> +#include <functional> #include <optional> #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 @@ -46,9 +47,12 @@ public: virtual void UnsetBlankWalletFlag(WalletBatch&) = 0; virtual bool CanSupportFeature(enum WalletFeature) const = 0; virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0; - virtual const CKeyingMaterial& GetEncryptionKey() const = 0; + //! Pass the encryption key to cb(). + 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 @@ -177,7 +181,7 @@ public: virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } //! Check that the given decryption key is valid for this ScriptPubKeyMan, i.e. it decrypts all of the keys handled by it. - virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; } + virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key) { return false; } virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; } virtual util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; } @@ -377,7 +381,7 @@ public: util::Result<CTxDestination> GetNewDestination(const OutputType type) override; isminetype IsMine(const CScript& script) const override; - bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; + bool CheckDecryptionKey(const CKeyingMaterial& master_key) override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override; @@ -608,7 +612,7 @@ public: util::Result<CTxDestination> GetNewDestination(const OutputType type) override; isminetype IsMine(const CScript& script) const override; - bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; + bool CheckDecryptionKey(const CKeyingMaterial& master_key) override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override; @@ -628,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; @@ -664,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 b51cd6332f..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) @@ -1127,7 +1134,12 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( return util::Error{err.empty() ?_("Insufficient funds") : err}; } const SelectionResult& result = *select_coins_res; - TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result.GetAlgo()).c_str(), result.GetTarget(), result.GetWaste(), result.GetSelectedValue()); + TRACE5(coin_selection, selected_coins, + wallet.GetName().c_str(), + GetAlgorithmName(result.GetAlgo()).c_str(), + result.GetTarget(), + result.GetWaste(), + result.GetSelectedValue()); const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee); if (change_amount > 0) { @@ -1336,8 +1348,11 @@ util::Result<CreatedTransactionResult> CreateTransaction( LOCK(wallet.cs_wallet); auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign); - TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res), - res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0); + TRACE4(coin_selection, normal_create_tx_internal, + wallet.GetName().c_str(), + bool(res), + res ? res->fee : 0, + res && res->change_pos.has_value() ? int32_t(*res->change_pos) : -1); if (!res) return res; const auto& txr_ungrouped = *res; // try with avoidpartialspends unless it's enabled already @@ -1354,8 +1369,12 @@ util::Result<CreatedTransactionResult> CreateTransaction( auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign); // if fee of this alternative one is within the range of the max fee, we use this one const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false}; - TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(), - txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0); + TRACE5(coin_selection, aps_create_tx_internal, + wallet.GetName().c_str(), + use_aps, + txr_grouped.has_value(), + txr_grouped.has_value() ? txr_grouped->fee : 0, + txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? int32_t(*txr_grouped->change_pos) : -1); if (txr_grouped) { wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped"); @@ -1365,18 +1384,11 @@ util::Result<CreatedTransactionResult> CreateTransaction( return res; } -util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl) +util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, bool lockUnspents, CCoinControl coinControl) { - std::vector<CRecipient> vecSend; - - // Turn the txout set into a CRecipient vector. - for (size_t idx = 0; idx < tx.vout.size(); idx++) { - const CTxOut& txOut = tx.vout[idx]; - CTxDestination dest; - ExtractDestination(txOut.scriptPubKey, dest); - CRecipient recipient = {dest, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1}; - vecSend.push_back(recipient); - } + // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients. + // This sets us up to remove tx completely in a future PR in favor of passing the inputs directly. + assert(tx.vout.empty()); // Set the user desired locktime coinControl.m_locktime = tx.nLockTime; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 3bd37cfd0e..62a7b4e4c8 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -224,7 +224,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const * Insert additional inputs into the transaction by * calling CreateTransaction(); */ -util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); +util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector<CRecipient>& recipients, std::optional<unsigned int> change_pos, bool lockUnspents, CCoinControl); } // namespace wallet #endif // BITCOIN_WALLET_SPEND_H diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index db9989163d..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> @@ -110,7 +112,7 @@ Mutex SQLiteDatabase::g_sqlite_mutex; int SQLiteDatabase::g_sqlite_count = 0; SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock) - : WalletDatabase(), m_mock(mock), m_dir_path(fs::PathToString(dir_path)), m_file_path(fs::PathToString(file_path)), m_use_unsafe_sync(options.use_unsafe_sync) + : WalletDatabase(), m_mock(mock), m_dir_path(fs::PathToString(dir_path)), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync) { { LOCK(g_sqlite_mutex); @@ -377,6 +379,17 @@ void SQLiteDatabase::Close() m_db = nullptr; } +bool SQLiteDatabase::HasActiveTxn() +{ + // 'sqlite3_get_autocommit' returns true by default, and false if a transaction has begun and not been committed or rolled back. + return m_db && sqlite3_get_autocommit(m_db) == 0; +} + +int SQliteExecHandler::Exec(SQLiteDatabase& database, const std::string& statement) +{ + return sqlite3_exec(database.m_db, statement.data(), nullptr, nullptr, nullptr); +} + std::unique_ptr<DatabaseBatch> SQLiteDatabase::MakeBatch(bool flush_on_close) { // We ignore flush_on_close because we don't do manual flushing for SQLite @@ -394,12 +407,18 @@ SQLiteBatch::SQLiteBatch(SQLiteDatabase& database) void SQLiteBatch::Close() { - // If m_db is in a transaction (i.e. not in autocommit mode), then abort the transaction in progress - if (m_database.m_db && sqlite3_get_autocommit(m_database.m_db) == 0) { + bool force_conn_refresh = false; + + // If we began a transaction, and it wasn't committed, abort the transaction in progress + if (m_txn) { if (TxnAbort()) { LogPrintf("SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted\n"); } else { - LogPrintf("SQLiteBatch: Batch closed and failed to abort transaction\n"); + // If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case + // by closing and reopening the database. Closing the database should also ensure that any changes made since the transaction + // was opened will be rolled back and future transactions can succeed without committing old data. + force_conn_refresh = true; + LogPrintf("SQLiteBatch: Batch closed and failed to abort transaction, resetting db connection..\n"); } } @@ -420,6 +439,19 @@ void SQLiteBatch::Close() } *stmt_prepared = nullptr; } + + if (force_conn_refresh) { + m_database.Close(); + try { + m_database.Open(); + // If TxnAbort failed and we refreshed the connection, the semaphore was not released, so release it here to avoid deadlocks on future writes. + m_database.m_write_semaphore.post(); + } catch (const std::runtime_error&) { + // If open fails, cleanup this object and rethrow the exception + m_database.Close(); + throw; + } + } } bool SQLiteBatch::ReadKey(DataStream&& key, DataStream& value) @@ -465,6 +497,9 @@ bool SQLiteBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite) if (!BindBlobToStatement(stmt, 1, key, "key")) return false; if (!BindBlobToStatement(stmt, 2, value, "value")) return false; + // Acquire semaphore if not previously acquired when creating a transaction. + if (!m_txn) m_database.m_write_semaphore.wait(); + // Execute int res = sqlite3_step(stmt); sqlite3_clear_bindings(stmt); @@ -472,6 +507,9 @@ bool SQLiteBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite) if (res != SQLITE_DONE) { LogPrintf("%s: Unable to execute statement: %s\n", __func__, sqlite3_errstr(res)); } + + if (!m_txn) m_database.m_write_semaphore.post(); + return res == SQLITE_DONE; } @@ -483,6 +521,9 @@ bool SQLiteBatch::ExecStatement(sqlite3_stmt* stmt, Span<const std::byte> blob) // Bind: leftmost parameter in statement is index 1 if (!BindBlobToStatement(stmt, 1, blob, "key")) return false; + // Acquire semaphore if not previously acquired when creating a transaction. + if (!m_txn) m_database.m_write_semaphore.wait(); + // Execute int res = sqlite3_step(stmt); sqlite3_clear_bindings(stmt); @@ -490,6 +531,9 @@ bool SQLiteBatch::ExecStatement(sqlite3_stmt* stmt, Span<const std::byte> blob) if (res != SQLITE_DONE) { LogPrintf("%s: Unable to execute statement: %s\n", __func__, sqlite3_errstr(res)); } + + if (!m_txn) m_database.m_write_semaphore.post(); + return res == SQLITE_DONE; } @@ -606,30 +650,43 @@ std::unique_ptr<DatabaseCursor> SQLiteBatch::GetNewPrefixCursor(Span<const std:: bool SQLiteBatch::TxnBegin() { - if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) == 0) return false; - int res = sqlite3_exec(m_database.m_db, "BEGIN TRANSACTION", nullptr, nullptr, nullptr); + if (!m_database.m_db || m_txn) return false; + m_database.m_write_semaphore.wait(); + Assert(!m_database.HasActiveTxn()); + int res = Assert(m_exec_handler)->Exec(m_database, "BEGIN TRANSACTION"); if (res != SQLITE_OK) { LogPrintf("SQLiteBatch: Failed to begin the transaction\n"); + m_database.m_write_semaphore.post(); + } else { + m_txn = true; } return res == SQLITE_OK; } bool SQLiteBatch::TxnCommit() { - if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) != 0) return false; - int res = sqlite3_exec(m_database.m_db, "COMMIT TRANSACTION", nullptr, nullptr, nullptr); + if (!m_database.m_db || !m_txn) return false; + Assert(m_database.HasActiveTxn()); + int res = Assert(m_exec_handler)->Exec(m_database, "COMMIT TRANSACTION"); if (res != SQLITE_OK) { LogPrintf("SQLiteBatch: Failed to commit the transaction\n"); + } else { + m_txn = false; + m_database.m_write_semaphore.post(); } return res == SQLITE_OK; } bool SQLiteBatch::TxnAbort() { - if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) != 0) return false; - int res = sqlite3_exec(m_database.m_db, "ROLLBACK TRANSACTION", nullptr, nullptr, nullptr); + if (!m_database.m_db || !m_txn) return false; + Assert(m_database.HasActiveTxn()); + int res = Assert(m_exec_handler)->Exec(m_database, "ROLLBACK TRANSACTION"); if (res != SQLITE_OK) { LogPrintf("SQLiteBatch: Failed to abort the transaction\n"); + } else { + m_txn = false; + m_database.m_write_semaphore.post(); } return res == SQLITE_OK; } diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index f1ce0567e1..0a3243fe19 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -36,11 +36,21 @@ public: Status Next(DataStream& key, DataStream& value) override; }; +/** Class responsible for executing SQL statements in SQLite databases. + * Methods are virtual so they can be overridden by unit tests testing unusual database conditions. */ +class SQliteExecHandler +{ +public: + virtual ~SQliteExecHandler() {} + virtual int Exec(SQLiteDatabase& database, const std::string& statement); +}; + /** RAII class that provides access to a WalletDatabase */ class SQLiteBatch : public DatabaseBatch { private: SQLiteDatabase& m_database; + std::unique_ptr<SQliteExecHandler> m_exec_handler{std::make_unique<SQliteExecHandler>()}; sqlite3_stmt* m_read_stmt{nullptr}; sqlite3_stmt* m_insert_stmt{nullptr}; @@ -48,6 +58,18 @@ private: sqlite3_stmt* m_delete_stmt{nullptr}; sqlite3_stmt* m_delete_prefix_stmt{nullptr}; + /** Whether this batch has started a database transaction and whether it owns SQLiteDatabase::m_write_semaphore. + * If the batch starts a db tx, it acquires the semaphore and sets this to true, keeping the semaphore + * until the transaction ends to prevent other batch objects from writing to the database. + * + * If this batch did not start a transaction, the semaphore is acquired transiently when writing and m_txn + * is not set. + * + * m_txn is different from HasActiveTxn() as it is only true when this batch has started the transaction, + * not just when any batch has started a transaction. + */ + bool m_txn{false}; + void SetupSQLStatements(); bool ExecStatement(sqlite3_stmt* stmt, Span<const std::byte> blob); @@ -61,6 +83,8 @@ public: explicit SQLiteBatch(SQLiteDatabase& database); ~SQLiteBatch() override { Close(); } + void SetExecHandler(std::unique_ptr<SQliteExecHandler>&& handler) { m_exec_handler = std::move(handler); } + /* No-op. See comment on SQLiteDatabase::Flush */ void Flush() override {} @@ -103,6 +127,10 @@ public: ~SQLiteDatabase(); + // Batches must acquire this semaphore on writing, and release when done writing. + // This ensures that only one batch is modifying the database at a time. + CSemaphore m_write_semaphore; + bool Verify(bilingual_str& error); /** Open the database if it is not already opened */ @@ -142,6 +170,9 @@ public: /** Make a SQLiteBatch connected to this database */ std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override; + /** Return true if there is an on-going txn in this connection */ + bool HasActiveTxn(); + sqlite3* m_db{nullptr}; bool m_use_unsafe_sync; }; 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 d341e84d9b..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> @@ -205,5 +207,155 @@ BOOST_AUTO_TEST_CASE(db_cursor_prefix_byte_test) } } +BOOST_AUTO_TEST_CASE(db_availability_after_write_error) +{ + // Ensures the database remains accessible without deadlocking after a write error. + // To simulate the behavior, record overwrites are disallowed, and the test verifies + // that the database remains active after failing to store an existing record. + for (const auto& database : TestDatabases(m_path_root)) { + // Write original record + std::unique_ptr<DatabaseBatch> batch = database->MakeBatch(); + std::string key = "key"; + std::string value = "value"; + std::string value2 = "value_2"; + BOOST_CHECK(batch->Write(key, value)); + // Attempt to overwrite the record (expect failure) + BOOST_CHECK(!batch->Write(key, value2, /*fOverwrite=*/false)); + // Successfully overwrite the record + BOOST_CHECK(batch->Write(key, value2, /*fOverwrite=*/true)); + // Sanity-check; read and verify the overwritten value + std::string read_value; + BOOST_CHECK(batch->Read(key, read_value)); + BOOST_CHECK_EQUAL(read_value, value2); + } +} + +// 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 +constexpr int TEST_SQLITE_ERROR = -999; + +class DbExecBlocker : public SQliteExecHandler +{ +private: + SQliteExecHandler m_base_exec; + std::set<std::string> m_blocked_statements; +public: + DbExecBlocker(std::set<std::string> blocked_statements) : m_blocked_statements(blocked_statements) {} + int Exec(SQLiteDatabase& database, const std::string& statement) override { + if (m_blocked_statements.contains(statement)) return TEST_SQLITE_ERROR; + return m_base_exec.Exec(database, statement); + } +}; + +BOOST_AUTO_TEST_CASE(txn_close_failure_dangling_txn) +{ + // Verifies that there is no active dangling, to-be-reversed db txn + // after the batch object that initiated it is destroyed. + DatabaseOptions options; + DatabaseStatus status; + bilingual_str error; + std::unique_ptr<SQLiteDatabase> database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error); + + std::string key = "key"; + std::string value = "value"; + + std::unique_ptr<SQLiteBatch> batch = std::make_unique<SQLiteBatch>(*database); + BOOST_CHECK(batch->TxnBegin()); + BOOST_CHECK(batch->Write(key, value)); + // Set a handler to prevent txn abortion during destruction. + // Mimicking a db statement execution failure. + batch->SetExecHandler(std::make_unique<DbExecBlocker>(std::set<std::string>{"ROLLBACK TRANSACTION"})); + // Destroy batch + batch.reset(); + + // Ensure there is no dangling, to-be-reversed db txn + BOOST_CHECK(!database->HasActiveTxn()); + + // And, just as a sanity check; verify that new batchs only write what they suppose to write + // and nothing else. + std::string key2 = "key2"; + std::unique_ptr<SQLiteBatch> batch2 = std::make_unique<SQLiteBatch>(*database); + BOOST_CHECK(batch2->Write(key2, value)); + // The first key must not exist + BOOST_CHECK(!batch2->Exists(key)); +} + +BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere) +{ + std::string key = "key"; + std::string value = "value"; + std::string value2 = "value_2"; + + DatabaseOptions options; + DatabaseStatus status; + bilingual_str error; + const auto& database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error); + + std::unique_ptr<DatabaseBatch> handler = Assert(database)->MakeBatch(); + + // Verify concurrent db transactions does not interfere between each other. + // Start db txn, write key and check the key does exist within the db txn. + BOOST_CHECK(handler->TxnBegin()); + BOOST_CHECK(handler->Write(key, value)); + BOOST_CHECK(handler->Exists(key)); + + // But, the same key, does not exist in another handler + std::unique_ptr<DatabaseBatch> handler2 = Assert(database)->MakeBatch(); + BOOST_CHECK(handler2->Exists(key)); + + // Attempt to commit the handler txn calling the handler2 methods. + // Which, must not be possible. + BOOST_CHECK(!handler2->TxnCommit()); + BOOST_CHECK(!handler2->TxnAbort()); + + // Only the first handler can commit the changes. + BOOST_CHECK(handler->TxnCommit()); + // And, once commit is completed, handler2 can read the record + std::string read_value; + BOOST_CHECK(handler2->Read(key, read_value)); + BOOST_CHECK_EQUAL(read_value, value); + + // Also, once txn is committed, single write statements are re-enabled. + // Which means that handler2 can read the record changes directly. + BOOST_CHECK(handler->Write(key, value2, /*fOverwrite=*/true)); + BOOST_CHECK(handler2->Read(key, read_value)); + BOOST_CHECK_EQUAL(read_value, value2); +} +#endif // USE_SQLITE + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet 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/fees.cpp b/src/wallet/test/fuzz/fees.cpp index 2f7892dc0a..c2e785651a 100644 --- a/src/wallet/test/fuzz/fees.cpp +++ b/src/wallet/test/fuzz/fees.cpp @@ -37,6 +37,10 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup) } if (fuzzed_data_provider.ConsumeBool()) { + wallet.m_fallback_fee = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; + } + + if (fuzzed_data_provider.ConsumeBool()) { wallet.m_discard_rate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; } (void)GetDiscardRate(wallet); @@ -58,6 +62,9 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup) if (fuzzed_data_provider.ConsumeBool()) { coin_control.m_confirm_target = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 999'000); } + if (fuzzed_data_provider.ConsumeBool()) { + coin_control.m_fee_mode = fuzzed_data_provider.ConsumeBool() ? FeeEstimateMode::CONSERVATIVE : FeeEstimateMode::ECONOMICAL; + } FeeCalculation fee_calculation; FeeCalculation* maybe_fee_calculation{fuzzed_data_provider.ConsumeBool() ? nullptr : &fee_calculation}; diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 203ab5f606..792079e6c6 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -3,7 +3,6 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <addresstype.h> -#include <common/args.h> #include <consensus/amount.h> #include <interfaces/chain.h> #include <kernel/chain.h> @@ -89,8 +88,6 @@ void ImportDescriptors(CWallet& wallet, const std::string& seed_insecure) * Wraps a descriptor wallet for fuzzing. */ struct FuzzedWallet { - ArgsManager args; - WalletContext context; std::shared_ptr<CWallet> wallet; FuzzedWallet(const std::string& name, const std::string& seed_insecure) { @@ -109,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) @@ -132,6 +127,14 @@ struct FuzzedWallet { } } } + std::vector<CRecipient> recipients; + for (size_t idx = 0; idx < tx.vout.size(); idx++) { + const CTxOut& tx_out = tx.vout[idx]; + CTxDestination dest; + ExtractDestination(tx_out.scriptPubKey, dest); + CRecipient recipient = {dest, tx_out.nValue, subtract_fee_from_outputs.count(idx) == 1}; + recipients.push_back(recipient); + } CCoinControl coin_control; coin_control.m_allow_other_inputs = fuzzed_data_provider.ConsumeBool(); CallOneOf( @@ -158,7 +161,10 @@ struct FuzzedWallet { int change_position{fuzzed_data_provider.ConsumeIntegralInRange<int>(-1, tx.vout.size() - 1)}; bilingual_str error; - (void)FundTransaction(*wallet, tx, change_position, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + tx.vout.clear(); + (void)FundTransaction(*wallet, tx, recipients, change_position, /*lockUnspents=*/false, coin_control); } }; diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index b0c955f482..228e9629ed 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -49,9 +49,21 @@ void initialize_spkm() MOCKED_DESC_CONVERTER.Init(); } +/** + * Key derivation is expensive. Deriving deep derivation paths take a lot of compute and we'd rather spend time + * elsewhere in this target, like on actually fuzzing the DescriptorScriptPubKeyMan. So rule out strings which could + * correspond to a descriptor containing a too large derivation path. + */ +static bool TooDeepDerivPath(std::string_view desc) +{ + const FuzzBufferType desc_buf{reinterpret_cast<const unsigned char *>(desc.data()), desc.size()}; + return HasDeepDerivPath(desc_buf); +} + static std::optional<std::pair<WalletDescriptor, FlatSigningProvider>> CreateWalletDescriptor(FuzzedDataProvider& fuzzed_data_provider) { const std::string mocked_descriptor{fuzzed_data_provider.ConsumeRandomLengthString()}; + if (TooDeepDerivPath(mocked_descriptor)) return {}; const auto desc_str{MOCKED_DESC_CONVERTER.GetDescriptor(mocked_descriptor)}; if (!desc_str.has_value()) return std::nullopt; diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp index 95d5c1b9ce..dfad0e2126 100644 --- a/src/wallet/test/ismine_tests.cpp +++ b/src/wallet/test/ismine_tests.cpp @@ -47,8 +47,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) pubkeys[i] = keys[i].GetPubKey(); } - CKey uncompressedKey; - uncompressedKey.MakeNewKey(false); + CKey uncompressedKey = GenerateRandomKey(/*compressed=*/false); CPubKey uncompressedPubkey = uncompressedKey.GetPubKey(); std::unique_ptr<interfaces::Chain>& chain = m_node.chain; 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 1c5ca1483f..3a67b9a433 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -231,8 +231,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) keys.push_back(key); key.clear(); key.setObject(); - CKey futureKey; - futureKey.MakeNewKey(true); + CKey futureKey = GenerateRandomKey(); key.pushKV("scriptPubKey", HexStr(GetScriptForRawPubKey(futureKey.GetPubKey()))); key.pushKV("timestamp", newTip->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1); key.pushKV("internal", UniValue(true)); @@ -446,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())); @@ -704,8 +705,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) static size_t CalculateNestedKeyhashInputSize(bool use_max_sig) { // Generate ephemeral valid pubkey - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); CPubKey pubkey = key.GetPubKey(); // Generate pubkey hash @@ -789,8 +789,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) context.args = &m_args; context.chain = m_node.chain.get(); auto wallet = TestLoadWallet(context); - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); AddKey(*wallet, key); TestUnloadWallet(std::move(wallet)); @@ -815,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; @@ -843,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); @@ -866,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 @@ -891,15 +890,14 @@ 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; context.args = &m_args; context.chain = m_node.chain.get(); auto wallet = TestLoadWallet(context); - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); AddKey(*wallet, key); std::string error; @@ -907,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(); @@ -917,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 507174413d..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> @@ -555,7 +553,7 @@ void CWallet::UpgradeDescriptorCache() SetWalletFlag(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED); } -bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool accept_no_keys) +bool CWallet::Unlock(const SecureString& strWalletPassphrase) { CCrypter crypter; CKeyingMaterial _vMasterKey; @@ -568,7 +566,7 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool accept_no_key return false; if (!crypter.Decrypt(pMasterKey.second.vchCryptedKey, _vMasterKey)) continue; // try another master key - if (Unlock(_vMasterKey, accept_no_keys)) { + if (Unlock(_vMasterKey)) { // Now that we've unlocked, upgrade the key metadata UpgradeKeyMetadata(); // Now that we've unlocked, upgrade the descriptor cache @@ -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) @@ -2359,22 +2416,32 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add { LOCK(cs_wallet); std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address); - fUpdated = (mi != m_address_book.end() && !mi->second.IsChange()); - m_address_book[address].SetLabel(strName); + fUpdated = mi != m_address_book.end() && !mi->second.IsChange(); + + CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address]; + record.SetLabel(strName); is_mine = IsMine(address) != ISMINE_NO; if (new_purpose) { /* update purpose only if requested */ - purpose = m_address_book[address].purpose = new_purpose; - } else { - purpose = m_address_book[address].purpose; + record.purpose = new_purpose; } + purpose = record.purpose; + } + + const std::string& encoded_dest = EncodeDestination(address); + if (new_purpose && !batch.WritePurpose(encoded_dest, PurposeToString(*new_purpose))) { + WalletLogPrintf("Error: fail to write address book 'purpose' entry\n"); + return false; + } + if (!batch.WriteName(encoded_dest, strName)) { + WalletLogPrintf("Error: fail to write address book 'name' entry\n"); + return false; } + // In very old wallets, address purpose may not be recorded so we derive it from IsMine NotifyAddressBookChanged(address, strName, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), (fUpdated ? CT_UPDATED : CT_NEW)); - if (new_purpose && !batch.WritePurpose(EncodeDestination(address), PurposeToString(*new_purpose))) - return false; - return batch.WriteName(EncodeDestination(address), strName); + return true; } bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose) @@ -2385,7 +2452,14 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s bool CWallet::DelAddressBook(const CTxDestination& address) { - WalletBatch batch(GetDatabase()); + return RunWithinTxn(GetDatabase(), /*process_desc=*/"address book entry removal", [&](WalletBatch& batch){ + return DelAddressBookWithDB(batch, address); + }); +} + +bool CWallet::DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address) +{ + const std::string& dest = EncodeDestination(address); { LOCK(cs_wallet); // If we want to delete receiving addresses, we should avoid calling EraseAddressData because it will delete the previously_spent value. Could instead just erase the label so it becomes a change address, and keep the data. @@ -2396,14 +2470,30 @@ bool CWallet::DelAddressBook(const CTxDestination& address) return false; } // Delete data rows associated with this address - batch.EraseAddressData(address); + if (!batch.EraseAddressData(address)) { + WalletLogPrintf("Error: cannot erase address book entry data\n"); + return false; + } + + // Delete purpose entry + if (!batch.ErasePurpose(dest)) { + WalletLogPrintf("Error: cannot erase address book entry purpose\n"); + return false; + } + + // Delete name entry + if (!batch.EraseName(dest)) { + WalletLogPrintf("Error: cannot erase address book entry name\n"); + return false; + } + + // finally, remove it from the map m_address_book.erase(address); } + // All good, signal changes NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED); - - batch.ErasePurpose(EncodeDestination(address)); - return batch.EraseName(EncodeDestination(address)); + return true; } size_t CWallet::KeypoolCountExternalKeys() const @@ -2547,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; } @@ -2573,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)) { @@ -2582,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) @@ -2725,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; } @@ -3099,6 +3191,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri if (time_first_key) walletInstance->MaybeUpdateBirthTime(*time_first_key); if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { + walletInstance->m_chain_notifications_handler.reset(); // Reset this pointer so that the wallet will actually be unloaded return nullptr; } @@ -3314,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 { @@ -3373,12 +3466,12 @@ bool CWallet::Lock() return true; } -bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys) +bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn) { { LOCK(cs_wallet); for (const auto& spk_man_pair : m_spk_managers) { - if (!spk_man_pair.second->CheckDecryptionKey(vMasterKeyIn, accept_no_keys)) { + if (!spk_man_pair.second->CheckDecryptionKey(vMasterKeyIn)) { return false; } } @@ -3402,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; @@ -3424,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; } @@ -3449,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; } @@ -3512,9 +3628,10 @@ void CWallet::SetupLegacyScriptPubKeyMan() AddScriptPubKeyMan(id, std::move(spk_manager)); } -const CKeyingMaterial& CWallet::GetEncryptionKey() const +bool CWallet::WithEncryptionKey(std::function<bool (const CKeyingMaterial&)> cb) const { - return vMasterKey; + LOCK(cs_wallet); + return cb(vMasterKey); } bool CWallet::HasEncryptionKeys() const @@ -3531,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) @@ -3552,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); } } @@ -3578,8 +3704,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { // Make a seed - CKey seed_key; - seed_key.MakeNewKey(true); + CKey seed_key = GenerateRandomKey(); CPubKey seed = seed_key.GetPubKey(); assert(seed_key.VerifyPubKey(seed)); @@ -3863,7 +3988,11 @@ std::optional<MigrationData> CWallet::GetDescriptorsForLegacy(bilingual_str& err AssertLockHeld(cs_wallet); LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan(); - assert(legacy_spkm); + if (!Assume(legacy_spkm)) { + // This shouldn't happen + error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing")); + return std::nullopt; + } std::optional<MigrationData> res = legacy_spkm->MigrateToDescriptor(); if (res == std::nullopt) { @@ -3878,8 +4007,9 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) AssertLockHeld(cs_wallet); LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan(); - if (!legacy_spkm) { - error = _("Error: This wallet is already a descriptor wallet"); + if (!Assume(legacy_spkm)) { + // This shouldn't happen + error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing")); return false; } @@ -3890,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."); @@ -3921,6 +4053,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } } + // Get best block locator so that we can copy it to the watchonly and solvables + CBlockLocator best_block_locator; + if (!WalletBatch(GetDatabase()).ReadBestBlock(best_block_locator)) { + error = _("Error: Unable to read wallet's best block locator record"); + return false; + } + // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet. // We need to go through these in the tx insertion order so that lookups to spends works. std::vector<uint256> txids_to_delete; @@ -3931,32 +4070,47 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) LOCK(data.watchonly_wallet->cs_wallet); data.watchonly_wallet->nOrderPosNext = nOrderPosNext; watchonly_batch->WriteOrderPosNext(data.watchonly_wallet->nOrderPosNext); + // Write the best block locator to avoid rescanning on reload + if (!watchonly_batch->WriteBestBlock(best_block_locator)) { + error = _("Error: Unable to write watchonly wallet best block locator record"); + return false; + } + } + if (data.solvable_wallet) { + // Write the best block locator to avoid rescanning on reload + if (!WalletBatch(data.solvable_wallet->GetDatabase()).WriteBestBlock(best_block_locator)) { + error = _("Error: Unable to write solvable wallet best block locator record"); + return false; + } } for (const auto& [_pos, wtx] : wtxOrdered) { - if (!IsMine(*wtx->tx) && !IsFromMe(*wtx->tx)) { - // Check it is the watchonly wallet's - // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for - if (data.watchonly_wallet) { - LOCK(data.watchonly_wallet->cs_wallet); - if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) { - // Add to watchonly wallet - const uint256& hash = wtx->GetHash(); - const CWalletTx& to_copy_wtx = *wtx; - if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) { - if (!new_tx) return false; - ins_wtx.SetTx(to_copy_wtx.tx); - ins_wtx.CopyFrom(to_copy_wtx); - return true; - })) { - error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex()); - return false; - } - watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash)); - // Mark as to remove from this wallet + // Check it is the watchonly wallet's + // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for + bool is_mine = IsMine(*wtx->tx) || IsFromMe(*wtx->tx); + if (data.watchonly_wallet) { + LOCK(data.watchonly_wallet->cs_wallet); + if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) { + // Add to watchonly wallet + const uint256& hash = wtx->GetHash(); + const CWalletTx& to_copy_wtx = *wtx; + if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) { + if (!new_tx) return false; + ins_wtx.SetTx(to_copy_wtx.tx); + ins_wtx.CopyFrom(to_copy_wtx); + return true; + })) { + error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex()); + return false; + } + watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash)); + // Mark as to remove from the migrated wallet only if it does not also belong to it + if (!is_mine) { txids_to_delete.push_back(hash); - continue; } + continue; } + } + if (!is_mine) { // Both not ours and not in the watchonly wallet error = strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex()); return false; @@ -3965,112 +4119,95 @@ 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"); + if (auto res = RemoveTxs(txids_to_delete); !res) { + error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); return false; } - if (deleted_txids != txids_to_delete) { - error = _("Error: Not all watchonly txs could be deleted"); + } + + // Pair external wallets with their corresponding db handler + std::vector<std::pair<std::shared_ptr<CWallet>, std::unique_ptr<WalletBatch>>> wallets_vec; + for (const auto& ext_wallet : {data.watchonly_wallet, data.solvable_wallet}) { + if (!ext_wallet) continue; + + std::unique_ptr<WalletBatch> batch = std::make_unique<WalletBatch>(ext_wallet->GetDatabase()); + if (!batch->TxnBegin()) { + error = strprintf(_("Error: database transaction cannot be executed for wallet %s"), ext_wallet->GetName()); return false; } - // Tell the GUI of each tx - for (const uint256& txid : deleted_txids) { - NotifyTransactionChanged(txid, CT_UPDATED); - } + wallets_vec.emplace_back(ext_wallet, std::move(batch)); } + // Write address book entry to disk + auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) { + auto address{EncodeDestination(dest)}; + if (entry.purpose) batch.WritePurpose(address, PurposeToString(*entry.purpose)); + if (entry.label) batch.WriteName(address, *entry.label); + for (const auto& [id, request] : entry.receive_requests) { + batch.WriteAddressReceiveRequest(dest, id, request); + } + if (entry.previously_spent) batch.WriteAddressPreviouslySpent(dest, true); + }; + // Check the address book data in the same way we did for transactions std::vector<CTxDestination> dests_to_delete; - for (const auto& addr_pair : m_address_book) { - // Labels applied to receiving addresses should go based on IsMine - if (addr_pair.second.purpose == AddressPurpose::RECEIVE) { - if (!IsMine(addr_pair.first)) { - // Check the address book data is the watchonly wallet's - if (data.watchonly_wallet) { - LOCK(data.watchonly_wallet->cs_wallet); - if (data.watchonly_wallet->IsMine(addr_pair.first)) { - // Add to the watchonly. Preserve the labels, purpose, and change-ness - std::string label = addr_pair.second.GetLabel(); - data.watchonly_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose; - if (!addr_pair.second.IsChange()) { - data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label); - } - dests_to_delete.push_back(addr_pair.first); - continue; - } - } - if (data.solvable_wallet) { - LOCK(data.solvable_wallet->cs_wallet); - if (data.solvable_wallet->IsMine(addr_pair.first)) { - // Add to the solvable. Preserve the labels, purpose, and change-ness - std::string label = addr_pair.second.GetLabel(); - data.solvable_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose; - if (!addr_pair.second.IsChange()) { - data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label); - } - dests_to_delete.push_back(addr_pair.first); - continue; - } - } + for (const auto& [dest, record] : m_address_book) { + // Ensure "receive" entries that are no longer part of the original wallet are transferred to another wallet + // Entries for everything else ("send") will be cloned to all wallets. + bool require_transfer = record.purpose == AddressPurpose::RECEIVE && !IsMine(dest); + bool copied = false; + for (auto& [wallet, batch] : wallets_vec) { + LOCK(wallet->cs_wallet); + if (require_transfer && !wallet->IsMine(dest)) continue; + + // Copy the entire address book entry + wallet->m_address_book[dest] = record; + func_store_addr(*batch, dest, record); + + copied = true; + // Only delete 'receive' records that are no longer part of the original wallet + if (require_transfer) { + dests_to_delete.push_back(dest); + break; + } + } - // Skip invalid/non-watched scripts that will not be migrated - if (not_migrated_dests.count(addr_pair.first) > 0) { - dests_to_delete.push_back(addr_pair.first); - continue; - } + // Fail immediately if we ever found an entry that was ours and cannot be transferred + // to any of the created wallets (watch-only, solvable). + // Means that no inferred descriptor maps to the stored entry. Which mustn't happen. + if (require_transfer && !copied) { - // Not ours, not in watchonly wallet, and not in solvable - error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets"); - return false; - } - } else { - // Labels for everything else ("send") should be cloned to all - if (data.watchonly_wallet) { - LOCK(data.watchonly_wallet->cs_wallet); - // Add to the watchonly. Preserve the labels, purpose, and change-ness - std::string label = addr_pair.second.GetLabel(); - data.watchonly_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose; - if (!addr_pair.second.IsChange()) { - data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label); - } - } - if (data.solvable_wallet) { - LOCK(data.solvable_wallet->cs_wallet); - // Add to the solvable. Preserve the labels, purpose, and change-ness - std::string label = addr_pair.second.GetLabel(); - data.solvable_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose; - if (!addr_pair.second.IsChange()) { - data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label); - } + // Skip invalid/non-watched scripts that will not be migrated + if (not_migrated_dests.count(dest) > 0) { + dests_to_delete.push_back(dest); + continue; } + + error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets"); + return false; } } - // Persist added address book entries (labels, purpose) for watchonly and solvable wallets - auto persist_address_book = [](const CWallet& wallet) { - LOCK(wallet.cs_wallet); - WalletBatch batch{wallet.GetDatabase()}; - for (const auto& [destination, addr_book_data] : wallet.m_address_book) { - auto address{EncodeDestination(destination)}; - std::optional<std::string> label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel()); - // don't bother writing default values (unknown purpose) - if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose)); - if (label) batch.WriteName(address, *label); + // Persist external wallets address book entries + for (auto& [wallet, batch] : wallets_vec) { + if (!batch->TxnCommit()) { + error = strprintf(_("Error: address book copy failed for wallet %s"), wallet->GetName()); + return false; } - }; - if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet); - if (data.solvable_wallet) persist_address_book(*data.solvable_wallet); + } - // Remove the things to delete + // Remove the things to delete in this wallet + WalletBatch local_wallet_batch(GetDatabase()); + local_wallet_batch.TxnBegin(); if (dests_to_delete.size() > 0) { for (const auto& dest : dests_to_delete) { - if (!DelAddressBook(dest)) { + if (!DelAddressBookWithDB(local_wallet_batch, dest)) { error = _("Error: Unable to remove watchonly address book data"); return false; } } } + local_wallet_batch.TxnCommit(); // Connect the SPKM signals ConnectScriptPubKeyManNotifiers(); @@ -4202,11 +4339,13 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle std::vector<bilingual_str> warnings; // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it + bool was_loaded = false; if (auto wallet = GetWallet(context, wallet_name)) { if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { return util::Error{_("Unable to unload the wallet before migrating")}; } UnloadWallet(std::move(wallet)); + was_loaded = true; } // Load the wallet but only in the context of this function. @@ -4227,42 +4366,66 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error}; } + // Helper to reload as normal for some of our exit scenarios + const auto& reload_wallet = [&](std::shared_ptr<CWallet>& to_reload) { + assert(to_reload.use_count() == 1); + std::string name = to_reload->GetName(); + to_reload.reset(); + to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + return to_reload != nullptr; + }; + // Before anything else, check if there is something to migrate. - if (!local_wallet->GetLegacyScriptPubKeyMan()) { + if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + if (was_loaded) { + reload_wallet(local_wallet); + } return util::Error{_("Error: This wallet is already a descriptor wallet")}; } // 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) { + reload_wallet(local_wallet); + } return util::Error{_("Error: Unable to make a backup of your wallet")}; } res.backup_path = backup_path; bool success = false; - { - LOCK(local_wallet->cs_wallet); - // Unlock the wallet if needed - if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) { - if (passphrase.find('\0') == std::string::npos) { - return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}; - } else { - return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase entered was incorrect. " - "The passphrase contains a null character (ie - a zero byte). " - "If this passphrase was set with a version of this software prior to 25.0, " - "please try again with only the characters up to — but not including — " - "the first null character.")}; - } + // Unlock the wallet if needed + if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) { + if (was_loaded) { + reload_wallet(local_wallet); + } + if (passphrase.find('\0') == std::string::npos) { + return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}; + } else { + return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase entered was incorrect. " + "The passphrase contains a null character (ie - a zero byte). " + "If this passphrase was set with a version of this software prior to 25.0, " + "please try again with only the characters up to — but not including — " + "the first null character.")}; } + } + { + LOCK(local_wallet->cs_wallet); // First change to using SQLite if (!local_wallet->MigrateToSQLite(error)) return util::Error{error}; - // Do the migration, and cleanup if it fails - success = DoMigration(*local_wallet, context, error, res); + // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails + success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); + if (!success) { + success = DoMigration(*local_wallet, context, error, res); + } else { + // Make sure that descriptors flag is actually set + local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + } } // In case of reloading failure, we need to remember the wallet dirs to remove @@ -4272,24 +4435,19 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle std::set<fs::path> wallet_dirs; if (success) { // Migration successful, unload all wallets locally, then reload them. - const auto& reload_wallet = [&](std::shared_ptr<CWallet>& to_reload) { - assert(to_reload.use_count() == 1); - std::string name = to_reload->GetName(); - wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path()); - to_reload.reset(); - to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); - return to_reload != nullptr; - }; // Reload the main wallet + wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(local_wallet); res.wallet = local_wallet; res.wallet_name = wallet_name; if (success && res.watchonly_wallet) { // Reload watchonly + wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(res.watchonly_wallet); } if (success && res.solvables_wallet) { // Reload solvables + wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(res.solvables_wallet); } } @@ -4347,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 487921443f..6a998fa398 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -302,7 +302,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati private: CKeyingMaterial vMasterKey GUARDED_BY(cs_wallet); - bool Unlock(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys = false); + bool Unlock(const CKeyingMaterial& vMasterKeyIn); std::atomic<bool> fAbortRescan{false}; std::atomic<bool> fScanningWallet{false}; // controlled by WalletRescanReserver @@ -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); @@ -578,7 +577,7 @@ public: // Used to prevent deleting the passphrase from memory when it is still in use. RecursiveMutex m_relock_mutex; - bool Unlock(const SecureString& strWalletPassphrase, bool accept_no_keys = false); + bool Unlock(const SecureString& strWalletPassphrase); bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); bool EncryptWallet(const SecureString& strWalletPassphrase); @@ -790,11 +789,14 @@ 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); bool DelAddressBook(const CTxDestination& address); + bool DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address); bool IsAddressPreviouslySpent(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressPreviouslySpent(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -936,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; @@ -962,7 +965,8 @@ public: //! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external. void SetupLegacyScriptPubKeyMan(); - const CKeyingMaterial& GetEncryptionKey() const override; + bool WithEncryptionKey(std::function<bool (const CKeyingMaterial&)> cb) const override; + bool HasEncryptionKeys() const override; /** Get last block processed height */ @@ -990,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 @@ -1010,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); @@ -1041,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 ba453b47e7..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 - Span key_data{key}; - - std::string type; - key >> type; - - if (types.count(type) > 0) { - if (!m_batch->Erase(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() @@ -1498,20 +1409,26 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas if (format == DatabaseFormat::SQLITE) { #ifdef USE_SQLITE - return MakeSQLiteDatabase(path, options, status, error); -#else - error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + if constexpr (true) { + return MakeSQLiteDatabase(path, options, status, error); + } else #endif + { + error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); + status = DatabaseStatus::FAILED_BAD_FORMAT; + return nullptr; + } } #ifdef USE_BDB - return MakeBerkeleyDatabase(path, options, status, error); -#else - error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + if constexpr (true) { + return MakeBerkeleyDatabase(path, options, status, error); + } else #endif + { + error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); + status = DatabaseStatus::FAILED_BAD_FORMAT; + return nullptr; + } } } // namespace wallet 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 2f3f8ef77d..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> @@ -68,7 +66,6 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa } if (load_wallet_ret != DBErrors::LOAD_OK) { - wallet_instance = nullptr; if (load_wallet_ret == DBErrors::CORRUPT) { tfm::format(std::cerr, "Error loading %s: Wallet corrupted", name); return nullptr; @@ -194,10 +191,15 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) DatabaseOptions options; ReadDatabaseArgs(args, options); options.require_existing = true; - const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options); - if (!wallet_instance) return false; + DatabaseStatus status; bilingual_str error; - bool ret = DumpWallet(args, *wallet_instance, error); + std::unique_ptr<WalletDatabase> database = MakeDatabase(path, options, status, error); + if (!database) { + tfm::format(std::cerr, "%s\n", error.original); + return false; + } + + bool ret = DumpWallet(args, *database, error); if (!ret && !error.empty()) { tfm::format(std::cerr, "%s\n", error.original); return ret; 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 |