From 4d90ce54e7d58bdeb5fe7e30de414b4d81c7957f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 18 Jul 2022 15:48:04 -0400 Subject: [PATCH 1/8] refactor: Add util::Result failure values Add util::Result support for returning more error information. For better error handling, adds the ability to return a value on failure, not just a value on success. This is a key missing feature that made the result class not useful for functions like LoadChainstate() which produce different errors that need to be handled differently. This change also makes some more minor improvements: - Smaller type size. util::Result is 16 bytes, and util::Result is 8 bytes. Previously util::Result and util::Result were 72 bytes. - More documentation and tests. --- src/test/result_tests.cpp | 112 +++++++++++++-- src/util/result.h | 278 ++++++++++++++++++++++++++++++-------- src/wallet/wallet.cpp | 2 +- 3 files changed, 321 insertions(+), 71 deletions(-) diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 4284864422d..9d776d847f4 100644 --- a/src/test/result_tests.cpp +++ b/src/test/result_tests.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include @@ -33,6 +34,34 @@ std::ostream& operator<<(std::ostream& os, const NoCopy& o) return os << "NoCopy(" << *o.m_n << ")"; } +struct NoCopyNoMove { + NoCopyNoMove(int n) : m_n{n} {} + NoCopyNoMove(const NoCopyNoMove&) = delete; + NoCopyNoMove(NoCopyNoMove&&) = delete; + int m_n; +}; + +bool operator==(const NoCopyNoMove& a, const NoCopyNoMove& b) +{ + return a.m_n == b.m_n; +} + +std::ostream& operator<<(std::ostream& os, const NoCopyNoMove& o) +{ + os << "NoCopyNoMove(" << o.m_n << ")"; + return os; +} + +util::Result VoidSuccessFn() +{ + return {}; +} + +util::Result VoidFailFn() +{ + return util::Error{Untranslated("void fail.")}; +} + util::Result IntFn(int i, bool success) { if (success) return i; @@ -45,21 +74,46 @@ util::Result StrFn(bilingual_str s, bool success) return util::Error{Untranslated(strprintf("str %s error.", s.original))}; } -util::Result NoCopyFn(int i, bool success) +util::Result NoCopyFn(int i, bool success) { if (success) return {i}; - return util::Error{Untranslated(strprintf("nocopy %i error.", i))}; + return {util::Error{Untranslated(strprintf("nocopy %i error.", i))}, i}; } -template -void ExpectResult(const util::Result& result, bool success, const bilingual_str& str) +util::Result NoCopyNoMoveFn(int i, bool success) +{ + if (success) return {i}; + return {util::Error{Untranslated(strprintf("nocopynomove %i error.", i))}, i}; +} + +enum FnError { ERR1, ERR2 }; + +util::Result IntFailFn(int i, bool success) +{ + if (success) return i; + return {util::Error{Untranslated(strprintf("int %i error.", i))}, i % 2 ? ERR1 : ERR2}; +} + +util::Result EnumFailFn(FnError ret) +{ + return {util::Error{Untranslated("enum fail.")}, ret}; +} + +util::Result TruthyFalsyFn(int i, bool success) +{ + if (success) return i; + return {util::Error{Untranslated(strprintf("failure value %i.", i))}, i}; +} + +template +void ExpectResult(const util::Result& result, bool success, const bilingual_str& str) { BOOST_CHECK_EQUAL(bool(result), success); BOOST_CHECK_EQUAL(util::ErrorString(result), str); } -template -void ExpectSuccess(const util::Result& result, const bilingual_str& str, Args&&... args) +template +void ExpectSuccess(const util::Result& result, const bilingual_str& str, Args&&... args) { ExpectResult(result, true, str); BOOST_CHECK_EQUAL(result.has_value(), true); @@ -68,20 +122,42 @@ void ExpectSuccess(const util::Result& result, const bilingual_str& str, Args BOOST_CHECK_EQUAL(&result.value(), &*result); } -template -void ExpectFail(const util::Result& result, const bilingual_str& str) +template +void ExpectFail(const util::Result& result, bilingual_str str, Args&&... args) { ExpectResult(result, false, str); + F expect_failure{std::forward(args)...}; + BOOST_CHECK_EQUAL(result.GetFailure(), expect_failure); } BOOST_AUTO_TEST_CASE(check_returned) { + ExpectResult(VoidSuccessFn(), true, {}); + ExpectResult(VoidFailFn(), false, Untranslated("void fail.")); ExpectSuccess(IntFn(5, true), {}, 5); - ExpectFail(IntFn(5, false), Untranslated("int 5 error.")); + ExpectResult(IntFn(5, false), false, Untranslated("int 5 error.")); ExpectSuccess(NoCopyFn(5, true), {}, 5); - ExpectFail(NoCopyFn(5, false), Untranslated("nocopy 5 error.")); + ExpectFail(NoCopyFn(5, false), Untranslated("nocopy 5 error."), 5); + ExpectSuccess(NoCopyNoMoveFn(5, true), {}, 5); + ExpectFail(NoCopyNoMoveFn(5, false), Untranslated("nocopynomove 5 error."), 5); ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S")); - ExpectFail(StrFn(Untranslated("S"), false), Untranslated("str S error.")); + ExpectResult(StrFn(Untranslated("S"), false), false, Untranslated("str S error.")); + ExpectFail(EnumFailFn(ERR2), Untranslated("enum fail."), ERR2); + ExpectSuccess(TruthyFalsyFn(0, true), {}, 0); + ExpectFail(TruthyFalsyFn(0, false), Untranslated("failure value 0."), 0); + ExpectSuccess(TruthyFalsyFn(1, true), {}, 1); + ExpectFail(TruthyFalsyFn(1, false), Untranslated("failure value 1."), 1); +} + +BOOST_AUTO_TEST_CASE(check_dereference_operators) +{ + util::Result> mutable_result; + const auto& const_result{mutable_result}; + mutable_result.value() = {1, "23"}; + BOOST_CHECK_EQUAL(mutable_result->first, 1); + BOOST_CHECK_EQUAL(const_result->second, "23"); + (*mutable_result).first = 5; + BOOST_CHECK_EQUAL((*const_result).first, 5); } BOOST_AUTO_TEST_CASE(check_value_or) @@ -94,4 +170,18 @@ BOOST_AUTO_TEST_CASE(check_value_or) BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B")); } +struct Derived : NoCopyNoMove { + using NoCopyNoMove::NoCopyNoMove; +}; + +util::Result> DerivedToBaseFn(util::Result> derived) +{ + return derived; +} + +BOOST_AUTO_TEST_CASE(derived_to_base) +{ + BOOST_CHECK_EQUAL(*Assert(*Assert(DerivedToBaseFn(std::make_unique(5)))), 5); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/result.h b/src/util/result.h index 122a7638fad..f77e7a246fd 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -8,91 +8,251 @@ #include #include -#include +#include +#include +#include +#include +#include +#include +#include namespace util { +//! The Result class provides +//! an efficient way for functions to return structured result information, as +//! well as descriptive error messages. +//! +//! Logically, a result object is equivalent to: +//! +//! tuple, MessagesType> +//! +//! But the physical representation is more efficient because it avoids +//! allocating memory for FailureType and MessagesType fields unless there is an +//! error. +//! +//! Result objects support the same operators as +//! std::optional, such as !result, *result, result->member, to +//! make SuccessType values easy to access. They also provide +//! result.GetFailure() and result.GetMessages() methods to +//! access other parts of the result. A simple usage example is: +//! +//! util::Result AddNumbers(int a, int b) +//! { +//! if (b == 0) return util::Error{_("Not adding 0, that's dumb.")}; +//! return a + b; +//! } +//! +//! void TryAddNumbers(int a, int b) +//! { +//! if (auto result = AddNumbers(a, b)) { +//! LogPrintf("%i + %i = %i\n", a, b, *result); +//! } else { +//! LogPrintf("Error: %s\n", util::ErrorString(result).translated); +//! } +//! } +//! +//! The `Result` class is intended to be used for high-level functions that need +//! to report error messages to end users. Low-level functions that don't need +//! error-reporting and only need error-handling should avoid `Result` and +//! instead use standard classes like `std::optional`, `std::variant`, +//! `std::expected`, and `std::tuple`, or custom structs and enum types to +//! return function results. +//! +//! Usage examples can be found in \example ../test/result_tests.cpp. +template +class Result; +//! Wrapper object to pass an error string to the Result constructor. struct Error { bilingual_str message; }; -//! The util::Result class provides a standard way for functions to return -//! either error messages or result values. -//! -//! It is intended for high-level functions that need to report error strings to -//! end users. Lower-level functions that don't need this error-reporting and -//! only need error-handling should avoid util::Result and instead use standard -//! classes like std::optional, std::variant, and std::tuple, or custom structs -//! and enum types to return function results. -//! -//! Usage examples can be found in \example ../test/result_tests.cpp, but in -//! general code returning `util::Result` values is very similar to code -//! returning `std::optional` values. Existing functions returning -//! `std::optional` can be updated to return `util::Result` and return -//! error strings usually just replacing `return std::nullopt;` with `return -//! util::Error{error_string};`. -template -class Result +namespace detail { +//! Substitute for std::monostate that doesn't depend on std::variant. +struct Monostate{}; + +//! Implemention note: Result class inherits from a FailDataHolder class holding +//! a unique_ptr to FailureType and MessagesTypes values, and a SuccessHolder +//! class holding a SuccessType value in an anonymous union. +//! @{ +//! Container for FailureType and MessagesType, providing public operator +//! bool(), GetFailure(), GetMessages(), and EnsureMessages() methods. +template +class FailDataHolder { -private: - using T = std::conditional_t, std::monostate, M>; +protected: + struct FailData { + std::optional, Monostate, FailureType>> failure{}; + MessagesType messages{}; + }; + std::unique_ptr m_fail_data; - std::variant m_variant; + // Private accessor, create FailData if it doesn't exist. + FailData& EnsureFailData() LIFETIMEBOUND + { + if (!m_fail_data) m_fail_data = std::make_unique(); + return *m_fail_data; + } - //! Disallow copy constructor, require Result to be moved for efficiency. - Result(const Result&) = delete; +public: + // Public accessors. + explicit operator bool() const { return !m_fail_data || !m_fail_data->failure; } + const auto& GetFailure() const LIFETIMEBOUND { assert(!*this); return *m_fail_data->failure; } + auto& GetFailure() LIFETIMEBOUND { assert(!*this); return *m_fail_data->failure; } + const auto* GetMessages() const LIFETIMEBOUND { return m_fail_data ? &m_fail_data->messages : nullptr; } + auto* GetMessages() LIFETIMEBOUND { return m_fail_data ? &m_fail_data->messages : nullptr; } + auto& EnsureMessages() LIFETIMEBOUND { return EnsureFailData().messages; } +}; + +//! Container for SuccessType, providing public accessor methods similar to +//! std::optional methods to access the success value. +template +class SuccessHolder : public FailDataHolder +{ +protected: + //! Success value embedded in an anonymous union so it doesn't need to be + //! constructed if the result is holding a failure value, + union { SuccessType m_success; }; + + //! Empty constructor that needs to be declared because the class contains a union. + SuccessHolder() {} + ~SuccessHolder() { if (*this) m_success.~SuccessType(); } + +public: + // Public accessors. + bool has_value() const { return bool{*this}; } + const SuccessType& value() const LIFETIMEBOUND { assert(has_value()); return m_success; } + SuccessType& value() LIFETIMEBOUND { assert(has_value()); return m_success; } + template + SuccessType value_or(U&& default_value) const& + { + return has_value() ? value() : std::forward(default_value); + } + template + SuccessType value_or(U&& default_value) && + { + return has_value() ? std::move(value()) : std::forward(default_value); + } + const SuccessType* operator->() const LIFETIMEBOUND { return &value(); } + const SuccessType& operator*() const LIFETIMEBOUND { return value(); } + SuccessType* operator->() LIFETIMEBOUND { return &value(); } + SuccessType& operator*() LIFETIMEBOUND { return value(); } +}; + +//! Specialization of SuccessHolder when SuccessType is void. +template +class SuccessHolder : public FailDataHolder +{ +}; +//! @} +} // namespace detail + +// Result type class, documented at the top of this file. +template +class Result : public detail::SuccessHolder +{ +public: + using SuccessType = SuccessType_; + using FailureType = FailureType_; + using MessagesType = MessagesType_; + static constexpr bool is_result{true}; + + //! Construct a Result object setting a success or failure value and + //! optional error messages. Initial util::Error arguments are processed + //! first to add error messages. + //! Then, any remaining arguments are passed to the SuccessType + //! constructor and used to construct a success value in the success case. + //! In the failure case, if any util::Error arguments were passed, any + //! remaining arguments are passed to the FailureType constructor and used + //! to construct a failure value. + template + Result(Args&&... args) + { + Construct(*this, std::forward(args)...); + } + + //! Move-construct a Result object from another Result object, moving the + //! success or failure value and any error message. + template + requires (std::decay_t::is_result) + Result(O&& other) + { + Move(*this, other); + } //! 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 - friend bilingual_str ErrorString(const Result& result); +protected: + template + friend class 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(Result&&) = default; - ~Result() = default; + //! Helper function to construct a new success or failure value using the + //! arguments provided. + template + static void Construct(Result& result, Args&&... args) + { + if constexpr (Failure) { + static_assert(sizeof...(args) > 0 || !std::is_scalar_v, + "Refusing to default-construct failure value with int, float, enum, or pointer type, please specify an explicit failure value."); + result.EnsureFailData().failure.emplace(std::forward(args)...); + } else if constexpr (std::is_same_v) { + static_assert(sizeof...(args) == 0, "Error: Arguments cannot be passed to a Result constructor."); + } else { + new (&result.m_success) typename Result::SuccessType{std::forward(args)...}; + } + } - //! std::optional methods, so functions returning optional can change to - //! return Result with minimal changes to existing code, and vice versa. - bool has_value() const noexcept { return m_variant.index() == 1; } - const T& value() const LIFETIMEBOUND + //! Construct() overload peeling off a util::Error constructor argument. + template + static void Construct(Result& result, util::Error error, Args&&... args) { - assert(has_value()); - return std::get<1>(m_variant); + result.EnsureFailData().messages = std::move(error.message); + Construct(result, std::forward(args)...); } - T& value() LIFETIMEBOUND + + //! Move success, failure, and messages from source Result object to + //! destination object. The source and + //! destination results are not required to have the same types, and + //! assigning void source types to non-void destinations type is allowed, + //! since no source information is lost. But assigning non-void source types + //! to void destination types is not allowed, since this would discard + //! source information. + template + static void Move(DstResult& dst, SrcResult& src) { - assert(has_value()); - return std::get<1>(m_variant); + // Move messages values first, then move success or failure value below. + if (src.GetMessages() && !src.GetMessages()->empty()) { + dst.EnsureMessages() = std::move(*src.GetMessages()); + } + if (src) { + // src has a success value, so move it to dst. If the src success + // type is void and the dst success type is non-void, just + // initialize the dst success value by default initialization. + if constexpr (!std::is_same_v) { + new (&dst.m_success) typename DstResult::SuccessType{std::move(src.m_success)}; + } else if constexpr (!std::is_same_v) { + new (&dst.m_success) typename DstResult::SuccessType{}; + } + } else { + // src has a failure value, so move it to dst. If the src failure + // type is void, just initialize the dst failure value by default + // initialization. + if constexpr (!std::is_same_v) { + dst.EnsureFailData().failure.emplace(std::move(src.GetFailure())); + } else { + dst.EnsureFailData().failure.emplace(); + } + } } - template - T value_or(U&& default_value) const& - { - return has_value() ? value() : std::forward(default_value); - } - template - T value_or(U&& default_value) && - { - return has_value() ? std::move(value()) : std::forward(default_value); - } - explicit operator bool() const noexcept { return has_value(); } - const T* operator->() const LIFETIMEBOUND { return &value(); } - const T& operator*() const LIFETIMEBOUND { return value(); } - T* operator->() LIFETIMEBOUND { return &value(); } - T& operator*() LIFETIMEBOUND { return value(); } }; -template -bilingual_str ErrorString(const Result& result) +template +bilingual_str ErrorString(const Result& result) { - return result ? bilingual_str{} : std::get<0>(result.m_variant); + return !result && result.GetMessages() ? *result.GetMessages() : bilingual_str{}; } } // namespace util diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7abd17d31e9..fe455f528a4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2409,7 +2409,7 @@ util::Result CWallet::RemoveTxs(std::vector& txs_to_remove) bool was_txn_committed = RunWithinTxn(GetDatabase(), /*process_desc=*/"remove transactions", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { util::Result result{RemoveTxs(batch, txs_to_remove)}; if (!result) str_err = util::ErrorString(result); - return result.has_value(); + return bool{result}; }); if (!str_err.empty()) return util::Error{str_err}; if (!was_txn_committed) return util::Error{_("Error starting/committing db txn for wallet transactions removal process")}; From 4140f871d604b98adc3c7fba5cd5a69a8e576f1d Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 19 Jul 2024 08:07:35 -0400 Subject: [PATCH 2/8] wallet: fix clang-tidy warning performance-no-automatic-move Previous commit causes the warning to be triggered, but the suboptimal return from const statement was added earlier in 517e204bacd9d. Warning was causing CI failure https://cirrus-ci.com/task/6159914970644480 --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fe455f528a4..37fe164521a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2972,7 +2972,7 @@ static util::Result GetWalletPath(const std::string& name) // 2. Path to an existing directory. // 3. Path to a symlink to a directory. // 4. For backwards compatibility, the name of a data file in -walletdir. - const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); + fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); fs::file_type path_type = fs::symlink_status(wallet_path).type(); if (!(path_type == fs::file_type::not_found || path_type == fs::file_type::directory || (path_type == fs::file_type::symlink && fs::is_directory(wallet_path)) || From 6a18bcfcf87d3a25ab6f16d0dccf9bd5b9f9b2dd Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 18 Jul 2022 15:48:04 -0400 Subject: [PATCH 3/8] refactor: Add util::Result::Update() method Add util::Result Update method to make it possible to change the value of an existing Result object. This will be useful for functions that can return multiple error and warning messages, and want to be able to change the result value while keeping existing messages. --- src/test/result_tests.cpp | 20 ++++++++++++++++++++ src/util/result.h | 40 +++++++++++++++++++++++++++++++++------ src/wallet/wallet.cpp | 10 ++++------ 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 9d776d847f4..c58ae82442a 100644 --- a/src/test/result_tests.cpp +++ b/src/test/result_tests.cpp @@ -149,6 +149,26 @@ BOOST_AUTO_TEST_CASE(check_returned) ExpectFail(TruthyFalsyFn(1, false), Untranslated("failure value 1."), 1); } +BOOST_AUTO_TEST_CASE(check_update) +{ + // Test using Update method to change a result value from success -> failure, + // and failure->success. + util::Result result; + ExpectSuccess(result, {}, 0); + result.Update({util::Error{Untranslated("error")}, ERR1}); + ExpectFail(result, Untranslated("error"), ERR1); + 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")}, 3}); + ExpectFail(result2, Untranslated("error"), 3); + result2.Update(4); + ExpectSuccess(result2, Untranslated(""), 4); +} + BOOST_AUTO_TEST_CASE(check_dereference_operators) { util::Result> mutable_result; diff --git a/src/util/result.h b/src/util/result.h index f77e7a246fd..8706220aa90 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -176,13 +176,20 @@ public: requires (std::decay_t::is_result) Result(O&& other) { - Move(*this, other); + Move(*this, other); } - //! 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. + //! Update this result by moving from another result object. + Result& Update(Result&& other) LIFETIMEBOUND + { + Move(*this, other); + return *this; + } + + //! 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=(Result&&) = delete; protected: @@ -220,13 +227,32 @@ protected: //! since no source information is lost. But assigning non-void source types //! to void destination types is not allowed, since this would discard //! source information. - template + template static void Move(DstResult& dst, SrcResult& src) { // Move messages values first, then move success or failure value below. if (src.GetMessages() && !src.GetMessages()->empty()) { dst.EnsureMessages() = std::move(*src.GetMessages()); } + // If DstConstructed is true, it means dst has either a success value or + // a failure value set, which needs to be destroyed and replaced. If + // DstConstructed is false, then dst is being constructed now and has no + // values set. + if constexpr (DstConstructed) { + if (dst) { + // dst has a success value, so destroy it before replacing it with src failure value + if constexpr (!std::is_same_v) { + using DstSuccess = typename DstResult::SuccessType; + dst.m_success.~DstSuccess(); + } + } else { + // dst has a failure value, so reset it before replacing it with src success value + dst.m_fail_data->failure.reset(); + } + } + // At this point dst has no success or failure value set. Can assert + // there is no failure value. + assert(dst); if (src) { // src has a success value, so move it to dst. If the src success // type is void and the dst success type is non-void, just @@ -236,6 +262,7 @@ protected: } else if constexpr (!std::is_same_v) { new (&dst.m_success) typename DstResult::SuccessType{}; } + assert(dst); } else { // src has a failure value, so move it to dst. If the src failure // type is void, just initialize the dst failure value by default @@ -245,6 +272,7 @@ protected: } else { dst.EnsureFailData().failure.emplace(); } + assert(!dst); } } }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 37fe164521a..d2504609116 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2405,15 +2405,13 @@ DBErrors CWallet::LoadWallet() util::Result CWallet::RemoveTxs(std::vector& txs_to_remove) { AssertLockHeld(cs_wallet); + util::Result result; bilingual_str str_err; // future: make RunWithinTxn return a util::Result bool was_txn_committed = RunWithinTxn(GetDatabase(), /*process_desc=*/"remove transactions", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { - util::Result result{RemoveTxs(batch, txs_to_remove)}; - if (!result) str_err = util::ErrorString(result); - return bool{result}; + return bool{result.Update(RemoveTxs(batch, txs_to_remove))}; }); - if (!str_err.empty()) return util::Error{str_err}; - if (!was_txn_committed) return util::Error{_("Error starting/committing db txn for wallet transactions removal process")}; - return {}; // all good + if (result && !was_txn_committed) result.Update(util::Error{_("Error starting/committing db txn for wallet transactions removal process")}); + return result; } util::Result CWallet::RemoveTxs(WalletBatch& batch, std::vector& txs_to_remove) From 48949d1c601b7ff169cea3f1167adc718d1f8186 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 1 Nov 2024 20:14:04 -0400 Subject: [PATCH 4/8] refactor: Use util::Result class in LoadChainstate and VerifyLoadedChainstate --- src/bitcoin-chainstate.cpp | 8 +-- src/init.cpp | 42 +++++++-------- src/node/chainstate.cpp | 95 ++++++++++++++++++++-------------- src/node/chainstate.h | 28 +++------- src/test/util/setup_common.cpp | 8 +-- 5 files changed, 91 insertions(+), 90 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index e657f018715..8be48d733fd 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -129,13 +129,13 @@ int main(int argc, char* argv[]) ChainstateManager chainman{interrupt, chainman_opts, blockman_opts}; node::ChainstateLoadOptions options; - auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options); - if (status != node::ChainstateLoadStatus::SUCCESS) { + auto load_result{node::LoadChainstate(chainman, cache_sizes, options)}; + if (!load_result) { std::cerr << "Failed to load Chain state from your datadir." << std::endl; goto epilogue; } else { - std::tie(status, error) = node::VerifyLoadedChainstate(chainman, options); - if (status != node::ChainstateLoadStatus::SUCCESS) { + auto verify_result{node::VerifyLoadedChainstate(chainman, options)}; + if (!verify_result) { std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl; goto epilogue; } diff --git a/src/init.cpp b/src/init.cpp index 7fdbf75dc66..95009c47da1 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -120,12 +120,11 @@ using common::AmountErrMsg; using common::InvalidPortErrMsg; using common::ResolveErrMsg; - +using kernel::InterruptResult; using node::ApplyArgsManOptions; using node::BlockManager; using node::CalculateCacheSizes; -using node::ChainstateLoadResult; -using node::ChainstateLoadStatus; +using node::ChainstateLoadError; using node::DEFAULT_PERSIST_MEMPOOL; using node::DEFAULT_PRINT_MODIFIED_FEE; using node::DEFAULT_STOPATHEIGHT; @@ -1221,7 +1220,7 @@ bool CheckHostPortOptions(const ArgsManager& args) { // A GUI user may opt to retry once with do_reindex set if there is a failure during chainstate initialization. // The function therefore has to support re-entry. -static ChainstateLoadResult InitAndLoadChainstate( +util::Result InitAndLoadChainstate( NodeContext& node, bool do_reindex, const bool do_reindex_chainstate, @@ -1237,7 +1236,7 @@ static ChainstateLoadResult InitAndLoadChainstate( bilingual_str mempool_error; node.mempool = std::make_unique(mempool_opts, mempool_error); if (!mempool_error.empty()) { - return {ChainstateLoadStatus::FAILURE_FATAL, mempool_error}; + return {util::Error{mempool_error}, ChainstateLoadError::FAILURE_FATAL}; } LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024)); ChainstateManager::Options chainman_opts{ @@ -1267,12 +1266,12 @@ static ChainstateLoadResult InitAndLoadChainstate( node.chainman = std::make_unique(*Assert(node.shutdown_signal), chainman_opts, blockman_opts); } catch (dbwrapper_error& e) { LogError("%s", e.what()); - return {ChainstateLoadStatus::FAILURE, _("Error opening block database")}; + return {util::Error{_("Error opening block database")}, ChainstateLoadError::FAILURE}; } catch (std::exception& e) { - return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()))}; + return {util::Error{Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()))}, ChainstateLoadError::FAILURE_FATAL}; } ChainstateManager& chainman = *node.chainman; - if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}}; + if (chainman.m_interrupt) return kernel::Interrupted{}; // This is defined and set here instead of inline in validation.h to avoid a hard // dependency between validation and index/base, since the latter is not in @@ -1307,27 +1306,27 @@ static ChainstateLoadResult InitAndLoadChainstate( "", CClientUIInterface::MSG_ERROR); }; uiInterface.InitMessage(_("Loading block index…")); - auto catch_exceptions = [](auto&& f) -> ChainstateLoadResult { + auto catch_exceptions = [](auto&& f) -> util::Result { try { return f(); } catch (const std::exception& e) { LogError("%s\n", e.what()); - return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error loading databases")); + return {util::Error{_("Error loading databases")}, node::ChainstateLoadError::FAILURE}; } }; - auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); }); - if (status == node::ChainstateLoadStatus::SUCCESS) { + auto result = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); }); + if (result && !IsInterrupted(*result)) { uiInterface.InitMessage(_("Verifying blocks…")); if (chainman.m_blockman.m_have_pruned && options.check_blocks > MIN_BLOCKS_TO_KEEP) { LogWarning("pruned datadir may not have more than %d blocks; only checking available blocks\n", MIN_BLOCKS_TO_KEEP); } - std::tie(status, error) = catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); }); - if (status == node::ChainstateLoadStatus::SUCCESS) { + result.Update(catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); })); + if (result && !IsInterrupted(*result)) { LogInfo("Block index and chainstate loaded"); } } - return {status, error}; + return result; }; bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) @@ -1685,14 +1684,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) const bool do_reindex_chainstate{args.GetBoolArg("-reindex-chainstate", false)}; // Chainstate initialization and loading may be retried once with reindexing by GUI users - auto [status, error] = InitAndLoadChainstate( + auto result = InitAndLoadChainstate( node, do_reindex, do_reindex_chainstate, kernel_cache_sizes, args); - if (status == ChainstateLoadStatus::FAILURE && !do_reindex && !ShutdownRequested(node)) { + if (!result && result.GetFailure() == ChainstateLoadError::FAILURE && !do_reindex && !ShutdownRequested(node)) { // suggest a reindex + const auto& error = util::ErrorString(result); bool do_retry = uiInterface.ThreadSafeQuestion( error + Untranslated(".\n\n") + _("Do you want to rebuild the databases now?"), error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.", @@ -1704,15 +1704,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (!Assert(node.shutdown_signal)->reset()) { LogError("Internal error: failed to reset shutdown signal.\n"); } - std::tie(status, error) = InitAndLoadChainstate( + result.Update(InitAndLoadChainstate( node, do_reindex, do_reindex_chainstate, kernel_cache_sizes, - args); + args)); } - if (status != ChainstateLoadStatus::SUCCESS && status != ChainstateLoadStatus::INTERRUPTED) { - return InitError(error); + if (!result) { + return InitError(util::ErrorString(result)); } // As LoadBlockIndex can take several minutes, it's possible the user diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index c88bd5bad23..89437db5a3d 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -27,35 +27,40 @@ #include using kernel::CacheSizes; +using kernel::Interrupted; +using kernel::InterruptResult; namespace node { // Complete initialization of chainstates after the initial call has been made // to ChainstateManager::InitializeChainstate(). -static ChainstateLoadResult CompleteChainstateInitialization( +static util::Result CompleteChainstateInitialization( ChainstateManager& chainman, const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}}; + if (chainman.m_interrupt) return Interrupted{}; // LoadBlockIndex will load m_have_pruned if we've ever removed a // block file from disk. // Note that it also sets m_blockfiles_indexed based on the disk flag! if (!chainman.LoadBlockIndex()) { - if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}}; - return {ChainstateLoadStatus::FAILURE, _("Error loading block database")}; + if (chainman.m_interrupt) { + return Interrupted{}; + } else { + return {util::Error{_("Error loading block database")}, ChainstateLoadError::FAILURE}; + } } if (!chainman.BlockIndex().empty() && !chainman.m_blockman.LookupBlockIndex(chainman.GetConsensus().hashGenesisBlock)) { // If the loaded chain has a wrong genesis, bail out immediately // (we're likely using a testnet datadir, or the other way around). - return {ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB, _("Incorrect or no genesis block found. Wrong datadir for network?")}; + return {util::Error{_("Incorrect or no genesis block found. Wrong datadir for network?")}, ChainstateLoadError::FAILURE_INCOMPATIBLE_DB}; } // Check for changed -prune state. What we are concerned about is a user who has pruned blocks // in the past, but is now trying to run unpruned. if (chainman.m_blockman.m_have_pruned && !options.prune) { - return {ChainstateLoadStatus::FAILURE, _("You need to rebuild the database using -reindex to go back to unpruned mode. This will redownload the entire blockchain")}; + return {util::Error{_("You need to rebuild the database using -reindex to go back to unpruned mode. This will redownload the entire blockchain")}, ChainstateLoadError::FAILURE}; } // At this point blocktree args are consistent with what's on disk. @@ -63,7 +68,7 @@ static ChainstateLoadResult CompleteChainstateInitialization( // (otherwise we use the one already on disk). // This is called again in ImportBlocks after the reindex completes. if (chainman.m_blockman.m_blockfiles_indexed && !chainman.ActiveChainstate().LoadGenesisBlock()) { - return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")}; + return {util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE}; } auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { @@ -93,7 +98,7 @@ static ChainstateLoadResult CompleteChainstateInitialization( /*should_wipe=*/options.wipe_chainstate_db); } catch (dbwrapper_error& err) { LogError("%s\n", err.what()); - return {ChainstateLoadStatus::FAILURE, _("Error opening coins database")}; + return {util::Error{_("Error opening coins database")}, ChainstateLoadError::FAILURE}; } if (options.coins_error_cb) { @@ -103,14 +108,15 @@ static ChainstateLoadResult CompleteChainstateInitialization( // Refuse to load unsupported database format. // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate if (chainstate->CoinsDB().NeedsUpgrade()) { - return {ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB, _("Unsupported chainstate database format found. " - "Please restart with -reindex-chainstate. This will " - "rebuild the chainstate database.")}; + return {util::Error{_("Unsupported chainstate database format found. " + "Please restart with -reindex-chainstate. This will " + "rebuild the chainstate database.")}, + ChainstateLoadError::FAILURE_INCOMPATIBLE_DB}; } // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate if (!chainstate->ReplayBlocks()) { - return {ChainstateLoadStatus::FAILURE, _("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.")}; + return {util::Error{_("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.")}, ChainstateLoadError::FAILURE}; } // The on-disk coinsdb is now in a good state, create the cache @@ -120,7 +126,7 @@ static ChainstateLoadResult CompleteChainstateInitialization( if (!is_coinsview_empty(chainstate)) { // LoadChainTip initializes the chain based on CoinsTip()'s best block if (!chainstate->LoadChainTip()) { - return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")}; + return {util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE}; } assert(chainstate->m_chain.Tip() != nullptr); } @@ -129,8 +135,8 @@ static ChainstateLoadResult CompleteChainstateInitialization( auto chainstates{chainman.GetAll()}; if (std::any_of(chainstates.begin(), chainstates.end(), [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) { - return {ChainstateLoadStatus::FAILURE, strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."), - chainman.GetConsensus().SegwitHeight)}; + return {util::Error{strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."), + chainman.GetConsensus().SegwitHeight)}, ChainstateLoadError::FAILURE}; }; // Now that chainstates are loaded and we're able to flush to @@ -138,12 +144,13 @@ static ChainstateLoadResult CompleteChainstateInitialization( // on the condition of each chainstate. chainman.MaybeRebalanceCaches(); - return {ChainstateLoadStatus::SUCCESS, {}}; + return {}; } -ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes, - const ChainstateLoadOptions& options) +util::Result LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes, + const ChainstateLoadOptions& options) { + util::Result result; if (!chainman.AssumedValidBlock().IsNull()) { LogPrintf("Assuming ancestors of block %s have valid signatures.\n", chainman.AssumedValidBlock().GetHex()); } else { @@ -173,13 +180,14 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize if (has_snapshot && options.wipe_chainstate_db) { LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n"); if (!chainman.DeleteSnapshotChainstate()) { - return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")}; + result.Update({util::Error{Untranslated("Couldn't remove snapshot chainstate.")}, ChainstateLoadError::FAILURE_FATAL}); + return result; } } - auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options); - if (init_status != ChainstateLoadStatus::SUCCESS) { - return {init_status, init_error}; + result.Update(CompleteChainstateInitialization(chainman, options)); + if (!result || IsInterrupted(*result)) { + return result; } // If a snapshot chainstate was fully validated by a background chainstate during @@ -197,7 +205,8 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) { LogPrintf("[snapshot] cleaning up unneeded background chainstate, then reinitializing\n"); if (!chainman.ValidatedSnapshotCleanup()) { - return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Background chainstate cleanup failed unexpectedly.")}; + result.Update({util::Error{Untranslated("Background chainstate cleanup failed unexpectedly.")}, ChainstateLoadError::FAILURE_FATAL}); + return result; } // Because ValidatedSnapshotCleanup() has torn down chainstates with @@ -213,20 +222,22 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // for the fully validated chainstate. chainman.ActiveChainstate().ClearBlockIndexCandidates(); - auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options); - if (init_status != ChainstateLoadStatus::SUCCESS) { - return {init_status, init_error}; + auto result{CompleteChainstateInitialization(chainman, options)}; + if (!result || IsInterrupted(*result)) { + return result; } } else { - return {ChainstateLoadStatus::FAILURE_FATAL, _( + result.Update({util::Error{_( "UTXO snapshot failed to validate. " - "Restart to resume normal initial block download, or try loading a different snapshot.")}; + "Restart to resume normal initial block download, or try loading a different snapshot.")}, + ChainstateLoadError::FAILURE_FATAL}); + return result; } - return {ChainstateLoadStatus::SUCCESS, {}}; + return result; } -ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options) +util::Result VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options) { auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { return options.wipe_chainstate_db || chainstate->CoinsTip().GetBestBlock().IsNull(); @@ -234,36 +245,42 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C LOCK(cs_main); + util::Result result; for (Chainstate* chainstate : chainman.GetAll()) { if (!is_coinsview_empty(chainstate)) { const CBlockIndex* tip = chainstate->m_chain.Tip(); if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) { - return {ChainstateLoadStatus::FAILURE, _("The block database contains a block which appears to be from the future. " - "This may be due to your computer's date and time being set incorrectly. " - "Only rebuild the block database if you are sure that your computer's date and time are correct")}; + result.Update({util::Error{_("The block database contains a block which appears to be from the future. " + "This may be due to your computer's date and time being set incorrectly. " + "Only rebuild the block database if you are sure that your computer's date and time are correct")}, + ChainstateLoadError::FAILURE}); + return result; } - VerifyDBResult result = CVerifyDB(chainman.GetNotifications()).VerifyDB( + VerifyDBResult verify_result = CVerifyDB(chainman.GetNotifications()).VerifyDB( *chainstate, chainman.GetConsensus(), chainstate->CoinsDB(), options.check_level, options.check_blocks); - switch (result) { + switch (verify_result) { case VerifyDBResult::SUCCESS: case VerifyDBResult::SKIPPED_MISSING_BLOCKS: break; case VerifyDBResult::INTERRUPTED: - return {ChainstateLoadStatus::INTERRUPTED, _("Block verification was interrupted")}; + result.Update(Interrupted{}); + return result; case VerifyDBResult::CORRUPTED_BLOCK_DB: - return {ChainstateLoadStatus::FAILURE, _("Corrupted block database detected")}; + result.Update({util::Error{_("Corrupted block database detected")}, ChainstateLoadError::FAILURE}); + return result; case VerifyDBResult::SKIPPED_L3_CHECKS: if (options.require_full_verification) { - return {ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE, _("Insufficient dbcache for block verification")}; + result.Update({util::Error{_("Insufficient dbcache for block verification")}, ChainstateLoadError::FAILURE_INSUFFICIENT_DBCACHE}); + return result; } break; } // no default case, so the compiler can warn about missing cases } } - return {ChainstateLoadStatus::SUCCESS, {}}; + return result; } } // namespace node diff --git a/src/node/chainstate.h b/src/node/chainstate.h index 843e8e09a66..dbaa8c3ad03 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_NODE_CHAINSTATE_H #define BITCOIN_NODE_CHAINSTATE_H +#include +#include #include #include @@ -41,34 +43,16 @@ struct ChainstateLoadOptions { //! case, and treat other cases as errors. More complex applications may want to //! try reindexing in the generic failure case, and pass an interrupt callback //! and exit cleanly in the interrupted case. -enum class ChainstateLoadStatus { - SUCCESS, +enum class ChainstateLoadError { FAILURE, //!< Generic failure which reindexing may fix FAILURE_FATAL, //!< Fatal error which should not prompt to reindex FAILURE_INCOMPATIBLE_DB, FAILURE_INSUFFICIENT_DBCACHE, - INTERRUPTED, }; -//! Chainstate load status code and optional error string. -using ChainstateLoadResult = std::tuple; - -/** This sequence can have 4 types of outcomes: - * - * 1. Success - * 2. Shutdown requested - * - nothing failed but a shutdown was triggered in the middle of the - * sequence - * 3. Soft failure - * - a failure that might be recovered from with a reindex - * 4. Hard failure - * - a failure that definitively cannot be recovered from with a reindex - * - * LoadChainstate returns a (status code, error string) tuple. - */ -ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes, - const ChainstateLoadOptions& options); -ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); +util::Result LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes, + const ChainstateLoadOptions& options); +util::Result VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); } // namespace node #endif // BITCOIN_NODE_CHAINSTATE_H diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index bf26997c076..db20688ae17 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -290,11 +290,11 @@ void ChainTestingSetup::LoadVerifyActivateChainstate() options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS); options.check_level = m_args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL); options.require_full_verification = m_args.IsArgSet("-checkblocks") || m_args.IsArgSet("-checklevel"); - auto [status, error] = LoadChainstate(chainman, m_kernel_cache_sizes, options); - assert(status == node::ChainstateLoadStatus::SUCCESS); + auto load_result{LoadChainstate(chainman, m_kernel_cache_sizes, options)}; + Assert(load_result); - std::tie(status, error) = VerifyLoadedChainstate(chainman, options); - assert(status == node::ChainstateLoadStatus::SUCCESS); + auto verify_result{VerifyLoadedChainstate(chainman, options)}; + Assert(verify_result); BlockValidationState state; if (!chainman.ActiveChainstate().ActivateBestChain(state)) { From 4599760cc655faf04b4e8abdc1aeedc853a65af2 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Mar 2024 14:17:01 -0400 Subject: [PATCH 5/8] refactor: Add util::Result multiple error and warning messages Add util::Result support for returning warning messages and multiple errors, not just a single error string. This provides a way for functions to report errors and warnings in a standard way, and simplifies interfaces. The functionality is unit tested here, and put to use in followup PR https://github.com/bitcoin/bitcoin/pull/25722 --- src/kernel/CMakeLists.txt | 1 + src/test/result_tests.cpp | 74 +++++++++++++++++- src/util/CMakeLists.txt | 1 + src/util/result.cpp | 33 ++++++++ src/util/result.h | 157 +++++++++++++++++++++++++++++++++----- 5 files changed, 244 insertions(+), 22 deletions(-) create mode 100644 src/util/result.cpp diff --git a/src/kernel/CMakeLists.txt b/src/kernel/CMakeLists.txt index b9f37969d37..4d1d689de21 100644 --- a/src/kernel/CMakeLists.txt +++ b/src/kernel/CMakeLists.txt @@ -68,6 +68,7 @@ add_library(bitcoinkernel ../util/hasher.cpp ../util/moneystr.cpp ../util/rbf.cpp + ../util/result.cpp ../util/serfloat.cpp ../util/signalinterrupt.cpp ../util/strencodings.cpp diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index c58ae82442a..4210cd0dfaa 100644 --- a/src/test/result_tests.cpp +++ b/src/test/result_tests.cpp @@ -2,10 +2,17 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include +#include +#include #include +#include +#include +#include +#include inline bool operator==(const bilingual_str& a, const bilingual_str& b) { @@ -90,15 +97,60 @@ enum FnError { ERR1, ERR2 }; util::Result IntFailFn(int i, bool success) { - if (success) return i; + if (success) return {util::Warning{Untranslated(strprintf("int %i warn.", i))}, i}; return {util::Error{Untranslated(strprintf("int %i error.", i))}, i % 2 ? ERR1 : ERR2}; } +util::Result StrFailFn(int i, bool success) +{ + util::Result result; + if (auto int_result{IntFailFn(i, success) >> result}) { + result.Update(strprintf("%i", *int_result)); + } else { + result.Update({util::Error{Untranslated("str error")}, int_result.GetFailure()}); + } + return result; +} + util::Result EnumFailFn(FnError ret) { return {util::Error{Untranslated("enum fail.")}, ret}; } +util::Result WarnFn() +{ + return {util::Warning{Untranslated("warn.")}}; +} + +util::Result MultiWarnFn(int ret) +{ + util::Result result; + for (int i = 0; i < ret; ++i) { + result.AddWarning(Untranslated(strprintf("warn %i.", i))); + } + result.Update(ret); + return result; +} + +util::Result ChainedFailFn(FnError arg, int ret) +{ + util::Result result{util::Error{Untranslated("chained fail.")}, ret}; + EnumFailFn(arg) >> result; + WarnFn() >> result; + return result; +} + +util::Result AccumulateFn(bool success) +{ + util::Result result; + util::Result x = MultiWarnFn(1) >> result; + BOOST_REQUIRE(x); + util::Result y = MultiWarnFn(2) >> result; + BOOST_REQUIRE(y); + result.Update(IntFailFn(*x + *y, success)); + return result; +} + util::Result TruthyFalsyFn(int i, bool success) { if (success) return i; @@ -142,7 +194,13 @@ BOOST_AUTO_TEST_CASE(check_returned) ExpectFail(NoCopyNoMoveFn(5, false), Untranslated("nocopynomove 5 error."), 5); ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S")); ExpectResult(StrFn(Untranslated("S"), false), false, Untranslated("str S error.")); + ExpectSuccess(StrFailFn(1, true), Untranslated("int 1 warn."), "1"); + ExpectFail(StrFailFn(2, false), Untranslated("int 2 error. str error"), ERR2); ExpectFail(EnumFailFn(ERR2), Untranslated("enum fail."), ERR2); + ExpectFail(ChainedFailFn(ERR1, 5), Untranslated("chained fail. enum fail. warn."), 5); + ExpectSuccess(MultiWarnFn(3), Untranslated("warn 0. warn 1. warn 2."), 3); + ExpectSuccess(AccumulateFn(true), Untranslated("warn 0. warn 0. warn 1. int 3 warn."), 3); + ExpectFail(AccumulateFn(false), Untranslated("int 3 error. warn 0. warn 0. warn 1."), ERR1); ExpectSuccess(TruthyFalsyFn(0, true), {}, 0); ExpectFail(TruthyFalsyFn(0, false), Untranslated("failure value 0."), 0); ExpectSuccess(TruthyFalsyFn(1, true), {}, 1); @@ -158,7 +216,7 @@ BOOST_AUTO_TEST_CASE(check_update) result.Update({util::Error{Untranslated("error")}, ERR1}); ExpectFail(result, Untranslated("error"), ERR1); result.Update(2); - ExpectSuccess(result, Untranslated(""), 2); + ExpectSuccess(result, Untranslated("error"), 2); // Test the same thing but with non-copyable success and failure types. util::Result result2{0}; @@ -166,7 +224,7 @@ BOOST_AUTO_TEST_CASE(check_update) result2.Update({util::Error{Untranslated("error")}, 3}); ExpectFail(result2, Untranslated("error"), 3); result2.Update(4); - ExpectSuccess(result2, Untranslated(""), 4); + ExpectSuccess(result2, Untranslated("error"), 4); } BOOST_AUTO_TEST_CASE(check_dereference_operators) @@ -190,6 +248,16 @@ BOOST_AUTO_TEST_CASE(check_value_or) BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B")); } +BOOST_AUTO_TEST_CASE(check_message_accessors) +{ + util::Result result{util::Error{Untranslated("Error.")}, util::Warning{Untranslated("Warning.")}}; + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors.size(), 1); + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors[0], Untranslated("Error.")); + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings.size(), 1); + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings[0], Untranslated("Warning.")); + BOOST_CHECK_EQUAL(util::ErrorString(result), Untranslated("Error. Warning.")); +} + struct Derived : NoCopyNoMove { using NoCopyNoMove::NoCopyNoMove; }; diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 4999dbf13f0..dc5121f77cf 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -17,6 +17,7 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL moneystr.cpp rbf.cpp readwritefile.cpp + result.cpp serfloat.cpp signalinterrupt.cpp sock.cpp diff --git a/src/util/result.cpp b/src/util/result.cpp new file mode 100644 index 00000000000..ea79375aa3a --- /dev/null +++ b/src/util/result.cpp @@ -0,0 +1,33 @@ +// Copyright (c) 2024 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include +#include +#include + +namespace util { +namespace detail { +bilingual_str JoinMessages(const Messages& messages) +{ + bilingual_str result; + for (const auto& messages : {messages.errors, messages.warnings}) { + for (const auto& message : messages) { + if (!result.empty()) result += Untranslated(" "); + result += message; + } + } + return result; +} +} // namespace detail + +void ResultTraits::Update(Messages& dst, Messages& src) { + dst.errors.insert(dst.errors.end(), std::make_move_iterator(src.errors.begin()), std::make_move_iterator(src.errors.end())); + dst.warnings.insert(dst.warnings.end(), std::make_move_iterator(src.warnings.begin()), std::make_move_iterator(src.warnings.end())); + src.errors.clear(); + src.warnings.clear(); +} +} // namespace util diff --git a/src/util/result.h b/src/util/result.h index 8706220aa90..f4ec3be623e 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -17,9 +17,15 @@ #include namespace util { +//! Default MessagesType, simple list of errors and warnings. +struct Messages { + std::vector errors{}; + std::vector warnings{}; +}; + //! The Result class provides //! an efficient way for functions to return structured result information, as -//! well as descriptive error messages. +//! well as descriptive error and warning messages. //! //! Logically, a result object is equivalent to: //! @@ -58,15 +64,67 @@ namespace util { //! return function results. //! //! Usage examples can be found in \example ../test/result_tests.cpp. -template +template class Result; //! Wrapper object to pass an error string to the Result constructor. struct Error { bilingual_str message; }; +//! Wrapper object to pass a warning string to the Result constructor. +struct Warning { + bilingual_str message; +}; + +//! Type trait that can be specialized to control the way SuccessType / +//! FailureType / MessagesType values are combined. Default behavior +//! is for new values to overwrite existing ones, but this can be specialized +//! for custom behavior when Result::Update() method is called or << operator is +//! used. For example, this is specialized for Messages struct below to append +//! error and warning messages instead of overwriting them. It can also be used, +//! for example, to merge FailureType values when a function can return multiple +//! failures. +template +struct ResultTraits { + template + static void Update(O& dst, T& src) + { + dst = std::move(src); + } +}; + +//! Type trait that can be specialized to let the Result class work with custom +//! MessagesType types holding error and warning messages. +template +struct MessagesTraits; + +//! ResultTraits specialization for Messages struct. +template<> +struct ResultTraits { + static void Update(Messages& dst, Messages& src); +}; + +//! MessagesTraits specialization for Messages struct. +template<> +struct MessagesTraits { + static void AddError(Messages& messages, bilingual_str error) + { + messages.errors.emplace_back(std::move(error)); + } + static void AddWarning(Messages& messages, bilingual_str warning) + { + messages.warnings.emplace_back(std::move(warning)); + } + static bool HasMessages(const Messages& messages) + { + return messages.errors.size() || messages.warnings.size(); + } +}; namespace detail { +//! Helper function to join messages in space separated string. +bilingual_str JoinMessages(const Messages& messages); + //! Substitute for std::monostate that doesn't depend on std::variant. struct Monostate{}; @@ -157,9 +215,9 @@ public: static constexpr bool is_result{true}; //! Construct a Result object setting a success or failure value and - //! optional error messages. Initial util::Error arguments are processed - //! first to add error messages. - //! Then, any remaining arguments are passed to the SuccessType + //! optional warning and error messages. Initial util::Error and + //! util::Warning arguments are processed first to add warning and error + //! messages. Then, any remaining arguments are passed to the SuccessType //! constructor and used to construct a success value in the success case. //! In the failure case, if any util::Error arguments were passed, any //! remaining arguments are passed to the FailureType constructor and used @@ -171,7 +229,7 @@ public: } //! Move-construct a Result object from another Result object, moving the - //! success or failure value and any error message. + //! success or failure value and any error or warning messages. template requires (std::decay_t::is_result) Result(O&& other) @@ -179,7 +237,10 @@ public: Move(*this, other); } - //! Update this result by moving from another result object. + //! Update this result by moving from another result object. Existing + //! success, failure, and messages values are updated (using + //! ResultTraits::Update specializations), so errors and warning messages + //! get appended instead of overwriting existing ones. Result& Update(Result&& other) LIFETIMEBOUND { Move(*this, other); @@ -187,11 +248,18 @@ public: } //! 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. + //! accidentally clearing error and warning messages when combining results. Result& operator=(Result&&) = delete; + void AddError(bilingual_str error) + { + if (!error.empty()) MessagesTraits::AddError(this->EnsureFailData().messages, std::move(error)); + } + void AddWarning(bilingual_str warning) + { + if (!warning.empty()) MessagesTraits::AddWarning(this->EnsureFailData().messages, std::move(warning)); + } + protected: template friend class Result; @@ -216,12 +284,22 @@ protected: template static void Construct(Result& result, util::Error error, Args&&... args) { - result.EnsureFailData().messages = std::move(error.message); + result.AddError(std::move(error.message)); Construct(result, std::forward(args)...); } + //! Construct() overload peeling off a util::Warning constructor argument. + template + static void Construct(Result& result, util::Warning warning, Args&&... args) + { + result.AddWarning(std::move(warning.message)); + Construct(result, std::forward(args)...); + } + //! Move success, failure, and messages from source Result object to - //! destination object. The source and + //! destination object. Existing values are updated (using + //! ResultTraits::Update specializations), so destination errors and warning + //! messages get appended to instead of overwritten. The source and //! destination results are not required to have the same types, and //! assigning void source types to non-void destinations type is allowed, //! since no source information is lost. But assigning non-void source types @@ -230,16 +308,27 @@ protected: template static void Move(DstResult& dst, SrcResult& src) { - // Move messages values first, then move success or failure value below. - if (src.GetMessages() && !src.GetMessages()->empty()) { - dst.EnsureMessages() = std::move(*src.GetMessages()); - } + // Use operator>> to move messages value first, then move + // success or failure value below. + src >> dst; // If DstConstructed is true, it means dst has either a success value or - // a failure value set, which needs to be destroyed and replaced. If + // a failure value set, which needs to be updated or replaced. If // DstConstructed is false, then dst is being constructed now and has no // values set. if constexpr (DstConstructed) { - if (dst) { + if (dst && src) { + // dst and src both hold success values, so update dst from src and return + if constexpr (!std::is_same_v) { + ResultTraits::Update(*dst, *src); + } + return; + } else if (!dst && !src) { + // dst and src both hold failure values, so update dst from src and return + if constexpr (!std::is_same_v) { + ResultTraits::Update(dst.GetFailure(), src.GetFailure()); + } + return; + } else if (dst) { // dst has a success value, so destroy it before replacing it with src failure value if constexpr (!std::is_same_v) { using DstSuccess = typename DstResult::SuccessType; @@ -277,10 +366,40 @@ protected: } }; +//! Move information from a source Result object to a destination object. It +//! only moves MessagesType values without affecting SuccessType or +//! FailureType values of either Result object. +//! +//! This is useful for combining error and warning messages from multiple result +//! objects into a single object, e.g.: +//! +//! util::Result result; +//! auto r1 = DoSomething() >> result; +//! auto r2 = DoSomethingElse() >> result; +//! ... +//! return result; +//! +template +requires (std::decay_t::is_result) +decltype(auto) operator>>(SrcResult&& src LIFETIMEBOUND, DstResult&& dst) +{ + using SrcType = std::decay_t; + if (src.GetMessages() && MessagesTraits::HasMessages(*src.GetMessages())) { + ResultTraits::Update(dst.EnsureMessages(), *src.GetMessages()); + } + return static_cast(src); +} + +//! Join error and warning messages in a space separated string. This is +//! intended for simple applications where there's probably only one error or +//! warning message to report, but multiple messages should not be lost if they +//! are present. More complicated applications should use Result::GetMessages() +//! method directly. template bilingual_str ErrorString(const Result& result) { - return !result && result.GetMessages() ? *result.GetMessages() : bilingual_str{}; + const auto* messages{result.GetMessages()}; + return messages ? detail::JoinMessages(*messages) : bilingual_str{}; } } // namespace util From 69b14c8122d69ae25b42a46e70f80aad1f783f0e Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 20 Jul 2023 13:52:40 -0400 Subject: [PATCH 6/8] test: add static test for util::Result memory usage Suggested by Martin Leitner-Ankerl https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174298529 Co-authored-by: Martin Leitner-Ankerl --- src/test/result_tests.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 4210cd0dfaa..595de9028bc 100644 --- a/src/test/result_tests.cpp +++ b/src/test/result_tests.cpp @@ -182,6 +182,12 @@ void ExpectFail(const util::Result& result, bilingual_str str, Args&&... a BOOST_CHECK_EQUAL(result.GetFailure(), expect_failure); } +BOOST_AUTO_TEST_CASE(check_sizes) +{ + static_assert(sizeof(util::Result) == sizeof(void*)*2); + static_assert(sizeof(util::Result) == sizeof(void*)); +} + BOOST_AUTO_TEST_CASE(check_returned) { ExpectResult(VoidSuccessFn(), true, {}); From 86607f813ddb7318fa58e8ce2d277f8421e7ff9f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 6 Sep 2022 11:10:25 -0400 Subject: [PATCH 7/8] Add util::ResultPtr class The util::ResultPtr class is a wrapper for util::Result providing syntax sugar to make it less awkward to use with returned pointers. Instead of needing to be deferenced twice with **result or (*result)->member, it only needs to be dereferenced once with *result or result->member. Also when it is used in a boolean context, instead of evaluating to true when there is no error and the pointer is null, it evaluates to false so it is straightforward to determine whether the result points to something. This PR is only partial a solution to #26004 because while it makes it easier to check for null pointer values, it does not make it impossible to return a null pointer value inside a successful result. It would be possible to enforce that successful results always contain non-null pointers by using a not_null type (https://github.com/bitcoin/bitcoin/issues/24423) in combination with ResultPtr, though. --- src/test/result_tests.cpp | 35 +++++++++++++++++++++++++++++++++++ src/util/result.h | 27 +++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 595de9028bc..327625f90d3 100644 --- a/src/test/result_tests.cpp +++ b/src/test/result_tests.cpp @@ -275,7 +275,42 @@ util::Result> DerivedToBaseFn(util::Result(5)))), 5); + + // Check conversions work between util::Result and util::ResultPtr + util::ResultPtr> derived{std::make_unique(5)}; + util::ResultPtr> base{DerivedToBaseFn(std::move(derived))}; + BOOST_CHECK_EQUAL(*Assert(base), 5); +} + +//! For testing ResultPtr, return pointer to a pair of ints, or return pointer to null, or return an error message. +util::ResultPtr>> PtrFn(std::optional> i, bool success) +{ + if (success) return i ? std::make_unique>(*i) : nullptr; + return util::Error{Untranslated(strprintf("PtrFn(%s) error.", i ? strprintf("%i, %i", i->first, i->second) : "nullopt"))}; +} + +BOOST_AUTO_TEST_CASE(check_ptr) +{ + auto result_pair = PtrFn(std::pair{1, 2}, true); + ExpectResult(result_pair, true, {}); + BOOST_CHECK(result_pair); + BOOST_CHECK_EQUAL(result_pair->first, 1); + BOOST_CHECK_EQUAL(result_pair->second, 2); + BOOST_CHECK(*result_pair == std::pair(1,2)); + + auto result_null = PtrFn(std::nullopt, true); + ExpectResult(result_null, true, {}); + BOOST_CHECK(!result_null); + + auto result_error_pair = PtrFn(std::pair{1, 2}, false); + ExpectResult(result_error_pair, false, Untranslated("PtrFn(1, 2) error.")); + BOOST_CHECK(!result_error_pair); + + auto result_error_null = PtrFn(std::nullopt, false); + ExpectResult(result_error_null, false, Untranslated("PtrFn(nullopt) error.")); + BOOST_CHECK(!result_error_null); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/result.h b/src/util/result.h index f4ec3be623e..dc7ee05d686 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -390,6 +390,33 @@ decltype(auto) operator>>(SrcResult&& src LIFETIMEBOUND, DstResult&& dst) return static_cast(src); } +//! Wrapper around util::Result that is less awkward to use with pointer types. +//! +//! It overloads pointer and bool operators so it isn't necessary to dereference +//! the result object twice to access the result value, so it possible to call +//! methods with `result->Method()` rather than `(*result)->Method()` and check +//! whether the pointer is null with `if (result)` rather than `if (result && +//! *result)`. +//! +//! The `ResultPtr` class just adds syntax sugar to `Result` class. It is still +//! possible to access the result pointer directly using `ResultPtr` `value()` +//! and `has_value()` methods. +template +class ResultPtr : public Result +{ +public: + // Inherit Result constructors (excluding copy and move constructors). + using Result::Result; + + // Inherit Result copy and move constructors. + template + ResultPtr(O&& other) : Result{std::forward(other)} {} + + explicit operator bool() const noexcept { return this->has_value() && this->value(); } + auto* operator->() const { assert(this->value()); return &*this->value(); } + auto& operator*() const { assert(this->value()); return *this->value(); } +}; + //! Join error and warning messages in a space separated string. This is //! intended for simple applications where there's probably only one error or //! warning message to report, but multiple messages should not be lost if they From 06512c44b6264f4ece4108dfcb572c0a90ec55b9 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 6 Sep 2022 11:47:47 -0400 Subject: [PATCH 8/8] Use ResultPtr instead of Result --- src/addrdb.cpp | 2 +- src/addrdb.h | 2 +- src/init.cpp | 2 +- src/interfaces/wallet.h | 6 +++--- src/qt/walletcontroller.cpp | 6 +++--- src/wallet/interfaces.cpp | 30 +++++++++--------------------- 6 files changed, 18 insertions(+), 30 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 889f7b3859b..11a6ddd1191 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -188,7 +188,7 @@ void ReadFromStream(AddrMan& addr, DataStream& ssPeers) DeserializeDB(ssPeers, addr, false); } -util::Result> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args) +util::ResultPtr> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args) { auto check_addrman = std::clamp(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); bool deterministic = HasTestOption(args, "addrman"); // use a deterministic addrman only for tests diff --git a/src/addrdb.h b/src/addrdb.h index cc3014dce29..c6c28257c5c 100644 --- a/src/addrdb.h +++ b/src/addrdb.h @@ -49,7 +49,7 @@ public: }; /** Returns an error string on failure */ -util::Result> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args); +util::ResultPtr> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args); /** * Dump the anchor IP address database (anchors.dat) diff --git a/src/init.cpp b/src/init.cpp index 95009c47da1..c6048b3cae1 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1482,7 +1482,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) uiInterface.InitMessage(_("Loading P2P addresses…")); auto addrman{LoadAddrman(*node.netgroupman, args)}; if (!addrman) return InitError(util::ErrorString(addrman)); - node.addrman = std::move(*addrman); + node.addrman = std::move(addrman.value()); } FastRandomContext rng; diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index df1ced48a71..a195f8062eb 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -328,16 +328,16 @@ class WalletLoader : public ChainClient { public: //! Create new wallet. - virtual util::Result> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector& warnings) = 0; + virtual util::ResultPtr> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector& warnings) = 0; //! Load existing wallet. - virtual util::Result> loadWallet(const std::string& name, std::vector& warnings) = 0; + virtual util::ResultPtr> loadWallet(const std::string& name, std::vector& warnings) = 0; //! Return default wallet directory. virtual std::string getWalletDir() = 0; //! Restore backup wallet - virtual util::Result> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) = 0; + virtual util::ResultPtr> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) = 0; //! Migrate a wallet virtual util::Result migrateWallet(const std::string& name, const SecureString& passphrase) = 0; diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index dd093e984a3..79face5fc35 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -270,7 +270,7 @@ void CreateWalletActivity::createWallet() auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)}; if (wallet) { - m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet)); + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value())); } else { m_error_message = util::ErrorString(wallet); } @@ -359,7 +359,7 @@ void OpenWalletActivity::open(const std::string& path) auto wallet{node().walletLoader().loadWallet(path, m_warning_message)}; if (wallet) { - m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet)); + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value())); } else { m_error_message = util::ErrorString(wallet); } @@ -412,7 +412,7 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)}; if (wallet) { - m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet)); + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value())); } else { m_error_message = util::ErrorString(wallet); } diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 21e8a0b3bd2..808c8316285 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -599,7 +599,7 @@ public: void schedulerMockForward(std::chrono::seconds delta) override { Assert(m_context.scheduler)->MockForward(delta); } //! WalletLoader methods - util::Result> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector& warnings) override + util::ResultPtr> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector& warnings) override { DatabaseOptions options; DatabaseStatus status; @@ -608,37 +608,25 @@ public: options.create_flags = wallet_creation_flags; options.create_passphrase = passphrase; bilingual_str error; - std::unique_ptr wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; - if (wallet) { - return wallet; - } else { - return util::Error{error}; - } + util::ResultPtr> wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; + return wallet ? std::move(wallet) : util::Error{error}; } - util::Result> loadWallet(const std::string& name, std::vector& warnings) override + util::ResultPtr> loadWallet(const std::string& name, std::vector& warnings) override { DatabaseOptions options; DatabaseStatus status; ReadDatabaseArgs(*m_context.args, options); options.require_existing = true; bilingual_str error; - std::unique_ptr wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; - if (wallet) { - return wallet; - } else { - return util::Error{error}; - } + util::ResultPtr> wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; + return wallet ? std::move(wallet) : util::Error{error}; } - util::Result> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) override + util::ResultPtr> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) override { DatabaseStatus status; bilingual_str error; - std::unique_ptr wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))}; - if (wallet) { - return wallet; - } else { - return util::Error{error}; - } + util::ResultPtr> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))}; + return wallet ? std::move(wallet) : util::Error{error}; } util::Result migrateWallet(const std::string& name, const SecureString& passphrase) override {