util: Add some more Unexpected and Expected methods #34032

maflcko · 2025-12-09T08:01:34Z

Reviewers requested more member functions In #34006.

They are currently unused, but bring the port closer to the original std::expected implementation:

Make Expected::value() throw when no value exists
Add Unexpected::error() methods
Add Expected<void, E> specialization
Add Expected::value()&& and Expected::error()&& methods
Add Expected::swap()

Also, include a tiny tidy fixup:

tidy: Set AllowCastToVoid in the bugprone-unused-return-value check

DrahtBot · 2025-12-09T08:01:41Z

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/34032.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, stickies-v
Stale ACK	hodlinator

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34264 (fuzz: Extend spend coverage by Chand-ra)
#34208 (bench: add fluent API for untimed setup steps in nanobench by l0rinc)
#33689 (http: replace WorkQueue and single threads handling for ThreadPool by furszy)

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.

src/util/expected.h

ryanofsky

Code review ACK fa20a50. Thanks for the followups.

I'd probably replace std::variant<std::monostate, E> with std::optional<E> but both seem reasonable.

maflcko · 2025-12-09T14:00:08Z

I'd probably replace std::variant<std::monostate, E> with std::optional<E> but both seem reasonable.

I kept the variant, because it is more consistent and shows that the error() methods are just 1:1 copy-pasted between both templates.

maflcko · 2025-12-09T16:38:26Z

pushed a commit, so that the two compile and behave identically:

    const auto moved{*std::move(no_copy)};
    const auto moved{std::move(*no_copy)};

Sorry for missing it earlier, but I think now all && methods are implemented completely and correctly.

hodlinator

ACK fabef9bbef255796f28c04c5b478df9544c749eb

(Caveat: Not yet fully at ease with ampersands after the method parameters such as in the last two commits, example: constexpr E&& error() &&).

src/test/util_expected_tests.cpp

src/util/expected.h

ryanofsky

Code review ACK fa0a1a6. Left some suggestions which I think would make this better in significant ways, but I do this is already an improvement in its current form.

src/util/expected.h

It only makes sense to turn this off with C++26, which introduces the _ placeholder.

hodlinator

re-ACK fa0a1a6

src/util/expected.h

src/test/util_expected_tests.cpp

src/util/expected.h

This is not needed, but a bit closer to the std lib.

hodlinator

re-ACK fa874c9

Thanks for grabbing swap!

src/util/expected.h

ryanofsky

Code review fa874c9. Looks mostly good and thanks for considering the previous suggestions. One thing that I think may be a problem is use of throwing value() method in operator methods declared noexcept but I left a more detailed comment below.

Various changes were made since last review: renaming m_err, getting rid of Assume() in value(), testing void value() specialization, adding noexcepts, adding method & qualifiers, reformatting and using std::get_if, adding swap method.

ryanofsky · 2025-12-17T18:41:07Z

src/util/expected.h

    {
-        assert(has_value());
+        if (!has_value()) {
+            throw BadExpectedAccess{};


In commit "util: Make Expected::value() throw" (fa5a49c)

The change seems unsafe because none of the other functions currently calling value() are switched to use operator* instead, so they will now throw instead of assert if the value is not set. This would make it dangerous to swap the util::Expected class with std::expected in the future.

I'd recommend reconsidering the suggestion from #34032 (comment) and avoiding unnecessary uses of value() and has_value() preferring to use c++'s built in operators, and setting a better example for other code to follow. Alternately more asserts could be added to fix this. This is also sort-of fixed in a later commit fa9aad7 adding noexcept to pointer operators, but asserts would probably lead to clearer runtime errors than noexcept violations.

The change seems unsafe because none of the other functions currently calling value() are switched to use operator* instead, so they will now throw instead of assert if the value is not set. This would make it dangerous to swap the util::Expected class with std::expected in the future.

I understand that switching util::Expected::operator* to std::expected::operator* is unsafe, but this is documented in https://en.cppreference.com/w/cpp/utility/expected/operator%252A.html:

If has_value() is false, the behavior is undefined. (until C++26)

Though, that is true with or without this pull request, so it seems unrelated.

As for this implementation: I think it is perfectly fine to do whatever on UB, and asserting, or throwing seems better than UB.

but asserts would probably lead to clearer runtime errors than noexcept violations.

I don't understand why that would be. The only difference with an assert is that it prints the file name and line number, but this will just be a inside this util header, not in the place in the code where the assertion happens. So reading the tb is needed anyway. For reference, with noexecpt, it looks like:

valgrind --tool=none ./bld-cmake/bin/test_bitcoin -t util_expected_tests/expected_error --catch_system_errors=no ==1207793== Nulgrind, the minimal Valgrind tool ... ==1207793== Command: ./bld-cmake/bin/test_bitcoin -t util_expected_tests/expected_error --catch_system_errors=no ==1207793== Running 1 test case... terminate called after throwing an instance of 'util::BadExpectedAccess' what(): Bad util::Expected access ==1207793== ==1207793== Process terminating with default action of signal 6 (SIGABRT): dumping core ... ==1207793== by 0x4F1A98A: util_expected_tests::expected_error::test_method() (expected.h:117) ==1207793== by 0x4F19B56: util_expected_tests::expected_error_invoker() (./test/util_expected_tests.cpp:79) ... Aborted (core dumped)

I think this is perfectly fine, and I don't see the benefit of the assertion.

@ryanofsky No rush, but if you get a chance to take another look here before branch-off [1], that'd be great. I think the pull is currently blocked because of your use of the spicy word "unsafe" 😅

[1] It would be good to get this in before branch-off. Otherwise, there could be (unlikely) compile-failures or silent performance regressions for backports due to #34032 (comment)

re: #34032 (comment)

@ryanofsky No rush, but if you get a chance to take another look here before branch-off [1], that'd be great. I think the pull is currently blocked because of your use of the spicy word "unsafe" 😅

I'm not saying the pull is unsafe. I'm saying this specific commit fa90c44 is unsafe and the updated commit message is misleading.

This commit is unsafe and the new commit message saying that pointer operators throwing exceptions is "fine" is misleading, because if these operators throw exceptions, code can catch those exceptions, and that code will be broken when Expected is replaced with expected, in bad ways triggering UB, in code paths likely to be missing test coverage because we often don't have tests checking error paths.

However, the pull request as a whole is safe because a later commit fa15f71 adds noexcept. That later commit fixes safety issue apparently by accident because it does not even mention any change in behavior.

IMO, this pull is not in a good state even if it is technically safe. These issues could be fixed at a surface level by adding noexcept in fa90c44 instead of fa15f71. In that case, it would be good to have comments next to these noexcepts, because normally when developers see noexcept we think "exceptions are not expected to be thrown here" not "exceptions are thrown one level down from here but this triggers std::terminate and that is good."

I do think the real mistake here is implementing pointer methods in terms of value() instead of the other way around. Using value() complicates implementation of this class, makes error reporting more confusing and less reliable, and sets a bad example of how you should deal with nullable values in c++. Using value instead of bool/*/-> makes unsafe dereferences look like normal method calls, and negates decades of pattern recognition c/c++ developers have dealing with code with nullable values.

Thanks for the reply. I think I understand your point better now.

These issues could be fixed at a surface level by adding noexcept in fa90c44 instead of fa15f71.

Thanks, done.

normally when developers see noexcept we think "exceptions are not expected to be thrown here" not "exceptions are thrown one level down from here but this triggers std::terminate and that is good."

I think the first interpretation is correct, because no code should exist that dereferences a nullopt, so I'll not add a comment for now.

I do think the real mistake here is implementing pointer methods in terms of value() instead of the other way around. Using value() complicates implementation of this class, makes error reporting more confusing and less reliable, and sets a bad example of how you should deal with nullable values in c++. Using value instead of bool/*/-> makes unsafe dereferences look like normal method calls, and negates decades of pattern recognition c/c++ developers have dealing with code with nullable values.

I guess I just disagree here.

I don't like UB and I think we should move toward eliminating it, so I am not going to re-write this as unsafe code. Just because C++ is know for its UB for decades doesn't mean all future code needs to be unsafe as well.

Also, I disagree that Assert has a nicer error message here than terminate. See my prior reply.

I think the implementation is simple and also uses less code than the alternative.

re: #34032 (comment)

I don't like UB and I think we should move toward eliminating it, so I am not going to re-write this as unsafe code.

If you think something in the suggestion #34032 (comment) is unsafe, it would help to clarify what you are referring to.

Just because C++ is know for its UB for decades doesn't mean all future code needs to be unsafe as well.

We have no disagreement about this. I am not saying it is bad to use value() in general. I think it is great to use value() in code that catches exceptions. However, it is bad to use value() in code that does not catch exceptions because:

It is misleading, not distinguishing between preconditions that are never expected to be violated and expected errors.

It makes unsafe dereferences blend into code looking like normal method calls instead of standing out clearly, bypassing extra scrutiny c/c++ developers give to * and -> uses.

src/util/expected.h

ryanofsky · 2025-12-17T19:01:48Z

src/util/expected.h


-    constexpr T* operator->() LIFETIMEBOUND { return &value(); }
-    constexpr const T* operator->() const LIFETIMEBOUND { return &value(); }
+    constexpr T* operator->() noexcept LIFETIMEBOUND { return &value(); }


In commit "util: Implement Expected::operator*()&&" (fa9aad7)

Would be good for commit message to mention it is also adding noexcept to other overloads

stickies-v

utACK faebb16

stickies-v · 2026-01-05T09:08:47Z

src/util/expected.h

+    constexpr E& error() & noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
+    constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(error()); }
+
+    constexpr void swap(Expected& other) noexcept { m_data.swap(other.m_data); }


review note: std::variant::swap is not unconditionally noexcept but has a specification. However, I think for our minimal implementation just letting it std::terminate is fine.

hodlinator

re-ACK faebb16

Only improved commit messages and made operator* for Expected<void, E>-specialization throw on error since my previous review (#34032 (review))

ryanofsky

Code review ACK faebb16. I really only like the first commit fad4a9f in this PR and think the other commits are neutral or bad (see below), but these changes seem ok if others want them.

ryanofsky · 2026-01-15T14:12:14Z

src/util/expected.h

    {
-        assert(has_value());
+        if (!has_value()) {
+            throw BadExpectedAccess{};


re: #34032 (comment)

@ryanofsky No rush, but if you get a chance to take another look here before branch-off [1], that'd be great. I think the pull is currently blocked because of your use of the spicy word "unsafe" 😅

I'm not saying the pull is unsafe. I'm saying this specific commit fa90c44 is unsafe and the updated commit message is misleading.

This commit is unsafe and the new commit message saying that pointer operators throwing exceptions is "fine" is misleading, because if these operators throw exceptions, code can catch those exceptions, and that code will be broken when Expected is replaced with expected, in bad ways triggering UB, in code paths likely to be missing test coverage because we often don't have tests checking error paths.

However, the pull request as a whole is safe because a later commit fa15f71 adds noexcept. That later commit fixes safety issue apparently by accident because it does not even mention any change in behavior.

IMO, this pull is not in a good state even if it is technically safe. These issues could be fixed at a surface level by adding noexcept in fa90c44 instead of fa15f71. In that case, it would be good to have comments next to these noexcepts, because normally when developers see noexcept we think "exceptions are not expected to be thrown here" not "exceptions are thrown one level down from here but this triggers std::terminate and that is good."

I do think the real mistake here is implementing pointer methods in terms of value() instead of the other way around. Using value() complicates implementation of this class, makes error reporting more confusing and less reliable, and sets a bad example of how you should deal with nullable values in c++. Using value instead of bool/*/-> makes unsafe dereferences look like normal method calls, and negates decades of pattern recognition c/c++ developers have dealing with code with nullable values.

This is not expected to be needed in this codebase, but brings the implementation closer to std::expected::value(). Also, add noexcept, where std::expected has them. This will make operator-> and operator* terminate, when has_value() is false.

This is not needed, but a bit closer to the std lib, because std::monostate is no longer leaked through ValueType from the value() method.

They are currently unused, but implementing them is closer to the std::expected.

It is currently unused, but implementing it is closer to std::expected.

maflcko · 2026-01-15T15:17:04Z

Force pushed something. Should be trivial to re-review, because nothing changed:

$ git diff faebb16547 faa59b3679 | wc -l
0

ryanofsky

Code review ACK faa59b3. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.

ryanofsky · 2026-01-15T15:54:56Z

src/util/expected.h

    {
-        assert(has_value());
+        if (!has_value()) {
+            throw BadExpectedAccess{};


re: #34032 (comment)

I don't like UB and I think we should move toward eliminating it, so I am not going to re-write this as unsafe code.

If you think something in the suggestion #34032 (comment) is unsafe, it would help to clarify what you are referring to.

Just because C++ is know for its UB for decades doesn't mean all future code needs to be unsafe as well.

We have no disagreement about this. I am not saying it is bad to use value() in general. I think it is great to use value() in code that catches exceptions. However, it is bad to use value() in code that does not catch exceptions because:

It is misleading, not distinguishing between preconditions that are never expected to be violated and expected errors.

It makes unsafe dereferences blend into code looking like normal method calls instead of standing out clearly, bypassing extra scrutiny c/c++ developers give to * and -> uses.

stickies-v · 2026-01-16T11:32:25Z

re-ACK faa59b3

DrahtBot added the Utils/log/libs label Dec 9, 2025

maflcko mentioned this pull request Dec 9, 2025

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

Merged

ryanofsky reviewed Dec 9, 2025

View reviewed changes

src/util/expected.h Show resolved Hide resolved

ryanofsky approved these changes Dec 9, 2025

View reviewed changes

hodlinator approved these changes Dec 10, 2025

View reviewed changes

src/test/util_expected_tests.cpp Outdated Show resolved Hide resolved

src/util/expected.h Outdated Show resolved Hide resolved

src/util/expected.h Show resolved Hide resolved

DrahtBot requested a review from ryanofsky December 10, 2025 15:42

maflcko force-pushed the 2512-exp branch from fabef9b to fa0a1a6 Compare December 10, 2025 16:29

ryanofsky approved these changes Dec 10, 2025

View reviewed changes

src/util/expected.h Outdated Show resolved Hide resolved

src/util/expected.h Outdated Show resolved Hide resolved

src/util/expected.h Show resolved Hide resolved

DrahtBot requested a review from hodlinator December 10, 2025 18:33

hodlinator mentioned this pull request Dec 11, 2025

rest: allow reading partial block data from storage #33657

Merged

MarcoFalke added 2 commits December 11, 2025 09:46

Set bugprone-unused-return-value.AllowCastToVoid

fad4a9f

It only makes sense to turn this off with C++26, which introduces the _ placeholder.

test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK ==

faa109f

hodlinator reviewed Dec 11, 2025

View reviewed changes

src/util/expected.h Outdated Show resolved Hide resolved

src/util/expected.h Show resolved Hide resolved

src/test/util_expected_tests.cpp Show resolved Hide resolved

src/util/expected.h Outdated Show resolved Hide resolved

util: Add Unexpected::error()

fa1de11

This is not needed, but a bit closer to the std lib.

maflcko force-pushed the 2512-exp branch 2 times, most recently from fa6fcae to fa874c9 Compare December 11, 2025 09:47

DrahtBot added the CI failed label Dec 11, 2025

hodlinator approved these changes Dec 11, 2025

View reviewed changes

src/util/expected.h Show resolved Hide resolved

src/util/expected.h Show resolved Hide resolved

DrahtBot requested a review from ryanofsky December 11, 2025 10:43

DrahtBot removed the CI failed label Dec 11, 2025

maflcko requested a review from stickies-v December 15, 2025 11:14

ryanofsky reviewed Dec 17, 2025

View reviewed changes

DrahtBot requested a review from ryanofsky December 17, 2025 19:13

maflcko force-pushed the 2512-exp branch from fa874c9 to faebb16 Compare December 18, 2025 08:01

DrahtBot mentioned this pull request Dec 29, 2025

http: replace WorkQueue and single threads handling for ThreadPool #33689

Open

stickies-v approved these changes Jan 5, 2026

View reviewed changes

DrahtBot requested a review from hodlinator January 5, 2026 09:11

maflcko requested review from ryanofsky and removed request for ryanofsky January 5, 2026 10:10

hodlinator approved these changes Jan 6, 2026

View reviewed changes

This was referenced Jan 6, 2026

bench: add fluent API for untimed setup steps in nanobench #34208

Open

fuzz: Extend spend coverage #34264

Open

ryanofsky approved these changes Jan 15, 2026

View reviewed changes

MarcoFalke added 3 commits January 15, 2026 16:04

util: Make Expected::value() throw

fa6575d

This is not expected to be needed in this codebase, but brings the implementation closer to std::expected::value(). Also, add noexcept, where std::expected has them. This will make operator-> and operator* terminate, when has_value() is false.

util: Add Expected<void, E> specialization

fac4800

This is not needed, but a bit closer to the std lib, because std::monostate is no longer leaked through ValueType from the value() method.

util: Implement Expected::value()&& and Expected::error()&&

fab9721

They are currently unused, but implementing them is closer to the std::expected.

maflcko force-pushed the 2512-exp branch from faebb16 to fada9cb Compare January 15, 2026 15:13

MarcoFalke added 2 commits January 15, 2026 16:15

util: Implement Expected::operator*()&&

fabb47e

It is currently unused, but implementing it is closer to std::expected.

util: Add Expected::swap()

faa59b3

maflcko force-pushed the 2512-exp branch from fada9cb to faa59b3 Compare January 15, 2026 15:16

ryanofsky approved these changes Jan 15, 2026

View reviewed changes

DrahtBot requested review from hodlinator and stickies-v January 15, 2026 16:04

DrahtBot added CI failed and removed CI failed labels Jan 15, 2026

util: Add some more Unexpected and Expected methods #34032

Are you sure you want to change the base?

util: Add some more Unexpected and Expected methods #34032

Conversation

maflcko commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 9, 2025

Uh oh!

maflcko commented Dec 9, 2025

Uh oh!

hodlinator left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

maflcko commented Dec 9, 2025 •

edited

Loading

DrahtBot commented Dec 9, 2025 •

edited

Loading

hodlinator left a comment •

edited

Loading

ryanofsky Jan 15, 2026 •

edited

Loading

ryanofsky Jan 15, 2026 •

edited

Loading