reduce cs_main scope, guard block index 'nFile' under a read/write mutex

By not having to lock cs_main every time we access block data on disk,
we can avoid slowing down concurrent tasks due to threads waiting
for the lock to be released.

src/bench/block_assemble.cpp

@@ -55,6 +55,9 @@ static void BlockAssemblerAddPackageTxns(benchmark::Bench& bench)
     node::BlockAssembler::Options assembler_options;
     assembler_options.test_block_validity = false;
 
+    // Connect genesis block
+    assert(testing_setup->m_node.chainman->ActiveChainstate().LoadGenesisBlock());
+
     bench.run([&] {
         PrepareBlock(testing_setup->m_node, P2WSH_OP_TRUE, assembler_options);
     });

src/chain.cpp

@@ -7,6 +7,8 @@
 #include <tinyformat.h>
 #include <util/time.h>
 
+SharedMutex g_cs_blockindex_data;
+
 std::string CBlockFileInfo::ToString() const
 {
     return strprintf("CBlockFileInfo(blocks=%u, size=%u, heights=%u...%u, time=%s...%s)", nBlocks, nSize, nHeightFirst, nHeightLast, FormatISO8601Date(nTimeFirst), FormatISO8601Date(nTimeLast));
@@ -18,6 +20,14 @@ std::string CBlockIndex::ToString() const
                      pprev, nHeight, hashMerkleRoot.ToString(), GetBlockHash().ToString());
 }
 
+void CBlockIndex::SetFileData(int file_num, int data_pos, int undo_pos)
+{
+    LOCK(g_cs_blockindex_data);
+    nFile = file_num;
+    nDataPos = data_pos;
+    nUndoPos = undo_pos;
+}
+
 void CChain::SetTip(CBlockIndex& block)
 {
     CBlockIndex* pindex = &block;

src/chain.h

@@ -19,6 +19,7 @@
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
+#include <shared_mutex>
 #include <string>
 #include <vector>
 
@@ -132,6 +133,8 @@ enum BlockStatus : uint32_t {
                                       //!< ancestors before they were validated, and unset when they were validated.
 };
 
+extern SharedMutex g_cs_blockindex_data;
+
 /** 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 next block. A blockindex may have multiple pprev pointing
@@ -153,13 +156,13 @@ public:
     int nHeight{0};
 
     //! Which # file this block is stored in (blk?????.dat)
-    int nFile GUARDED_BY(::cs_main){-1};
+    int nFile GUARDED_BY(g_cs_blockindex_data){-1};
 
     //! Byte offset within blk?????.dat where this block's data is stored
-    unsigned int nDataPos GUARDED_BY(::cs_main){0};
+    unsigned int nDataPos GUARDED_BY(g_cs_blockindex_data){0};
 
     //! Byte offset within rev?????.dat where this block's undo data is stored
-    unsigned int nUndoPos GUARDED_BY(::cs_main){0};
+    unsigned int nUndoPos GUARDED_BY(g_cs_blockindex_data){0};
 
     //! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block
     arith_uint256 nChainWork{};
@@ -206,26 +209,42 @@ public:
     {
     }
 
+    void SetFileData(int file_num, int data_pos, int undo_pos);
+    void SetUndoPos(int undo_pos) {
+        LOCK(g_cs_blockindex_data);
+        nUndoPos = undo_pos;
+    }
+
+    int GetFileNum() const {
+        LOCK_SHARED(g_cs_blockindex_data);
+        return nFile;
+    }
+    int GetDataPos() const {
+        LOCK_SHARED(g_cs_blockindex_data);
+        return nDataPos;
+    }
+
+    FlatFilePos GetFilePos(bool is_undo) const
+    {
+        LOCK_SHARED(g_cs_blockindex_data);
+        FlatFilePos ret;
+        if (nFile >= 0) {
+            ret.nFile = nFile;
+            ret.nPos = is_undo ? nUndoPos : nDataPos;
+        }
+        return ret;
+    }
+
     FlatFilePos GetBlockPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
         AssertLockHeld(::cs_main);
-        FlatFilePos ret;
-        if (nStatus & BLOCK_HAVE_DATA) {
-            ret.nFile = nFile;
-            ret.nPos = nDataPos;
-        }
-        return ret;
+        return nStatus & BLOCK_HAVE_DATA ? GetFilePos(/*is_undo=*/false) : FlatFilePos{};
     }
 
     FlatFilePos GetUndoPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
         AssertLockHeld(::cs_main);
-        FlatFilePos ret;
-        if (nStatus & BLOCK_HAVE_UNDO) {
-            ret.nFile = nFile;
-            ret.nPos = nUndoPos;
-        }
-        return ret;
+        return nStatus & BLOCK_HAVE_UNDO ? GetFilePos(/*is_undo=*/true) : FlatFilePos{};
     }
 
     CBlockHeader GetBlockHeader() const
@@ -384,6 +403,7 @@ public:
         READWRITE(VARINT_MODE(obj.nHeight, VarIntMode::NONNEGATIVE_SIGNED));
         READWRITE(VARINT(obj.nStatus));
         READWRITE(VARINT(obj.nTx));
+        LOCK_SHARED(g_cs_blockindex_data);
         if (obj.nStatus & (BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO)) READWRITE(VARINT_MODE(obj.nFile, VarIntMode::NONNEGATIVE_SIGNED));
         if (obj.nStatus & BLOCK_HAVE_DATA) READWRITE(VARINT(obj.nDataPos));
         if (obj.nStatus & BLOCK_HAVE_UNDO) READWRITE(VARINT(obj.nUndoPos));

src/kernel/chain.cpp

@@ -18,9 +18,8 @@ interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data
         info.prev_hash = index->pprev ? index->pprev->phashBlock : nullptr;
         info.height = index->nHeight;
         info.chain_time_max = index->GetBlockTimeMax();
-        LOCK(::cs_main);
-        info.file_number = index->nFile;
-        info.data_pos = index->nDataPos;
+        info.file_number = index->GetFileNum();
+        info.data_pos = index->GetDataPos();
     }
     info.data = data;
     return info;

src/node/blockstorage.cpp

@@ -117,6 +117,7 @@ bool BlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, s
             if (pcursor->GetValue(diskindex)) {
                 // Construct block index object
                 CBlockIndex* pindexNew = insertBlockIndex(diskindex.ConstructBlockHash());
+                LOCK(g_cs_blockindex_data);
                 pindexNew->pprev          = insertBlockIndex(diskindex.hashPrev);
                 pindexNew->nHeight        = diskindex.nHeight;
                 pindexNew->nFile          = diskindex.nFile;
@@ -242,12 +243,10 @@ void BlockManager::PruneOneBlockFile(const int fileNumber)
 
     for (auto& entry : m_block_index) {
         CBlockIndex* pindex = &entry.second;
-        if (pindex->nFile == fileNumber) {
+        if (pindex->GetFileNum() == fileNumber) {
             pindex->nStatus &= ~BLOCK_HAVE_DATA;
             pindex->nStatus &= ~BLOCK_HAVE_UNDO;
-            pindex->nFile = -1;
-            pindex->nDataPos = 0;
-            pindex->nUndoPos = 0;
+            pindex->SetFileData(/*file_num=*/-1, /*data_pos=*/0, /*undo_pos=*/0);
             m_dirty_blockindex.insert(pindex);
 
             // Prune from m_blocks_unlinked -- any block we prune would have
@@ -524,12 +523,12 @@ bool BlockManager::LoadBlockIndexDB(const std::optional<uint256>& snapshot_block
     std::set<int> setBlkDataFiles;
     for (auto& [_, block_index] : m_block_index) {
         if (block_index.nStatus & BLOCK_HAVE_DATA) {
-            setBlkDataFiles.insert(block_index.nFile);
+            setBlkDataFiles.insert(block_index.GetFileNum());
         } else {
             // In case we don't have the block, the position must be -1.
             // (applies to older clients that set 'nFile=0' during pruning)
-            if (block_index.nFile == 0) {
-                block_index.nFile = -1;
+            if (block_index.GetFileNum() == 0) {
+                block_index.SetFileData(/*file_num=*/-1, /*data_pos=*/0, /*undo_pos=*/0);
             }
         }
     }
@@ -708,8 +707,10 @@ bool BlockManager::UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos
 
 bool BlockManager::UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& index) const
 {
-    const FlatFilePos pos{WITH_LOCK(::cs_main, return index.GetUndoPos())};
+    if (index.nHeight == 0) return false; // nothing to do
 
+    LOCK_SHARED(g_cs_blockindex_data); // keep lock until we finish reading data from disk
+    const FlatFilePos pos = index.GetFilePos(/*is_undo=*/true);
     if (pos.IsNull()) {
         LogError("%s: no undo data available\n", __func__);
         return false;
@@ -1009,7 +1010,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid
     // Write undo information to disk
     if (block.GetUndoPos().IsNull()) {
         FlatFilePos _pos;
-        if (!FindUndoPos(state, block.nFile, _pos, ::GetSerializeSize(blockundo) + 40)) {
+        if (!FindUndoPos(state, block.GetFileNum(), _pos, ::GetSerializeSize(blockundo) + 40)) {
             LogError("ConnectBlock(): FindUndoPos failed\n");
             return false;
         }
@@ -1034,7 +1035,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid
             cursor.undo_height = block.nHeight;
         }
         // update nUndoPos in block index
-        block.nUndoPos = _pos.nPos;
+        block.SetUndoPos(_pos.nPos);
         block.nStatus |= BLOCK_HAVE_UNDO;
         m_dirty_blockindex.insert(&block);
     }
@@ -1078,11 +1079,16 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos) cons
 
 bool BlockManager::ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) const
 {
-    const FlatFilePos block_pos{WITH_LOCK(cs_main, return index.GetBlockPos())};
+    FlatFilePos block_pos;
 
-    if (!ReadBlockFromDisk(block, block_pos)) {
-        return false;
+    {
+        LOCK_SHARED(g_cs_blockindex_data); // keep lock until we finish reading the block from disk
+        block_pos = index.GetFilePos(/*is_undo=*/false);
+        if (!ReadBlockFromDisk(block, block_pos)) {
+            return false;
+        }
     }
+
     if (block.GetHash() != index.GetBlockHash()) {
         LogError("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s\n",
                      index.ToString(), block_pos.ToString());

src/rpc/blockchain.cpp

@@ -586,6 +586,11 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockin
         }
     }
 
+    // No need to go to disk to get the genesis block
+    if (blockindex.nHeight == 0) {
+        return Params().GenesisBlock();
+    }
+
     if (!blockman.ReadBlockFromDisk(block, blockindex)) {
         // Block not found on disk. This could be because we have the block
         // header in our index but not yet have the block or did not accept the

src/test/util/blockfilter.cpp

@@ -8,17 +8,18 @@
 #include <node/blockstorage.h>
 #include <primitives/block.h>
 #include <undo.h>
-#include <validation.h>
 
 using node::BlockManager;
 
 bool ComputeFilter(BlockFilterType filter_type, const CBlockIndex& block_index, BlockFilter& filter, const BlockManager& blockman)
 {
-    LOCK(::cs_main);
-
     CBlock block;
-    if (!blockman.ReadBlockFromDisk(block, block_index.GetBlockPos())) {
-        return false;
+
+    {
+        LOCK_SHARED(g_cs_blockindex_data); // keep lock until we finish reading the block from disk
+        if (!blockman.ReadBlockFromDisk(block, block_index.GetFilePos(/*is_undo=*/false))) {
+            return false;
+        }
     }
 
     CBlockUndo block_undo;

src/validation.cpp

@@ -3699,9 +3699,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
             pindexNew->nHeight, pindexNew->nChainTx, prev_tx_sum(*pindexNew), PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT);
         pindexNew->nChainTx = 0;
     }
-    pindexNew->nFile = pos.nFile;
-    pindexNew->nDataPos = pos.nPos;
-    pindexNew->nUndoPos = 0;
+    pindexNew->SetFileData(/*file_num=*/pos.nFile, /*data_pos=*/pos.nPos, /*undo_pos=*/0);
     pindexNew->nStatus |= BLOCK_HAVE_DATA;
     if (DeploymentActiveAt(*pindexNew, *this, Consensus::DEPLOYMENT_SEGWIT)) {
         pindexNew->nStatus |= BLOCK_OPT_WITNESS;