resolve reindexing deadlock/crash (#188)

* remove unnecessary locks

* add missing lock before AcceptToMemoryPoolWorker

* remove some improper cs_main locks and replace with mapblockindex locks

* add nullptr check to GetMedianTimePast

* check for pindexprev before getting the median time past which uses prev

src/chain/blockindex.h

@@ -269,7 +269,12 @@ class CBlockIndex
 
         const CBlockIndex *pindex = this;
         for (int i = 0; i < nMedianTimeSpan && pindex; i++, pindex = pindex->pprev)
-            *(--pbegin) = pindex->GetBlockTime();
+        {
+            if (pindex)
+            {
+                *(--pbegin) = pindex->GetBlockTime();
+            }
+        }
 
         std::sort(pbegin, pend);
         return pbegin[(pend - pbegin) / 2];

src/chain/chainman.cpp

@@ -333,7 +333,6 @@ bool CChainManager::LoadBlockIndexDB()
 
 bool CChainManager::LoadExternalBlockFile(const CNetworkTemplate &chainparams, FILE *fileIn, CDiskBlockPos *dbp)
 {
-    WRITELOCK(cs_mapBlockIndex);
     // std::map of disk positions for blocks with unknown parent (only used for reindex)
     static std::multimap<uint256, CDiskBlockPos> mapBlocksUnknownParent;
     int64_t nStart = GetTimeMillis();
@@ -348,7 +347,7 @@ bool CChainManager::LoadExternalBlockFile(const CNetworkTemplate &chainparams, F
         {
             if (shutdown_threads.load())
             {
-                break;
+                return nLoaded;
             }
 
             blkdat.SetPos(nRewind);
@@ -453,7 +452,9 @@ bool CChainManager::LoadExternalBlockFile(const CNetworkTemplate &chainparams, F
         AbortNode(std::string("System error: ") + e.what());
     }
     if (nLoaded > 0)
+    {
         LogPrintf("Loaded %i blocks from external file in %dms\n", nLoaded, GetTimeMillis() - nStart);
+    }
     return nLoaded > 0;
 }
 

src/init.cpp

@@ -695,10 +695,14 @@ void ThreadImport(std::vector<fs::path> vImportFiles)
         {
             CDiskBlockPos pos(nFile, 0);
             if (!fs::exists(GetBlockPosFilename(pos, "blk")))
+            {
                 break; // No block files left to reindex
+            }
             FILE *file = OpenBlockFile(pos, true);
             if (!file)
+            {
                 break; // This error is logged in OpenBlockFile
+            }
             LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
             pnetMan->getChainActive()->LoadExternalBlockFile(chainparams, file, &pos);
             nFile++;
@@ -1440,7 +1444,6 @@ bool AppInit2(thread_group &threadGroup)
     LogPrintf("* Using %.1fMiB for chain state database\n", nCoinDBCache * (1.0 / 1024 / 1024));
     LogPrintf("* Using %.1fMiB for in-memory UTXO set\n", nCoinCacheUsage * (1.0 / 1024 / 1024));
 
-    LOCK(cs_main);
     bool fLoaded = false;
     while (!fLoaded)
     {

src/main.cpp

@@ -821,6 +821,7 @@ bool AcceptToMemoryPool(CTxMemPool &pool,
     bool fRejectAbsurdFee)
 {
     std::vector<COutPoint> vCoinsToUncache;
+    LOCK(cs_main);
     bool res = AcceptToMemoryPoolWorker(
         pool, state, tx, fLimitFree, pfMissingInputs, fOverrideMempoolLimit, fRejectAbsurdFee, vCoinsToUncache);
     if (pfMissingInputs && !res && !*pfMissingInputs)
@@ -1266,8 +1267,7 @@ void PruneBlockIndexCandidates()
 
 bool InvalidateBlock(CValidationState &state, const Consensus::Params &consensusParams, CBlockIndex *pindex)
 {
-    AssertLockHeld(cs_main);
-
+    WRITELOCK(pnetMan->getChainActive()->cs_mapBlockIndex);
     // Mark the block itself as invalid.
     pindex->nStatus |= BLOCK_FAILED_VALID;
     setDirtyBlockIndex.insert(pindex);
@@ -1292,20 +1292,15 @@ bool InvalidateBlock(CValidationState &state, const Consensus::Params &consensus
     LimitMempoolSize(mempool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000,
         gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60);
 
-    // The resulting new best tip may not be in setBlockIndexCandidates anymore, so
-    // add it again.
+    BlockMap::iterator it = pnetMan->getChainActive()->mapBlockIndex.begin();
+    while (it != pnetMan->getChainActive()->mapBlockIndex.end())
     {
-        READLOCK(pnetMan->getChainActive()->cs_mapBlockIndex);
-        BlockMap::iterator it = pnetMan->getChainActive()->mapBlockIndex.begin();
-        while (it != pnetMan->getChainActive()->mapBlockIndex.end())
+        if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx &&
+            !setBlockIndexCandidates.value_comp()(it->second, pnetMan->getChainActive()->chainActive.Tip()))
         {
-            if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx &&
-                !setBlockIndexCandidates.value_comp()(it->second, pnetMan->getChainActive()->chainActive.Tip()))
-            {
-                setBlockIndexCandidates.insert(it->second);
-            }
-            it++;
+            setBlockIndexCandidates.insert(it->second);
         }
+        it++;
     }
 
     InvalidChainFound(pindex);
@@ -1316,11 +1311,8 @@ bool InvalidateBlock(CValidationState &state, const Consensus::Params &consensus
 
 bool ReconsiderBlock(CValidationState &state, CBlockIndex *pindex)
 {
-    AssertLockHeld(cs_main);
-
     int nHeight = pindex->nHeight;
-
-    READLOCK(pnetMan->getChainActive()->cs_mapBlockIndex);
+    WRITELOCK(pnetMan->getChainActive()->cs_mapBlockIndex);
     // Remove the invalidity flag from this block
     if (!pindex->IsValid())
     {
@@ -1337,7 +1329,6 @@ bool ReconsiderBlock(CValidationState &state, CBlockIndex *pindex)
             pindexBestInvalid = NULL;
         }
     }
-
     // Remove the invalidity flag from all descendants.
     BlockMap::iterator it = pnetMan->getChainActive()->mapBlockIndex.begin();
     while (it != pnetMan->getChainActive()->mapBlockIndex.end())
@@ -1655,8 +1646,12 @@ bool ContextualCheckBlock(const CBlock &block, CValidationState &state, CBlockIn
     int nLockTimeFlags = 0;
     nLockTimeFlags |= LOCKTIME_MEDIAN_TIME_PAST;
 
-    int64_t nLockTimeCutoff =
-        (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) ? pindexPrev->GetMedianTimePast() : block.GetBlockTime();
+    int64_t nLockTimeCutoff;
+    if (pindexPrev == nullptr)
+        nLockTimeCutoff = block.GetBlockTime();
+    else
+        nLockTimeCutoff =
+            (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) ? pindexPrev->GetMedianTimePast() : block.GetBlockTime();
 
     // Check that all transactions are finalized
     for (auto const &tx : block.vtx)

src/net/messages.cpp

@@ -82,7 +82,6 @@ std::unique_ptr<CRollingBloomFilter> recentRejects;
 std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> > mapBlocksInFlight;
 uint256 hashRecentRejectsChainTip;
 std::map<uint256, std::pair<NodeId, bool> > mapBlockSource;
-/** Relay map, protected by cs_main. */
 CCriticalSection cs_mapRelay;
 std::map<uint256, CTransaction> mapRelay;
 std::deque<std::pair<int64_t, std::map<uint256, CTransaction>::iterator> > vRelayExpiration;

src/rpc/rpcblockchain.cpp

@@ -807,7 +807,6 @@ static std::set<CBlockIndex *, CompareBlocksByHeight> GetChainTips()
     std::set<CBlockIndex *> setOrphans;
     std::set<CBlockIndex *> setPrevs;
 
-    AssertLockHeld(cs_main); // for chainActive
     READLOCK(pnetMan->getChainActive()->cs_mapBlockIndex);
     for (const std::pair<const uint256, CBlockIndex *> &item : pnetMan->getChainActive()->mapBlockIndex)
     {