Merge bitcoin/bitcoin#29906: Disable util::Result copying and assignment

6a8b2befeab25e4e92d8e947a23e78014695e06c refactor: Avoid copying util::Result values (Ryan Ofsky)
834f65e82405bbed336f98996bc8cef366bbed0f refactor: Drop util::Result operator= (Ryan Ofsky)

Pull request description:

  This PR just contains the first two commits of #25665.

  It disables copying of `util::Result` objects because unnecessary copies are inefficient and not possible after #25665, which makes `util::Result` object move-only.

  It disables the assignment operator and replaces it with an `Update()` method, because #25665 adds more information to `util::Result` objects (warning and error messages and failure values) and having an assignment operator that overwrites data instead of merging it would make it easy to accidentally erase existing information while trying to assign new information.

ACKs for top commit:
  stickies-v:
    re-ACK 6a8b2befeab25e4e92d8e947a23e78014695e06c
  achow101:
    ACK 6a8b2befeab25e4e92d8e947a23e78014695e06c
  furszy:
    re-ACK https://github.com/bitcoin/bitcoin/commit/6a8b2befeab25e4e92d8e947a23e78014695e06c

Tree-SHA512: 3f21af9031d50d6c68cca69133de03080f69b1ddcf8b140bdeb762069f14645209b2586037236d15b6ebd8973af0fbefd7e83144aeb7b84078a4cb4df812f984

4 files changed, 22 insertions, 25 deletions

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 5d23ebd35a..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,25 +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(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<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
@@ -866,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.
@@ -885,19 +885,17 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
                 // If any specific error message appears here, then something particularly wrong might have happened.
                 // Save the error and continue the selection process. So if no solutions gets found, we can return
                 // the detailed error to the upper layers.
-                if (HasErrorMsg(res)) res_detailed_errors.emplace_back(res);
+                if (HasErrorMsg(res)) res_detailed_errors.emplace_back(std::move(res));
             }
         }
 
         // Return right away if we have a detailed error
-        if (!res_detailed_errors.empty()) return res_detailed_errors.front();
+        if (!res_detailed_errors.empty()) return std::move(res_detailed_errors.front());
 
 
         // General "Insufficient Funds"
-        return util::Result<SelectionResult>(util::Error());
-    }();
-
-    return res;
+        return util::Error{};
+    }
 }
 
 static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256& block_hash)
diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index 297432de9e..331590df7f 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -291,7 +291,10 @@ FUZZ_TARGET(coinselection)
     }
 
     std::vector<COutput> utxos;
-    std::vector<util::Result<SelectionResult>> results{result_srd, result_knapsack, result_bnb};
+    std::vector<util::Result<SelectionResult>> results;
+    results.emplace_back(std::move(result_srd));
+    results.emplace_back(std::move(result_knapsack));
+    results.emplace_back(std::move(result_bnb));
     CAmount new_total_balance{CreateCoins(fuzzed_data_provider, utxos, coin_params, next_locktime)};
     if (new_total_balance > 0) {
         std::set<std::shared_ptr<COutput>> new_utxo_pool;
diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
index 9a515828fe..792079e6c6 100644
--- a/src/wallet/test/fuzz/notifications.cpp
+++ b/src/wallet/test/fuzz/notifications.cpp
@@ -106,13 +106,11 @@ struct FuzzedWallet {
     CTxDestination GetDestination(FuzzedDataProvider& fuzzed_data_provider)
     {
         auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)};
-        util::Result<CTxDestination> op_dest{util::Error{}};
         if (fuzzed_data_provider.ConsumeBool()) {
-            op_dest = wallet->GetNewDestination(type, "");
+            return *Assert(wallet->GetNewDestination(type, ""));
         } else {
-            op_dest = wallet->GetNewChangeDestination(type);
+            return *Assert(wallet->GetNewChangeDestination(type));
         }
-        return *Assert(op_dest);
     }
     CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider) { return GetScriptForDestination(GetDestination(fuzzed_data_provider)); }
     void FundTx(FuzzedDataProvider& fuzzed_data_provider, CMutableTransaction tx)
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index 3509bc116f..963c0f838b 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -97,13 +97,11 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
     // so that the recipient's amount is no longer equal to the user's selected target of 299 BTC.
 
     // First case, use 'subtract_fee_from_outputs=true'
-    util::Result<CreatedTransactionResult> res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control);
-    BOOST_CHECK(!res_tx.has_value());
+    BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control));
 
     // Second case, don't use 'subtract_fee_from_outputs'.
     recipients[0].fSubtractFeeFromAmount = false;
-    res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control);
-    BOOST_CHECK(!res_tx.has_value());
+    BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control));
 }
 
 BOOST_AUTO_TEST_SUITE_END()