From cdf6329a064a52d0be15468d1c3afdd72f12ca4c Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 15 Sep 2022 11:15:06 -0400 Subject: [PATCH] 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 | 2 +- src/qt/addresstablemodel.cpp | 2 +- src/test/mempool_tests.cpp | 2 +- src/test/result_tests.cpp | 20 ++++++++++++++++++++ src/util/result.h | 13 +++++++++++++ src/validation.cpp | 2 +- src/wallet/spend.cpp | 14 +++++++------- src/wallet/test/fuzz/notifications.cpp | 4 ++-- src/wallet/test/spend_tests.cpp | 2 +- 9 files changed, 47 insertions(+), 14 deletions(-) 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()); }