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

pull ryanofsky wants to merge 24 commits into bitcoin:master from ryanofsky:pr/bresult-load changing 41 files +1379 −765
  2. DrahtBot added the label Refactoring on Jul 27, 2022
  3. DrahtBot commented at 8:53 PM on July 27, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35182 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
    • #35018 (wallet, bench: Use Nanobench setup() for wallet benchmarks, and remove DuplicateMockDatabase by achow101)
    • #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
    • #34544 (wallet: Disallow wallet names that are paths including .. and . elements by achow101)
    • #34411 ([POC] Full Libevent removal by fanquake)
    • #34400 (wallet: parallel fast rescan (approx 5x speed up with 8 threads) by Eunovo)
    • #34193 (wallet: make migration more robust against failures by furszy)
    • #34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
    • #33112 (wallet: relax external_signer flag constraints by Sjors)
    • #33034 (wallet: Store transactions in a separate sqlite table by achow101)
    • #33033 (wallet, sqlite: Encapsulate SQLite statements in a RAII class by achow101)
    • #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
    • #29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
    • #28690 (build: Introduce internal kernel library by sedited)
    • #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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • deal with with error and warning messages -> deal with error and warning messages [Typo: β€œwith” is duplicated in the util/messages.h file comment, making the sentence grammatically incorrect.]

    Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • ExpectSuccess(IntFn(5, true), {}, 5) in src/test/result_tests.cpp
    • ExpectSuccess(NoCopyFn(5, true), {}, 5) in src/test/result_tests.cpp
    • ExpectError(NoCopyFn(5, false), Untranslated("nocopy 5 error."), 5) in src/test/result_tests.cpp
    • ExpectSuccess(NoCopyNoMoveFn(5, true), {}, 5) in src/test/result_tests.cpp
    • ExpectError(NoCopyNoMoveFn(5, false), Untranslated("nocopynomove 5 error."), 5) in src/test/result_tests.cpp
    • ExpectError(ChainedErrorFn(ERR1, 5), Untranslated("chained error. enum error. warn."), 5) in src/test/result_tests.cpp
    • ExpectSuccess(MultiWarnFn(3), Untranslated("warn 0. warn 1. warn 2."), 3) in src/test/result_tests.cpp
    • ExpectSuccess(AccumulateFn(true), Untranslated("warn 0. warn 0. warn 1. int 3 warn."), 3) in src/test/result_tests.cpp
    • ExpectSuccess(TruthyFalsyFn(0, true), {}, 0) in src/test/result_tests.cpp
    • ExpectError(TruthyFalsyFn(0, false), Untranslated("error value 0."), 0) in src/test/result_tests.cpp
    • ExpectSuccess(TruthyFalsyFn(1, true), {}, 1) in src/test/result_tests.cpp
    • ExpectError(TruthyFalsyFn(1, false), Untranslated("error value 1."), 1) in src/test/result_tests.cpp
    • ExpectSuccess(result, {}, 0) in src/test/result_tests.cpp
    • ExpectSuccess(result, Untranslated("error"), 2) in src/test/result_tests.cpp
    • ExpectSuccess(result2, {}, 0) in src/test/result_tests.cpp
    • ExpectError(result2, Untranslated("error"), 3) in src/test/result_tests.cpp
    • ExpectSuccess(result2, Untranslated("error"), 4) in src/test/result_tests.cpp

    <sup>2026-04-01 12:18:28</sup>

  4. DrahtBot added the label Needs rebase on Aug 5, 2022
  5. ryanofsky force-pushed on Aug 5, 2022
  6. DrahtBot removed the label Needs rebase on Aug 5, 2022
  7. DrahtBot added the label Needs rebase on Aug 10, 2022
  8. ryanofsky force-pushed on Aug 15, 2022
  9. ryanofsky marked this as ready for review on Aug 15, 2022
  10. 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

  11. DrahtBot removed the label Needs rebase on Aug 15, 2022
  12. DrahtBot added the label Needs rebase on Aug 16, 2022
  13. ryanofsky referenced this in commit 834857e56b on Aug 30, 2022
  14. ryanofsky referenced this in commit 82c549aa53 on Sep 1, 2022
  15. ryanofsky force-pushed on Sep 1, 2022
  16. DrahtBot removed the label Needs rebase on Sep 1, 2022
  17. ryanofsky referenced this in commit c14e904f66 on Sep 1, 2022
  18. ryanofsky force-pushed on Sep 1, 2022
  19. ryanofsky force-pushed on Sep 2, 2022
  20. ryanofsky referenced this in commit a8973ae714 on Sep 6, 2022
  21. ryanofsky referenced this in commit 05a97d3208 on Sep 13, 2022
  22. ryanofsky referenced this in commit e04d8a754f on Sep 13, 2022
  23. ryanofsky referenced this in commit 8e12547694 on Sep 13, 2022
  24. DrahtBot added the label Needs rebase on Sep 13, 2022
  25. ryanofsky referenced this in commit 52a4e50fb4 on Sep 13, 2022
  26. ryanofsky force-pushed on Sep 14, 2022
  27. ryanofsky referenced this in commit f9accbc6e2 on Sep 15, 2022
  28. ryanofsky referenced this in commit 776d9b3fbb on Sep 15, 2022
  29. ryanofsky force-pushed on Sep 15, 2022
  30. DrahtBot removed the label Needs rebase on Sep 15, 2022
  31. DrahtBot added the label Needs rebase on Sep 16, 2022
  32. ryanofsky referenced this in commit f05441a572 on Sep 20, 2022
  33. ryanofsky referenced this in commit 456e3d4ecc on Sep 20, 2022
  34. ryanofsky force-pushed on Sep 20, 2022
  35. DrahtBot removed the label Needs rebase on Sep 20, 2022
  36. ryanofsky referenced this in commit 8ba9cb26a1 on Sep 20, 2022
  37. ryanofsky referenced this in commit 8ebf0cd171 on Sep 20, 2022
  38. DrahtBot added the label Needs rebase on Oct 3, 2022
  39. ryanofsky referenced this in commit fee8670c7c on Oct 14, 2022
  40. ryanofsky referenced this in commit 28a6934da9 on Oct 14, 2022
  41. ryanofsky force-pushed on Oct 14, 2022
  42. DrahtBot removed the label Needs rebase on Oct 14, 2022
  43. DrahtBot added the label Needs rebase on Dec 1, 2022
  44. ryanofsky referenced this in commit f4d55d858d on Jan 6, 2023
  45. ryanofsky referenced this in commit 979dbd5c73 on Jan 6, 2023
  46. ryanofsky force-pushed on Jan 20, 2023
  47. DrahtBot removed the label Needs rebase on Jan 20, 2023
  48. DrahtBot added the label Needs rebase on Jan 23, 2023
  49. ryanofsky referenced this in commit b57a898a98 on Feb 10, 2023
  50. ryanofsky referenced this in commit 513adde96f on Feb 10, 2023
  51. ryanofsky referenced this in commit eb50fcd685 on Feb 10, 2023
  52. ryanofsky force-pushed on Feb 11, 2023
  53. DrahtBot removed the label Needs rebase on Feb 11, 2023
  54. ryanofsky referenced this in commit 501ef88b94 on Feb 16, 2023
  55. DrahtBot added the label Needs rebase on Feb 17, 2023
  56. ryanofsky referenced this in commit d785176df3 on Mar 1, 2023
  57. ryanofsky referenced this in commit 6056f7b0da on Mar 1, 2023
  58. ryanofsky force-pushed on Mar 1, 2023
  59. DrahtBot removed the label Needs rebase on Mar 1, 2023
  60. DrahtBot added the label Needs rebase on Mar 8, 2023
  61. ryanofsky referenced this in commit e766928ed1 on Mar 16, 2023
  62. ryanofsky referenced this in commit 6afc0431f0 on Mar 16, 2023
  63. ryanofsky referenced this in commit 6d7d7bbb7b on Apr 4, 2023
  64. ryanofsky referenced this in commit 460ef0a18b on Apr 4, 2023
  65. ryanofsky force-pushed on Apr 7, 2023
  66. DrahtBot removed the label Needs rebase on Apr 7, 2023
  67. DrahtBot added the label Needs rebase on Apr 12, 2023
  68. ryanofsky force-pushed on Apr 13, 2023
  69. DrahtBot removed the label Needs rebase on Apr 13, 2023
  70. 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.

  71. 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

  72. in src/util/result.h:77 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 :-)

  73. 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:

    diff --git a/src/util/result.h b/src/util/result.h
index 5599da43dd..3c163cf398 100644
--- a/src/util/result.h
+++ b/src/util/result.h
@@ -151,33 +151,35 @@ template <typename T, typename F = void>
 class Result : public detail::ResultBase<T, F>
 {
 protected:
-    template <typename Fn, typename... Args>
-    void Construct(const Fn& fn, Args&&... args)
+    template <bool IsValueConstruction, typename... Args>
+    void Construct(Args&&... args)
     {
-        fn(std::forward<Args>(args)...);
+        if constexpr (IsValueConstruction) {
+            this->ConstructValue(std::forward<Args>(args)...);
+        } else {
+            this->Info().failure.emplace(std::forward<Args>(args)...);
+        }
     }
 
-    template <typename Fn, typename... Args>
-    void Construct(const Fn& fn, Error error, Args&&... args)
+    template <bool IsValueConstruction, typename... Args>
+    void Construct(Error error, Args&&... args)
     {
         this->AddError(std::move(error.message));
-        Construct([&](auto&&... x) {
-            this->Info().failure.emplace(std::forward<decltype(x)>(x)...);
-        }, std::forward<Args>(args)...);
+        Construct<false /*always false from here on*/>(std::forward<Args>(args)...);
     }
 
-    template <typename Fn, typename... Args>
-    void Construct(const Fn& fn, Warning warning, Args&&... args)
+    template <bool IsValueConstruction, typename... Args>
+    void Construct(Warning warning, Args&&... args)
     {
         this->AddWarning(std::move(warning.message));
-        Construct(fn, std::forward<Args>(args)...);
+        Construct<IsValueConstruction>(std::forward<Args>(args)...);
     }
 
-    template <typename Fn, typename OT, typename OF, typename... Args>
-    void Construct(const Fn& fn, Result<OT, OF>&& other, Args&&... args)
+    template <bool IsValueConstruction, typename OT, typename OF, typename... Args>
+    void Construct(Result<OT, OF>&& other, Args&&... args)
     {
         this->MoveMessages(other);
-        Construct(fn, std::forward<Args>(args)...);
+        Construct<IsValueConstruction>(std::forward<Args>(args)...);
     }
 
     template <typename OT, typename OF>
@@ -198,7 +200,8 @@ public:
     template <typename... Args>
     Result(Args&&... args)
     {
-        Construct([this](auto&&... x) { this->ConstructValue(std::forward<decltype(x)>(x)...); }, std::forward<Args>(args)...);
+        // recursively peels of the first argument until we reach the base Construct method
+        Construct<true>(std::forward<Args>(args)...);
     }
 
     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"

  74. in src/util/result.h:207 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

    template <typename OT, typename OF>
Result& 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

  75. in src/util/result.h:232 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.

  76. in src/util/result.h:150 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.

    sedited 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.

  77. in src/test/result_tests.cpp:149 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:

    static_assert(sizeof(util::Result<int>) == sizeof(void*)*2);
static_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:

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

    Thanks, added to result_tests.cpp

  78. in src/util/result.h:166 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.

  79. in src/wallet/spend.cpp:746 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

  80. 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

  81. in src/wallet/spend.cpp:720 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

  82. martinus changes_requested
  83. 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.

  84. DrahtBot added the label CI failed on Apr 23, 2023
  85. DrahtBot added the label Needs rebase on May 2, 2023
  86. ryanofsky referenced this in commit aa73bee496 on May 2, 2023
  87. ryanofsky referenced this in commit 28a954c703 on May 2, 2023
  88. ryanofsky force-pushed on May 3, 2023
  89. DrahtBot removed the label Needs rebase on May 3, 2023
  90. DrahtBot removed the label CI failed on May 3, 2023
  91. DrahtBot added the label Needs rebase on May 15, 2023
  92. ryanofsky referenced this in commit cdf11c66c0 on Jul 21, 2023
  93. ryanofsky referenced this in commit 40f09de73e on Jul 21, 2023
  94. ryanofsky force-pushed on Jul 21, 2023
  95. 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.

  96. DrahtBot removed the label Needs rebase on Jul 21, 2023
  97. ryanofsky referenced this in commit 4d995d3fa6 on Jul 21, 2023
  98. ryanofsky referenced this in commit 775b54e881 on Jul 21, 2023
  99. DrahtBot added the label CI failed on Jul 21, 2023
  100. 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?

  101. ryanofsky marked this as a draft on Aug 1, 2023
  102. ryanofsky referenced this in commit cf01e29b9d on Aug 1, 2023
  103. ryanofsky referenced this in commit 1de05ef919 on Aug 1, 2023
  104. ryanofsky referenced this in commit 6ddcc9bec8 on Aug 1, 2023
  105. ryanofsky referenced this in commit 08f5febfc5 on Aug 1, 2023
  106. ryanofsky force-pushed on Aug 2, 2023
  107. DrahtBot removed the label CI failed on Aug 2, 2023
  108. ryanofsky referenced this in commit 9cea9e8e38 on Aug 3, 2023
  109. ryanofsky referenced this in commit b0beb4c504 on Aug 3, 2023
  110. ryanofsky referenced this in commit 835f09452b on Aug 4, 2023
  111. ryanofsky referenced this in commit 9e80d0b754 on Aug 4, 2023
  112. ryanofsky referenced this in commit b21e2b067e on Aug 7, 2023
  113. ryanofsky referenced this in commit 7f883b33bb on Aug 7, 2023
  114. DrahtBot added the label Needs rebase on Aug 24, 2023
  115. ryanofsky referenced this in commit 5529ea16ad on Sep 6, 2023
  116. ryanofsky referenced this in commit a21ac50c2b on Sep 6, 2023
  117. ryanofsky referenced this in commit ed61b012b1 on Sep 6, 2023
  118. ryanofsky referenced this in commit 956bec1eca on Sep 6, 2023
  119. ryanofsky force-pushed on Sep 25, 2023
  120. DrahtBot removed the label Needs rebase on Sep 25, 2023
  121. DrahtBot added the label CI failed on Oct 5, 2023
  122. DrahtBot added the label Needs rebase on Oct 16, 2023
  123. ryanofsky referenced this in commit 256a16e999 on Oct 18, 2023
  124. ryanofsky referenced this in commit 29f6cfdabe on Oct 18, 2023
  125. ryanofsky referenced this in commit 1f6190ac9b on Oct 18, 2023
  126. ryanofsky referenced this in commit 24999606f8 on Oct 18, 2023
  127. ryanofsky referenced this in commit e2b184ec86 on Oct 26, 2023
  128. ryanofsky referenced this in commit f158686962 on Oct 26, 2023
  129. ryanofsky referenced this in commit 2e1cc4f55f on Oct 26, 2023
  130. ryanofsky referenced this in commit 3fe48360c3 on Oct 26, 2023
  131. ryanofsky referenced this in commit 1c7d8bea4f on Jan 24, 2024
  132. ryanofsky referenced this in commit f822ac9a24 on Jan 24, 2024
  133. ryanofsky referenced this in commit 774240a82b on Feb 21, 2024
  134. ryanofsky referenced this in commit 4ec6b060a8 on Feb 21, 2024
  135. ryanofsky referenced this in commit 2c78cce3c5 on Feb 21, 2024
  136. ryanofsky referenced this in commit f27418f37e on Feb 21, 2024
  137. ryanofsky referenced this in commit b0687b7e1e on Feb 21, 2024
  138. ryanofsky referenced this in commit 2055659814 on Feb 21, 2024
  139. ryanofsky referenced this in commit b91edfa4f1 on Feb 21, 2024
  140. ryanofsky force-pushed on Feb 21, 2024
  141. 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

  142. DrahtBot removed the label CI failed on Feb 22, 2024
  143. DrahtBot removed the label Needs rebase on Feb 22, 2024
  144. ryanofsky referenced this in commit 16b1f910be on Feb 22, 2024
  145. ryanofsky referenced this in commit cdf7bb1756 on Feb 22, 2024
  146. ryanofsky force-pushed on Feb 22, 2024
  147. 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.

  148. sedited referenced this in commit 50220e34f6 on Mar 10, 2024
  149. sedited referenced this in commit 539f101dfe on Mar 10, 2024
  150. sedited referenced this in commit fe260d92a3 on Mar 11, 2024
  151. sedited referenced this in commit 5029e59b9d on Mar 11, 2024
  152. sedited referenced this in commit f3dbbbc3d5 on Mar 12, 2024
  153. sedited referenced this in commit 63b92a8c80 on Mar 12, 2024
  154. sedited referenced this in commit a72ae6a43f on Mar 12, 2024
  155. sedited referenced this in commit 76fe2f8d41 on Mar 12, 2024
  156. sedited referenced this in commit 2331edb22e on Mar 12, 2024
  157. sedited referenced this in commit 5797f3cb7c on Mar 12, 2024
  158. sedited referenced this in commit 31ae72fdfa on Mar 15, 2024
  159. sedited referenced this in commit 6b7813c256 on Mar 15, 2024
  160. sedited referenced this in commit f6118b724c on Mar 19, 2024
  161. sedited referenced this in commit ee80f88bcb on Mar 19, 2024
  162. ryanofsky referenced this in commit 40b009dfee on Mar 22, 2024
  163. ryanofsky referenced this in commit e0cbb8862c on Mar 22, 2024
  164. ryanofsky referenced this in commit d52c2e949c on Mar 26, 2024
  165. ryanofsky referenced this in commit efb463788f on Mar 26, 2024
  166. sedited referenced this in commit c7d4f43198 on Mar 26, 2024
  167. sedited referenced this in commit 5e38b42b1e on Mar 26, 2024
  168. ryanofsky referenced this in commit 88821fe2a0 on Mar 26, 2024
  169. ryanofsky referenced this in commit 110bccc371 on Mar 26, 2024
  170. ryanofsky referenced this in commit d859ea511f on Mar 26, 2024
  171. ryanofsky referenced this in commit 6adeafab68 on Mar 26, 2024
  172. ryanofsky referenced this in commit 3eb47b4641 on Mar 27, 2024
  173. ryanofsky referenced this in commit d4579e303b on Mar 27, 2024
  174. ryanofsky referenced this in commit 9fe6a68170 on Mar 28, 2024
  175. ryanofsky referenced this in commit 7a4741eaf8 on Mar 28, 2024
  176. ryanofsky referenced this in commit 5467ecbe58 on Apr 18, 2024
  177. ryanofsky referenced this in commit 28e2081210 on Apr 18, 2024
  178. ryanofsky referenced this in commit d293d0ba52 on Apr 24, 2024
  179. ryanofsky referenced this in commit c45cb13b9e on Apr 24, 2024
  180. ryanofsky referenced this in commit 6602231564 on Apr 24, 2024
  181. ryanofsky referenced this in commit f74da4c69b on Apr 24, 2024
  182. ryanofsky referenced this in commit cbcdc67c64 on Apr 24, 2024
  183. ryanofsky referenced this in commit da1f3d1c72 on Apr 24, 2024
  184. ryanofsky referenced this in commit 23abc129d2 on Apr 24, 2024
  185. ryanofsky referenced this in commit 8310de510b on Apr 24, 2024
  186. ryanofsky referenced this in commit 990f9d65c5 on Apr 24, 2024
  187. ryanofsky referenced this in commit 10ff4de2b6 on Apr 24, 2024
  188. ryanofsky referenced this in commit 98cb2e1bf9 on Apr 24, 2024
  189. ryanofsky referenced this in commit 8e0323bc8e on Apr 25, 2024
  190. ryanofsky referenced this in commit 4fb08c28ba on Apr 25, 2024
  191. ryanofsky referenced this in commit 0290a2e819 on Apr 25, 2024
  192. ryanofsky referenced this in commit 88aa093356 on Apr 25, 2024
  193. ryanofsky referenced this in commit 7cd5b8d3b6 on Apr 26, 2024
  194. ryanofsky referenced this in commit f45705988e on Apr 26, 2024
  195. ryanofsky referenced this in commit 3103e87086 on Apr 26, 2024
  196. ryanofsky referenced this in commit 0cbcb66257 on Apr 26, 2024
  197. ryanofsky referenced this in commit e9a5ad3f4b on Apr 26, 2024
  198. ryanofsky referenced this in commit 55d7de92bb on Apr 26, 2024
  199. ryanofsky referenced this in commit 9bc6779338 on Apr 26, 2024
  200. ryanofsky referenced this in commit 2d22f23766 on Apr 26, 2024
  201. ryanofsky referenced this in commit 616572cbb4 on Apr 26, 2024
  202. ryanofsky referenced this in commit e1be443315 on Apr 26, 2024
  203. ryanofsky referenced this in commit cd62acaf6a on Apr 26, 2024
  204. ryanofsky referenced this in commit 0c8a1bb144 on Apr 26, 2024
  205. ryanofsky referenced this in commit 7a1982fe1d on Apr 26, 2024
  206. ryanofsky referenced this in commit 9552619961 on Apr 26, 2024
  207. ryanofsky referenced this in commit 834f65e824 on Apr 30, 2024
  208. ryanofsky referenced this in commit 6a8b2befea on Apr 30, 2024
  209. DrahtBot added the label Needs rebase on Apr 30, 2024
  210. ryanofsky referenced this in commit 84b347df68 on May 1, 2024
  211. ryanofsky referenced this in commit db91dbb5a9 on May 1, 2024
  212. ryanofsky referenced this in commit c1db95485c on May 1, 2024
  213. ryanofsky referenced this in commit 5d8923d53e on May 1, 2024
  214. ryanofsky force-pushed on May 1, 2024
  215. DrahtBot removed the label Needs rebase on May 1, 2024
  216. DrahtBot added the label CI failed on May 1, 2024
  217. DrahtBot commented at 10:42 PM on May 1, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 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.

    <sub>Debug: https://github.com/bitcoin/bitcoin/runs/24477515855</sub>

  218. ryanofsky force-pushed on May 2, 2024
  219. ryanofsky force-pushed on May 2, 2024
  220. ryanofsky force-pushed on May 3, 2024
  221. DrahtBot removed the label CI failed on May 3, 2024
  222. 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

  223. DrahtBot added the label Needs rebase on May 20, 2024
  224. janus referenced this in commit 785f5c7679 on Jun 6, 2024
  225. janus referenced this in commit 0ac7fbf62d on Jun 6, 2024
  226. ryanofsky referenced this in commit ba959dbed3 on Jun 17, 2024
  227. ryanofsky referenced this in commit b08548336e on Jun 17, 2024
  228. ryanofsky referenced this in commit 90f7d39983 on Jun 17, 2024
  229. ryanofsky referenced this in commit 387cf87d52 on Jun 17, 2024
  230. ryanofsky force-pushed on Jun 17, 2024
  231. DrahtBot removed the label Needs rebase on Jun 17, 2024
  232. DrahtBot added the label CI failed on Jun 18, 2024
  233. DrahtBot commented at 2:33 AM on June 18, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 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.

    <sub>Debug: https://github.com/bitcoin/bitcoin/runs/26340616800</sub>

  234. ryanofsky force-pushed on Jun 18, 2024
  235. DrahtBot commented at 8:36 AM on June 19, 2024: contributor 
    wallet/test/fuzz/wallet_bdb_parser.cpp:22:15: error: using decl 'DatabaseError' is unused [misc-unused-using-decls,-warnings-as-errors]
   22 | using wallet::DatabaseError;
      |               ^
wallet/test/fuzz/wallet_bdb_parser.cpp:22:15: note: remove the using
   22 | using wallet::DatabaseError;
      | ~~~~~~~~~~~~~~^~~~~~~~~~~~~~
^^^ ⚠️ Failure generated from clang-tidy
+ echo '^^^ ⚠️ Failure generated from clang-tidy'
+ false
οΏ½οΏ½οΏ½οΏ½οΏ½οΏ½οΏ½οΏ½
  236. ryanofsky force-pushed on Jun 20, 2024
  237. DrahtBot removed the label CI failed on Jun 20, 2024
  238. DrahtBot added the label Needs rebase on Jul 2, 2024
  239. ryanofsky referenced this in commit 7be482627f on Jul 9, 2024
  240. ryanofsky referenced this in commit a7e05126ef on Jul 9, 2024
  241. ryanofsky referenced this in commit 74c124cbf0 on Jul 9, 2024
  242. ryanofsky referenced this in commit 4eb36d1c41 on Jul 9, 2024
  243. ryanofsky force-pushed on Jul 18, 2024
  244. DrahtBot added the label CI failed on Jul 18, 2024
  245. DrahtBot commented at 10:46 PM on July 18, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  246. DrahtBot removed the label Needs rebase on Jul 18, 2024
  247. ryanofsky force-pushed on Jul 19, 2024
  248. DrahtBot removed the label CI failed on Jul 19, 2024
  249. ryanofsky referenced this in commit ebb45ad32d on Jul 19, 2024
  250. ryanofsky referenced this in commit 15b673d122 on Jul 19, 2024
  251. DrahtBot added the label Needs rebase on Aug 15, 2024
  252. ryanofsky referenced this in commit 3f858220b6 on Nov 4, 2024
  253. ryanofsky referenced this in commit a78c1c9ca6 on Nov 4, 2024
  254. ryanofsky referenced this in commit b6ff54babf on Nov 4, 2024
  255. ryanofsky referenced this in commit 609bb02d97 on Nov 4, 2024
  256. ryanofsky referenced this in commit b23df5e070 on Nov 4, 2024
  257. ryanofsky referenced this in commit 7e2b35711e on Nov 4, 2024
  258. ryanofsky force-pushed on Nov 4, 2024
  259. 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 Rebased e2376f9847c8cd07bb5f2c82f8b0055a72086b66 -> 0284396a4229025a6201d7486484b17c00f3eb8c (pr/bresult-load.33 -> pr/bresult-load.34, compare) due to conflicts with #31241, #31451, and other PRs Rebased 0284396a4229025a6201d7486484b17c00f3eb8c -> 88dfdac4bc23c87f79c55ed6b2c255f29137768e (pr/bresult-load.34 -> pr/bresult-load.35, compare) due to conflicts with #31961 and #31519

    <!-- begin push-36 -->

    Rebased 88dfdac4bc23c87f79c55ed6b2c255f29137768e -> 96f66136f02833f00520eb18e32d0a2848cb4c2a (pr/bresult-load.35 -> pr/bresult-load.36, compare)<!-- end -->

    <!-- begin push-37 -->

    Rebased 96f66136f02833f00520eb18e32d0a2848cb4c2a -> fdff2b4550e8b2b64e25d413298283fa1b516088 (pr/bresult-load.36 -> pr/bresult-load.37, compare)<!-- end -->

    <!-- begin push-38 -->

    Rebased fdff2b4550e8b2b64e25d413298283fa1b516088 -> 46d01725ed1434fba8d974f57f6893e2cd1facee (pr/bresult-load.37 -> pr/bresult-load.38, compare)<!-- end --> due to conflict with #30595

    <!-- begin push-39 -->

    Rebased 46d01725ed1434fba8d974f57f6893e2cd1facee -> 2dc6d076e76f9c778dbced9adfd0ef64aea732b1 (pr/bresult-load.38 -> pr/bresult-load.39, compare)<!-- end --> on top of updated base PR to fix CI errors

    <!-- begin push-40 -->

    Rebased 2dc6d076e76f9c778dbced9adfd0ef64aea732b1 -> d3e677f59d6369dc295edb7b5f112b83f99ec3c9 (pr/bresult-load.39 -> pr/bresult-load.40, compare)<!-- end -->

    <!-- begin push-41 -->

    Rebased d3e677f59d6369dc295edb7b5f112b83f99ec3c9 -> 2c7996d7c398b0927008a1903aad786d81c3e949 (pr/bresult-load.40 -> pr/bresult-load.41, compare)<!-- end -->

    <!-- begin push-42 -->

    Rebased 2c7996d7c398b0927008a1903aad786d81c3e949 -> 06c3f0abda7bf258993c7c1742502d511574a50e (pr/bresult-load.41 -> pr/bresult-load.42, compare)<!-- end --> on top of #26022 pr/bresult-ptr.17

    <!-- begin push-43 -->

    Updated 06c3f0abda7bf258993c7c1742502d511574a50e -> 45da1d9a13b26d5030354c7d5d874fd59cf35d84 (pr/bresult-load.42 -> pr/bresult-load.43, compare)<!-- end --> to fix CI errors https://github.com/bitcoin/bitcoin/actions/runs/21101803730/job/60687048929?pr=25722

    <!-- begin push-44 -->

    Rebased 45da1d9a13b26d5030354c7d5d874fd59cf35d84 -> 23df7666d1cfae87609b0054e7184bfad71467dd (pr/bresult-load.43 -> pr/bresult-load.44, compare)<!-- end --> on top of #25665 pr/bresult2.73 due to conflicts with #31650 and #34269

    <!-- begin push-45 -->

    Added 1 commits 23df7666d1cfae87609b0054e7184bfad71467dd -> 9382817901067c57b5ff4044d4c7aa09aee25376 (pr/bresult-load.44 -> pr/bresult-load.45, compare)<!-- end --> to fix ci iwyu error https://github.com/bitcoin/bitcoin/actions/runs/21266012856/job/61207857203?pr=25722

    <!-- begin push-46 -->

    Rebased 9382817901067c57b5ff4044d4c7aa09aee25376 -> e164e1e7a5c89eab4c0ef30a4a772a249a95ce0c (pr/bresult-load.45 -> pr/bresult-load.46, compare)<!-- end --> due to conflicts with #32636

    <!-- begin push-47 -->

    Updated e164e1e7a5c89eab4c0ef30a4a772a249a95ce0c -> 8253c7087f2cd7f5d0e3e35f39120e29557be846 (pr/bresult-load.46 -> pr/bresult-load.47, compare)<!-- end --> to fix tidy and iwyu errors https://github.com/bitcoin/bitcoin/actions/runs/21833918575/job/62999419836?pr=25722

    <!-- begin push-48 -->

    Rebased 8253c7087f2cd7f5d0e3e35f39120e29557be846 -> d75ff7517eb28aa549a0fcfaa93c7fa20f391055 (pr/bresult-load.47 -> pr/bresult-load.48, compare)<!-- end --> on top of #25665 pr/bresult2.76

    <!-- begin push-49 -->

    Rebased d75ff7517eb28aa549a0fcfaa93c7fa20f391055 -> 89c866499cfc52496327cef6294b40ac7a3ae512 (pr/bresult-load.48 -> pr/bresult-load.49, compare)<!-- end --> on top of #25665 pr/bresult2.77

  260. DrahtBot removed the label Needs rebase on Nov 4, 2024
  261. DrahtBot added the label CI failed on Nov 4, 2024
  262. ryanofsky force-pushed on Nov 4, 2024
  263. DrahtBot removed the label CI failed on Nov 4, 2024
  264. DrahtBot added the label Needs rebase on Dec 4, 2024
  265. ryanofsky referenced this in commit 3864a57835 on Dec 6, 2024
  266. ryanofsky referenced this in commit 58c33f0f3a on Dec 6, 2024
  267. ryanofsky referenced this in commit dcedb20a8a on Dec 6, 2024
  268. ryanofsky referenced this in commit 108368d7e2 on Dec 6, 2024
  269. ryanofsky referenced this in commit 1def53ca4d on Dec 6, 2024
  270. ryanofsky referenced this in commit e10e4eb5b0 on Dec 6, 2024
  271. ryanofsky referenced this in commit 643b881011 on Dec 6, 2024
  272. ryanofsky referenced this in commit 5e14665e04 on Dec 6, 2024
  273. ryanofsky force-pushed on Dec 6, 2024
  274. DrahtBot removed the label Needs rebase on Dec 6, 2024
  275. DrahtBot added the label Needs rebase on Jan 17, 2025
  276. ryanofsky referenced this in commit 6a2152a073 on Mar 11, 2025
  277. ryanofsky referenced this in commit 5f469c9103 on Mar 11, 2025
  278. ryanofsky referenced this in commit 244d430b51 on Mar 11, 2025
  279. ryanofsky referenced this in commit 15adb64db8 on Mar 11, 2025
  280. ryanofsky referenced this in commit 4599760cc6 on Mar 11, 2025
  281. ryanofsky referenced this in commit 69b14c8122 on Mar 11, 2025
  282. ryanofsky force-pushed on Mar 12, 2025
  283. DrahtBot removed the label Needs rebase on Mar 12, 2025
  284. DrahtBot added the label Needs rebase on Mar 14, 2025
  285. ryanofsky force-pushed on Mar 20, 2025
  286. DrahtBot removed the label Needs rebase on Mar 20, 2025
  287. DrahtBot added the label Needs rebase on Mar 27, 2025
  288. ryanofsky referenced this in commit 1069ffddc0 on Jul 31, 2025
  289. ryanofsky referenced this in commit 9334d83c79 on Jul 31, 2025
  290. ryanofsky referenced this in commit 5f1f23bc17 on Jul 31, 2025
  291. ryanofsky referenced this in commit 8b892d41fd on Jul 31, 2025
  292. ryanofsky force-pushed on Aug 1, 2025
  293. DrahtBot removed the label Needs rebase on Aug 2, 2025
  294. DrahtBot added the label Needs rebase on Aug 8, 2025
  295. ryanofsky force-pushed on Oct 15, 2025
  296. DrahtBot removed the label Needs rebase on Oct 15, 2025
  297. ryanofsky referenced this in commit 8423d62a98 on Oct 28, 2025
  298. ryanofsky referenced this in commit 90b6a005d2 on Oct 28, 2025
  299. DrahtBot added the label Needs rebase on Nov 4, 2025
  300. ryanofsky referenced this in commit f3ec0bd5e8 on Nov 11, 2025
  301. ryanofsky referenced this in commit 2c77a46ccc on Nov 11, 2025
  302. ryanofsky referenced this in commit aa64b5bba0 on Nov 11, 2025
  303. ryanofsky referenced this in commit bddf91ae9b on Nov 11, 2025
  304. ryanofsky force-pushed on Nov 11, 2025
  305. DrahtBot added the label CI failed on Nov 11, 2025
  306. DrahtBot commented at 11:15 PM on November 11, 2025: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task test each commit: https://github.com/bitcoin/bitcoin/actions/runs/19279987246/job/55128809872</sub> <sub>LLM reason (✨ experimental): Compilation failed due to a missing/renamed API: ChainstateLoadStatus and its SUCCESS member no longer exist in node, causing multiple compile errors.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

    </details>

  307. DrahtBot removed the label Needs rebase on Nov 11, 2025
  308. ryanofsky force-pushed on Nov 12, 2025
  309. DrahtBot added the label Needs rebase on Dec 9, 2025
  310. ryanofsky referenced this in commit 4fccfc42ae on Dec 12, 2025
  311. ryanofsky referenced this in commit c382c3d104 on Dec 12, 2025
  312. ryanofsky referenced this in commit 9f5e68e872 on Dec 12, 2025
  313. ryanofsky referenced this in commit 1cdab246de on Dec 12, 2025
  314. ryanofsky force-pushed on Dec 12, 2025
  315. DrahtBot removed the label Needs rebase on Dec 12, 2025
  316. DrahtBot added the label Needs rebase on Dec 15, 2025
  317. ryanofsky referenced this in commit 7a25b428ab on Dec 16, 2025
  318. ryanofsky referenced this in commit a06c7b8bb3 on Dec 16, 2025
  319. ryanofsky referenced this in commit 7214d8f06e on Dec 16, 2025
  320. ryanofsky referenced this in commit 2ddc060120 on Dec 16, 2025
  321. ryanofsky force-pushed on Dec 16, 2025
  322. DrahtBot removed the label Needs rebase on Dec 16, 2025
  323. DrahtBot removed the label CI failed on Dec 16, 2025
  324. DrahtBot added the label Needs rebase on Dec 16, 2025
  325. ryanofsky referenced this in commit d0c8b9575f on Jan 7, 2026
  326. ryanofsky referenced this in commit b687ce7628 on Jan 7, 2026
  327. ryanofsky referenced this in commit 37a3d36427 on Jan 7, 2026
  328. ryanofsky referenced this in commit 176196dce3 on Jan 7, 2026
  329. ryanofsky referenced this in commit 873582ff04 on Jan 7, 2026
  330. ryanofsky referenced this in commit 339f0d9f76 on Jan 7, 2026
  331. ryanofsky force-pushed on Jan 17, 2026
  332. DrahtBot removed the label Needs rebase on Jan 17, 2026
  333. DrahtBot added the label CI failed on Jan 17, 2026
  334. ryanofsky force-pushed on Jan 18, 2026
  335. DrahtBot removed the label CI failed on Jan 18, 2026
  336. DrahtBot added the label Needs rebase on Jan 19, 2026
  337. ryanofsky force-pushed on Jan 22, 2026
  338. DrahtBot removed the label Needs rebase on Jan 22, 2026
  339. ryanofsky force-pushed on Jan 22, 2026
  340. DrahtBot added the label CI failed on Jan 22, 2026
  341. DrahtBot commented at 11:47 PM on January 22, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/21266012856/job/61207857203</sub> <sub>LLM reason (✨ experimental): IWYU reported a failure while fixing includes in src/kernel/bitcoinkernel.cpp, causing the CI to fail.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

    </details>

  342. DrahtBot removed the label CI failed on Jan 23, 2026
  343. DrahtBot added the label Needs rebase on Feb 4, 2026
  344. ryanofsky force-pushed on Feb 9, 2026
  345. DrahtBot removed the label Needs rebase on Feb 9, 2026
  346. ryanofsky force-pushed on Feb 9, 2026
  347. DrahtBot added the label CI failed on Feb 9, 2026
  348. DrahtBot commented at 6:06 PM on February 9, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21833918575/job/62999419836</sub> <sub>LLM reason (✨ experimental): Clang-tidy failed the build due to an unused using declaration (using util::Unexpected) in wallet.cpp, causing the CI to fail.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

    </details>

  349. DrahtBot removed the label CI failed on Feb 9, 2026
  350. DrahtBot added the label Needs rebase on Feb 10, 2026
  351. ryanofsky referenced this in commit bc914f08e2 on Mar 2, 2026
  352. ryanofsky referenced this in commit e237f352a4 on Mar 2, 2026
  353. ryanofsky referenced this in commit 9540e2701f on Mar 2, 2026
  354. ryanofsky referenced this in commit 677f66a42f on Mar 2, 2026
  355. ryanofsky referenced this in commit 78f39378e0 on Mar 2, 2026
  356. ryanofsky referenced this in commit d91775c8ea on Mar 2, 2026
  357. 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. On 64-bit platforms, 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.
    e4c8ff6c75
  358. 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
    022c6f5c68
  359. 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.
    443adefa3b
  360. refactor: Use util::Result class in LoadChainstate and VerifyLoadedChainstate 2a4e031f8e
  361. 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
    bc748d0c6f
  362. 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>
    5ef13873a9
  363. Merge branch 'pr/bresult2' into pr/bresult-ptr 2c51c00e22
  364. 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.
    56171227ae
  365. Use ResultPtr<unique_ptr> instead of Result<unique_ptr> e0982ed684
  366. Merge branch 'pr/bresult-ptr' into pr/bresult-load ae8a0fcc47
  367. Add temporary ResultExtract helper for porting to util::Result 62c2d8faa1
  368. refactor: Use util::Result class in wallet/sqlite e18af43fe6
  369. refactor: Use util::Result class in wallet/migrate 26a1639cad
  370. refactor: Use util::Result class in wallet::MakeDatabase 1f57aa9203
  371. refactor: Use util::Result class in wallet/dump 682b8d283b
  372. refactor: Use util::Result class in wallet/wallettool 7392dc11b7
  373. refactor: Use util::Result class in wallet/wallet 707e53e715
  374. refactor: Use util::Result class in wallet/load 6aac228547
  375. refactor: Use util::Result class in wallet/rpc 4d2a684657
  376. refactor: Use util::Result class in wallet/interfaces d71873e041
  377. refactor: Use util::Result class in wallet/test 4cd6dc7252
  378. Drop temporary ResultExtract helper for porting to util::Result 97a3e65ba7
  379. 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-
    b4f69aa545
  380. ryanofsky referenced this in commit 6b3d3822b7 on Mar 31, 2026
  381. ryanofsky referenced this in commit 4ba2953425 on Mar 31, 2026
  382. ryanofsky referenced this in commit 78cd1c0b7d on Mar 31, 2026
  383. ryanofsky referenced this in commit 92b85fc9f6 on Mar 31, 2026
  384. ryanofsky force-pushed on Mar 31, 2026
  385. DrahtBot added the label CI failed on Mar 31, 2026
  386. DrahtBot removed the label Needs rebase on Apr 1, 2026
  387. ci: Avoid -Wno-error=maybe-uninitialized false positives
    Avoid false positive maybe-uninitialized errors in native_previous_releases
CI job.

CI errors look like:
https://github.com/bitcoin/bitcoin/actions/runs/23822910072/job/69439270277?pr=25722

/home/admin/actions-runner/_work/_temp/src/wallet/sqlite.cpp: In member function β€˜util::Result<void> wallet::SQLiteDatabase::Verify()’:
/home/admin/actions-runner/_work/_temp/src/wallet/sqlite.cpp:210:5: error: β€˜*(int*)((char*)&read_user_ver + offsetof(util::Result<int, void, util::Messages>,util::Result<int, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<int, void, util::Messages>::<unnamed>))’ may be used uninitialized [-Werror=maybe-uninitialized]
  210 |     if (user_ver != WALLET_SCHEMA_VERSION) {
      |     ^~
/home/admin/actions-runner/_work/_temp/src/wallet/sqlite.cpp:204:10: note: β€˜*(int*)((char*)&read_user_ver + offsetof(util::Result<int, void, util::Messages>,util::Result<int, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<int, void, util::Messages>::<unnamed>))’ was declared here
  204 |     auto read_user_ver{ReadPragmaInteger(m_db, "user_version", "sqlite wallet schema version") >> result};
      |          ^~~~~~~~~~~~~
/home/admin/actions-runner/_work/_temp/src/wallet/sqlite.cpp:196:23: error: β€˜*(int*)((char*)&read_app_id + offsetof(util::Result<int, void, util::Messages>,util::Result<int, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<int, void, util::Messages>::<unnamed>))’ may be used uninitialized [-Werror=maybe-uninitialized]
  196 |     uint32_t app_id = static_cast<uint32_t>(*read_app_id);
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/admin/actions-runner/_work/_temp/src/wallet/sqlite.cpp:191:10: note: β€˜*(int*)((char*)&read_app_id + offsetof(util::Result<int, void, util::Messages>,util::Result<int, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<int, void, util::Messages>::<unnamed>))’ was declared here
  191 |     auto read_app_id{ReadPragmaInteger(m_db, "application_id", "the application id") >> result};
      |          ^~~~~~~~~~~
cc1plus: all warnings being treated as errors
    89c866499c
  388. ryanofsky force-pushed on Apr 1, 2026
  389. DrahtBot removed the label CI failed on Apr 1, 2026
Contributors
ryanofskyDrahtBotmartinusseditedmaflcko
Labels
Refactoring
Linked (view graph)
#25665 refactor: Add util::Result failure types and ability to merge result values#26022 Add util::ResultPtr class#31362 kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate`
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me