From 57ba59c0cdf20de322afabe4a132ad17e483ce77 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 8 Jan 2025 14:40:04 +0100 Subject: [PATCH 1/3] refactor: Remove redundant reindex check The check for whether the block tree db has been wiped before calling NeedsRedownload() is confusing. The boolean is set in case of a reindex. It was originally introduced to guard NeedsRedownload in case of a reindex in #21009. However NeedsRedownload already returns early if the chain's tip is not loaded. Since that is the case during a reindex, the pre-check is redundant. --- src/node/chainstate.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 3ee557ae622..10b6e52c78b 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -155,14 +155,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 From 7fbb1bc44b1461f008284533f1667677e729f0c0 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 20 Jan 2025 17:40:21 +0100 Subject: [PATCH 2/3] kernel: Move block tree db open to block manager This commit is done in preparation for the next commit. Here, the block tree options are moved to the blockmanager options and the block tree is instantiated through a helper method of the BlockManager, which is removed again in the next commit. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> --- src/bitcoin-chainstate.cpp | 6 +++++- src/init.cpp | 11 ++++++++++- src/kernel/blockmanager_opts.h | 2 ++ src/kernel/chainstatemanager_opts.h | 1 - src/node/blockmanager_args.cpp | 3 +++ src/node/blockstorage.h | 2 ++ src/node/chainstate.cpp | 14 ++++---------- src/node/chainstate.h | 5 ----- src/node/chainstatemanager_args.cpp | 1 - src/test/blockmanager_tests.cpp | 8 ++++++++ src/test/util/setup_common.cpp | 15 +++++++-------- src/test/validation_chainstatemanager_tests.cpp | 5 +++++ 12 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 51c482607aa..e657f018715 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -106,6 +106,7 @@ int main(int argc, char* argv[]) }; auto notifications = std::make_unique(); + kernel::CacheSizes cache_sizes{DEFAULT_KERNEL_CACHE}; // SETUP: Chainstate auto chainparams = CChainParams::Main(); @@ -119,11 +120,14 @@ int main(int argc, char* argv[]) .chainparams = chainman_opts.chainparams, .blocks_dir = abs_datadir / "blocks", .notifications = chainman_opts.notifications, + .block_tree_db_params = DBParams{ + .path = abs_datadir / "blocks" / "index", + .cache_bytes = cache_sizes.block_tree_db, + }, }; util::SignalInterrupt interrupt; ChainstateManager chainman{interrupt, chainman_opts, blockman_opts}; - kernel::CacheSizes cache_sizes{DEFAULT_KERNEL_CACHE}; node::ChainstateLoadOptions options; auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options); if (status != node::ChainstateLoadStatus::SUCCESS) { diff --git a/src/init.cpp b/src/init.cpp index 116823c36f0..a306449dbdf 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1057,6 +1057,10 @@ bool AppInitParameterInteraction(const ArgsManager& args) .chainparams = chainman_opts_dummy.chainparams, .blocks_dir = args.GetBlocksDirPath(), .notifications = chainman_opts_dummy.notifications, + .block_tree_db_params = DBParams{ + .path = args.GetDataDirNet() / "blocks" / "index", + .cache_bytes = 0, + }, }; auto blockman_result{ApplyArgsManOptions(args, blockman_opts_dummy)}; if (!blockman_result) { @@ -1199,10 +1203,16 @@ static ChainstateLoadResult InitAndLoadChainstate( .signals = node.validation_signals.get(), }; Assert(ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction + BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, .blocks_dir = args.GetBlocksDirPath(), .notifications = chainman_opts.notifications, + .block_tree_db_params = DBParams{ + .path = args.GetDataDirNet() / "blocks" / "index", + .cache_bytes = cache_sizes.block_tree_db, + .wipe_data = do_reindex, + }, }; Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction try { @@ -1233,7 +1243,6 @@ static ChainstateLoadResult InitAndLoadChainstate( }; node::ChainstateLoadOptions options; options.mempool = Assert(node.mempool.get()); - 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); diff --git a/src/kernel/blockmanager_opts.h b/src/kernel/blockmanager_opts.h index 261ec3be582..5c7b24d7170 100644 --- a/src/kernel/blockmanager_opts.h +++ b/src/kernel/blockmanager_opts.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H #define BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H +#include #include #include @@ -27,6 +28,7 @@ struct BlockManagerOpts { bool fast_prune{false}; const fs::path blocks_dir; Notifications& notifications; + DBParams block_tree_db_params; }; } // namespace kernel diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index 1b605f3d55d..15a8fbec618 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -42,7 +42,6 @@ struct ChainstateManagerOpts { std::optional assumed_valid_block{}; //! If the tip is older than this, the node is considered to be in initial block download. std::chrono::seconds max_tip_age{DEFAULT_MAX_TIP_AGE}; - DBOptions block_tree_db{}; DBOptions coins_db{}; CoinsViewOptions coins_view{}; Notifications& notifications; diff --git a/src/node/blockmanager_args.cpp b/src/node/blockmanager_args.cpp index 0fc4e1646a1..5186b65546c 100644 --- a/src/node/blockmanager_args.cpp +++ b/src/node/blockmanager_args.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -34,6 +35,8 @@ util::Result 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 diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ac6db0558df..9db80c26c58 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -268,6 +268,8 @@ public: using Options = kernel::BlockManagerOpts; explicit BlockManager(const util::SignalInterrupt& interrupt, Options opts); + auto MakeBlockTreeDb() { return std::make_unique(m_opts.block_tree_db_params); } + auto OptsWipeBlockTreeDb() { return m_opts.block_tree_db_params.wipe_data; } const util::SignalInterrupt& m_interrupt; std::atomic m_importing{false}; diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 10b6e52c78b..4fc7a851cdf 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -36,7 +36,6 @@ 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}; @@ -44,18 +43,13 @@ static ChainstateLoadResult CompleteChainstateInitialization( // fails if it's still open from the previous loop. Close it first: pblocktree.reset(); try { - pblocktree = std::make_unique(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}); + pblocktree = chainman.m_blockman.MakeBlockTreeDb(); } catch (dbwrapper_error& err) { LogError("%s\n", err.what()); return {ChainstateLoadStatus::FAILURE, _("Error opening block database")}; } - if (options.wipe_block_tree_db) { + if (chainman.m_blockman.OptsWipeBlockTreeDb()) { 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 @@ -206,7 +200,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}; } @@ -242,7 +236,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}; } diff --git a/src/node/chainstate.h b/src/node/chainstate.h index ce414fa0434..843e8e09a66 100644 --- a/src/node/chainstate.h +++ b/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}; diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index 0ac96c55149..db36d03fd5c 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -49,7 +49,6 @@ util::Result 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); diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index c2b95dd861e..ec91f2b276e 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -33,6 +33,10 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) .chainparams = *params, .blocks_dir = m_args.GetBlocksDirPath(), .notifications = notifications, + .block_tree_db_params = DBParams{ + .path = m_args.GetDataDirNet() / "blocks" / "index", + .cache_bytes = 0, + }, }; BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts}; // simulate adding a genesis block normally @@ -140,6 +144,10 @@ BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file) .chainparams = Params(), .blocks_dir = m_args.GetBlocksDirPath(), .notifications = notifications, + .block_tree_db_params = DBParams{ + .path = m_args.GetDataDirNet() / "blocks" / "index", + .cache_bytes = 0, + }, }; BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts}; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2cf25c4a2ad..83b78d7b5ad 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -62,7 +62,6 @@ #include using namespace util::hex_literals; -using kernel::BlockTreeDB; using node::ApplyArgsManOptions; using node::BlockAssembler; using node::BlockManager; @@ -250,14 +249,16 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) .chainparams = chainman_opts.chainparams, .blocks_dir = m_args.GetBlocksDirPath(), .notifications = chainman_opts.notifications, + .block_tree_db_params = DBParams{ + .path = m_args.GetDataDirNet() / "blocks" / "index", + .cache_bytes = m_kernel_cache_sizes.block_tree_db, + .memory_only = opts.block_tree_db_in_memory, + .wipe_data = m_args.GetBoolArg("-reindex", false), + }, }; m_node.chainman = std::make_unique(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts); LOCK(m_node.chainman->GetMutex()); - m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ - .path = m_args.GetDataDirNet() / "blocks" / "index", - .cache_bytes = m_kernel_cache_sizes.block_tree_db, - .memory_only = true, - }); + m_node.chainman->m_blockman.m_block_tree_db = m_node.chainman->m_blockman.MakeBlockTreeDb(); }; m_make_chainman(); } @@ -283,9 +284,7 @@ void ChainTestingSetup::LoadVerifyActivateChainstate() auto& chainman{*Assert(m_node.chainman)}; node::ChainstateLoadOptions options; 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.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); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 6c2a825e647..bf440ca6397 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -393,6 +393,11 @@ struct SnapshotTestSetup : TestChain100Setup { .chainparams = chainman_opts.chainparams, .blocks_dir = m_args.GetBlocksDirPath(), .notifications = chainman_opts.notifications, + .block_tree_db_params = DBParams{ + .path = chainman.m_options.datadir / "blocks" / "index", + .cache_bytes = m_kernel_cache_sizes.block_tree_db, + .memory_only = m_block_tree_db_in_memory, + }, }; // For robustness, ensure the old manager is destroyed before creating a // new one. From 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 23 Sep 2024 00:21:43 +0200 Subject: [PATCH 3/3] kernel: Move block tree db open to BlockManager constructor Make the block db open RAII style by calling it in the BlockManager constructor. 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. --- src/init.cpp | 9 +++++++++ src/node/blockstorage.cpp | 15 ++++++++++++++- src/node/blockstorage.h | 2 -- src/node/chainstate.cpp | 23 ----------------------- src/test/util/setup_common.cpp | 2 -- 5 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index a306449dbdf..3fed590dfb2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1215,12 +1215,21 @@ static ChainstateLoadResult InitAndLoadChainstate( }, }; Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction + + // Creating the chainstate manager internally creates a BlockManager, opens + // the blocks tree db, and wipes existing block files in case of a reindex. + // The coinsdb is opened at a later point on LoadChainstate. try { node.chainman = std::make_unique(*Assert(node.shutdown_signal), chainman_opts, blockman_opts); + } catch (dbwrapper_error& e) { + LogError("%s", e.what()); + return {ChainstateLoadStatus::FAILURE, _("Error opening block database")}; } catch (std::exception& e) { return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()))}; } ChainstateManager& chainman = *node.chainman; + if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}}; + // This is defined and set here instead of inline in validation.h to avoid a hard // dependency between validation and index/base, since the latter is not in // libbitcoinkernel. diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 07878a56029..f26b0e73800 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -36,6 +36,7 @@ #include #include +#include #include #include #include @@ -1183,7 +1184,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(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 { diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 9db80c26c58..ac6db0558df 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -268,8 +268,6 @@ public: using Options = kernel::BlockManagerOpts; explicit BlockManager(const util::SignalInterrupt& interrupt, Options opts); - auto MakeBlockTreeDb() { return std::make_unique(m_opts.block_tree_db_params); } - auto OptsWipeBlockTreeDb() { return m_opts.block_tree_db_params.wipe_data; } const util::SignalInterrupt& m_interrupt; std::atomic m_importing{false}; diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 4fc7a851cdf..c88bd5bad23 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -23,10 +23,7 @@ #include #include -#include #include -#include -#include #include using kernel::CacheSizes; @@ -38,26 +35,6 @@ static ChainstateLoadResult CompleteChainstateInitialization( ChainstateManager& chainman, 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 = chainman.m_blockman.MakeBlockTreeDb(); - } catch (dbwrapper_error& err) { - LogError("%s\n", err.what()); - return {ChainstateLoadStatus::FAILURE, _("Error opening block database")}; - } - - if (chainman.m_blockman.OptsWipeBlockTreeDb()) { - 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 diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 83b78d7b5ad..30db579f0ba 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -257,8 +257,6 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) }, }; m_node.chainman = std::make_unique(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts); - LOCK(m_node.chainman->GetMutex()); - m_node.chainman->m_blockman.m_block_tree_db = m_node.chainman->m_blockman.MakeBlockTreeDb(); }; m_make_chainman(); }