Merge bitcoin/bitcoin#29817: kernel: De-globalize fReindex

b47bd95920 kernel: De-globalize fReindex (TheCharlatan)

Pull request description:

  fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.

  ---

  This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).

ACKs for top commit:
  achow101:
    ACK b47bd95920
  ryanofsky:
    Code review ACK b47bd95920. I rereviewed the whole PR, but the only change since last review was reverting the bugfix https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024 and make the change a pure refactoring.
  mzumsande:
    Code Review ACK b47bd95920
  stickies-v:
    ACK b47bd95920

Tree-SHA512: f7399d01f93bc0c0c7428fe95d19b9d29b4ed00a4f1deabca78fb0c4fecb434ec971e890feecb105938b5247c926850b1b7b4a4a9caa333a061e40777d0c8463

src/node/blockmanager_args.cpp

@@ -33,6 +33,8 @@ 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

src/node/blockstorage.cpp

@@ -150,7 +150,6 @@ bool BlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, s
 } // namespace kernel
 
 namespace node {
-std::atomic_bool fReindex(false);
 
 bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const
 {
@@ -552,7 +551,7 @@ bool BlockManager::LoadBlockIndexDB(const std::optional<uint256>& snapshot_block
     // Check whether we need to continue reindexing
     bool fReindexing = false;
     m_block_tree_db->ReadReindexing(fReindexing);
-    if (fReindexing) fReindex = true;
+    if (fReindexing) m_reindexing = true;
 
     return true;
 }
@@ -1183,7 +1182,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
     ImportingNow imp{chainman.m_blockman.m_importing};
 
     // -reindex
-    if (fReindex) {
+    if (chainman.m_blockman.m_reindexing) {
         int nFile = 0;
         // Map of disk positions for blocks with unknown parent (only used for reindex);
         // parent hash -> child disk position, multiple children can have the same parent.
@@ -1206,7 +1205,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
             nFile++;
         }
         WITH_LOCK(::cs_main, chainman.m_blockman.m_block_tree_db->WriteReindexing(false));
-        fReindex = false;
+        chainman.m_blockman.m_reindexing = false;
         LogPrintf("Reindexing finished\n");
         // To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
         chainman.ActiveChainstate().LoadGenesisBlock();

src/node/blockstorage.h

@@ -76,8 +76,6 @@ static const unsigned int MAX_BLOCKFILE_SIZE = 0x8000000; // 128 MiB
 /** Size of header written by WriteBlockToDisk before a serialized CBlock */
 static constexpr size_t BLOCK_SERIALIZATION_HEADER_SIZE = std::tuple_size_v<MessageStartChars> + sizeof(unsigned int);
 
-extern std::atomic_bool fReindex;
-
 // Because validation code takes pointers to the map's CBlockIndex objects, if
 // we ever switch to another associative container, we need to either use a
 // container that has stable addressing (true of all std associative
@@ -269,11 +267,19 @@ 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_interrupt{interrupt},
+          m_reindexing{m_opts.reindex} {};
 
     const util::SignalInterrupt& m_interrupt;
     std::atomic<bool> m_importing{false};
 
+    /**
+     * Tracks if a reindex is currently in progress. Set to true when a reindex
+     * is requested and false when reindexing completes. Its value is persisted
+     * in the BlockTreeDB across restarts.
+     */
+    std::atomic_bool m_reindexing;
+
     BlockMap m_block_index GUARDED_BY(cs_main);
 
     /**
@@ -353,7 +359,7 @@ public:
     [[nodiscard]] uint64_t GetPruneTarget() const { return m_opts.prune_target; }
     static constexpr auto PRUNE_TARGET_MANUAL{std::numeric_limits<uint64_t>::max()};
 
-    [[nodiscard]] bool LoadingBlocks() const { return m_importing || fReindex; }
+    [[nodiscard]] bool LoadingBlocks() const { return m_importing || m_reindexing; }
 
     /** Calculate the amount of disk space the block & undo files currently use */
     uint64_t CalculateCurrentUsage();

src/node/chainstate.cpp

@@ -60,8 +60,8 @@ static ChainstateLoadResult CompleteChainstateInitialization(
 
     // LoadBlockIndex will load m_have_pruned if we've ever removed a
     // block file from disk.
-    // Note that it also sets fReindex global based on the disk flag!
-    // From here on, fReindex and options.reindex values may be different!
+    // 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")};
@@ -84,7 +84,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     // If we're not mid-reindex (based on disk + args), add a genesis block on disk
     // (otherwise we use the one already on disk).
     // This is called again in ImportBlocks after the reindex completes.
-    if (!fReindex && !chainman.ActiveChainstate().LoadGenesisBlock()) {
+    if (!chainman.m_blockman.m_reindexing && !chainman.ActiveChainstate().LoadGenesisBlock()) {
         return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")};
     }