blockstorage: Remove cs_LastBlockFile recursive mutex #35359

sedited commented at 12:44 PM on May 22, 2026: contributor

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 (besides for test-only code).

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 (e.g. in LoadBlockIndexDB).

Since it has no semantic meaning, and seems confusing to developers, remove it.

An alternative to this patch would be expanding the scope of what cs_LastBlockFile covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine.

DrahtBot added the label Block storage on May 22, 2026

DrahtBot commented at 12:44 PM on May 22, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35359.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, janb84, pablomartin4btc
Stale ACK	w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35307 (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sedited marked this as ready for review on May 22, 2026

stickies-v commented at 1:36 PM on May 22, 2026: contributor

Approach ACK. While directionally we aim to minimize cs_main usage, we should only introduce a new (well-scoped) mutex when it actually avoids acquiring cs_main, which cs_LastBlockFile seems to not at all do. Thus, making the code less confusing is a win.

PR seems pretty straightforward to review with solid assurances that the changes are safe.

w0xlt commented at 2:00 PM on May 22, 2026: contributor

Concept ACK.

in src/node/blockstorage.cpp:948 in 9dc6185d0f

 944 | @@ -950,10 +945,9 @@ void BlockManager::UpdateBlockInfo(const CBlock& block, unsigned int nHeight, co
 945 |  
 946 |  bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize)
 947 |  {
 948 | +    AssertLockHeld(cs_main);

janb84 commented at 12:05 PM on May 28, 2026:

NIT AssertLockHeld(cs_main); => AssertLockHeld(::cs_main); Align code with rest of the file, now that we are changing the file.

sedited commented at 1:08 PM on May 28, 2026:

Ugh, missed that one. Will take this if I have to push again, or if there are some more nits.

janb84 commented at 12:48 PM on May 28, 2026: contributor

ACK 9dc6185d0f76e45bfe611319fab235971896424e

To me the cs_LastBlockFile scoped mutex is a failure of follow-through. To me it was introduced with a vision that didn't come to fruition. Now it does not provide value and makes reasoning about the code harder.

By removing this code we are giving ourselves a fresh start if anyone ever wants narrower locking here again.

I did a manual code review, and a race condition search together with Claude but could not find that this change introduces a race-condition.

Added a NIT, to align the code now that we are touching it.

DrahtBot requested review from stickies-v on May 28, 2026

w0xlt commented at 8:39 PM on May 30, 2026: contributor

ACK 9dc6185d0f76e45bfe611319fab235971896424e

The relevant call sites that used cs_LastBlockFile already hold cs_main, and the protected state is tied to block index / pruning / chainstate flush logic that is also serialized through cs_main.

So in the current code, cs_LastBlockFile seems redundant and makes the locking model harder to reason about.

A narrower block storage mutex could still be introduced in a future cs_main refactor, but this mutex does not provide that separation today. Removing it looks like a good cleanup.

in src/node/blockstorage.h:382 in 9dc6185d0f

 378 | @@ -381,7 +379,7 @@ class BlockManager
 379 |      const CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 380 |  
 381 |      /** Get block file info entry for one block file */
 382 | -    CBlockFileInfo* GetBlockFileInfo(size_t n);
 383 | +    CBlockFileInfo* GetBlockFileInfo(size_t n) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

pablomartin4btc commented at 7:38 PM on June 7, 2026:

Following same pattern as noticed by @janb84 earlier, only if you have to re-touch:

    CBlockFileInfo* GetBlockFileInfo(size_t n) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

in src/validation.cpp:2716 in 9dc6185d0f outdated

2712 | @@ -2715,7 +2713,6 @@ bool Chainstate::FlushStateToDisk(
2713 |          bool fFlushForPrune = false;
2714 |

pablomartin4btc commented at 8:10 PM on June 7, 2026:

One question: after removing cs_LastBlockFile here, the pruning/blockfile state accessed below is now protected by ::cs_main, which FlushStateToDisk() takes internally. Would it make sense to make that locking contract explicit on FlushStateToDisk() as well, or is the intention to keep this function responsible for taking ::cs_main itself?

I noticed some callers, e.g. AcceptBlock(), already holds cs_main before calling it, but I have not checked whether all callers do. If they do, perhaps FlushStateToDisk() could eventually become EXCLUSIVE_LOCKS_REQUIRED(::cs_main) and use AssertLockHeld(::cs_main) instead of locking it internally.

sedited commented at 8:58 PM on June 7, 2026:

I think this is one of the places where it just isn't clear at all what cs_LastBlockFile is responsible for (as far as I can tell you can just remove it in FlushStateToDisk on current master and nothing will fail). I purposefully did not want to add or change any cs_main locks in production code in this pull request. The patches here are supposed to demonstrate to the reviewer that cs_LastBlockFile is redundant in all production code cases. Further cleanup should probably be done in a separate pull request with a new motivation.

Would it make sense to make that locking contract explicit on FlushStateToDisk() as well, or is the intention to keep this function responsible for taking ::cs_main itself?

Yes, this could make sense, but I think it is out of scope here. There are many other such cases throughout the code and we should work on them eventually.

pablomartin4btc commented at 8:22 PM on June 7, 2026: member

Concept ACK.

This removes cs_LastBlockFile and makes the existing dependency on ::cs_main explicit through lock annotations/ assertions, which seems like a simplification compared to keeping a separate recursive mutex whose locking relationship with cs_main was less clear.

One caveat is that this moves the blockfile cursor/ info state under ::cs_main, which is itself still the major remaining RecursiveMutex listed in #19303. So this does not directly reduce cs_main usage, but it does seem to make the current locking model easier to reason about before any future attempt to split out a narrower blockstorage lock.

in src/node/blockstorage.h:228 in 9dc6185d0f

 222 | @@ -223,9 +223,9 @@ class BlockManager
 223 |       * The nAddSize argument passed to this function should include not just the size of the serialized CBlock, but also the size of
 224 |       * separator fields (STORAGE_HEADER_BYTES).
 225 |       */
 226 | -    [[nodiscard]] FlatFilePos FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime);
 227 | -    [[nodiscard]] bool FlushChainstateBlockFile(int tip_height);
 228 | -    bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize);
 229 | +    [[nodiscard]] FlatFilePos FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 230 | +    [[nodiscard]] bool FlushChainstateBlockFile(int tip_height) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 231 | +    bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

stickies-v commented at 1:52 PM on June 8, 2026:

nit: while touching, would be good to also make this [[nodiscard]]

in src/node/blockstorage.h:275 in 9dc6185d0f outdated

 272 | +        m_blockfile_cursors GUARDED_BY(::cs_main) = {
 273 |              BlockfileCursor{},
 274 |              std::nullopt,
 275 |      };
 276 | -    int MaxBlockfileNum() const EXCLUSIVE_LOCKS_REQUIRED(cs_LastBlockFile)
 277 | +    int MaxBlockfileNum() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)

stickies-v commented at 2:25 PM on June 8, 2026:

Any reason we're not also using AssertLockHeld here?

diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index a601cd2659..df2d8aa3bf 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -274,6 +274,7 @@ private:
     };
     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);

</details>

in src/node/blockstorage.cpp:530 in 9dc6185d0f

 526 | @@ -528,12 +527,13 @@ void BlockManager::WriteBlockIndexDB()
 527 |          vBlocks.push_back(*it);
 528 |          m_dirty_blockindex.erase(it++);
 529 |      }
 530 | -    int max_blockfile = WITH_LOCK(cs_LastBlockFile, return this->MaxBlockfileNum());
 531 | +    int max_blockfile = this->MaxBlockfileNum();

stickies-v commented at 2:32 PM on June 8, 2026:

narrowing nit (+ in ScanAndUnlinkAlreadyPrunedFiles)

    int max_blockfile{this->MaxBlockfileNum()};

stickies-v approved

stickies-v commented at 2:37 PM on June 8, 2026: contributor

ACK 9dc6185d0f76e45bfe611319fab235971896424e

DrahtBot requested review from pablomartin4btc on Jun 8, 2026

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.

ec6cf49b91

sedited force-pushed on Jun 8, 2026

sedited commented at 3:00 PM on June 8, 2026: contributor

Thanks for the reviews, decided to push to address the nits:

Updated 9dc6185d0f76e45bfe611319fab235971896424e -> ec6cf49b91f726aa8cb70087eb3fcacb14b4ff22 (rmLastBlockFileMutex_0 -> rmLastBlockFileMutex_1, compare)

Addressed @stickies-v's comment, added nodiscard
Addressed @stickies-v's comment, added AssertLockHeld. Skipped before because I though it does not provide much value with the annotation right next to it.
Addressed @stickies-v's comment, guard against narrowing.
Addresed @janb84's comment and @pablomartin4btc's comment, use :: for cs_main consistently.

stickies-v commented at 3:02 PM on June 8, 2026: contributor

re-ACK ec6cf49b91f726aa8cb70087eb3fcacb14b4ff22

DrahtBot requested review from janb84 on Jun 8, 2026

DrahtBot requested review from w0xlt on Jun 8, 2026

janb84 commented at 3:11 PM on June 8, 2026: contributor

re- ACK ec6cf49b91f726aa8cb70087eb3fcacb14b4ff22

pablomartin4btc commented at 3:19 PM on June 8, 2026: member

ACK ec6cf49b91f726aa8cb70087eb3fcacb14b4ff22

fanquake merged this on Jun 8, 2026

fanquake closed this on Jun 8, 2026