From 056cb3c0d2efb86447b7ff7788c206e2e7d72c12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 26 Mar 2025 12:40:20 +0100 Subject: [PATCH] refactor: clear up blockstorage/streams in preparation for optimization Made every OpenBlockFile#fReadOnly value explicit. Replaced hard-coded values in ReadRawBlock with STORAGE_HEADER_BYTES. Changed `STORAGE_HEADER_BYTES` and `UNDO_DATA_DISK_OVERHEAD` to `uint32_t` to avoid casts. Also added `LIFETIMEBOUND` to the `AutoFile` parameter of `BufferedFile`, which stores a reference to the underlying `AutoFile`, allowing Clang to emit warnings if the referenced `AutoFile` might be destroyed while `BufferedFile` still exists. Without this attribute, code with lifetime violations wouldn't trigger compiler warnings. Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com> --- src/node/blockstorage.cpp | 23 +++++++++++------------ src/node/blockstorage.h | 6 +++--- src/streams.h | 2 +- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 085a2460a6f..cf2e388c174 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -529,7 +529,7 @@ bool BlockManager::LoadBlockIndexDB(const std::optional& snapshot_block } for (std::set::iterator it = setBlkDataFiles.begin(); it != setBlkDataFiles.end(); it++) { FlatFilePos pos(*it, 0); - if (OpenBlockFile(pos, true).IsNull()) { + if (OpenBlockFile(pos, /*fReadOnly=*/true).IsNull()) { return false; } } @@ -933,7 +933,7 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt // Write undo information to disk if (block.GetUndoPos().IsNull()) { FlatFilePos pos; - const unsigned int blockundo_size{static_cast(GetSerializeSize(blockundo))}; + const auto blockundo_size{static_cast(GetSerializeSize(blockundo))}; if (!FindUndoPos(state, block.nFile, pos, blockundo_size + UNDO_DATA_DISK_OVERHEAD)) { LogError("FindUndoPos failed for %s while writing block undo", pos.ToString()); return false; @@ -996,7 +996,7 @@ bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos) const block.SetNull(); // Open history file to read - AutoFile filein{OpenBlockFile(pos, true)}; + AutoFile filein{OpenBlockFile(pos, /*fReadOnly=*/true)}; if (filein.IsNull()) { LogError("OpenBlockFile failed for %s while reading block", pos.ToString()); return false; @@ -1041,15 +1041,14 @@ bool BlockManager::ReadBlock(CBlock& block, const CBlockIndex& index) const bool BlockManager::ReadRawBlock(std::vector& block, const FlatFilePos& pos) const { - FlatFilePos hpos = pos; - // If nPos is less than 8 the pos is null and we don't have the block data - // Return early to prevent undefined behavior of unsigned int underflow - if (hpos.nPos < 8) { - LogError("Failed for %s while reading raw block", pos.ToString()); + if (pos.nPos < STORAGE_HEADER_BYTES) { + // If nPos is less than STORAGE_HEADER_BYTES, we can't read the header that precedes the block data + // This would cause an unsigned integer underflow when trying to position the file cursor + // This can happen after pruning or default constructed positions + LogError("Failed for %s while reading raw block storage header", pos.ToString()); return false; } - hpos.nPos -= 8; // Seek back 8 bytes for meta header - AutoFile filein{OpenBlockFile(hpos, true)}; + AutoFile filein{OpenBlockFile({pos.nFile, pos.nPos - STORAGE_HEADER_BYTES}, /*fReadOnly=*/true)}; if (filein.IsNull()) { LogError("OpenBlockFile failed for %s while reading raw block", pos.ToString()); return false; @@ -1091,7 +1090,7 @@ FlatFilePos BlockManager::WriteBlock(const CBlock& block, int nHeight) LogError("FindNextBlockPos failed for %s while writing block", pos.ToString()); return FlatFilePos(); } - AutoFile fileout{OpenBlockFile(pos)}; + AutoFile fileout{OpenBlockFile(pos, /*fReadOnly=*/false)}; if (fileout.IsNull()) { LogError("OpenBlockFile failed for %s while writing block", pos.ToString()); m_opts.notifications.fatalError(_("Failed to write block.")); @@ -1210,7 +1209,7 @@ void ImportBlocks(ChainstateManager& chainman, std::span import_ if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) { break; // No block files left to reindex } - AutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)}; + AutoFile file{chainman.m_blockman.OpenBlockFile(pos, /*fReadOnly=*/true)}; if (file.IsNull()) { break; // This error is logged in OpenBlockFile } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index e4da408afb0..324f8e68605 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -75,10 +75,10 @@ static const unsigned int UNDOFILE_CHUNK_SIZE = 0x100000; // 1 MiB static const unsigned int MAX_BLOCKFILE_SIZE = 0x8000000; // 128 MiB /** Size of header written by WriteBlock before a serialized CBlock (8 bytes) */ -static constexpr size_t STORAGE_HEADER_BYTES{std::tuple_size_v + sizeof(unsigned int)}; +static constexpr uint32_t STORAGE_HEADER_BYTES{std::tuple_size_v + sizeof(unsigned int)}; /** Total overhead when writing undo data: header (8 bytes) plus checksum (32 bytes) */ -static constexpr size_t UNDO_DATA_DISK_OVERHEAD{STORAGE_HEADER_BYTES + uint256::size()}; +static constexpr uint32_t UNDO_DATA_DISK_OVERHEAD{STORAGE_HEADER_BYTES + uint256::size()}; // Because validation code takes pointers to the map's CBlockIndex objects, if // we ever switch to another associative container, we need to either use a @@ -400,7 +400,7 @@ public: void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Open a block file (blk?????.dat) */ - AutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const; + AutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const; /** Translation to a filesystem path */ fs::path GetBlockPosFilename(const FlatFilePos& pos) const; diff --git a/src/streams.h b/src/streams.h index 20bdaf2c060..81d95feaa8e 100644 --- a/src/streams.h +++ b/src/streams.h @@ -523,7 +523,7 @@ private: } public: - BufferedFile(AutoFile& file, uint64_t nBufSize, uint64_t nRewindIn) + BufferedFile(AutoFile& file LIFETIMEBOUND, uint64_t nBufSize, uint64_t nRewindIn) : m_src{file}, nReadLimit{std::numeric_limits::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0}) { if (nRewindIn >= nBufSize)