From 3b91d4b9947adbec74721f538e46c712db22587c Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Wed, 1 Dec 2021 18:16:29 -0500
Subject: [PATCH] refactor: Reduce number of LoadChainstate parameters

---
 src/bitcoin-chainstate.cpp     | 25 +++++--------
 src/init.cpp                   | 43 ++++++++++------------
 src/node/chainstate.cpp        | 65 ++++++++++++++--------------------
 src/node/chainstate.h          | 37 ++++++++++---------
 src/test/util/setup_common.cpp | 29 ++++++---------
 5 files changed, 86 insertions(+), 113 deletions(-)

diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index d53e917aba6..2981fc6c2f4 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -18,6 +18,7 @@
 #include <consensus/validation.h>
 #include <core_io.h>
 #include <node/blockstorage.h>
+#include <node/caches.h>
 #include <node/chainstate.h>
 #include <scheduler.h>
 #include <script/sigcache.h>
@@ -83,26 +84,18 @@ int main(int argc, char* argv[])
     };
     ChainstateManager chainman{chainman_opts};
 
-    auto rv = node::LoadChainstate(false,
-                                   std::ref(chainman),
-                                   nullptr,
-                                   false,
-                                   false,
-                                   2 << 20,
-                                   2 << 22,
-                                   (450 << 20) - (2 << 20) - (2 << 22),
-                                   false,
-                                   false,
-                                   []() { return false; });
+    node::CacheSizes cache_sizes;
+    cache_sizes.block_tree_db = 2 << 20;
+    cache_sizes.coins_db = 2 << 22;
+    cache_sizes.coins = (450 << 20) - (2 << 20) - (2 << 22);
+    node::ChainstateLoadOptions options;
+    options.check_interrupt = [] { return false; };
+    auto rv = node::LoadChainstate(chainman, cache_sizes, options);
     if (rv.has_value()) {
         std::cerr << "Failed to load Chain state from your datadir." << std::endl;
         goto epilogue;
     } else {
-        auto maybe_verify_error = node::VerifyLoadedChainstate(std::ref(chainman),
-                                                               false,
-                                                               false,
-                                                               DEFAULT_CHECKBLOCKS,
-                                                               DEFAULT_CHECKLEVEL);
+        auto maybe_verify_error = node::VerifyLoadedChainstate(chainman, options);
         if (maybe_verify_error.has_value()) {
             std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl;
             goto epilogue;
diff --git a/src/init.cpp b/src/init.cpp
index 5e9df42881d..83067a8bff3 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1452,29 +1452,27 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         node.chainman = std::make_unique<ChainstateManager>(chainman_opts);
         ChainstateManager& chainman = *node.chainman;
 
-        const bool fReset = fReindex;
         bilingual_str strLoadError;
 
+        node::ChainstateLoadOptions options;
+        options.mempool = Assert(node.mempool.get());
+        options.reindex = node::fReindex;
+        options.reindex_chainstate = fReindexChainState;
+        options.prune = node::fPruneMode;
+        options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
+        options.check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL);
+        options.check_interrupt = ShutdownRequested;
+        options.coins_error_cb = [] {
+            uiInterface.ThreadSafeMessageBox(
+                _("Error reading from database, shutting down."),
+                "", CClientUIInterface::MSG_ERROR);
+        };
+
         uiInterface.InitMessage(_("Loading block index…").translated);
         const int64_t load_block_index_start_time = GetTimeMillis();
         std::optional<ChainstateLoadingError> maybe_load_error;
         try {
-            maybe_load_error = LoadChainstate(fReset,
-                                              chainman,
-                                              Assert(node.mempool.get()),
-                                              fPruneMode,
-                                              fReindexChainState,
-                                              cache_sizes.block_tree_db,
-                                              cache_sizes.coins_db,
-                                              cache_sizes.coins,
-                                              /*block_tree_db_in_memory=*/false,
-                                              /*coins_db_in_memory=*/false,
-                                              /*shutdown_requested=*/ShutdownRequested,
-                                              /*coins_error_cb=*/[]() {
-                                                  uiInterface.ThreadSafeMessageBox(
-                                                                                   _("Error reading from database, shutting down."),
-                                                                                   "", CClientUIInterface::MSG_ERROR);
-                                              });
+            maybe_load_error = LoadChainstate(chainman, cache_sizes, options);
         } catch (const std::exception& e) {
             LogPrintf("%s\n", e.what());
             maybe_load_error = ChainstateLoadingError::ERROR_GENERIC_BLOCKDB_OPEN_FAILED;
@@ -1518,16 +1516,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             std::optional<ChainstateLoadVerifyError> maybe_verify_error;
             try {
                 uiInterface.InitMessage(_("Verifying blocks…").translated);
-                auto check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
-                if (chainman.m_blockman.m_have_pruned && check_blocks > MIN_BLOCKS_TO_KEEP) {
+                if (chainman.m_blockman.m_have_pruned && options.check_blocks > MIN_BLOCKS_TO_KEEP) {
                     LogPrintfCategory(BCLog::PRUNE, "pruned datadir may not have more than %d blocks; only checking available blocks\n",
                                       MIN_BLOCKS_TO_KEEP);
                 }
-                maybe_verify_error = VerifyLoadedChainstate(chainman,
-                                                            fReset,
-                                                            fReindexChainState,
-                                                            check_blocks,
-                                                            args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL));
+                maybe_verify_error = VerifyLoadedChainstate(chainman, options);
             } catch (const std::exception& e) {
                 LogPrintf("%s\n", e.what());
                 maybe_verify_error = ChainstateLoadVerifyError::ERROR_GENERIC_FAILURE;
@@ -1554,7 +1547,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         if (!fLoaded && !ShutdownRequested()) {
             // first suggest a reindex
-            if (!fReset) {
+            if (!options.reindex) {
                 bool fRet = uiInterface.ThreadSafeQuestion(
                     strLoadError + Untranslated(".\n\n") + _("Do you want to rebuild the block database now?"),
                     strLoadError.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 60a60f86656..47248953d2d 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -8,6 +8,7 @@
 #include <coins.h>
 #include <consensus/params.h>
 #include <node/blockstorage.h>
+#include <node/caches.h>
 #include <sync.h>
 #include <threadsafety.h>
 #include <txdb.h>
@@ -22,49 +23,40 @@
 #include <vector>
 
 namespace node {
-std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
-                                                     ChainstateManager& chainman,
-                                                     CTxMemPool* mempool,
-                                                     bool fPruneMode,
-                                                     bool fReindexChainState,
-                                                     int64_t nBlockTreeDBCache,
-                                                     int64_t nCoinDBCache,
-                                                     int64_t nCoinCacheUsage,
-                                                     bool block_tree_db_in_memory,
-                                                     bool coins_db_in_memory,
-                                                     std::function<bool()> shutdown_requested,
-                                                     std::function<void()> coins_error_cb)
+std::optional<ChainstateLoadingError> LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,
+                                                     const ChainstateLoadOptions& options)
 {
     auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull();
+        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
     };
 
     LOCK(cs_main);
-    chainman.InitializeChainstate(mempool);
-    chainman.m_total_coinstip_cache = nCoinCacheUsage;
-    chainman.m_total_coinsdb_cache = nCoinDBCache;
+    chainman.InitializeChainstate(options.mempool);
+    chainman.m_total_coinstip_cache = cache_sizes.coins;
+    chainman.m_total_coinsdb_cache = cache_sizes.coins_db;
 
     auto& pblocktree{chainman.m_blockman.m_block_tree_db};
     // new CBlockTreeDB tries to delete the existing file, which
     // fails if it's still open from the previous loop. Close it first:
     pblocktree.reset();
-    pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, block_tree_db_in_memory, fReset));
+    pblocktree.reset(new CBlockTreeDB(cache_sizes.block_tree_db, options.block_tree_db_in_memory, options.reindex));
 
-    if (fReset) {
+    if (options.reindex) {
         pblocktree->WriteReindexing(true);
         //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
-        if (fPruneMode)
+        if (options.prune) {
             CleanupBlockRevFiles();
+        }
     }
 
-    if (shutdown_requested && shutdown_requested()) return ChainstateLoadingError::SHUTDOWN_PROBED;
+    if (options.check_interrupt && options.check_interrupt()) return ChainstateLoadingError::SHUTDOWN_PROBED;
 
     // LoadBlockIndex will load m_have_pruned if we've ever removed a
     // block file from disk.
-    // Note that it also sets fReindex based on the disk flag!
-    // From here on out fReindex and fReset mean something different!
+    // Note that it also sets fReindex global based on the disk flag!
+    // From here on, fReindex and options.reindex values may be different!
     if (!chainman.LoadBlockIndex()) {
-        if (shutdown_requested && shutdown_requested()) return ChainstateLoadingError::SHUTDOWN_PROBED;
+        if (options.check_interrupt && options.check_interrupt()) return ChainstateLoadingError::SHUTDOWN_PROBED;
         return ChainstateLoadingError::ERROR_LOADING_BLOCK_DB;
     }
 
@@ -75,7 +67,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
 
     // Check for changed -prune state.  What we are concerned about is a user who has pruned blocks
     // in the past, but is now trying to run unpruned.
-    if (chainman.m_blockman.m_have_pruned && !fPruneMode) {
+    if (chainman.m_blockman.m_have_pruned && !options.prune) {
         return ChainstateLoadingError::ERROR_PRUNED_NEEDS_REINDEX;
     }
 
@@ -92,12 +84,12 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
 
     for (CChainState* chainstate : chainman.GetAll()) {
         chainstate->InitCoinsDB(
-            /*cache_size_bytes=*/nCoinDBCache,
-            /*in_memory=*/coins_db_in_memory,
-            /*should_wipe=*/fReset || fReindexChainState);
+            /*cache_size_bytes=*/cache_sizes.coins_db,
+            /*in_memory=*/options.coins_db_in_memory,
+            /*should_wipe=*/options.reindex || options.reindex_chainstate);
 
-        if (coins_error_cb) {
-            chainstate->CoinsErrorCatcher().AddReadErrCallback(coins_error_cb);
+        if (options.coins_error_cb) {
+            chainstate->CoinsErrorCatcher().AddReadErrCallback(options.coins_error_cb);
         }
 
         // Refuse to load unsupported database format.
@@ -112,7 +104,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
         }
 
         // The on-disk coinsdb is now in a good state, create the cache
-        chainstate->InitCoinsCache(nCoinCacheUsage);
+        chainstate->InitCoinsCache(cache_sizes.coins);
         assert(chainstate->CanFlushToDisk());
 
         if (!is_coinsview_empty(chainstate)) {
@@ -124,7 +116,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
         }
     }
 
-    if (!fReset) {
+    if (!options.reindex) {
         auto chainstates{chainman.GetAll()};
         if (std::any_of(chainstates.begin(), chainstates.end(),
                         [](const CChainState* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
@@ -136,13 +128,10 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
 }
 
 std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManager& chainman,
-                                                                bool fReset,
-                                                                bool fReindexChainState,
-                                                                int check_blocks,
-                                                                int check_level)
+                                                                const ChainstateLoadOptions& options)
 {
     auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull();
+        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
     };
 
     LOCK(cs_main);
@@ -156,8 +145,8 @@ std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManage
 
             if (!CVerifyDB().VerifyDB(
                     *chainstate, chainman.GetConsensus(), chainstate->CoinsDB(),
-                    check_level,
-                    check_blocks)) {
+                    options.check_level,
+                    options.check_blocks)) {
                 return ChainstateLoadVerifyError::ERROR_CORRUPTED_BLOCK_DB;
             }
         }
diff --git a/src/node/chainstate.h b/src/node/chainstate.h
index 5c495da229d..4851ef1ae67 100644
--- a/src/node/chainstate.h
+++ b/src/node/chainstate.h
@@ -5,6 +5,8 @@
 #ifndef BITCOIN_NODE_CHAINSTATE_H
 #define BITCOIN_NODE_CHAINSTATE_H
 
+#include <validation.h>
+
 #include <cstdint>
 #include <functional>
 #include <optional>
@@ -13,6 +15,22 @@ class ChainstateManager;
 class CTxMemPool;
 
 namespace node {
+
+struct CacheSizes;
+
+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};
+    int64_t check_blocks{DEFAULT_CHECKBLOCKS};
+    int64_t check_level{DEFAULT_CHECKLEVEL};
+    std::function<bool()> check_interrupt;
+    std::function<void()> coins_error_cb;
+};
+
 enum class ChainstateLoadingError {
     ERROR_LOADING_BLOCK_DB,
     ERROR_BAD_GENESIS_BLOCK,
@@ -52,18 +70,8 @@ enum class ChainstateLoadingError {
  *  - else
  *      - Success!
  */
-std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
-                                                     ChainstateManager& chainman,
-                                                     CTxMemPool* mempool,
-                                                     bool fPruneMode,
-                                                     bool fReindexChainState,
-                                                     int64_t nBlockTreeDBCache,
-                                                     int64_t nCoinDBCache,
-                                                     int64_t nCoinCacheUsage,
-                                                     bool block_tree_db_in_memory,
-                                                     bool coins_db_in_memory,
-                                                     std::function<bool()> shutdown_requested = nullptr,
-                                                     std::function<void()> coins_error_cb = nullptr);
+std::optional<ChainstateLoadingError> LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,
+                                                     const ChainstateLoadOptions& options);
 
 enum class ChainstateLoadVerifyError {
     ERROR_BLOCK_FROM_FUTURE,
@@ -72,10 +80,7 @@ enum class ChainstateLoadVerifyError {
 };
 
 std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManager& chainman,
-                                                                bool fReset,
-                                                                bool fReindexChainState,
-                                                                int check_blocks,
-                                                                int check_level);
+                                                                const ChainstateLoadOptions& options);
 } // namespace node
 
 #endif // BITCOIN_NODE_CHAINSTATE_H
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 0fba9258f15..db5937121be 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -54,8 +54,6 @@
 
 using node::BlockAssembler;
 using node::CalculateCacheSizes;
-using node::fPruneMode;
-using node::fReindex;
 using node::LoadChainstate;
 using node::NodeContext;
 using node::RegenerateCommitments;
@@ -218,24 +216,19 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
     // instead of unit tests, but for now we need these here.
     RegisterAllCoreRPCCommands(tableRPC);
 
-    auto maybe_load_error = LoadChainstate(fReindex.load(),
-                                           *Assert(m_node.chainman.get()),
-                                           Assert(m_node.mempool.get()),
-                                           fPruneMode,
-                                           m_args.GetBoolArg("-reindex-chainstate", false),
-                                           m_cache_sizes.block_tree_db,
-                                           m_cache_sizes.coins_db,
-                                           m_cache_sizes.coins,
-                                           /*block_tree_db_in_memory=*/true,
-                                           /*coins_db_in_memory=*/true);
+    node::ChainstateLoadOptions options;
+    options.mempool = Assert(m_node.mempool.get());
+    options.block_tree_db_in_memory = true;
+    options.coins_db_in_memory = true;
+    options.reindex = node::fReindex;
+    options.reindex_chainstate = m_args.GetBoolArg("-reindex-chainstate", false);
+    options.prune = node::fPruneMode;
+    options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
+    options.check_level = m_args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL);
+    auto maybe_load_error = LoadChainstate(*Assert(m_node.chainman), m_cache_sizes, options);
     assert(!maybe_load_error.has_value());
 
-    auto maybe_verify_error = VerifyLoadedChainstate(
-        *Assert(m_node.chainman),
-        fReindex.load(),
-        m_args.GetBoolArg("-reindex-chainstate", false),
-        m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS),
-        m_args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL));
+    auto maybe_verify_error = VerifyLoadedChainstate(*Assert(m_node.chainman), options);
     assert(!maybe_verify_error.has_value());
 
     BlockValidationState state;