kernel: Add less confusing reindex options

Drop confusing kernel options:

  BlockManagerOpts::reindex
  ChainstateLoadOptions::reindex
  ChainstateLoadOptions::reindex_chainstate

Replacing them with more straightforward options:

  ChainstateLoadOptions::wipe_block_tree_db
  ChainstateLoadOptions::wipe_chainstate_db

Having two options called "reindex" which did slightly different things
was needlessly confusing (one option wiped the block tree database, and
the other caused block files to be rescanned). Also the previous set of
options did not allow rebuilding the block database without also
rebuilding the chainstate database, when it should be possible to do
those independently.

7 files changed, 27 insertions, 24 deletions

diff --git a/src/init.cpp b/src/init.cpp
index 343e23280a..fe69393795 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1482,7 +1482,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status);
     ReadNotificationArgs(args, *node.notifications);
-    bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false);
     ChainstateManager::Options chainman_opts{
         .chainparams = chainparams,
         .datadir = args.GetDataDirNet(),
@@ -1531,6 +1530,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }
     LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));
 
+    bool do_reindex{args.GetBoolArg("-reindex", false)};
+    const bool do_reindex_chainstate{args.GetBoolArg("-reindex-chainstate", false)};
+
     for (bool fLoaded = false; !fLoaded && !ShutdownRequested(node);) {
         node.mempool = std::make_unique<CTxMemPool>(mempool_opts);
 
@@ -1558,8 +1560,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         node::ChainstateLoadOptions options;
         options.mempool = Assert(node.mempool.get());
-        options.reindex = blockman_opts.reindex;
-        options.reindex_chainstate = fReindexChainState;
+        options.wipe_block_tree_db = do_reindex;
+        options.wipe_chainstate_db = do_reindex || do_reindex_chainstate;
         options.prune = chainman.m_blockman.IsPruneMode();
         options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
         options.check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL);
@@ -1600,13 +1602,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         if (!fLoaded && !ShutdownRequested(node)) {
             // first suggest a reindex
-            if (!blockman_opts.reindex) {
+            if (!do_reindex) {
                 bool fRet = uiInterface.ThreadSafeQuestion(
                     error + Untranslated(".\n\n") + _("Do you want to rebuild the block database now?"),
                     error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
                     "", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);
                 if (fRet) {
-                    blockman_opts.reindex = true;
+                    do_reindex = true;
                     if (!Assert(node.shutdown)->reset()) {
                         LogPrintf("Internal error: failed to reset shutdown signal.\n");
                     }
@@ -1694,7 +1696,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     int chain_active_height = WITH_LOCK(cs_main, return chainman.ActiveChain().Height());
 
     // On first startup, warn on low block storage space
-    if (!chainman.m_blockman.m_reindexing && !fReindexChainState && chain_active_height <= 1) {
+    if (!do_reindex && !do_reindex_chainstate && chain_active_height <= 1) {
         uint64_t assumed_chain_bytes{chainparams.AssumedBlockchainSize() * 1024 * 1024 * 1024};
         uint64_t additional_bytes_needed{
             chainman.m_blockman.IsPruneMode() ?
diff --git a/src/kernel/blockmanager_opts.h b/src/kernel/blockmanager_opts.h
index 16072b669b..deeba7e318 100644
--- a/src/kernel/blockmanager_opts.h
+++ b/src/kernel/blockmanager_opts.h
@@ -24,7 +24,6 @@ struct BlockManagerOpts {
     bool fast_prune{false};
     const fs::path blocks_dir;
     Notifications& notifications;
-    bool reindex{false};
 };
 
 } // namespace kernel
diff --git a/src/node/blockmanager_args.cpp b/src/node/blockmanager_args.cpp
index dd8419a68a..fa76566652 100644
--- a/src/node/blockmanager_args.cpp
+++ b/src/node/blockmanager_args.cpp
@@ -33,8 +33,6 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op
 
     if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
 
-    if (auto value{args.GetBoolArg("-reindex")}) opts.reindex = *value;
-
     return {};
 }
 } // namespace node
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index a501067091..bc09d102e2 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -267,8 +267,7 @@ public:
     explicit BlockManager(const util::SignalInterrupt& interrupt, Options opts)
         : m_prune_mode{opts.prune_target > 0},
           m_opts{std::move(opts)},
-          m_interrupt{interrupt},
-          m_reindexing{m_opts.reindex} {};
+          m_interrupt{interrupt} {}
 
     const util::SignalInterrupt& m_interrupt;
     std::atomic<bool> m_importing{false};
@@ -278,7 +277,7 @@ public:
      * is requested and false when reindexing completes. Its value is persisted
      * in the BlockTreeDB across restarts.
      */
-    std::atomic_bool m_reindexing;
+    std::atomic_bool m_reindexing{false};
 
     BlockMap m_block_index GUARDED_BY(cs_main);
 
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index d6eb14f513..0e1c2177fc 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -45,11 +45,12 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         .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.reindex,
+        .wipe_data = options.wipe_block_tree_db,
         .options = chainman.m_options.block_tree_db});
 
-    if (options.reindex) {
+    if (options.wipe_block_tree_db) {
         pblocktree->WriteReindexing(true);
+        chainman.m_blockman.m_reindexing = true;
         //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
         if (options.prune) {
             chainman.m_blockman.CleanupBlockRevFiles();
@@ -61,7 +62,6 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     // LoadBlockIndex will load m_have_pruned if we've ever removed a
     // block file from disk.
     // Note that it also sets m_reindexing based on the disk flag!
-    // From here on, m_reindexing and options.reindex values may be different!
     if (!chainman.LoadBlockIndex()) {
         if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
         return {ChainstateLoadStatus::FAILURE, _("Error loading block database")};
@@ -89,7 +89,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     }
 
     auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
+        return options.wipe_chainstate_db || chainstate->CoinsTip().GetBestBlock().IsNull();
     };
 
     assert(chainman.m_total_coinstip_cache > 0);
@@ -110,7 +110,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         chainstate->InitCoinsDB(
             /*cache_size_bytes=*/chainman.m_total_coinsdb_cache * init_cache_fraction,
             /*in_memory=*/options.coins_db_in_memory,
-            /*should_wipe=*/options.reindex || options.reindex_chainstate);
+            /*should_wipe=*/options.wipe_chainstate_db);
 
         if (options.coins_error_cb) {
             chainstate->CoinsErrorCatcher().AddReadErrCallback(options.coins_error_cb);
@@ -142,7 +142,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         }
     }
 
-    if (!options.reindex) {
+    if (!options.wipe_block_tree_db) {
         auto chainstates{chainman.GetAll()};
         if (std::any_of(chainstates.begin(), chainstates.end(),
                         [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
@@ -188,7 +188,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     // Load a chain created from a UTXO snapshot, if any exist.
     bool has_snapshot = chainman.DetectSnapshotChainstate();
 
-    if (has_snapshot && (options.reindex || options.reindex_chainstate)) {
+    if (has_snapshot && options.wipe_chainstate_db) {
         LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n");
         if (!chainman.DeleteSnapshotChainstate()) {
             return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")};
@@ -247,7 +247,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
 ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options)
 {
     auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
+        return options.wipe_chainstate_db || chainstate->CoinsTip().GetBestBlock().IsNull();
     };
 
     LOCK(cs_main);
diff --git a/src/node/chainstate.h b/src/node/chainstate.h
index a6e9a0331b..bb0c4f2b87 100644
--- a/src/node/chainstate.h
+++ b/src/node/chainstate.h
@@ -22,8 +22,13 @@ struct ChainstateLoadOptions {
     CTxMemPool* mempool{nullptr};
     bool block_tree_db_in_memory{false};
     bool coins_db_in_memory{false};
-    bool reindex{false};
-    bool reindex_chainstate{false};
+    // Whether to wipe the block tree database when loading it. If set, this
+    // will also set a reindexing flag so any existing block data files will be
+    // scanned and added to the database.
+    bool wipe_block_tree_db{false};
+    // Whether to wipe the chainstate database when loading it. If set, this
+    // will cause the chainstate database to be rebuilt starting from genesis.
+    bool wipe_chainstate_db{false};
     bool prune{false};
     //! Setting require_full_verification to true will require all checks at
     //! check_level (below) to succeed for loading to succeed. Setting it to
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index fd07931716..b366e83dde 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -276,8 +276,8 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
     options.mempool = Assert(m_node.mempool.get());
     options.block_tree_db_in_memory = m_block_tree_db_in_memory;
     options.coins_db_in_memory = m_coins_db_in_memory;
-    options.reindex = chainman.m_blockman.m_reindexing;
-    options.reindex_chainstate = m_args.GetBoolArg("-reindex-chainstate", false);
+    options.wipe_block_tree_db = m_args.GetBoolArg("-reindex", false);
+    options.wipe_chainstate_db = m_args.GetBoolArg("-reindex", false) || m_args.GetBoolArg("-reindex-chainstate", false);
     options.prune = chainman.m_blockman.IsPruneMode();
     options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
     options.check_level = m_args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL);