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

2 files changed, 9 insertions, 5 deletions

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 128cae6715..5f5db967c7 100644
--- a/src/rpc/blockchain.cpp
+++ b/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)");
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 1df1492fe0..ff0d8a4e0f 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/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";