Skip to content

kernel, refactor: return error status on all fatal errors#29700

Draft
ryanofsky wants to merge 23 commits into
bitcoin:masterfrom
ryanofsky:pr/fatalresult
Draft

kernel, refactor: return error status on all fatal errors#29700
ryanofsky wants to merge 23 commits into
bitcoin:masterfrom
ryanofsky:pr/fatalresult

Conversation

@ryanofsky

@ryanofsky ryanofsky commented Mar 21, 2024

Copy link
Copy Markdown
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:

@DrahtBot

DrahtBot commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

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

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #35714 (validation: stop writes after flush failure by l0rinc)
  • #35713 (Remove boost as a unit test runner by rustaceanrob)
  • #35675 (mining: add block template manager by ismaelsadeeq)
  • #35581 (node: add block template manager and track waitNext fee inflow by ismaelsadeeq)
  • #35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
  • #35557 (kernel, validation: Add btck_chainstate_manager_set_clock_time by ryanofsky)
  • #35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
  • #35307 (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)
  • #35286 (rpc: add testsubmitpackage for 1p1c test submissions by instagibbs)
  • #35205 (kernel,node: add dbcache setter and clarify defaults by l0rinc)
  • #35187 (kernel: Add non-utxo set block validation to API by sedited)
  • #35071 (Reindex: save progress to continue after interruption by pinheadmz)
  • #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
  • #34729 (Reduce log noise by ajtowns)
  • #34672 (mining: add reason/debug to submitSolution and unify with submitBlock by w0xlt)
  • #34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
  • #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
  • #33324 (blocks: add -reobfuscate-blocks argument to enable (de)obfuscating existing blocks by l0rinc)
  • #32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
  • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
  • #30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
  • #28690 (build: Introduce internal kernel library by sedited)
  • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
  • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
  • #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 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):

  • ProcessNewBlock(block, true, true, nullptr, process_result) in src/test/blockfilter_index_tests.cpp
  • ProcessNewBlock(std::make_shared<CBlock>(Params().GenesisBlock()), true, true, &ignored, process_result) in src/test/validation_block_tests.cpp
  • ProcessNewBlock(block, true, true, &new_block, process_result) in src/test/util/mining.cpp
  • AcceptBlock(new_block, state, accept_result, &new_block_index, true, nullptr, nullptr, true) in src/test/coinstatsindex_tests.cpp
  • AcceptBlock(pblockone, state, accept_result, &pindex, true, nullptr, &newblock, true) in src/test/validation_chainstate_tests.cpp
  • ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt) in src/test/txpackage_tests.cpp

2026-07-14 22:18:05

@DrahtBot

Copy link
Copy Markdown
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.

Debug: https://github.com/bitcoin/bitcoin/runs/22957677751

@Graysonbarton Graysonbarton left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixing fatal errors.

ryanofsky and others added 23 commits July 13, 2026 16:29
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 <martin.ankerl@gmail.com>
bitcoin#25722 (comment)

Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>
Avoid false positive maybe-uninitialized errors in native_previous_releases
CI job.

Fix was suggested by maflcko in
bitcoin#25665 (comment)

CI errors look like:
https://github.com/bitcoin/bitcoin/actions/runs/25426711469/job/74581986567?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:143:10: 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]
  143 |     auto spkm = Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false));
      |          ^~~~
cc1plus: all warnings being treated as errors
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.
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.
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.
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.
Return fatal error and interrupt status from LoadBlockIndex functions and
update callers to use new result types.
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.
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.
…nctions

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.
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.
…nctions

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.
Return fatal errors from AcceptBlock, ProcessNewBlock, TestBlockValidity,
LoadGenesisBlock, and LoadExternalBlockFile. Also add nodiscard annotations so
callers handle the result values.
Return fatal error ImportBlocks function and add nodiscard annotation.
Return ConnectBlock errors from VerifyDB.
Needed due to base PR, can be dropped before merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants