Merge #951: [Consensus] Protect mapBlockIndex with its own mutex

13dd025 chain: Set all CBlockIndex members to null, remove SetNull helper (MarcoFalke)
ad442a4 Give mapBlockIndex its own lock. (Zannick)

Pull request description:

  ### Problem
  tsan identified a data race on CChainState, particularly between AcceptBlock and ActivateBestChain (used often by the miner threads).

  ### Solution
  Mark `mapBlockIndex` as guarded by `cs_mapblockindex`, and ensure that the lock is taken prior to any use. Some code has been reorganized slightly to reduce the time spent holding the lock and avoid locking a second time for the same information.

  Move many lookups to use LookupBlockIndex, which grabs the lock itself, and remove lots of cs_main uses that would have otherwise blocked only for LookupBlockIndex.

  ### Tested
  Original: Configured with --with-sanitizers=tsan, built with clang, run on regtest with 2 SHA mining threads enabled.
  With cs_mapblockindex: configured --with-sanitizers=tsan, built with clang, run on testnet to catch up and mine 2 sha threads.

Tree-SHA512: 3a972c964fa365bd455bcd28a3f62d1d28e023bd17c47dbd42d2349549d95940b08271bb214bc1a1cea811fc106c57543e20e96297e84dbd0490de0c1dece0ec

src/chain.h

@@ -179,93 +179,105 @@ enum BlockMemFlags: uint32_t {
 class CBlockIndex
 {
 public:
-    //! pointer to the hash of the block, if any. Memory is owned by this CBlockIndex
-    const uint256* phashBlock;
+    //! pointer to the hash of the block, if any. Memory is owned by mapBlockIndex
+    const uint256* phashBlock{nullptr};
 
     //! pointer to the index of the predecessor of this block
-    CBlockIndex* pprev;
+    CBlockIndex* pprev{nullptr};
 
     //! pointer to the index of some further predecessor of this block
-    CBlockIndex* pskip;
+    CBlockIndex* pskip{nullptr};
 
     //! height of the entry in the chain. The genesis block has height 0
-    int nHeight;
+    int nHeight{0};
 
     //! money supply tracking
-    int64_t nMoneySupply;
+    int64_t nMoneySupply{0};
 
     //! zerocoin mint supply tracking
-    int64_t nMint;
+    int64_t nMint{0};
 
     //! Which # file this block is stored in (blk?????.dat)
-    int nFile;
+    int nFile{0};
 
     //! Byte offset within blk?????.dat where this block's data is stored
-    unsigned int nDataPos;
+    unsigned int nDataPos{0};
 
     //! Byte offset within rev?????.dat where this block's undo data is stored
-    unsigned int nUndoPos;
+    unsigned int nUndoPos{0};
 
     //! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block
-    arith_uint256 nChainWork;
+    arith_uint256 nChainWork{};
 
     //! (memory only) Total amount of work (only looking at PoW) in the chain up to and including this block
-    arith_uint256 nChainPoW;
+    arith_uint256 nChainPoW{};
 
     //! Number of transactions in this block.
     //! Note: in a potential headers-first mode, this number cannot be relied upon
-    unsigned int nTx;
+    unsigned int nTx{0};
 
     //! (memory only) Number of transactions in the chain up to and including this block.
     //! This value will be non-zero only if and only if transactions for this block and all its parents are available.
     //! Change to 64-bit type when necessary; won't happen before 2030
-    unsigned int nChainTx;
+    unsigned int nChainTx{0};
 
-    int64_t nAnonOutputs; // last index
+    int64_t nAnonOutputs{0}; // last index
 
     //! Verification status of this block. See enum BlockStatus
-    uint32_t nStatus;
+    uint32_t nStatus{0};
 
-    bool fProofOfStake;
-    bool fProofOfFullNode;
+    bool fProofOfStake{false};
+    bool fProofOfFullNode{false};
 
     //! Funds sent into the network to serve as an additional reward to stakers and miners
-    CAmount nNetworkRewardReserve;
+    CAmount nNetworkRewardReserve{0};
 
     //! block header
-    int32_t nVersion;
-    uint256 hashVeilData;
-    uint32_t nTime;
-    uint32_t nBits;
-    uint32_t nNonce;
+    int32_t nVersion{0};
+    uint256 hashVeilData{};
+    uint32_t nTime{0};
+    uint32_t nBits{0};
+    uint32_t nNonce{0};
 
     //! ProgPow Header items
     // Height was already in the CBlockIndex
-    uint64_t nNonce64;
-    uint256 mixHash;
+    uint64_t nNonce64{0};
+    uint256 mixHash{};
 
     //! (memory only) Sequential id assigned to distinguish order in which blocks are received.
-    int32_t nSequenceId;
+    int32_t nSequenceId{0};
 
     //! zerocoin specific fields
     std::map<libzerocoin::CoinDenomination, int64_t> mapZerocoinSupply;
     std::vector<libzerocoin::CoinDenomination> vMintDenominationsInBlock;
 
     //! (memory only) Maximum nTime in the chain up to and including this block.
-    unsigned int nTimeMax;
+    unsigned int nTimeMax{0};
 
     //! Hash value for the accumulator. Can be used to access the zerocoindb for the accumulator value
     std::map<libzerocoin::CoinDenomination ,uint256> mapAccumulatorHashes;
 
-    uint256 hashMerkleRoot;
-    uint256 hashWitnessMerkleRoot;
-    uint256 hashPoFN;
-    uint256 hashAccumulators;
+    uint256 hashMerkleRoot{};
+    uint256 hashWitnessMerkleRoot{};
+    uint256 hashPoFN{};
+    uint256 hashAccumulators{};
 
     //! vector that holds a proof of stake proof hash if the block has one. If not, its empty and has less memory
     //! overhead than an empty uint256
     std::vector<unsigned char> vHashProof;
 
+    void ResetMaps()
+    {
+        for (auto& denom : libzerocoin::zerocoinDenomList) {
+            mapAccumulatorHashes[denom] = uint256();
+        }
+
+        // Start supply of each denomination with 0s
+        for (auto& denom : libzerocoin::zerocoinDenomList) {
+            mapZerocoinSupply.insert(std::make_pair(denom, 0));
+        }
+    }
+
     void SetNull()
     {
         phashBlock = nullptr;
@@ -301,14 +313,7 @@ class CBlockIndex
         hashWitnessMerkleRoot = uint256();
         hashAccumulators = uint256();
 
-        for (auto& denom : libzerocoin::zerocoinDenomList) {
-            mapAccumulatorHashes[denom] = uint256();
-        }
-
-        // Start supply of each denomination with 0s
-        for (auto& denom : libzerocoin::zerocoinDenomList) {
-            mapZerocoinSupply.insert(std::make_pair(denom, 0));
-        }
+        ResetMaps();
 
         vMintDenominationsInBlock.clear();
 
@@ -327,27 +332,23 @@ class CBlockIndex
 
     CBlockIndex()
     {
-        SetNull();
+        ResetMaps();
     }
 
     explicit CBlockIndex(const CBlockHeader& block)
+        : nHeight{static_cast<int>(block.nHeight)},
+          nVersion{block.nVersion},
+          hashVeilData{block.hashVeilData},
+          nTime{block.nTime},
+          nBits{block.nBits},
+          nNonce{block.nNonce},
+          nNonce64{block.nNonce64},
+          mixHash{block.mixHash},
+          hashMerkleRoot{block.hashMerkleRoot},
+          hashWitnessMerkleRoot{block.hashWitnessMerkleRoot},
+          hashAccumulators{block.hashAccumulators}
     {
-        SetNull();
-
-        nVersion       = block.nVersion;
-        hashVeilData   = block.hashVeilData;
-        hashMerkleRoot = block.hashMerkleRoot;
-        hashWitnessMerkleRoot = block.hashWitnessMerkleRoot;
-        hashAccumulators = block.hashAccumulators;
-        nTime          = block.nTime;
-        nBits          = block.nBits;
-        nNonce         = block.nNonce;
-        nMint          = 0;
-
-        //ProgPow
-        nNonce64       = block.nNonce64;
-        mixHash        = block.mixHash;
-        nHeight        = block.nHeight;
+        ResetMaps();
     }
 
     CDiskBlockPos GetBlockPos() const {

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

@@ -1614,10 +1614,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
@@ -1847,14 +1850,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);