diff options
-rw-r--r-- | src/init.cpp | 6 | ||||
-rw-r--r-- | src/qt/addresstablemodel.cpp | 11 | ||||
-rw-r--r-- | src/test/mempool_tests.cpp | 16 | ||||
-rw-r--r-- | src/util/result.h | 12 | ||||
-rw-r--r-- | src/validation.cpp | 13 | ||||
-rw-r--r-- | src/wallet/spend.cpp | 30 | ||||
-rw-r--r-- | src/wallet/test/fuzz/coinselection.cpp | 5 | ||||
-rw-r--r-- | src/wallet/test/fuzz/notifications.cpp | 6 | ||||
-rw-r--r-- | src/wallet/test/spend_tests.cpp | 6 |
9 files changed, 60 insertions, 45 deletions
diff --git a/src/init.cpp b/src/init.cpp index c19d596c7f..47d649be3d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -992,10 +992,8 @@ bool AppInitParameterInteraction(const ArgsManager& args) InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections)); // ********************************************************* Step 3: parameter-to-internal-flags - auto result = init::SetLoggingCategories(args); - if (!result) return InitError(util::ErrorString(result)); - result = init::SetLoggingLevel(args); - if (!result) return InitError(util::ErrorString(result)); + if (auto result{init::SetLoggingCategories(args)}; !result) return InitError(util::ErrorString(result)); + if (auto result{init::SetLoggingLevel(args)}; !result) return InitError(util::ErrorString(result)); nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT); if (nConnectTimeout <= 0) { diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index c52ef7cd67..efdc3966d1 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -369,21 +369,22 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con else if(type == Receive) { // Generate a new address to associate with given label - auto op_dest = walletModel->wallet().getNewDestination(address_type, strLabel); - if (!op_dest) { + if (auto dest{walletModel->wallet().getNewDestination(address_type, strLabel)}) { + strAddress = EncodeDestination(*dest); + } else { WalletModel::UnlockContext ctx(walletModel->requestUnlock()); if (!ctx.isValid()) { // Unlock wallet failed or was cancelled editStatus = WALLET_UNLOCK_FAILURE; return QString(); } - op_dest = walletModel->wallet().getNewDestination(address_type, strLabel); - if (!op_dest) { + if (auto dest_retry{walletModel->wallet().getNewDestination(address_type, strLabel)}) { + strAddress = EncodeDestination(*dest_retry); + } else { editStatus = KEY_GENERATION_FAILURE; return QString(); } } - strAddress = EncodeDestination(*op_dest); } else { diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 217e4a6d22..9f8d434213 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -202,9 +202,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx7.vout[1].nValue = 1 * COIN; - auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(2000000LL).FromTx(tx7), CTxMemPool::Limits::NoLimits())}; - BOOST_REQUIRE(ancestors_calculated.has_value()); - BOOST_CHECK(*ancestors_calculated == setAncestors); + { + auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(2000000LL).FromTx(tx7), CTxMemPool::Limits::NoLimits())}; + BOOST_REQUIRE(ancestors_calculated.has_value()); + BOOST_CHECK(*ancestors_calculated == setAncestors); + } pool.addUnchecked(entry.FromTx(tx7), setAncestors); BOOST_CHECK_EQUAL(pool.size(), 7U); @@ -260,9 +262,11 @@ 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()); - BOOST_REQUIRE(ancestors_calculated); - BOOST_CHECK(*ancestors_calculated == setAncestors); + { + auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits())}; + BOOST_REQUIRE(ancestors_calculated); + BOOST_CHECK(*ancestors_calculated == setAncestors); + } pool.addUnchecked(entry.FromTx(tx10), setAncestors); diff --git a/src/util/result.h b/src/util/result.h index b99995c7e5..122a7638fa 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -39,6 +39,16 @@ private: std::variant<bilingual_str, T> 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 + //! messages. + Result& operator=(const Result&) = delete; + Result& operator=(Result&&) = delete; + template <typename FT> friend bilingual_str ErrorString(const Result<FT>& result); @@ -46,6 +56,8 @@ 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(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. diff --git a/src/validation.cpp b/src/validation.cpp index 903f9caf13..f57851b4f7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -946,8 +946,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}; - if (!ancestors) { + if (auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}) { + ws.m_ancestors = std::move(*ancestors); + } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. // Contracting/payment channels CPFP carve-out: // If the new transaction is relatively small (up to 40k weight) @@ -970,11 +971,13 @@ 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); - if (!ancestors) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); + if (auto ancestors_retry{m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits)}) { + ws.m_ancestors = std::move(*ancestors_retry); + } else { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); + } } - ws.m_ancestors = *ancestors; // Even though just checking direct mempool parents for inheritance would be sufficient, we // check using the full ancestor set here because it's more convenient to use what we have // already calculated. 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() |