Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main

Summary: > CBlockIndex::nStatus may be racy, i.e. potentially accessed by multiple threads, see [[bitcoin/bitcoin#17161 | core#17161]]. A solution is to guard it by cs_main, along with fellow data members nFile, nDataPos and nUndoPos. Co-authored-by: Vasil Dimov <[email protected]> This concludes backport of [[bitcoin/bitcoin#22932 | core#22932]] and [[bitcoin/bitcoin#24197 | core#24197]] bitcoin/bitcoin@6ea5682 bitcoin/bitcoin@20276ca Depends on D13037 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D13038
Bitcoin-ABC · Jan 24, 2023 · dcac626 · dcac626
1 parent bd5235a
commit dcac626
Show file tree

Hide file tree

Showing 10 changed files with 18 additions and 11 deletions.
diff --git a/src/avalanche/test/processor_tests.cpp b/src/avalanche/test/processor_tests.cpp
@@ -285,6 +285,7 @@ struct BlockProvider {
     }
 
     void invalidateItem(CBlockIndex *pindex) {
+        LOCK(::cs_main);
         pindex->nStatus = pindex->nStatus.withFailed();
     }
 

diff --git a/src/blockindex.h b/src/blockindex.h
@@ -39,13 +39,13 @@ class CBlockIndex {
     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
@@ -85,7 +85,7 @@ class CBlockIndex {
 
 public:
     //! Verification status of this block. See enum BlockStatus
-    BlockStatus nStatus{};
+    BlockStatus nStatus GUARDED_BY(::cs_main){};
 
     //! block header
     int32_t nVersion{0};

diff --git a/src/chain.h b/src/chain.h
@@ -81,6 +81,7 @@ class CDiskBlockIndex : public CBlockIndex {
     }
 
     SERIALIZE_METHODS(CDiskBlockIndex, obj) {
+        LOCK(::cs_main);
         int _nVersion = s.GetVersion();
         if (!(s.GetType() & SER_GETHASH)) {
             READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));

diff --git a/src/rpc/avalanche.cpp b/src/rpc/avalanche.cpp
@@ -1184,7 +1184,8 @@ static RPCHelpMan isfinaltransaction() {
                 if (!tx) {
                     std::string errmsg;
                     if (pindex) {
-                        if (!pindex->nStatus.hasData()) {
+                        if (WITH_LOCK(::cs_main,
+                                      return !pindex->nStatus.hasData())) {
                             throw JSONRPCError(
                                 RPC_MISC_ERROR,
                                 "Block data not downloaded yet.");

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -928,7 +928,7 @@ static RPCHelpMan getblockfrompeer() {
                 throw JSONRPCError(RPC_MISC_ERROR, "Block header missing");
             }
 
-            if (index->nStatus.hasData()) {
+            if (WITH_LOCK(::cs_main, return index->nStatus.hasData())) {
                 throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded");
             }
 

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
@@ -251,7 +251,8 @@ static RPCHelpMan getrawtransaction() {
             if (!tx) {
                 std::string errmsg;
                 if (blockindex) {
-                    if (!blockindex->nStatus.hasData()) {
+                    if (WITH_LOCK(::cs_main,
+                                  return !blockindex->nStatus.hasData())) {
                         throw JSONRPCError(RPC_MISC_ERROR,
                                            "Block not available");
                     }

diff --git a/src/test/blockindex_tests.cpp b/src/test/blockindex_tests.cpp
@@ -44,6 +44,7 @@ BOOST_AUTO_TEST_CASE(get_disk_positions) {
         BlockValidity::UNKNOWN, BlockValidity::RESERVED,
         BlockValidity::TREE,    BlockValidity::TRANSACTIONS,
         BlockValidity::CHAIN,   BlockValidity::SCRIPTS};
+    LOCK(cs_main);
     for (BlockValidity validity : validityValues) {
         // Test against all combinations of data and undo flags
         for (int flags = 0; flags <= 0x03; flags++) {
@@ -68,7 +69,6 @@ BOOST_AUTO_TEST_CASE(get_disk_positions) {
             }
 
             // Data and undo positions should be unmodified
-            LOCK(::cs_main);
             FlatFilePos dataPosition = index.GetBlockPos();
             if (flags & 0x01) {
                 BOOST_CHECK(dataPosition.nFile == expectedFile);
@@ -269,6 +269,7 @@ BOOST_AUTO_TEST_CASE(index_validity_tests) {
         BlockValidity::TREE,    BlockValidity::TRANSACTIONS,
         BlockValidity::CHAIN,   BlockValidity::SCRIPTS};
     std::set<bool> boolValues = {false, true};
+    LOCK(::cs_main);
     for (BlockValidity validity : validityValues) {
         for (bool withFailed : boolValues) {
             for (bool withFailedParent : boolValues) {
@@ -278,8 +279,6 @@ BOOST_AUTO_TEST_CASE(index_validity_tests) {
                                     .withFailedParent(withFailedParent);
 
                 for (BlockValidity validUpTo : validityValues) {
-                    LOCK(::cs_main);
-
                     // Test isValidity()
                     bool isValid = index.IsValid(validUpTo);
                     if (validUpTo <= validity && !withFailed &&

diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp
@@ -150,6 +150,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/txdb.cpp b/src/txdb.cpp
@@ -300,6 +300,7 @@ bool CBlockTreeDB::ReadFlag(const std::string &name, bool &fValue) {
 bool CBlockTreeDB::LoadBlockIndexGuts(
     const Consensus::Params &params,
     std::function<CBlockIndex *(const BlockHash &)> insertBlockIndex) {
+    AssertLockHeld(::cs_main);
     std::unique_ptr<CDBIterator> pcursor(NewIterator());
 
     uint64_t version = 0;

diff --git a/src/txdb.h b/src/txdb.h
@@ -119,7 +119,9 @@ class CBlockTreeDB : public CDBWrapper {
     bool ReadFlag(const std::string &name, bool &fValue);
     bool LoadBlockIndexGuts(
         const Consensus::Params &params,
-        std::function<CBlockIndex *(const BlockHash &)> insertBlockIndex);
+        std::function<CBlockIndex *(const BlockHash &)> insertBlockIndex)
+        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    ;
 
     //! Attempt to update from an older database format.
     //! Returns whether an error occurred.
-Original file line number
+Diff line change
@@ Expand Up / @@ -285,6 +285,7 @@ struct BlockProvider { @@
         }
         void invalidateItem(CBlockIndex *pindex) {
+            LOCK(::cs_main);
             pindex->nStatus = pindex->nStatus.withFailed();
         }
@@ Expand Down @@