From 9552619961049d7673f84c40917b0385be70b782 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Mar 2024 18:44:23 -0400 Subject: [PATCH] refactor: Avoid copying util::Result values Copying util::Result values is less efficient than moving them because they allocate memory and contain strings. Also this is needed to avoid compile errors in https://github.com/bitcoin/bitcoin/pull/25722 which adds a std::unique_ptr member to util::Result which implicity disables copying. --- src/util/result.h | 4 +++- src/wallet/spend.cpp | 30 ++++++++++++-------------- src/wallet/test/fuzz/coinselection.cpp | 5 ++++- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/util/result.h b/src/util/result.h index 11fe89af25332..122a7638fad22 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -39,6 +39,9 @@ class Result std::variant m_variant; + //! Disallow copy constructor, require Result to be moved for efficiency. + Result(const Result&) = delete; + //! Disallow operator= to avoid confusion in the future when the Result //! class gains support for richer error reporting, and callers should have //! ability to set a new result value without clearing existing error @@ -53,7 +56,6 @@ class Result Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {} // constructor for void Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {} Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {} - Result(const Result&) = default; Result(Result&&) = default; ~Result() = default; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 5d23ebd35acee..9a7e166e689d4 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -683,11 +683,11 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co // Vector of results. We will choose the best one based on waste. std::vector results; std::vector> errors; - auto append_error = [&] (const util::Result& result) { + auto append_error = [&] (util::Result&& 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 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,25 +707,25 @@ util::Result 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(cg_result); + 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 @@ -818,7 +818,7 @@ util::Result 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 res = [&] { + { // Place coins eligibility filters on a scope increasing order. std::vector ordered_filters{ // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six @@ -866,9 +866,9 @@ util::Result 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({_("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(util::Error()); // General "Insufficient Funds" + return util::Error{}; // General "Insufficient Funds" } // Walk-through the filters until the solution gets found. @@ -885,19 +885,17 @@ util::Result 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(util::Error()); - }(); - - return res; + return util::Error{}; + } } static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256& block_hash) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 297432de9efc5..331590df7feb6 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -291,7 +291,10 @@ FUZZ_TARGET(coinselection) } std::vector utxos; - std::vector> results{result_srd, result_knapsack, result_bnb}; + std::vector> 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> new_utxo_pool;