From 781c01f58066d375c14b1a717160f51c2f2ebe20 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 25 Sep 2024 13:10:02 +0200 Subject: [PATCH 1/3] init: Check mempool arguments in AppInitParameterInteractions This makes the handling more consistent and reports errors sooner. --- src/init.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 2a301e114ee..9a687c6fe39 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1072,6 +1072,13 @@ bool AppInitParameterInteraction(const ArgsManager& args) if (!blockman_result) { return InitError(util::ErrorString(blockman_result)); } + CTxMemPool::Options mempool_opts{ + .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, + }; + auto mempool_result{ApplyArgsManOptions(args, chainparams, mempool_opts)}; + if (!mempool_result) { + return InitError(util::ErrorString(mempool_result)); + } } return true; @@ -1543,10 +1550,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, .signals = &validation_signals, }; - auto result{ApplyArgsManOptions(args, chainparams, mempool_opts)}; - if (!result) { - return InitError(util::ErrorString(result)); - } + Assert(ApplyArgsManOptions(args, chainparams, mempool_opts)); // no error can happen, already checked in AppInitParameterInteraction bool do_reindex{args.GetBoolArg("-reindex", false)}; const bool do_reindex_chainstate{args.GetBoolArg("-reindex-chainstate", false)}; From c1d8870ea4155c2766d30d38fc5b1afc63dcd364 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 25 Sep 2024 13:24:10 +0200 Subject: [PATCH 2/3] refactor: Move most of init retry for loop to a function This makes it clearer which state is being mutated by the function and facilitates getting rid of the for loop in the following commit. Move creation of the required options into the function too, such that the function takes fewer arguments and is more self-contained. Co-Authored-By: Ryan Ofsky --- src/init.cpp | 209 +++++++++++++++++++++++++++------------------------ 1 file changed, 112 insertions(+), 97 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 9a687c6fe39..05a8eabc659 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -123,17 +123,19 @@ using node::ApplyArgsManOptions; using node::BlockManager; using node::CacheSizes; using node::CalculateCacheSizes; +using node::ChainstateLoadResult; +using node::ChainstateLoadStatus; using node::DEFAULT_PERSIST_MEMPOOL; using node::DEFAULT_PRINT_MODIFIED_FEE; using node::DEFAULT_STOPATHEIGHT; using node::DumpMempool; -using node::LoadMempool; +using node::ImportBlocks; using node::KernelNotifications; using node::LoadChainstate; +using node::LoadMempool; using node::MempoolPath; using node::NodeContext; using node::ShouldPersistMempool; -using node::ImportBlocks; using node::VerifyLoadedChainstate; using util::Join; using util::ReplaceAll; @@ -1183,6 +1185,104 @@ bool CheckHostPortOptions(const ArgsManager& args) { return true; } +// A GUI user may opt to retry once if there is a failure during chainstate initialization. +// The function therefore has to support re-entry. +static ChainstateLoadResult InitAndLoadChainstate( + NodeContext& node, + bool do_reindex, + const bool do_reindex_chainstate, + CacheSizes& cache_sizes, + const ArgsManager& args) +{ + const CChainParams& chainparams = Params(); + CTxMemPool::Options mempool_opts{ + .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, + .signals = node.validation_signals.get(), + }; + Assert(ApplyArgsManOptions(args, chainparams, mempool_opts)); // no error can happen, already checked in AppInitParameterInteraction + bilingual_str mempool_error; + node.mempool = std::make_unique(mempool_opts, mempool_error); + if (!mempool_error.empty()) { + return {ChainstateLoadStatus::FAILURE_FATAL, mempool_error}; + } + 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)); + ChainstateManager::Options chainman_opts{ + .chainparams = chainparams, + .datadir = args.GetDataDirNet(), + .notifications = *node.notifications, + .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, + }; + Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction + try { + node.chainman = std::make_unique(*Assert(node.shutdown), chainman_opts, blockman_opts); + } catch (std::exception& e) { + return {ChainstateLoadStatus::FAILURE_FATAL, strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what())}; + } + ChainstateManager& chainman = *node.chainman; + // 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. + chainman.snapshot_download_completed = [&node]() { + if (!node.chainman->m_blockman.IsPruneMode()) { + LogPrintf("[snapshot] re-enabling NODE_NETWORK services\n"); + node.connman->AddLocalServices(NODE_NETWORK); + } + LogPrintf("[snapshot] restarting indexes\n"); + // Drain the validation interface queue to ensure that the old indexes + // don't have any pending work. + Assert(node.validation_signals)->SyncWithValidationInterfaceQueue(); + for (auto* index : node.indexes) { + index->Interrupt(); + index->Stop(); + if (!(index->Init() && index->StartBackgroundSync())) { + LogPrintf("[snapshot] WARNING failed to restart index %s on snapshot chain\n", index->GetName()); + } + } + }; + 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); + options.check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL); + options.require_full_verification = args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel"); + options.coins_error_cb = [] { + uiInterface.ThreadSafeMessageBox( + _("Error reading from database, shutting down."), + "", CClientUIInterface::MSG_ERROR); + }; + uiInterface.InitMessage(_("Loading block index…").translated); + const auto load_block_index_start_time{SteadyClock::now()}; + auto catch_exceptions = [](auto&& f) { + try { + return f(); + } catch (const std::exception& e) { + LogError("%s\n", e.what()); + return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database")); + } + }; + auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); }); + if (status == node::ChainstateLoadStatus::SUCCESS) { + uiInterface.InitMessage(_("Verifying blocks…").translated); + if (chainman.m_blockman.m_have_pruned && options.check_blocks > MIN_BLOCKS_TO_KEEP) { + LogWarning("pruned datadir may not have more than %d blocks; only checking available blocks\n", + MIN_BLOCKS_TO_KEEP); + } + std::tie(status, error) = catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); }); + if (status == node::ChainstateLoadStatus::SUCCESS) { + LogPrintf(" block index %15dms\n", Ticks(SteadyClock::now() - load_block_index_start_time)); + } + } + return {status, error}; +}; + bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) { const ArgsManager& args = *Assert(node.args); @@ -1514,20 +1614,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) node.notifications = std::make_unique(*Assert(node.shutdown), node.exit_status, *Assert(node.warnings)); ReadNotificationArgs(args, *node.notifications); - ChainstateManager::Options chainman_opts{ - .chainparams = chainparams, - .datadir = args.GetDataDirNet(), - .notifications = *node.notifications, - .signals = &validation_signals, - }; - 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, - }; - Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction // cache size calculations CacheSizes cache_sizes = CalculateCacheSizes(args, g_enabled_filter_types.size()); @@ -1546,96 +1632,25 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.mempool); assert(!node.chainman); - CTxMemPool::Options mempool_opts{ - .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, - .signals = &validation_signals, - }; - Assert(ApplyArgsManOptions(args, chainparams, mempool_opts)); // no error can happen, already checked in AppInitParameterInteraction - bool do_reindex{args.GetBoolArg("-reindex", false)}; const bool do_reindex_chainstate{args.GetBoolArg("-reindex-chainstate", false)}; for (bool fLoaded = false; !fLoaded && !ShutdownRequested(node);) { - bilingual_str mempool_error; - node.mempool = std::make_unique(mempool_opts, mempool_error); - if (!mempool_error.empty()) { - return InitError(mempool_error); - } - 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)); - - try { - node.chainman = std::make_unique(*Assert(node.shutdown), chainman_opts, blockman_opts); - } catch (std::exception& e) { - return InitError(strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what())); - } - ChainstateManager& chainman = *node.chainman; - - // 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. - chainman.snapshot_download_completed = [&node]() { - if (!node.chainman->m_blockman.IsPruneMode()) { - LogPrintf("[snapshot] re-enabling NODE_NETWORK services\n"); - node.connman->AddLocalServices(NODE_NETWORK); - } - - LogPrintf("[snapshot] restarting indexes\n"); - - // Drain the validation interface queue to ensure that the old indexes - // don't have any pending work. - Assert(node.validation_signals)->SyncWithValidationInterfaceQueue(); - - for (auto* index : node.indexes) { - index->Interrupt(); - index->Stop(); - if (!(index->Init() && index->StartBackgroundSync())) { - LogPrintf("[snapshot] WARNING failed to restart index %s on snapshot chain\n", index->GetName()); - } - } - }; - - 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); - options.check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL); - options.require_full_verification = args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel"); - options.coins_error_cb = [] { - uiInterface.ThreadSafeMessageBox( - _("Error reading from database, shutting down."), - "", CClientUIInterface::MSG_ERROR); - }; - - uiInterface.InitMessage(_("Loading block index…").translated); - const auto load_block_index_start_time{SteadyClock::now()}; - auto catch_exceptions = [](auto&& f) { - try { - return f(); - } catch (const std::exception& e) { - LogError("%s\n", e.what()); - return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database")); - } - }; - auto [status, error] = catch_exceptions([&]{ return LoadChainstate(chainman, cache_sizes, options); }); - if (status == node::ChainstateLoadStatus::SUCCESS) { - uiInterface.InitMessage(_("Verifying blocks…").translated); - if (chainman.m_blockman.m_have_pruned && options.check_blocks > MIN_BLOCKS_TO_KEEP) { - LogWarning("pruned datadir may not have more than %d blocks; only checking available blocks\n", - MIN_BLOCKS_TO_KEEP); - } - std::tie(status, error) = catch_exceptions([&]{ return VerifyLoadedChainstate(chainman, options);}); - if (status == node::ChainstateLoadStatus::SUCCESS) { - fLoaded = true; - LogPrintf(" block index %15dms\n", Ticks(SteadyClock::now() - load_block_index_start_time)); - } - } + auto [status, error] = InitAndLoadChainstate( + node, + do_reindex, + do_reindex_chainstate, + cache_sizes, + args); if (status == node::ChainstateLoadStatus::FAILURE_FATAL || status == node::ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB || status == node::ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE) { return InitError(error); } + if (status == ChainstateLoadStatus::SUCCESS) { + fLoaded = true; + } + if (!fLoaded && !ShutdownRequested(node)) { // first suggest a reindex if (!do_reindex) { From e9d60af9889c12b4d427adefa53fd234e417f8f6 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 25 Sep 2024 14:14:07 +0200 Subject: [PATCH 3/3] refactor: Replace init retry for loop with if statement The for loop has been a long standing source of confusion and bugs, both because its purpose is not clearly documented and because the body of the for loop contains a lot of logic. Co-Authored-By: Ryan Ofsky --- src/init.cpp | 56 +++++++++++++++++++++++----------------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 05a8eabc659..7755e7dad73 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1635,42 +1635,36 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) bool do_reindex{args.GetBoolArg("-reindex", false)}; const bool do_reindex_chainstate{args.GetBoolArg("-reindex-chainstate", false)}; - for (bool fLoaded = false; !fLoaded && !ShutdownRequested(node);) { - auto [status, error] = InitAndLoadChainstate( + // Chainstate initialization and loading may be retried once with reindexing by GUI users + auto [status, error] = InitAndLoadChainstate( + node, + do_reindex, + do_reindex_chainstate, + cache_sizes, + args); + if (status == ChainstateLoadStatus::FAILURE && !do_reindex && !ShutdownRequested(node)) { + // suggest a reindex + bool do_retry = 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 (!do_retry) { + LogError("Aborted block database rebuild. Exiting.\n"); + return false; + } + do_reindex = true; + if (!Assert(node.shutdown)->reset()) { + LogError("Internal error: failed to reset shutdown signal.\n"); + } + std::tie(status, error) = InitAndLoadChainstate( node, do_reindex, do_reindex_chainstate, cache_sizes, args); - - if (status == node::ChainstateLoadStatus::FAILURE_FATAL || status == node::ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB || status == node::ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE) { - return InitError(error); - } - - if (status == ChainstateLoadStatus::SUCCESS) { - fLoaded = true; - } - - if (!fLoaded && !ShutdownRequested(node)) { - // first suggest a 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) { - do_reindex = true; - if (!Assert(node.shutdown)->reset()) { - LogError("Internal error: failed to reset shutdown signal.\n"); - } - } else { - LogError("Aborted block database rebuild. Exiting.\n"); - return false; - } - } else { - return InitError(error); - } - } + } + if (status != ChainstateLoadStatus::SUCCESS && status != ChainstateLoadStatus::INTERRUPTED) { + return InitError(error); } // As LoadBlockIndex can take several minutes, it's possible the user