diff --git a/src/init.cpp b/src/init.cpp index 988daefeec800..8cb593754c9f9 100644 --- a/src/init.cpp +++ b/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); diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index c52ef7cd67d30..292333a477bbd 100644 --- a/src/qt/addresstablemodel.cpp +++ b/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(); diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 217e4a6d223c8..2858494625fb2 100644 --- a/src/test/mempool_tests.cpp +++ b/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); diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 6a23a7b8950a9..563a798342840 100644 --- a/src/test/result_tests.cpp +++ b/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 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 result2{0}; + ExpectSuccess(result2, {}, 0); + result2.Update({util::Error{Untranslated("error")}}); + ExpectFail(result2, Untranslated("error")); + result2.Update(util::Result{4}); + ExpectSuccess(result2, Untranslated(""), 4); +} + BOOST_AUTO_TEST_CASE(check_value_or) { BOOST_CHECK_EQUAL(IntFn(10, true).value_or(20), 10); diff --git a/src/util/result.h b/src/util/result.h index b99995c7e5620..26c951afc9701 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -39,6 +39,13 @@ class Result std::variant 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 friend bilingual_str ErrorString(const Result& result); @@ -46,6 +53,9 @@ 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; //! std::optional methods, so functions returning optional can change to //! return Result 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 diff --git a/src/validation.cpp b/src/validation.cpp index 81a3c35864995..a872bc96d8332 100644 --- a/src/validation.cpp +++ b/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); } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 5d23ebd35acee..d0a640cf640d5 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 diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 9a515828fef01..d0b06b726e929 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -108,9 +108,9 @@ struct FuzzedWallet { auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)}; util::Result 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); } diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 3509bc116f7aa..640e63903a00e 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/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()); }