refactor: Use util::Result class for wallet loading

ryanofsky commented at 7:26 pm on July 27, 2022: contributor

This is based on #25665 + #26022. The non-base commits are:

Wallet loading functions up and down the stack have lots of error and warning parameters, and return error information in different ways. This PR makes them uniformly return util::Result, without changing behavior.

DrahtBot added the label Refactoring on Jul 27, 2022

DrahtBot commented at 8:53 pm on July 27, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/25722.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/807 (refactor: interfaces, make ‘createTransaction’ less error-prone by furszy)
#31519 (refactor: Use std::span over Span by maflcko)
#31483 (kernel: Move kernel-related cache constants to kernel cache by TheCharlatan)
#31451 (wallet: migration, avoid loading legacy wallet after failure when BDB isn’t compiled by furszy)
#31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#31241 (wallet: remove BDB dependency from wallet migration benchmark by furszy)
#31061 (refactor: Check translatable format strings at compile-time by maflcko)
#30965 (kernel: Move block tree db open to block manager by TheCharlatan)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#29694 (fuzz: wallet: add target for spkm migration by brunoerg)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Aug 5, 2022

ryanofsky force-pushed on Aug 5, 2022

DrahtBot removed the label Needs rebase on Aug 5, 2022

DrahtBot added the label Needs rebase on Aug 10, 2022

ryanofsky force-pushed on Aug 15, 2022

ryanofsky marked this as ready for review on Aug 15, 2022

ryanofsky commented at 6:19 pm on August 15, 2022: contributor

Rebased a09c21c05a9adf0ca5a4b6a15021b45c6c2e0b2a -> e7457d0acefeccd636c134d0c10567ffaade2fc7 (pr/bresult-load.2 -> pr/bresult-load.3, compare) due to conflict with #25616 Rebased e7457d0acefeccd636c134d0c10567ffaade2fc7 -> 874003fcea81aba1276b53d5ad9c5dadefddd5ff (pr/bresult-load.3 -> pr/bresult-load.4, compare) due to conflict with #25504 Rebased 874003fcea81aba1276b53d5ad9c5dadefddd5ff -> d020d0cce052bc7b4141347a51d34fd85f8a10a1 (pr/bresult-load.4 -> pr/bresult-load.5, compare) on top of newer base PR #25665 tag r/bresult2.18 for derived-to-base conversion bugfix Rebased d020d0cce052bc7b4141347a51d34fd85f8a10a1 -> f942d9253e39df6fedf7f3fe4b0bdb2921206de0 (pr/bresult-load.5 -> pr/bresult-load.6, compare) due to silent conflict with #19602 Rebased f942d9253e39df6fedf7f3fe4b0bdb2921206de0 -> 4e533926fe5549aafb70caf94718447ea497c84c (pr/bresult-load.6 -> pr/bresult-load.7, compare) on top of newer version of #25665 (pr/bresult2.21) Rebased 4e533926fe5549aafb70caf94718447ea497c84c -> f3d165866329ec16e39a7289bd9769ad209cd56a (pr/bresult-load.7 -> pr/bresult-load.8, compare) due to conflict with #26021 Rebased f3d165866329ec16e39a7289bd9769ad209cd56a -> f718aec8db4d92037cf21bde2f92c72ed39c1d09 (pr/bresult-load.8 -> pr/bresult-load.9, compare) due to conflict with #26005 Rebased f718aec8db4d92037cf21bde2f92c72ed39c1d09 -> 7bd578a5caf91f73ee9127cd621e2838d5a3b427 (pr/bresult-load.9 -> pr/bresult-load.10, compare) due to conflicts with #25667, #26198, and #26282 Rebased 7bd578a5caf91f73ee9127cd621e2838d5a3b427 -> 2c675f03f017125390f3b578ee6169870deae985 (pr/bresult-load.10 -> pr/bresult-load.11, compare) due to conflicts with #26238, #26462, #26594, #26618, #26758 Rebased 2c675f03f017125390f3b578ee6169870deae985 -> 2e12b25fae4bca7539c3cf7ccb9e12e03c9e517d (pr/bresult-load.11 -> pr/bresult-load.12, compare) on top of pr/bresult2.28 Rebased 2e12b25fae4bca7539c3cf7ccb9e12e03c9e517d -> 5c1be5339b05f5923b8843fe3caa183044454608 (pr/bresult-load.12 -> pr/bresult-load.13, compare) due to conflicts with #26595 and #26032 Rebased 5c1be5339b05f5923b8843fe3caa183044454608 -> 9c2817208cd5e0a521e765b43d71bb78afc5a7f9 (pr/bresult-load.13 -> pr/bresult-load.14, compare) due to conflicts with #25666 and #24845 Rebased 9c2817208cd5e0a521e765b43d71bb78afc5a7f9 -> 2a9db070ec5acfe6843e44e1f8476f86899a8e29 (pr/bresult-load.14 -> pr/bresult-load.15, compare) due to conflict with #27279

DrahtBot removed the label Needs rebase on Aug 15, 2022

DrahtBot added the label Needs rebase on Aug 16, 2022

ryanofsky referenced this in commit 834857e56b on Aug 30, 2022

ryanofsky referenced this in commit 82c549aa53 on Sep 1, 2022

ryanofsky force-pushed on Sep 1, 2022

DrahtBot removed the label Needs rebase on Sep 1, 2022

ryanofsky referenced this in commit c14e904f66 on Sep 1, 2022

ryanofsky force-pushed on Sep 1, 2022

ryanofsky force-pushed on Sep 2, 2022

ryanofsky referenced this in commit a8973ae714 on Sep 6, 2022

ryanofsky referenced this in commit 05a97d3208 on Sep 13, 2022

ryanofsky referenced this in commit e04d8a754f on Sep 13, 2022

ryanofsky referenced this in commit 8e12547694 on Sep 13, 2022

DrahtBot added the label Needs rebase on Sep 13, 2022

ryanofsky referenced this in commit 52a4e50fb4 on Sep 13, 2022

ryanofsky force-pushed on Sep 14, 2022

ryanofsky referenced this in commit f9accbc6e2 on Sep 15, 2022

ryanofsky referenced this in commit 776d9b3fbb on Sep 15, 2022

ryanofsky force-pushed on Sep 15, 2022

DrahtBot removed the label Needs rebase on Sep 15, 2022

DrahtBot added the label Needs rebase on Sep 16, 2022

ryanofsky referenced this in commit f05441a572 on Sep 20, 2022

ryanofsky referenced this in commit 456e3d4ecc on Sep 20, 2022

ryanofsky force-pushed on Sep 20, 2022

DrahtBot removed the label Needs rebase on Sep 20, 2022

ryanofsky referenced this in commit 8ba9cb26a1 on Sep 20, 2022

ryanofsky referenced this in commit 8ebf0cd171 on Sep 20, 2022

DrahtBot added the label Needs rebase on Oct 3, 2022

ryanofsky referenced this in commit fee8670c7c on Oct 14, 2022

ryanofsky referenced this in commit 28a6934da9 on Oct 14, 2022

ryanofsky force-pushed on Oct 14, 2022

DrahtBot removed the label Needs rebase on Oct 14, 2022

DrahtBot added the label Needs rebase on Dec 1, 2022

ryanofsky referenced this in commit f4d55d858d on Jan 6, 2023

ryanofsky referenced this in commit 979dbd5c73 on Jan 6, 2023

ryanofsky force-pushed on Jan 20, 2023

DrahtBot removed the label Needs rebase on Jan 20, 2023

DrahtBot added the label Needs rebase on Jan 23, 2023

ryanofsky referenced this in commit b57a898a98 on Feb 10, 2023

ryanofsky referenced this in commit 513adde96f on Feb 10, 2023

ryanofsky referenced this in commit eb50fcd685 on Feb 10, 2023

ryanofsky force-pushed on Feb 11, 2023

DrahtBot removed the label Needs rebase on Feb 11, 2023

ryanofsky referenced this in commit 501ef88b94 on Feb 16, 2023

DrahtBot added the label Needs rebase on Feb 17, 2023

ryanofsky referenced this in commit d785176df3 on Mar 1, 2023

ryanofsky referenced this in commit 6056f7b0da on Mar 1, 2023

ryanofsky force-pushed on Mar 1, 2023

DrahtBot removed the label Needs rebase on Mar 1, 2023

DrahtBot added the label Needs rebase on Mar 8, 2023

ryanofsky referenced this in commit e766928ed1 on Mar 16, 2023

ryanofsky referenced this in commit 6afc0431f0 on Mar 16, 2023

ryanofsky referenced this in commit 6d7d7bbb7b on Apr 4, 2023

ryanofsky referenced this in commit 460ef0a18b on Apr 4, 2023

ryanofsky force-pushed on Apr 7, 2023

DrahtBot removed the label Needs rebase on Apr 7, 2023

DrahtBot added the label Needs rebase on Apr 12, 2023

ryanofsky force-pushed on Apr 13, 2023

DrahtBot removed the label Needs rebase on Apr 13, 2023

in src/util/result.h:62 in 2a9db070ec outdated

58+    }
59+
60+    //! Value setter methods that do nothing because this class has value type T=void.
61+    void ConstructValue() {}
62+    template <typename O>
63+    void MoveValue(O& other) {}

martinus commented at 8:04 am on April 16, 2023:

Maybe it would be a bit clearer to use (O&& other) here?

ryanofsky commented at 7:40 pm on May 2, 2023:

re: #25722 (review)

Maybe it would be a bit clearer to use (O&& other) here?

Sure, changed this.

in src/util/result.h:184 in 2a9db070ec outdated

183+        this->MoveMessages(other);
184+        Construct(fn, std::forward<Args>(args)...);
185+    }
186+
187+    template <typename OT, typename OF>
188+    void MoveConstruct(Result<OT, OF>& other)

martinus commented at 8:04 am on April 16, 2023:

Same here, I’d say void MoveConstruct(Result<OT, OF>&& other) is a bit clearer

ryanofsky commented at 7:41 pm on May 2, 2023:

re: #25722 (review)

Same here, I’d say void MoveConstruct(Result<OT, OF>&& other) is a bit clearer

Sure, changed

in src/util/result.h:237 in 2a9db070ec outdated

73+    const std::vector<bilingual_str>& GetWarnings() const LIFETIMEBOUND { return m_info ? m_info->warnings : EMPTY_LIST; }
74+};
75+
76+//! Result base class for T value type. Holds value and provides accessor methods.
77+template <typename T, typename F>
78+class ResultBase : public ResultBase<void, F>

martinus commented at 8:06 am on April 16, 2023:

Ha, nice trick; I’ve never seen such an inheritance before :-)

in src/util/result.h:154 in 2a9db070ec outdated

153+class Result : public detail::ResultBase<T, F>
154 {
155-private:
156-    std::variant<bilingual_str, T> m_variant;
157+protected:
158+    template <typename Fn, typename... Args>

martinus commented at 10:48 am on April 16, 2023:

It took me a while to figure out what’s going on here with the recursive Construct calls and the lambda that’s passed along. How about instead of passing a lambda along use a bool template argument like so:

 0diff --git a/src/util/result.h b/src/util/result.h
 1index 5599da43dd..3c163cf398 100644
 2--- a/src/util/result.h
 3+++ b/src/util/result.h
 4@@ -151,33 +151,35 @@ template <typename T, typename F = void>
 5 class Result : public detail::ResultBase<T, F>
 6 {
 7 protected:
 8-    template <typename Fn, typename... Args>
 9-    void Construct(const Fn& fn, Args&&... args)
10+    template <bool IsValueConstruction, typename... Args>
11+    void Construct(Args&&... args)
12     {
13-        fn(std::forward<Args>(args)...);
14+        if constexpr (IsValueConstruction) {
15+            this->ConstructValue(std::forward<Args>(args)...);
16+        } else {
17+            this->Info().failure.emplace(std::forward<Args>(args)...);
18+        }
19     }
20 
21-    template <typename Fn, typename... Args>
22-    void Construct(const Fn& fn, Error error, Args&&... args)
23+    template <bool IsValueConstruction, typename... Args>
24+    void Construct(Error error, Args&&... args)
25     {
26         this->AddError(std::move(error.message));
27-        Construct([&](auto&&... x) {
28-            this->Info().failure.emplace(std::forward<decltype(x)>(x)...);
29-        }, std::forward<Args>(args)...);
30+        Construct<false /*always false from here on*/>(std::forward<Args>(args)...);
31     }
32 
33-    template <typename Fn, typename... Args>
34-    void Construct(const Fn& fn, Warning warning, Args&&... args)
35+    template <bool IsValueConstruction, typename... Args>
36+    void Construct(Warning warning, Args&&... args)
37     {
38         this->AddWarning(std::move(warning.message));
39-        Construct(fn, std::forward<Args>(args)...);
40+        Construct<IsValueConstruction>(std::forward<Args>(args)...);
41     }
42 
43-    template <typename Fn, typename OT, typename OF, typename... Args>
44-    void Construct(const Fn& fn, Result<OT, OF>&& other, Args&&... args)
45+    template <bool IsValueConstruction, typename OT, typename OF, typename... Args>
46+    void Construct(Result<OT, OF>&& other, Args&&... args)
47     {
48         this->MoveMessages(other);
49-        Construct(fn, std::forward<Args>(args)...);
50+        Construct<IsValueConstruction>(std::forward<Args>(args)...);
51     }
52 
53     template <typename OT, typename OF>
54@@ -198,7 +200,8 @@ public:
55     template <typename... Args>
56     Result(Args&&... args)
57     {
58-        Construct([this](auto&&... x) { this->ConstructValue(std::forward<decltype(x)>(x)...); }, std::forward<Args>(args)...);
59+        // recursively peels of the first argument until we reach the base Construct method
60+        Construct<true>(std::forward<Args>(args)...);
61     }
62 
63     template <typename OT, typename OF>

ryanofsky commented at 7:40 pm on May 2, 2023:

re: #25722 (review)

It took me a while to figure out what’s going on here with the recursive Construct calls and the lambda that’s passed along. How about instead of passing a lambda along use a bool template argument like so:

Thanks, that’s much better! Took suggestion jsut renaming argument to “Failure”

in src/util/result.h:230 in 2a9db070ec outdated

214-    bool has_value() const noexcept { return m_variant.index() == 1; }
215-    const T& value() const LIFETIMEBOUND
216+    template <typename OT, typename OF>
217+    Result(Result<OT, OF>&& other) { MoveConstruct(other); }
218+
219+    Result& Set(Result&& other) LIFETIMEBOUND

martinus commented at 5:28 pm on April 16, 2023:

Mayby explicitly mark the move-assignment as delete

0template <typename OT, typename OF>
1Result& operator=(Result<OT, OF>&& other) = delete;

Then the compiler error messages are a bit nicer when one tries to move-assign

ryanofsky commented at 7:41 pm on May 2, 2023:

re: #25722 (review)

Mayby explicitly mark the move-assignment as delete

Thanks, that changes error message from error: object of type 'util::Result<int>' cannot be assigned because its copy assignment operator is implicitly deleted to error: overload resolution selected deleted operator=, which should be more helpful now because a comment at the deleted operator= declaration explains why the operator is deleted and recommends using Set() instead

in src/util/result.h:264 in 2a9db070ec outdated

250+    template<typename S>
251+    void MoveMessages(S& src)
252     {
253-        return has_value() ? std::move(value()) : std::forward<U>(default_value);
254+        if (src.m_info) {
255+            if (!src.m_info->errors.empty()) detail::MoveMessages(src.m_info->errors, this->Info().errors);

martinus commented at 5:33 pm on April 16, 2023:

nit: I don’t think the if (!src.m_info->errors.empty()) check is necessary.

ryanofsky commented at 7:41 pm on May 2, 2023:

re: #25722 (review)

nit: I don’t think the if (!src.m_info->errors.empty()) check is necessary.

It’s not strictly necessary, but it avoids allocating an ErrorInfo object in the common case where there are no errors or warnings. Since this is called whenever result objects are moved, it’s good to avoid unnecessary allocations here.

in src/util/result.h:161 in 2a9db070ec outdated

147+//! types of errors, and can suffice just using the type `T` success value on
148+//! success, and descriptive error strings when there's a failure. But
149+//! applications that do need more complicated error-handling behavior can
150+//! override the default `F = void` failure type and get failure values by
151+//! calling result.GetFailure().
152+template <typename T, typename F = void>

martinus commented at 5:39 am on April 22, 2023:

maybe consider forbidding type bool for T. I recently had similar code in another project, and its utterly confusing; because Result can convert to bool but that only tells if it has a bool or not. It’s very error prone.

TheCharlatan commented at 12:09 pm on June 19, 2023:

I guess the same goes for something like Result<std::optional<int>, int>, or really any type implementing operator bool()? How did you handle these cases in your code? Sometimes the non-presence of a result is not really an error.

ryanofsky commented at 6:18 pm on July 20, 2023:

re: #25722 (review)

I agree with all of this and want to do more work to address it.

I think by default types convertible to bool should be disallowed with Result. I think Result<bool> or Result<optional<T>> types are inherently ambiguous and code accessing those return values would be fragile and prone to errors from dereferencing incorrectly.

But disallowing all types convertible to bool would go to far because being able to return types like unique_ptr and shared_ptr is important. Returning types like Result<int> should probably fine in practice too, even though ints are convertible to bool, and there could be a little ambiguity.

So there is some fine-tuning to be done here, and I want to code to be organized so Result and ResultPtr (#26022) classes handle this safety stuff, and ResultBase class handles all the real result functionality.

in src/test/result_tests.cpp:213 in 3a919e576f outdated

155+    ExpectSuccess(TruthyFalsyFn(0, true), {}, 0);
156+    ExpectFail(TruthyFalsyFn(0, false), Untranslated("failure value 0."), 0);
157+    ExpectSuccess(TruthyFalsyFn(1, true), {}, 1);
158+    ExpectFail(TruthyFalsyFn(1, false), Untranslated("failure value 1."), 1);
159+}
160+

martinus commented at 5:45 am on April 22, 2023:

I’d add a check somewhere to ensure that the size of the Result is as claimed in the commit message, something like that:

0static_assert(sizeof(util::Result<int>) == sizeof(void*)*2);
1static_assert(sizeof(util::Result<void>) == sizeof(void*));

ryanofsky commented at 7:40 pm on May 2, 2023:

I’d add a check somewhere to ensure that the size of the Result is as claimed in the commit message, something like that:
0static_assert(sizeof(util::Result<int>) == sizeof(void*)*2);
1static_assert(sizeof(util::Result<void>) == sizeof(void*));

Thanks, added to result_tests.cpp

in src/util/result.h:205 in 3a919e576f outdated

196     }
197-    template <class U>
198-    T value_or(U&& default_value) &&
199+
200+    template <typename OT, typename OF>
201+    Result(Result<OT, OF>&& other) { MoveConstruct(other); }

martinus commented at 1:22 pm on April 22, 2023:

move constructors should usually be noexcept, but then all methods it uses also have to be noexcept.

ryanofsky commented at 7:41 pm on May 2, 2023:

re: #25722 (review)

move constructors should usually be noexcept, but then all methods it uses also have to be noexcept.

It would be possible to add a constructor that move constructs Result<T, F> from Result<OT, OF> and is noexcept. For this to work, move constructing T from OT would have to be noexcept, and F and OF types would need to be identical so unique_ptr<ErrorInfo<F>> could be moved without any allocations.

Right now I’m just trying to do the simplest thing that’s correct, and I think probably in most cases where the result class is used, having the move constructor be marked noexcept isn’t going impact performance. But it is something that could be optimized later.

in src/wallet/spend.cpp:736 in 3a919e576f outdated

743-        if (!res_detailed_errors.empty()) return res_detailed_errors.front();
744+        if (!res_detailed_errors.empty()) return std::move(res_detailed_errors.front());
745 
746 
747         // General "Insufficient Funds"
748         return util::Result<SelectionResult>(util::Error());

martinus commented at 1:28 pm on April 22, 2023:

Could be just return util::Error();

ryanofsky commented at 7:42 pm on May 2, 2023:

re: #25722 (review)

Could be just return util::Error();

Thanks, simplified

martinus commented at 1:37 pm on April 22, 2023: contributor

After rebasing I get a compile error in spend.cpp ChooseSelectionResult, adding a bunch of std::move helps

in src/wallet/spend.cpp:710 in 3a919e576f outdated

714@@ -715,7 +715,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
715         if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select) {
716             // Special case, too-long-mempool cluster.
717             if (total_amount + total_unconf_long_chain > value_to_select) {
718-                return util::Result<SelectionResult>({_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")});
719+                return util::Error{_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")};
720             }
721             return util::Result<SelectionResult>(util::Error()); // General "Insufficient Funds"

martinus commented at 2:22 pm on April 22, 2023:

could be just return util::Error();

ryanofsky commented at 7:42 pm on May 2, 2023:

re: #25722 (review)

could be just return util::Error();

Thanks, simplified

martinus changes_requested

martinus commented at 2:25 pm on April 22, 2023: contributor

Code review 2a9db07, apart from the compile error when rebasing I found mostly nits. This makes util::Result quite a bit more powerful, and it’s still simple to use. The code itself of util::Result is a bit hard to understand though, but I don’t have a better way either.

DrahtBot added the label CI failed on Apr 23, 2023

DrahtBot added the label Needs rebase on May 2, 2023

ryanofsky referenced this in commit aa73bee496 on May 2, 2023

ryanofsky referenced this in commit 28a954c703 on May 2, 2023

ryanofsky force-pushed on May 3, 2023

DrahtBot removed the label Needs rebase on May 3, 2023

DrahtBot removed the label CI failed on May 3, 2023

DrahtBot added the label Needs rebase on May 15, 2023

ryanofsky referenced this in commit cdf11c66c0 on Jul 21, 2023

ryanofsky referenced this in commit 40f09de73e on Jul 21, 2023

ryanofsky force-pushed on Jul 21, 2023

ryanofsky commented at 5:03 pm on July 21, 2023: contributor

Rebased f7d4451b98014176c083adc02bdf59e6dd2b9355 -> 17891a95ab5873aa978a5bf5ed8985ee1513e728 (pr/bresult-load.16 -> pr/bresult-load.17, compare making most of the suggested changes. I’m working on some more changes that should use ResultPtr from #26022 to simplify some of the code that has changed and made it safer. It will take more work to make ResultPtr compatible, and relatedly change the Result class to discourage using it with confusing types that are convertible to bool.

DrahtBot removed the label Needs rebase on Jul 21, 2023

ryanofsky referenced this in commit 4d995d3fa6 on Jul 21, 2023

ryanofsky referenced this in commit 775b54e881 on Jul 21, 2023

DrahtBot added the label CI failed on Jul 21, 2023

maflcko commented at 2:55 pm on August 1, 2023: member

Maybe mark as draft for as long as CI is red and this is based on something else?

ryanofsky marked this as a draft on Aug 1, 2023

ryanofsky referenced this in commit cf01e29b9d on Aug 1, 2023

ryanofsky referenced this in commit 1de05ef919 on Aug 1, 2023

ryanofsky referenced this in commit 6ddcc9bec8 on Aug 1, 2023

ryanofsky referenced this in commit 08f5febfc5 on Aug 1, 2023

ryanofsky force-pushed on Aug 2, 2023

DrahtBot removed the label CI failed on Aug 2, 2023

ryanofsky referenced this in commit 9cea9e8e38 on Aug 3, 2023

ryanofsky referenced this in commit b0beb4c504 on Aug 3, 2023

ryanofsky referenced this in commit 835f09452b on Aug 4, 2023

ryanofsky referenced this in commit 9e80d0b754 on Aug 4, 2023

ryanofsky referenced this in commit b21e2b067e on Aug 7, 2023

ryanofsky referenced this in commit 7f883b33bb on Aug 7, 2023

DrahtBot added the label Needs rebase on Aug 24, 2023

ryanofsky referenced this in commit 5529ea16ad on Sep 6, 2023

ryanofsky referenced this in commit a21ac50c2b on Sep 6, 2023

ryanofsky referenced this in commit ed61b012b1 on Sep 6, 2023

ryanofsky referenced this in commit 956bec1eca on Sep 6, 2023

ryanofsky force-pushed on Sep 25, 2023

DrahtBot removed the label Needs rebase on Sep 25, 2023

DrahtBot added the label CI failed on Oct 5, 2023

DrahtBot added the label Needs rebase on Oct 16, 2023

ryanofsky referenced this in commit 256a16e999 on Oct 18, 2023

ryanofsky referenced this in commit 29f6cfdabe on Oct 18, 2023

ryanofsky referenced this in commit 1f6190ac9b on Oct 18, 2023

ryanofsky referenced this in commit 24999606f8 on Oct 18, 2023

ryanofsky referenced this in commit e2b184ec86 on Oct 26, 2023

ryanofsky referenced this in commit f158686962 on Oct 26, 2023

ryanofsky referenced this in commit 2e1cc4f55f on Oct 26, 2023

ryanofsky referenced this in commit 3fe48360c3 on Oct 26, 2023

ryanofsky referenced this in commit 1c7d8bea4f on Jan 24, 2024

ryanofsky referenced this in commit f822ac9a24 on Jan 24, 2024

ryanofsky referenced this in commit 774240a82b on Feb 21, 2024

ryanofsky referenced this in commit 4ec6b060a8 on Feb 21, 2024

ryanofsky referenced this in commit 2c78cce3c5 on Feb 21, 2024

ryanofsky referenced this in commit f27418f37e on Feb 21, 2024

ryanofsky referenced this in commit b0687b7e1e on Feb 21, 2024

ryanofsky referenced this in commit 2055659814 on Feb 21, 2024

ryanofsky referenced this in commit b91edfa4f1 on Feb 21, 2024

ryanofsky force-pushed on Feb 21, 2024

ryanofsky commented at 11:39 pm on February 21, 2024: contributor

Rebased 1b2a5f12b42543d13a4bcafb9585e3d1df488914 -> 6700299bcb6f560e9e4caf34254d7d86d4e78833 (pr/bresult-load.19 -> pr/bresult-load.20, compare) on top of #26022 using #26022 to simplify some changes in this PR

DrahtBot removed the label CI failed on Feb 22, 2024

DrahtBot removed the label Needs rebase on Feb 22, 2024

ryanofsky referenced this in commit 16b1f910be on Feb 22, 2024

ryanofsky referenced this in commit cdf7bb1756 on Feb 22, 2024

ryanofsky force-pushed on Feb 22, 2024

ryanofsky commented at 8:14 pm on February 22, 2024: contributor

Updated 6700299bcb6f560e9e4caf34254d7d86d4e78833 -> 2385cb37dbbe00004f8bffe1d1d784778d89a21e (pr/bresult-load.20 -> pr/bresult-load.21, compare) splitting this up from one big to commit to multiple commits so it is reasonable to review.

However, looking over the commits, this still belongs in draft state, because since the time this PR was opened, a lot of improvements were made to the util::Result class in base PR #25665 based on review feedback, so there are a lot of updates that can be made in this PR to make it nicer.

TheCharlatan referenced this in commit 50220e34f6 on Mar 10, 2024

TheCharlatan referenced this in commit 539f101dfe on Mar 10, 2024

TheCharlatan referenced this in commit fe260d92a3 on Mar 11, 2024

TheCharlatan referenced this in commit 5029e59b9d on Mar 11, 2024

TheCharlatan referenced this in commit f3dbbbc3d5 on Mar 12, 2024

TheCharlatan referenced this in commit 63b92a8c80 on Mar 12, 2024

TheCharlatan referenced this in commit a72ae6a43f on Mar 12, 2024

TheCharlatan referenced this in commit 76fe2f8d41 on Mar 12, 2024

TheCharlatan referenced this in commit 2331edb22e on Mar 12, 2024

TheCharlatan referenced this in commit 5797f3cb7c on Mar 12, 2024

TheCharlatan referenced this in commit 31ae72fdfa on Mar 15, 2024

TheCharlatan referenced this in commit 6b7813c256 on Mar 15, 2024

TheCharlatan referenced this in commit f6118b724c on Mar 19, 2024

TheCharlatan referenced this in commit ee80f88bcb on Mar 19, 2024

ryanofsky referenced this in commit 40b009dfee on Mar 22, 2024

ryanofsky referenced this in commit e0cbb8862c on Mar 22, 2024

ryanofsky referenced this in commit d52c2e949c on Mar 26, 2024

ryanofsky referenced this in commit efb463788f on Mar 26, 2024

TheCharlatan referenced this in commit c7d4f43198 on Mar 26, 2024

TheCharlatan referenced this in commit 5e38b42b1e on Mar 26, 2024

ryanofsky referenced this in commit 88821fe2a0 on Mar 26, 2024

ryanofsky referenced this in commit 110bccc371 on Mar 26, 2024

ryanofsky referenced this in commit d859ea511f on Mar 26, 2024

ryanofsky referenced this in commit 6adeafab68 on Mar 26, 2024

ryanofsky referenced this in commit 3eb47b4641 on Mar 27, 2024

ryanofsky referenced this in commit d4579e303b on Mar 27, 2024

ryanofsky referenced this in commit 9fe6a68170 on Mar 28, 2024

ryanofsky referenced this in commit 7a4741eaf8 on Mar 28, 2024

ryanofsky referenced this in commit 5467ecbe58 on Apr 18, 2024

ryanofsky referenced this in commit 28e2081210 on Apr 18, 2024

ryanofsky referenced this in commit d293d0ba52 on Apr 24, 2024

ryanofsky referenced this in commit c45cb13b9e on Apr 24, 2024

ryanofsky referenced this in commit 6602231564 on Apr 24, 2024

ryanofsky referenced this in commit f74da4c69b on Apr 24, 2024

ryanofsky referenced this in commit cbcdc67c64 on Apr 24, 2024

ryanofsky referenced this in commit da1f3d1c72 on Apr 24, 2024

ryanofsky referenced this in commit 23abc129d2 on Apr 24, 2024

ryanofsky referenced this in commit 8310de510b on Apr 24, 2024

ryanofsky referenced this in commit 990f9d65c5 on Apr 24, 2024

ryanofsky referenced this in commit 10ff4de2b6 on Apr 24, 2024

ryanofsky referenced this in commit 98cb2e1bf9 on Apr 24, 2024

ryanofsky referenced this in commit 8e0323bc8e on Apr 25, 2024

ryanofsky referenced this in commit 4fb08c28ba on Apr 25, 2024

ryanofsky referenced this in commit 0290a2e819 on Apr 25, 2024

ryanofsky referenced this in commit 88aa093356 on Apr 25, 2024

ryanofsky referenced this in commit 7cd5b8d3b6 on Apr 26, 2024

ryanofsky referenced this in commit f45705988e on Apr 26, 2024

ryanofsky referenced this in commit 3103e87086 on Apr 26, 2024

ryanofsky referenced this in commit 0cbcb66257 on Apr 26, 2024

ryanofsky referenced this in commit e9a5ad3f4b on Apr 26, 2024

ryanofsky referenced this in commit 55d7de92bb on Apr 26, 2024

ryanofsky referenced this in commit 9bc6779338 on Apr 26, 2024

ryanofsky referenced this in commit 2d22f23766 on Apr 26, 2024

ryanofsky referenced this in commit 616572cbb4 on Apr 26, 2024

ryanofsky referenced this in commit e1be443315 on Apr 26, 2024

ryanofsky referenced this in commit cd62acaf6a on Apr 26, 2024

ryanofsky referenced this in commit 0c8a1bb144 on Apr 26, 2024

ryanofsky referenced this in commit 7a1982fe1d on Apr 26, 2024

ryanofsky referenced this in commit 9552619961 on Apr 26, 2024

ryanofsky referenced this in commit 834f65e824 on Apr 30, 2024

ryanofsky referenced this in commit 6a8b2befea on Apr 30, 2024

DrahtBot added the label Needs rebase on Apr 30, 2024

ryanofsky referenced this in commit 84b347df68 on May 1, 2024

ryanofsky referenced this in commit db91dbb5a9 on May 1, 2024

ryanofsky referenced this in commit c1db95485c on May 1, 2024

ryanofsky referenced this in commit 5d8923d53e on May 1, 2024

ryanofsky force-pushed on May 1, 2024

DrahtBot removed the label Needs rebase on May 1, 2024

DrahtBot added the label CI failed on May 1, 2024

DrahtBot commented at 10:42 pm on May 1, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24477515855

ryanofsky force-pushed on May 2, 2024

ryanofsky force-pushed on May 3, 2024

DrahtBot removed the label CI failed on May 3, 2024

ryanofsky commented at 5:55 pm on May 3, 2024: contributor

Rebased 2385cb37dbbe00004f8bffe1d1d784778d89a21e -> 987b28bbc991868ed305dbfc8f0846dd8b0b893c (pr/bresult-load.21 -> pr/bresult-load.22, compare) to use newer result API in #25722. Updated 987b28bbc991868ed305dbfc8f0846dd8b0b893c -> 548bf508e26aae39c732b3e61861d5c29669df06 (pr/bresult-load.22 -> pr/bresult-load.23, compare) to fix a bug in wallet tool and an accidental change to error message ordering Updated 548bf508e26aae39c732b3e61861d5c29669df06 -> ae0b9b8fcece154bae7f4383990e9d10f6cc0d2b (pr/bresult-load.23 -> pr/bresult-load.24, compare) to fix more unintended changes to error message order. Updated ae0b9b8fcece154bae7f4383990e9d10f6cc0d2b -> 5beaefaa806e54327d5995d86fd2d56d4687d575 (pr/bresult-load.24 -> pr/bresult-load.25, compare) to fix clang-tidy errors in https://cirrus-ci.com/task/6679456805289984 and ResultExtract bug in intermediate commit https://github.com/bitcoin/bitcoin/actions/runs/8931046064/job/24532379963?pr=25722

DrahtBot added the label Needs rebase on May 20, 2024

janus referenced this in commit 785f5c7679 on Jun 6, 2024

janus referenced this in commit 0ac7fbf62d on Jun 6, 2024

ryanofsky referenced this in commit ba959dbed3 on Jun 17, 2024

ryanofsky referenced this in commit b08548336e on Jun 17, 2024

ryanofsky referenced this in commit 90f7d39983 on Jun 17, 2024

ryanofsky referenced this in commit 387cf87d52 on Jun 17, 2024

ryanofsky force-pushed on Jun 17, 2024

DrahtBot removed the label Needs rebase on Jun 17, 2024

DrahtBot added the label CI failed on Jun 18, 2024

DrahtBot commented at 2:33 am on June 18, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/26340616800

ryanofsky force-pushed on Jun 18, 2024

DrahtBot commented at 8:36 am on June 19, 2024: contributor

0wallet/test/fuzz/wallet_bdb_parser.cpp:22:15: error: using decl 'DatabaseError' is unused [misc-unused-using-decls,-warnings-as-errors]
1   22 | using wallet::DatabaseError;
2      |               ^
3wallet/test/fuzz/wallet_bdb_parser.cpp:22:15: note: remove the using
4   22 | using wallet::DatabaseError;
5      | ~~~~~~~~~~~~~~^~~~~~~~~~~~~~
6^^^ ⚠️ Failure generated from clang-tidy
7+ echo '^^^ ⚠️ Failure generated from clang-tidy'
8+ false
9��������

ryanofsky force-pushed on Jun 20, 2024

DrahtBot removed the label CI failed on Jun 20, 2024

DrahtBot added the label Needs rebase on Jul 2, 2024

ryanofsky referenced this in commit 7be482627f on Jul 9, 2024

ryanofsky referenced this in commit a7e05126ef on Jul 9, 2024

ryanofsky referenced this in commit 74c124cbf0 on Jul 9, 2024

ryanofsky referenced this in commit 4eb36d1c41 on Jul 9, 2024

ryanofsky force-pushed on Jul 18, 2024

DrahtBot added the label CI failed on Jul 18, 2024

DrahtBot commented at 10:46 pm on July 18, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27634111678

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label Needs rebase on Jul 18, 2024

ryanofsky force-pushed on Jul 19, 2024

DrahtBot removed the label CI failed on Jul 19, 2024

ryanofsky referenced this in commit ebb45ad32d on Jul 19, 2024

ryanofsky referenced this in commit 15b673d122 on Jul 19, 2024

DrahtBot added the label Needs rebase on Aug 15, 2024

ryanofsky referenced this in commit 3f858220b6 on Nov 4, 2024

ryanofsky referenced this in commit a78c1c9ca6 on Nov 4, 2024

ryanofsky referenced this in commit b6ff54babf on Nov 4, 2024

ryanofsky referenced this in commit 609bb02d97 on Nov 4, 2024

ryanofsky referenced this in commit b23df5e070 on Nov 4, 2024

ryanofsky referenced this in commit 7e2b35711e on Nov 4, 2024

ryanofsky force-pushed on Nov 4, 2024

ryanofsky commented at 1:38 pm on November 4, 2024: contributor

Rebased 2b0e70295ed6ff4be91cc5775ddaa9351a1ed430 -> caed6aec4241a2e5d849c9446725c889348e1b7e (pr/bresult-load.30 -> pr/bresult-load.31, compare) due to various conflicts Updated caed6aec4241a2e5d849c9446725c889348e1b7e -> 830570fe684e05c170def209633c682363d3b830 (pr/bresult-load.31 -> pr/bresult-load.32, compare) to fix DoMigration error handling bug https://github.com/bitcoin/bitcoin/actions/runs/11665336901/job/32477794828?pr=25722 Rebased 830570fe684e05c170def209633c682363d3b830 -> e2376f9847c8cd07bb5f2c82f8b0055a72086b66 (pr/bresult-load.32 -> pr/bresult-load.33, compare) due to various conflicts

DrahtBot removed the label Needs rebase on Nov 4, 2024

DrahtBot added the label CI failed on Nov 4, 2024

ryanofsky force-pushed on Nov 4, 2024

DrahtBot removed the label CI failed on Nov 4, 2024

DrahtBot added the label Needs rebase on Dec 4, 2024

ryanofsky referenced this in commit 3864a57835 on Dec 6, 2024

ryanofsky referenced this in commit 58c33f0f3a on Dec 6, 2024

ryanofsky referenced this in commit dcedb20a8a on Dec 6, 2024

ryanofsky referenced this in commit 108368d7e2 on Dec 6, 2024

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<int> is 16 bytes, and util::Result<void> is 8
  bytes. Previously util::Result<int> and util::Result<void> were 72 bytes.

- More documentation and tests.

0a0f60dd2b

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

988d7e5b90

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.

361ef0891b

refactor: Use util::Result class in LoadChainstate and VerifyLoadedChainstate 2dd48496a4

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

643b881011

test: add static test for util::Result memory usage

Suggested by Martin Leitner-Ankerl <martin.ankerl@gmail.com>
https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174298529

Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>

5e14665e04

Merge remote-tracking branch 'origin/pull/25665/head' c130136ad3

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.

28e3cb3938

Use ResultPtr<unique_ptr> instead of Result<unique_ptr> f44cb2416f

Merge remote-tracking branch 'origin/pull/26022/head' 063b5d8f1e

Add temporary ResultExtract helper for porting to util::Result 7665903504

refactor: Use util::Result class in wallet/sqlite c59404ce74

refactor: Use util::Result class in wallet/bdb cf327bffdc

refactor: Use util::Result class in wallet/migrate fcb41875c3

refactor: Use util::Result class in wallet::MakeDatabase 5ff81d9450

refactor: Use util::Result class in wallet/dump 0afd7b55f8

refactor: Use util::Result class in wallet/salvage 3faa53122d

refactor: Use util::Result class in wallet/wallettool 9319a3709b

refactor: Use util::Result class in wallet/wallet da907cbf01

refactor: Use util::Result class in wallet/load feace0a121

refactor: Use util::Result class in wallet/rpc 04185045dc

refactor: Use util::Result class in wallet/interfaces 3d657ccd02

refactor: Use util::Result class in wallet/test ab0476db30

Drop temporary ResultExtract helper for porting to util::Result f193dfd14e

scripted-diff: replace wallet DatabaseStatus with DatabaseError

-BEGIN VERIFY SCRIPT-
git grep -l DatabaseStatus src | xargs sed -i s/DatabaseStatus/DatabaseError/g
sed -i '/^    SUCCESS,$/d' src/wallet/db.h
-END VERIFY SCRIPT-

e2376f9847

ryanofsky referenced this in commit 1def53ca4d on Dec 6, 2024

ryanofsky referenced this in commit e10e4eb5b0 on Dec 6, 2024

ryanofsky force-pushed on Dec 6, 2024

DrahtBot removed the label Needs rebase on Dec 6, 2024

refactor: Use util::Result class for wallet loading #25722

Code Coverage & Benchmarks

Reviews

Conflicts