kernel: De-globalize fReindex #29817

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.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

For detailed information about the code coverage, see the test coverage report.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
• #29678 (Bugfix: Correct first-run free space checks by luke-jr)
• #28052 (blockstorage: XOR blocksdir *.dat files by maflcko)
• #26022 (Add util::ResultPtr class by ryanofsky)
• #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
• #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23504545411}

Approach ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26 and code LGTM.

Some thoughts I have from reviewing this PR, but that are orthogonal to it:

It seems like we have 4 places where we store information about reindexing being required or being in progress:

1. BlockTreeDB (through WriteReindexing() and ReadReindexing())
2. BlockManager::m_reindex (previously fReindex)
3. BlockManagerOpts::reindex
4. ChainstateLoadOptions::reindex

1. and 2. are updated based on reindex progress, and should stay in sync with each other most of the time. 3. and 4. are set up to one time after initialization, and then never updated again, and thus will stay in sync with each other but may fall out of sync with 1. and 2.. Perhaps this is intentional?

2. (BlockManager::m_reindex) seems to be indicate both whether a reindex is requested or in progress, whereas 1. (BlockTreeDB) is exclusively used to indicate if the reindex is in progress, and 3. and 4. (BlockManagerOpts::reindex and ChainstateLoadOptions::reindex) is exclusively used to indicate if the reindex is requested.

I wonder if this could and should be cleaned up? For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests:

<details> <summary>git diff on d9bcbbf229</summary>

diff --git a/src/init.cpp b/src/init.cpp
index 0550eec970..c9183b5c7b 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1560,7 +1560,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         node::ChainstateLoadOptions options;
         options.mempool = Assert(node.mempool.get());
-        options.reindex = chainman.m_blockman.m_reindex;
         options.reindex_chainstate = fReindexChainState;
         options.prune = chainman.m_blockman.IsPruneMode();
         options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
@@ -1602,7 +1601,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         if (!fLoaded && !ShutdownRequested(node)) {
             // first suggest a reindex
-            if (!options.reindex) {
+            if (!chainman.m_blockman.m_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.",
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 3ee712ab7e..70d4c18c1d 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -45,10 +45,10 @@ 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 = chainman.m_blockman.m_reindex,
         .options = chainman.m_options.block_tree_db});
 
-    if (options.reindex) {
+    if (chainman.m_blockman.m_reindex) {
         pblocktree->WriteReindexing(true);
         //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
         if (options.prune) {
@@ -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 chainman.m_blockman.m_reindex || options.reindex_chainstate || 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=*/chainman.m_blockman.m_reindex || options.reindex_chainstate);
 
         if (options.coins_error_cb) {
             chainstate->CoinsErrorCatcher().AddReadErrCallback(options.coins_error_cb);
@@ -142,7 +142,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         }
     }
 
-    if (!options.reindex) {
+    if (!chainman.m_blockman.m_reindex) {
         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 && (chainman.m_blockman.m_reindex || options.reindex_chainstate)) {
         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 chainman.m_blockman.m_reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
     };
 
     LOCK(cs_main);
diff --git a/src/node/chainstate.h b/src/node/chainstate.h
index a6e9a0331b..ebcff861ce 100644
--- a/src/node/chainstate.h
+++ b/src/node/chainstate.h
@@ -22,7 +22,6 @@ 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};
     bool prune{false};
     //! Setting require_full_verification to true will require all checks at
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 3140f43dcf..1b960ad266 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -278,7 +278,6 @@ 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_reindex;
     options.reindex_chainstate = m_args.GetBoolArg("-reindex-chainstate", false);
     options.prune = chainman.m_blockman.IsPruneMode();
     options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);

</details>

Most of these concerns are quite orthogonal to this PR, since it's the same situation on master with fReindex, so we don't need to discuss this further here but I thought I'd raise it.

Code review ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26, but I think reindex /m_reindex naming is a little confusing so I left some suggestions below to clean it up.

I also really like stickies suggestion from #29817#pullrequestreview-2001088868 to delete the ChainstateLoadOptions::reindex variable and think it would be great to add it as a second commit. If you do add it, I also think it would also be good idea to add a third commit renaming ChainstateLoadOptions::reindex_chainstate ChainstateLoadOptions::should_wipe as a small scripted diff so that name is more descriptive and internally consistent.

Stickies other suggestion #29817 (review) to avoid duplicating the default value also seems good.

Re #29817#pullrequestreview-2006662114 and #29817#pullrequestreview-2001088868

For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests ... I also really like stickies suggestion from #29817#pullrequestreview-2001088868 to delete the ChainstateLoadOptions::reindex variable and think it would be great to add it as a second commi

I thought abour this initially as well, but the way I read the code, this is actually a change in behaviour. The persisted reindexing value is read from the database with a call to LoadBlockIndex. Further down, this would lead to the coins being wiped again after a restart. Meaning if the user restarts without setting reindex, but with reindexing set in the database, it would not wipe the block tree db, but would wipe the coins, while with the current behaviour it would not wipe anything and continue from where it left off.

Updated d9bcbbf2293ef427b37eefca30f074be5eeeca26 -> 9b2c9c2fce32fe858d1361e863c72108a384a5c8 (noGlobalReindex_0 -> noGlobalReindex_1, compare)

• Addressed @stickies-v's comment, removed double definition of a default option value.
• Addressed @ryanofsky's comment, changed new reindexing argument in GetSynchronizationState to a bool.
• Addressed @ryanofsky's comment, renamed m_reindex to m_reindexing and added a comment describing its purpose.

Code review ACK 9b2c9c2fce32fe858d1361e863c72108a384a5c8. This looks good in its current form. I just had one suggestion below (not important) and some questions about possible minor pre-existing bugs.

re: #29817 (comment)

I thought abour this initially as well, but the way I read the code, this is actually a change in behaviour [...]

Thanks for the explanation, that makes sense.

Updated 9b2c9c2fce32fe858d1361e863c72108a384a5c8 -> b0594f6cf157e9bd71b2062cdf0aa65936fd2282 (noGlobalReindex_1 -> noGlobalReindex_2, compare)

• Addressed @ryanofsky's comment and comment, using blockman_opts.reindex instead of chainman.m_blockman.m_reindexing.

Rebased b0594f6cf157e9bd71b2062cdf0aa65936fd2282 -> 46b9ec10dd238c5c1191bcdfcf06d1361cde15f1 (noGlobalReindex_2 -> noGlobalReindex_3, compare)

• Fixed conflict with #30083

Updated 46b9ec10dd238c5c1191bcdfcf06d1361cde15f1 -> b47bd959207e82555f07e028cc2246943d32d4c3 (noGlobalReindex_3 -> noGlobalReindex_4, compare)

• Reverted previous change in init code that would no longer wipe the indexes when restarting in the middle of a reindex without the -reindex flag.

Code review ACK b47bd959207e82555f07e028cc2246943d32d4c3. I rereviewed the whole PR, but the only change since last review was reverting the bugfix #29817 (review) and make the change a pure refactoring.

I think this change makes the code a lot less confusing by using separate names for the -reindex option and reindexing state, and could avoid bugs like the one mentioned in the future.

Code Review ACK b47bd959207e82555f07e028cc2246943d32d4c3

ACK b47bd959207e82555f07e028cc2246943d32d4c3

ACK b47bd959207e82555f07e028cc2246943d32d4c3