util: generalize `util::Result` to support custom errors #34005

l0rinc · 2025-12-04T14:06:20Z

Context

While reviewing #33657 it became clear we don’t have a good value-or-error wrapper, similar in spirit to std::expected<T, E> in C++23.

Problem

The util::Result helper currently stores a std::variant<bilingual_str, T> and is effectively only usable for high-level code that needs to propagate user-facing error strings.
Low-level code that wants typed error codes instead of strings still has to expose raw std::variant (or roll custom structs).

Fix

Generalize util::Result to take a second template parameter E for the error type, and store std::variant<E, T> internally.
The default E remains bilingual_str, so existing uses of Result<T> and util::Error{...} behave exactly as before and ErrorString(Result<T>) continues to work.

New code can now use typed errors and avoid the awkward std::variant getter, e.g.:

if (auto ret{chainman->m_blockman.ReadRawBlock(WITH_LOCK(cs_main, return index.GetBlockPos()))}) {
    block = std::move(*ret);
    return true;
}
return false;

DrahtBot · 2025-12-04T14:06:28Z

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

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34006 (Add util::Expected (std::expected) by maflcko)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages 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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

uses continue -> continues / will continue [grammatical error: "uses continue" is incorrect; use "continues" or "will continue" for proper verb form]

^2025-12-04

fanquake · 2025-12-04T14:09:05Z

Have you looked at #25665?

ryanofsky · 2025-12-04T14:28:17Z

Have you looked at #25665?

Looking at bb61eca55a563d7486df3c356d163c4af10a36c8, I think this is compatible with #25665. This PR is a minimal tweak to util::Result that can let it work as a substitute for std::expected before std::expected is available to use. #25665 could be easily rebased on top of this. In the long run, though, I think code that doesn't need util::Result error reporting functionality should just use std::expected directly.

It might be good if this added an alias like:

template<typename T, typename F>
using Expected = Result<T, F>;

So it could be clearer which code should eventually use std::expected when it's available, and replacement could potentially be a scripted-diff. We did something similar with Span/std::span

The `util::Result` helper currently wraps a `std::variant<bilingual_str, T> `and is only usable for high-level code that needs to propagate user-facing error strings. This change generalizes `Result` so it can also be used in low-level code paths with typed error codes instead of raw `std::variant` or `std::expected`. Instead of hard-coding `bilingual_str` in the variant, the error is now templated, so the existing uses of `Result<T>` behave as before. Call sites get an explicit value-or-error result type without having to manipulate std::variant directly. The design mirrors the shape of `std::expected<T, E>` in C++23, but remains usable on our current C++20 baseline, similar to how `Span` provided a precursor to `std::span`.

maflcko · 2025-12-04T14:36:32Z

I wonder if this is the right approach. util::Result is mostly meant for high level code (dealing with translations), such as init, the wallet, or the gui. For low-level stuff, it could make sense to use std::expected directly?

I am not sure how fast we'll be getting C++23. C++20 was enabled two years ago in #28349, but there are still questions around which C++20 features are supported. (e.g. jthread is only in libc++ 20, std::format is only in g++13, ...)

So I think just porting a minimal std::expected seems easier?

So I did that in #34006

l0rinc · 2025-12-04T14:38:27Z

It might be good if this added an alias like

Good idea, added and used it in the new test cases.

Have you looked at #25665?

I agree with @ryanofsky that this is likely compatible with the PR and as far as I understand it addresses a small chunk of what it's also trying to achieve.

So I think just porting a minimal std::expected seems easier?
So I did that in #34006

I'm fine with both, let me know if I should close this to favor the other one. Not sure why we'd need two, but it's not a big deal either way.

maflcko · 2025-12-04T14:43:04Z

Looking at bb61eca, I think this is compatible with #25665. This PR is a minimal tweak to util::Result that can let it work as a substitute for std::expected before std::expected is available to use. #25665 could be easily rebased on top of this. In the long run, though, I think code that doesn't need util::Result error reporting functionality should just use std::expected directly.

Not sure they are conceptually compatible. 25665 aims to reduce the error footprint by wrapping it in a unique_ptr, whereas std::expected is basically a variant wrapper (keeps the memory in the struct itself).

So a scripted-diff going from an Expected (via Result), to std::expected, may have performance impact, if the error is large enough.

I think it is better to use std::unique_ptr explicitly in Expected/std::expected, when needed.

ryanofsky · 2025-12-04T14:46:08Z

So I think just porting a minimal std::expected seems easier?

So I did that in #34006

Nice, that's more complicated than this PR, but not too bad. I don't have any particular preference between these two PR's. Either seem fine.

maflcko · 2025-12-04T15:02:37Z

So I think just porting a minimal std::expected seems easier?
So I did that in #34006

Nice, that's more complicated than this PR, but not too bad. I don't have any particular preference between these two PR's. Either seem fine.

Yeah, it is a bit more code, but, the inner guts of util::Expected are mostly copy-pasted from the current util::Result in master, so it shouldn't be conceptually complicated.

ryanofsky · 2025-12-04T19:13:47Z

re: #34005 (comment)

Not sure they are conceptually compatible. 25665 aims to reduce the error footprint by wrapping it in a unique_ptr, whereas std::expected is basically a variant wrapper (keeps the memory in the struct itself).

Yes, it's true that hypothetically if this PR got merged and then #25665 got merged, performance of code using util::Expected could be affected because of unique_ptr use. And then, if there was a scripted-diff replacing util::Expected with std::expected it would switch back again.

I don't see that as a concern for this PR, since the current Result class could just be renamed to Expected in #25665 to avoid that if desired. So #34005 and #34006 both seem like good solutions to me, and I'd be happy with either.

maflcko · 2025-12-05T10:26:05Z

I don't see that as a concern for this PR, since the current Result class could just be renamed to Expected in #25665 to avoid that if desired. So #34005 and #34006 both seem like good solutions to me, and I'd be happy with either.

Either is fine by me as well. It just seems more churn to (1) introduce the Expected alias based on Result, (2) move it a different file, (3) likely rework it to remove the implicit bilingual_string error stuff (because it is not needed and confusing). It seems easier to just add Expected in its intended form in one go. But no strong opinion, anything seems fine here.

maflcko · 2025-12-05T10:26:54Z

src/util/result.h

 }
+
+template<typename T, typename F>
+using Expected = Result<T, F>;


if this alias is added, one should also be added for std::unexpected.

l0rinc · 2025-12-05T12:12:49Z

It seems easier to just add Expected in its intended form in one go

Glad you have that alternative ready, closing in favor of #34006

faa2373 refactor: Enable clang-tidy bugprone-unused-return-value (MarcoFalke) fa114be Add util::Expected (std::expected) (MarcoFalke) Pull request description: 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`. ACKs for top commit: romanz: tACK faa2373 sedited: Re-ACK faa2373 hodlinator: ACK faa2373 rkrux: light Code Review ACK faa2373 ryanofsky: Code review ACK faa2373, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review. stickies-v: ACK faa2373 Tree-SHA512: fdbd0f6bf439738ffe6a68da5522f1051537f8df9c308eb90bef6bd2e06931d79f1c5da22d5500765e9cb1d801d5be39e11e10d47c9659fec1a8c8804cb7c872

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

l0rinc mentioned this pull request Dec 4, 2025

rest: allow reading partial block data from storage #33657

Merged

l0rinc force-pushed the l0rinc/generalized-Result-error branch from bb61eca to d44f140 Compare December 4, 2025 14:34

DrahtBot added the CI failed label Dec 4, 2025

maflcko mentioned this pull request Dec 4, 2025

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

Merged

DrahtBot removed the CI failed label Dec 4, 2025

maflcko reviewed Dec 5, 2025

View reviewed changes

ryanofsky mentioned this pull request Dec 5, 2025

refactor: Add util::Result failure types and ability to merge result values #25665

Open

l0rinc closed this Dec 5, 2025

This was referenced Dec 5, 2025

Add util::ResultPtr class #26022

Draft

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

Draft

util: generalize util::Result to support custom errors #34005

util: generalize util::Result to support custom errors #34005

Uh oh!

Conversation

l0rinc commented Dec 4, 2025

Context

Problem

Fix

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

fanquake commented Dec 4, 2025

Uh oh!

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

Uh oh!

Uh oh!

maflcko commented Dec 4, 2025

Uh oh!

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

Uh oh!

Uh oh!

maflcko commented Dec 4, 2025

Uh oh!

ryanofsky commented Dec 4, 2025

Uh oh!

maflcko commented Dec 4, 2025

Uh oh!

ryanofsky commented Dec 4, 2025

Uh oh!

maflcko commented Dec 5, 2025

Uh oh!

maflcko Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

util: generalize `util::Result` to support custom errors #34005

util: generalize `util::Result` to support custom errors #34005

DrahtBot commented Dec 4, 2025 •

edited

Loading

ryanofsky commented Dec 4, 2025 •

edited

Loading

l0rinc commented Dec 4, 2025 •

edited

Loading