Change Details

The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site. Prior to this patch FlushBlockFile may have failed without returning in Chainstate::FlushStateToDisk, leading to a potential write from WriteBlockIndexDB that may refer to a block that is not fully flushed to disk yet. By returning if either FlushUndoFile or FlushBlockFile fail, we ensure that no further write operations take place that may lead to an inconsistent database when crashing. Add [[nodiscard]] annotations to them such that they are not ignored in future. Functions that call either FlushUndoFile or FlushBlockFile, need to handle these extra abort cases properly. Since Chainstate::FlushStateToDisk already produces an abort error in case of WriteBlockIndexDB failing, no extra logic for functions calling Chainstate::FlushStateToDisk is required. Besides Chainstate::FlushStateToDisk, FlushBlockFile is also called by FindBlockPos, while FlushUndoFile is only called by FlushBlockFile and WriteUndoDataForBlock. For both these cases, the flush error is not further bubbled up. Instead, the error is logged and a comment is provided why bubbling up an error would be less desirable in these cases. This is a backport of [[https://github.com/bitcoin/bitcoin/pull/27866 | core#27866]]

> blockstorage: Mark FindBlockPos as nodiscard > > A false return value indicates a fatal error (disk space being too low), > so make sure we always consume this error code. > > This commit is part of an ongoing process for making the handling of > fatal errors more transparent and easier to understand. > blockstorage: Log on fatal block file flush error > > By returning an error code if `FlushBlockFile` fails, the caller now has > to explicitly handle block file flushing errors. Before this change > such errors were non-explicitly ignored without a clear rationale. > > Prior to this patch `FlushBlockFile` may have failed silently in > `Chainstate::FlushStateToDisk`. Improve this with a log line. Also add a > TODO comment to flesh out whether returning early in the case of an > error is appropriate or not. Returning early might be appropriate to > prohibit `WriteBlockIndexDB` from writing a block index entry that does > not refer to a fully flushed block. > > Besides `Chainstate::FlushStateToDisk`, `FlushBlockFile` is also called > by `FindBlockPos`. Don't change the abort behavior there, since we don't > want to fail the function if the flushing of already written blocks > fails. Instead, just document it. > blockstorage: Log on fatal undo file flush error > > By returning an error code if either `FlushUndoFile` or `FlushBlockFile` > fail, the caller now has to explicitly handle block undo file flushing > errors. Before this change such errors were non-explicitly ignored > without a clear rationale. > > Besides the call to `FlushUndoFile` in `FlushBlockFile`, ignore its > return code at its call site in `WriteUndoDataForBlock`. There, a failed > flush of the undo data should not be indicative of a failed write. > > Add [[nodiscard]] annotations to `FlushUndoFile` such that its return > value is not just ignored in the future. This does not change the way flush errors are handled other than by adding log prints and comments to callsites. This is a backport of [[https://github.com/bitcoin/bitcoin/pull/27866 | core#27866]]

The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site. Prior to this patch FlushBlockFile may have failed without returning in Chainstate::FlushStateToDisk> blockstorage: Mark FindBlockPos as nodiscard > > A false return value indicates a fatal error (disk space being too low), > so make sure we always consume this error code. > > This commit is part of an ongoing process for making the handling of > fatal errors more transparent and easier to understand. > blockstorage: Log on fatal block file flush error > > By returning an error code if `FlushBlockFile` fails, leading to a potential write from WriteBlockIndexDB that may refer to athe caller now has > to explicitly handle block that is not fullyfile flushed to disk yeting errors. By returning if either FlushUndoFile or FlushBlockFile fail,efore this change > such errors were non-explicitly ignored without a clear rationale. > > Prior to this patch `FlushBlockFile` may have failed silently in > `Chainstate::FlushStateToDisk`. we ensure that no further write operations take place that may lead to an inconsistent database when crashingImprove this with a log line. Add [[nodiscard]] annotations to them such that they are not ignored in future. Functions that call either FlushUndoFile or FlushBlockFile,lso add a > TODO comment to flesh out whether returning early in the case of an > error is appropriate or not. need to handle these extra abort cases properly. Since Chainstate::FlushStateToDisk already produces an abort error in case of WriteBlockIndexDB failingReturning early might be appropriate to > prohibit `WriteBlockIndexDB` from writing a block index entry that does > not refer to a fully flushed block. > > Besides `Chainstate::FlushStateToDisk`, no extra logic for functions calling Chainstate::FlushStateToDisk is required. Besides Chainstate::FlushStateToDisk,`FlushBlockFile` is also called > by `FindBlockPos`. FlushBlockFile is also called by FindBlockPosDon't change the abort behavior there, while FlushUndoFile is only called by FlushBlockFile and WriteUndoDataForBlock. For both these cases, the flush error is not further bubbled upsince we don't > want to fail the function if the flushing of already written blocks > fails. Instead, the error is logged and a comjust document is provided why bubbling up an error would be less desirable in these casesit. > blockstorage: Log on fatal undo file flush error > > By returning an error code if either `FlushUndoFile` or `FlushBlockFile` > fail, the caller now has to explicitly handle block undo file flushing > errors. Before this change such errors were non-explicitly ignored > without a clear rationale. > > Besides the call to `FlushUndoFile` in `FlushBlockFile`, ignore its > return code at its call site in `WriteUndoDataForBlock`. There, a failed > flush of the undo data should not be indicative of a failed write. > > Add [[nodiscard]] annotations to `FlushUndoFile` such that its return > value is not just ignored in the future. This does not change the way flush errors are handled other than by adding log prints and comments to callsites. This is a backport of [[https://github.com/bitcoin/bitcoin/pull/27866 | core#27866]]