kernel, refactor: return error status on all fatal errors #29700
+1,618
−612
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. 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. |
This was referenced Mar 22, 2024
ryanofsky
force-pushed
the
pr/fatalresult
branch
from
69defe0
to
bd4b7e7
Compare
ryanofsky
force-pushed
the
pr/fatalresult
branch
4 times, most recently
from
ee2456d
to
edc9fd1
Compare
ryanofsky
force-pushed
the
pr/fatalresult
branch
from
edc9fd1
to
bf3e999
Compare
This was referenced Apr 26, 2024
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 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. These methods will be changed to bubble up the FlushResult value to their callers in subsequent commits.
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.
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 upcommit 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 commits 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. Two callers, ActivateBestChainStep and TestBlockValidity will be updated in an upcoming commits to bubble results up to its 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.
|
Rebased 4d2c9de -> f65fa8c ( |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
3ce52fc00a0drefactor, kernel: Add FlushStatus / FlushResult typesc6bdac1320a2refactor: Add InfoType field to util::Resulte56a36cc7587refactor, blockstorage: Return FlushResult from flush methodsabd7d3622abcrefactor, blockstorage: Return fatal errors from block writes2c2134443ccfrefactor, blockstorage: Return fatal error from LoadBlockIndexdceb837707b1refactor, validation: Return fatal errors from FlushStateToDisk1acf496ebf82refactor, validation: Return fatal errors from mempool accept functionsa39b2cdf4a80refactor, validation: Return fatal errors from assumeutxo snapshot functions809970373cf5refactor, validation: Return fatal errors from block connect functions901ccdc42910refactor, validation: Return fatal errors from activate best chain functionsbbb8fec91d37refactor, validation: Return fatal errors from new block functions1e29b2eaaff4refactor, blockstorage: Return fatal error from ImportBlocksf65fa8c91130refactor, validation: Return more errors from VerifyDB