Require CBlockIndex::GetBlockPos() to hold mutex cs_main

Summary: > consensus: don't call GetBlockPos in ReadBlockFromDisk without lock The `cs_main` lock only covered the first invocation of `CBlockIndex::GetBlockPos()`. Use the `blockPos` local variable instead of calling the function twice, rename it to `block_pos`, and make it const This is a backport of [[bitcoin/bitcoin#22895 | core#22895]] ---- > Require CBlockIndex::GetBlockPos() to hold mutex cs_main This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@6fd4341 Test Plan: With clang and DEBUG: `ninja all check-all bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13031
Bitcoin-ABC · Jan 24, 2023 · 1e299c9 · 1e299c9
1 parent 6cb6383
commit 1e299c9
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 32 deletions.
diff --git a/src/blockindex.h b/src/blockindex.h
@@ -9,11 +9,14 @@
 #include <blockstatus.h>
 #include <flatfile.h>
 #include <primitives/block.h>
+#include <sync.h>
 #include <tinyformat.h>
 #include <uint256.h>
 
 struct BlockHash;
 
+extern RecursiveMutex cs_main;
+
 /**
  * The block chain is a tree shaped structure starting with the genesis block at
  * the root, with each block potentially having multiple candidates to be the
@@ -108,7 +111,8 @@ class CBlockIndex {
           nTime{block.nTime}, nBits{block.nBits}, nNonce{block.nNonce},
           nTimeReceived{0} {}
 
-    FlatFilePos GetBlockPos() const {
+    FlatFilePos GetBlockPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+        AssertLockHeld(::cs_main);
         FlatFilePos ret;
         if (nStatus.hasData()) {
             ret.nFile = nFile;

diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
@@ -58,7 +58,7 @@ bool TxIndex::WriteBlock(const CBlock &block, const CBlockIndex *pindex) {
         return true;
     }
 
-    CDiskTxPos pos(pindex->GetBlockPos(),
+    CDiskTxPos pos(WITH_LOCK(::cs_main, return pindex->GetBlockPos()),
                    GetSizeOfCompactSize(block.vtx.size()));
     std::vector<std::pair<TxId, CDiskTxPos>> vPos;
     vPos.reserve(block.vtx.size());

diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
@@ -761,20 +761,17 @@ bool ReadBlockFromDisk(CBlock &block, const FlatFilePos &pos,
 
 bool ReadBlockFromDisk(CBlock &block, const CBlockIndex *pindex,
                        const Consensus::Params &params) {
-    FlatFilePos blockPos;
-    {
-        LOCK(cs_main);
-        blockPos = pindex->GetBlockPos();
-    }
+    const FlatFilePos block_pos{
+        WITH_LOCK(cs_main, return pindex->GetBlockPos())};
 
-    if (!ReadBlockFromDisk(block, blockPos, params)) {
+    if (!ReadBlockFromDisk(block, block_pos, params)) {
         return false;
     }
 
     if (block.GetHash() != pindex->GetBlockHash()) {
         return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() "
                      "doesn't match index for %s at %s",
-                     pindex->ToString(), pindex->GetBlockPos().ToString());
+                     pindex->ToString(), block_pos.ToString());
     }
 
     return true;

diff --git a/src/test/blockindex_tests.cpp b/src/test/blockindex_tests.cpp
@@ -68,6 +68,7 @@ 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);

diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp
@@ -23,20 +23,23 @@ void test_one_input(const std::vector<uint8_t> &buffer) {
 
     const BlockHash 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->GetChainSize();
-    (void)disk_block_index->GetChainTxCount();
-    (void)disk_block_index->GetHeaderReceivedTime();
-    (void)disk_block_index->GetMedianTimePast();
-    (void)disk_block_index->GetReceivedTimeDiff();
-    (void)disk_block_index->GetUndoPos();
-    (void)disk_block_index->HaveTxsDownloaded();
-    (void)disk_block_index->IsValid();
-    (void)disk_block_index->ToString();
-    (void)disk_block_index->UpdateChainStats();
+    {
+        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->GetChainSize();
+        (void)disk_block_index->GetChainTxCount();
+        (void)disk_block_index->GetHeaderReceivedTime();
+        (void)disk_block_index->GetMedianTimePast();
+        (void)disk_block_index->GetReceivedTimeDiff();
+        (void)disk_block_index->GetUndoPos();
+        (void)disk_block_index->HaveTxsDownloaded();
+        (void)disk_block_index->IsValid();
+        (void)disk_block_index->ToString();
+        (void)disk_block_index->UpdateChainStats();
+    }
 
     const CBlockHeader block_header = disk_block_index->GetBlockHeader();
     (void)CDiskBlockIndex{*disk_block_index};

diff --git 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())) {

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -135,12 +135,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.
@@ -168,10 +169,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.
     {
@@ -206,12 +207,13 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) {
     CBlockIndex *newTip = m_node.chainman->ActiveTip();
 
     // 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