Skip to content

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jul 21, 2022

Add util::Result support 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:

  • 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 makes the result class not useful for functions like 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:

    -virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
    +virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name) = 0;
    -std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
    +util::Result<std::unique_ptr<WalletDatabase>, DatabaseError> MakeDatabase(const fs::path& path, const DatabaseOptions& options);

This change also makes some more minor improvements:

  • Smaller type size. util::Result<int> is 16 bytes, and util::Result<void> is 8 bytes. Previously both were 72 bytes.

  • Broader type compatibility. Supports noncopyable and nonmovable success and error types.

Comparison with std::expected

Currently, the util::Result class 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::expected remains a good choice for lower-level functions or simple functions that fail in only a few well-defined ways. Meanwhile,

  • Result becomes 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.)
  • Result can also be more useful than expected when 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.)
  • Result may also be a better choice than std::expected when returning pointer values. (Followup #26022 adds a ResultPtr specialization that avoids double dereferencing.)
  • Result can be more efficient than std::expected when failures are rare but failure objects are large, since failure values are heap-allocated.

This PR effectively makes Result a superset of expected, making the representations compatible and allowing them to be used together.

Footnotes

  1. Ability to return error values was actually present in the original implementation of #25218, but unfortunately removed in later iterations. See design notes.

@ryanofsky
Copy link
Contributor Author

Draft PR since I want to add a commit taking advantages of ability to return chain results and return warnings.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 21, 2022

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

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK arejula27
Approach ACK hebasto
Stale ACK w0xlt, stickies-v, hernanmarino, jonatack, maflcko, laanwj, achow101, 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:

  • #34004 (Implementation of SwiftSync by rustaceanrob)
  • #32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
  • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
  • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
  • #10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • "helper functions types that can be used with" -> "helper functions and types that can be used with" [“functions types” is invalid; needs "functions and types" to be grammatical]
  • "to for dealing with error and warning messages." -> "for dealing with error and warning messages." [extra "to" makes the phrase ungrammatical]
  • "allowing each to be understood to changed more easily." -> "allowing each to be understood or changed more easily." ["to changed" is ungrammatical; "or changed" restores intended meaning]

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

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

2026-01-07

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::expected class.
  • Updated documentation to have more examples and focus on features not in the std::expected class.
  • Tried to respond to hodlinators concerns by moving message types and functions to a new util/messages.h file, 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.

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>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//! 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 164 to 167
//! 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>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #25665 (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?

Yes exactly. Added a comment mentioning the void type to make this clearer.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20166559939/job/57903572196
LLM reason (✨ experimental): Lint checks failed due to a new circular dependency: util/messages -> util/result -> util/messages.

Hints

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.

@arejula27
Copy link

arejula27 commented Dec 13, 2025

Re @ryanofsky

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

If you don't see any benefits keep your implementation, a generalisation can be implemented later if required 😄

Copy link
Contributor

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +53 to +55
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 16, 2025
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));
      |          ^~~~
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 16, 2025
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));
      |          ^~~~
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 16, 2025
Use result.Update in CompleteChainstateInit() and
ChainstateManager::ActivateSnapshot() so it is possible for them to return
warning messages (about flush failures) in upcoming commits.

CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665
and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but
previously these functions only returned Result values themselves, and did not
call other functions that return Result values. Now, some functions they are
calling will also return Result values, so refactor these functions to use
result.Update so they can merge results and return complete error and warning
messages.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 16, 2025
Use result.Update in CompleteChainstateInit() and
ChainstateManager::ActivateSnapshot() so it is possible for them to return
warning messages (about flush failures) in upcoming commits.

CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665
and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but
previously these functions only returned Result values themselves, and did not
call other functions that return Result values. Now, some functions they are
calling will also return Result values, so refactor these functions to use
result.Update so they can merge results and return complete error and warning
messages.
{
protected:
struct FailData {
std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> failure{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe this could be made more distinct from warnings by changing the name?

Suggested change
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; }

ryanofsky and others added 7 commits January 6, 2026 05:45
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));
      |          ^~~~
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 7, 2026
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));
      |          ^~~~
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 7, 2026
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));
      |          ^~~~
@ryanofsky ryanofsky force-pushed the pr/bresult2 branch 2 times, most recently from 2db8bd9 to a3c37e6 Compare January 7, 2026 03:39
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 7, 2026

🚧 At least one of the CI tasks failed.
Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/20769214790/job/59641680094
LLM reason (✨ experimental): IWYU detected include issues (failure generated from IWYU), causing the CI to fail.

Hints

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.