From 73133c36aa9cc09546eabac18d0ea35274dd5d72 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 7 Jul 2023 17:32:54 -0400 Subject: [PATCH] refactor: Add NodeContext::shutdown member Add NodeContext::shutdown variable and start using it to replace the kernel::Context::interrupt variable. The latter can't easily be removed right away but will be removed later in this PR. Moving the interrupt object from the kernel context to the node context increases flexibility of the kernel API so it is possible to use multiple interrupt objects, or avoid creating one if one is not needed. It will also allow getting rid of the kernel::g_context global later in this PR, replacing it with a private SignalInterrupt instance in init.cpp There is no change in behavior in this commit outside of unit tests. In unit tests there should be no visible change either, but internally now each test has its own interrupt variable so the variable will be automatically reset between tests. --- src/bitcoin-chainstate.cpp | 3 ++- src/bitcoind.cpp | 1 + src/init.cpp | 2 +- src/node/context.h | 2 ++ src/node/interfaces.cpp | 1 + src/test/blockmanager_tests.cpp | 2 +- src/test/util/setup_common.cpp | 3 ++- src/test/util/setup_common.h | 1 + src/test/validation_chainstatemanager_tests.cpp | 2 +- 9 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index f241bdae4d..ee8b0a44c5 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -125,7 +125,8 @@ int main(int argc, char* argv[]) .blocks_dir = abs_datadir / "blocks", .notifications = chainman_opts.notifications, }; - ChainstateManager chainman{kernel_context.interrupt, chainman_opts, blockman_opts}; + util::SignalInterrupt interrupt; + ChainstateManager chainman{interrupt, chainman_opts, blockman_opts}; node::CacheSizes cache_sizes; cache_sizes.block_tree_db = 2 << 20; diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index e2224befef..ccdc628a0f 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -185,6 +185,7 @@ static bool AppInit(NodeContext& node) } node.kernel = std::make_unique(); + node.shutdown = &node.kernel->interrupt; // TEMPORARY: will go away when kernel->interrupt member is removed if (!AppInitSanityChecks(*node.kernel)) { // InitError will have been called with detailed error, which ends up on console diff --git a/src/init.cpp b/src/init.cpp index 8bff75ec91..dfc05ebb67 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1485,7 +1485,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) { node.mempool = std::make_unique(mempool_opts); - node.chainman = std::make_unique(node.kernel->interrupt, chainman_opts, blockman_opts); + node.chainman = std::make_unique(*Assert(node.shutdown), chainman_opts, blockman_opts); ChainstateManager& chainman = *node.chainman; // This is defined and set here instead of inline in validation.h to avoid a hard diff --git a/src/node/context.h b/src/node/context.h index dae900ff7f..4f3b640b2d 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -50,6 +50,8 @@ struct NodeContext { std::unique_ptr kernel; //! Init interface for initializing current process and connecting to other processes. interfaces::Init* init{nullptr}; + //! Interrupt object used to track whether node shutdown was requested. + util::SignalInterrupt* shutdown{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 b5691f0e57..088497399a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -99,6 +99,7 @@ public: if (!AppInitParameterInteraction(args())) return false; m_context->kernel = std::make_unique(); + m_context->shutdown = &m_context->kernel->interrupt; // TEMPORARY: will go away when kernel->interrupt member is removed if (!AppInitSanityChecks(*m_context->kernel)) return false; if (!AppInitLockDataDirectory()) return false; diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index e1b31c20f4..8a777ee5ff 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -33,7 +33,7 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) .blocks_dir = m_args.GetBlocksDirPath(), .notifications = notifications, }; - BlockManager blockman{m_node.kernel->interrupt, blockman_opts}; + BlockManager blockman{*Assert(m_node.shutdown), blockman_opts}; // simulate adding a genesis block normally BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); // simulate what happens during reindex diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index bc639da4dd..4b700b18d6 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -102,6 +102,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vecto : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()}, m_args{} { + m_node.shutdown = &m_interrupt; m_node.args = &gArgs; std::vector arguments = Cat( { @@ -194,7 +195,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto .blocks_dir = m_args.GetBlocksDirPath(), .notifications = chainman_opts.notifications, }; - m_node.chainman = std::make_unique(m_node.kernel->interrupt, chainman_opts, blockman_opts); + m_node.chainman = std::make_unique(*Assert(m_node.shutdown), chainman_opts, blockman_opts); m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ .path = m_args.GetDataDirNet() / "blocks" / "index", .cache_bytes = static_cast(m_cache_sizes.block_tree_db), diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index c3a4df28bf..9ff4c372a5 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -47,6 +47,7 @@ static constexpr CAmount CENT{1000000}; * This just configures logging, data dir and chain parameters. */ struct BasicTestingSetup { + util::SignalInterrupt m_interrupt; node::NodeContext m_node; // keep as first member to be destructed last explicit BasicTestingSetup(const ChainType chainType = ChainType::MAIN, const std::vector& extra_args = {}); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 6969822ad7..6c81cd618b 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -394,7 +394,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(m_node.kernel->interrupt, chainman_opts, blockman_opts); + m_node.chainman = std::make_unique(*Assert(m_node.shutdown), chainman_opts, blockman_opts); } return *Assert(m_node.chainman); }