From 5ca28ef28bcca1775ff49921fc2528d9439b71ab Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 25 Sep 2024 10:45:34 +0200 Subject: [PATCH] refactor: Split up NodeContext shutdown_signal and shutdown_request Instead of having a single NodeContext::shutdown member that is used both to request shutdowns and check if they have been requested, use separate members for each. Benefits of this change: 1. Should make code a little clearer and easier to search because it is easier to see which parts of code are triggering shutdowns and which parts are just checking to see if they were triggered. 2. Makes it possible for init.cpp to specify additional code to run when a shutdown is requested, like signalling the m_tip_block_cv condition variable. Motivation for this change was to remove hacky NodeContext argument and m_tip_block_cv access from the StopRPC function, so StopRPC can just be concerned with RPC functionality, not other node functionality. --- src/bitcoind.cpp | 2 +- src/index/base.cpp | 2 +- src/init.cpp | 25 ++++++++++++------- src/node/abort.cpp | 4 +-- src/node/abort.h | 7 ++---- src/node/context.h | 4 ++- src/node/interfaces.cpp | 6 +++-- src/node/kernel_notifications.cpp | 6 ++--- src/node/kernel_notifications.h | 11 +++----- src/rpc/server.cpp | 7 ++---- src/rpc/server.h | 3 +-- src/test/blockfilter_index_tests.cpp | 2 +- src/test/blockmanager_tests.cpp | 8 +++--- src/test/coinstatsindex_tests.cpp | 4 +-- src/test/txindex_tests.cpp | 2 +- src/test/util/setup_common.cpp | 7 +++--- .../validation_chainstatemanager_tests.cpp | 4 +-- 17 files changed, 53 insertions(+), 51 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 1e27924dfdb..9d08c425588 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -274,7 +274,7 @@ MAIN_FUNCTION if (ProcessInitCommands(args)) return EXIT_SUCCESS; // Start application - if (!AppInit(node) || !Assert(node.shutdown)->wait()) { + if (!AppInit(node) || !Assert(node.shutdown_signal)->wait()) { node.exit_status = EXIT_FAILURE; } Interrupt(node); diff --git a/src/index/base.cpp b/src/index/base.cpp index 810358394a1..1a7eb9cd5e8 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -31,7 +31,7 @@ template void BaseIndex::FatalErrorf(util::ConstevalFormatString fmt, const Args&... args) { auto message = tfm::format(fmt, args...); - node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message), m_chain->context()->warnings.get()); + node::AbortNode(m_chain->context()->shutdown_request, m_chain->context()->exit_status, Untranslated(message), m_chain->context()->warnings.get()); } CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash) diff --git a/src/init.cpp b/src/init.cpp index 3b8474dd469..433a2d07488 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -207,7 +207,14 @@ void InitContext(NodeContext& node) g_shutdown.emplace(); node.args = &gArgs; - node.shutdown = &*g_shutdown; + node.shutdown_signal = &*g_shutdown; + node.shutdown_request = [&node] { + assert(node.shutdown_signal); + if (!(*node.shutdown_signal)()) return false; + // Wake any threads that may be waiting for the tip to change. + if (node.notifications) WITH_LOCK(node.notifications->m_tip_block_mutex, node.notifications->m_tip_block_cv.notify_all()); + return true; + }; } ////////////////////////////////////////////////////////////////////////////// @@ -235,7 +242,7 @@ void InitContext(NodeContext& node) bool ShutdownRequested(node::NodeContext& node) { - return bool{*Assert(node.shutdown)}; + return bool{*Assert(node.shutdown_signal)}; } #if HAVE_SYSTEM @@ -286,7 +293,7 @@ void Shutdown(NodeContext& node) StopHTTPRPC(); StopREST(); - StopRPC(&node); + StopRPC(); StopHTTPServer(); for (const auto& client : node.chain_clients) { client->flush(); @@ -707,7 +714,7 @@ static void StartupNotify(const ArgsManager& args) static bool AppInitServers(NodeContext& node) { const ArgsManager& args = *Assert(node.args); - if (!InitHTTPServer(*Assert(node.shutdown))) { + if (!InitHTTPServer(*Assert(node.shutdown_signal))) { return false; } StartRPC(); @@ -1216,7 +1223,7 @@ static ChainstateLoadResult InitAndLoadChainstate( }; 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); + node.chainman = std::make_unique(*Assert(node.shutdown_signal), chainman_opts, blockman_opts); } catch (std::exception& e) { return {ChainstateLoadStatus::FAILURE_FATAL, strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what())}; } @@ -1327,7 +1334,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) constexpr uint64_t min_disk_space = 50 << 20; // 50 MB if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) { LogError("Shutting down due to lack of disk space!\n"); - if (!(*Assert(node.shutdown))()) { + if (!(Assert(node.shutdown_request))()) { LogError("Failed to send shutdown signal after disk space check\n"); } } @@ -1608,7 +1615,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // ********************************************************* Step 7: load block chain - node.notifications = std::make_unique(*Assert(node.shutdown), node.exit_status, *Assert(node.warnings)); + node.notifications = std::make_unique(Assert(node.shutdown_request), node.exit_status, *Assert(node.warnings)); ReadNotificationArgs(args, *node.notifications); // cache size calculations @@ -1649,7 +1656,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) return false; } do_reindex = true; - if (!Assert(node.shutdown)->reset()) { + if (!Assert(node.shutdown_signal)->reset()) { LogError("Internal error: failed to reset shutdown signal.\n"); } std::tie(status, error) = InitAndLoadChainstate( @@ -1794,7 +1801,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) ImportBlocks(chainman, vImportFiles); if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) { LogPrintf("Stopping after block import\n"); - if (!(*Assert(node.shutdown))()) { + if (!(Assert(node.shutdown_request))()) { LogError("Failed to send shutdown signal after finishing block import\n"); } return; diff --git a/src/node/abort.cpp b/src/node/abort.cpp index 8a17c41fd22..c15bf047c86 100644 --- a/src/node/abort.cpp +++ b/src/node/abort.cpp @@ -15,12 +15,12 @@ namespace node { -void AbortNode(util::SignalInterrupt* shutdown, std::atomic& exit_status, const bilingual_str& message, node::Warnings* warnings) +void AbortNode(const std::function& shutdown_request, std::atomic& exit_status, const bilingual_str& message, node::Warnings* warnings) { if (warnings) warnings->Set(Warning::FATAL_INTERNAL_ERROR, message); InitError(_("A fatal internal error occurred, see debug.log for details: ") + message); exit_status.store(EXIT_FAILURE); - if (shutdown && !(*shutdown)()) { + if (shutdown_request && !shutdown_request()) { LogError("Failed to send shutdown signal\n"); }; } diff --git a/src/node/abort.h b/src/node/abort.h index c881af46345..c8514628bc0 100644 --- a/src/node/abort.h +++ b/src/node/abort.h @@ -6,16 +6,13 @@ #define BITCOIN_NODE_ABORT_H #include +#include struct bilingual_str; -namespace util { -class SignalInterrupt; -} // namespace util - namespace node { class Warnings; -void AbortNode(util::SignalInterrupt* shutdown, std::atomic& exit_status, const bilingual_str& message, node::Warnings* warnings); +void AbortNode(const std::function& shutdown_request, std::atomic& exit_status, const bilingual_str& message, node::Warnings* warnings); } // namespace node #endif // BITCOIN_NODE_ABORT_H diff --git a/src/node/context.h b/src/node/context.h index 398089ff3c0..debc1221206 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -59,8 +59,10 @@ struct NodeContext { std::unique_ptr ecc_context; //! Init interface for initializing current process and connecting to other processes. interfaces::Init* init{nullptr}; + //! Function to request a shutdown. + std::function shutdown_request; //! Interrupt object used to track whether node shutdown was requested. - util::SignalInterrupt* shutdown{nullptr}; + util::SignalInterrupt* shutdown_signal{nullptr}; std::unique_ptr addrman; std::unique_ptr connman; std::unique_ptr mempool; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index febd9683138..25a9029f061 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -134,13 +134,15 @@ public: } void startShutdown() override { - if (!(*Assert(Assert(m_context)->shutdown))()) { + NodeContext& ctx{*Assert(m_context)}; + if (!(Assert(ctx.shutdown_request))()) { LogError("Failed to send shutdown signal\n"); } + // Stop RPC for clean shutdown if any of waitfor* commands is executed. if (args().GetBoolArg("-server", false)) { InterruptRPC(); - StopRPC(m_context); + StopRPC(); } } bool shutdownRequested() override { return ShutdownRequested(*Assert(m_context)); }; diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 40d45c8c2b8..05fe2f22481 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -58,7 +58,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state uiInterface.NotifyBlockTip(state, &index); if (m_stop_at_height && index.nHeight >= m_stop_at_height) { - if (!m_shutdown()) { + if (!m_shutdown_request()) { LogError("Failed to send shutdown signal after reaching stop height\n"); } return kernel::Interrupted{}; @@ -90,12 +90,12 @@ void KernelNotifications::warningUnset(kernel::Warning id) void KernelNotifications::flushError(const bilingual_str& message) { - AbortNode(&m_shutdown, m_exit_status, message, &m_warnings); + AbortNode(m_shutdown_request, m_exit_status, message, &m_warnings); } void KernelNotifications::fatalError(const bilingual_str& message) { - node::AbortNode(m_shutdown_on_fatal_error ? &m_shutdown : nullptr, + node::AbortNode(m_shutdown_on_fatal_error ? m_shutdown_request : nullptr, m_exit_status, message, &m_warnings); } diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index 12017f2b2c2..a39a20e2e2f 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -13,6 +13,7 @@ #include #include +#include class ArgsManager; class CBlockIndex; @@ -23,10 +24,6 @@ namespace kernel { enum class Warning; } // namespace kernel -namespace util { -class SignalInterrupt; -} // namespace util - namespace node { class Warnings; @@ -35,8 +32,8 @@ static constexpr int DEFAULT_STOPATHEIGHT{0}; class KernelNotifications : public kernel::Notifications { public: - KernelNotifications(util::SignalInterrupt& shutdown, std::atomic& exit_status, node::Warnings& warnings) - : m_shutdown(shutdown), m_exit_status{exit_status}, m_warnings{warnings} {} + KernelNotifications(const std::function& shutdown_request, std::atomic& exit_status, node::Warnings& warnings) + : m_shutdown_request(shutdown_request), m_exit_status{exit_status}, m_warnings{warnings} {} [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override EXCLUSIVE_LOCKS_REQUIRED(!m_tip_block_mutex); @@ -63,7 +60,7 @@ public: uint256 m_tip_block GUARDED_BY(m_tip_block_mutex); private: - util::SignalInterrupt& m_shutdown; + const std::function& m_shutdown_request; std::atomic& m_exit_status; node::Warnings& m_warnings; }; diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index ab2135a9193..6c844ffc8fe 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -169,7 +169,7 @@ static RPCHelpMan stop() { // Event loop will exit after current HTTP requests have been handled, so // this reply will get back to the client. - CHECK_NONFATAL((*CHECK_NONFATAL(EnsureAnyNodeContext(jsonRequest.context).shutdown))()); + CHECK_NONFATAL((CHECK_NONFATAL(EnsureAnyNodeContext(jsonRequest.context).shutdown_request))()); if (jsonRequest.params[0].isNum()) { UninterruptibleSleep(std::chrono::milliseconds{jsonRequest.params[0].getInt()}); } @@ -294,7 +294,7 @@ void InterruptRPC() }); } -void StopRPC(const std::any& context) +void StopRPC() { static std::once_flag g_rpc_stop_flag; // This function could be called twice if the GUI has been started with -server=1. @@ -303,9 +303,6 @@ void StopRPC(const std::any& context) LogDebug(BCLog::RPC, "Stopping RPC\n"); WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear()); DeleteAuthCookie(); - node::NodeContext& node = EnsureAnyNodeContext(context); - // The notifications interface doesn't exist between initialization step 4a and 7. - if (node.notifications) WITH_LOCK(node.notifications->m_tip_block_mutex, node.notifications->m_tip_block_cv.notify_all()); LogDebug(BCLog::RPC, "RPC stopped.\n"); }); } diff --git a/src/rpc/server.h b/src/rpc/server.h index a8896d4247a..5a22279a58e 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -9,7 +9,6 @@ #include #include -#include #include #include #include @@ -173,7 +172,7 @@ extern CRPCTable tableRPC; void StartRPC(); void InterruptRPC(); -void StopRPC(const std::any& context); +void StopRPC(); UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors); #endif // BITCOIN_RPC_SERVER_H diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index f6024f1ef35..48ae874fcd8 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -144,7 +144,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup) BOOST_REQUIRE(filter_index.StartBackgroundSync()); // Allow filter index to catch up with the block index. - IndexWaitSynced(filter_index, *Assert(m_node.shutdown)); + IndexWaitSynced(filter_index, *Assert(m_node.shutdown_signal)); // Check that filter index has all blocks that were in the chain before it started. { diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 121f00bd25e..c2b95dd861e 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -28,13 +28,13 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) { const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)}; - KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)}; + KernelNotifications notifications{Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)}; const BlockManager::Options blockman_opts{ .chainparams = *params, .blocks_dir = m_args.GetBlocksDirPath(), .notifications = notifications, }; - BlockManager blockman{*Assert(m_node.shutdown), blockman_opts}; + BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts}; // simulate adding a genesis block normally BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); // simulate what happens during reindex @@ -135,13 +135,13 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file) { - KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)}; + KernelNotifications notifications{Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)}; node::BlockManager::Options blockman_opts{ .chainparams = Params(), .blocks_dir = m_args.GetBlocksDirPath(), .notifications = notifications, }; - BlockManager blockman{*Assert(m_node.shutdown), blockman_opts}; + BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts}; // Test blocks with no transactions, not even a coinbase CBlock block1; diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index 08814c1499d..e09aad05e94 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -35,7 +35,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) BOOST_REQUIRE(coin_stats_index.StartBackgroundSync()); - IndexWaitSynced(coin_stats_index, *Assert(m_node.shutdown)); + IndexWaitSynced(coin_stats_index, *Assert(m_node.shutdown_signal)); // Check that CoinStatsIndex works for genesis block. const CBlockIndex* genesis_block_index; @@ -86,7 +86,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) CoinStatsIndex index{interfaces::MakeChain(m_node), 1 << 20}; BOOST_REQUIRE(index.Init()); BOOST_REQUIRE(index.StartBackgroundSync()); - IndexWaitSynced(index, *Assert(m_node.shutdown)); + IndexWaitSynced(index, *Assert(m_node.shutdown_signal)); std::shared_ptr new_block; CBlockIndex* new_block_index = nullptr; { diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp index 5a32b02ad9c..9ee53878306 100644 --- a/src/test/txindex_tests.cpp +++ b/src/test/txindex_tests.cpp @@ -33,7 +33,7 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup) BOOST_REQUIRE(txindex.StartBackgroundSync()); // Allow tx index to catch up with the block index. - IndexWaitSynced(txindex, *Assert(m_node.shutdown)); + IndexWaitSynced(txindex, *Assert(m_node.shutdown_signal)); // Check that txindex excludes genesis block transactions. const CBlock& genesis_block = Params().GenesisBlock(); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index fa89ceb332a..74658463562 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -104,7 +104,8 @@ static void ExitFailure(std::string_view str_err) BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts) : m_args{} { - m_node.shutdown = &m_interrupt; + m_node.shutdown_signal = &m_interrupt; + m_node.shutdown_request = [this]{ return m_interrupt(); }; m_node.args = &gArgs; std::vector arguments = Cat( { @@ -224,7 +225,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) m_cache_sizes = CalculateCacheSizes(m_args); - m_node.notifications = std::make_unique(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)); + m_node.notifications = std::make_unique(Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)); m_make_chainman = [this, &chainparams, opts] { Assert(!m_node.chainman); @@ -245,7 +246,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) .blocks_dir = m_args.GetBlocksDirPath(), .notifications = chainman_opts.notifications, }; - m_node.chainman = std::make_unique(*Assert(m_node.shutdown), chainman_opts, blockman_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 = std::make_unique(DBParams{ .path = m_args.GetDataDirNet() / "blocks" / "index", diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index b4fcfbd8537..6c2a825e647 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -382,7 +382,7 @@ struct SnapshotTestSetup : TestChain100Setup { LOCK(::cs_main); chainman.ResetChainstates(); BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0); - m_node.notifications = std::make_unique(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)); + m_node.notifications = std::make_unique(Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)); const ChainstateManager::Options chainman_opts{ .chainparams = ::Params(), .datadir = chainman.m_options.datadir, @@ -397,7 +397,7 @@ struct SnapshotTestSetup : TestChain100Setup { // For robustness, ensure the old manager is destroyed before creating a // new one. m_node.chainman.reset(); - m_node.chainman = std::make_unique(*Assert(m_node.shutdown), chainman_opts, blockman_opts); + m_node.chainman = std::make_unique(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts); } return *Assert(m_node.chainman); }