-
Notifications
You must be signed in to change notification settings - Fork 36.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kernel, refactor: return error status on all fatal errors #29700
base: master
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing fatal errors.
69defe0
to
bd4b7e7
Compare
ee2456d
to
edc9fd1
Compare
edc9fd1
to
bf3e999
Compare
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.
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 <[email protected]> bitcoin#25722 (comment) Co-authored-by: Martin Leitner-Ankerl <[email protected]>
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.
af226a6
to
63aa292
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
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:
d1d1face112d
refactor, kernel: Add FlushStatus / FlushResult types76518a6651f9
refactor: Add InfoType field to util::Result0d9b3d98fefe
refactor, blockstorage: Return FlushResult from flush methods1acd64b64334
refactor, blockstorage: Return fatal errors from block writesf28da0bba7ab
refactor, validation: Switch to result.Update240658263dba
refactor, blockstorage: Return fatal error from LoadBlockIndex5f04c656352b
refactor, validation: Return fatal errors from FlushStateToDisk62b07f33f04f
refactor, validation: Return fatal errors from mempool accept functions4ddfbec2cbff
refactor, validation: Return fatal errors from assumeutxo snapshot functions6d9defff6b76
refactor, validation: Return fatal errors from block connect functions0244a91fbad6
refactor, validation: Return fatal errors from activate best chain functionseba59b3ee921
refactor, validation: Return fatal errors from new block functionsdcdf292b4af2
refactor, blockstorage: Return fatal error from ImportBlocks63aa292c8e68
refactor, validation: Return more errors from VerifyDB