kernel: Handle fatal errors through return values #29642

sedited · 2024-03-12T21:34:45Z

Based on #25665.

Currently functions issuing fatal errors call the fatalError notification 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 through util::Result's error string values with its .MoveMessages method, 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:

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, or CheckFatalFailure.
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:
- Returning it immediately
- Assigning it immediately to an existing result with .MoveMessages() or .Set()
- Eventually passing it as an argument to a .MoveMessages()

This PR is part of the libbitcoinkernel project and is a step towards stage 2, creating a more refined kernel API.

DrahtBot · 2024-03-12T21:34:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stickies-v, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29735 (AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure by instagibbs)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#29553 (assumeutxo: Add dumptxoutset height param, remove shell scripts by fjahr)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#29402 (mempool: Log added for dumping mempool transactions to disk by kevkevinpal)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
#29231 (logging: Update to new logging API by ajtowns)
#29015 (kernel: Streamline util library by ryanofsky)
#28843 ([refactor] Remove BlockAssembler m_mempool member by TheCharlatan)
#28687 (C++20 std::views::reverse by stickies-v)
#28233 (validation: don't clear cache on periodic flush: >2x block connection speed by andrewtoth)
#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages 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.

stickies-v · 2024-03-15T15:01:51Z

Concept ACK, nice one. Will first re-review #25665

ryanofsky · 2024-03-19T18:53:10Z

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 util::Result and [[nodiscard]].

It also seems like it would be nice to have local enums for functions like ProcessTransaction and ActivateBestChain that just have 2 or 3 error codes instead of 22 error codes like FatalError.

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 [[nodiscard]] to all functions where fatalError/flushError are called, and bubbles up util::Result values, and is just a plain refactoring that doesn't change behavior, and doesn't add the FatalError type and handlers? It seems like could make the PR smaller, and make libbitcoinkernel act like a normal library that can return errors, without changing other things.

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

…lError>

To propagate the FatalError, also add the type to LoadBlockIndexDB.

Also add using declarations where now possible.

DrahtBot · 2024-03-28T11:56:12Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2024-06-25T00:02:00Z