Give mapBlockIndex its own lock.

Declare mapBlockIndex to be protected by cs_mapblockindex.

Move a lot of generic lookups to LookupBlockIndex, while
other more complex uses (e.g. atomic lookup+insert) are left alone.

Make sure the lock is taken everywhere necessary (places found with clang
thread-safety warnings) and only as tightly as possible to avoid
potential lock cycles (detectable by tsan).

Many uses of cs_main are replaced by cs_mapblockindex if they were only
protecting mapBlockIndex, or removed (LookupBlockIndex taking the lock
itself), or moved to after LookupBlockIndex calls.

Author: Zannick

src/index/base.cpp

@@ -194,11 +194,7 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator)
     }
 
     const uint256& locator_tip_hash = locator.vHave.front();
-    const CBlockIndex* locator_tip_index;
-    {
-        LOCK(cs_main);
-        locator_tip_index = LookupBlockIndex(locator_tip_hash);
-    }
+    const CBlockIndex* locator_tip_index = LookupBlockIndex(locator_tip_hash);
 
     if (!locator_tip_index) {
         FatalError("%s: First block (hash=%s) in locator was not found",

src/init.cpp

@@ -1602,10 +1602,13 @@ bool AppInitMain()
                     break;
                 }
 
-                // If the loaded chain has a wrong genesis, bail out immediately
-                // (we're likely using a testnet datadir, or the other way around).
-                if (!mapBlockIndex.empty() && !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
-                    return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?"));
+                {
+                    LOCK(cs_mapblockindex);
+                    // If the loaded chain has a wrong genesis, bail out immediately
+                    // (we're likely using a testnet datadir, or the other way around).
+                    if (!mapBlockIndex.empty() && !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
+                        return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?"));
+                    }
                 }
 
                 // Check for changed -prune state.  What we are concerned about is a user who has pruned blocks
@@ -1835,14 +1838,12 @@ bool AppInitMain()
 
     // ********************************************************* Step 12: start node
 
-    int chain_active_height;
-
     //// debug print
     {
-        LOCK(cs_main);
+        LOCK(cs_mapblockindex);
         LogPrintf("mapBlockIndex.size() = %u\n", mapBlockIndex.size());
-        chain_active_height = chainActive.Height();
     }
+    int chain_active_height = chainActive.Height();
     LogPrintf("nBestHeight = %d\n", chain_active_height);
 
     if (gArgs.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION))

src/interfaces/wallet.cpp

@@ -237,8 +237,7 @@ WalletTxStatus MakeWalletTxStatus(const CWalletTx& wtx)
 {
     LOCK(cs_main);
     WalletTxStatus result;
-    auto mi = ::mapBlockIndex.find(wtx.hashBlock);
-    CBlockIndex* block = mi != ::mapBlockIndex.end() ? mi->second : nullptr;
+    CBlockIndex* block = LookupBlockIndex(wtx.hashBlock);
     result.block_height = (block ? block->nHeight : std::numeric_limits<int>::max());
     result.blocks_to_maturity = wtx.GetBlocksToMaturity();
     result.depth_in_main_chain = wtx.GetDepthInMainChain();
@@ -256,9 +255,8 @@ WalletTxStatus MakeWalletTxStatus(const CWalletTx& wtx)
 WalletTxStatus MakeWalletTxStatus(AnonWallet* pAnonWallet, const uint256 &hash, const CTransactionRecord &rtx)
 {
     WalletTxStatus result;
-    auto mi = ::mapBlockIndex.find(rtx.blockHash);
 
-    CBlockIndex* block = mi != ::mapBlockIndex.end() ? mi->second : nullptr;
+    CBlockIndex* block = LookupBlockIndex(rtx.blockHash);
     result.block_height = (block ? block->nHeight : std::numeric_limits<int>::max()),
 
             result.blocks_to_maturity = 0;

src/miner.cpp

@@ -168,13 +168,13 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
 
     CMutableTransaction txCoinStake;
     CBlockIndex* pindexPrev;
+    //Do not pass in the chain tip, because it can change. Instead pass the blockindex directly from mapblockindex, which is const.
+    auto pindexTip = chainActive.Tip();
+    if (!pindexTip)
+        return nullptr;
+    auto hashBest = pindexTip->GetBlockHash();
     {
-        LOCK(cs_main);
-        //Do not pass in the chain tip, because it can change. Instead pass the blockindex directly from mapblockindex, which is const.
-        auto pindexTip = chainActive.Tip();
-        if (!pindexTip)
-            return nullptr;
-        auto hashBest = pindexTip->GetBlockHash();
+        LOCK(cs_mapblockindex);
         pindexPrev = mapBlockIndex.at(hashBest);
     }
     if (fProofOfStake && pindexPrev->nHeight + 1 >= Params().HeightPoSStart()) {

src/net_processing.cpp

@@ -1156,30 +1156,29 @@ void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, c
     }
 
     bool need_activate_chain = false;
-    {
-        LOCK(cs_main);
-        const CBlockIndex* pindex = LookupBlockIndex(inv.hash);
-        if (pindex) {
-            if (pindex->nChainTx && !pindex->IsValid(BLOCK_VALID_SCRIPTS) &&
-                    pindex->IsValid(BLOCK_VALID_TREE)) {
-                // If we have the block and all of its parents, but have not yet validated it,
-                // we might be in the middle of connecting it (ie in the unlock of cs_main
-                // before ActivateBestChain but after AcceptBlock).
-                // In this case, we need to run ActivateBestChain prior to checking the relay
-                // conditions below.
-                need_activate_chain = true;
-            }
+
+    const CBlockIndex* pindex = LookupBlockIndex(inv.hash);
+    if (pindex) {
+        if (pindex->nChainTx && !pindex->IsValid(BLOCK_VALID_SCRIPTS) &&
+                pindex->IsValid(BLOCK_VALID_TREE)) {
+            // If we have the block and all of its parents, but have not yet validated it,
+            // we might be in the middle of connecting it
+            // (ie before ActivateBestChain but after AcceptBlock).
+            // In this case, we need to run ActivateBestChain prior to checking the relay
+            // conditions below.
+            need_activate_chain = true;
         }
-    } // release cs_main before calling ActivateBestChain
+    }
     if (need_activate_chain) {
         CValidationState state;
         if (!ActivateBestChain(state, Params(), a_recent_block)) {
             LogPrint(BCLog::NET, "failed to activate chain (%s)\n", FormatStateMessage(state));
         }
     }
 
+    pindex = LookupBlockIndex(inv.hash);
+
     LOCK(cs_main);
-    const CBlockIndex* pindex = LookupBlockIndex(inv.hash);
     if (pindex) {
         send = BlockRequestAllowed(pindex, consensusParams);
         if (!send) {
@@ -1577,7 +1576,10 @@ void ProcessStaging()
             continue;
         }
 
-        fProcessNext = mapBlockIndex.at(pblockStaged->hashPrevBlock)->nChainTx > 0;
+        {
+            LOCK(cs_mapblockindex);
+            fProcessNext = mapBlockIndex.at(pblockStaged->hashPrevBlock)->nChainTx > 0;
+        }
 
         if (!fProcessNext) {
             MilliSleep(100);
@@ -2447,14 +2449,14 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             return true;
         }
 
-        LOCK(cs_main);
-
         const CBlockIndex* pindex = LookupBlockIndex(req.blockhash);
         if (!pindex || !(pindex->nStatus & BLOCK_HAVE_DATA)) {
             LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block we don't have\n", pfrom->GetId());
             return true;
         }
 
+        LOCK(cs_main);
+
         if (pindex->nHeight < chainActive.Height() - MAX_BLOCKTXN_DEPTH) {
             // If an older block is requested (should never happen in practice,
             // but can happen in tests) send a block response instead of a
@@ -2776,10 +2778,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
 
         bool received_new_header = false;
 
-        {
-        LOCK(cs_main);
-
         if (!LookupBlockIndex(cmpctblock.header.hashPrevBlock)) {
+            LOCK(cs_main);
             // Doesn't connect (or is genesis), instead of DoSing in AcceptBlockHeader, request deeper headers
             if (!IsInitialBlockDownload())
                 connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), uint256()));
@@ -2789,7 +2789,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         if (!LookupBlockIndex(cmpctblock.header.GetHash())) {
             received_new_header = true;
         }
-        }
 
         const CBlockIndex *pindex = nullptr;
         CValidationState state;
@@ -3105,7 +3104,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         int nHeightNext = 0;
         const uint256 hash(pblock->GetHash());
         {
-            LOCK(cs_main);
+            LOCK2(cs_main, cs_mapblockindex);
             // Also always process if we requested the block explicitly, as we may
             // need it even though it is not a candidate for a new best tip.
             forceProcessing |= MarkBlockAsReceived(hash);

src/qt/coincontroldialog.cpp

@@ -565,14 +565,15 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
             uint256 hashBlock;
             if (!GetTransaction(txid, txRef, Params().GetConsensus(), hashBlock, true))
                 continue;
-            auto it = mapBlockIndex.find(hashBlock);
-            if (it == mapBlockIndex.end())
+
+            CBlockIndex* block = LookupBlockHash(hashBlock);
+            if (block == nullptr)
                 continue;
 
-            if (!chainActive.Contains(it->second))
+            if (!chainActive.Contains(block))
                 continue;
 
-            nDepth = chainActive.Height() - it->second->nHeight + 1;
+            nDepth = chainActive.Height() - block->nHeight + 1;
         }
 
         if (nDepth < 1)
@@ -888,15 +889,15 @@ void CoinControlDialog::updateView(int nCoinType)
                 if (!GetTransaction(txid, txRef, Params().GetConsensus(), hashBlock, true))
                     continue;
 
-                auto it = mapBlockIndex.find(hashBlock);
-                if (it == mapBlockIndex.end())
+                CBlockIndex* block = LookupBlockHash(hashBlock);
+                if (block == nullptr)
                     continue;
 
-                if (!chainActive.Contains(it->second))
+                if (!chainActive.Contains(block))
                     continue;
 
-                nDepth = chainActive.Height() - it->second->nHeight + 1;
-                nTimeTx = it->second->GetBlockTime();
+                nDepth = chainActive.Height() - block->nHeight + 1;
+                nTimeTx = block->GetBlockTime();
             }
 
             for (unsigned int i = 0; i < txRecord.vout.size(); i++) {
@@ -1062,15 +1063,15 @@ void CoinControlDialog::updateView(int nCoinType)
                 if (!GetTransaction(mint.txid, txRef, Params().GetConsensus(), hashBlock, true))
                     continue;
 
-                auto it = mapBlockIndex.find(hashBlock);
-                if (it == mapBlockIndex.end())
+                CBlockIndex* block = LookupBlockHash(hashBlock);
+                if (block == nullptr)
                     continue;
 
-                if (!chainActive.Contains(it->second))
+                if (!chainActive.Contains(block))
                     continue;
 
-                nDepth = chainActive.Height() - it->second->nHeight + 1;
-                nTimeTx = it->second->GetBlockTime();
+                nDepth = chainActive.Height() - block->nHeight + 1;
+                nTimeTx = block->GetBlockTime();
             }
 
             // increase the count for each denom we see

src/rest.cpp

@@ -147,16 +147,14 @@ static bool rest_headers(HTTPRequest* req,
     const CBlockIndex* tip = nullptr;
     std::vector<const CBlockIndex *> headers;
     headers.reserve(count);
-    {
-        LOCK(cs_main);
-        tip = chainActive.Tip();
-        const CBlockIndex* pindex = LookupBlockIndex(hash);
-        while (pindex != nullptr && chainActive.Contains(pindex)) {
-            headers.push_back(pindex);
-            if (headers.size() == (unsigned long)count)
-                break;
-            pindex = chainActive.Next(pindex);
-        }
+
+    tip = chainActive.Tip();
+    const CBlockIndex* pindex = LookupBlockIndex(hash);
+    while (pindex != nullptr && chainActive.Contains(pindex)) {
+        headers.push_back(pindex);
+        if (headers.size() == (unsigned long)count)
+            break;
+        pindex = chainActive.Next(pindex);
     }
 
     CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION);
@@ -210,22 +208,17 @@ static bool rest_block(HTTPRequest* req,
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
 
     CBlock block;
-    CBlockIndex* pblockindex = nullptr;
-    CBlockIndex* tip = nullptr;
-    {
-        LOCK(cs_main);
-        tip = chainActive.Tip();
-        pblockindex = LookupBlockIndex(hash);
-        if (!pblockindex) {
-            return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
-        }
+    CBlockIndex* tip = chainActive.Tip();
+    CBlockIndex* pblockindex = LookupBlockIndex(hash);
+    if (!pblockindex) {
+        return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
+    }
 
-        if (IsBlockPruned(pblockindex))
-            return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
+    if (IsBlockPruned(pblockindex))
+        return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
 
-        if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))
-            return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
-    }
+    if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))
+        return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
 
     CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
     ssBlock << block;