Merge bitcoin#2223: [BUG] Fix some chainstate-init-order bugs

4749d52 [Refactoring][BUG] Unchecked LoadGenesisBlock return value (random-zebra)
43cc880 Fix segfault when shutting down before fully loading (Matt Corallo)
5f1f014 Order chainstate init more logically. (random-zebra)
9b87537 More user-friendly error message if UTXO DB runs ahead of block DB (Matt Corallo)
9bcc942 Fix some LoadChainTip-related init-order bugs. (random-zebra)

Pull request description:

  Had this segfault when trying to shut down the wallet before the coins cache is loaded.
  ```
  CCoinsViewCache::DynamicMemoryUsage (this=0x0) at coins.cpp:60
  60	    return memusage::DynamicUsage(cacheCoins) +
  (gdb) bt
  #0  0x0000555555bb8b44 in CCoinsViewCache::DynamicMemoryUsage() const (this=0x0) at coins.cpp:60
  #1  0x0000555555996bc1 in FlushStateToDisk(CValidationState&, FlushStateMode) (state=..., mode=mode@entry=FLUSH_STATE_ALWAYS) at validation.cpp:1730
  #2  0x0000555555997540 in FlushStateToDisk() () at validation.cpp:1806
  #3  0x000055555582516c in PrepareShutdown() () at init.cpp:266
  #4  0x0000555555826005 in Shutdown() () at init.cpp:339
  ```

  This is due to a bug introduced in 64c525b (we should null-check `pcoinsTip` before calling `FlushStateToDisk` at line 266, same as we do at line 283).

  Upstream fixed it in bitcoin#10758 among few other things.
  Backported here without ff3a219 (as we don't have `RewindBlockIndex` or `reindex-chainstate` yet).

ACKs for top commit:
  Fuzzbawls:
    ACK 4749d52
  furszy:
    ACK 4749d52 and merging..

Tree-SHA512: 23f27c4a9422a72794054698313259f89ad22a58dba2809901653e08c964c73e4eaeab872ada42380e76eb46ddd7e57f08b65f22812c791d0c2b274e5f0202a7

src/init.cpp

@@ -263,7 +263,9 @@ void PrepareShutdown()
     }
 
     // FlushStateToDisk generates a SetBestChain callback, which we should avoid missing
-    FlushStateToDisk();
+    if (pcoinsTip != nullptr) {
+        FlushStateToDisk();
+    }
 
     // After there are no more peers/RPC left to give us new data which may generate
     // CValidationInterface callbacks, flush them...
@@ -688,7 +690,9 @@ void ThreadImport(const std::vector<fs::path>& vImportFiles)
         fReindex = false;
         LogPrintf("Reindexing finished\n");
         // To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
-        InitBlockIndex();
+        if (!LoadGenesisBlock()) {
+            throw std::runtime_error("Error initializing block database");
+        }
     }
 
     // hardcoded $DATADIR/bootstrap.dat
@@ -1581,18 +1585,9 @@ bool AppInitMain()
                 pSporkDB = new CSporkDB(0, false, false);
 
                 pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReindex);
-                pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReindex);
-                pcoinscatcher = new CCoinsViewErrorCatcher(pcoinsdbview);
 
                 if (fReindex) {
                     pblocktree->WriteReindexing(true);
-                } else {
-                    uiInterface.InitMessage(_("Upgrading coins database..."));
-                    // If necessary, upgrade from older database format.
-                    if (!pcoinsdbview->Upgrade()) {
-                        strLoadError = _("Error upgrading chainstate database");
-                        break;
-                    }
                 }
 
                 // End loop if shutdown was requested
@@ -1602,6 +1597,9 @@ bool AppInitMain()
                 uiInterface.InitMessage(_("Loading sporks..."));
                 sporkManager.LoadSporksFromDB();
 
+                // LoadBlockIndex will load fTxIndex from the db, or set it if
+                // we're reindexing. It will also load fHavePruned if we've
+                // ever removed a block file from disk.
                 uiInterface.InitMessage(_("Loading block index..."));
                 std::string strBlockIndexError;
                 if (!LoadBlockIndex(strBlockIndexError)) {
@@ -1619,24 +1617,54 @@ bool AppInitMain()
                 if (!mapBlockIndex.empty() && mapBlockIndex.count(consensus.hashGenesisBlock) == 0)
                     return UIError(_("Incorrect or no genesis block found. Wrong datadir for network?"));
 
-                // Initialize the block index (no-op if non-empty database was already loaded)
-                if (!InitBlockIndex()) {
+                // Check for changed -txindex state
+                if (fTxIndex != gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
+                    strLoadError = _("You need to rebuild the database using -reindex to change -txindex");
+                    break;
+                }
+
+                // At this point blocktree args are consistent with what's on disk.
+                // If we're not mid-reindex (based on disk + args), add a genesis block on disk.
+                // This is called again in ThreadImport in the reindex completes.
+                if (!fReindex && !LoadGenesisBlock()) {
                     strLoadError = _("Error initializing block database");
                     break;
                 }
 
-                // Check for changed -txindex state
-                if (fTxIndex != gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
-                    strLoadError = _("You need to rebuild the database using -reindex to change -txindex");
+                // At this point we're either in reindex or we've loaded a useful
+                // block tree into mapBlockIndex!
+
+                pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReindex);
+                pcoinscatcher = new CCoinsViewErrorCatcher(pcoinsdbview);
+
+                // If necessary, upgrade from older database format.
+                // This is a no-op if we cleared the coinsviewdb with -reindex (or -reindex-chainstate !TODO)
+                uiInterface.InitMessage(_("Upgrading coins database if needed..."));
+                // If necessary, upgrade from older database format.
+                if (!pcoinsdbview->Upgrade()) {
+                    strLoadError = _("Error upgrading chainstate database");
                     break;
                 }
 
+                // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex (or -reindex-chainstate !TODO)
                 if (!ReplayBlocks(chainparams, pcoinsdbview)) {
                     strLoadError = _("Unable to replay blocks. You will need to rebuild the database using -reindex.");
                     break;
                 }
+
+                // The on-disk coinsdb is now in a good state, create the cache
                 pcoinsTip = new CCoinsViewCache(pcoinscatcher);
-                LoadChainTip(chainparams);
+
+                // !TODO: after enabling reindex-chainstate
+                // if (!fReindex && !fReindexChainState) {
+                if (!fReindex) {
+                    // LoadChainTip sets chainActive based on pcoinsTip's best block
+                    if (!LoadChainTip(chainparams)) {
+                        strLoadError = _("Error initializing block database");
+                        break;
+                    }
+                    assert(chainActive.Tip() != NULL);
+                }
 
                 // Populate list of invalid/fraudulent outpoints that are banned from the chain
                 invalid_out::LoadOutpoints();
@@ -1664,6 +1692,8 @@ bool AppInitMain()
                     }
                 }
 
+                // !TODO: after enabling reindex-chainstate
+                // if (!fReindex && !fReindexChainState) {
                 if (!fReindex) {
                     uiInterface.InitMessage(_("Verifying blocks..."));
 

src/test/test_pivx.cpp

@@ -72,7 +72,9 @@ TestingSetup::TestingSetup()
         pblocktree = new CBlockTreeDB(1 << 20, true);
         pcoinsdbview = new CCoinsViewDB(1 << 23, true);
         pcoinsTip = new CCoinsViewCache(pcoinsdbview);
-        InitBlockIndex();
+        if (!LoadGenesisBlock()) {
+            throw std::runtime_error("Error initializing block database");
+        }
         {
             CValidationState state;
             bool ok = ActivateBestChain(state);

src/validation.cpp

@@ -3517,14 +3517,25 @@ bool static LoadBlockIndexDB(std::string& strError)
     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)) {
+            return false;
+        }
+    }
 
     // Load pointer to end of best chain
     BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock());
-    if (it == mapBlockIndex.end())
-        return;
+    if (it == mapBlockIndex.end()) {
+        return false;
+    }
     chainActive.SetTip(it->second);
 
     PruneBlockIndexCandidates();
@@ -3535,6 +3546,7 @@ void LoadChainTip(const CChainParams& chainparams)
             pChainTip->GetBlockHash().GetHex(), pChainTip->nHeight,
             DateTimeStrFormat("%Y-%m-%d %H:%M:%S", pChainTip->GetBlockTime()),
             Checkpoints::GuessVerificationProgress(pChainTip));
+    return true;
 }
 
 CVerifyDB::CVerifyDB()
@@ -3743,45 +3755,56 @@ void UnloadBlockIndex()
 
 bool LoadBlockIndex(std::string& strError)
 {
-    // Load block index from databases
-    if (!fReindex && !LoadBlockIndexDB(strError))
-        return false;
+    bool needs_init = fReindex;
+    if (!fReindex) {
+        if (!LoadBlockIndexDB(strError))
+            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 = gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX);
+        pblocktree->WriteFlag("txindex", fTxIndex);
+    }
     return true;
 }
 
 
-bool InitBlockIndex()
+bool LoadGenesisBlock()
 {
     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(Params().GenesisBlock().GetHash()))
         return true;
 
-    // Use the provided setting for -txindex in the new database
-    fTxIndex = gArgs.GetBoolArg("-txindex", true);
-    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&>(Params().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))
-                return error("LoadBlockIndex() : writing genesis block to disk failed");
-            CBlockIndex* pindex = AddToBlockIndex(block);
-            if (!ReceivedBlockTransactions(block, state, pindex, blockPos))
-                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&>(Params().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))
+            return error("%s: writing genesis block to disk failed", __func__);
+        CBlockIndex *pindex = AddToBlockIndex(block);
+        if (!ReceivedBlockTransactions(block, state, pindex, blockPos))
+            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;
 }

src/validation.h

@@ -183,12 +183,13 @@ FILE* OpenUndoFile(const CDiskBlockPos& pos, bool fReadOnly = false);
 fs::path GetBlockPosFilename(const CDiskBlockPos& pos, const char* prefix);
 /** Import blocks from an external file */
 bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos* dbp = NULL);
-/** Initialize a new block tree database + block data on disk */
-bool InitBlockIndex();
-/** Load the block tree and coins database from disk */
+/** Ensures we have a genesis block in the block tree, possibly writing one to disk. */
+bool LoadGenesisBlock();
+/** Load the block tree and coins database from disk,
+ * initializing state if we're running with -reindex. */
 bool LoadBlockIndex(std::string& strError);
 /** Update the chain tip based on database information. */
-void LoadChainTip(const CChainParams& chainparams);
+bool LoadChainTip(const CChainParams& chainparams);
 /** Unload database information */
 void UnloadBlockIndex();
 /** See whether the protocol update is enforced for connected nodes */