Merge bitcoin/bitcoin#31046: init: Some small chainstate load improvements

31cc500 init: Return fatal failure on snapshot validation failure (Martin Zumsande)
8f1246e init: Improve chainstate init db error messages (TheCharlatan)
cd09304 init: Remove incorrect comment about shutdown condition (MarcoFalke)
635e9f8 init: Remove misleading log line when user chooses not to retry (TheCharlatan)
720ce88 init: Improve comment describing chainstate load retry behaviour (Martin Zumsande)
baea842 init: Remove unneeded argument for mempool_opts checks (stickies-v)

Pull request description:

  These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.

  The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.

ACKs for top commit:
  achow101:
    ACK 31cc500
  mzumsande:
    Code Review / lightly tested ACK 31cc500
  BrandonOdiwuor:
    Code Review ACK 31cc500.
  stickies-v:
    ACK 31cc500

Tree-SHA512: 59fba4845ee45a3d91bf55807ae6b1c81458463b96bf664c8b1badfac503f6b01efd52a915fc399294e68a3f69985362a5a10a3844fa23f7707145ebe9ad349b

Author: achow101

src/init.cpp

@@ -1062,9 +1062,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         if (!blockman_result) {
             return InitError(util::ErrorString(blockman_result));
         }
-        CTxMemPool::Options mempool_opts{
-            .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0,
-        };
+        CTxMemPool::Options mempool_opts{};
         auto mempool_result{ApplyArgsManOptions(args, chainparams, mempool_opts)};
         if (!mempool_result) {
             return InitError(util::ErrorString(mempool_result));
@@ -1173,7 +1171,7 @@ bool CheckHostPortOptions(const ArgsManager& args) {
     return true;
 }
 
-// A GUI user may opt to retry once if there is a failure during chainstate initialization.
+// A GUI user may opt to retry once with do_reindex set if there is a failure during chainstate initialization.
 // The function therefore has to support re-entry.
 static ChainstateLoadResult InitAndLoadChainstate(
     NodeContext& node,
@@ -1253,7 +1251,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
             return f();
         } catch (const std::exception& e) {
             LogError("%s\n", e.what());
-            return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database"));
+            return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error loading databases"));
         }
     };
     auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
@@ -1634,11 +1632,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     if (status == ChainstateLoadStatus::FAILURE && !do_reindex && !ShutdownRequested(node)) {
         // suggest a reindex
         bool do_retry = uiInterface.ThreadSafeQuestion(
-            error + Untranslated(".\n\n") + _("Do you want to rebuild the block database now?"),
+            error + Untranslated(".\n\n") + _("Do you want to rebuild the databases now?"),
             error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
             "", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);
         if (!do_retry) {
-            LogError("Aborted block database rebuild. Exiting.\n");
             return false;
         }
         do_reindex = true;
@@ -1658,7 +1655,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // As LoadBlockIndex can take several minutes, it's possible the user
     // requested to kill the GUI during the last operation. If so, exit.
-    // As the program has not fully started yet, Shutdown() is possibly overkill.
     if (ShutdownRequested(node)) {
         LogPrintf("Shutdown requested. Exiting.\n");
         return false;

src/node/chainstate.cpp

@@ -41,12 +41,17 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     // new BlockTreeDB tries to delete the existing file, which
     // fails if it's still open from the previous loop. Close it first:
     pblocktree.reset();
-    pblocktree = std::make_unique<BlockTreeDB>(DBParams{
-        .path = chainman.m_options.datadir / "blocks" / "index",
-        .cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db),
-        .memory_only = options.block_tree_db_in_memory,
-        .wipe_data = options.wipe_block_tree_db,
-        .options = chainman.m_options.block_tree_db});
+    try {
+        pblocktree = std::make_unique<BlockTreeDB>(DBParams{
+            .path = chainman.m_options.datadir / "blocks" / "index",
+            .cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db),
+            .memory_only = options.block_tree_db_in_memory,
+            .wipe_data = options.wipe_block_tree_db,
+            .options = chainman.m_options.block_tree_db});
+    } catch (dbwrapper_error& err) {
+        LogError("%s\n", err.what());
+        return {ChainstateLoadStatus::FAILURE, _("Error opening block database")};
+    }
 
     if (options.wipe_block_tree_db) {
         pblocktree->WriteReindexing(true);
@@ -107,10 +112,15 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     for (Chainstate* chainstate : chainman.GetAll()) {
         LogPrintf("Initializing chainstate %s\n", chainstate->ToString());
 
-        chainstate->InitCoinsDB(
-            /*cache_size_bytes=*/chainman.m_total_coinsdb_cache * init_cache_fraction,
-            /*in_memory=*/options.coins_db_in_memory,
-            /*should_wipe=*/options.wipe_chainstate_db);
+        try {
+            chainstate->InitCoinsDB(
+                /*cache_size_bytes=*/chainman.m_total_coinsdb_cache * init_cache_fraction,
+                /*in_memory=*/options.coins_db_in_memory,
+                /*should_wipe=*/options.wipe_chainstate_db);
+        } catch (dbwrapper_error& err) {
+            LogError("%s\n", err.what());
+            return {ChainstateLoadStatus::FAILURE, _("Error opening coins database")};
+        }
 
         if (options.coins_error_cb) {
             chainstate->CoinsErrorCatcher().AddReadErrCallback(options.coins_error_cb);
@@ -236,7 +246,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
             return {init_status, init_error};
         }
     } else {
-        return {ChainstateLoadStatus::FAILURE, _(
+        return {ChainstateLoadStatus::FAILURE_FATAL, _(
            "UTXO snapshot failed to validate. "
            "Restart to resume normal initial block download, or try loading a different snapshot.")};
     }

test/functional/feature_init.py

@@ -100,13 +100,13 @@ def check_clean_start():
 
         files_to_delete = {
             'blocks/index/*.ldb': 'Error opening block database.',
-            'chainstate/*.ldb': 'Error opening block database.',
+            'chainstate/*.ldb': 'Error opening coins database.',
             'blocks/blk*.dat': 'Error loading block database.',
         }
 
         files_to_perturb = {
             'blocks/index/*.ldb': 'Error loading block database.',
-            'chainstate/*.ldb': 'Error opening block database.',
+            'chainstate/*.ldb': 'Error opening coins database.',
             'blocks/blk*.dat': 'Corrupted block database detected.',
         }