From d8041d4e042957660827313951b18c8dd9a99a16 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Tue, 25 Jul 2023 12:03:26 +0200 Subject: [PATCH] blockstorage: Return 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. --- src/node/blockstorage.cpp | 19 ++++++++++++++++--- src/node/blockstorage.h | 6 ++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index d5f4fed995e..934088aeed7 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -538,12 +538,14 @@ bool BlockManager::UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& in return true; } -void BlockManager::FlushUndoFile(int block_file, bool finalize) +bool BlockManager::FlushUndoFile(int block_file, bool finalize) { FlatFilePos undo_pos_old(block_file, m_blockfile_info[block_file].nUndoSize); if (!UndoFileSeq().Flush(undo_pos_old, finalize)) { m_opts.notifications.flushError("Flushing undo file to disk failed. This is likely the result of an I/O error."); + return false; } + return true; } bool BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) @@ -567,7 +569,11 @@ bool BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) } // we do not always flush the undo file, as the chain tip may be lagging behind the incoming blocks, // e.g. during IBD or a sync after a node going offline - if (!fFinalize || finalize_undo) FlushUndoFile(m_last_blockfile, finalize_undo); + if (!fFinalize || finalize_undo) { + if (!FlushUndoFile(m_last_blockfile, finalize_undo)) { + success = false; + } + } return success; } @@ -764,7 +770,14 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid // with the block writes (usually when a synced up node is getting newly mined blocks) -- this case is caught in // the FindBlockPos function if (_pos.nFile < m_last_blockfile && static_cast(block.nHeight) == m_blockfile_info[_pos.nFile].nHeightLast) { - FlushUndoFile(_pos.nFile, true); + // Do not propagate the return code, a failed flush here should not + // be an indication for a failed write. If it were propagated here, + // the caller would assume the undo data not to be written, when in + // fact it is. Note though, that a failed flush might leave the data + // file untrimmed. + if (!FlushUndoFile(_pos.nFile, true)) { + LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, "Failed to flush undo file %05i\n", _pos.nFile); + } } else if (_pos.nFile == m_last_blockfile && static_cast(block.nHeight) > m_undo_height_in_last_blockfile) { m_undo_height_in_last_blockfile = block.nHeight; } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 3e0962386f7..9950d9c477b 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -92,10 +92,12 @@ class BlockManager bool LoadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_main); - /** Return false if block file flushing fails. */ + /** Return false if block file or undo file flushing fails. */ [[nodiscard]] bool FlushBlockFile(bool fFinalize = false, bool finalize_undo = false); - void FlushUndoFile(int block_file, bool finalize = false); + /** Return false if undo file flushing fails. */ + [[nodiscard]] bool FlushUndoFile(int block_file, bool finalize = false); + [[nodiscard]] bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown); bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize);