Merge #10758: Fix some chainstate-init-order bugs.

c0025d0 Fix segfault when shutting down before fully loading (Matt Corallo)
1385697 Order chainstate init more logically. (Matt Corallo)
ff3a219 Call RewindBlockIndex even if we're about to run -reindex-chainstate (Matt Corallo)
b0f3249 More user-friendly error message if UTXO DB runs ahead of block DB (Matt Corallo)
eda888e Fix some LoadChainTip-related init-order bugs. (Matt Corallo)

Pull request description:

  This does a number of things to clean up chainstate init order,
  fixing some issues as it goes:

  * Order chainstate init more logically - first all of the
    blocktree-related loading, then coinsdb, then
    pcoinsTip/chainActive. Only create objects as needed.

  * More clearly document exactly what is and isn't called in
    -reindex and -reindex-chainstate both with comments noting
    calls as no-ops and by adding if guards.

  * Move the writing of fTxIndex to LoadBlockIndex - this fixes a
    bug introduced in d6af06d68aae985436cbc942f0d11078041d121b where
    InitBlockIndex was writing to fTxIndex which had not yet been
    checked (because LoadChainTip hadn't yet initialized the
    chainActive, which would otherwise have resulted in
    InitBlockIndex being a NOP), allowing you to modify -txindex
    without reindex, potentially corrupting your chainstate!

  * Rename InitBlockIndex to LoadGenesisBlock, which is now a more
    natural name for it. Also check mapBlockIndex instead of
    chainActive, fixing a bug where we'd write the genesis block out
    on every start.

  * Move LoadGenesisBlock further down in init. This is a more logical
    location for it, as it is after all of the blockindex-related
    loading and checking, but before any of the UTXO-related loading
    and checking.

  * Give LoadChainTip a return value - allowing it to indicate that
    the UTXO DB ran ahead of the block DB. This just provides a nicer
    error message instead of the previous mysterious
    assert(!setBlockIndexCandidates.empty()) error.

  * Calls ActivateBestChain in case we just loaded the genesis
    block in LoadChainTip, avoiding relying on the ActivateBestChain
    in ThreadImport before continuing init process.

  * Move all of the VerifyDB()-related stuff into a -reindex +
    -reindex-chainstate if guard. It couldn't do anything useful
    as chainActive.Tip() would be null at this point anyway.

Tree-SHA512: 3c96ee7ed44f4130bee3479a40c5cd99a619fda5e309c26d60b54feab9f6ec60fabab8cf47a049c9cf15e88999b2edb7f16cbe6819e97273560b201a89d90762

1 files changed, 67 insertions, 34 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index d223715c46..babf6f1522 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3539,14 +3539,24 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams)
     return true;
 }
 
-void LoadChainTip(const CChainParams& chainparams)
+bool LoadChainTip(const CChainParams& chainparams)
 {
-    if (chainActive.Tip() && chainActive.Tip()->GetBlockHash() == pcoinsTip->GetBestBlock()) return;
+    if (chainActive.Tip() && chainActive.Tip()->GetBlockHash() == pcoinsTip->GetBestBlock()) return true;
+
+    if (pcoinsTip->GetBestBlock().IsNull() && mapBlockIndex.size() == 1) {
+        // In case we just added the genesis block, connect it now, so
+        // that we always have a chainActive.Tip() when we return.
+        LogPrintf("%s: Connecting genesis block...\n", __func__);
+        CValidationState state;
+        if (!ActivateBestChain(state, chainparams)) {
+            return false;
+        }
+    }
 
     // Load pointer to end of best chain
     BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock());
     if (it == mapBlockIndex.end())
-        return;
+        return false;
     chainActive.SetTip(it->second);
 
     PruneBlockIndexCandidates();
@@ -3555,6 +3565,7 @@ void LoadChainTip(const CChainParams& chainparams)
         chainActive.Tip()->GetBlockHash().ToString(), chainActive.Height(),
         DateTimeStrFormat("%Y-%m-%d %H:%M:%S", chainActive.Tip()->GetBlockTime()),
         GuessVerificationProgress(chainparams.TxData(), chainActive.Tip()));
+    return true;
 }
 
 CVerifyDB::CVerifyDB()
@@ -3751,6 +3762,8 @@ bool RewindBlockIndex(const CChainParams& params)
 {
     LOCK(cs_main);
 
+    // Note that during -reindex-chainstate we are called with an empty chainActive!
+
     int nHeight = 1;
     while (nHeight <= chainActive.Height()) {
         if (IsWitnessEnabled(chainActive[nHeight - 1], params.GetConsensus()) && !(chainActive[nHeight]->nStatus & BLOCK_OPT_WITNESS)) {
@@ -3820,12 +3833,19 @@ bool RewindBlockIndex(const CChainParams& params)
         }
     }
 
-    PruneBlockIndexCandidates();
+    if (chainActive.Tip() != NULL) {
+        // We can't prune block index candidates based on our tip if we have
+        // no tip due to chainActive being empty!
+        PruneBlockIndexCandidates();
 
-    CheckBlockIndex(params.GetConsensus());
+        CheckBlockIndex(params.GetConsensus());
 
-    if (!FlushStateToDisk(params, state, FLUSH_STATE_ALWAYS)) {
-        return false;
+        // FlushStateToDisk can possibly read chainActive. Be conservative
+        // and skip it here, we're about to -reindex-chainstate anyway, so
+        // it'll get called a bunch real soon.
+        if (!FlushStateToDisk(params, state, FLUSH_STATE_ALWAYS)) {
+            return false;
+        }
     }
 
     return true;
@@ -3863,42 +3883,55 @@ void UnloadBlockIndex()
 bool LoadBlockIndex(const CChainParams& chainparams)
 {
     // Load block index from databases
-    if (!fReindex && !LoadBlockIndexDB(chainparams))
-        return false;
+    bool needs_init = fReindex;
+    if (!fReindex) {
+        bool ret = LoadBlockIndexDB(chainparams);
+        if (!ret) return false;
+        needs_init = mapBlockIndex.empty();
+    }
+
+    if (needs_init) {
+        // Everything here is for *new* reindex/DBs. Thus, though
+        // LoadBlockIndexDB may have set fReindex if we shut down
+        // mid-reindex previously, we don't check fReindex and
+        // instead only check it prior to LoadBlockIndexDB to set
+        // needs_init.
+
+        LogPrintf("Initializing databases...\n");
+        // Use the provided setting for -txindex in the new database
+        fTxIndex = GetBoolArg("-txindex", DEFAULT_TXINDEX);
+        pblocktree->WriteFlag("txindex", fTxIndex);
+    }
     return true;
 }
 
-bool InitBlockIndex(const CChainParams& chainparams)
+bool LoadGenesisBlock(const CChainParams& chainparams)
 {
     LOCK(cs_main);
 
-    // Check whether we're already initialized
-    if (chainActive.Genesis() != NULL)
+    // Check whether we're already initialized by checking for genesis in
+    // mapBlockIndex. Note that we can't use chainActive here, since it is
+    // set based on the coins db, not the block index db, which is the only
+    // thing loaded at this point.
+    if (mapBlockIndex.count(chainparams.GenesisBlock().GetHash()))
         return true;
 
-    // Use the provided setting for -txindex in the new database
-    fTxIndex = GetBoolArg("-txindex", DEFAULT_TXINDEX);
-    pblocktree->WriteFlag("txindex", fTxIndex);
-    LogPrintf("Initializing databases...\n");
-
     // Only add the genesis block if not reindexing (in which case we reuse the one already on disk)
-    if (!fReindex) {
-        try {
-            CBlock &block = const_cast<CBlock&>(chainparams.GenesisBlock());
-            // Start new block file
-            unsigned int nBlockSize = ::GetSerializeSize(block, SER_DISK, CLIENT_VERSION);
-            CDiskBlockPos blockPos;
-            CValidationState state;
-            if (!FindBlockPos(state, blockPos, nBlockSize+8, 0, block.GetBlockTime()))
-                return error("LoadBlockIndex(): FindBlockPos failed");
-            if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart()))
-                return error("LoadBlockIndex(): writing genesis block to disk failed");
-            CBlockIndex *pindex = AddToBlockIndex(block);
-            if (!ReceivedBlockTransactions(block, state, pindex, blockPos, chainparams.GetConsensus()))
-                return error("LoadBlockIndex(): genesis block not accepted");
-        } catch (const std::runtime_error& e) {
-            return error("LoadBlockIndex(): failed to initialize block database: %s", e.what());
-        }
+    try {
+        CBlock &block = const_cast<CBlock&>(chainparams.GenesisBlock());
+        // Start new block file
+        unsigned int nBlockSize = ::GetSerializeSize(block, SER_DISK, CLIENT_VERSION);
+        CDiskBlockPos blockPos;
+        CValidationState state;
+        if (!FindBlockPos(state, blockPos, nBlockSize+8, 0, block.GetBlockTime()))
+            return error("%s: FindBlockPos failed", __func__);
+        if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart()))
+            return error("%s: writing genesis block to disk failed", __func__);
+        CBlockIndex *pindex = AddToBlockIndex(block);
+        if (!ReceivedBlockTransactions(block, state, pindex, blockPos, chainparams.GetConsensus()))
+            return error("%s: genesis block not accepted", __func__);
+    } catch (const std::runtime_error& e) {
+        return error("%s: failed to write genesis block: %s", __func__, e.what());
     }
 
     return true;