Add util::Expected (std::expected)

maflcko commented at 2:36 pm on December 4, 2025: member

Some low-level code could benefit from being able to use std::expected from C++23:

Currently, some code is using std::optional<E> to denote an optional error. This is fine, but a bit confusing, because std::optional is normally used for values, not errors. Using std::expected<void, E> is clearer.
Currently, some code is using std::variant<V, E> to denote either a value or an error. This is fine, but a bit verbose, because std::variant requires a visitor or get_if/holds_alternative instead of a simple call of the operator bool for std::expected.

In theory, util::Result could be taught to behave similar to std::expected (see #34005). However, it is unclear if this is the right approach:

util::Result is mostly meant for higher level code, where errors come with translated error messages.
std::expected is mostly meant for lower level code, where errors could be an enum, or any other type.
#25665 aims to minimize the memory footprint of the error by wrapping it in a unique_ptr internally. std::expected requires the value and error to be “nested within it” (https://cplusplus.github.io/LWG/issue4141). So from a memory-layout perspective, the two are not compatible.
std::expected also comes with std::unexpected, which also does not map cleanly to util::Result.

So just add a minimal drop-in port of std::expected.

DrahtBot commented at 2:36 pm on December 4, 2025: 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/34006.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	romanz
Concept ACK	Sjors, hodlinator, arejula27
Approach ACK	stickies-v
Stale ACK	ryanofsky, sedited

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33160 (bench: Add more realistic Coin Selection Bench by murchandamus)
#28690 (build: Introduce internal kernel library by sedited)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values 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.

sedited commented at 3:16 pm on December 4, 2025: contributor

Concept ACK

Sjors commented at 3:19 pm on December 4, 2025: member

Concept ACK

FetchBlock() is a good usage example.

maflcko force-pushed on Dec 4, 2025

DrahtBot added the label CI failed on Dec 4, 2025

purpleKarrot commented at 5:12 pm on December 4, 2025: contributor

Some low-level code could benefit from being able to use std::expected from C++23.

Can you elaborate? How would it benefit? Over what alternative?

maflcko commented at 5:29 pm on December 4, 2025: member

Some low-level code could benefit from being able to use std::expected from C++23.

Can you elaborate? How would it benefit? Over what alternative?

Sure. The alternatives were listed in doc diff in the commit, but I’ve pulled it out and put in the pull description as well.

DrahtBot removed the label CI failed on Dec 4, 2025

purpleKarrot commented at 6:07 pm on December 4, 2025: contributor

Some low-level code could benefit from being able to use std::expected from C++23.

Can you elaborate? How would it benefit? Over what alternative?

Sure. The alternatives were listed in doc diff in the commit, but I’ve pulled it out and put in the pull description as well.

What is wrong with the most obvious approach to error handling?

ryanofsky commented at 6:54 pm on December 4, 2025: contributor

Concept ACK. We have a lot of low-level code where std::expected would be useful, and this PR enables that without having to wait for C++23. (#34005 is another PR that does the same thing and seems like a good approach as well.)

I do think util::Result can provide a lot of useful functionality for higher-level code that std::expected doesn’t, and I’ve been testing that idea in various PRs (#25665, #25722, #29700, #26022). But I think the fate of the util::Result class is a separate question, and that the classes can be compatible and each used where they make sense.

stickies-v commented at 8:26 pm on December 4, 2025: contributor

Concept ACK. Functions with return type std::optional<E> returning a truthy value on failure is unintuitive, this is much better.

sedited commented at 7:26 am on December 5, 2025: contributor

Re #34006 (comment)

What is wrong with the most obvious approach to error handling?

I am guessing you are asking why not just use exceptions? If so, I think they could be an alternative for these situations too. I’d be curious how a similar function contract could be enforced with an exception.

maflcko commented at 8:24 am on December 5, 2025: member

just use exceptions?

I don’t think exceptions are a suitable alternative for low-level code in this repo, where std::expected is appropriate. In places where exceptions are used today, it is tedious to review the code and its behavior. For example, the same exception from an IO error in a database will sometimes terminate the program unclean, and sometimes cleanly shut down the program (https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116540946), depending on which thread or with module called the function that threw.

If you are referring to P3166, then I don’t think it made it into C++26 (https://github.com/cplusplus/papers/issues/1829#issuecomment-2983235299), so I don’t think it is a practical alternative in places where std::expected is appropriate.

I am not saying there is no use-case for exceptions today, but I don’t think the presence of exceptions is a valid reason to rule out the use of std::expected.

in src/rpc/blockchain.cpp:539 in fa1eee4f46 outdated

535@@ -536,8 +536,8 @@ static RPCHelpMan getblockfrompeer()
536         throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded");
537     }
538 
539-    if (const auto err{peerman.FetchBlock(peer_id, *index)}) {
540-        throw JSONRPCError(RPC_MISC_ERROR, err.value());
541+    if (const auto res{peerman.FetchBlock(peer_id, *index)}; !res) {

hodlinator commented at 12:02 pm on December 5, 2025:

This example shows that replacing std::optional<ErrorType> with util::Expected<void, ErrorType> is not totally risk-free as the old condition would still compile but be inverted. err.value() on the next line would have failed in this case though since it would return void which is an invalid argument type to the JSONRPCError-ctor.

It’s tempting to add constraints to value() like so:

 0    constexpr const T& value() const LIFETIMEBOUND
 1        requires(!std::is_same_v<V, void>)
 2    {
 3        assert(has_value());
 4        return std::get<0>(m_data);
 5    }
 6    constexpr T& value() LIFETIMEBOUND
 7        requires(!std::is_same_v<V, void>)
 8    {
 9        assert(has_value());
10        return std::get<0>(m_data);
11    }

But I think that makes replacing util::Expected with std::expected in the future a hard-fork.

maflcko commented at 12:45 pm on December 5, 2025:

This example shows that replacing std::optional<ErrorType> with util::Expected<void, ErrorType> is not totally risk-free as the old condition would still compile but be inverted.

Yes, the inversions is one of the reasons to do this change. I agree there is a risk, but is should be pretty obvious for reviewers to look out for. Also, hopefully the cases are rare where the value type and the error type are close enough to make value() and error() swappable at the call-site.

constraints

Seems fine to add them, because I don’t think there will ever be code to unwrap a void result, but as you say, they don’t exist in std::expected, so I’ll leave as-is for now.

hodlinator commented at 12:14 pm on December 5, 2025: contributor

Concept ACK fa1eee4f46fd089d6e19b124addee7c7679585f4

I prefer introducing util::Expected as a distinct type from util::Result, with a clear upgrade path to std::expected once we have C++23. (Maybe util::Result can later be implemented as a special case of util::Expected).

Recently suggested std::variant in another PR instead of using out-parameters (https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2584393520) but would strongly prefer util::Expected, had it existed.

in src/net_processing.h:105 in fa1eee4f46 outdated

103@@ -103,7 +104,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
104      * @param[in]  block_index  The blockindex
105      * @returns std::nullopt if a request was successfully made, otherwise an error message

ryanofsky commented at 12:54 pm on December 5, 2025:

In commit “Add util::Expected (std::expected)” (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Looks like comment needs to be updated

maflcko commented at 2:08 pm on December 5, 2025:

thx, removed this line

in src/util/expected.h:40 in fa1eee4f46 outdated

32+private:
33+    using T = std::conditional_t<std::is_same_v<V, void>, std::monostate, V>;
34+    std::variant<T, E> m_data;
35+
36+public:
37+    constexpr Expected() : m_data{std::in_place_index_t<0>{}, std::monostate{}} {} // constructor for void

ryanofsky commented at 1:03 pm on December 5, 2025:

In commit “Add util::Expected (std::expected)” (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Is there a reason to use std::monostate{} here instead of T{}? Actual std::expected class seems to allow default construction

maflcko commented at 2:08 pm on December 5, 2025:

thx, used T{}

in src/util/expected.h:52 in fa1eee4f46 outdated

44+    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
45+    constexpr explicit operator bool() const noexcept { return has_value(); }
46+
47+    constexpr const T& value() const LIFETIMEBOUND
48+    {
49+        assert(has_value());

hodlinator commented at 1:03 pm on December 5, 2025:

Requires #include <cassert>.

maflcko commented at 1:30 pm on December 5, 2025:

thx, added 3 includes. Let’s see what the iwyu ci thinks.

Edit:

0
1(/home/admin/actions-runner/_work/_temp/src/util/expected.h has correct #includes/fwd-decls)
2
3(/home/admin/actions-runner/_work/_temp/src/util/expected.cpp has correct #includes/fwd-decls)

https://github.com/bitcoin/bitcoin/actions/runs/19964480963/job/57252616519?pr=34006#step:11:25408

in src/util/expected.h:36 in fa1eee4f46 outdated

28+/// can be added, if needed.
29+template <class V, class E>
30+class Expected
31+{
32+private:
33+    using T = std::conditional_t<std::is_same_v<V, void>, std::monostate, V>;

ryanofsky commented at 1:17 pm on December 5, 2025:

In commit “Add util::Expected (std::expected)” (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Normally T is a template parameter and internal types like this have more descriptive names. Would suggest renaming V to T to match std::expected documentation and T to ValueType or similar

maflcko commented at 2:08 pm on December 5, 2025:

thx, done

in src/util/expected.h:50 in fa1eee4f46 outdated

42+    }
43+
44+    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
45+    constexpr explicit operator bool() const noexcept { return has_value(); }
46+
47+    constexpr const T& value() const LIFETIMEBOUND

ryanofsky commented at 1:19 pm on December 5, 2025:

In commit “Add util::Expected (std::expected)” (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Might be better to use V instead f T here. My concern is just that with Expected<void> this will leak std::monostate and monostate should be a private implementation detail. Similarly for other accessors below returning T

maflcko commented at 2:08 pm on December 5, 2025:

You are right, but I can’t see this going wrong in practise, so I’ll leave this as-is for now and postpone to a follow-up. (The solution would probably be to specialize, but that requires writing a lot more code)

in src/test/util_expected_tests.cpp:19 in fa1eee4f46 outdated

14+BOOST_AUTO_TEST_CASE(expected_value)
15+{
16+    struct Obj {
17+        int x;
18+    };
19+    Expected<Obj, int> e{Obj{42}};

ryanofsky commented at 1:22 pm on December 5, 2025:

In commit “Add util::Expected (std::expected)” (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Maybe add a test for a default constructed instance (if this will be supported)

maflcko commented at 2:08 pm on December 5, 2025:

thx, done

in src/util/expected.h:72 in fa1eee4f46 outdated

64+    {
65+        assert(!has_value());
66+        return std::get<1>(m_data);
67+    }
68+
69+    constexpr T& operator*() { return value(); }

ryanofsky commented at 1:24 pm on December 5, 2025:

In commit “Add util::Expected (std::expected)” (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Should these have LIFETIMEBOUND annotations?

maflcko commented at 2:08 pm on December 5, 2025:

thx, done

in src/util/result.h:25 in fa1eee4f46

20@@ -21,9 +21,9 @@ struct Error {
21 //!
22 //! It is intended for high-level functions that need to report error strings to
23 //! end users. Lower-level functions that don't need this error-reporting and
24-//! only need error-handling should avoid util::Result and instead use standard
25-//! classes like std::optional, std::variant, and std::tuple, or custom structs
26-//! and enum types to return function results.
27+//! that only need error-handling should avoid util::Result and instead use
28+//! util::Expected, or custom structs and enum types to return function

ryanofsky commented at 1:29 pm on December 5, 2025:

In commit “Add util::Expected (std::expected)” (fa1eee4f46fd089d6e19b124addee7c7679585f4)

IMO std::optional and std::variant are still appropriate to use in many cases and it would nice to keep mentioning them.

maflcko commented at 2:08 pm on December 5, 2025:

thx, done

maflcko force-pushed on Dec 5, 2025

ryanofsky approved

ryanofsky commented at 1:40 pm on December 5, 2025: contributor

Code review ACK fa80cca5dd193668300df891876287dacca6159e. Looks great! This is a minimal implementation of the std::expected class that may be marginally less efficient (doesn’t have a void specialization, may do some unnecessary moves/copies) and lacks some features, but is usable and can be expanded, and is basically as simple as possible. I left some comments and suggestions below but none are important. I also updated the #25665 PR description to compare the Result and Expected classes.

DrahtBot requested review from hodlinator on Dec 5, 2025

DrahtBot requested review from sedited on Dec 5, 2025

DrahtBot requested review from stickies-v on Dec 5, 2025

DrahtBot requested review from Sjors on Dec 5, 2025

maflcko force-pushed on Dec 5, 2025

DrahtBot added the label CI failed on Dec 5, 2025

ryanofsky approved

ryanofsky commented at 2:41 pm on December 5, 2025: contributor

Code review ACK fa76a6620012fb738639d8fd7ce17b185bfd376c

in src/util/expected.h:52 in fa76a66200 outdated

47+    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
48+    constexpr explicit operator bool() const noexcept { return has_value(); }
49+
50+    constexpr const ValueType& value() const LIFETIMEBOUND
51+    {
52+        assert(has_value());

hodlinator commented at 2:47 pm on December 5, 2025:

nit: Not a fan of exceptions, and agree that it is a logic error to call value() or error() without first checking the state. But I think it might be slightly safer to throw here and below instead of asserting, so we have less of a behavior change once util::Expected is replaced by std::expected.

Otherwise users might experience changes under rare/improbable conditions where the process would shut down/crash before, and after the switch it would catch the exception and continue.

maflcko commented at 9:29 am on December 6, 2025:

I see your point, but exactly the same scenario can happen without this pull, when an util::Result is replaced by std::expected. (And the inverse can happen when going the other way)

I don’t feel strongly, but I think it is minimally safer to assert here, because for our code-base it is a programming logic error to call value when a value does not exist. So it seems minimally better to be able to detect those cases more easily while we have Expected (before switching to std::expected)

I understand the stdlib went a different path, looking at https://en.cppreference.com/w/cpp/experimental/optional/bad_optional_access.html, which initially inherited from logic_error, but later (https://en.cppreference.com/w/cpp/utility/bad_optional_access.html) it was only derived from std::exception.

In any case, I don’t feel strongly, and I don’t think (and hope) any of this will matter in practise, and I am happy to push this patch, if you insist:

 0diff --git a/src/util/expected.h b/src/util/expected.h
 1index 6938c97..081c263 100644
 2--- a/src/util/expected.h
 3+++ b/src/util/expected.h
 4@@ -8,6 +8,7 @@
 5 #include <attributes.h>
 6 
 7 #include <cassert>
 8+#include <exception>
 9 #include <type_traits>
10 #include <utility>
11 #include <variant>
12@@ -24,6 +25,11 @@ public:
13     E err;
14 };
15 
16+struct BadExpectedAccess : std::exception
17+{
18+    const char* what() const noexcept override { return "Bad util::Expected access"; }
19+};
20+
21 /// The util::Expected class provides a standard way for low-level functions to
22 /// return either error values or result values.
23 ///
24@@ -49,12 +55,16 @@ public:
25 
26     constexpr const ValueType& value() const LIFETIMEBOUND
27     {
28-        assert(has_value());
29+        if (!has_value()) {
30+            throw BadExpectedAccess{};
31+        }
32         return std::get<0>(m_data);
33     }
34     constexpr ValueType& value() LIFETIMEBOUND
35     {
36-        assert(has_value());
37+        if (!has_value()) {
38+            throw BadExpectedAccess{};
39+        }
40         return std::get<0>(m_data);
41     }
42

DrahtBot requested review from hodlinator on Dec 5, 2025

DrahtBot removed the label CI failed on Dec 5, 2025

in src/util/expected.h:24 in fa76a66200 outdated

19+template <class E>
20+class Unexpected
21+{
22+public:
23+    constexpr explicit Unexpected(E e) : err(std::move(e)) {}
24+    E err;

stickies-v commented at 4:55 pm on December 5, 2025:

std::unexpected exposes std::unexpected::error() instead of .err. Since this is a public member, it might get used and thus make drop-in replacements a bit more involved?

maflcko commented at 7:26 am on December 6, 2025:

I know, and this was intentional. I can’t imagine a real code snippet in this code-base that uses .err or .error(). The only use-case of Unexpected is to serve as a marker, which Expected constructor to pick internally. Am I missing something?

stickies-v commented at 12:08 pm on December 6, 2025:

I haven’t used std::{un}expected yet in practice, so I’m not really familiar with real life use cases yet. From research, one use case that seems plausible is to write a handler for failure cases (e.g. to log and then abort). Templating that function on std::unexpected<E> seems sensible? There are of course plenty of other ways to write that handler, but my point is that because it’s public people might do it, and then that’ll break drop-in replacement. Regardless, it probably will be a rare occurence, and it should be an easy fix when we switch to std::expected, so I’m not worried either way. I agree with the approach of this PR to not complicate the implementation any more than necessary. Up to you what you prefer.

maflcko commented at 12:27 pm on December 6, 2025:

Regardless, it probably will be a rare occurence, and it should be an easy fix when we switch to std::expected, so I’m not worried either way.

Same. Will leave as-is for now, but happy to push any diff, or review a follow-up, if there is need.

in src/net_processing.h:106 in fa76a66200 outdated

101@@ -101,9 +102,8 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
102      *
103      * @param[in]  peer_id      The peer id
104      * @param[in]  block_index  The blockindex
105-     * @returns std::nullopt if a request was successfully made, otherwise an error message
106      */
107-    virtual std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0;
108+    virtual util::Expected<void, std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0;

stickies-v commented at 5:04 pm on December 5, 2025:

This should probably be [[nodiscard]]? Could perhaps more generally enforce that with bugprone-unused-return-value for std::expected?

maflcko commented at 9:30 am on December 6, 2025:

Thanks. Very nice, pushed a commit to do that.

stickies-v commented at 5:07 pm on December 5, 2025: contributor

Approach ACK fa76a6620012fb738639d8fd7ce17b185bfd376c

in src/net_processing.h:14 in fa76a66200 outdated

10@@ -11,6 +11,7 @@
11 #include <node/txorphanage.h>
12 #include <protocol.h>
13 #include <threadsafety.h>
14+#include <util/expected.h>

arejula27 commented at 7:10 pm on December 5, 2025:

I wouldn’t modify src/net_processing.h in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use std::optional for errors, for example, line 828 in src/netbase.cpp. Updating only these functions and not the others feels arbitrary and unrelated to the scope of this PR. I suggest moving these changes to a separate PR and making the update more exhaustive.

maflcko commented at 7:23 am on December 6, 2025:

I wouldn’t modify src/net_processing.h in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use std::optional for errors, for example, line 828 in src/netbase.cpp.

No, it is not. This is a valid and intended use of std::optional:

0    std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
1    if (addr.has_value()) {

I’ll leave as-is for now. Also, I’ll keep touching the file, so that a valid and intended use of std::expected can be seen.

Sjors commented at 9:05 am on December 6, 2025:

FetchBlock() is only used by the getblockfrompeer RPC (which I introduced in #20295), so it’s a nice and fairly isolated example to use.

Typically we gradually replace old code with more modern patterns while editing something in the same area, so the other examples will follow later.

in src/util/result.h:22 in fa76a66200 outdated

20@@ -21,8 +21,8 @@ struct Error {
21 //!
22 //! It is intended for high-level functions that need to report error strings to

arejula27 commented at 7:19 pm on December 5, 2025:

I think the current wording is a bit vague. It might be rewritten along the lines of:

0//! It is intended for high-level functions that need to report error strings **directly**
1//! to end users. Any lower-level functions that don't need this error-reporting...

However, Result in our codebase isn’t limited to reporting errors to end users. As @ryanofsky noted in PR #25665, there are broader use cases. One strong example is:

Result may also be a better choice than std::expected when returning pointer values. (Followup #26022 adds a ResultPtr specialization that avoids double dereferencing.)

I think these considerations should also be part of the discussion for this PR, and we should agree on the responsibilities of each type. Consequently, the documentation should be updated to reflect these broader use cases more accurately.

maflcko commented at 7:24 am on December 6, 2025:

However, Result in our codebase isn’t limited to reporting errors to end users. As @ryanofsky noted in PR #25665, there are broader use cases. One strong example is:

Result may also be a better choice than std::expected when returning pointer values. (Followup #26022 adds a ResultPtr specialization that avoids double dereferencing.)

Thx, this is a good point. However, I think modifying the Result and Expected docs for ResultPtr is unrelated to this pull and can be done in the pull introducing ResultPtr.

I’ll leave as-is for now.

arejula27 commented at 10:12 am on December 6, 2025:

Agree, I started with your review, and now I am doing #25665. The doc improvement I suggested feels more suitable in the other one; however, I would like you to consider adding directly as I mentioned, as you already did some small text corrections on results.h

arejula27 commented at 7:27 pm on December 5, 2025: none

Concept ACK [fa76a6620012fb738639d8fd7ce17b185bfd376c]

This PR is valuable because it creates a smooth path toward adopting the new C++23 standard. Instead of requiring a full refactor to std::expected all at once, it allows the codebase to transition gradually, simply by replacing utils::expected with std::expected later on.

I also like the idea of introducing this error handling paradigm into the repository. expected is a clean way to handle errors and aligns with the modern programming languages such as Rust, where a function returns either a value or an error. By contrast, try/catch approach (also mentioned in the discussion) often lead to poor error handling, either vague error messages or overly complex and hard-to-reason-about control flow. Using an explicit result type avoids these issues.

std::optional is currently used in this repo as an approximation of this pattern, but it isn’t a correct fit. A function can succeed with a value, succeed without a value, or fail with an error, all of which optional cannot distinguish cleanly.

Looking forward, I’d like to consider using only std::expected throughout the codebase, with utils::Result becoming a thin wrapper around std::expected only when additional logging is required. Alternatively, instead of utils::Result we could provide static helper functions, which consumes std::expected to make error logging user-friendly. Unifying the error-handling approach would make maintenance and review easier, since developers wouldn’t need to think about which mechanism is appropriate for each function, there would be just one.

However, as I mentioned in an inline comment, there are also good arguments in favor of keeping both types. If we go in that direction, I would suggest improving the documentation and adding a constructor (or conversion path) from utils::expected to utils::Result. This would make it easier to use both patterns together: utils::expected for nested or lower-level functions, and utils::Result only at the top level where a user-facing error message is ultimately needed.

DrahtBot requested review from stickies-v on Dec 5, 2025

sedited approved

sedited commented at 9:18 am on December 6, 2025: contributor

ACK fa76a6620012fb738639d8fd7ce17b185bfd376c

DrahtBot requested review from arejula27 on Dec 6, 2025

maflcko force-pushed on Dec 6, 2025

DrahtBot added the label CI failed on Dec 6, 2025

maflcko force-pushed on Dec 6, 2025

arejula27 commented at 10:41 am on December 6, 2025: none

@maflcko Do you think it will be interesting to follow the std::expected class methods and add:

I think it will be valuable and will give more flexibility in error handling cases

DrahtBot removed the label CI failed on Dec 6, 2025

maflcko force-pushed on Dec 6, 2025

maflcko commented at 11:49 am on December 6, 2025: member

@maflcko Do you think it will be interesting to follow the std::expected class methods and add:
* [value_or](https://en.cppreference.com/w/cpp/utility/expected/value_or.html)

* [error_or](https://en.cppreference.com/w/cpp/utility/expected/error_or.html)
I think it will be valuable and will give more flexibility in error handling cases

I don’t see a valid use-case for error_or, but i guess value_or may come in handy possibly in the future. Other than that, please refer to the docs:

0/// The util::Expected class provides a standard way for low-level functions to
1/// return either error values or result values.
2///
3/// It provides a smaller version of std::expected from C++23. Missing features
4/// can be added, if needed.

Add util::Expected (std::expected) fa114be27b

refactor: Enable clang-tidy bugprone-unused-return-value

This requires some small refactors to silence false-positive warnings.

Also, expand the bugprone-unused-return-value.CheckedReturnTypes option
to include util::Result, and util::Expected.

faa23738fc

maflcko force-pushed on Dec 6, 2025

DrahtBot added the label CI failed on Dec 6, 2025

DrahtBot commented at 12:07 pm on December 6, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/19988056440/job/57324791049 LLM reason (✨ experimental): clang-tidy failure: use-after-move detected in util_expected_tests.cpp, causing CI to fail.

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.

DrahtBot removed the label CI failed on Dec 6, 2025

romanz commented at 2:35 pm on December 6, 2025: contributor

tACK faa23738fc

Works with #33657 (rebased over this PR).

DrahtBot requested review from sedited on Dec 6, 2025

DrahtBot requested review from ryanofsky on Dec 6, 2025

in src/ipc/test/ipc_test.cpp:67 in faa23738fc

62@@ -63,7 +63,9 @@ void IpcPipeTest()
63         auto foo_client = std::make_unique<mp::ProxyClient<gen::FooInterface>>(
64             connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs<gen::FooInterface>(),
65             connection_client.get(), /* destroy_connection= */ true);
66-        connection_client.release();
67+        {
68+            [[maybe_unused]] auto _{connection_client.release()};

sedited commented at 4:07 pm on December 6, 2025:

Can’t this be released when creating the ProxyClient? Looks like destroy_connection being true means the client is taking ownership of the connection.

DrahtBot requested review from sedited on Dec 6, 2025

Add util::Expected (std::expected) #34006

Code Coverage & Benchmarks

Reviews

Conflicts