From 533eab7d67d78f217f74909662133086b79ea808 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 17 May 2024 11:58:58 +0200 Subject: [PATCH 1/6] bugfix: Streamline setting reindex option Reverts a bug introduced in b47bd959207e82555f07e028cc2246943d32d4c3 "kernel: De-globalize fReindex". The change leads to a GUI user being prompted to re-index on a chainstate loading failure more than once as well as the node actually not reindexing if the user chooses to. Fix this by setting the reindexing option instead of the atomic, which can be safely re-used to indicate that a reindex should be attempted. The bug specifically is caused by the chainman, and thus the blockman and its m_reindexing atomic being destroyed on every iteration of the for loop. The reindex option for ChainstateLoadOptions is currently also set in a confusing way. By using the reindex atomic, it is not obvious in which scenario it is true or false. The atomic is controlled by both the user passing the -reindex option, the user chosing to reindex if something went wrong during chainstate loading when running the gui, and by reading the reindexing flag from the block tree database in LoadBlockIndexDB. In practice this read is done through the chainstate module's CompleteChainstateInitialization's call to LoadBlockIndex. Since this is only done after the reindex option is set already, it does not have an effect on it. Make this clear by using the reindex option from the blockman opts which is only controlled by the user. --- src/init.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0aac2ac65f..343e23280a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1558,7 +1558,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) node::ChainstateLoadOptions options; options.mempool = Assert(node.mempool.get()); - options.reindex = chainman.m_blockman.m_reindexing; + options.reindex = blockman_opts.reindex; options.reindex_chainstate = fReindexChainState; options.prune = chainman.m_blockman.IsPruneMode(); options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS); @@ -1600,13 +1600,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (!fLoaded && !ShutdownRequested(node)) { // first suggest a reindex - if (!options.reindex) { + if (!blockman_opts.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) { - chainman.m_blockman.m_reindexing = true; + blockman_opts.reindex = true; if (!Assert(node.shutdown)->reset()) { LogPrintf("Internal error: failed to reset shutdown signal.\n"); } From e17255322378076edce3ef6f06cd36ca58d2e236 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 17 May 2024 18:35:45 +0200 Subject: [PATCH 2/6] validation: Remove needs_init from LoadBlockIndex It does not control any actual logic and the log message as well as the comment are obsolete, since no database initialization takes place there anymore. Log messages indicating when indexes and chainstate databases are loaded exist in other places. --- src/validation.cpp | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d398ec7406..3109045521 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4790,7 +4790,6 @@ bool ChainstateManager::LoadBlockIndex() { AssertLockHeld(cs_main); // Load block index from databases - bool needs_init = m_blockman.m_reindexing; if (!m_blockman.m_reindexing) { bool ret{m_blockman.LoadBlockIndexDB(SnapshotBlockhash())}; if (!ret) return false; @@ -4822,18 +4821,6 @@ bool ChainstateManager::LoadBlockIndex() if (pindex->IsValid(BLOCK_VALID_TREE) && (m_best_header == nullptr || CBlockIndexWorkComparator()(m_best_header, pindex))) m_best_header = pindex; } - - needs_init = m_blockman.m_block_index.empty(); - } - - if (needs_init) { - // Everything here is for *new* reindex/DBs. Thus, though - // LoadBlockIndexDB may have set m_reindexing if we shut down - // mid-reindex previously, we don't check m_reindexing and - // instead only check it prior to LoadBlockIndexDB to set - // needs_init. - - LogPrintf("Initializing databases...\n"); } return true; } From 804f09dfa116300914e2aeef05ed9710dd504e8c Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 7 Jun 2024 12:12:34 +0200 Subject: [PATCH 3/6] 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. --- src/init.cpp | 14 ++++++++------ src/kernel/blockmanager_opts.h | 1 - src/node/blockmanager_args.cpp | 2 -- src/node/blockstorage.h | 5 ++--- src/node/chainstate.cpp | 16 ++++++++-------- src/node/chainstate.h | 9 +++++++-- src/test/util/setup_common.cpp | 4 ++-- 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(*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(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 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 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(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); From 201c1a92824c71ae646d5bba9963871b1d704cc1 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 17 May 2024 12:48:09 +0200 Subject: [PATCH 4/6] indexes: Don't wipe indexes again when already reindexing Before this change continuing a reindex without the -reindex flag set would leave the block and coins db intact, but discard the data of the optional indexes. While not a bug per se, wiping the data again is wasteful, both in terms of having to write it again, and potentially leading to longer startup times. When initially running a reindex, both the block index and any further activated indexes are wiped. On an index's Init(), both the best block stored by the index and the chain's tip are null. An index's m_synced member is therefore true. This means that it will process blocks through validation events while the reindex is running. Currently, if the reindex is continued without the user re-specifying the reindex flag, the block index is preserved but further index data is wiped. This leads to the stored best block being null, but the chain tip existing. The m_synced member will be set to false. The index will not process blocks through the validation interface, but instead use the background sync once the reindex is completed. If the index is preserved (this change) after a restart its best block may potentially match the chain tip. The m_synced member will be set to true and the index can process validation events during the rest of the reindex. --- src/init.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index fe69393795..ec306e4eb9 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1641,17 +1641,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // ********************************************************* Step 8: start indexers if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { - g_txindex = std::make_unique(interfaces::MakeChain(node), cache_sizes.tx_index, false, chainman.m_blockman.m_reindexing); + g_txindex = std::make_unique(interfaces::MakeChain(node), cache_sizes.tx_index, false, do_reindex); node.indexes.emplace_back(g_txindex.get()); } for (const auto& filter_type : g_enabled_filter_types) { - InitBlockFilterIndex([&]{ return interfaces::MakeChain(node); }, filter_type, cache_sizes.filter_index, false, chainman.m_blockman.m_reindexing); + InitBlockFilterIndex([&]{ return interfaces::MakeChain(node); }, filter_type, cache_sizes.filter_index, false, do_reindex); node.indexes.emplace_back(GetBlockFilterIndex(filter_type)); } if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) { - g_coin_stats_index = std::make_unique(interfaces::MakeChain(node), /*cache_size=*/0, false, chainman.m_blockman.m_reindexing); + g_coin_stats_index = std::make_unique(interfaces::MakeChain(node), /*cache_size=*/0, false, do_reindex); node.indexes.emplace_back(g_coin_stats_index.get()); } From 1b1c6dcca0cc891bd35d29b61628c39098cd94ce Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Thu, 23 May 2024 14:11:39 +0200 Subject: [PATCH 5/6] test: Add functional test for continuing a reindex Co-authored-by: furszy --- test/functional/feature_reindex.py | 20 ++++++++++++++++++++ test/functional/test_framework/test_node.py | 4 ++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py index f0f32a61ab..835cd0c5cf 100755 --- a/test/functional/feature_reindex.py +++ b/test/functional/feature_reindex.py @@ -73,6 +73,25 @@ class ReindexTest(BitcoinTestFramework): # All blocks should be accepted and processed. assert_equal(self.nodes[0].getblockcount(), 12) + def continue_reindex_after_shutdown(self): + node = self.nodes[0] + self.generate(node, 1500) + + # Restart node with reindex and stop reindex as soon as it starts reindexing + self.log.info("Restarting node while reindexing..") + node.stop_node() + with node.busy_wait_for_debug_log([b'initload thread start']): + node.start(['-blockfilterindex', '-reindex']) + node.wait_for_rpc_connection(wait_for_import=False) + node.stop_node() + + # Start node without the reindex flag and verify it does not wipe the indexes data again + db_path = node.chain_path / 'indexes' / 'blockfilter' / 'basic' / 'db' + with node.assert_debug_log(expected_msgs=[f'Opening LevelDB in {db_path}'], unexpected_msgs=[f'Wiping LevelDB in {db_path}']): + node.start(['-blockfilterindex']) + node.wait_for_rpc_connection(wait_for_import=False) + node.stop_node() + def run_test(self): self.reindex(False) self.reindex(True) @@ -80,6 +99,7 @@ class ReindexTest(BitcoinTestFramework): self.reindex(True) self.out_of_order() + self.continue_reindex_after_shutdown() if __name__ == '__main__': diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index d228bd8991..1a2448d8cd 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -241,7 +241,7 @@ class TestNode(): if self.start_perf: self._start_perf() - def wait_for_rpc_connection(self): + def wait_for_rpc_connection(self, *, wait_for_import=True): """Sets up an RPC connection to the bitcoind process. Returns False if unable to connect.""" # Poll at a rate of four times per second poll_per_s = 4 @@ -263,7 +263,7 @@ class TestNode(): ) rpc.getblockcount() # If the call to getblockcount() succeeds then the RPC connection is up - if self.version_is_at_least(190000): + if self.version_is_at_least(190000) and wait_for_import: # getmempoolinfo.loaded is available since commit # bb8ae2c (version 0.19.0) self.wait_until(lambda: rpc.getmempoolinfo()['loaded']) From f68cba29b3be0dec7877022b18a193a3b78c1099 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 7 Jun 2024 13:05:06 +0200 Subject: [PATCH 6/6] blockman: Replace m_reindexing with m_blockfiles_indexed This is a just a mechanical change, renaming and inverting the meaning of the indexing variable. "m_blockfiles_indexed" is a more straightforward name for this variable because this variable just indicates whether or not /blocks/blk?????.dat files have been indexed in the /blocks/index LevelDB database. The name "m_reindexing" was more confusing, it could be true even if -reindex was not specified, and false when it was specified. Also, the previous name unnecessarily required thinking about the whole reindexing process just to understand simple checks in validation code about whether blocks were indexed. The motivation for this change is to follow up on previous commits, moving away from having multiple variables called "reindex" internally, and instead naming variables individually after what they do and represent. --- src/bitcoin-chainstate.cpp | 2 +- src/init.cpp | 2 +- src/node/blockstorage.cpp | 6 +++--- src/node/blockstorage.h | 11 ++++++----- src/node/chainstate.cpp | 6 +++--- src/validation.cpp | 18 +++++++++--------- 6 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 4d2a6f0c2a..ca4434a882 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -151,7 +151,7 @@ int main(int argc, char* argv[]) { LOCK(chainman.GetMutex()); std::cout - << "\t" << "Reindexing: " << std::boolalpha << chainman.m_blockman.m_reindexing.load() << std::noboolalpha << std::endl + << "\t" << "Blockfiles Indexed: " << std::boolalpha << chainman.m_blockman.m_blockfiles_indexed.load() << std::noboolalpha << std::endl << "\t" << "Snapshot Active: " << std::boolalpha << chainman.IsSnapshotActive() << std::noboolalpha << std::endl << "\t" << "Active Height: " << chainman.ActiveHeight() << std::endl << "\t" << "Active IBD: " << std::boolalpha << chainman.IsInitialBlockDownload() << std::noboolalpha << std::endl; diff --git a/src/init.cpp b/src/init.cpp index ec306e4eb9..253c8b7582 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1670,7 +1670,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // if pruning, perform the initial blockstore prune // after any wallet rescanning has taken place. if (chainman.m_blockman.IsPruneMode()) { - if (!chainman.m_blockman.m_reindexing) { + if (chainman.m_blockman.m_blockfiles_indexed) { LOCK(cs_main); for (Chainstate* chainstate : chainman.GetAll()) { uiInterface.InitMessage(_("Pruning blockstore…").translated); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 4067ccee51..fb62e78138 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -551,7 +551,7 @@ bool BlockManager::LoadBlockIndexDB(const std::optional& snapshot_block // Check whether we need to continue reindexing bool fReindexing = false; m_block_tree_db->ReadReindexing(fReindexing); - if (fReindexing) m_reindexing = true; + if (fReindexing) m_blockfiles_indexed = false; return true; } @@ -1182,7 +1182,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile ImportingNow imp{chainman.m_blockman.m_importing}; // -reindex - if (chainman.m_blockman.m_reindexing) { + if (!chainman.m_blockman.m_blockfiles_indexed) { 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. @@ -1205,7 +1205,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile nFile++; } WITH_LOCK(::cs_main, chainman.m_blockman.m_block_tree_db->WriteReindexing(false)); - chainman.m_blockman.m_reindexing = false; + chainman.m_blockman.m_blockfiles_indexed = true; 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(); diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index bc09d102e2..108a08a72b 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -273,11 +273,12 @@ public: std::atomic 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. + * Whether all blockfiles have been added to the block tree database. + * Normally true, but set to false when a reindex is requested and the + * database is wiped. The value is persisted in the database across restarts + * and will be false until reindexing completes. */ - std::atomic_bool m_reindexing{false}; + std::atomic_bool m_blockfiles_indexed{true}; BlockMap m_block_index GUARDED_BY(cs_main); @@ -358,7 +359,7 @@ public: [[nodiscard]] uint64_t GetPruneTarget() const { return m_opts.prune_target; } static constexpr auto PRUNE_TARGET_MANUAL{std::numeric_limits::max()}; - [[nodiscard]] bool LoadingBlocks() const { return m_importing || m_reindexing; } + [[nodiscard]] bool LoadingBlocks() const { return m_importing || !m_blockfiles_indexed; } /** Calculate the amount of disk space the block & undo files currently use */ uint64_t CalculateCurrentUsage(); diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 0e1c2177fc..d7e6176be1 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -50,7 +50,7 @@ static ChainstateLoadResult CompleteChainstateInitialization( if (options.wipe_block_tree_db) { pblocktree->WriteReindexing(true); - chainman.m_blockman.m_reindexing = 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(); @@ -61,7 +61,7 @@ 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! + // Note that it also sets m_blockfiles_indexed based on the disk flag! 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 (!chainman.m_blockman.m_reindexing && !chainman.ActiveChainstate().LoadGenesisBlock()) { + if (chainman.m_blockman.m_blockfiles_indexed && !chainman.ActiveChainstate().LoadGenesisBlock()) { return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")}; } diff --git a/src/validation.cpp b/src/validation.cpp index 3109045521..90d04418fe 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2641,7 +2641,7 @@ bool Chainstate::FlushStateToDisk( CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(); LOCK(m_blockman.cs_LastBlockFile); - if (m_blockman.IsPruneMode() && (m_blockman.m_check_for_pruning || nManualPruneHeight > 0) && !m_chainman.m_blockman.m_reindexing) { + if (m_blockman.IsPruneMode() && (m_blockman.m_check_for_pruning || nManualPruneHeight > 0) && m_chainman.m_blockman.m_blockfiles_indexed) { // make sure we don't prune above any of the prune locks bestblocks // pruning is height-based int last_prune{m_chain.Height()}; // last height we can prune @@ -3254,10 +3254,10 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* return true; } -static SynchronizationState GetSynchronizationState(bool init, bool reindexing) +static SynchronizationState GetSynchronizationState(bool init, bool blockfiles_indexed) { if (!init) return SynchronizationState::POST_INIT; - if (reindexing) return SynchronizationState::INIT_REINDEX; + if (!blockfiles_indexed) return SynchronizationState::INIT_REINDEX; return SynchronizationState::INIT_DOWNLOAD; } @@ -3279,7 +3279,7 @@ static bool NotifyHeaderTip(ChainstateManager& chainman) LOCKS_EXCLUDED(cs_main) } // Send block tip changed notifications without cs_main if (fNotify) { - chainman.GetNotifications().headerTip(GetSynchronizationState(fInitialBlockDownload, chainman.m_blockman.m_reindexing), pindexHeader->nHeight, pindexHeader->nTime, false); + chainman.GetNotifications().headerTip(GetSynchronizationState(fInitialBlockDownload, chainman.m_blockman.m_blockfiles_indexed), pindexHeader->nHeight, pindexHeader->nTime, false); } return fNotify; } @@ -3398,7 +3398,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< } // Always notify the UI if a new block tip was connected - if (kernel::IsInterrupted(m_chainman.GetNotifications().blockTip(GetSynchronizationState(still_in_ibd, m_chainman.m_blockman.m_reindexing), *pindexNewTip))) { + if (kernel::IsInterrupted(m_chainman.GetNotifications().blockTip(GetSynchronizationState(still_in_ibd, m_chainman.m_blockman.m_blockfiles_indexed), *pindexNewTip))) { // Just breaking and returning success for now. This could // be changed to bubble up the kernel::Interrupted value to // the caller so the caller could distinguish between @@ -3624,7 +3624,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // parameter indicating the source of the tip change so hooks can // distinguish user-initiated invalidateblock changes from other // changes. - (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_reindexing), *to_mark_failed->pprev); + (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_blockfiles_indexed), *to_mark_failed->pprev); } return true; } @@ -4263,7 +4263,7 @@ void ChainstateManager::ReportHeadersPresync(const arith_uint256& work, int64_t m_last_presync_update = now; } bool initial_download = IsInitialBlockDownload(); - GetNotifications().headerTip(GetSynchronizationState(initial_download, m_blockman.m_reindexing), height, timestamp, /*presync=*/true); + GetNotifications().headerTip(GetSynchronizationState(initial_download, m_blockman.m_blockfiles_indexed), height, timestamp, /*presync=*/true); if (initial_download) { int64_t blocks_left{(NodeClock::now() - NodeSeconds{std::chrono::seconds{timestamp}}) / GetConsensus().PowTargetSpacing()}; blocks_left = std::max(0, blocks_left); @@ -4790,7 +4790,7 @@ bool ChainstateManager::LoadBlockIndex() { AssertLockHeld(cs_main); // Load block index from databases - if (!m_blockman.m_reindexing) { + if (m_blockman.m_blockfiles_indexed) { bool ret{m_blockman.LoadBlockIndexDB(SnapshotBlockhash())}; if (!ret) return false; @@ -4961,7 +4961,7 @@ void ChainstateManager::LoadExternalBlockFile( } } - if (m_blockman.IsPruneMode() && !m_blockman.m_reindexing && pblock) { + if (m_blockman.IsPruneMode() && m_blockman.m_blockfiles_indexed && pblock) { // must update the tip for pruning to work while importing with -loadblock. // this is a tradeoff to conserve disk space at the expense of time // spent updating the tip to be able to prune.