refactor: Add util::Result failure types and ability to merge result values#25665
refactor: Add util::Result failure types and ability to merge result values#25665ryanofsky wants to merge 7 commits intobitcoin:masterfrom
Conversation
|
Draft PR since I want to add a commit taking advantages of ability to return chain results and return warnings. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/25665. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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:
Possible places where named args for integral literals may be used (e.g.
If you want, I can propose specific inline comment names for each literal to improve readability. 2026-03-02 02:56:06 |
|
At this point I would prefer I think
Maybe it would be clearer to use something like class ErrorReport
{
std::unique_ptr<detail::Messages> m_data;
......to fulfill the wish to keep memory usage low for the happy path. An alternative would be to implement Compiling example based upon fa1eee4f46fd089d6e19b124addee7c7679585f4 from #34006.diff --git a/src/util/expected.h b/src/util/expected.h
index 38baef4d21..bf8fa2f25c 100644
--- a/src/util/expected.h
+++ b/src/util/expected.h
@@ -7,6 +7,7 @@
#include <attributes.h>
+#include <cassert>
#include <variant>
namespace util {
diff --git a/src/util/result.h b/src/util/result.h
index 39c224f61b..fe00bf3b41 100644
--- a/src/util/result.h
+++ b/src/util/result.h
@@ -5,11 +5,9 @@
#ifndef BITCOIN_UTIL_RESULT_H
#define BITCOIN_UTIL_RESULT_H
-#include <attributes.h>
+#include <util/expected.h>
#include <util/translation.h>
-#include <variant>
-
namespace util {
struct Error {
@@ -31,14 +29,10 @@ struct Error {
//! `std::optional<T>` can be updated to return `util::Result<T>` and return
//! error strings usually just replacing `return std::nullopt;` with `return
//! util::Error{error_string};`.
-template <class M>
-class Result
+template <class V, typename E = bilingual_str>
+class Result : public Expected<V, E>
{
private:
- using T = std::conditional_t<std::is_same_v<M, void>, std::monostate, M>;
-
- std::variant<bilingual_str, T> m_variant;
-
//! Disallow copy constructor, require Result to be moved for efficiency.
Result(const Result&) = delete;
@@ -49,50 +43,31 @@ private:
Result& operator=(const Result&) = delete;
Result& operator=(Result&&) = delete;
- template <typename FT>
- friend bilingual_str ErrorString(const Result<FT>& result);
+ template <typename T>
+ friend bilingual_str ErrorString(const Result<T, bilingual_str>& result);
public:
- Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {} // constructor for void
- Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}
- Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {}
+ using Expected<V, E>::Expected;
+ constexpr Result(Error error) requires(std::is_same_v<E, bilingual_str>) : Expected<V, E>{Unexpected<E>{std::move(error.message)}} {}
Result(Result&&) = default;
~Result() = default;
- //! std::optional methods, so functions returning optional<T> can change to
- //! return Result<T> with minimal changes to existing code, and vice versa.
- bool has_value() const noexcept { return m_variant.index() == 1; }
- const T& value() const LIFETIMEBOUND
- {
- assert(has_value());
- return std::get<1>(m_variant);
- }
- T& value() LIFETIMEBOUND
- {
- assert(has_value());
- return std::get<1>(m_variant);
- }
template <class U>
- T value_or(U&& default_value) const&
+ V value_or(U&& default_value) const&
{
- return has_value() ? value() : std::forward<U>(default_value);
+ return this->has_value() ? this->value() : std::forward<U>(default_value);
}
template <class U>
- T value_or(U&& default_value) &&
+ V value_or(U&& default_value) &&
{
- return has_value() ? std::move(value()) : std::forward<U>(default_value);
+ return this->has_value() ? std::move(this->value()) : std::forward<U>(default_value);
}
- explicit operator bool() const noexcept { return has_value(); }
- const T* operator->() const LIFETIMEBOUND { return &value(); }
- const T& operator*() const LIFETIMEBOUND { return value(); }
- T* operator->() LIFETIMEBOUND { return &value(); }
- T& operator*() LIFETIMEBOUND { return value(); }
};
template <typename T>
-bilingual_str ErrorString(const Result<T>& result)
+bilingual_str ErrorString(const Result<T, bilingual_str>& result)
{
- return result ? bilingual_str{} : std::get<0>(result.m_variant);
+ return result ? bilingual_str{} : result.error();
}
} // namespace util
Then from that point, more advanced template <class V, typename E = std::unique_ptr<Messages>>
class Result : public Expected<V, E>or if preferred: template <class V>
class Result : public Expected<V, std::unique_ptr<Messages>> |
|
re: #25665 (comment) Thanks for taking a look at this.
The result class is handling both of these things (since c++ works most conveniently with a single return value), but I don't see a sense in which it is conflating them. The implementation handles them independently accepting Lines 67 to 68 in f0aff63 You suggestion to use a Your suggestions to make the I think if an |
|
Concept ACK [f0aff63]
I agree that these are interesting problems to address. However, I believe the first one is already solved by This PR has been open for three years, and it is clear that a lot of work and constant updates have gone into it. Still, I feel that merging another One of the most appealing aspects of the current void TryAddNumbers(int a, int b)
{
if (auto result = AddNumbers(a, b)) {
LogPrintf("%i + %i = %i\n", a, b, *result);
} else {
LogPrintf("Error: %s\n", util::ErrorString(result).translated);
}
}This feels smooth and does not add verbosity. However, I would also like to consider an approach like this: template <typename T, typename E>
requires requires(const E& e) { e.messages; } // has .messages
bilingual_str ErrorString(const utils::expected<T,E>& result)
{
if (result.has_value())
return bilingual_str{};
const Messages& m = result.error().messages;
return detail::JoinMessages(m);
}If you agree, this direction could be interesting, I can continue exploring the idea and evaluating how viable it is. I doubt the snippet I provided would compile as-is, it is only meant to illustrate the proposal. Another good option (as mentioned earlier in the discussion) would be to base the template <class V>
class Result : public Expected<V, std::unique_ptr<Messages>>This would isolate all error-handling mechanics in the Finally, I would like to mention that clearer documentation would be valuable. It would be helpful to specify when this class should be used, as the author mentioned several use cases beyond simply reporting errors to the user. |
src/util/result.cpp
Outdated
|
|
||
| #include <util/result.h> | ||
|
|
||
| #include <algorithm> |
There was a problem hiding this comment.
I removed this include and it continue compiling, are we sure this is required? I executed the following commands:
➜ bitcoin (25665) % git log -1 --format=%H 17:21
f0aff63b5ad51566e626d5b24eee08eb81df54a1
➜ bitcoin (25665) % git diff src/util/result.cpp 17:21
diff --git a/src/util/result.cpp b/src/util/result.cpp
index ea79375aa3..423618e85d 100644
--- a/src/util/result.cpp
+++ b/src/util/result.cpp
@@ -4,7 +4,6 @@
#include <util/result.h>
-#include <algorithm>
#include <initializer_list>
#include <iterator>
#include <util/translation.h>
➜ bitcoin (25665) % rm -rf build 17:22
➜ bitcoin (25665) % cmake -B build > /dev/null 2>&1 17:22
➜ bitcoin (25665) % cmake --build build -j "$(($(nproc)/2))" > /dev/null 2>&1 17:22
➜ bitcoin (25665) % ./build/bin/test_bitcoin 17:22
Running 686 test cases...
*** No errors detected
There was a problem hiding this comment.
We usually treat missing or extra includes as nits. There is an iwyu run as part of the tidy CI job. See the logs here: https://github.com/bitcoin/bitcoin/actions/runs/19279986116/job/55128814542?pr=25665#step:9:13821 .
There was a problem hiding this comment.
Does this mean that I should not review includes? I am starting to contribute, sorry if this is not relevant
There was a problem hiding this comment.
If it is important to you, it is enough to link the respective lines in the iwyu report.
There was a problem hiding this comment.
Thx🫡, I guess this conversation can be marked as resolved then
hodlinator
left a comment
hodlinator
left a comment
There was a problem hiding this comment.
re: #25665 (comment)
Excuse me, I somehow glossed over the fact that you had 3 template parameters in this PR, not 2. 🤦 I missed that you allowed for a FailureType apart from MessageType, the former defaulting to void but messages being default-on. I see a greater justification for the distinct Result type. However, it seems the either/or aspect remains, if I'm not mistaken.
Inspired by your reply, I nerd-sniped myself into experimenting with a branch based on #25722 to check my assumptions. I'm able to replace Result with raw Expected in most cases, but wallet code forces me to use:
template <typename T, typename FailureType = void>
using ResultLog = Expected<T, ErrorLog<FailureType>>;The benefit I see to using Expected even as part of "ResultLog" is that expected is a known pattern in C++, so it would slot into people's understanding. From there it comes down providing sufficient ergonomics of mutating/merging when using ErrorLog.
src/util/result.h
Outdated
| using T = std::conditional_t<std::is_same_v<M, void>, std::monostate, M>; | ||
|
|
||
| std::variant<bilingual_str, T> m_variant; | ||
| //! tuple<variant<SuccessType, FailureType>, MessagesType> |
There was a problem hiding this comment.
in e78cfb3 "refactor: Add util::Result failure values":
Isn't it closer to:
| //! tuple<variant<SuccessType, FailureType>, MessagesType> | |
| //! tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>> |
There was a problem hiding this comment.
re: #25665 (comment)
Isn't it closer to:
tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>>
The current comment here says "Logically, a result object is equivalent to tuple<variant<SuccessType, ErrorType>, MessagesType>".
This is just trying to get across the idea that a Result holds a success value or a failure value, plus some messages.
IMO, adding unique_ptr to this description would make it more confusing (and the specific suggestion would also not be accurate since failure and message values can be set independently). Use of unique_ptr is an implementation detail not exposed to callers. The heap allocation is relevant since it could have performance implications, so it's mentioned in the comment. But it's not important for understanding what information is stored in the class or how the class can be used.
| util::Result<void> result{RemoveTxs(batch, txs_to_remove)}; | ||
| if (!result) str_err = util::ErrorString(result); | ||
| return result.has_value(); | ||
| return bool{result}; |
There was a problem hiding this comment.
remark: Took a few minutes to realize that has_value() doesn't exist because of the void template specialization of SuccessHolder which doesn't have that. Makes sense.
src/util/result.h
Outdated
| //! to void destination types is not allowed, since this would discard | ||
| //! source information. | ||
| template <bool DstConstructed, typename DstResult, typename SrcResult> | ||
| static void Move(DstResult& dst, SrcResult& src) |
There was a problem hiding this comment.
Would anything be gained from making the template parameter an r-value? (Earlier commits do explicit std::move()s, justifying the function's name, but that justification appears to have dissipated).
| static void Move(DstResult& dst, SrcResult& src) | |
| static void Move(DstResult& dst, SrcResult&& src) |
There was a problem hiding this comment.
re: #25665 (comment)
Would anything be gained from making the template parameter an r-value? (Earlier commits do explicit
std::move()s, justifying the function's name, but that justification appears to have dissipated).
There isn't a great way of forcing this to be an rvalue, since just replacing & && would make it a universal reference and make using the deduced SrcResult type more awkward. I also don't think it would be too helpful in any case, since this is not a public function and there are only 2 internal callers. Using a plain reference here is the simplest approach, I believe.
| // 3. Path to a symlink to a directory. | ||
| // 4. For backwards compatibility, the name of a data file in -walletdir. | ||
| const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); | ||
| fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); |
There was a problem hiding this comment.
nit Q in 3c535e2 "wallet: fix clang-tidy warning performance-no-automatic-move":
I got the impression our expectation is that all commits should pass CI. So I would expect this change to come before or in the same commit that would cause CI failure. Is that only valid for the HEAD commit when it comes to commits that resolve clang-tidy and similar checks?
There was a problem hiding this comment.
re: #25665 (comment)
nit Q in 3c535e2 "wallet: fix clang-tidy warning performance-no-automatic-move": I got the impression our expectation is that all commits should pass CI. So I would expect this change to come before or in the same commit that would cause CI failure. Is that only valid for the HEAD commit when it comes to commits that resolve clang-tidy and similar checks?
I don't think there's a rule about this, but yes I wouldn't choose a commit only fixing a clang-tidy warning (that seems very brittle anyway) to be the first change here, since it seems easier to understand as a followup. Could reorder commits though if you or anyone else feels this is important.
src/util/result.h
Outdated
| //! Container for SuccessType, providing public accessor methods similar to | ||
| //! std::optional methods to access the success value. | ||
| template <typename SuccessType, typename FailureType, typename MessagesType> | ||
| class SuccessHolder : public FailDataHolder<FailureType, MessagesType> |
There was a problem hiding this comment.
I'm guessing the reason for the existence of a separate SuccessHolder type is in order to specialize away a minimal subset of functionality in SuccessHolder<void, ...>. If that's part of the reason, it could be admitted in the comment block for the main SuccessHolder template?
There was a problem hiding this comment.
re: #25665 (comment)
I'm guessing the reason for the existence of a separate
SuccessHoldertype is in order to specialize away a minimal subset of functionality inSuccessHolder<void, ...>. If that's part of the reason, it could be admitted in the comment block for the mainSuccessHoldertemplate?
Yes exactly. Added a comment mentioning the void type to make this clearer.
| util::Result<kernel::InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes, | ||
| const ChainstateLoadOptions& options); | ||
| util::Result<kernel::InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); |
There was a problem hiding this comment.
An example from my larger experimentation showed that Expected can be used to implement a variant of the chainstate refactor from this PR (0ad3a45), see self-contained commit hodlinator@b40a36c.
There was a problem hiding this comment.
I maintain that the current example in this PR of applying util::Result to LoadChainstate & VerifyLoadedChainstate is not well motivated as they don't use warnings or multiple errors, and are thus a better fit for util::Expected as in my linked commit.
It would be better to use a different example in this PR unless I'm still missing something.
There was a problem hiding this comment.
I think it could use a struct instead of a raw std::pair, but otherwise, if std::expected or util::Expected work out of the box here, it seems easier to just use them?
There was a problem hiding this comment.
if std::expected or util::Expected work out of the box here
They don't. Hodlinator is suggesting replacing:
Result<InterruptResult, ChainstateLoadError>
with:
Expected<InterruptResult, pair<ChainstateLoadError, bilingual_str>>.
So using pair + Expected together to replicate functionality of Result.
This does work fine (it's just slightly more verbose) in cases like this example where you have one function or a set of functions that each produce their own error messages and need to return those messages to callers. But it falls apart when the functions call other functions that also produce error messages and those errors need to be forwarded or merged (#25722, #29700).
The point of Result is to be a superset of Expected and drop-in replacement that provides:
- An implicit
messagesfield so you do not need specifybilingual_streverywhere and can avoidpair<X, bilingual_str>,make_pairboilerplate. - Merging operators and methods (
>>,update) so whenResultfunctions call otherResultfunctions, complete error messages can be returned with information about low level failures and higher level context. - Merging hooks so
Resultfunctions calling otherResultfunctions can bubble up other information as well, such as fatal/nonfatal error status and flush status in #29700.
Expected is a good way to return simple status information from low-level C++ functions. Result is an extension of Expected intended to be used for high level functions which can fail in complicated ways and should be able to return detailed descriptive messages. (It also supports returning warnings which are used heavily in wallet code, though not currently in the kernel.)
There was a problem hiding this comment.
Returning both error enum value and error string is a bit unusual. It seems one could reduce to the more conventional Expected<InterruptResult, ChainstateLoadError> by slightly increasing the number of ChainstateLoadError-enum members and having a separate mapping from error enum value -> error string.
nop
There's only one I realized this doesn't rely on local information, so could also be moved:strprintf that would lead to actually loosing information
bitcoin/src/node/chainstate.cpp
Lines 132 to 133 in 64294c8
There was a problem hiding this comment.
Returning both error enum value and error string is a bit unusual.
Right, that is why Result second parameter defaults to void. The point is that generally Result should be a better choice than Expected when you do want to return error messages.
It seems one could reduce to the more conventional
Expected<InterruptResult, ChainstateLoadError>by slightly increasing the number ofChainstateLoadError-enum members and having a separate mapping from error enum value -> error string.
Disagree that enums are a good way to return error messages from high level code. This results in programs telling you things like "permissions error" without telling you which resource couldn't be accessed or "syntax error" without telling you where the problem occurred. If you want to return helpful error messages, you need to a way to construct them in them places where context is available, and a way to bubble them up to callers, and this is what the Result class provides. Or we could just go with bool and "Oops, something went wrong!" 😉
There was a problem hiding this comment.
Or we could just go with
booland "Oops, something went wrong!" 😉
Don't start me on GenericError being used everywhere. :)
If you want to return helpful error messages, you need to a way to construct them in them places where context is available
To some extent util::Result and util::Expected both compete with Rust's std::Result which is empowered by Rust enums being able to contain error strings. I guess defining the untranslated error string at the place in the code that triggers the error should make for the best messages.
This does work fine (it's just slightly more verbose) in cases like this example where you have one function or a set of functions that each produce their own error messages and need to return those messages to callers. But it falls apart when the functions call other functions that also produce error messages and those errors need to be forwarded or merged (#25722, #29700).
An inner Expected<InterruptResult, pair<ChainstateLoadError, bilingual_str>> could be transformed into an outer Expected<FooResult, pair<FooError, bilingual_str>> in that the outer error message could include some part of the inner error string. In Rust one could even preserve two levels of error enum values, that is not supported by this PR's util::Result nor util::Expected.
What I mean to say at the beginning of the thread is that there probably are other use-cases (definitely in #25722 and probably in #29700) which better motivate your modifications to util::Result in this PR.
There was a problem hiding this comment.
re: #25665 (comment)
Thanks analogies with rust are interesting and I think the approach described is probably pretty good for error handling, but not well suited for error reporting, at least in high level application code. For manageable error handling you want to return coarse, rigidly structured types, and for good error reporting you want to be return more detailed less structured information. So the two goals are in tension and you will create problems (vague or incomplete error reports or overly complicated error handling) if you mix up the purposes of the returned information.
What I mean to say at the beginning of the thread is that there probably are other use-cases (definitely in #25722 and probably in #29700) which better motivate your modifications to
util::Resultin this PR.
Yes, your initial comment here led me to add some toy examples in the Result class documentation. I just don't see other ways to act on the feedback. The Result class has features for handling error messages and error status values, and the LoadChainstate example shows both of those features. It also has features for merging results that are useful when you have a library full of functions returning error messages. These are shown in #25722 and #29700 (which makes sense as separate PR's because they affect many more functions).
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Thanks for the reviews! The feedback's been very helpful and led to a few updates here.
Rebased f0aff63 -> 074cb32 (pr/bresult2.69 -> pr/bresult2.70, compare) due to conflict with #34006. Also:
- Renamed methods and types to be consistent with the
std::expectedclass. - Updated documentation to have more examples and focus on features not in the
std::expectedclass. - Tried to respond to hodlinators concerns by moving message types and functions to a new
util/messages.hfile, to allow wider use and make it clear the result and message types are independent.
re: @arejula27 #25665 (comment)
If you agree this direction could be interesting, I can continue exploring the idea and evaluate how viable it is.
IIUC the suggestion there is to change implementation of ErrorString so it could work with std::expected and not be tied to util::Result. If so that does seem like a potentially useful change and maybe a simplifying one. TBH I don't know of code that would directly benefit from combining ErrorString with std::expected because I think validation and wallet code are better off using util::Result, which I've tried to show in s #25722 and #29700. But I don't see harm in generalizing the implementation.
re: hodlinator #25665 (review)
However, it seems the either/or aspect remains, if I'm not mistaken.
...
The benefit I see to using Expected even as part of "ResultLog" is that expected is a known pattern in C++, so it would slot into people's understanding. From there it comes down providing sufficient ergonomics of mutating/merging when using ErrorLog.
Thanks for looking into this. These experiments seem useful and likely to generate some good ideas. I will say I'm not really clear on what things you'd like to provide which the Result class isn't providing in this PR. I do think the current implementation should "slot into" someone's understanding of std::expected extremely well since it has the same interface, and just adds extra features for merging result instances and returning error messages.
src/util/result.h
Outdated
| using T = std::conditional_t<std::is_same_v<M, void>, std::monostate, M>; | ||
|
|
||
| std::variant<bilingual_str, T> m_variant; | ||
| //! tuple<variant<SuccessType, FailureType>, MessagesType> |
There was a problem hiding this comment.
re: #25665 (comment)
Isn't it closer to:
tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>>
The current comment here says "Logically, a result object is equivalent to tuple<variant<SuccessType, ErrorType>, MessagesType>".
This is just trying to get across the idea that a Result holds a success value or a failure value, plus some messages.
IMO, adding unique_ptr to this description would make it more confusing (and the specific suggestion would also not be accurate since failure and message values can be set independently). Use of unique_ptr is an implementation detail not exposed to callers. The heap allocation is relevant since it could have performance implications, so it's mentioned in the comment. But it's not important for understanding what information is stored in the class or how the class can be used.
src/util/result.h
Outdated
| //! to void destination types is not allowed, since this would discard | ||
| //! source information. | ||
| template <bool DstConstructed, typename DstResult, typename SrcResult> | ||
| static void Move(DstResult& dst, SrcResult& src) |
There was a problem hiding this comment.
re: #25665 (comment)
Would anything be gained from making the template parameter an r-value? (Earlier commits do explicit
std::move()s, justifying the function's name, but that justification appears to have dissipated).
There isn't a great way of forcing this to be an rvalue, since just replacing & && would make it a universal reference and make using the deduced SrcResult type more awkward. I also don't think it would be too helpful in any case, since this is not a public function and there are only 2 internal callers. Using a plain reference here is the simplest approach, I believe.
| // 3. Path to a symlink to a directory. | ||
| // 4. For backwards compatibility, the name of a data file in -walletdir. | ||
| const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); | ||
| fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); |
There was a problem hiding this comment.
re: #25665 (comment)
nit Q in 3c535e2 "wallet: fix clang-tidy warning performance-no-automatic-move": I got the impression our expectation is that all commits should pass CI. So I would expect this change to come before or in the same commit that would cause CI failure. Is that only valid for the HEAD commit when it comes to commits that resolve clang-tidy and similar checks?
I don't think there's a rule about this, but yes I wouldn't choose a commit only fixing a clang-tidy warning (that seems very brittle anyway) to be the first change here, since it seems easier to understand as a followup. Could reorder commits though if you or anyone else feels this is important.
src/util/result.h
Outdated
| //! Container for SuccessType, providing public accessor methods similar to | ||
| //! std::optional methods to access the success value. | ||
| template <typename SuccessType, typename FailureType, typename MessagesType> | ||
| class SuccessHolder : public FailDataHolder<FailureType, MessagesType> |
There was a problem hiding this comment.
re: #25665 (comment)
I'm guessing the reason for the existence of a separate
SuccessHoldertype is in order to specialize away a minimal subset of functionality inSuccessHolder<void, ...>. If that's part of the reason, it could be admitted in the comment block for the mainSuccessHoldertemplate?
Yes exactly. Added a comment mentioning the void type to make this clearer.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Re @ryanofsky
If you don't see any benefits keep your implementation, a generalisation can be implemented later if required 😄 |
hodlinator
left a comment
hodlinator
left a comment
There was a problem hiding this comment.
Thanks for the updates!
Annoyed at myself for not investigating my assumed either/or-ness of your proposed util::Result sooner. Somehow I'd interpreted FailDataHolders:
explicit operator bool() const { return !m_fail_data || !m_fail_data->failure; }as plainly:
explicit operator bool() const { return !m_fail_data; }The former makes it a clearer departure from util::Expected - warnings can be added and merged into parent results while still returning successful values.
There was a problem hiding this comment.
nit in ab94499 commit message:
- Smaller type size section could mention that given values are for 64-bit platforms, since we still ship ARM32 releases.
| util::Result<kernel::InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes, | ||
| const ChainstateLoadOptions& options); | ||
| util::Result<kernel::InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); |
There was a problem hiding this comment.
I maintain that the current example in this PR of applying util::Result to LoadChainstate & VerifyLoadedChainstate is not well motivated as they don't use warnings or multiple errors, and are thus a better fit for util::Expected as in my linked commit.
It would be better to use a different example in this PR unless I'm still missing something.
src/util/result.h
Outdated
| { | ||
| protected: | ||
| struct FailData { | ||
| std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> failure{}; |
There was a problem hiding this comment.
nit: Maybe this could be made more distinct from warnings by changing the name?
| std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> failure{}; | |
| std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> error{}; |
Would hopefully decrease misunderstandings like I had myself in #25665 (review), since in that case it would be:
explicit operator bool() const { return !m_fail_data || !m_fail_data->error; }
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
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.
Previous commit causes the warning to be triggered, but the suboptimal return from const statement was added earlier in 517e204. Warning was causing CI failure https://cirrus-ci.com/task/6159914970644480
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.
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 bitcoin#25722
Suggested by Martin Leitner-Ankerl <[email protected]> bitcoin#25722 (comment) Co-authored-by: Martin Leitner-Ankerl <[email protected]>
Avoid false positive maybe-uninitialized errors in 00_setup_env_native_fuzz_with_valgrind CI jobs. Similar errors also happen in 00_setup_env_arm and 00_setup_env_win64 jobs. Problem was pointed out and fix was suggested by maflcko in bitcoin#25665 (comment) CI errors look like: https://github.com/bitcoin/bitcoin/actions/runs/18881332233/job/53885646274?pr=25665 /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp: In function ‘wallet::DescriptorScriptPubKeyMan* wallet::CreateDescriptor(CWallet&, const std::string&, bool)’: /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp:213:29: error: ‘*(const std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>*)((char*)&spkm + offsetof(util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>,util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>)).std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>::_M_data’ may be used uninitialized [-Werror=maybe-uninitialized] 213 | return &spkm.value().get(); | ~~~~~~~~~~~~~~~~^~ /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp:212:10: note: ‘*(const std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>*)((char*)&spkm + offsetof(util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>,util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>)).std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>::_M_data’ was declared here 212 | auto spkm = Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false)); | ^~~~
18 participants
Add
util::Resultsupport for returning more error information and make use of it in LoadChainstate method as an initial application. Followup PRs #25722 and #29700 use it more broadly to return errors and warnings from wallet and kernel functions as well.This change adds two major features to the result class:
LoadChainstate()which produce different errors that need to be handled differently 1.For better error reporting, adds the ability to return 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:
This change also makes some more minor improvements:
Smaller type size.
util::Result<int>is 16 bytes, andutil::Result<void>is 8 bytes. Previously both were 72 bytes.Broader type compatibility. Supports noncopyable and nonmovable success and error types.
Comparison with
std::expectedCurrently, the
util::Resultclass is useful for error reporting—returning translated, user-facing error strings—but not as useful for error handling, where callers need structured failure information to decide how to proceed. By contrast,std::expected/util::Expected(#34005, #34006) is good for error handling but less helpful for reporting. This PR closes the gap between the two classes.With this PR,
std::expectedremains a good choice for lower-level functions or simple functions that fail in only a few well-defined ways. Meanwhile,Resultbecomes a better choice for operations that can fail in many different ways—loading a wallet, connecting a block, etc.—where callers may want complete error traces, not just low-level errors without context or high-level errors without detail. (Followup #25722 applies this to wallet-loading functions.)Resultcan also be more useful thanexpectedwhen merging status information from multiple calls. (Followup #29700 uses this to reliably propagate flush and fatal-error status from validation code that can internally trigger flushes or shutdowns.)Resultmay also be a better choice thanstd::expectedwhen returning pointer values. (Followup #26022 adds aResultPtrspecialization that avoids double dereferencing.)Resultcan be more efficient thanstd::expectedwhen failures are rare but failure objects are large, since failure values are heap-allocated.This PR effectively makes
Resulta superset ofexpected, making the representations compatible and allowing them to be used together.Footnotes
Ability to return error values was actually present in the original implementation of #25218, but unfortunately removed in later iterations. See design notes. ↩