From 4d90ce54e7d58bdeb5fe7e30de414b4d81c7957f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 18 Jul 2022 15:48:04 -0400 Subject: [PATCH 01/23] 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 02/23] 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 03/23] 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 04/23] 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 05/23] 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 06/23] 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 07/23] 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 08/23] 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 { From 8651006afbf9d79d29441d8c0b08ab2782217050 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 22 Feb 2024 12:10:37 -0500 Subject: [PATCH 09/23] Add temporary ResultExtract helper for porting to util::Result --- src/util/result.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/util/result.h b/src/util/result.h index dc7ee05d686..c1d9c3e4525 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -430,4 +430,35 @@ bilingual_str ErrorString(const Result& result) } } // namespace util +//! Temporary helper used to decompose util::Result into success / failure / +//! error / warning values. This is used in upcoming commits to incrementally +//! port code not using util::Result to start using it, and it is removed after +//! the last commit when everything is using util::Result. +//! +//! First ResultExtract argument is an input result object. Remaining arguments +//! are output arguments returning the result failure value and error and +//! warning messages, if any. Return value is the result success value, if the +//! success type is non-void, or a boolean indicating success / failure if the +//! success type is void. +template +auto ResultExtract(Result&& result, FailPtr failure=nullptr, bilingual_str* error = nullptr, std::vector* warnings = nullptr) +{ + if constexpr (!std::is_same_v) { + if (failure && !result) *failure = result.GetFailure(); + if (failure && result) *failure = typename Result::FailureType{}; + } + if (error && result.GetMessages() && !result.GetMessages()->errors.empty()) { + *error = util::ErrorString(result); + } + if (warnings && result.GetMessages()) { + const auto& w = result.GetMessages()->warnings; + warnings->insert(warnings->end(), w.begin(), w.end()); + } + if constexpr (std::is_same_v) { + return bool{result}; + } else { + return result ? std::move(result.value()) : typename Result::SuccessType{}; + } +} + #endif // BITCOIN_UTIL_RESULT_H From 1789bb0f8e4a96db301c81c909a867771b840613 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 10/23] refactor: Use util::Result class in wallet/sqlite --- src/wallet/sqlite.cpp | 70 +++++++++++++++++++----------------- src/wallet/sqlite.h | 5 +-- src/wallet/test/db_tests.cpp | 10 ++---- src/wallet/walletdb.cpp | 2 +- 4 files changed, 44 insertions(+), 43 deletions(-) diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index a8c9f8a8ab6..d4977d9bca0 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -78,21 +78,19 @@ static bool BindBlobToStatement(sqlite3_stmt* stmt, return true; } -static std::optional ReadPragmaInteger(sqlite3* db, const std::string& key, const std::string& description, bilingual_str& error) +static util::Result ReadPragmaInteger(sqlite3* db, const std::string& key, const std::string& description) { std::string stmt_text = strprintf("PRAGMA %s", key); sqlite3_stmt* pragma_read_stmt{nullptr}; int ret = sqlite3_prepare_v2(db, stmt_text.c_str(), -1, &pragma_read_stmt, nullptr); if (ret != SQLITE_OK) { sqlite3_finalize(pragma_read_stmt); - error = Untranslated(strprintf("SQLiteDatabase: Failed to prepare the statement to fetch %s: %s", description, sqlite3_errstr(ret))); - return std::nullopt; + return {util::Error{Untranslated(strprintf("SQLiteDatabase: Failed to prepare the statement to fetch %s: %s", description, sqlite3_errstr(ret)))}}; } ret = sqlite3_step(pragma_read_stmt); if (ret != SQLITE_ROW) { sqlite3_finalize(pragma_read_stmt); - error = Untranslated(strprintf("SQLiteDatabase: Failed to fetch %s: %s", description, sqlite3_errstr(ret))); - return std::nullopt; + return {util::Error{Untranslated(strprintf("SQLiteDatabase: Failed to fetch %s: %s", description, sqlite3_errstr(ret)))}}; } int result = sqlite3_column_int(pragma_read_stmt, 0); sqlite3_finalize(pragma_read_stmt); @@ -187,35 +185,42 @@ void SQLiteDatabase::Cleanup() noexcept } } -bool SQLiteDatabase::Verify(bilingual_str& error) +util::Result SQLiteDatabase::Verify() { assert(m_db); + util::Result result; // Check the application ID matches our network magic - auto read_result = ReadPragmaInteger(m_db, "application_id", "the application id", error); - if (!read_result.has_value()) return false; - uint32_t app_id = static_cast(read_result.value()); + auto read_app_id{ReadPragmaInteger(m_db, "application_id", "the application id") >> result}; + if (!read_app_id) { + result.Update(util::Error{}); + return result; + } + uint32_t app_id = static_cast(*read_app_id); uint32_t net_magic = ReadBE32(Params().MessageStart().data()); if (app_id != net_magic) { - error = strprintf(_("SQLiteDatabase: Unexpected application id. Expected %u, got %u"), net_magic, app_id); - return false; + result.Update(util::Error{strprintf(_("SQLiteDatabase: Unexpected application id. Expected %u, got %u"), net_magic, app_id)}); + return result; } // Check our schema version - read_result = ReadPragmaInteger(m_db, "user_version", "sqlite wallet schema version", error); - if (!read_result.has_value()) return false; - int32_t user_ver = read_result.value(); + auto read_user_ver{ReadPragmaInteger(m_db, "user_version", "sqlite wallet schema version") >> result}; + if (!read_user_ver) { + result.Update(util::Error{}); + return result; + } + int32_t user_ver = *read_user_ver; if (user_ver != WALLET_SCHEMA_VERSION) { - error = strprintf(_("SQLiteDatabase: Unknown sqlite wallet schema version %d. Only version %d is supported"), user_ver, WALLET_SCHEMA_VERSION); - return false; + result.Update(util::Error{strprintf(_("SQLiteDatabase: Unknown sqlite wallet schema version %d. Only version %d is supported"), user_ver, WALLET_SCHEMA_VERSION)}); + return result; } sqlite3_stmt* stmt{nullptr}; int ret = sqlite3_prepare_v2(m_db, "PRAGMA integrity_check", -1, &stmt, nullptr); if (ret != SQLITE_OK) { sqlite3_finalize(stmt); - error = strprintf(_("SQLiteDatabase: Failed to prepare statement to verify database: %s"), sqlite3_errstr(ret)); - return false; + result.Update(util::Error{strprintf(_("SQLiteDatabase: Failed to prepare statement to verify database: %s"), sqlite3_errstr(ret))}); + return result; } while (true) { ret = sqlite3_step(stmt); @@ -223,25 +228,25 @@ bool SQLiteDatabase::Verify(bilingual_str& error) break; } if (ret != SQLITE_ROW) { - error = strprintf(_("SQLiteDatabase: Failed to execute statement to verify database: %s"), sqlite3_errstr(ret)); + result.Update(util::Error{strprintf(_("SQLiteDatabase: Failed to execute statement to verify database: %s"), sqlite3_errstr(ret))}); break; } const char* msg = (const char*)sqlite3_column_text(stmt, 0); if (!msg) { - error = strprintf(_("SQLiteDatabase: Failed to read database verification error: %s"), sqlite3_errstr(ret)); + result.Update(util::Error{strprintf(_("SQLiteDatabase: Failed to read database verification error: %s"), sqlite3_errstr(ret))}); break; } std::string str_msg(msg); if (str_msg == "ok") { continue; } - if (error.empty()) { - error = _("Failed to verify database") + Untranslated("\n"); + if (result) { + result.Update(util::Error{_("Failed to verify database")}); } - error += Untranslated(strprintf("%s\n", str_msg)); + result.AddError(Untranslated(str_msg)); } sqlite3_finalize(stmt); - return error.empty(); + return result; } void SQLiteDatabase::Open() @@ -691,22 +696,21 @@ bool SQLiteBatch::TxnAbort() return res == SQLITE_OK; } -std::unique_ptr MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +util::ResultPtr, DatabaseStatus> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options) { + util::ResultPtr, DatabaseStatus> result; try { fs::path data_file = SQLiteDataFile(path); auto db = std::make_unique(data_file.parent_path(), data_file, options); - if (options.verify && !db->Verify(error)) { - status = DatabaseStatus::FAILED_VERIFY; - return nullptr; + if (options.verify && !(db->Verify() >> result)) { + result.Update({util::Error{}, DatabaseStatus::FAILED_VERIFY}); + } else { + result.Update(std::move(db)); } - status = DatabaseStatus::SUCCESS; - return db; } catch (const std::runtime_error& e) { - status = DatabaseStatus::FAILED_LOAD; - error = Untranslated(e.what()); - return nullptr; + result.Update({util::Error{Untranslated(e.what())}, DatabaseStatus::FAILED_LOAD}); } + return result; } std::string SQLiteDatabaseVersion() diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index 78a3accf890..219bd00e6d0 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -6,6 +6,7 @@ #define BITCOIN_WALLET_SQLITE_H #include +#include #include struct bilingual_str; @@ -132,7 +133,7 @@ public: // This ensures that only one batch is modifying the database at a time. CSemaphore m_write_semaphore; - bool Verify(bilingual_str& error); + util::Result Verify(); /** Open the database if it is not already opened */ void Open() override; @@ -178,7 +179,7 @@ public: bool m_use_unsafe_sync; }; -std::unique_ptr MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseStatus> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options); std::string SQLiteDatabaseVersion(); } // namespace wallet diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index 56a39c8d5f3..fa94bb9d15a 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -139,7 +139,7 @@ static std::vector> TestDatabases(const fs::path dbs.emplace_back(std::make_unique(BDBDataFile(path_root / "bdb"), /*open=*/false)); #endif #ifdef USE_SQLITE - dbs.emplace_back(MakeSQLiteDatabase(path_root / "sqlite", options, status, error)); + dbs.emplace_back(std::move(MakeSQLiteDatabase(path_root / "sqlite", options).value())); #endif dbs.emplace_back(CreateMockableWalletDatabase()); return dbs; @@ -316,9 +316,7 @@ BOOST_AUTO_TEST_CASE(txn_close_failure_dangling_txn) // Verifies that there is no active dangling, to-be-reversed db txn // after the batch object that initiated it is destroyed. DatabaseOptions options; - DatabaseStatus status; - bilingual_str error; - std::unique_ptr database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error); + auto database = MakeSQLiteDatabase(m_path_root / "sqlite", options); std::string key = "key"; std::string value = "value"; @@ -351,9 +349,7 @@ BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere) std::string value2 = "value_2"; DatabaseOptions options; - DatabaseStatus status; - bilingual_str error; - const auto& database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error); + auto database = MakeSQLiteDatabase(m_path_root / "sqlite", options); std::unique_ptr handler = Assert(database)->MakeBatch(); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c939ebb1fd4..625d75b9608 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1475,7 +1475,7 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas if (format == DatabaseFormat::SQLITE) { #ifdef USE_SQLITE if constexpr (true) { - return MakeSQLiteDatabase(path, options, status, error); + return ResultExtract(MakeSQLiteDatabase(path, options), &status, &error); } else #endif { From b27c25199ebbd2b8ed42faee1547770179a93ac8 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 11/23] refactor: Use util::Result class in wallet/bdb --- src/wallet/bdb.cpp | 56 +++++++++++----------- src/wallet/bdb.h | 7 +-- src/wallet/salvage.cpp | 2 +- src/wallet/test/db_tests.cpp | 4 +- src/wallet/test/fuzz/wallet_bdb_parser.cpp | 2 +- src/wallet/walletdb.cpp | 2 +- 6 files changed, 36 insertions(+), 37 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 79851dff33f..64b7f71bf18 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -143,18 +143,17 @@ BerkeleyEnvironment::~BerkeleyEnvironment() Close(); } -bool BerkeleyEnvironment::Open(bilingual_str& err) +util::Result BerkeleyEnvironment::Open() { if (fDbEnvInit) { - return true; + return {}; } fs::path pathIn = fs::PathFromString(strPath); TryCreateDirectories(pathIn); if (util::LockDirectory(pathIn, ".walletlock") != util::LockResult::Success) { LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance may be using it.\n", strPath); - err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory()))); - return false; + return {util::Error{strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory())))}}; } fs::path pathLogDir = pathIn / "database"; @@ -194,16 +193,16 @@ bool BerkeleyEnvironment::Open(bilingual_str& err) LogPrintf("BerkeleyEnvironment::Open: Error %d closing failed database environment: %s\n", ret2, DbEnv::strerror(ret2)); } Reset(); - err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory()))); + auto err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory()))); if (ret == DB_RUNRECOVERY) { err += Untranslated(" ") + _("This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newer version of Berkeley DB. If so, please use the software that last loaded this wallet"); } - return false; + return {util::Error{std::move(err)}}; } fDbEnvInit = true; fMockDb = false; - return true; + return {}; } //! Construct an in-memory mock Berkeley environment for testing @@ -312,7 +311,7 @@ BerkeleyDatabase::BerkeleyDatabase(std::shared_ptr env, fs: assert(inserted.second); } -bool BerkeleyDatabase::Verify(bilingual_str& errorStr) +util::Result BerkeleyDatabase::Verify() { fs::path walletDir = env->Directory(); fs::path file_path = walletDir / m_filename; @@ -320,9 +319,8 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr) LogPrintf("Using BerkeleyDB version %s\n", BerkeleyDatabaseVersion()); LogPrintf("Using wallet %s\n", fs::PathToString(file_path)); - if (!env->Open(errorStr)) { - return false; - } + util::Result opened = env->Open(); + if (!opened) return opened; if (fs::exists(file_path)) { @@ -332,12 +330,11 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr) const std::string strFile = fs::PathToString(m_filename); int result = db.verify(strFile.c_str(), nullptr, nullptr, 0); if (result != 0) { - errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), fs::quoted(fs::PathToString(file_path))); - return false; + return {util::Error{strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), fs::quoted(fs::PathToString(file_path)))}}; } } // also return true if files does not exists - return true; + return {}; } void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile) @@ -377,9 +374,10 @@ void BerkeleyDatabase::Open() { LOCK(cs_db); - bilingual_str open_err; - if (!env->Open(open_err)) - throw std::runtime_error("BerkeleyDatabase: Failed to open database environment."); + auto opened = env->Open(); + if (!opened) { + throw std::runtime_error("BerkeleyDatabase: Failed to open database environment. " + util::ErrorString(opened).original); + } if (m_db == nullptr) { int ret; @@ -499,8 +497,8 @@ void BerkeleyEnvironment::ReloadDbEnv() // Reset the environment Flush(true); // This will flush and close the environment Reset(); - bilingual_str open_err; - Open(open_err); + auto opened = Open(); + if (!opened) LogPrintf("BerkeleyEnvironment::ReloadDbEnv Open failed: %s\n", util::ErrorString(opened).original); } DbTxn* BerkeleyEnvironment::TxnBegin(int flags) @@ -945,8 +943,10 @@ std::unique_ptr BerkeleyDatabase::MakeBatch(bool flush_on_close) return std::make_unique(*this, false, flush_on_close); } -std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +util::ResultPtr, DatabaseStatus> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options) { + util::ResultPtr, DatabaseStatus> result; + fs::path data_file = BDBDataFile(path); std::unique_ptr db; { @@ -954,19 +954,19 @@ std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, con fs::path data_filename = data_file.filename(); std::shared_ptr env = GetBerkeleyEnv(data_file.parent_path(), options.use_shared_memory); if (env->m_databases.count(data_filename)) { - error = Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename))); - status = DatabaseStatus::FAILED_ALREADY_LOADED; - return nullptr; + result.Update({util::Error{Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename)))}, + DatabaseStatus::FAILED_ALREADY_LOADED}); + return result; } db = std::make_unique(std::move(env), std::move(data_filename), options); } - if (options.verify && !db->Verify(error)) { - status = DatabaseStatus::FAILED_VERIFY; - return nullptr; + if (options.verify && !(db->Verify() >> result)) { + result.Update({util::Error{}, DatabaseStatus::FAILED_VERIFY}); + return result; } - status = DatabaseStatus::SUCCESS; - return db; + result.Update(std::move(db)); + return result; } } // namespace wallet diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index f3fe8a19c19..6b74c53440e 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -66,7 +67,7 @@ public: bool IsInitialized() const { return fDbEnvInit; } fs::path Directory() const { return fs::PathFromString(strPath); } - bool Open(bilingual_str& error); + util::Result Open(); void Close(); void Flush(bool fShutdown); void CheckpointLSN(const std::string& strFile); @@ -127,7 +128,7 @@ public: void ReloadDbEnv() override; /** Verifies the environment and database file */ - bool Verify(bilingual_str& error); + util::Result Verify(); /** Return path to main database filename */ std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); } @@ -219,7 +220,7 @@ std::string BerkeleyDatabaseVersion(); bool BerkeleyDatabaseSanityCheck(); //! Return object giving access to Berkeley database at specified path. -std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseStatus> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options); } // namespace wallet #endif // BITCOIN_WALLET_BDB_H diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index b924239073c..b7386cbff9f 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -82,7 +82,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil std::string filename = berkeley_database.Filename(); std::shared_ptr env = berkeley_database.env; - if (!env->Open(error)) { + if (!ResultExtract(env->Open(), nullptr, &error)) { return false; } diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index fa94bb9d15a..f0bde728fd8 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -131,10 +131,8 @@ static std::vector> TestDatabases(const fs::path { std::vector> dbs; DatabaseOptions options; - DatabaseStatus status; - bilingual_str error; #ifdef USE_BDB - dbs.emplace_back(MakeBerkeleyDatabase(path_root / "bdb", options, status, error)); + dbs.emplace_back(std::move(MakeBerkeleyDatabase(path_root / "bdb", options).value())); // Needs BDB to make the DB to read dbs.emplace_back(std::make_unique(BDBDataFile(path_root / "bdb"), /*open=*/false)); #endif diff --git a/src/wallet/test/fuzz/wallet_bdb_parser.cpp b/src/wallet/test/fuzz/wallet_bdb_parser.cpp index 6482b65d064..28381576a3b 100644 --- a/src/wallet/test/fuzz/wallet_bdb_parser.cpp +++ b/src/wallet/test/fuzz/wallet_bdb_parser.cpp @@ -114,7 +114,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser) g_setup->m_args.ForceSetArg("-dumpfile", fs::PathToString(bdb_dumpfile)); try { - auto db{MakeBerkeleyDatabase(wallet_path, options, status, error)}; + auto db{ResultExtract(MakeBerkeleyDatabase(wallet_path, options), &status, &error)}; if (bdb_ro_err && !db) { return; } diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 625d75b9608..46fdd1055e3 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1491,7 +1491,7 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas #ifdef USE_BDB if constexpr (true) { - return MakeBerkeleyDatabase(path, options, status, error); + return ResultExtract(MakeBerkeleyDatabase(path, options), &status, &error); } else #endif { From c7df70a984be514423b2bf5a42dc64b1c99d0e6f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 12/23] refactor: Use util::Result class in wallet/migrate --- src/wallet/migrate.cpp | 10 +++------- src/wallet/migrate.h | 3 ++- src/wallet/test/fuzz/wallet_bdb_parser.cpp | 2 +- src/wallet/walletdb.cpp | 2 +- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/wallet/migrate.cpp b/src/wallet/migrate.cpp index d2c163027d2..3e3566884db 100644 --- a/src/wallet/migrate.cpp +++ b/src/wallet/migrate.cpp @@ -768,17 +768,13 @@ std::unique_ptr BerkeleyROBatch::GetNewPrefixCursor(Span(m_database, prefix); } -std::unique_ptr MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +util::ResultPtr, DatabaseStatus> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options) { fs::path data_file = BDBDataFile(path); try { - std::unique_ptr db = std::make_unique(data_file); - status = DatabaseStatus::SUCCESS; - return db; + return std::make_unique(data_file); } catch (const std::runtime_error& e) { - error.original = e.what(); - status = DatabaseStatus::FAILED_LOAD; - return nullptr; + return {util::Error{Untranslated(e.what())}, DatabaseStatus::FAILED_LOAD}; } } } // namespace wallet diff --git a/src/wallet/migrate.h b/src/wallet/migrate.h index 16eadeb019d..dc4fde95d02 100644 --- a/src/wallet/migrate.h +++ b/src/wallet/migrate.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_WALLET_MIGRATE_H #define BITCOIN_WALLET_MIGRATE_H +#include #include #include @@ -119,7 +120,7 @@ public: }; //! Return object giving access to Berkeley Read Only database at specified path. -std::unique_ptr MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseStatus> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options); } // namespace wallet #endif // BITCOIN_WALLET_MIGRATE_H diff --git a/src/wallet/test/fuzz/wallet_bdb_parser.cpp b/src/wallet/test/fuzz/wallet_bdb_parser.cpp index 28381576a3b..ab933122a93 100644 --- a/src/wallet/test/fuzz/wallet_bdb_parser.cpp +++ b/src/wallet/test/fuzz/wallet_bdb_parser.cpp @@ -61,7 +61,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser) bool bdb_ro_err = false; bool bdb_ro_strict_err = false; #endif - auto db{MakeBerkeleyRODatabase(wallet_path, options, status, error)}; + auto db{ResultExtract(MakeBerkeleyRODatabase(wallet_path, options), &status, &error)}; if (db) { assert(DumpWallet(g_setup->m_args, *db, error)); } else { diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 46fdd1055e3..ee2e405ec9c 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1486,7 +1486,7 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas } if (format == DatabaseFormat::BERKELEY_RO) { - return MakeBerkeleyRODatabase(path, options, status, error); + return ResultExtract(MakeBerkeleyRODatabase(path, options), &status, &error); } #ifdef USE_BDB From 89bda8a13747259678892fd6c99f91bacb49b94b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 18 Jul 2024 13:12:05 -0400 Subject: [PATCH 13/23] refactor: Use util::Result class in wallet::MakeDatabase --- src/wallet/db.h | 3 ++- src/wallet/dump.cpp | 2 +- src/wallet/salvage.cpp | 2 +- src/wallet/test/walletdb_tests.cpp | 2 +- src/wallet/wallet.cpp | 4 +-- src/wallet/walletdb.cpp | 42 ++++++++++-------------------- src/wallet/wallettool.cpp | 4 +-- 7 files changed, 23 insertions(+), 36 deletions(-) diff --git a/src/wallet/db.h b/src/wallet/db.h index e8790006a4d..b1f5d2b446c 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -220,7 +221,7 @@ enum class DatabaseStatus { std::vector> ListDatabases(const fs::path& path); void ReadDatabaseArgs(const ArgsManager& args, DatabaseOptions& options); -std::unique_ptr MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseStatus> MakeDatabase(const fs::path& path, const DatabaseOptions& options); fs::path BDBDataFile(const fs::path& path); fs::path SQLiteDataFile(const fs::path& path); diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index db2756e0ca8..a0065e5fa52 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -203,7 +203,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: ReadDatabaseArgs(args, options); options.require_create = true; options.require_format = data_format; - std::unique_ptr database = MakeDatabase(wallet_path, options, status, error); + std::unique_ptr database = ResultExtract(MakeDatabase(wallet_path, options), &status, &error); if (!database) return false; // dummy chain interface diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index b7386cbff9f..c101421b649 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -75,7 +75,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil options.require_existing = true; options.verify = false; options.require_format = DatabaseFormat::BERKELEY; - std::unique_ptr database = MakeDatabase(file_path, options, status, error); + std::unique_ptr database = ResultExtract(MakeDatabase(file_path, options), &status, &error); if (!database) return false; BerkeleyDatabase& berkeley_database = static_cast(*database); diff --git a/src/wallet/test/walletdb_tests.cpp b/src/wallet/test/walletdb_tests.cpp index fee8c85873e..82e4bdd07ba 100644 --- a/src/wallet/test/walletdb_tests.cpp +++ b/src/wallet/test/walletdb_tests.cpp @@ -38,7 +38,7 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock) options.require_format = db_format; DatabaseStatus status; bilingual_str error_string; - std::unique_ptr db = MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options, status, error_string); + std::unique_ptr db = ResultExtract(MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options), &status, &error_string); BOOST_CHECK_EQUAL(status, DatabaseStatus::SUCCESS); std::shared_ptr wallet(new CWallet(m_node.chain.get(), "", std::move(db))); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d2504609116..fff3d2bb2c9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2992,7 +2992,7 @@ std::unique_ptr MakeWalletDatabase(const std::string& name, cons status = DatabaseStatus::FAILED_BAD_PATH; return nullptr; } - return MakeDatabase(*wallet_path, options, status, error_string); + return ResultExtract(MakeDatabase(*wallet_path, options), &status, &error_string); } std::shared_ptr CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) @@ -4050,7 +4050,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) opts.require_create = true; opts.require_format = DatabaseFormat::SQLITE; DatabaseStatus db_status; - std::unique_ptr new_db = MakeDatabase(wallet_path, opts, db_status, error); + std::unique_ptr new_db = ResultExtract(MakeDatabase(wallet_path, opts), &db_status, &error); assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists. m_database.reset(); m_database = std::move(new_db); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index ee2e405ec9c..bc7a760c427 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1405,15 +1405,13 @@ void WalletBatch::RegisterTxnListener(const DbTxnListener& l) m_txn_listeners.emplace_back(l); } -std::unique_ptr MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +util::ResultPtr, DatabaseStatus> MakeDatabase(const fs::path& path, const DatabaseOptions& options) { bool exists; try { exists = fs::symlink_status(path).type() != fs::file_type::not_found; } catch (const fs::filesystem_error& e) { - error = Untranslated(strprintf("Failed to access database path '%s': %s", fs::PathToString(path), fsbridge::get_filesystem_error_message(e))); - status = DatabaseStatus::FAILED_BAD_PATH; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to access database path '%s': %s", fs::PathToString(path), fsbridge::get_filesystem_error_message(e)))}, DatabaseStatus::FAILED_BAD_PATH}; } std::optional format; @@ -1423,28 +1421,20 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas } if (IsSQLiteFile(SQLiteDataFile(path))) { if (format) { - error = Untranslated(strprintf("Failed to load database path '%s'. Data is in ambiguous format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Data is in ambiguous format.", fs::PathToString(path)))}, DatabaseStatus::FAILED_BAD_FORMAT}; } format = DatabaseFormat::SQLITE; } } else if (options.require_existing) { - error = Untranslated(strprintf("Failed to load database path '%s'. Path does not exist.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_NOT_FOUND; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Path does not exist.", fs::PathToString(path)))}, DatabaseStatus::FAILED_NOT_FOUND}; } if (!format && options.require_existing) { - error = Untranslated(strprintf("Failed to load database path '%s'. Data is not in recognized format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Data is not in recognized format.", fs::PathToString(path)))}, DatabaseStatus::FAILED_BAD_FORMAT}; } if (format && options.require_create) { - error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_ALREADY_EXISTS; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(path)))}, DatabaseStatus::FAILED_ALREADY_EXISTS}; } // If BERKELEY was the format, then change the format from BERKELEY to BERKELEY_RO @@ -1454,9 +1444,7 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas // A db already exists so format is set, but options also specifies the format, so make sure they agree if (format && options.require_format && format != options.require_format) { - error = Untranslated(strprintf("Failed to load database path '%s'. Data is not in required format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Data is not in required format.", fs::PathToString(path)))}, DatabaseStatus::FAILED_BAD_FORMAT}; } // Format is not set when a db doesn't already exist, so use the format specified by the options if it is set. @@ -1475,29 +1463,27 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas if (format == DatabaseFormat::SQLITE) { #ifdef USE_SQLITE if constexpr (true) { - return ResultExtract(MakeSQLiteDatabase(path, options), &status, &error); + return MakeSQLiteDatabase(path, options); } else #endif { - error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path)))}, + DatabaseStatus::FAILED_BAD_FORMAT}; } } if (format == DatabaseFormat::BERKELEY_RO) { - return ResultExtract(MakeBerkeleyRODatabase(path, options), &status, &error); + return MakeBerkeleyRODatabase(path, options); } #ifdef USE_BDB if constexpr (true) { - return ResultExtract(MakeBerkeleyDatabase(path, options), &status, &error); + return MakeBerkeleyDatabase(path, options); } else #endif { - error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + return {util::Error{Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)))}, + DatabaseStatus::FAILED_BAD_FORMAT}; } } } // namespace wallet diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index f7e7b74fc36..4b63be0bd35 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -49,7 +49,7 @@ static std::shared_ptr MakeWallet(const std::string& name, const fs::pa { DatabaseStatus status; bilingual_str error; - std::unique_ptr database = MakeDatabase(path, options, status, error); + std::unique_ptr database = ResultExtract(MakeDatabase(path, options), &status, &error); if (!database) { tfm::format(std::cerr, "%s\n", error.original); return nullptr; @@ -198,7 +198,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) } bilingual_str error; - std::unique_ptr database = MakeDatabase(path, options, status, error); + std::unique_ptr database = ResultExtract(MakeDatabase(path, options), &status, &error); if (!database) { tfm::format(std::cerr, "%s\n", error.original); return false; From 462f82974ea75df433288a4174178814f4edd8b6 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 14/23] refactor: Use util::Result class in wallet/dump --- src/wallet/dump.cpp | 108 ++++++++++----------- src/wallet/dump.h | 5 +- src/wallet/test/fuzz/wallet_bdb_parser.cpp | 4 +- src/wallet/wallettool.cpp | 4 +- 4 files changed, 58 insertions(+), 63 deletions(-) diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index a0065e5fa52..a7968b08369 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -21,37 +21,36 @@ namespace wallet { static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP"; uint32_t DUMP_VERSION = 1; -bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error) +util::Result DumpWallet(const ArgsManager& args, WalletDatabase& db) { + util::Result result; // Get the dumpfile std::string dump_filename = args.GetArg("-dumpfile", ""); if (dump_filename.empty()) { - error = _("No dump file provided. To use dump, -dumpfile= must be provided."); - return false; + result.Update(util::Error{_("No dump file provided. To use dump, -dumpfile= must be provided.")}); + return result; } fs::path path = fs::PathFromString(dump_filename); path = fs::absolute(path); if (fs::exists(path)) { - error = strprintf(_("File %s already exists. If you are sure this is what you want, move it out of the way first."), fs::PathToString(path)); - return false; + result.Update(util::Error{strprintf(_("File %s already exists. If you are sure this is what you want, move it out of the way first."), fs::PathToString(path))}); + return result; } std::ofstream dump_file; dump_file.open(path); if (dump_file.fail()) { - error = strprintf(_("Unable to open %s for writing"), fs::PathToString(path)); - return false; + result.Update(util::Error{strprintf(_("Unable to open %s for writing"), fs::PathToString(path))}); + return result; } HashWriter hasher{}; std::unique_ptr batch = db.MakeBatch(); - bool ret = true; std::unique_ptr cursor = batch->GetNewCursor(); if (!cursor) { - error = _("Error: Couldn't create cursor into database"); - ret = false; + result.Update(util::Error{_("Error: Couldn't create cursor into database")}); } // Write out a magic string with version @@ -70,7 +69,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro dump_file.write(line.data(), line.size()); hasher << Span{line}; - if (ret) { + if (result) { // Read the records while (true) { @@ -78,11 +77,10 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro DataStream ss_value{}; DatabaseCursor::Status status = cursor->Next(ss_key, ss_value); if (status == DatabaseCursor::Status::DONE) { - ret = true; + result.Update({}); break; } else if (status == DatabaseCursor::Status::FAIL) { - error = _("Error reading next record from wallet database"); - ret = false; + result.Update(util::Error{_("Error reading next record from wallet database")}); break; } std::string key_str = HexStr(ss_key); @@ -96,7 +94,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro cursor.reset(); batch.reset(); - if (ret) { + if (result) { // Write the hash tfm::format(dump_file, "checksum,%s\n", HexStr(hasher.GetHash())); dump_file.close(); @@ -106,7 +104,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro fs::remove(path); } - return ret; + return result; } // The standard wallet deleter function blocks on the validation interface @@ -119,20 +117,21 @@ static void WalletToolReleaseWallet(CWallet* wallet) delete wallet; } -bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector& warnings) +util::Result CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path) { + util::Result result; // Get the dumpfile std::string dump_filename = args.GetArg("-dumpfile", ""); if (dump_filename.empty()) { - error = _("No dump file provided. To use createfromdump, -dumpfile= must be provided."); - return false; + result.Update(util::Error{_("No dump file provided. To use createfromdump, -dumpfile= must be provided.")}); + return result; } fs::path dump_path = fs::PathFromString(dump_filename); dump_path = fs::absolute(dump_path); if (!fs::exists(dump_path)) { - error = strprintf(_("Dump file %s does not exist."), fs::PathToString(dump_path)); - return false; + result.Update(util::Error{strprintf(_("Dump file %s does not exist."), fs::PathToString(dump_path))}); + return result; } std::ifstream dump_file{dump_path}; @@ -146,21 +145,21 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: std::string version_value; std::getline(dump_file, version_value, '\n'); if (magic_key != DUMP_MAGIC) { - error = strprintf(_("Error: Dumpfile identifier record is incorrect. Got \"%s\", expected \"%s\"."), magic_key, DUMP_MAGIC); dump_file.close(); - return false; + result.Update(util::Error{strprintf(_("Error: Dumpfile identifier record is incorrect. Got \"%s\", expected \"%s\"."), magic_key, DUMP_MAGIC)}); + return result; } // Check the version number (value of first record) uint32_t ver; if (!ParseUInt32(version_value, &ver)) { - error =strprintf(_("Error: Unable to parse version %u as a uint32_t"), version_value); dump_file.close(); - return false; + result.Update(util::Error{strprintf(_("Error: Unable to parse version %u as a uint32_t"), version_value)}); + return result; } if (ver != DUMP_VERSION) { - error = strprintf(_("Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version %s"), version_value); dump_file.close(); - return false; + result.Update(util::Error{strprintf(_("Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version %s"), version_value)}); + return result; } std::string magic_hasher_line = strprintf("%s,%s\n", magic_key, version_value); hasher << Span{magic_hasher_line}; @@ -171,15 +170,15 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: std::string format_value; std::getline(dump_file, format_value, '\n'); if (format_key != "format") { - error = strprintf(_("Error: Dumpfile format record is incorrect. Got \"%s\", expected \"format\"."), format_key); dump_file.close(); - return false; + result.Update(util::Error{strprintf(_("Error: Dumpfile format record is incorrect. Got \"%s\", expected \"format\"."), format_key)}); + return result; } // Get the data file format with format_value as the default std::string file_format = args.GetArg("-format", format_value); if (file_format.empty()) { - error = _("No wallet file format provided. To use createfromdump, -format= must be provided."); - return false; + result.Update(util::Error{_("No wallet file format provided. To use createfromdump, -format= must be provided.")}); + return result; } DatabaseFormat data_format; if (file_format == "bdb") { @@ -189,32 +188,33 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: } else if (file_format == "bdb_swap") { data_format = DatabaseFormat::BERKELEY_SWAP; } else { - error = strprintf(_("Unknown wallet file format \"%s\" provided. Please provide one of \"bdb\" or \"sqlite\"."), file_format); - return false; + result.Update(util::Error{strprintf(_("Unknown wallet file format \"%s\" provided. Please provide one of \"bdb\" or \"sqlite\"."), file_format)}); + return result; } if (file_format != format_value) { - warnings.push_back(strprintf(_("Warning: Dumpfile wallet format \"%s\" does not match command line specified format \"%s\"."), format_value, file_format)); + result.AddWarning(strprintf(_("Warning: Dumpfile wallet format \"%s\" does not match command line specified format \"%s\"."), format_value, file_format)); } std::string format_hasher_line = strprintf("%s,%s\n", format_key, format_value); hasher << Span{format_hasher_line}; DatabaseOptions options; - DatabaseStatus status; ReadDatabaseArgs(args, options); options.require_create = true; options.require_format = data_format; - std::unique_ptr database = ResultExtract(MakeDatabase(wallet_path, options), &status, &error); - if (!database) return false; + auto database{MakeDatabase(wallet_path, options) >> result}; + if (!database) { + result.Update(util::Error{}); + return result; + } // dummy chain interface - bool ret = true; - std::shared_ptr wallet(new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet); + std::shared_ptr wallet(new CWallet(/*chain=*/nullptr, name, std::move(database.value())), WalletToolReleaseWallet); { LOCK(wallet->cs_wallet); DBErrors load_wallet_ret = wallet->LoadWallet(); if (load_wallet_ret != DBErrors::LOAD_OK) { - error = strprintf(_("Error creating %s"), name); - return false; + result.Update(util::Error{strprintf(_("Error creating %s"), name)}); + return result; } // Get the database handle @@ -232,8 +232,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: if (key == "checksum") { std::vector parsed_checksum = ParseHex(value); if (parsed_checksum.size() != checksum.size()) { - error = Untranslated("Error: Checksum is not the correct size"); - ret = false; + result.Update(util::Error{Untranslated("Error: Checksum is not the correct size")}); break; } std::copy(parsed_checksum.begin(), parsed_checksum.end(), checksum.begin()); @@ -248,37 +247,32 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: } if (!IsHex(key)) { - error = strprintf(_("Error: Got key that was not hex: %s"), key); - ret = false; + result.Update(util::Error{strprintf(_("Error: Got key that was not hex: %s"), key)}); break; } if (!IsHex(value)) { - error = strprintf(_("Error: Got value that was not hex: %s"), value); - ret = false; + result.Update(util::Error{strprintf(_("Error: Got value that was not hex: %s"), value)}); break; } std::vector k = ParseHex(key); std::vector v = ParseHex(value); if (!batch->Write(Span{k}, Span{v})) { - error = strprintf(_("Error: Unable to write record to new wallet")); - ret = false; + result.Update(util::Error{strprintf(_("Error: Unable to write record to new wallet"))}); break; } } - if (ret) { + if (result) { uint256 comp_checksum = hasher.GetHash(); if (checksum.IsNull()) { - error = _("Error: Missing checksum"); - ret = false; + result.Update(util::Error{_("Error: Missing checksum")}); } else if (checksum != comp_checksum) { - error = strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum)); - ret = false; + result.Update(util::Error{strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum))}); } } - if (ret) { + if (result) { batch->TxnCommit(); } else { batch->TxnAbort(); @@ -291,10 +285,10 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: wallet.reset(); // The pointer deleter will close the wallet for us. // Remove the wallet dir if we have a failure - if (!ret) { + if (!result) { fs::remove_all(wallet_path); } - return ret; + return result; } } // namespace wallet diff --git a/src/wallet/dump.h b/src/wallet/dump.h index 9b44af922e1..9f641acd1ec 100644 --- a/src/wallet/dump.h +++ b/src/wallet/dump.h @@ -6,6 +6,7 @@ #define BITCOIN_WALLET_DUMP_H #include +#include #include #include @@ -16,8 +17,8 @@ class ArgsManager; namespace wallet { class WalletDatabase; -bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error); -bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector& warnings); +util::Result DumpWallet(const ArgsManager& args, WalletDatabase& db); +util::Result CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path); } // namespace wallet #endif // BITCOIN_WALLET_DUMP_H diff --git a/src/wallet/test/fuzz/wallet_bdb_parser.cpp b/src/wallet/test/fuzz/wallet_bdb_parser.cpp index ab933122a93..14b43234e9f 100644 --- a/src/wallet/test/fuzz/wallet_bdb_parser.cpp +++ b/src/wallet/test/fuzz/wallet_bdb_parser.cpp @@ -63,7 +63,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser) #endif auto db{ResultExtract(MakeBerkeleyRODatabase(wallet_path, options), &status, &error)}; if (db) { - assert(DumpWallet(g_setup->m_args, *db, error)); + assert(DumpWallet(g_setup->m_args, *db)); } else { #ifdef USE_BDB_NON_MSVC bdb_ro_err = true; @@ -124,7 +124,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser) return; } assert(!bdb_ro_err); - assert(DumpWallet(g_setup->m_args, *db, error)); + assert(DumpWallet(g_setup->m_args, *db)); } catch (const std::runtime_error& e) { if (bdb_ro_err) return; throw e; diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 4b63be0bd35..37e2374d7a0 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -204,7 +204,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) return false; } - bool ret = DumpWallet(args, *database, error); + bool ret = ResultExtract(DumpWallet(args, *database), nullptr, &error); if (!ret && !error.empty()) { tfm::format(std::cerr, "%s\n", error.original); return ret; @@ -214,7 +214,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) } else if (command == "createfromdump") { bilingual_str error; std::vector warnings; - bool ret = CreateFromDump(args, name, path, error, warnings); + bool ret = ResultExtract(CreateFromDump(args, name, path), nullptr, &error, &warnings); for (const auto& warning : warnings) { tfm::format(std::cout, "%s\n", warning.original); } From e15ee007828bb962f14daed71337de435030e981 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 15/23] refactor: Use util::Result class in wallet/salvage --- src/wallet/salvage.cpp | 67 ++++++++++++++++++++------------------- src/wallet/salvage.h | 2 +- src/wallet/wallettool.cpp | 2 +- 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index c101421b649..168f038467f 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -67,23 +67,27 @@ public: std::unique_ptr MakeBatch(bool flush_on_close = true) override { return std::make_unique(); } }; -bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bilingual_str& error, std::vector& warnings) +util::Result RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path) { + util::Result result; DatabaseOptions options; - DatabaseStatus status; ReadDatabaseArgs(args, options); options.require_existing = true; options.verify = false; options.require_format = DatabaseFormat::BERKELEY; - std::unique_ptr database = ResultExtract(MakeDatabase(file_path, options), &status, &error); - if (!database) return false; + auto database{MakeDatabase(file_path, options) >> result}; + if (!database) { + result.Update(util::Error{}); + return result; + } BerkeleyDatabase& berkeley_database = static_cast(*database); std::string filename = berkeley_database.Filename(); std::shared_ptr env = berkeley_database.env; - if (!ResultExtract(env->Open(), nullptr, &error)) { - return false; + if (!(env->Open() >> result)) { + result.Update(util::Error{}); + return result; } // Recovery procedure: @@ -96,12 +100,12 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil int64_t now = GetTime(); std::string newFilename = strprintf("%s.%d.bak", filename, now); - int result = env->dbenv->dbrename(nullptr, filename.c_str(), nullptr, - newFilename.c_str(), DB_AUTO_COMMIT); - if (result != 0) + int ret = env->dbenv->dbrename(nullptr, filename.c_str(), nullptr, + newFilename.c_str(), DB_AUTO_COMMIT); + if (ret != 0) { - error = Untranslated(strprintf("Failed to rename %s to %s", filename, newFilename)); - return false; + result.Update(util::Error{Untranslated(strprintf("Failed to rename %s to %s", filename, newFilename))}); + return result; } /** @@ -115,13 +119,13 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil std::stringstream strDump; Db db(env->dbenv.get(), 0); - result = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE); - if (result == DB_VERIFY_BAD) { - warnings.emplace_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable.")); + ret = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE); + if (ret == DB_VERIFY_BAD) { + result.AddWarning(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable.")); } - if (result != 0 && result != DB_VERIFY_BAD) { - error = Untranslated(strprintf("Salvage: Database salvage failed with result %d.", result)); - return false; + if (ret != 0 && ret != DB_VERIFY_BAD) { + result.Update(util::Error{Untranslated(strprintf("Salvage: Database salvage failed with result %d.", ret))}); + return result; } // Format of bdb dump is ascii lines: @@ -144,38 +148,36 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil break; getline(strDump, valueHex); if (valueHex == DATA_END) { - warnings.emplace_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values.")); + result.AddWarning(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values.")); break; } salvagedData.emplace_back(ParseHex(keyHex), ParseHex(valueHex)); } } - bool fSuccess; if (keyHex != DATA_END) { - warnings.emplace_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output.")); - fSuccess = false; - } else { - fSuccess = (result == 0); + result.Update({util::Error{}, util::Warning{Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output.")}}); + } else if (ret != 0) { + result.Update(util::Error{}); } if (salvagedData.empty()) { - error = Untranslated(strprintf("Salvage(aggressive) found no records in %s.", newFilename)); - return false; + result.Update(util::Error{Untranslated(strprintf("Salvage(aggressive) found no records in %s.", newFilename))}); + return result; } std::unique_ptr pdbCopy = std::make_unique(env->dbenv.get(), 0); - int ret = pdbCopy->open(nullptr, // Txn pointer + ret = pdbCopy->open(nullptr, // Txn pointer filename.c_str(), // Filename "main", // Logical db name DB_BTREE, // Database type DB_CREATE, // Flags 0); if (ret > 0) { - error = Untranslated(strprintf("Cannot create database file %s", filename)); pdbCopy->close(0); - return false; + result.Update(util::Error{Untranslated(strprintf("Cannot create database file %s", filename))}); + return result; } DbTxn* ptxn = env->TxnBegin(DB_TXN_WRITE_NOSYNC); @@ -204,18 +206,19 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil if (!fReadOK) { - warnings.push_back(Untranslated(strprintf("WARNING: WalletBatch::Recover skipping %s: %s", strType, strErr))); + result.AddWarning(Untranslated(strprintf("WARNING: WalletBatch::Recover skipping %s: %s", strType, strErr))); continue; } Dbt datKey(row.first.data(), row.first.size()); Dbt datValue(row.second.data(), row.second.size()); int ret2 = pdbCopy->put(ptxn, &datKey, &datValue, DB_NOOVERWRITE); - if (ret2 > 0) - fSuccess = false; + if (ret2 > 0) { + result.Update(util::Error{}); + } } ptxn->commit(0); pdbCopy->close(0); - return fSuccess; + return result; } } // namespace wallet diff --git a/src/wallet/salvage.h b/src/wallet/salvage.h index fbf152ec792..13f3503f353 100644 --- a/src/wallet/salvage.h +++ b/src/wallet/salvage.h @@ -13,7 +13,7 @@ class ArgsManager; struct bilingual_str; namespace wallet { -bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bilingual_str& error, std::vector& warnings); +util::Result RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path); } // namespace wallet #endif // BITCOIN_WALLET_SALVAGE_H diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 37e2374d7a0..109b3dc47d3 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -173,7 +173,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) #ifdef USE_BDB bilingual_str error; std::vector warnings; - bool ret = RecoverDatabaseFile(args, path, error, warnings); + bool ret = ResultExtract(RecoverDatabaseFile(args, path), nullptr, &error, &warnings); if (!ret) { for (const auto& warning : warnings) { tfm::format(std::cerr, "%s\n", warning.original); From a308f72ee14fcd84f1273f48d3046b6b1da7c334 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 16/23] refactor: Use util::Result class in wallet/wallettool --- src/wallet/wallettool.cpp | 54 +++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 109b3dc47d3..5c82ce727c5 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -47,16 +47,14 @@ static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flag static std::shared_ptr MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options) { - DatabaseStatus status; - bilingual_str error; - std::unique_ptr database = ResultExtract(MakeDatabase(path, options), &status, &error); + auto database{MakeDatabase(path, options)}; if (!database) { - tfm::format(std::cerr, "%s\n", error.original); + tfm::format(std::cerr, "%s\n", util::ErrorString(database).original); return nullptr; } // dummy chain interface - std::shared_ptr wallet_instance{new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet}; + std::shared_ptr wallet_instance{new CWallet(/*chain=*/nullptr, name, std::move(database.value())), WalletToolReleaseWallet}; DBErrors load_wallet_ret; try { load_wallet_ret = wallet_instance->LoadWallet(); @@ -171,18 +169,16 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) wallet_instance->Close(); } else if (command == "salvage") { #ifdef USE_BDB - bilingual_str error; - std::vector warnings; - bool ret = ResultExtract(RecoverDatabaseFile(args, path), nullptr, &error, &warnings); + auto ret{RecoverDatabaseFile(args, path)}; if (!ret) { - for (const auto& warning : warnings) { + for (const auto& warning : ret.GetMessages()->warnings) { tfm::format(std::cerr, "%s\n", warning.original); } - if (!error.empty()) { + for (const auto& error : ret.GetMessages()->errors) { tfm::format(std::cerr, "%s\n", error.original); } } - return ret; + return bool{ret}; #else tfm::format(std::cerr, "Salvage command is not available as BDB support is not compiled"); return false; @@ -191,37 +187,33 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) DatabaseOptions options; ReadDatabaseArgs(args, options); options.require_existing = true; - DatabaseStatus status; - if (args.GetBoolArg("-withinternalbdb", false) && IsBDBFile(BDBDataFile(path))) { options.require_format = DatabaseFormat::BERKELEY_RO; } - - bilingual_str error; - std::unique_ptr database = ResultExtract(MakeDatabase(path, options), &status, &error); + auto database{MakeDatabase(path, options)}; if (!database) { - tfm::format(std::cerr, "%s\n", error.original); + tfm::format(std::cerr, "%s\n", util::ErrorString(database).original); return false; } - bool ret = ResultExtract(DumpWallet(args, *database), nullptr, &error); - if (!ret && !error.empty()) { - tfm::format(std::cerr, "%s\n", error.original); - return ret; + auto ret{DumpWallet(args, *database)}; + if (!ret) { + tfm::format(std::cerr, "%s\n", util::ErrorString(ret).original); + return false; } tfm::format(std::cout, "The dumpfile may contain private keys. To ensure the safety of your Bitcoin, do not share the dumpfile.\n"); - return ret; + return bool{ret}; } else if (command == "createfromdump") { - bilingual_str error; - std::vector warnings; - bool ret = ResultExtract(CreateFromDump(args, name, path), nullptr, &error, &warnings); - for (const auto& warning : warnings) { - tfm::format(std::cout, "%s\n", warning.original); + auto ret{CreateFromDump(args, name, path)}; + if (ret.GetMessages()) { + for (const auto& warning : ret.GetMessages()->warnings) { + tfm::format(std::cout, "%s\n", warning.original); + } + for (const auto& error : ret.GetMessages()->errors) { + tfm::format(std::cerr, "%s\n", error.original); + } } - if (!ret && !error.empty()) { - tfm::format(std::cerr, "%s\n", error.original); - } - return ret; + return bool{ret}; } else { tfm::format(std::cerr, "Invalid command: %s\n", command); return false; From c16cfa3f01e7bce4ccabdd0680e27e9e9cab13a4 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 17/23] refactor: Use util::Result class in wallet/wallet --- src/bench/wallet_create.cpp | 2 +- src/wallet/interfaces.cpp | 10 +- src/wallet/load.cpp | 10 +- src/wallet/rpc/backup.cpp | 2 +- src/wallet/rpc/wallet.cpp | 6 +- src/wallet/test/util.cpp | 4 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 507 ++++++++++++++++--------------- src/wallet/wallet.h | 19 +- 9 files changed, 282 insertions(+), 280 deletions(-) diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp index 3b916d7c39b..ae9c654493d 100644 --- a/src/bench/wallet_create.cpp +++ b/src/bench/wallet_create.cpp @@ -47,7 +47,7 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted) auto wallet_path = fs::PathToString(test_setup->m_path_root / "test_wallet"); bench.run([&] { - auto wallet = CreateWallet(context, wallet_path, /*load_on_start=*/std::nullopt, options, status, error_string, warnings); + auto wallet{ResultExtract(CreateWallet(context, wallet_path, /*load_on_start=*/std::nullopt, options), &status, &error_string, &warnings)}; assert(status == DatabaseStatus::SUCCESS); assert(wallet != nullptr); diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 808c8316285..10319d579e0 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -608,7 +608,7 @@ public: options.create_flags = wallet_creation_flags; options.create_passphrase = passphrase; bilingual_str error; - util::ResultPtr> wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; + util::ResultPtr> wallet{MakeWallet(m_context, ResultExtract(CreateWallet(m_context, name, /*load_on_start=*/true, options), &status, &error, &warnings))}; return wallet ? std::move(wallet) : util::Error{error}; } util::ResultPtr> loadWallet(const std::string& name, std::vector& warnings) override @@ -618,14 +618,14 @@ public: ReadDatabaseArgs(*m_context.args, options); options.require_existing = true; bilingual_str error; - util::ResultPtr> wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; + util::ResultPtr> wallet{MakeWallet(m_context, ResultExtract(LoadWallet(m_context, name, /*load_on_start=*/true, options), &status, &error, &warnings))}; return wallet ? std::move(wallet) : util::Error{error}; } util::ResultPtr> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) override { DatabaseStatus status; bilingual_str error; - util::ResultPtr> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))}; + util::ResultPtr> wallet{MakeWallet(m_context, ResultExtract(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 @@ -649,9 +649,7 @@ public: // Unloaded wallet, read db DatabaseOptions options; options.require_existing = true; - DatabaseStatus status; - bilingual_str error; - auto db = MakeWalletDatabase(wallet_name, options, status, error); + auto db{MakeWalletDatabase(wallet_name, options)}; if (!db) return false; return WalletBatch(*db).IsEncrypted(); } diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index e77999b1115..6cb14efe90a 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -63,7 +63,7 @@ bool VerifyWallets(WalletContext& context) bilingual_str error_string; options.require_existing = true; options.verify = false; - if (MakeWalletDatabase("", options, status, error_string)) { + if (ResultExtract(MakeWalletDatabase("", options), &status, &error_string)) { common::SettingsValue wallets(common::SettingsValue::VARR); wallets.push_back(""); // Default wallet name is "" // Pass write=false because no need to write file and probably @@ -96,7 +96,7 @@ bool VerifyWallets(WalletContext& context) options.require_existing = true; options.verify = true; bilingual_str error_string; - if (!MakeWalletDatabase(wallet_file, options, status, error_string)) { + if (!ResultExtract(MakeWalletDatabase(wallet_file, options), &status, &error_string)) { if (status == DatabaseStatus::FAILED_NOT_FOUND) { chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", error_string.original))); } else { @@ -131,12 +131,12 @@ bool LoadWallets(WalletContext& context) options.verify = false; // No need to verify, assuming verified earlier in VerifyWallets() bilingual_str error; std::vector warnings; - std::unique_ptr database = MakeWalletDatabase(name, options, status, error); + std::unique_ptr database{ResultExtract(MakeWalletDatabase(name, options), &status, &error)}; if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) { continue; } chain.initMessage(_("Loading wallet…")); - std::shared_ptr pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr; + std::shared_ptr pwallet{database ? ResultExtract(CWallet::Create(context, name, std::move(database), options.create_flags), nullptr, &error, &warnings) : nullptr}; if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); if (!pwallet) { chain.initError(error); @@ -187,7 +187,7 @@ void UnloadWallets(WalletContext& context) auto wallet = wallets.back(); wallets.pop_back(); std::vector warnings; - RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt, warnings); + ResultExtract(RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt), nullptr, nullptr, &warnings); WaitForDeleteWallet(std::move(wallet)); } } diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index ac23b092d40..31919127eff 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1967,7 +1967,7 @@ RPCHelpMan restorewallet() bilingual_str error; std::vector warnings; - const std::shared_ptr wallet = RestoreWallet(context, backup_file, wallet_name, load_on_start, status, error, warnings); + const std::shared_ptr wallet{ResultExtract(RestoreWallet(context, backup_file, wallet_name, load_on_start), &status, &error, &warnings)}; HandleWalletError(wallet, status, error); diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 89b174d0f7a..7ae99b44bed 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -264,7 +264,7 @@ static RPCHelpMan loadwallet() } } - std::shared_ptr const wallet = LoadWallet(context, name, load_on_start, options, status, error, warnings); + std::shared_ptr const wallet{ResultExtract(LoadWallet(context, name, load_on_start, options), &status, &error, &warnings)}; HandleWalletError(wallet, status, error); @@ -435,7 +435,7 @@ static RPCHelpMan createwallet() options.create_passphrase = passphrase; bilingual_str error; std::optional load_on_start = request.params[6].isNull() ? std::nullopt : std::optional(request.params[6].get_bool()); - const std::shared_ptr wallet = CreateWallet(context, request.params[0].get_str(), load_on_start, options, status, error, warnings); + const std::shared_ptr wallet{ResultExtract(CreateWallet(context, request.params[0].get_str(), load_on_start, options), &status, &error, &warnings)}; if (!wallet) { RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR; throw JSONRPCError(code, error.original); @@ -497,7 +497,7 @@ static RPCHelpMan unloadwallet() // Note that any attempt to load the same wallet would fail until the wallet // is destroyed (see CheckUniqueFileid). std::optional load_on_start{self.MaybeArg("load_on_startup")}; - if (!RemoveWallet(context, wallet, load_on_start, warnings)) { + if (!ResultExtract(RemoveWallet(context, wallet, load_on_start), nullptr, nullptr, &warnings)) { throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); } } diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index bc53510fe49..8a85e49d277 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -51,7 +51,7 @@ std::shared_ptr TestLoadWallet(std::unique_ptr database { bilingual_str error; std::vector warnings; - auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings); + auto wallet{ResultExtract(CWallet::Create(context, "", std::move(database), create_flags), nullptr, &error, &warnings)}; NotifyWalletLoaded(context, wallet); if (context.chain) { wallet->postInitProcess(); @@ -66,7 +66,7 @@ std::shared_ptr TestLoadWallet(WalletContext& context) DatabaseStatus status; bilingual_str error; std::vector warnings; - auto database = MakeWalletDatabase("", options, status, error); + auto database{ResultExtract(MakeWalletDatabase("", options), &status, &error)}; return TestLoadWallet(std::move(database), context, options.create_flags); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b5de4b4b3d3..23833d79325 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -452,7 +452,7 @@ void TestLoadWallet(const std::string& name, DatabaseFormat format, std::functio DatabaseStatus status; bilingual_str error; std::vector warnings; - auto database{MakeWalletDatabase(name, options, status, error)}; + auto database{ResultExtract(MakeWalletDatabase(name, options), &status, &error)}; auto wallet{std::make_shared(chain.get(), "", std::move(database))}; BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::LOAD_OK); WITH_LOCK(wallet->cs_wallet, f(wallet)); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fff3d2bb2c9..72a81688f9a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -119,17 +119,18 @@ bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_nam return chain.updateRwSetting("wallet", update_function); } -static void UpdateWalletSetting(interfaces::Chain& chain, - const std::string& wallet_name, - std::optional load_on_startup, - std::vector& warnings) +static util::Result UpdateWalletSetting(interfaces::Chain& chain, + const std::string& wallet_name, + std::optional load_on_startup) { - if (!load_on_startup) return; + util::Result result; + if (!load_on_startup) return result; if (load_on_startup.value() && !AddWalletSetting(chain, wallet_name)) { - warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.")); + result.AddWarning(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.")); } else if (!load_on_startup.value() && !RemoveWalletSetting(chain, wallet_name)) { - warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may still be loaded next node startup.")); + result.AddWarning(Untranslated("Wallet load on startup setting could not be updated, so wallet may still be loaded next node startup.")); } + return result; } /** @@ -158,9 +159,10 @@ bool AddWallet(WalletContext& context, const std::shared_ptr& wallet) return true; } -bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start, std::vector& warnings) +util::Result RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start) { assert(wallet); + util::Result result; interfaces::Chain& chain = wallet->chain(); std::string name = wallet->GetName(); @@ -170,22 +172,19 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet { LOCK(context.wallets_mutex); std::vector>::iterator i = std::find(context.wallets.begin(), context.wallets.end(), wallet); - if (i == context.wallets.end()) return false; + if (i == context.wallets.end()) { + result.Update(util::Error{}); + return result; + } context.wallets.erase(i); } // Notify unload so that upper layers release the shared pointer. wallet->NotifyUnload(); // Write the wallet setting - UpdateWalletSetting(chain, name, load_on_start, warnings); + UpdateWalletSetting(chain, name, load_on_start) >> result; - return true; -} - -bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start) -{ - std::vector warnings; - return RemoveWallet(context, wallet, load_on_start, warnings); + return result; } std::vector> GetWallets(WalletContext& context) @@ -271,41 +270,44 @@ void WaitForDeleteWallet(std::shared_ptr&& wallet) } namespace { -std::shared_ptr LoadWalletInternal(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) +util::ResultPtr, DatabaseStatus> LoadWalletInternal(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options) { + util::ResultPtr, DatabaseStatus> result; try { - std::unique_ptr database = MakeWalletDatabase(name, options, status, error); + auto database{MakeWalletDatabase(name, options) >> result}; if (!database) { - error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; - return nullptr; + auto& errors{result.EnsureMessages().errors}; + errors.insert(errors.begin(), Untranslated("Wallet file verification failed.")); + result.Update({util::Error{}, database.GetFailure()}); + return result; } context.chain->initMessage(_("Loading wallet…")); - std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings); + auto wallet{CWallet::Create(context, name, std::move(database.value()), options.create_flags) >> result}; if (!wallet) { - error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error; - status = DatabaseStatus::FAILED_LOAD; - return nullptr; + auto& errors{result.EnsureMessages().errors}; + errors.insert(errors.begin(), Untranslated("Wallet loading failed.")); + result.Update({util::Error{}, DatabaseStatus::FAILED_LOAD}); + return result; } // Legacy wallets are being deprecated, warn if the loaded wallet is legacy if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - warnings.emplace_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); + result.AddWarning(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); } - NotifyWalletLoaded(context, wallet); - AddWallet(context, wallet); + NotifyWalletLoaded(context, wallet.value()); + AddWallet(context, wallet.value()); wallet->postInitProcess(); // Write the wallet setting - UpdateWalletSetting(*context.chain, name, load_on_start, warnings); + UpdateWalletSetting(*context.chain, name, load_on_start) >> result; - return wallet; + result.Update(std::move(wallet.value())); } catch (const std::runtime_error& e) { - error = Untranslated(e.what()); - status = DatabaseStatus::FAILED_LOAD; - return nullptr; + result.Update({util::Error{Untranslated(e.what())}, DatabaseStatus::FAILED_LOAD}); } + return result; } class FastWalletRescanFilter @@ -367,21 +369,20 @@ private: }; } // namespace -std::shared_ptr LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) +util::ResultPtr, DatabaseStatus> LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options) { auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(name)); if (!result.second) { - error = Untranslated("Wallet already loading."); - status = DatabaseStatus::FAILED_LOAD; - return nullptr; + return {util::Error{Untranslated("Wallet already loading.")}, DatabaseStatus::FAILED_LOAD}; } - auto wallet = LoadWalletInternal(context, name, load_on_start, options, status, error, warnings); + auto wallet{LoadWalletInternal(context, name, load_on_start, options)}; WITH_LOCK(g_loading_wallet_mutex, g_loading_wallet_set.erase(result.first)); return wallet; } -std::shared_ptr CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) +util::ResultPtr, DatabaseStatus> CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options) { + util::Result, DatabaseStatus> result; uint64_t wallet_creation_flags = options.create_flags; const SecureString& passphrase = options.create_passphrase; @@ -402,55 +403,51 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& // Private keys must be disabled for an external signer wallet if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { - error = Untranslated("Private keys must be disabled when using an external signer"); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; + result.Update({util::Error{Untranslated("Private keys must be disabled when using an external signer")}, DatabaseStatus::FAILED_CREATE}); + return result; } // Descriptor support must be enabled for an external signer wallet if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) { - error = Untranslated("Descriptor support must be enabled when using an external signer"); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; + result.Update({util::Error{Untranslated("Descriptor support must be enabled when using an external signer")}, DatabaseStatus::FAILED_CREATE}); + return result; } // Do not allow a passphrase when private keys are disabled if (!passphrase.empty() && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { - error = Untranslated("Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled."); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; + result.Update({util::Error{Untranslated("Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.")}, DatabaseStatus::FAILED_CREATE}); + return result; } // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. - std::unique_ptr database = MakeWalletDatabase(name, options, status, error); + auto database{MakeWalletDatabase(name, options) >> result}; if (!database) { - error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; - status = DatabaseStatus::FAILED_VERIFY; - return nullptr; + auto& errors{result.EnsureMessages().errors}; + errors.insert(errors.begin(), Untranslated("Wallet file verification failed.")); + result.Update({util::Error{}, DatabaseStatus::FAILED_VERIFY}); + return result; } // Make the wallet context.chain->initMessage(_("Loading wallet…")); - std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), wallet_creation_flags, error, warnings); - if (!wallet) { - error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; - status = DatabaseStatus::FAILED_CREATE; - return nullptr; + auto create{CWallet::Create(context, name, std::move(database.value()), wallet_creation_flags) >> result}; + if (!create) { + result.Update({util::Error{Untranslated("Wallet creation failed.")}, DatabaseStatus::FAILED_CREATE}); + return result; } + std::shared_ptr wallet = create.value(); // Encrypt the wallet if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { if (!wallet->EncryptWallet(passphrase)) { - error = Untranslated("Error: Wallet created but failed to encrypt."); - status = DatabaseStatus::FAILED_ENCRYPT; - return nullptr; + result.Update({util::Error{Untranslated("Error: Wallet created but failed to encrypt.")}, DatabaseStatus::FAILED_ENCRYPT}); + return result; } if (!create_blank) { // Unlock the wallet if (!wallet->Unlock(passphrase)) { - error = Untranslated("Error: Wallet was encrypted but could not be unlocked"); - status = DatabaseStatus::FAILED_ENCRYPT; - return nullptr; + result.Update({util::Error{Untranslated("Error: Wallet was encrypted but could not be unlocked")}, DatabaseStatus::FAILED_ENCRYPT}); + return result; } // Set a seed for the wallet @@ -461,9 +458,8 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& } else { for (auto spk_man : wallet->GetActiveScriptPubKeyMans()) { if (!spk_man->SetupGeneration()) { - error = Untranslated("Unable to generate initial keys"); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; + result.Update({util::Error{Untranslated("Unable to generate initial keys")}, DatabaseStatus::FAILED_CREATE}); + return result; } } } @@ -479,59 +475,57 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& wallet->postInitProcess(); // Write the wallet settings - UpdateWalletSetting(*context.chain, name, load_on_start, warnings); + UpdateWalletSetting(*context.chain, name, load_on_start) >> result; // Legacy wallets are being deprecated, warn if a newly created wallet is legacy if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) { - warnings.emplace_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.")); + result.AddWarning(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.")); } - status = DatabaseStatus::SUCCESS; - return wallet; + result.Update(std::move(wallet)); + return result; } // Re-creates wallet from the backup file by renaming and moving it into the wallet's directory. // If 'load_after_restore=true', the wallet object will be fully initialized and appended to the context. -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore) +util::ResultPtr, DatabaseStatus> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, bool load_after_restore) { + util::ResultPtr, DatabaseStatus> result; DatabaseOptions options; ReadDatabaseArgs(*context.args, options); options.require_existing = true; const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name)); auto wallet_file = wallet_path / "wallet.dat"; - std::shared_ptr wallet; try { if (!fs::exists(backup_file)) { - error = Untranslated("Backup file does not exist"); - status = DatabaseStatus::FAILED_INVALID_BACKUP_FILE; - return nullptr; + result.Update({util::Error{Untranslated("Backup file does not exist")}, DatabaseStatus::FAILED_INVALID_BACKUP_FILE}); + return result; } if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) { - error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path))); - status = DatabaseStatus::FAILED_ALREADY_EXISTS; - return nullptr; + result.Update({util::Error{Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)))}, DatabaseStatus::FAILED_ALREADY_EXISTS}); + return result; } fs::copy_file(backup_file, wallet_file, fs::copy_options::none); if (load_after_restore) { - wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); + result.Update(LoadWallet(context, wallet_name, load_on_start, options)); } } catch (const std::exception& e) { - assert(!wallet); - if (!error.empty()) error += Untranslated("\n"); - error += Untranslated(strprintf("Unexpected exception: %s", e.what())); + assert(!result.value()); + result.Update({util::Error{Untranslated(strprintf("Unexpected exception: %s", e.what()))}, DatabaseStatus::FAILED_LOAD}); + return result; } // Remove created wallet path only when loading fails - if (load_after_restore && !wallet) { + if (load_after_restore && !result) { fs::remove_all(wallet_path); } - return wallet; + return result; } /** @defgroup mapWallet @@ -2984,19 +2978,21 @@ static util::Result GetWalletPath(const std::string& name) return wallet_path; } -std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string) +util::ResultPtr, DatabaseStatus> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options) { - const auto& wallet_path = GetWalletPath(name); + util::ResultPtr, DatabaseStatus> result; + auto wallet_path{GetWalletPath(name) >> result}; if (!wallet_path) { - error_string = util::ErrorString(wallet_path); - status = DatabaseStatus::FAILED_BAD_PATH; - return nullptr; + result.Update({util::Error{}, DatabaseStatus::FAILED_BAD_PATH}); + return result; } - return ResultExtract(MakeDatabase(*wallet_path, options), &status, &error_string); + result.Update(MakeDatabase(*wallet_path, options)); + return result; } -std::shared_ptr CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) +util::ResultPtr> CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags) { + util::ResultPtr> result; interfaces::Chain* chain = context.chain; ArgsManager& args = *Assert(context.args); const std::string& walletFile = database->Filename(); @@ -3013,43 +3009,43 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri DBErrors nLoadWalletRet = walletInstance->LoadWallet(); if (nLoadWalletRet != DBErrors::LOAD_OK) { if (nLoadWalletRet == DBErrors::CORRUPT) { - error = strprintf(_("Error loading %s: Wallet corrupted"), walletFile); - return nullptr; + result.Update(util::Error{strprintf(_("Error loading %s: Wallet corrupted"), walletFile)}); + return result; } else if (nLoadWalletRet == DBErrors::NONCRITICAL_ERROR) { - warnings.push_back(strprintf(_("Error reading %s! All keys read correctly, but transaction data" + result.AddWarning(strprintf(_("Error reading %s! All keys read correctly, but transaction data" " or address metadata may be missing or incorrect."), walletFile)); } else if (nLoadWalletRet == DBErrors::TOO_NEW) { - error = strprintf(_("Error loading %s: Wallet requires newer version of %s"), walletFile, CLIENT_NAME); - return nullptr; + result.Update(util::Error{strprintf(_("Error loading %s: Wallet requires newer version of %s"), walletFile, CLIENT_NAME)}); + return result; } else if (nLoadWalletRet == DBErrors::EXTERNAL_SIGNER_SUPPORT_REQUIRED) { - error = strprintf(_("Error loading %s: External signer wallet being loaded without external signer support compiled"), walletFile); - return nullptr; + result.Update(util::Error{strprintf(_("Error loading %s: External signer wallet being loaded without external signer support compiled"), walletFile)}); + return result; } else if (nLoadWalletRet == DBErrors::NEED_REWRITE) { - error = strprintf(_("Wallet needed to be rewritten: restart %s to complete"), CLIENT_NAME); - return nullptr; + result.Update(util::Error{strprintf(_("Wallet needed to be rewritten: restart %s to complete"), CLIENT_NAME)}); + return result; } else if (nLoadWalletRet == DBErrors::NEED_RESCAN) { - warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." + result.AddWarning(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." " Rescanning wallet."), walletFile)); rescan_required = true; } else if (nLoadWalletRet == DBErrors::UNKNOWN_DESCRIPTOR) { - error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" + result.Update(util::Error{strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" "The wallet might had been created on a newer version.\n" - "Please try running the latest software version.\n"), walletFile); - return nullptr; + "Please try running the latest software version.\n"), walletFile)}); + return result; } else if (nLoadWalletRet == DBErrors::UNEXPECTED_LEGACY_ENTRY) { - error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n" - "The wallet might have been tampered with or created with malicious intent.\n"), walletFile); - return nullptr; + result.Update(util::Error{strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n" + "The wallet might have been tampered with or created with malicious intent.\n"), walletFile)}); + return result; } else { - error = strprintf(_("Error loading %s"), walletFile); - return nullptr; + result.Update(util::Error{strprintf(_("Error loading %s"), walletFile)}); + return result; } } @@ -3078,8 +3074,8 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri // Legacy wallets need SetupGeneration here. for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) { if (!spk_man->SetupGeneration()) { - error = _("Unable to generate initial keys"); - return nullptr; + result.Update(util::Error{_("Unable to generate initial keys")}); + return result; } } } @@ -3090,12 +3086,12 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { // Make it impossible to disable private keys after creation - error = strprintf(_("Error loading %s: Private keys can only be disabled during creation"), walletFile); - return nullptr; + result.Update(util::Error{strprintf(_("Error loading %s: Private keys can only be disabled during creation"), walletFile)}); + return result; } else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) { if (spk_man->HavePrivateKeys()) { - warnings.push_back(strprintf(_("Warning: Private keys detected in wallet {%s} with disabled private keys"), walletFile)); + result.AddWarning(strprintf(_("Warning: Private keys detected in wallet {%s} with disabled private keys"), walletFile)); break; } } @@ -3104,8 +3100,8 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (!args.GetArg("-addresstype", "").empty()) { std::optional parsed = ParseOutputType(args.GetArg("-addresstype", "")); if (!parsed) { - error = strprintf(_("Unknown address type '%s'"), args.GetArg("-addresstype", "")); - return nullptr; + result.Update(util::Error{strprintf(_("Unknown address type '%s'"), args.GetArg("-addresstype", ""))}); + return result; } walletInstance->m_default_address_type = parsed.value(); } @@ -3113,8 +3109,8 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (!args.GetArg("-changetype", "").empty()) { std::optional parsed = ParseOutputType(args.GetArg("-changetype", "")); if (!parsed) { - error = strprintf(_("Unknown change type '%s'"), args.GetArg("-changetype", "")); - return nullptr; + result.Update(util::Error{strprintf(_("Unknown change type '%s'"), args.GetArg("-changetype", ""))}); + return result; } walletInstance->m_default_change_type = parsed.value(); } @@ -3122,10 +3118,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (args.IsArgSet("-mintxfee")) { std::optional min_tx_fee = ParseMoney(args.GetArg("-mintxfee", "")); if (!min_tx_fee) { - error = AmountErrMsg("mintxfee", args.GetArg("-mintxfee", "")); - return nullptr; + result.Update(util::Error{AmountErrMsg("mintxfee", args.GetArg("-mintxfee", ""))}); + return result; } else if (min_tx_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") + + result.AddWarning(AmountHighWarn("-mintxfee") + Untranslated(" ") + _("This is the minimum transaction fee you pay on every transaction.")); } @@ -3138,23 +3134,23 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_max_aps_fee = -1; } else if (std::optional max_fee = ParseMoney(max_aps_fee)) { if (max_fee.value() > HIGH_APS_FEE) { - warnings.push_back(AmountHighWarn("-maxapsfee") + Untranslated(" ") + + result.AddWarning(AmountHighWarn("-maxapsfee") + Untranslated(" ") + _("This is the maximum transaction fee you pay (in addition to the normal fee) to prioritize partial spend avoidance over regular coin selection.")); } walletInstance->m_max_aps_fee = max_fee.value(); } else { - error = AmountErrMsg("maxapsfee", max_aps_fee); - return nullptr; + result.Update(util::Error{AmountErrMsg("maxapsfee", max_aps_fee)}); + return result; } } if (args.IsArgSet("-fallbackfee")) { std::optional fallback_fee = ParseMoney(args.GetArg("-fallbackfee", "")); if (!fallback_fee) { - error = strprintf(_("Invalid amount for %s=: '%s'"), "-fallbackfee", args.GetArg("-fallbackfee", "")); - return nullptr; + result.Update(util::Error{strprintf(_("Invalid amount for %s=: '%s'"), "-fallbackfee", args.GetArg("-fallbackfee", ""))}); + return result; } else if (fallback_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-fallbackfee") + Untranslated(" ") + + result.AddWarning(AmountHighWarn("-fallbackfee") + Untranslated(" ") + _("This is the transaction fee you may pay when fee estimates are not available.")); } walletInstance->m_fallback_fee = CFeeRate{fallback_fee.value()}; @@ -3166,10 +3162,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (args.IsArgSet("-discardfee")) { std::optional discard_fee = ParseMoney(args.GetArg("-discardfee", "")); if (!discard_fee) { - error = strprintf(_("Invalid amount for %s=: '%s'"), "-discardfee", args.GetArg("-discardfee", "")); - return nullptr; + result.Update(util::Error{strprintf(_("Invalid amount for %s=: '%s'"), "-discardfee", args.GetArg("-discardfee", ""))}); + return result; } else if (discard_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-discardfee") + Untranslated(" ") + + result.AddWarning(AmountHighWarn("-discardfee") + Untranslated(" ") + _("This is the transaction fee you may discard if change is smaller than dust at this level")); } walletInstance->m_discard_rate = CFeeRate{discard_fee.value()}; @@ -3178,35 +3174,35 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (args.IsArgSet("-paytxfee")) { std::optional pay_tx_fee = ParseMoney(args.GetArg("-paytxfee", "")); if (!pay_tx_fee) { - error = AmountErrMsg("paytxfee", args.GetArg("-paytxfee", "")); - return nullptr; + result.Update(util::Error{AmountErrMsg("paytxfee", args.GetArg("-paytxfee", ""))}); + return result; } else if (pay_tx_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-paytxfee") + Untranslated(" ") + + result.AddWarning(AmountHighWarn("-paytxfee") + Untranslated(" ") + _("This is the transaction fee you will pay if you send a transaction.")); } walletInstance->m_pay_tx_fee = CFeeRate{pay_tx_fee.value(), 1000}; if (chain && walletInstance->m_pay_tx_fee < chain->relayMinFee()) { - error = strprintf(_("Invalid amount for %s=: '%s' (must be at least %s)"), - "-paytxfee", args.GetArg("-paytxfee", ""), chain->relayMinFee().ToString()); - return nullptr; + result.Update(util::Error{strprintf(_("Invalid amount for %s=: '%s' (must be at least %s)"), + "-paytxfee", args.GetArg("-paytxfee", ""), chain->relayMinFee().ToString())}); + return result; } } if (args.IsArgSet("-maxtxfee")) { std::optional max_fee = ParseMoney(args.GetArg("-maxtxfee", "")); if (!max_fee) { - error = AmountErrMsg("maxtxfee", args.GetArg("-maxtxfee", "")); - return nullptr; + result.Update(util::Error{AmountErrMsg("maxtxfee", args.GetArg("-maxtxfee", ""))}); + return result; } else if (max_fee.value() > HIGH_MAX_TX_FEE) { - warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee")); + result.AddWarning(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee")); } if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) { - error = strprintf(_("Invalid amount for %s=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), - "-maxtxfee", args.GetArg("-maxtxfee", ""), chain->relayMinFee().ToString()); - return nullptr; + result.Update(util::Error{strprintf(_("Invalid amount for %s=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), + "-maxtxfee", args.GetArg("-maxtxfee", ""), chain->relayMinFee().ToString())}); + return result; } walletInstance->m_default_max_tx_fee = max_fee.value(); @@ -3216,14 +3212,14 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri if (std::optional consolidate_feerate = ParseMoney(args.GetArg("-consolidatefeerate", ""))) { walletInstance->m_consolidate_feerate = CFeeRate(*consolidate_feerate); } else { - error = AmountErrMsg("consolidatefeerate", args.GetArg("-consolidatefeerate", "")); - return nullptr; + result.Update(util::Error{AmountErrMsg("consolidatefeerate", args.GetArg("-consolidatefeerate", ""))}); + return result; } } if (chain && chain->relayMinFee().GetFeePerK() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-minrelaytxfee") + Untranslated(" ") + - _("The wallet will avoid paying less than the minimum relay fee.")); + result.AddWarning(AmountHighWarn("-minrelaytxfee") + Untranslated(" ") + + _("The wallet will avoid paying less than the minimum relay fee.")); } walletInstance->m_confirm_target = args.GetIntArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); @@ -3243,9 +3239,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (time_first_key) walletInstance->MaybeUpdateBirthTime(*time_first_key); - if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { + if (chain && !(AttachChain(walletInstance, *chain, rescan_required) >> result)) { walletInstance->m_chain_notifications_handler.reset(); // Reset this pointer so that the wallet will actually be unloaded - return nullptr; + result.Update(util::Error{}); + return result; } { @@ -3256,10 +3253,11 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->WalletLogPrintf("m_address_book.size() = %u\n", walletInstance->m_address_book.size()); } - return walletInstance; + result.Update(std::move(walletInstance)); + return result; } -bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector& warnings) +util::Result CWallet::AttachChain(const std::shared_ptr& walletInstance, interfaces::Chain& chain, const bool rescan_required) { LOCK(walletInstance->cs_wallet); // allow setting the chain if it hasn't been set already but prevent changing it @@ -3274,8 +3272,7 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf // Wallet is assumed to be from another chain, if genesis block in the active // chain differs from the genesis block known to the wallet. if (chain.getBlockHash(0) != locator.vHave.back()) { - error = Untranslated("Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override."); - return false; + return {util::Error{Untranslated("Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override.")}}; } } } @@ -3350,15 +3347,14 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf // If a block is pruned after this check, we will load the wallet, // but fail the rescan with a generic error. - error = chain.havePruned() ? + return {util::Error{chain.havePruned() ? _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)") : strprintf(_( "Error loading wallet. Wallet requires blocks to be downloaded, " "and software does not currently support loading wallets while " "blocks are being downloaded out of order when using assumeutxo " "snapshots. Wallet should be able to load successfully after " - "node sync reaches height %s"), block_height); - return false; + "node sync reaches height %s"), block_height)}}; } } @@ -3368,8 +3364,7 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf { WalletRescanReserver reserver(*walletInstance); if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/true).status)) { - error = _("Failed to rescan the wallet during initialization"); - return false; + return {util::Error{_("Failed to rescan the wallet during initialization")}}; } } walletInstance->m_attaching_chain = false; @@ -3378,7 +3373,7 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf } walletInstance->m_attaching_chain = false; - return true; + return {}; } const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest, bool allow_change) const @@ -3998,15 +3993,17 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat return spk_man; } -bool CWallet::MigrateToSQLite(bilingual_str& error) +util::Result CWallet::MigrateToSQLite() { AssertLockHeld(cs_wallet); + util::Result result; + WalletLogPrintf("Migrating wallet storage database from BerkeleyDB to SQLite.\n"); if (m_database->Format() == "sqlite") { - error = _("Error: This wallet already uses SQLite"); - return false; + result.Update(util::Error{_("Error: This wallet already uses SQLite")}); + return result; } // Get all of the records for DB type migration @@ -4014,8 +4011,8 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) std::unique_ptr cursor = batch->GetNewCursor(); std::vector> records; if (!cursor) { - error = _("Error: Unable to begin reading all records in the database"); - return false; + result.Update(util::Error{_("Error: Unable to begin reading all records in the database")}); + return result; } DatabaseCursor::Status status = DatabaseCursor::Status::FAIL; while (true) { @@ -4032,8 +4029,8 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) cursor.reset(); batch.reset(); if (status != DatabaseCursor::Status::DONE) { - error = _("Error: Unable to read all records in the database"); - return false; + result.Update(util::Error{_("Error: Unable to read all records in the database")}); + return result; } // Close this database and delete the file @@ -4049,11 +4046,10 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) DatabaseOptions opts; opts.require_create = true; opts.require_format = DatabaseFormat::SQLITE; - DatabaseStatus db_status; - std::unique_ptr new_db = ResultExtract(MakeDatabase(wallet_path, opts), &db_status, &error); + auto new_db{MakeDatabase(wallet_path, opts) >> result}; assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists. m_database.reset(); - m_database = std::move(new_db); + m_database = std::move(new_db.value()); // Write existing records into the new DB batch = m_database->MakeBatch(); @@ -4069,26 +4065,24 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) } bool committed = batch->TxnCommit(); assert(committed); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution. - return true; + return result; } -std::optional CWallet::GetDescriptorsForLegacy(bilingual_str& error) const +util::Result CWallet::GetDescriptorsForLegacy() const { AssertLockHeld(cs_wallet); LegacyDataSPKM* legacy_spkm = GetLegacyDataSPKM(); if (!Assume(legacy_spkm)) { // This shouldn't happen - error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing")); - return std::nullopt; + return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))}; } std::optional res = legacy_spkm->MigrateToDescriptor(); if (res == std::nullopt) { - error = _("Error: Unable to produce descriptors for this legacy wallet. Make sure to provide the wallet's passphrase if it is encrypted."); - return std::nullopt; + return {util::Error{_("Error: Unable to produce descriptors for this legacy wallet. Make sure to provide the wallet's passphrase if it is encrypted.")}}; } - return res; + return std::move(*res); } util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, MigrationData& data) @@ -4292,13 +4286,17 @@ bool CWallet::CanGrindR() const return !IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); } -bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +util::Result DoMigration(CWallet& wallet, WalletContext& context, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); + util::Result result; // Get all of the descriptors from the legacy wallet - std::optional data = wallet.GetDescriptorsForLegacy(error); - if (data == std::nullopt) return false; + auto data{wallet.GetDescriptorsForLegacy() >> result}; + if (!data) { + result.Update(util::Error{}); + return result; + } // Create the watchonly and solvable wallets if necessary if (data->watch_descs.size() > 0 || data->solvable_descs.size() > 0) { @@ -4321,20 +4319,19 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, if (data->watch_descs.size() > 0) { wallet.WalletLogPrintf("Making a new watchonly wallet containing the watched scripts\n"); - DatabaseStatus status; - std::vector warnings; std::string wallet_name = wallet.GetName() + "_watchonly"; - std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); + auto database{MakeWalletDatabase(wallet_name, options) >> result}; if (!database) { - error = strprintf(_("Wallet file creation failed: %s"), error); - return false; + result.Update(util::Error{_("Wallet file creation failed")}); + return result; } - data->watchonly_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); - if (!data->watchonly_wallet) { - error = _("Error: Failed to create new watchonly wallet"); - return false; + auto watchonly_wallet{CWallet::Create(empty_context, wallet_name, std::move(database.value()), options.create_flags) >> result}; + if (!watchonly_wallet) { + result.Update(util::Error{_("Error: Failed to create new watchonly wallet")}); + return result; } + data->watchonly_wallet = std::move(watchonly_wallet.value()); res.watchonly_wallet = data->watchonly_wallet; LOCK(data->watchonly_wallet->cs_wallet); @@ -4353,25 +4350,24 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, } // Add the wallet to settings - UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings); + UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true) >> result; } if (data->solvable_descs.size() > 0) { wallet.WalletLogPrintf("Making a new watchonly wallet containing the unwatched solvable scripts\n"); - DatabaseStatus status; - std::vector warnings; std::string wallet_name = wallet.GetName() + "_solvables"; - std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); + auto database{MakeWalletDatabase(wallet_name, options) >> result}; if (!database) { - error = strprintf(_("Wallet file creation failed: %s"), error); - return false; + result.Update(util::Error{_("Wallet file creation failed")}); + return result; } - data->solvable_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); - if (!data->solvable_wallet) { - error = _("Error: Failed to create new watchonly wallet"); - return false; + auto solvable_wallet{CWallet::Create(empty_context, wallet_name, std::move(database.value()), options.create_flags) >> result}; + if (!solvable_wallet) { + result.Update(util::Error{_("Error: Failed to create new watchonly wallet")}); + return result; } + data->solvable_wallet = std::move(solvable_wallet.value()); res.solvables_wallet = data->solvable_wallet; LOCK(data->solvable_wallet->cs_wallet); @@ -4390,31 +4386,30 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, } // Add the wallet to settings - UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings); + UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true) >> result; } } // Add the descriptors to wallet, remove LegacyScriptPubKeyMan, and cleanup txs and address book data - return RunWithinTxn(wallet.GetDatabase(), /*process_desc=*/"apply migration process", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet){ - if (auto res_migration = wallet.ApplyMigrationData(batch, *data); !res_migration) { - error = util::ErrorString(res_migration); - return false; - } - wallet.WalletLogPrintf("Wallet migration complete.\n"); - return true; - }); + bool committed{RunWithinTxn(wallet.GetDatabase(), /*process_desc=*/"apply migration process", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet){ + result.Update(wallet.ApplyMigrationData(batch, *data)); + if (result) wallet.WalletLogPrintf("Wallet migration complete.\n"); + return bool{result}; + })}; + if (result && !committed) result.Update(util::Error{_("Error starting/committing db txn for wallet migration process")}); + return result; } util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context) { - std::vector warnings; - bilingual_str error; + util::Result result; // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it bool was_loaded = false; if (auto wallet = GetWallet(context, wallet_name)) { if (wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - return util::Error{_("Error: This wallet is already a descriptor wallet")}; + result.Update(util::Error{_("Error: This wallet is already a descriptor wallet")}); + return result; } // Flush chain state before unloading wallet @@ -4422,8 +4417,9 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle WITH_LOCK(wallet->cs_wallet, context.chain->findBlock(wallet->GetLastBlockHash(), FoundBlock().locator(locator))); if (!locator.IsNull()) wallet->chainStateFlushed(ChainstateRole::NORMAL, locator); - if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { - return util::Error{_("Unable to unload the wallet before migrating")}; + if (!(RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt) >> result)) { + result.Update(util::Error{_("Unable to unload the wallet before migrating")}); + return result; } WaitForDeleteWallet(std::move(wallet)); was_loaded = true; @@ -4448,30 +4444,32 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle DatabaseOptions options; options.require_existing = true; options.require_format = DatabaseFormat::BERKELEY_RO; - DatabaseStatus status; - std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); + auto database{MakeWalletDatabase(wallet_name, options) >> result}; if (!database) { - return util::Error{Untranslated("Wallet file verification failed.") + Untranslated(" ") + error}; + auto& errors{result.EnsureMessages().errors}; + errors.insert(errors.begin(), Untranslated("Wallet file verification failed.")); + result.Update(util::Error{}); + return result; } // Make the local wallet - std::shared_ptr local_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + auto local_wallet{CWallet::Create(empty_context, wallet_name, std::move(database.value()), options.create_flags) >> result}; if (!local_wallet) { - return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error}; + auto& errors{result.EnsureMessages().errors}; + errors.insert(errors.begin(), Untranslated("Wallet loading failed.")); + result.Update(util::Error{}); + return result; } - return MigrateLegacyToDescriptor(std::move(local_wallet), passphrase, context, was_loaded); + return MigrateLegacyToDescriptor(std::move(local_wallet.value()), passphrase, context, was_loaded); } util::Result MigrateLegacyToDescriptor(std::shared_ptr local_wallet, const SecureString& passphrase, WalletContext& context, bool was_loaded) { - MigrationResult res; - bilingual_str error; - std::vector warnings; + util::Result result; DatabaseOptions options; options.require_existing = true; - DatabaseStatus status; const std::string wallet_name = local_wallet->GetName(); @@ -4480,7 +4478,9 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr assert(to_reload.use_count() == 1); std::string name = to_reload->GetName(); to_reload.reset(); - to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + if (auto wallet{LoadWallet(context, name, /*load_on_start=*/std::nullopt, options) >> result}) { + to_reload = wallet.value(); + } return to_reload != nullptr; }; @@ -4489,7 +4489,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr if (was_loaded) { reload_wallet(local_wallet); } - return util::Error{_("Error: This wallet is already a descriptor wallet")}; + result.Update(util::Error{_("Error: This wallet is already a descriptor wallet")}); + return result; } // Make a backup of the DB @@ -4500,9 +4501,10 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr if (was_loaded) { reload_wallet(local_wallet); } - return util::Error{_("Error: Unable to make a backup of your wallet")}; + result.Update(util::Error{_("Error: Unable to make a backup of your wallet")}); + return result; } - res.backup_path = backup_path; + result->backup_path = backup_path; bool success = false; @@ -4512,25 +4514,29 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr reload_wallet(local_wallet); } if (passphrase.find('\0') == std::string::npos) { - return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}; + result.Update(util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}); } else { - return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase entered was incorrect. " + result.Update(util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase entered was incorrect. " "The passphrase contains a null character (ie - a zero byte). " "If this passphrase was set with a version of this software prior to 25.0, " "please try again with only the characters up to — but not including — " - "the first null character.")}; + "the first null character.")}); } + return result; } { LOCK(local_wallet->cs_wallet); // First change to using SQLite - if (!local_wallet->MigrateToSQLite(error)) return util::Error{error}; + if (!(local_wallet->MigrateToSQLite() >> result)) { + result.Update(util::Error{}); + return result; + } // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); if (!success) { - success = DoMigration(*local_wallet, context, error, res); + success = bool{DoMigration(*local_wallet, context, *result) >> result}; } else { // Make sure that descriptors flag is actually set local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -4547,17 +4553,17 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Reload the main wallet wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(local_wallet); - res.wallet = local_wallet; - res.wallet_name = wallet_name; - if (success && res.watchonly_wallet) { + result->wallet = local_wallet; + result->wallet_name = wallet_name; + if (success && result->watchonly_wallet) { // Reload watchonly - wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path()); - success = reload_wallet(res.watchonly_wallet); + wallet_dirs.insert(fs::PathFromString(result->watchonly_wallet->GetDatabase().Filename()).parent_path()); + success = reload_wallet(result->watchonly_wallet); } - if (success && res.solvables_wallet) { + if (success && result->solvables_wallet) { // Reload solvables - wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path()); - success = reload_wallet(res.solvables_wallet); + wallet_dirs.insert(fs::PathFromString(result->solvables_wallet->GetDatabase().Filename()).parent_path()); + success = reload_wallet(result->solvables_wallet); } } if (!success) { @@ -4569,8 +4575,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Make list of wallets to cleanup std::vector> created_wallets; if (local_wallet) created_wallets.push_back(std::move(local_wallet)); - if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet)); - if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet)); + if (result->watchonly_wallet) created_wallets.push_back(std::move(result->watchonly_wallet)); + if (result->solvables_wallet) created_wallets.push_back(std::move(result->solvables_wallet)); // Get the directories to remove after unloading for (std::shared_ptr& w : created_wallets) { @@ -4581,9 +4587,9 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr for (std::shared_ptr& w : created_wallets) { if (w->HaveChain()) { // Unloading for wallets that were loaded for normal use - if (!RemoveWallet(context, w, /*load_on_start=*/false)) { - error += _("\nUnable to cleanup failed migration"); - return util::Error{error}; + if (!(RemoveWallet(context, w, /*load_on_start=*/false) >> result)) { + result.Update(util::Error{_("\nUnable to cleanup failed migration")}); + return result; } WaitForDeleteWallet(std::move(w)); } else { @@ -4601,11 +4607,10 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Restore the backup // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory. // Reload it into memory if the wallet was previously loaded. - bilingual_str restore_error; - const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded); - if (!restore_error.empty()) { - error += restore_error + _("\nUnable to restore backup of wallet."); - return util::Error{error}; + auto ptr_wallet{RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, /*load_after_restore=*/was_loaded) >> result}; + if (!ptr_wallet) { + result.Update(util::Error{_("Unable to restore backup of wallet.")});; + return result; } // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir @@ -4614,12 +4619,12 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null. // This check is performed after restoration to avoid an early error before saving the backup. - bool wallet_reloaded = ptr_wallet != nullptr; + bool wallet_reloaded = ptr_wallet.value() != nullptr; assert(was_loaded == wallet_reloaded); - return util::Error{error}; + result.Update(util::Error{}); } - return res; + return result; } void CWallet::CacheNewScriptPubKeys(const std::set& spks, ScriptPubKeyMan* spkm) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c475718eb94..802120a8bc7 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -88,17 +88,16 @@ struct WalletContext; void WaitForDeleteWallet(std::shared_ptr&& wallet); bool AddWallet(WalletContext& context, const std::shared_ptr& wallet); -bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start, std::vector& warnings); -bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start); +util::Result RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start); std::vector> GetWallets(WalletContext& context); std::shared_ptr GetDefaultWallet(WalletContext& context, size_t& count); std::shared_ptr GetWallet(WalletContext& context, const std::string& name); -std::shared_ptr LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); -std::shared_ptr CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore = true); +util::ResultPtr, DatabaseStatus> LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options); +util::ResultPtr, DatabaseStatus> CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options); +util::ResultPtr, DatabaseStatus> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, bool load_after_restore = true); std::unique_ptr HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet); void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr& wallet); -std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseStatus> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options); //! -paytxfee default constexpr CAmount DEFAULT_PAY_TX_FEE = 0; @@ -433,7 +432,7 @@ private: * block locator and m_last_block_processed, and registering for * notifications about new blocks and transactions. */ - static bool AttachChain(const std::shared_ptr& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector& warnings); + static util::Result AttachChain(const std::shared_ptr& wallet, interfaces::Chain& chain, const bool rescan_required); static NodeClock::time_point GetDefaultNextResend(); @@ -877,7 +876,7 @@ public: bool MarkReplaced(const uint256& originalHash, const uint256& newHash); /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ - static std::shared_ptr Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings); + static util::ResultPtr> Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags); /** * Wallet post-init setup @@ -1046,10 +1045,10 @@ public: * A backup is not created. * May crash if something unexpected happens in the filesystem. */ - bool MigrateToSQLite(bilingual_str& error) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + util::Result MigrateToSQLite() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Get all of the descriptors from a legacy wallet - std::optional GetDescriptorsForLegacy(bilingual_str& error) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + util::Result GetDescriptorsForLegacy() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds the ScriptPubKeyMans given in MigrationData to this wallet, removes LegacyScriptPubKeyMan, //! and where needed, moves tx and address book entries to watchonly_wallet or solvable_wallet From 80acae33d89db5ccaf4b97c150859d0b1e466f1d Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 25 Jul 2022 17:40:49 -0400 Subject: [PATCH 18/23] refactor: Use util::Result class in wallet/load --- src/wallet/load.cpp | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 6cb14efe90a..4e0b8c65730 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -58,12 +58,10 @@ bool VerifyWallets(WalletContext& context) // wallets directory, include it in the default list of wallets to load. if (!args.IsArgSet("wallet")) { DatabaseOptions options; - DatabaseStatus status; ReadDatabaseArgs(args, options); - bilingual_str error_string; options.require_existing = true; options.verify = false; - if (ResultExtract(MakeWalletDatabase("", options), &status, &error_string)) { + if (MakeWalletDatabase("", options)) { common::SettingsValue wallets(common::SettingsValue::VARR); wallets.push_back(""); // Default wallet name is "" // Pass write=false because no need to write file and probably @@ -91,16 +89,15 @@ bool VerifyWallets(WalletContext& context) } DatabaseOptions options; - DatabaseStatus status; ReadDatabaseArgs(args, options); options.require_existing = true; options.verify = true; - bilingual_str error_string; - if (!ResultExtract(MakeWalletDatabase(wallet_file, options), &status, &error_string)) { - if (status == DatabaseStatus::FAILED_NOT_FOUND) { - chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", error_string.original))); + auto result{MakeWalletDatabase(wallet_file, options)}; + if (!result) { + if (result.GetFailure() == DatabaseStatus::FAILED_NOT_FOUND) { + chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", util::ErrorString(result).original))); } else { - chain.initError(error_string); + chain.initError(util::ErrorString(result)); return false; } } @@ -125,26 +122,24 @@ bool LoadWallets(WalletContext& context) continue; } DatabaseOptions options; - DatabaseStatus status; ReadDatabaseArgs(*context.args, options); options.require_existing = true; options.verify = false; // No need to verify, assuming verified earlier in VerifyWallets() - bilingual_str error; - std::vector warnings; - std::unique_ptr database{ResultExtract(MakeWalletDatabase(name, options), &status, &error)}; - if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) { + util::Result result; + auto database{MakeWalletDatabase(name, options) >> result}; + if (!database && database.GetFailure() == DatabaseStatus::FAILED_NOT_FOUND) { continue; } chain.initMessage(_("Loading wallet…")); - std::shared_ptr pwallet{database ? ResultExtract(CWallet::Create(context, name, std::move(database), options.create_flags), nullptr, &error, &warnings) : nullptr}; - if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); + auto pwallet{database ? CWallet::Create(context, name, std::move(database.value()), options.create_flags) >> result : nullptr}; + if (result.GetMessages() && !result.GetMessages()->warnings.empty()) chain.initWarning(Join(result.GetMessages()->warnings, Untranslated("\n"))); if (!pwallet) { - chain.initError(error); + chain.initError(Join(result.GetMessages()->errors, Untranslated("\n"))); return false; } - NotifyWalletLoaded(context, pwallet); - AddWallet(context, pwallet); + NotifyWalletLoaded(context, pwallet.value()); + AddWallet(context, pwallet.value()); } return true; } catch (const std::runtime_error& e) { @@ -186,8 +181,8 @@ void UnloadWallets(WalletContext& context) while (!wallets.empty()) { auto wallet = wallets.back(); wallets.pop_back(); - std::vector warnings; - ResultExtract(RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt), nullptr, nullptr, &warnings); + // Note: Warnings returned from RemoveWallet are silently discarded. + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); WaitForDeleteWallet(std::move(wallet)); } } From 4a2e578df0d6c35580256376f7636f5d9c0248ad Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 1 May 2024 12:20:59 -0400 Subject: [PATCH 19/23] refactor: Use util::Result class in wallet/rpc --- src/wallet/rpc/backup.cpp | 12 +++++------- src/wallet/rpc/util.cpp | 6 +++--- src/wallet/rpc/util.h | 3 ++- src/wallet/rpc/wallet.cpp | 40 +++++++++++++++++++-------------------- 4 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 31919127eff..21077b5c908 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1963,17 +1963,15 @@ RPCHelpMan restorewallet() std::optional load_on_start = request.params[2].isNull() ? std::nullopt : std::optional(request.params[2].get_bool()); - DatabaseStatus status; - bilingual_str error; - std::vector warnings; + auto wallet{RestoreWallet(context, backup_file, wallet_name, load_on_start)}; - const std::shared_ptr wallet{ResultExtract(RestoreWallet(context, backup_file, wallet_name, load_on_start), &status, &error, &warnings)}; - - HandleWalletError(wallet, status, error); + HandleWalletError(wallet); UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); - PushWarnings(warnings, obj); + if (wallet.GetMessages()) { + PushWarnings(wallet.GetMessages()->warnings, obj); + } return obj; diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 414f0deeb2d..62ba9c2d1b0 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -136,13 +136,13 @@ void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, entry.pushKV("parent_descs", std::move(parent_descs)); } -void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& status, bilingual_str& error) +void HandleWalletError(const util::ResultPtr, DatabaseStatus>& wallet) { if (!wallet) { // Map bad format to not found, since bad format is returned when the // wallet directory exists, but doesn't contain a data file. RPCErrorCode code = RPC_WALLET_ERROR; - switch (status) { + switch (wallet.GetFailure()) { case DatabaseStatus::FAILED_NOT_FOUND: case DatabaseStatus::FAILED_BAD_FORMAT: code = RPC_WALLET_NOT_FOUND; @@ -159,7 +159,7 @@ void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& st default: // RPC_WALLET_ERROR is returned for all other cases. break; } - throw JSONRPCError(code, error.original); + throw JSONRPCError(code, util::ErrorString(wallet).original); } } diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h index 002d0355e50..cc7df8f83b2 100644 --- a/src/wallet/rpc/util.h +++ b/src/wallet/rpc/util.h @@ -7,6 +7,7 @@ #include #include