Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

This is annotating the function, but not the underlying data member that needs the annotation.

Not sure how involved it will be to annotate everything properly here. Also, given that cs_main is the validation lock, we should (eventually/ideally) have a separate node_storage lock. Though, this again requires a major validation/node_storage rewrite.

However an argument against adding the lock annotation to the data member could be that chain is supposed to be a data-structure-only primitive, like transaction or block. In that case only annotating the getter and setter functions (and making them standalone) would seem appropriate.

Having a quick look at annotating nStatus to assess.

Appended a small commit to guard CBlockIndex::nStatus by cs_main as a starting point.

nit: unit tests are single-threaded, so you can just take the lock for the whole function. It is likely that there are other lock violations anyway: See #17161 (comment)

done

Same: Can include this in the cs_main scope as well, I think.

done, thank you

e8cc6987f34d19f88de8c461af1efcdd8c3825ec

Missing

AssertLockHeld(cs_main);

Thanks @promag, done.

9bec673716e68b1854562e4e48a6cb26d8949a59

Missing

AssertLockHeld(cs_main);

Done.

9bec673716e68b1854562e4e48a6cb26d8949a59

Why is this needed? WriteUndoDataForBlock is only called from CChainState::ConnectBlock which already locks cs_main.

@promag You're right; Clang 13 seems unable to detect this on its own, but the unneeded lock can be avoided by requiring WriteUndoDataForBlock() to hold cs_main instead. Done in 686ae2d97.

9cacc50225f5d8990aeb4bd3cb6d03a18d776ff0

Again, cs_main is already held here right?

Thanks! Removed in both places in this function as described in #22932 (review).

9cacc50225f5d8990aeb4bd3cb6d03a18d776ff0

nit,

const bool block_have_data{WITH_LOCK(cs_main, return blockindex->nStatus & BLOCK_HAVE_DATA)};

Done.

How about getting rid of this function entirely:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2a30afdb5..030a8c736 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1848,7 +1848,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
         // Fast-path: in this case it is possible to serve the block directly from disk,
         // as the network format matches the format on disk
         std::vector<uint8_t> block_data;
-        if (!ReadRawBlockFromDisk(block_data, pindex, m_chainparams.MessageStart())) {
+        if (!ReadRawBlockFromDisk(block_data, pindex->GetBlockPos(), m_chainparams.MessageStart())) {
             assert(!"cannot load block from disk");
         }
         m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, Span{block_data}));
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 1af24ecf6..bf47ab351 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -446,12 +446,6 @@ bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, c
     return true;
 }
 
-bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start)
-{
-    const FlatFilePos block_pos{WITH_LOCK(cs_main, return pindex->GetBlockPos())};
-    return ReadRawBlockFromDisk(block, block_pos, message_start);
-}
-
 /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
 FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp)
 {
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index 93e3b1e93..6872848f2 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -73,7 +73,6 @@ void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
 bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
 bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
 bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start);
-bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start);
 
 bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);
 bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

?

Done

style nit, here and after: For new code maybe be explicit about the global namespace:

extern RecursiveMutex ::cs_main;

?

Done.

style nit: Is it more readable:

bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)
    EXCLUSIVE_LOCKS_REQUIRED(cs_main);

?

Not to me ;) but ok, done

0bfa32b131ed2da8799e1d1239bf390dd612b7bb style nit: clang-format-diff.py insists on:

        GetSizeOfCompactSize(block.vtx.size())};

done

0bfa32b131ed2da8799e1d1239bf390dd612b7bb (here and after) UnlinkPrunedFiles includes a logging operation. Could we keep logging out of the ::cs_main lock?

done

Why a scope of this lock is so wide?

Scope narrowed per git diff 71bfaa1 e9560c6

--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -308,7 +308,6 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
             CDiskBlockIndex diskindex;
             if (pcursor->GetValue(diskindex)) {
                 // Construct block index object
-                LOCK(::cs_main);
                 CBlockIndex* pindexNew = insertBlockIndex(diskindex.GetBlockHash());
                 pindexNew->pprev          = insertBlockIndex(diskindex.hashPrev);
                 pindexNew->nHeight        = diskindex.nHeight;
@@ -320,11 +319,15 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
                 pindexNew->nTime          = diskindex.nTime;
                 pindexNew->nBits          = diskindex.nBits;
                 pindexNew->nNonce         = diskindex.nNonce;
-                pindexNew->nStatus        = diskindex.nStatus;
                 pindexNew->nTx            = diskindex.nTx;
+                {
+                    LOCK(::cs_main);
+                    pindexNew->nStatus = diskindex.nStatus;
+                }

All places that call IsValid() already own cs_main. Thus I think it is better just to mandate that it is required when calling this function and avoid locking it recursively:

<details> <summary>suggested change</summary>

     //! Check whether this block index entry is valid up to the passed validity level.
     bool IsValid(enum BlockStatus nUpTo = BLOCK_VALID_TRANSACTIONS) const
+        EXCLUSIVE_LOCKS_REQUIRED(cs_main)
     {
+        AssertLockHeld(cs_main);
         assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed.
-        LOCK(::cs_main);
         if (nStatus & BLOCK_FAILED_MASK) {

</details>

That is indeed better (thanks!). ACK, tested, done.

Same as IsValid() (except some tests):

<details> <summary>suggested change</summary>

diff --git i/src/chain.h w/src/chain.h
index 2a7e420c31..291e684eef 100644
--- i/src/chain.h
+++ w/src/chain.h

[...]
     //! [@returns](/bitcoin-bitcoin/contributor/returns/) true if the block is assumed-valid; this means it is queued to be
     //!   validated by a background chainstate.
-    bool IsAssumedValid() const
+    bool IsAssumedValid() const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
     {
-        LOCK(::cs_main);
+        AssertLockHeld(cs_main);
         return nStatus & BLOCK_ASSUMED_VALID;
     }
[...]

diff --git i/src/test/validation_chainstatemanager_tests.cpp w/src/test/validation_chainstatemanager_tests.cpp
index 47d9a77126..e3fd1f203a 100644
--- i/src/test/validation_chainstatemanager_tests.cpp
+++ w/src/test/validation_chainstatemanager_tests.cpp
@@ -230,13 +230,16 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
     BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash->IsNull());
     BOOST_CHECK_EQUAL(
         *chainman.ActiveChainstate().m_from_snapshot_blockhash,
         *chainman.SnapshotBlockhash());
 
     // Ensure that the genesis block was not marked assumed-valid.
-    BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid());
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid());
+    }
 
     const AssumeutxoData& au_data = *ExpectedAssumeutxo(snapshot_height, ::Params());
     const CBlockIndex* tip = chainman.ActiveTip();
 
     BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
 
@@ -351,16 +354,16 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
     // tip candidates.
     reload_all_block_indexes();
     BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), cs1.m_chain.Height() + 1);
 
     // Mark some region of the chain assumed-valid.
     for (int i = 0; i <= cs1.m_chain.Height(); ++i) {
+        LOCK(::cs_main);
         auto index = cs1.m_chain[i];
 
         if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) {
-            LOCK(::cs_main);
             index->nStatus = BlockStatus::BLOCK_VALID_TREE | BlockStatus::BLOCK_ASSUMED_VALID;
         }
 
         ++num_indexes;
         if (index->IsAssumedValid()) ++num_assumed_valid;

</details>

ACK, tested, done.

Same as IsValid() (except some tests):

<details> <summary>suggested change</summary>

diff --git i/src/chain.h w/src/chain.h
index 2a7e420c31..291e684eef 100644
--- i/src/chain.h
+++ w/src/chain.h

[...]
     //! Raise the validity level of this block index entry.
     //! Returns true if the validity was changed.
-    bool RaiseValidity(enum BlockStatus nUpTo)
+    bool RaiseValidity(enum BlockStatus nUpTo) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
     {
+        AssertLockHeld(cs_main);
         assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed.
-        LOCK(::cs_main);
         if (nStatus & BLOCK_FAILED_MASK) return false;
[...]

diff --git i/src/test/fuzz/chain.cpp w/src/test/fuzz/chain.cpp
index 4f97e1ebb1..e4fd2b32f6 100644
--- i/src/test/fuzz/chain.cpp
+++ w/src/test/fuzz/chain.cpp
@@ -55,13 +55,13 @@ FUZZ_TARGET(chain)
             BlockStatus::BLOCK_FAILED_MASK,
             BlockStatus::BLOCK_OPT_WITNESS,
         });
         if (block_status & ~BLOCK_VALID_MASK) {
             continue;
         }
-        (void)disk_block_index->RaiseValidity(block_status);
+        WITH_LOCK(cs_main, (void)disk_block_index->RaiseValidity(block_status));
     }
 
     CBlockIndex block_index{block_header};
     block_index.phashBlock = &zero;
     (void)block_index.GetBlockHash();
     (void)block_index.ToString();
diff --git i/src/test/validation_chainstatemanager_tests.cpp w/src/test/validation_chainstatemanager_tests.cpp
index 47d9a77126..e3fd1f203a 100644
--- i/src/test/validation_chainstatemanager_tests.cpp
+++ w/src/test/validation_chainstatemanager_tests.cpp
@@ -230,13 +230,16 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
     BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash->IsNull());
     BOOST_CHECK_EQUAL(
         *chainman.ActiveChainstate().m_from_snapshot_blockhash,
         *chainman.SnapshotBlockhash());
 
     // Ensure that the genesis block was not marked assumed-valid.
-    BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid());
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid());
+    }
 
     const AssumeutxoData& au_data = *ExpectedAssumeutxo(snapshot_height, ::Params());
     const CBlockIndex* tip = chainman.ActiveTip();
 
     BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
 
@@ -351,16 +354,16 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
     // tip candidates.
     reload_all_block_indexes();
     BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), cs1.m_chain.Height() + 1);
 
     // Mark some region of the chain assumed-valid.
     for (int i = 0; i <= cs1.m_chain.Height(); ++i) {
+        LOCK(::cs_main);
         auto index = cs1.m_chain[i];
 
         if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) {
-            LOCK(::cs_main);
             index->nStatus = BlockStatus::BLOCK_VALID_TREE | BlockStatus::BLOCK_ASSUMED_VALID;
         }
 
         ++num_indexes;
         if (index->IsAssumedValid()) ++num_assumed_valid;

</details>

Done.

Same as IsValid(), except 3 of 4 callers of IsBlockPruned() already own cs_main. The 4th caller, blockToJSON() could be changed to acquire the mutex.

<details> <summary>suggested change</summary>

diff --git i/src/node/blockstorage.cpp w/src/node/blockstorage.cpp
index 5074080ed1..2217a4d6c8 100644
--- i/src/node/blockstorage.cpp
+++ w/src/node/blockstorage.cpp
@@ -424,13 +424,13 @@ CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
     }
     return nullptr;
 }
 
 bool IsBlockPruned(const CBlockIndex* pblockindex)
 {
-    LOCK(::cs_main);
+    AssertLockHeld(cs_main);
     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
 }
 
 // If we're using -prune with -reindex, then delete block files that will be ignored by the
 // reindex.  Since reindexing works by starting at block file 0 and looping until a blockfile
 // is missing, do the same here to delete any later block files after a gap.  Also delete all
diff --git i/src/node/blockstorage.h w/src/node/blockstorage.h
index 7efa7e3b72..0cdab404da 100644
--- i/src/node/blockstorage.h
+++ w/src/node/blockstorage.h
@@ -163,13 +163,13 @@ public:
     {
         Unload();
     }
 };
 
 //! Check whether the block associated with this index entry is pruned or not.
-bool IsBlockPruned(const CBlockIndex* pblockindex);
+bool IsBlockPruned(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
 void CleanupBlockRevFiles();
 
 /** Open a block file (blk?????.dat) */
 FILE* OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false);
 /** Translation to a filesystem path */
diff --git i/src/rpc/blockchain.cpp w/src/rpc/blockchain.cpp
index 455c32c57a..d7c1572099 100644
--- i/src/rpc/blockchain.cpp
+++ w/src/rpc/blockchain.cpp
@@ -172,13 +172,17 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIn
             }
             break;
 
         case TxVerbosity::SHOW_DETAILS:
         case TxVerbosity::SHOW_DETAILS_AND_PREVOUT:
             CBlockUndo blockUndo;
-            const bool have_undo = !IsBlockPruned(blockindex) && UndoReadFromDisk(blockUndo, blockindex);
+            bool have_undo;
+            {
+                LOCK(cs_main);
+                have_undo = !IsBlockPruned(blockindex) && UndoReadFromDisk(blockUndo, blockindex);
+            }
 
             for (size_t i = 0; i < block.vtx.size(); ++i) {
                 const CTransactionRef& tx = block.vtx.at(i);
                 // coinbase transaction (i.e. i == 0) doesn't have undo data
                 const CTxUndo* txundo = (have_undo && i > 0) ? &blockUndo.vtxundo.at(i - 1) : nullptr;
                 UniValue objTx(UniValue::VOBJ);
@@ -925,14 +929,15 @@ static RPCHelpMan getblockheader()
 
     return blockheaderToJSON(tip, pblockindex);
 },
     };
 }
 
-static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
+static CBlock GetBlockChecked(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
+    AssertLockHeld(cs_main);
     CBlock block;
     if (IsBlockPruned(pblockindex)) {
         throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");
     }
 
     if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) {
@@ -942,14 +947,15 @@ static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
         throw JSONRPCError(RPC_MISC_ERROR, "Block not found on disk");
     }
 
     return block;
 }
 
-static CBlockUndo GetUndoChecked(const CBlockIndex* pblockindex)
+static CBlockUndo GetUndoChecked(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
+    AssertLockHeld(cs_main);
     CBlockUndo blockUndo;
     if (IsBlockPruned(pblockindex)) {
         throw JSONRPCError(RPC_MISC_ERROR, "Undo data not available (pruned data)");
     }
 
     if (!UndoReadFromDisk(blockUndo, pblockindex)) {

</details>

I think it is better to call UndoReadFromDisk() also under the same acquisition of cs_main as IsBlockPruned() (as in the patch above) because that would ensure nStatus remains unchanged until after the disk read has completed (i.e. we don't end up with a status that indicates no file on disk and still trying to read the file, if nStatus is changed after we read it and before we read the file from disk).

I had planned to add lock annotations to IsBlockPruned() in a follow-up, but I may as well do it here with the other changes. Done.

If this pull is now too large to attract re-ACKs, in that case it could be reverted to its original version with the first 3 commits only, and the nStatus preparation and change could be proposed separately. We'll see.

This access pattern requires that nFile stays in sync with nStatus - we don't want nStatus to be changed concurrently by another thread after we have read it and entered this if but before we have read nFile. That is to say - nFile, nDataPos and nUndoPos should be protected by the same mechanism as nStatus. This is easy:

<details> <summary>suggested change</summary>

diff --git i/src/chain.h w/src/chain.h
index 2a7e420c31..d8f108b466 100644
--- i/src/chain.h
+++ w/src/chain.h
@@ -161,19 +161,19 @@ public:
     CBlockIndex* pskip{nullptr};
 
     //! height of the entry in the chain. The genesis block has height 0
     int nHeight{0};
 
     //! Which # file this block is stored in (blk?????.dat)
-    int nFile{0};
+    int nFile GUARDED_BY(::cs_main){0};
 
     //! Byte offset within blk?????.dat where this block's data is stored
-    unsigned int nDataPos{0};
+    unsigned int nDataPos GUARDED_BY(::cs_main){0};
 
     //! Byte offset within rev?????.dat where this block's undo data is stored
-    unsigned int nUndoPos{0};
+    unsigned int nUndoPos GUARDED_BY(::cs_main){0};
 
     //! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block
     arith_uint256 nChainWork{};
 
     //! Number of transactions in this block.
     //! Note: in a potential headers-first mode, this number cannot be relied upon
diff --git i/src/txdb.cpp w/src/txdb.cpp
index 3fd5ca5b19..e2aed1da7c 100644
--- i/src/txdb.cpp
+++ w/src/txdb.cpp
@@ -308,23 +308,23 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
             CDiskBlockIndex diskindex;
             if (pcursor->GetValue(diskindex)) {
                 // Construct block index object
                 CBlockIndex* pindexNew = insertBlockIndex(diskindex.GetBlockHash());
                 pindexNew->pprev          = insertBlockIndex(diskindex.hashPrev);
                 pindexNew->nHeight        = diskindex.nHeight;
-                pindexNew->nFile          = diskindex.nFile;
-                pindexNew->nDataPos       = diskindex.nDataPos;
-                pindexNew->nUndoPos       = diskindex.nUndoPos;
                 pindexNew->nVersion       = diskindex.nVersion;
                 pindexNew->hashMerkleRoot = diskindex.hashMerkleRoot;
                 pindexNew->nTime          = diskindex.nTime;
                 pindexNew->nBits          = diskindex.nBits;
                 pindexNew->nNonce         = diskindex.nNonce;
                 pindexNew->nTx            = diskindex.nTx;
                 {
                     LOCK(::cs_main);
+                    pindexNew->nFile = diskindex.nFile;
+                    pindexNew->nDataPos = diskindex.nDataPos;
+                    pindexNew->nUndoPos = diskindex.nUndoPos;
                     pindexNew->nStatus = diskindex.nStatus;
                 }
 
                 if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, consensusParams)) {
                     return error("%s: CheckProofOfWork failed: %s", __func__, pindexNew->ToString());
                 }

</details>

Done in the nStatus commit (thanks!)

I am just curious - why prefix with ::? There is no conflict with a local symbol under the same name, so the :: shouldn't be necessary? Right now in the codebase there are 154 occurrences of ::cs_main and 421 occurrences of cs_main (without the prefix).

I do prefer to specify the global namespace for the sake being explicit.

Right, my understanding from review feedback is that it is preferred for new or updated code.

For all global variables or just for cs_main? Global variables are already supposed to start with g_, so prefixing them with :: is kind of redundant if the purpose is to show to the reader that this is a global variable.

cs_main doesn't start with g_

can you explain why this is needed?

CDiskBlockIndex creates a copy of the data and it isn't used in validation or blockstorage code. It is only used to serialize a struct and then discarded.

can you explain why this is needed?

CDiskBlockIndex creates a copy of the data and it isn't used in validation or blockstorage code. It is only used to serialize a struct and then discarded.

Good point. Opened #24199 as an approach to address this.

Can you explain why this is needed? The calling function should already hold cs_main?

Good catch--we can drop the lock in favor of an annotation; done in #24197.