Merge bitcoin/bitcoin#30965: kernel: Move block tree db open to block manager

0cdddeb224 kernel: Move block tree db open to BlockManager constructor (TheCharlatan)
7fbb1bc44b kernel: Move block tree db open to block manager (TheCharlatan)
57ba59c0cd refactor: Remove redundant reindex check (TheCharlatan)

Pull request description:

  Before this change the block tree db was needlessly re-opened during startup when loading a completed snapshot. Improve this by letting the block manager open it on construction. This also simplifies the test code a bit.

  The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.

ACKs for top commit:
  maflcko:
    re-ACK 0cdddeb224 🏪
  achow101:
    ACK 0cdddeb224
  mzumsande:
    re-ACK 0cdddeb224

Tree-SHA512: fe3d557a725367e549e6a0659f64259cfef6aaa565ec867d9a177be0143ff18a2c4a20dd57e35e15f97cf870df476d88c05b03b6a7d9e8d51c568d9eda8947ef

src/node/blockmanager_args.cpp

@@ -6,6 +6,7 @@
 
 #include <common/args.h>
 #include <node/blockstorage.h>
+#include <node/database_args.h>
 #include <tinyformat.h>
 #include <util/result.h>
 #include <util/translation.h>
@@ -34,6 +35,8 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op
 
     if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
 
+    ReadDatabaseArgs(args, opts.block_tree_db_params.options);
+
     return {};
 }
 } // namespace node

src/node/blockstorage.cpp

@@ -36,6 +36,7 @@
 #include <util/translation.h>
 #include <validation.h>
 
+#include <cstddef>
 #include <map>
 #include <ranges>
 #include <unordered_map>
@@ -1169,7 +1170,19 @@ BlockManager::BlockManager(const util::SignalInterrupt& interrupt, Options opts)
       m_opts{std::move(opts)},
       m_block_file_seq{FlatFileSeq{m_opts.blocks_dir, "blk", m_opts.fast_prune ? 0x4000 /* 16kB */ : BLOCKFILE_CHUNK_SIZE}},
       m_undo_file_seq{FlatFileSeq{m_opts.blocks_dir, "rev", UNDOFILE_CHUNK_SIZE}},
-      m_interrupt{interrupt} {}
+      m_interrupt{interrupt}
+{
+    m_block_tree_db = std::make_unique<BlockTreeDB>(m_opts.block_tree_db_params);
+
+    if (m_opts.block_tree_db_params.wipe_data) {
+        m_block_tree_db->WriteReindexing(true);
+        m_blockfiles_indexed = false;
+        // If we're reindexing in prune mode, wipe away unusable block files and all undo data files
+        if (m_prune_mode) {
+            CleanupBlockRevFiles();
+        }
+    }
+}
 
 class ImportingNow
 {

src/node/chainstate.cpp

@@ -23,10 +23,7 @@
 #include <validation.h>
 
 #include <algorithm>
-#include <atomic>
 #include <cassert>
-#include <limits>
-#include <memory>
 #include <vector>
 
 using kernel::CacheSizes;
@@ -36,34 +33,8 @@ namespace node {
 // to ChainstateManager::InitializeChainstate().
 static ChainstateLoadResult CompleteChainstateInitialization(
     ChainstateManager& chainman,
-    const CacheSizes& cache_sizes,
     const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {
-    auto& pblocktree{chainman.m_blockman.m_block_tree_db};
-    // new BlockTreeDB tries to delete the existing file, which
-    // fails if it's still open from the previous loop. Close it first:
-    pblocktree.reset();
-    try {
-        pblocktree = std::make_unique<BlockTreeDB>(DBParams{
-            .path = chainman.m_options.datadir / "blocks" / "index",
-            .cache_bytes = 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);
-        chainman.m_blockman.m_blockfiles_indexed = false;
-        //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
-        if (options.prune) {
-            chainman.m_blockman.CleanupBlockRevFiles();
-        }
-    }
-
     if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
 
     // LoadBlockIndex will load m_have_pruned if we've ever removed a
@@ -155,14 +126,12 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         }
     }
 
-    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(); })) {
-            return {ChainstateLoadStatus::FAILURE, strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
-                                                             chainman.GetConsensus().SegwitHeight)};
-        };
-    }
+    auto chainstates{chainman.GetAll()};
+    if (std::any_of(chainstates.begin(), chainstates.end(),
+                    [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
+        return {ChainstateLoadStatus::FAILURE, strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
+                                                         chainman.GetConsensus().SegwitHeight)};
+    };
 
     // Now that chainstates are loaded and we're able to flush to
     // disk, rebalance the coins caches to desired levels based
@@ -208,7 +177,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
         }
     }
 
-    auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options);
+    auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options);
     if (init_status != ChainstateLoadStatus::SUCCESS) {
         return {init_status, init_error};
     }
@@ -244,7 +213,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
         // for the fully validated chainstate.
         chainman.ActiveChainstate().ClearBlockIndexCandidates();
 
-        auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options);
+        auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options);
         if (init_status != ChainstateLoadStatus::SUCCESS) {
             return {init_status, init_error};
         }

src/node/chainstate.h

@@ -22,12 +22,7 @@ namespace node {
 
 struct ChainstateLoadOptions {
     CTxMemPool* mempool{nullptr};
-    bool block_tree_db_in_memory{false};
     bool coins_db_in_memory{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};

src/node/chainstatemanager_args.cpp

@@ -49,7 +49,6 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
 
     if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};
 
-    ReadDatabaseArgs(args, opts.block_tree_db);
     ReadDatabaseArgs(args, opts.coins_db);
     ReadCoinsViewArgs(args, opts.coins_view);