kernel: Handle fatal errors through return values #29642
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
TheCharlatan
force-pushed
the
returnTypeShutdown
branch
from
a6cbc25
to
1f4d172
Compare
|
Concept ACK, nice one. Will first re-review #25665 |
TheCharlatan
force-pushed
the
returnTypeShutdown
branch
from
1f4d172
to
4d92399
Compare
TheCharlatan
force-pushed
the
returnTypeShutdown
branch
from
4d92399
to
f251761
Compare
|
Concept ACK, it is better for a function to return an error status than set a flag, initiate a global shutdown, and not return a success or failure value. But the FatalError enum and HandleFatalError / CheckFatal / clang-tidy machinery do not seem very friendly to use, and I don't see what benefits they offer over just using It also seems like it would be nice to have local enums for functions like Also, I understand the goal of this is to improve the libbitcoinkernel interface, but I'm not sure this should mean moving AbortNode calls out of kernel code into net_processing code. I think it would be probably be better if the net_processing functions could return success/failure as well, and AbortNode calls could move to an even higher level. Or if AbortNode calls could just be left where they are and this could be a pure refactoring that just adds missing return values. I guess my main question is would it be possible to make a stripped down version of this PR that just adds |
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.
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>
The check warns about incorrect usage of util::Result<T, FatalError>.
The check enforces the following rules:
1. If a function calls another function with a util::Result<T,
FatalCondition> return type, its return type has to be util::Result<T,
FatalCondition> too, or it has to handle the value returned by the
function with one of "CheckFatal", "HandleFatalError",
"UnwrapFatalError", "CheckFatalFailure".
2. In functions returning a util::Result<T, FatalCondition> a call to a
function returning a util::Result<T, FatalCondition> needs to propagate
the value by either:
a) Returning it immediately
b) Assigning it immediately to an existing result with .MoveMessages()
or .Set()
c) Eventually passing it as an arugment to a .MoveMessages() call
This introduces a new FatalError enum class whose semantics when used as a failure type in a util::Result<T, FatalError> are controlled by the previously introduced bitcoin-fatal-error clang-tidy plugin. The kernel's `UnwrapFatalError` method is meant to be used in unit and fuzz tests, while production code should use `CheckFatal` to handle fatal errors.
…atalError> This slightly refactors the method to return the FatalError instead of throwing.
…lt<T, FatalError>
To propagate the FatalError, also add the type to InvalidateCoinsDBOnDisk and AcceptBlock. The net_processing module now has to handle a FatalError, so introduce shutdown and exit_status member variables for to be able to abort.
…lError> To propagate the FatalError, also add the type to PreciousBlock and ActivateBestChain.
To propagate the FatalError, also add the type to PruneBlockFilesManual, AcceptToMemoryPool, ProcessNewPackage, ResizeCoinsCaches, ForceFlushStateToDisk, PruneAndFlush, DisconnectTip, InvalidateBlock, MaybeUpdateMempoolForReorg, ProcessTransaction, MaybeRebalanceCaches, LoadMempool
To propagate the FatalError, also add the type to VerifyDB and TestBlockValidity. The miner module now has to handle a FatalError, so introduce shutdown and exit_status member variables for it to be able to abort.
To propagate the FatalError, also add the type to LoadGenesisBlock.
To propagate the FatalError, also add the type to FlushChainstateBlockFile.
To propagate the FatalError, also add the type to LoadBlockIndexDB.
Also add using declarations where now possible.
TheCharlatan
force-pushed
the
returnTypeShutdown
branch
from
f251761
to
bd0a9bb
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Based on #25665.
Currently functions issuing fatal errors call the
fatalErrornotification method to issue a shutdown procedure. This makes it hard for higher level functions to figure out if an error deeper in the call stack occurred. For users of the kernel it might also be difficult to assert which function call issued a fatal error if they are being run concurrently. If the kernel would eventually be used by external users, getting fatal error information through a callback instead of function return values would also be cumbersome and limiting. Unit, bench, and fuzz tests currently don't have a way to effectively test against fatal errors.This patch set is an attempt to make fatal error handling in the kernel code more transparent. Fatal errors are now passed up the call stack through
util::Result<T, FatalError>failure values. A previous attempt at this by theuni always immediately returned failure values if a function call returned a failure. However, this is not always desirable (see discussion here). Sometimes, further operations can still be completed even if a fatal error was issued. The solution to this is that these "ignored" errors are still moved throughutil::Result's error string values with its.MoveMessagesmethod, even while a failure value in the result is not present.Next to some smaller behavior changes, the most significant change is that the issuing of a shutdown procedure is delayed until a potential fatal error is handled as opposed to immediately when it is first encountered. Another effect is that potential fatal errors are now asserted in the bench, fuzz and unit tests. Some of the currently not immediately returned fatal errors need some further scrutiny. These are marked with a
TODO (fatal error)comment and could be tackled in a later PR.To validate this approach a new clang-tidy check is introduced. It implements the following checks:
util::Result<T, FatalCondition>return type, its return type has to beutil::Result<T, FatalCondition>too, or it has to handle the value returned by the function with one ofCheckFatal,HandleFatalError,UnwrapFatalError, orCheckFatalFailure.util::Result<T, FatalCondition>a call to a function returning autil::Result<T, FatalCondition>needs to propagate the value by either:.MoveMessages()or.Set().MoveMessages()This PR is part of the libbitcoinkernel project and is a step towards stage 2, creating a more refined kernel API.