highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-09-12 05:32:22 +02:00
Code Issues Packages Projects Releases Wiki Activity

blockstorage: Remove cs_LastBlockFile recursive mutex

Browse Source 
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.
This commit is contained in:
sedited
2026-05-21 16:09:32 +02:00
parent 0553ce5ddb
commit ec6cf49b91
5 changed files with 30 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
src/bench/readwriteblock.cpp
View File

@@ -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<const TestingSetup>(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);

33
src/node/blockstorage.cpp
View File

@@ -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<uint256>& snapshot_blockhash)
{
AssertLockHeld(::cs_main);
if (!LoadBlockIndex(snapshot_blockhash)) {
return false;
}
@@ -573,7 +573,6 @@ bool BlockManager::LoadBlockIndexDB(const std::optional<uint256>& 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<int>(i), 0};
@@ -597,7 +596,7 @@ bool BlockManager::LoadBlockIndexDB(const std::optional<uint256>& 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<unsigned int>(GetSerializeSize(TX_WITH_WITNESS(block)))};
FlatFilePos pos{FindNextBlockPos(block_size + STORAGE_HEADER_BYTES, nHeight, block.GetBlockTime())};
if (pos.IsNull()) {

23
src/node/blockstorage.h
View File

@@ -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<std::optional<BlockfileCursor>, 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.

2
src/test/blockmanager_tests.cpp
View File

@@ -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.

1
src/validation.cpp
View File

@@ -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