kernel, refactor: return error status on all fatal errors #29700
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29700. ReviewsSee the guideline for information on the review process. 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. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
69defe0 to
bd4b7e7
Compare
ee2456d to
edc9fd1
Compare
edc9fd1 to
bf3e999
Compare
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.
f169e60 to
a4cf35d
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
a4cf35d to
3c4b28b
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Return util::Result objects from all functions that can trigger fatal errors.
There are many validation functions that handle failures by calling
AbortNodeand 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:
5e7aa31110ecrefactor, kernel: Add FlushStatus / FlushResult types9ceb6dd86fdarefactor: Add InfoType field to util::Result1c23190d2c56refactor, blockstorage: Return FlushResult from flush methods62abe1884826refactor, blockstorage: Return fatal errors from block writese8b5610ae11brefactor, validation: Switch to result.Update22866b5fafa9refactor, blockstorage: Return fatal error from LoadBlockIndexbd73c12c963frefactor, validation: Return fatal errors from FlushStateToDiskd1480f14fc3frefactor, validation: Return fatal errors from mempool accept functions855a2f468990refactor, validation: Return fatal errors from assumeutxo snapshot functions0faedbf75b43refactor, validation: Return fatal errors from block connect functions47b32c886dc5refactor, validation: Return fatal errors from activate best chain functionse886270b9f56refactor, validation: Return fatal errors from new block functions0c7f648cf044refactor, blockstorage: Return fatal error from ImportBlocks3c4b28b4d807refactor, validation: Return more errors from VerifyDB