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

pull ryanofsky wants to merge 21 commits into bitcoin:master from ryanofsky:pr/fatalresult changing 51 files +1667 −654
  1. 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:

  2. DrahtBot commented at 11:11 pm on March 21, 2024: 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 for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33094 (refactor: unify container presence checks by l0rinc)
    • #33078 (kernel: improve BlockChecked ownership semantics by stickies-v)
    • #33042 (refactor: inline constant return values from dbwrapper write methods by l0rinc)
    • #32950 (validation: remove BLOCK_FAILED_CHILD by stratospher)
    • #32541 (index: store per-block transaction locations for efficient lookups by romanz)
    • #32414 (validation: periodically flush dbcache during reindex-chainstate by andrewtoth)
    • #32317 (kernel: Separate UTXO set access from validation functions by TheCharlatan)
    • #31615 (validation: ensure assumevalid is always used during reindex by Eunovo)
    • #31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
    • #30595 (kernel: Introduce initial C header API by TheCharlatan)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #28690 (build: Introduce internal kernel library by TheCharlatan)
    • #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.

  3. ryanofsky marked this as a draft on Mar 21, 2024
  4. DrahtBot added the label CI failed on Mar 22, 2024
  5. 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.

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

  6. DrahtBot added the label Needs rebase on Mar 22, 2024
  7. Graysonbarton approved
  8. Graysonbarton commented at 2:52 pm on March 25, 2024: none
    Fixing fatal errors.
  9. ryanofsky force-pushed on Mar 26, 2024
  10. DrahtBot removed the label Needs rebase on Mar 26, 2024
  11. ryanofsky force-pushed on Mar 26, 2024
  12. ryanofsky force-pushed on Mar 26, 2024
  13. ryanofsky force-pushed on Mar 27, 2024
  14. ryanofsky force-pushed on Mar 27, 2024
  15. DrahtBot added the label Needs rebase on Mar 28, 2024
  16. ryanofsky force-pushed on Mar 28, 2024
  17. DrahtBot removed the label Needs rebase on Mar 28, 2024
  18. DrahtBot added the label Needs rebase on Apr 11, 2024
  19. ryanofsky force-pushed on Apr 18, 2024
  20. DrahtBot removed the label Needs rebase on Apr 18, 2024
  21. ryanofsky force-pushed on Apr 26, 2024
  22. DrahtBot added the label Needs rebase on Apr 30, 2024
  23. ryanofsky force-pushed on May 1, 2024
  24. ryanofsky commented at 5:53 pm on May 1, 2024: contributor
    Rebased 4d2c9de24916f8d69514ea7c7251136e2762fa5c -> f65fa8c91130931713848a97606d5add0fc9b8c5 (pr/fatalresult.13 -> pr/fatalresult.14, compare) due to silent conflict with #28970
  25. DrahtBot removed the label Needs rebase on May 1, 2024
  26. DrahtBot added the label Needs rebase on May 13, 2024
  27. ryanofsky force-pushed on Jun 17, 2024
  28. DrahtBot removed the label Needs rebase on Jun 17, 2024
  29. ryanofsky force-pushed on Jun 18, 2024
  30. DrahtBot commented at 8:43 am on June 19, 2024: contributor

    https://cirrus-ci.com/task/4621016628985856?logs=ci#L3516

    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
    
  31. ryanofsky force-pushed on Jun 20, 2024
  32. DrahtBot added the label Needs rebase on Jun 25, 2024
  33. ryanofsky force-pushed on Jul 10, 2024
  34. ryanofsky commented at 5:45 pm on July 10, 2024: contributor
    Rebased 692fc11f0aae3c8013f6f01d133139ce8eba7135 -> ad1e73590d102edf3738986c1382b5e36a0ed727 (pr/fatalresult.17 -> pr/fatalresult.18, compare) to fix conflicts
  35. DrahtBot removed the label CI failed on Jul 10, 2024
  36. DrahtBot removed the label Needs rebase on Jul 10, 2024
  37. DrahtBot added the label Needs rebase on Jul 10, 2024
  38. ryanofsky force-pushed on Jul 18, 2024
  39. ryanofsky commented at 6:20 pm on July 18, 2024: contributor
    Rebased ad1e73590d102edf3738986c1382b5e36a0ed727 -> e04e259146238f9a733edf3fd7b192db95f66e71 (pr/fatalresult.18 -> pr/fatalresult.19, compare) to fix conflicts with #29668 and #30428
  40. DrahtBot removed the label Needs rebase on Jul 18, 2024
  41. DrahtBot commented at 9:46 pm on July 18, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27631437225

    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.

  42. DrahtBot added the label CI failed on Jul 18, 2024
  43. ryanofsky force-pushed on Jul 19, 2024
  44. DrahtBot removed the label CI failed on Jul 19, 2024
  45. DrahtBot added the label Needs rebase on Jul 26, 2024
  46. in src/node/blockstorage.cpp:513 in e22926a383 outdated
    507 {
    508-    if (!LoadBlockIndex(snapshot_blockhash)) {
    509-        return false;
    510+    auto result{LoadBlockIndexData(snapshot_blockhash)};
    511+    if (!result || IsInterrupted(*result)) {
    512+        return result;
    


    maflcko commented at 2:33 pm on July 26, 2024:
    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.

    ryanofsky commented at 6:27 pm on August 7, 2024:

    re: #29700 (review)

    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.

  47. ryanofsky force-pushed on Aug 7, 2024
  48. ryanofsky commented at 6:39 pm on August 7, 2024: contributor
    Rebased e22926a383223b286fe97a12ea4efaa81414bb41 -> 3ab9a46ac67964c9d5ad54120b680b5eee7f16f0 (pr/fatalresult.20 -> pr/fatalresult.21, compare) due to conflicts with #30517 and #30497
  49. DrahtBot added the label CI failed on Aug 7, 2024
  50. DrahtBot removed the label Needs rebase on Aug 7, 2024
  51. DrahtBot added the label Needs rebase on Aug 9, 2024
  52. TheCharlatan commented at 3:19 pm on October 31, 2024: contributor
    Can you rebase this?
  53. ryanofsky force-pushed on Nov 4, 2024
  54. ryanofsky commented at 1:28 pm on November 4, 2024: contributor
    Rebased 3ab9a46ac67964c9d5ad54120b680b5eee7f16f0 -> 05438605ba95df14705bf2f57924455f3b2e40e1 (pr/fatalresult.21 -> pr/fatalresult.22, compare) due to various conflicts. Still have not moved InterruptResult from success type to failure type yet (https://github.com/bitcoin/bitcoin/pull/29700#discussion_r1707623223), but would like to try that Updated 05438605ba95df14705bf2f57924455f3b2e40e1 -> 37edd648608b35ee8647b24a2810cb68318d0e88 (pr/fatalresult.22 -> pr/fatalresult.23, compare) to fix feature_assumeutxo.py errors https://github.com/bitcoin/bitcoin/actions/runs/11665121674/job/32477102140?pr=29700 Updated 37edd648608b35ee8647b24a2810cb68318d0e88 -> c8b555b18dc149f43f3fffdff7962c3e69e655a9 (pr/fatalresult.23 -> pr/fatalresult.24, compare) to fix windows stderr line ending error https://github.com/bitcoin/bitcoin/actions/runs/11667772974/job/32485867701#step:12:131 Updated c8b555b18dc149f43f3fffdff7962c3e69e655a9 -> a1077c21dd8995c5f11a56b65432da759a3c1b8e (pr/fatalresult.24 -> pr/fatalresult.25, compare) to fix windows errors caused by previous fix (https://github.com/bitcoin/bitcoin/actions/runs/11674634334/job/32507680468) Rebased a1077c21dd8995c5f11a56b65432da759a3c1b8e -> af226a6a39ebab9d5886fcccdfbe63ea7da4fd9e (pr/fatalresult.25 -> pr/fatalresult.26, compare) due to various conflicts Rebased af226a6a39ebab9d5886fcccdfbe63ea7da4fd9e -> 63aa292c8e68c51cf49baba799cbb6aa573dec82 (pr/fatalresult.26 -> pr/fatalresult.27, compare) due to conflict with #31072 Rebased 63aa292c8e68c51cf49baba799cbb6aa573dec82 -> 07d8ec809e10872e5bf3c870abab365bdcb9adba (pr/fatalresult.27 -> pr/fatalresult.28, compare) due to conflicts with #31490, #30965, and #31439 Rebased 07d8ec809e10872e5bf3c870abab365bdcb9adba -> b795ab78b725c3ece4a5aca40330724e4929bc7f (pr/fatalresult.28 -> pr/fatalresult.29, compare) due to conflict with #32033 Rebased b795ab78b725c3ece4a5aca40330724e4929bc7f -> 42a73b451cd68be443c7a0dd94f7a00477775e23 (pr/fatalresult.29 -> pr/fatalresult.30, compare) due to silent conflict with #31910 causing nodiscard error https://cirrus-ci.com/task/5763662914256896 Rebased 42a73b451cd68be443c7a0dd94f7a00477775e23 -> f169e600c0868f96acf42bc3847acfa220db925d (pr/fatalresult.30 -> pr/fatalresult.31, compare) due to various conflicts Rebased f169e600c0868f96acf42bc3847acfa220db925d -> a4cf35ddbde791e491375217470649c0e844955b (pr/fatalresult.31 -> pr/fatalresult.32, compare) due to conflict with #31385 Updated a4cf35ddbde791e491375217470649c0e844955b -> 3c4b28b4d8072abd07405ffbe18c16de7c9d0a86 (pr/fatalresult.32 -> pr/fatalresult.33, compare) to fix ci lint and tidy errors https://cirrus-ci.com/task/6460361353723904 https://cirrus-ci.com/task/5052986470170624
  55. DrahtBot removed the label Needs rebase on Nov 4, 2024
  56. ryanofsky force-pushed on Nov 4, 2024
  57. DrahtBot removed the label CI failed on Nov 4, 2024
  58. ryanofsky force-pushed on Nov 4, 2024
  59. ryanofsky force-pushed on Nov 5, 2024
  60. DrahtBot added the label Needs rebase on Nov 14, 2024
  61. ryanofsky force-pushed on Dec 6, 2024
  62. ryanofsky force-pushed on Dec 6, 2024
  63. DrahtBot removed the label Needs rebase on Dec 6, 2024
  64. DrahtBot added the label Needs rebase on Dec 13, 2024
  65. ryanofsky force-pushed on Mar 12, 2025
  66. DrahtBot removed the label Needs rebase on Mar 12, 2025
  67. DrahtBot added the label Needs rebase on Mar 17, 2025
  68. ryanofsky force-pushed on Mar 20, 2025
  69. DrahtBot removed the label Needs rebase on Mar 20, 2025
  70. DrahtBot added the label CI failed on Mar 23, 2025
  71. DrahtBot commented at 3:55 pm on March 23, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39130149980

    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.

  72. ryanofsky force-pushed on Mar 24, 2025
  73. DrahtBot removed the label CI failed on Mar 24, 2025
  74. DrahtBot added the label Needs rebase on Apr 16, 2025
  75. 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. 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.
    25ab05694e
  76. wallet: fix clang-tidy warning performance-no-automatic-move
    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
    5b50b2a44f
  77. 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.
    f142cc48c7
  78. refactor: Use util::Result class in LoadChainstate and VerifyLoadedChainstate 79c3fe93cf
  79. 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
    5f1f23bc17
  80. 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>
    8b892d41fd
  81. Merge remote-tracking branch 'origin/pull/25665/head' b71847a581
  82. refactor, kernel: Add FlushStatus / FlushResult types 5e7aa31110
  83. refactor: Add InfoType field to util::Result
    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.
    9ceb6dd86f
  84. 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.
    1c23190d2c
  85. 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.
    62abe18848
  86. 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.
    e8b5610ae1
  87. refactor, blockstorage: Return fatal error from LoadBlockIndex
    Return fatal error and interrupt status from LoadBlockIndex functions and
    update callers to use new result types.
    22866b5faf
  88. 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.
    bd73c12c96
  89. 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.
    d1480f14fc
  90. 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.
    855a2f4689
  91. 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.
    0faedbf75b
  92. 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.
    47b32c886d
  93. 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.
    e886270b9f
  94. refactor, blockstorage: Return fatal error from ImportBlocks
    Return fatal error ImportBlocks function and add nodiscard annotation.
    0c7f648cf0
  95. refactor, validation: Return more errors from VerifyDB
    Return ConnectBlock errors from VerifyDB.
    3c4b28b4d8
  96. ryanofsky force-pushed on Aug 1, 2025
  97. ryanofsky force-pushed on Aug 1, 2025
  98. DrahtBot removed the label Needs rebase on Aug 1, 2025
  99. DrahtBot added the label CI failed on Aug 1, 2025
  100. 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.

  101. ryanofsky force-pushed on Aug 1, 2025
  102. DrahtBot removed the label CI failed on Aug 2, 2025

github-metadata-mirror

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: 2025-08-08 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me