refactor: Drop util::Result operator=

In cases where different error conditions need to be handled differently, it is
useful for `util::`Result` objects to be able to hold failure values and error
messages simultaneously. Also it is useful for `util::Result` objects to be
able to hold multiple error message strings and warnings, instead of just
single error strings.

In both of these cases, which are supported in upcoming commits, having an
`operator=` method is potentially dangerous if it clears existing error and
warnings strings while setting result values, or confusing if it doesn't clear
them, so the safest thing is to disable `operator=` and provide an explicit
`Update()` method so callers have a way of setting result values without having to
clear message strings.

src/init.cpp

@@ -993,7 +993,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
     // ********************************************************* Step 3: parameter-to-internal-flags
     auto result = init::SetLoggingCategories(args);
     if (!result) return InitError(util::ErrorString(result));
-    result = init::SetLoggingLevel(args);
+    result.Update(init::SetLoggingLevel(args));
     if (!result) return InitError(util::ErrorString(result));
 
     nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT);

src/qt/addresstablemodel.cpp

@@ -377,7 +377,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
                 editStatus = WALLET_UNLOCK_FAILURE;
                 return QString();
             }
-            op_dest = walletModel->wallet().getNewDestination(address_type, strLabel);
+            op_dest.Update(walletModel->wallet().getNewDestination(address_type, strLabel));
             if (!op_dest) {
                 editStatus = KEY_GENERATION_FAILURE;
                 return QString();

src/test/mempool_tests.cpp

@@ -260,7 +260,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
     tx10.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
     tx10.vout[0].nValue = 10 * COIN;
 
-    ancestors_calculated = pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits());
+    ancestors_calculated.Update(pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits()));
     BOOST_REQUIRE(ancestors_calculated);
     BOOST_CHECK(*ancestors_calculated == setAncestors);
 

src/test/result_tests.cpp

@@ -83,6 +83,26 @@ BOOST_AUTO_TEST_CASE(check_returned)
     ExpectFail(StrFn(Untranslated("S"), false), Untranslated("str S error."));
 }
 
+BOOST_AUTO_TEST_CASE(check_update)
+{
+    // Test using Update method to change a result value from success -> failure,
+    // and failure->success.
+    util::Result<int> result{0};
+    ExpectSuccess(result, {}, 0);
+    result.Update({util::Error{Untranslated("error")}});
+    ExpectFail(result, Untranslated("error"));
+    result.Update(2);
+    ExpectSuccess(result, Untranslated(""), 2);
+
+    // Test the same thing but with non-copyable success and failure types.
+    util::Result<NoCopy> result2{0};
+    ExpectSuccess(result2, {}, 0);
+    result2.Update({util::Error{Untranslated("error")}});
+    ExpectFail(result2, Untranslated("error"));
+    result2.Update(util::Result<NoCopy>{4});
+    ExpectSuccess(result2, Untranslated(""), 4);
+}
+
 BOOST_AUTO_TEST_CASE(check_value_or)
 {
     BOOST_CHECK_EQUAL(IntFn(10, true).value_or(20), 10);

src/util/result.h

@@ -39,13 +39,23 @@ class Result
 
     std::variant<bilingual_str, T> m_variant;
 
+    //! Disallow operator= and require explicit Result::Update() calls 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 messages.
+    Result& operator=(const Result&) = default;
+    Result& operator=(Result&&) = default;
+
     template <typename FT>
     friend bilingual_str ErrorString(const Result<FT>& result);
 
 public:
     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;
 
     //! std::optional methods, so functions returning optional<T> can change to
     //! return Result<T> with minimal changes to existing code, and vice versa.
@@ -75,6 +85,9 @@ class Result
     const T& operator*() const LIFETIMEBOUND { return value(); }
     T* operator->() LIFETIMEBOUND { return &value(); }
     T& operator*() LIFETIMEBOUND { return value(); }
+
+    //! Update this result by moving from another result object.
+    Result& Update(Result&& other) LIFETIMEBOUND { return *this = std::move(other); }
 };
 
 template <typename T>

src/validation.cpp

@@ -953,7 +953,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) {
             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message);
         }
-        ancestors = m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits);
+        ancestors.Update(m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits));
         if (!ancestors) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message);
     }
 

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

src/wallet/test/fuzz/notifications.cpp

@@ -108,9 +108,9 @@ struct FuzzedWallet {
         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, "");
+            op_dest.Update(wallet->GetNewDestination(type, ""));
         } else {
-            op_dest = wallet->GetNewChangeDestination(type);
+            op_dest.Update(wallet->GetNewChangeDestination(type));
         }
         return *Assert(op_dest);
     }

src/wallet/test/spend_tests.cpp

@@ -102,7 +102,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
 
     // Second case, don't use 'subtract_fee_from_outputs'.
     recipients[0].fSubtractFeeFromAmount = false;
-    res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control);
+    res_tx.Update(CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control));
     BOOST_CHECK(!res_tx.has_value());
 }