From ec6cf49b91f726aa8cb70087eb3fcacb14b4ff22 Mon Sep 17 00:00:00 2001 From: sedited Date: Thu, 21 May 2026 16:09:32 +0200 Subject: [PATCH] blockstorage: Remove cs_LastBlockFile recursive mutex The cs_LastBlockFile mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking cs_LastBlockFile with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced. It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info`. Since it has no semantic meaning, and seems confusing to developers, remove it. --- src/bench/readwriteblock.cpp | 5 +++-- src/node/blockstorage.cpp | 33 ++++++++++++++------------------- src/node/blockstorage.h | 23 +++++++++++------------ src/test/blockmanager_tests.cpp | 2 ++ src/validation.cpp | 1 - 5 files changed, 30 insertions(+), 34 deletions(-) diff --git a/src/bench/readwriteblock.cpp b/src/bench/readwriteblock.cpp index b8e226c6eb9..f1ad24a6d6a 100644 --- a/src/bench/readwriteblock.cpp +++ b/src/bench/readwriteblock.cpp @@ -32,6 +32,7 @@ static void WriteBlockBench(benchmark::Bench& bench) auto& blockman{testing_setup->m_node.chainman->m_blockman}; const CBlock block{CreateTestBlock()}; bench.run([&] { + LOCK(::cs_main); const auto pos{blockman.WriteBlock(block, 413'567)}; assert(!pos.IsNull()); }); @@ -43,7 +44,7 @@ static void ReadBlockBench(benchmark::Bench& bench) auto& blockman{testing_setup->m_node.chainman->m_blockman}; const auto& test_block{CreateTestBlock()}; const auto& expected_hash{test_block.GetHash()}; - const auto& pos{blockman.WriteBlock(test_block, 413'567)}; + const auto& pos{WITH_LOCK(::cs_main, return blockman.WriteBlock(test_block, 413'567))}; bench.run([&] { CBlock block; const auto success{blockman.ReadBlock(block, pos, expected_hash)}; @@ -55,7 +56,7 @@ static void ReadRawBlockBench(benchmark::Bench& bench) { const auto testing_setup{MakeNoLogFileContext(ChainType::MAIN)}; auto& blockman{testing_setup->m_node.chainman->m_blockman}; - const auto pos{blockman.WriteBlock(CreateTestBlock(), 413'567)}; + const auto pos{WITH_LOCK(::cs_main, return blockman.WriteBlock(CreateTestBlock(), 413'567))}; bench.run([&] { const auto res{blockman.ReadRawBlock(pos)}; assert(res); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index b0842a00505..8cc93fb46c8 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -258,7 +258,6 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, CBlockInde void BlockManager::PruneOneBlockFile(const int fileNumber) { AssertLockHeld(cs_main); - LOCK(cs_LastBlockFile); for (auto& entry : m_block_index) { CBlockIndex* pindex = &entry.second; @@ -296,7 +295,7 @@ void BlockManager::FindFilesToPruneManual( { assert(IsPruneMode() && nManualPruneHeight > 0); - LOCK2(cs_main, cs_LastBlockFile); + LOCK(::cs_main); if (chain.m_chain.Height() < 0) { return; } @@ -324,7 +323,7 @@ void BlockManager::FindFilesToPrune( const Chainstate& chain, ChainstateManager& chainman) { - LOCK2(cs_main, cs_LastBlockFile); + LOCK(::cs_main); // Compute `target` value with maximum size (in bytes) of blocks below the // `last_prune` height which should be preserved and not pruned. The // `target` value will be derived from the -prune preference provided by the @@ -528,12 +527,13 @@ void BlockManager::WriteBlockIndexDB() vBlocks.push_back(*it); m_dirty_blockindex.erase(it++); } - int max_blockfile = WITH_LOCK(cs_LastBlockFile, return this->MaxBlockfileNum()); + int max_blockfile{this->MaxBlockfileNum()}; m_block_tree_db->WriteBatchSync(vFiles, max_blockfile, vBlocks); } bool BlockManager::LoadBlockIndexDB(const std::optional& snapshot_blockhash) { + AssertLockHeld(::cs_main); if (!LoadBlockIndex(snapshot_blockhash)) { return false; } @@ -573,7 +573,6 @@ bool BlockManager::LoadBlockIndexDB(const std::optional& snapshot_block { // Initialize the blockfile cursors. - LOCK(cs_LastBlockFile); for (size_t i = 0; i < m_blockfile_info.size(); ++i) { const auto last_height_in_file = m_blockfile_info[i].nHeightLast; m_blockfile_cursors[BlockfileTypeForHeight(last_height_in_file)] = {static_cast(i), 0}; @@ -597,7 +596,7 @@ bool BlockManager::LoadBlockIndexDB(const std::optional& snapshot_block void BlockManager::ScanAndUnlinkAlreadyPrunedFiles() { AssertLockHeld(::cs_main); - int max_blockfile = WITH_LOCK(cs_LastBlockFile, return this->MaxBlockfileNum()); + int max_blockfile{this->MaxBlockfileNum()}; if (!m_have_pruned) { return; } @@ -695,8 +694,7 @@ void BlockManager::CleanupBlockRevFiles() const CBlockFileInfo* BlockManager::GetBlockFileInfo(size_t n) { - LOCK(cs_LastBlockFile); - + AssertLockHeld(::cs_main); return &m_blockfile_info.at(n); } @@ -747,8 +745,8 @@ bool BlockManager::FlushUndoFile(int block_file, bool finalize) bool BlockManager::FlushBlockFile(int blockfile_num, bool fFinalize, bool finalize_undo) { + AssertLockHeld(::cs_main); bool success = true; - LOCK(cs_LastBlockFile); if (m_blockfile_info.size() < 1) { // Return if we haven't loaded any blockfiles yet. This happens during @@ -784,7 +782,7 @@ BlockfileType BlockManager::BlockfileTypeForHeight(int height) bool BlockManager::FlushChainstateBlockFile(int tip_height) { - LOCK(cs_LastBlockFile); + AssertLockHeld(::cs_main); auto& cursor = m_blockfile_cursors[BlockfileTypeForHeight(tip_height)]; // If the cursor does not exist, it means an assumeutxo snapshot is loaded, // but no blocks past the snapshot height have been written yet, so there @@ -798,8 +796,7 @@ bool BlockManager::FlushChainstateBlockFile(int tip_height) uint64_t BlockManager::CalculateCurrentUsage() { - LOCK(cs_LastBlockFile); - + AssertLockHeld(::cs_main); uint64_t retval = 0; for (const CBlockFileInfo& file : m_blockfile_info) { retval += file.nSize + file.nUndoSize; @@ -838,8 +835,7 @@ fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime) { - LOCK(cs_LastBlockFile); - + AssertLockHeld(::cs_main); const BlockfileType chain_type = BlockfileTypeForHeight(nHeight); if (!m_blockfile_cursors[chain_type]) { @@ -928,8 +924,7 @@ FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int n void BlockManager::UpdateBlockInfo(const CBlock& block, unsigned int nHeight, const FlatFilePos& pos) { - LOCK(cs_LastBlockFile); - + AssertLockHeld(::cs_main); // Update the cursor so it points to the last file. const BlockfileType chain_type{BlockfileTypeForHeight(nHeight)}; auto& cursor{m_blockfile_cursors[chain_type]}; @@ -950,10 +945,9 @@ void BlockManager::UpdateBlockInfo(const CBlock& block, unsigned int nHeight, co bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize) { + AssertLockHeld(::cs_main); pos.nFile = nFile; - LOCK(cs_LastBlockFile); - pos.nPos = m_blockfile_info[nFile].nUndoSize; m_blockfile_info[nFile].nUndoSize += nAddSize; m_dirty_fileinfo.insert(nFile); @@ -974,7 +968,7 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt { AssertLockHeld(::cs_main); const BlockfileType type = BlockfileTypeForHeight(block.nHeight); - auto& cursor = *Assert(WITH_LOCK(cs_LastBlockFile, return m_blockfile_cursors[type])); + auto& cursor = *Assert(m_blockfile_cursors[type]); // Write undo information to disk if (block.GetUndoPos().IsNull()) { @@ -1139,6 +1133,7 @@ BlockManager::ReadRawBlockResult BlockManager::ReadRawBlock(const FlatFilePos& p FlatFilePos BlockManager::WriteBlock(const CBlock& block, int nHeight) { + AssertLockHeld(::cs_main); const unsigned int block_size{static_cast(GetSerializeSize(TX_WITH_WITNESS(block)))}; FlatFilePos pos{FindNextBlockPos(block_size + STORAGE_HEADER_BYTES, nHeight, block.GetBlockTime())}; if (pos.IsNull()) { diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 0857ba4c8a8..347dbc3c9e8 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -209,7 +209,7 @@ private: EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Return false if block file or undo file flushing fails. */ - [[nodiscard]] bool FlushBlockFile(int blockfile_num, bool fFinalize, bool finalize_undo); + [[nodiscard]] bool FlushBlockFile(int blockfile_num, bool fFinalize, bool finalize_undo) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Return false if undo file flushing fails. */ [[nodiscard]] bool FlushUndoFile(int block_file, bool finalize = false); @@ -223,9 +223,9 @@ private: * The nAddSize argument passed to this function should include not just the size of the serialized CBlock, but also the size of * separator fields (STORAGE_HEADER_BYTES). */ - [[nodiscard]] FlatFilePos FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime); - [[nodiscard]] bool FlushChainstateBlockFile(int tip_height); - bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize); + [[nodiscard]] FlatFilePos FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + [[nodiscard]] bool FlushChainstateBlockFile(int tip_height) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + [[nodiscard]] bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); AutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const; @@ -257,8 +257,6 @@ private: const Chainstate& chain, ChainstateManager& chainman); - RecursiveMutex cs_LastBlockFile; - //! Since assumedvalid chainstates may be syncing a range of the chain that is very //! far away from the normal/background validation process, we should segment blockfiles //! for assumed chainstates. Otherwise, we might have wildly different height ranges @@ -270,12 +268,13 @@ private: //! //! The first element is the NORMAL cursor, second is ASSUMED. std::array, BlockfileType::NUM_TYPES> - m_blockfile_cursors GUARDED_BY(cs_LastBlockFile) = { + m_blockfile_cursors GUARDED_BY(::cs_main) = { BlockfileCursor{}, std::nullopt, }; - int MaxBlockfileNum() const EXCLUSIVE_LOCKS_REQUIRED(cs_LastBlockFile) + int MaxBlockfileNum() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); static const BlockfileCursor empty_cursor; const auto& normal = m_blockfile_cursors[BlockfileType::NORMAL].value_or(empty_cursor); const auto& assumed = m_blockfile_cursors[BlockfileType::ASSUMED].value_or(empty_cursor); @@ -381,7 +380,7 @@ public: const CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Get block file info entry for one block file */ - CBlockFileInfo* GetBlockFileInfo(size_t n); + CBlockFileInfo* GetBlockFileInfo(size_t n) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex& block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); @@ -394,7 +393,7 @@ public: * @returns in case of success, the position to which the block was written to * in case of an error, an empty FlatFilePos */ - FlatFilePos WriteBlock(const CBlock& block, int nHeight); + FlatFilePos WriteBlock(const CBlock& block, int nHeight) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Update blockfile info while processing a block during reindex. The block must be available on disk. * @@ -402,7 +401,7 @@ public: * @param[in] nHeight the height of the block * @param[in] pos the position of the serialized CBlock on disk */ - void UpdateBlockInfo(const CBlock& block, unsigned int nHeight, const FlatFilePos& pos); + void UpdateBlockInfo(const CBlock& block, unsigned int nHeight, const FlatFilePos& pos) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Whether running in -prune mode. */ [[nodiscard]] bool IsPruneMode() const { return m_prune_mode; } @@ -414,7 +413,7 @@ public: [[nodiscard]] bool LoadingBlocks() const { return m_importing || !m_blockfiles_indexed; } /** Calculate the amount of disk space the block & undo files currently use */ - uint64_t CalculateCurrentUsage(); + uint64_t CalculateCurrentUsage() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! Check if all blocks in the [upper_block, lower_block] range have data available as //! defined by the status mask. diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 6ca4eb9b07a..2eea73d04d3 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -42,6 +42,7 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) }; BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts}; // simulate adding a genesis block normally + LOCK(::cs_main); BOOST_CHECK_EQUAL(blockman.WriteBlock(params->GenesisBlock(), 0).nPos, STORAGE_HEADER_BYTES); // simulate what happens during reindex // simulate a well-formed genesis block being found at offset 8 in the blk00000.dat file @@ -257,6 +258,7 @@ BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file) constexpr int TEST_BLOCK_SIZE{81}; // Blockstore is empty + LOCK(::cs_main); BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), 0); // Write the first block to a new location. diff --git a/src/validation.cpp b/src/validation.cpp index 211a8122a95..b254f11e483 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2713,7 +2713,6 @@ bool Chainstate::FlushStateToDisk( bool fFlushForPrune = false; CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(); - LOCK(m_blockman.cs_LastBlockFile); if (m_blockman.IsPruneMode() && (m_blockman.m_check_for_pruning || nManualPruneHeight > 0) && m_chainman.m_blockman.m_blockfiles_indexed) { // make sure we don't prune above any of the prune locks bestblocks // pruning is height-based