ryanofsky
commented at 11:11 pm on March 21, 2024:
contributor
Return util::Result objects from all functions that can trigger fatal errors.
There are many validation functions that handle failures by calling AbortNode and triggering shutdowns, without returning error information to their callers. This makes error handling in libbitcoinkernel application code difficult, because the only way to handle these errors is to register for notification callbacks. Improve this by making all functions that trigger fatal errors return util::Result objects with the error information.
This PR is a pure refactoring that returns extra result information from functions without changing their behavior. It’s a possible alternative to and subset of #29642, which adds similar return information but also makes behavior changes and exposes a FatalError type.
This is based on #25665. The non-base commits are:
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:
#34589 (ci: Temporarily use clang in valgrind tasks by maflcko)
#34544 (wallet: Disallow wallet names that are paths including .. and . elements by achow101)
#34521 (validation: fix UB in LoadChainTip by marcofleon)
#34439 (qa: Drop recursive deletes from test code, add lint checks. by davidgumberg)
#34435 (refactor: use _MiB/_GiB consistently for byte conversions by l0rinc)
#34271 (net_processing: make m_tx_for_private_broadcast optional by vasild)
#34208 (bench: add fluent API for untimed setup steps in nanobench by l0rinc)
#34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
#34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
#34004 (Implementation of SwiftSync by rustaceanrob)
#33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
#33854 (fix assumevalid is ignored during reindex by Eunovo)
#33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#33324 (blocks: add -reobfuscate-blocks argument to enable (de)obfuscating existing blocks by l0rinc)
#32950 (validation: remove BLOCK_FAILED_CHILD by stratospher)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)
#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
#31382 (kernel: Flush in ChainstateManager destructor by sedited)
#31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)
#30342 (kernel, logging: Pass Logger instances to kernel objects 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” -> “helper functions and types” [the phrase as written is ungrammatical; it likely intends to say both helper functions and types]
“to for dealing” -> “for dealing” (or “to deal”) [contains both “to” and “for”; one should be removed to form a valid phrase]
2026-03-02 15:41:30
ryanofsky marked this as a draft
on Mar 21, 2024
DrahtBot added the label
CI failed
on Mar 22, 2024
DrahtBot
commented at 0:41 am on March 22, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
0 test 2024-06-18T23:26:56.881000Z TestFramework (ERROR): Assertion failed
1 Traceback (most recent call last):
2 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
3 self.run_test()
4 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_pruning.py", line 447, in run_test
5 self.reorg_back()
6 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_pruning.py", line 231, in reorg_back
7 assert not self.nodes[2].verifychain(checklevel=4, nblocks=0)
8 AssertionError
ryanofsky force-pushed
on Jun 20, 2024
DrahtBot added the label
Needs rebase
on Jun 25, 2024
ryanofsky force-pushed
on Jul 10, 2024
ryanofsky
commented at 5:45 pm on July 10, 2024:
contributor
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot added the label
CI failed
on Jul 18, 2024
ryanofsky force-pushed
on Jul 19, 2024
DrahtBot removed the label
CI failed
on Jul 19, 2024
DrahtBot added the label
Needs rebase
on Jul 26, 2024
in
src/node/blockstorage.cpp:509
in
e22926a383outdated
507 {
508- if (!LoadBlockIndex(snapshot_blockhash)) {
509- return false;
510+ auto result{LoadBlockIndexData(snapshot_blockhash)};
511+ if (!result || IsInterrupted(*result)) {
512+ return result;
Seems fragile to use util::Result<InterruptResult>. Wrapping one result into another is fragile, because call-sites may easily forget to unwrap the inner result, especially if the inner result is otherwise void. My recommendation would be to treat interruption as util::Error, so that callers don’t miss it if they check for errors (but forget to check for interruption), but are free to check it, if they inspect the precise error.
Seems fragile to use util::Result<InterruptResult>. Wrapping one result into another is fragile, because call-sites may easily forget to unwrap the inner result, especially if the inner result is otherwise void. My recommendation would be to treat interruption as util::Error, so that callers don’t miss it if they check for errors (but forget to check for interruption), but are free to check it, if they inspect the precise error.
It sounds like you want Result<InterruptResult, void> to be replaced with Result<void, InterruptResult> which would be ok with me. I don’t think the difference between these things is very significant, but I agree maybe if the caller forgets to check for the interrupted status, incorrectly treating the interrupted status like a failure might be safer than incorrectly treating the interrupted status like a success.
ryanofsky force-pushed
on Aug 7, 2024
ryanofsky
commented at 6:39 pm on August 7, 2024:
contributor
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.
ryanofsky force-pushed
on Mar 24, 2025
DrahtBot removed the label
CI failed
on Mar 24, 2025
DrahtBot added the label
Needs rebase
on Apr 16, 2025
ryanofsky force-pushed
on Aug 1, 2025
ryanofsky force-pushed
on Aug 1, 2025
DrahtBot removed the label
Needs rebase
on Aug 1, 2025
DrahtBot added the label
CI failed
on Aug 1, 2025
DrahtBot
commented at 9:16 pm on August 1, 2025:
contributor
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/47214217413
LLM reason (✨ experimental): The CI failure was caused by a trailing whitespace check failure during linting.
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.
ryanofsky force-pushed
on Aug 1, 2025
DrahtBot removed the label
CI failed
on Aug 2, 2025
DrahtBot added the label
Needs rebase
on Aug 20, 2025
ryanofsky force-pushed
on Oct 15, 2025
DrahtBot removed the label
Needs rebase
on Oct 15, 2025
DrahtBot added the label
Needs rebase
on Oct 31, 2025
ryanofsky force-pushed
on Nov 11, 2025
DrahtBot removed the label
Needs rebase
on Nov 11, 2025
DrahtBot added the label
Needs rebase
on Dec 2, 2025
ryanofsky force-pushed
on Dec 12, 2025
DrahtBot removed the label
Needs rebase
on Dec 12, 2025
DrahtBot added the label
Needs rebase
on Dec 12, 2025
DrahtBot added the label
CI failed
on Dec 12, 2025
DrahtBot
commented at 4:54 pm on December 12, 2025:
contributor
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.
ryanofsky force-pushed
on Dec 16, 2025
DrahtBot removed the label
Needs rebase
on Dec 16, 2025
DrahtBot added the label
Needs rebase
on Dec 16, 2025
ryanofsky force-pushed
on Jan 16, 2026
DrahtBot removed the label
Needs rebase
on Jan 16, 2026
ryanofsky force-pushed
on Jan 16, 2026
ryanofsky force-pushed
on Jan 17, 2026
DrahtBot removed the label
CI failed
on Jan 17, 2026
DrahtBot added the label
Needs rebase
on Jan 26, 2026
ryanofsky force-pushed
on Feb 3, 2026
ryanofsky force-pushed
on Feb 3, 2026
DrahtBot added the label
CI failed
on Feb 3, 2026
DrahtBot
commented at 7:33 pm on February 3, 2026:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot removed the label
Needs rebase
on Feb 3, 2026
DrahtBot removed the label
CI failed
on Feb 3, 2026
DrahtBot added the label
Needs rebase
on Feb 5, 2026
ryanofsky force-pushed
on Feb 9, 2026
DrahtBot removed the label
Needs rebase
on Feb 9, 2026
DrahtBot added the label
CI failed
on Feb 9, 2026
ryanofsky force-pushed
on Feb 9, 2026
DrahtBot removed the label
CI failed
on Feb 9, 2026
DrahtBot added the label
Needs rebase
on Feb 18, 2026
refactor: Add util::Result failure values
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 517e204bacd9d.
Warning was causing CI failure https://cirrus-ci.com/task/6159914970644480
dcd98ef306
refactor: Add util::Result::Update() method
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.
cb2e91be30
refactor: Use util::Result class in LoadChainstate and VerifyLoadedChainstatef95eeb5cb3
refactor: Add util::Result multiple error and warning 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
https://github.com/bitcoin/bitcoin/pull/25722
78f39378e0
test: add static test for util::Result memory usage
Suggested by Martin Leitner-Ankerl <martin.ankerl@gmail.com>
https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174298529
Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>
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
https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3457573218
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));
| ^~~~
ba5f435034
Merge branch 'pr/bresult2' into pr/fatalresultec1ae09a1e
Add new InfoType field to util::Result which unlike SuccessType and FailureType
is useful for returning extra information in a result regardless of whether the
function succeeded for failed.
This will be used to return FlushStatus values from libbitcoinkernel functions
indicating if flushed data in addition to whether they succeeded or failed.
The InfoType field is stored directly in the util::Result object, unlike
FailureType and MessagesType which are stored in dynamically allocated memory
and require an extra memory allocation if they are accessed.
85277d6251
refactor, blockstorage: Return FlushResult from flush methods
Return FlushResult instead of bool from BlockStorage FlushUndoFile,
FlushBlockFile, FlushChainstateBlockFile methods and update all callers of
these methods to use the FlushResult type internally and provide context
information for the flush failure. Three callers:
BlockManager::FindNextBlockPos, BlockManager::WriteUndoDataForBlock, and
Chainstate::FlushStateToDisk will be updated in upcoming commits to bubble
results up to their callers.
771c55b5ec
refactor, blockstorage: Return fatal errors from block writes
Return fatal errors from BlockManager methods that write block data. Also
update callers to use the new result types. The callers will be changed to
bubble up the results to their callers in subsequent commits.
6923030948
refactor, validation: Switch to result.Update
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 #25665
and ChainstateManager::ActivateSnapshot() was changed to use it in #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.
13133392c2
refactor, blockstorage: Return fatal error from LoadBlockIndex
Return fatal error and interrupt status from LoadBlockIndex functions and
update callers to use new result types.
5d556d3de7
refactor, validation: Return fatal errors from FlushStateToDisk
Return fatal errors from the Chainstate::FlushStateToDisk method and several
small, related methods which wrap it: ForceFlushStateToDisk, PruneAndFlush,
ResizeCoinsCaches, and MaybeRebalanceCaches.
Also add nodiscard annotations so callers do not accidentally ignore the result
values. Callers in init and rpc files are updated to explicitly ignore the
flush results, and other callers (AcceptToMemoryPool, ProcessNewPackage,
DisconnectTip, ConnectTip, ActivateBestChainStep, ActivateSnapshot,
MaybeCompleteSnapshotValidation) are updated to store the results in this
commit, and will be updated in upcoming commits to bubble results up to their
callers.
7bca93bb0f
refactor, validation: Return fatal errors from mempool accept functions
Return fatal errors from AcceptToMemoryPool ProcessNewPackage,
ProcessTransaction, MaybeUpdateMempoolForReorg, and LoadMempool functions.
Also add nodiscard annotations so callers handle the result values. Two callers
ActivateBestChainStep and InvalidateBlock will be updated in upcoming commits
to bubble results up to their callers.
9058aa68d8
refactor, validation: Return fatal errors from assumeutxo snapshot functions
Return fatal errors from ActivateSnapshot, MaybeCompleteSnapshotValidation, and
ValidatedSnapshotCleanup functions. Also add nodiscard annotations so callers
handle the result values. One caller, ConnectTip, will be updated in an
upcoming commit to bubble results up to its callers.
0d4937caab
refactor, validation: Return fatal errors from block connect functions
Return fatal errors from ConnectBlock, ConnectTip, DisconnectTip, and
InvalidateBlock. Also add nodiscard annotations so callers handle the result
values. Three callers: ActivateBestChainStep TestBlockValidity, and
CVerifyDB::VerifyDB will be updated in upcoming commits to bubble results up
to their callers.
7636dc1201
refactor, validation: Return fatal errors from activate best chain functions
Return fatal errors from ActivateBestChain, ActivateBestChainStep, and
PreciousBlock functions. Also add nodiscard annotations so callers handle the
result values. Two callers, ProcessNewBlock and LoadExternalBlockFile, will be
updated in an upcoming commits to bubble results up to its callers.
8f440a7750
refactor, validation: Return fatal errors from new block functions
Return fatal errors from AcceptBlock, ProcessNewBlock, TestBlockValidity,
LoadGenesisBlock, and LoadExternalBlockFile. Also add nodiscard annotations so
callers handle the result values.
37ceb62284
refactor, blockstorage: Return fatal error from ImportBlocks
Return fatal error ImportBlocks function and add nodiscard annotation.
c603fa5800
refactor, validation: Return more errors from VerifyDB
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot removed the label
Needs rebase
on Mar 2, 2026
ryanofsky force-pushed
on Mar 2, 2026
DrahtBot removed the label
CI failed
on Mar 2, 2026
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-03-03 03:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
