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

6ea56827842b9b2bd730edc38f3a7b1f46f6247b Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack)
5d59ae0ba88849b1eb0d7350871bc19fcd5ef601 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov)
eaeeb88768db529b5241ccd42f1e87579908b4df Require IsBlockPruned() to hold mutex cs_main (Jon Atack)
ca47b005770f71aa229ecc1f7b8146a96ff02151 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov)
e9f3aa5f6a7b39e8d5f2069617e5e382798d8d60 Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov)
8ef457cb83fac796f8b6a56977b1016193fc1185 Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov)
572393448b4d32f91b92edc84b4200ab52d62422 Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack)
2e557ced2830fc54476e598d52225f1679205e7d Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack)
6fd4341c10b319399c58d71c4ddeae4417e337d7 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 6ea56827842b9b2bd730edc38f3a7b1f46f6247b

Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b

4 files changed, 18 insertions, 11 deletions

diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp
index 326904a811..8c0ed32d51 100644
--- a/src/test/fuzz/chain.cpp
+++ b/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();
-    (void)disk_block_index->GetBlockTime();
-    (void)disk_block_index->GetBlockTimeMax();
-    (void)disk_block_index->GetMedianTimePast();
-    (void)disk_block_index->GetUndoPos();
-    (void)disk_block_index->HaveTxsDownloaded();
-    (void)disk_block_index->IsValid();
-    (void)disk_block_index->ToString();
+    {
+        LOCK(::cs_main);
+        (void)disk_block_index->GetBlockHash();
+        (void)disk_block_index->GetBlockPos();
+        (void)disk_block_index->GetBlockTime();
+        (void)disk_block_index->GetBlockTimeMax();
+        (void)disk_block_index->GetMedianTimePast();
+        (void)disk_block_index->GetUndoPos();
+        (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};
diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp
index f4bf6ff8c9..49b7d2003b 100644
--- a/src/test/interfaces_tests.cpp
+++ b/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();
 
diff --git a/src/test/util/blockfilter.cpp b/src/test/util/blockfilter.cpp
index 538981ce36..3ae22921b9 100644
--- a/src/test/util/blockfilter.cpp
+++ b/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;
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index f5742b65a1..26392e690d 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/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) {