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

6ea5682784 Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack)
5d59ae0ba8 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov)
eaeeb88768 Require IsBlockPruned() to hold mutex cs_main (Jon Atack)
ca47b00577 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov)
e9f3aa5f6a Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov)
8ef457cb83 Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov)
572393448b Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack)
2e557ced28 Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack)
6fd4341c10 Require CBlockIndex::GetBlockPos() to hold mutex cs_main (Jon Atack)

Pull request description:

  Issues:

  - `CBlockIndex` member functions `GetBlockPos()`, `GetUndoPos()`, `IsAssumedValid()`, `RaiseValidity()`, and `IsValid()` and block storage functions `WriteUndoDataForBlock()` and `IsBlockPruned()` are missing thread safety lock annotations to help ensure that they are called with mutex cs_main to avoid bugs like #22895. Doing this also enables the next step:

  - `CBlockIndex::nStatus` may be racy, i.e. potentially accessed by multiple threads, see #17161. A solution is to guard it by cs_main, along with fellow data members `nFile`, `nDataPos` and `nUndoPos`.

  This pull:

  - adds thread safety lock annotations for the functions listed above
  - guards `CBlockIndex::nStatus`, `nFile`, `nDataPos` and `nUndoPos` by cs_main

  How to review and test:
  - debug build with clang and verify there are no `-Wthread-safety-analysis` warnings
  - review the code to verify each annotation or lock is necessary and sensible, or if any are missing
  - look for whether taking a lock can be replaced by a lock annotation instead
  - for more information about Clang thread safety analysis, see
      - https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
      - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes
      - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

  Mitigates/potentially closes #17161.

ACKs for top commit:
  laanwj:
    Code review ACK 6ea5682784

Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b

src/chain.h

@@ -10,6 +10,7 @@
 #include <consensus/params.h>
 #include <flatfile.h>
 #include <primitives/block.h>
+#include <sync.h>
 #include <tinyformat.h>
 #include <uint256.h>
 
@@ -37,6 +38,8 @@ static constexpr int64_t TIMESTAMP_WINDOW = MAX_FUTURE_BLOCK_TIME;
  */
 static constexpr int64_t MAX_BLOCK_TIME_GAP = 90 * 60;
 
+extern RecursiveMutex cs_main;
+
 class CBlockFileInfo
 {
 public:
@@ -161,13 +164,13 @@ public:
     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{};
@@ -195,7 +198,7 @@ public:
     //! load to avoid the block index being spuriously rewound.
     //! @sa NeedsRedownload
     //! @sa ActivateSnapshot
-    uint32_t nStatus{0};
+    uint32_t nStatus GUARDED_BY(::cs_main){0};
 
     //! block header
     int32_t nVersion{0};
@@ -223,8 +226,9 @@ public:
     {
     }
 
-    FlatFilePos GetBlockPos() const
+    FlatFilePos GetBlockPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
+        AssertLockHeld(::cs_main);
         FlatFilePos ret;
         if (nStatus & BLOCK_HAVE_DATA) {
             ret.nFile = nFile;
@@ -233,8 +237,9 @@ public:
         return ret;
     }
 
-    FlatFilePos GetUndoPos() const
+    FlatFilePos GetUndoPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
+        AssertLockHeld(::cs_main);
         FlatFilePos ret;
         if (nStatus & BLOCK_HAVE_UNDO) {
             ret.nFile = nFile;
@@ -306,7 +311,9 @@ public:
 
     //! 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.
         if (nStatus & BLOCK_FAILED_MASK)
             return false;
@@ -315,12 +322,17 @@ public:
 
     //! @returns true if the block is assumed-valid; this means it is queued to be
     //!   validated by a background chainstate.
-    bool IsAssumedValid() const { return nStatus & BLOCK_ASSUMED_VALID; }
+    bool IsAssumedValid() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+    {
+        AssertLockHeld(::cs_main);
+        return nStatus & BLOCK_ASSUMED_VALID;
+    }
 
     //! 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.
         if (nStatus & BLOCK_FAILED_MASK) return false;
 
@@ -370,6 +382,7 @@ public:
 
     SERIALIZE_METHODS(CDiskBlockIndex, obj)
     {
+        LOCK(::cs_main);
         int _nVersion = s.GetVersion();
         if (!(s.GetType() & SER_GETHASH)) READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
 

src/index/txindex.cpp

@@ -59,7 +59,9 @@ bool TxIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
     // Exclude genesis block transaction because outputs are not spendable.
     if (pindex->nHeight == 0) return true;
 
-    CDiskTxPos pos(pindex->GetBlockPos(), GetSizeOfCompactSize(block.vtx.size()));
+    CDiskTxPos pos{
+        WITH_LOCK(::cs_main, return pindex->GetBlockPos()),
+        GetSizeOfCompactSize(block.vtx.size())};
     std::vector<std::pair<uint256, CDiskTxPos>> vPos;
     vPos.reserve(block.vtx.size());
     for (const auto& tx : block.vtx) {

src/net_processing.cpp

@@ -1880,7 +1880,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}));

src/node/blockstorage.cpp

@@ -429,6 +429,7 @@ CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
 
 bool IsBlockPruned(const CBlockIndex* pblockindex)
 {
+    AssertLockHeld(::cs_main);
     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
 }
 
@@ -513,7 +514,8 @@ static bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const
 
 bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex)
 {
-    FlatFilePos pos = pindex->GetUndoPos();
+    const FlatFilePos pos{WITH_LOCK(::cs_main, return pindex->GetUndoPos())};
+
     if (pos.IsNull()) {
         return error("%s: no undo data available", __func__);
     }
@@ -712,6 +714,7 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa
 
 bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)
 {
+    AssertLockHeld(::cs_main);
     // Write undo information to disk
     if (pindex->GetUndoPos().IsNull()) {
         FlatFilePos _pos;
@@ -818,17 +821,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)
-{
-    FlatFilePos block_pos;
-    {
-        LOCK(cs_main);
-        block_pos = 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 BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp)
 {

src/node/blockstorage.h

@@ -7,12 +7,15 @@
 
 #include <fs.h>
 #include <protocol.h> // For CMessageHeader::MessageStartChars
+#include <sync.h>
 #include <txdb.h>
 
 #include <atomic>
 #include <cstdint>
 #include <vector>
 
+extern RecursiveMutex cs_main;
+
 class ArgsManager;
 class BlockValidationState;
 class CBlock;
@@ -146,7 +149,8 @@ public:
     /** Get block file info entry for one block file */
     CBlockFileInfo* GetBlockFileInfo(size_t n);
 
-    bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams);
+    bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)
+        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp);
 
@@ -163,7 +167,7 @@ public:
 };
 
 //! 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();
 
@@ -181,7 +185,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);
 

src/rpc/blockchain.cpp

@@ -185,7 +185,7 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIn
         case TxVerbosity::SHOW_DETAILS:
         case TxVerbosity::SHOW_DETAILS_AND_PREVOUT:
             CBlockUndo blockUndo;
-            const bool have_undo = !IsBlockPruned(blockindex) && UndoReadFromDisk(blockUndo, blockindex);
+            const bool have_undo{WITH_LOCK(::cs_main, return !IsBlockPruned(blockindex) && UndoReadFromDisk(blockUndo, blockindex))};
 
             for (size_t i = 0; i < block.vtx.size(); ++i) {
                 const CTransactionRef& tx = block.vtx.at(i);
@@ -818,7 +818,8 @@ static RPCHelpMan getblockfrompeer()
         throw JSONRPCError(RPC_MISC_ERROR, "Block header missing");
     }
 
-    if (index->nStatus & BLOCK_HAVE_DATA) {
+    const bool block_has_data = WITH_LOCK(::cs_main, return index->nStatus & BLOCK_HAVE_DATA);
+    if (block_has_data) {
         throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded");
     }
 
@@ -929,8 +930,9 @@ static RPCHelpMan getblockheader()
     };
 }
 
-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)");
@@ -946,8 +948,9 @@ static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
     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)");

src/rpc/rawtransaction.cpp

@@ -241,7 +241,8 @@ static RPCHelpMan getrawtransaction()
     if (!tx) {
         std::string errmsg;
         if (blockindex) {
-            if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {
+            const bool block_has_data = WITH_LOCK(::cs_main, return blockindex->nStatus & BLOCK_HAVE_DATA);
+            if (!block_has_data) {
                 throw JSONRPCError(RPC_MISC_ERROR, "Block not available");
             }
             errmsg = "No such transaction found in the provided block";

src/test/fuzz/chain.cpp

@@ -21,15 +21,18 @@ FUZZ_TARGET(chain)
 
     const uint256 zero{};
     disk_block_index->phashBlock = &zero;
-    (void)disk_block_index->GetBlockHash();
+    {
-    (void)disk_block_index->GetBlockPos();
+        LOCK(::cs_main);
-    (void)disk_block_index->GetBlockTime();
+        (void)disk_block_index->GetBlockHash();
-    (void)disk_block_index->GetBlockTimeMax();
+        (void)disk_block_index->GetBlockPos();
-    (void)disk_block_index->GetMedianTimePast();
+        (void)disk_block_index->GetBlockTime();
-    (void)disk_block_index->GetUndoPos();
+        (void)disk_block_index->GetBlockTimeMax();
-    (void)disk_block_index->HaveTxsDownloaded();
+        (void)disk_block_index->GetMedianTimePast();
-    (void)disk_block_index->IsValid();
+        (void)disk_block_index->GetUndoPos();
-    (void)disk_block_index->ToString();
+        (void)disk_block_index->HaveTxsDownloaded();
+        (void)disk_block_index->IsValid();
+        (void)disk_block_index->ToString();
+    }
 
     const CBlockHeader block_header = disk_block_index->GetBlockHeader();
     (void)CDiskBlockIndex{*disk_block_index};
@@ -55,7 +58,7 @@ FUZZ_TARGET(chain)
         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};

src/test/interfaces_tests.cpp

@@ -123,6 +123,7 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
 
 BOOST_AUTO_TEST_CASE(hasBlocks)
 {
+    LOCK(::cs_main);
     auto& chain = m_node.chain;
     const CChain& active = Assert(m_node.chainman)->ActiveChain();
 

src/test/util/blockfilter.cpp

@@ -13,6 +13,8 @@ using node::UndoReadFromDisk;
 
 bool ComputeFilter(BlockFilterType filter_type, const CBlockIndex* block_index, BlockFilter& filter)
 {
+    LOCK(::cs_main);
+
     CBlock block;
     if (!ReadBlockFromDisk(block, block_index->GetBlockPos(), Params().GetConsensus())) {
         return false;

src/test/validation_chainstatemanager_tests.cpp

@@ -235,7 +235,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
         *chainman.SnapshotBlockhash());
 
     // Ensure that the genesis block was not marked assumed-valid.
-    BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid());
+    BOOST_CHECK(WITH_LOCK(::cs_main, return !chainman.ActiveChain().Genesis()->IsAssumedValid()));
 
     const AssumeutxoData& au_data = *ExpectedAssumeutxo(snapshot_height, ::Params());
     const CBlockIndex* tip = chainman.ActiveTip();
@@ -356,6 +356,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
 
     // 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) {

src/txdb.cpp

@@ -311,19 +311,23 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
                 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->nStatus        = diskindex.nStatus;
                 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))
+                if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, consensusParams)) {
                     return error("%s: CheckProofOfWork failed: %s", __func__, pindexNew->ToString());
+                }
 
                 pcursor->Next();
             } else {

src/wallet/test/wallet_tests.cpp

@@ -140,11 +140,13 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
     }
 
     // Prune the older block file.
+    int file_number;
     {
         LOCK(cs_main);
-        Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(oldTip->GetBlockPos().nFile);
+        file_number = oldTip->GetBlockPos().nFile;
+        Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(file_number);
     }
-    UnlinkPrunedFiles({oldTip->GetBlockPos().nFile});
+    UnlinkPrunedFiles({file_number});
 
     // Verify ScanForWalletTransactions only picks transactions in the new block
     // file.
@@ -169,9 +171,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
     // Prune the remaining block file.
     {
         LOCK(cs_main);
-        Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(newTip->GetBlockPos().nFile);
+        file_number = newTip->GetBlockPos().nFile;
+        Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(file_number);
     }
-    UnlinkPrunedFiles({newTip->GetBlockPos().nFile});
+    UnlinkPrunedFiles({file_number});
 
     // Verify ScanForWalletTransactions scans no blocks.
     {
@@ -202,11 +205,13 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
     CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
 
     // Prune the older block file.
+    int file_number;
     {
         LOCK(cs_main);
-        Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(oldTip->GetBlockPos().nFile);
+        file_number = oldTip->GetBlockPos().nFile;
+        Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(file_number);
     }
-    UnlinkPrunedFiles({oldTip->GetBlockPos().nFile});
+    UnlinkPrunedFiles({file_number});
 
     // Verify importmulti RPC returns failure for a key whose creation time is
     // before the missing block, and success for a key whose creation time is