From fa4c0750331f36121ba92bbc2f22c615b7934e52 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 25 Sep 2024 09:59:05 +0200 Subject: [PATCH 1/7] doc: Clarify waitTipChanged docs It should be obvious that a wait is not needed if the tip does not match. Also, remove a comment that the blockTip notification was only meant for the "UI". (It is used by other stuff for a long time) --- src/interfaces/mining.h | 3 +-- src/validation.cpp | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 421c5ba762f..2977811aa6d 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -64,8 +64,7 @@ public: * Waits for the tip to change * * @param[in] current_tip block hash of the current chain tip. Function waits - * for the chain tip to change if this matches, otherwise - * it returns right away. + * for the chain tip to differ from this. * @param[in] timeout how long to wait for a new tip * @returns Hash and height of the current chain tip after this call. */ diff --git a/src/validation.cpp b/src/validation.cpp index 5da3387ad29..d4ec42015bb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3533,7 +3533,6 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< m_chainman.m_options.signals->UpdatedBlockTip(pindexNewTip, pindexFork, still_in_ibd); } - // 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_blockfiles_indexed), *pindexNewTip))) { // Just breaking and returning success for now. This could // be changed to bubble up the kernel::Interrupted value to From fa18586c29d4faba3d3f478eaabf9c4cbc1a16e2 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 25 Sep 2024 10:35:15 +0200 Subject: [PATCH 2/7] refactor: Add missing GUARDED_BY(m_tip_block_mutex) Found by Cory Fields in https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1774001261 --- src/node/kernel_notifications.h | 2 +- src/test/validation_chainstate_tests.cpp | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index 9ff980ea56d..71947e5f98c 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -60,7 +60,7 @@ public: Mutex m_tip_block_mutex; std::condition_variable m_tip_block_cv; //! The block for which the last blockTip notification was received for. - uint256 m_tip_block; + uint256 m_tip_block GUARDED_BY(m_tip_block_mutex); private: util::SignalInterrupt& m_shutdown; diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 702af6578dc..c9cca8af04f 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -70,14 +70,18 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches) BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) { ChainstateManager& chainman = *Assert(m_node.chainman); - uint256 curr_tip = m_node.notifications->m_tip_block; + const auto get_notify_tip{[&]() { + LOCK(m_node.notifications->m_tip_block_mutex); + return m_node.notifications->m_tip_block; + }}; + uint256 curr_tip = get_notify_tip(); // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can // be found. mineBlocks(10); // After adding some blocks to the tip, best block should have changed. - BOOST_CHECK(m_node.notifications->m_tip_block != curr_tip); + BOOST_CHECK(get_notify_tip() != curr_tip); // Grab block 1 from disk; we'll add it to the background chain later. std::shared_ptr pblockone = std::make_shared(); @@ -92,15 +96,15 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) // Ensure our active chain is the snapshot chainstate. BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.IsSnapshotActive())); - curr_tip = m_node.notifications->m_tip_block; + curr_tip = get_notify_tip(); // Mine a new block on top of the activated snapshot chainstate. mineBlocks(1); // Defined in TestChain100Setup. // After adding some blocks to the snapshot tip, best block should have changed. - BOOST_CHECK(m_node.notifications->m_tip_block != curr_tip); + BOOST_CHECK(get_notify_tip() != curr_tip); - curr_tip = m_node.notifications->m_tip_block; + curr_tip = get_notify_tip(); BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2); @@ -136,10 +140,10 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) // Ensure tip is as expected BOOST_CHECK_EQUAL(background_cs.m_chain.Tip()->GetBlockHash(), pblockone->GetHash()); - // g_best_block should be unchanged after adding a block to the background + // get_notify_tip() should be unchanged after adding a block to the background // validation chain. BOOST_CHECK(block_added); - BOOST_CHECK_EQUAL(curr_tip, m_node.notifications->m_tip_block); + BOOST_CHECK_EQUAL(curr_tip, get_notify_tip()); } BOOST_AUTO_TEST_SUITE_END() From fad8e7fba7bf2c523a191309d392cab79668264b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 30 Sep 2024 11:53:05 +0200 Subject: [PATCH 3/7] bugfix: Mark m_tip_block_cv as guarded by m_tip_block_mutex This is not strictly required, but all places using m_tip_block_cv (except shutdown) already take the lock. The annotation makes it easier to catch potential deadlocks before review. Adding the missing lock to the shutdown sequence is a bugfix. An alternative would be to take the lock and release it before notifying, see https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716 --- src/node/kernel_notifications.h | 2 +- src/rpc/server.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index 71947e5f98c..12017f2b2c2 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -58,7 +58,7 @@ public: bool m_shutdown_on_fatal_error{true}; Mutex m_tip_block_mutex; - std::condition_variable m_tip_block_cv; + std::condition_variable m_tip_block_cv GUARDED_BY(m_tip_block_mutex); //! The block for which the last blockTip notification was received for. uint256 m_tip_block GUARDED_BY(m_tip_block_mutex); diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 086f609ac3e..ab2135a9193 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -305,7 +305,7 @@ void StopRPC(const std::any& context) DeleteAuthCookie(); node::NodeContext& node = EnsureAnyNodeContext(context); // The notifications interface doesn't exist between initialization step 4a and 7. - if (node.notifications) node.notifications->m_tip_block_cv.notify_all(); + 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"); }); } From 5ca28ef28bcca1775ff49921fc2528d9439b71ab Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 25 Sep 2024 10:45:34 +0200 Subject: [PATCH 4/7] 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); } From fa7f52af1a47ebe3d1bc00e3bac70bf9f6fa769b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 25 Sep 2024 10:08:22 +0200 Subject: [PATCH 5/7] refactor: Use wait_for predicate to check for interrupt Also use uint256::ZERO where appropriate for self-documenting code. --- src/interfaces/mining.h | 3 ++- src/node/interfaces.cpp | 15 ++++----------- src/node/kernel_notifications.h | 4 +++- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 2977811aa6d..f71f7d72512 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -61,7 +61,8 @@ public: virtual std::optional getTip() = 0; /** - * Waits for the tip to change + * Waits for the connected tip to change. If the tip was not connected on + * startup, this will wait. * * @param[in] current_tip block hash of the current chain tip. Function waits * for the chain tip to differ from this. diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 25a9029f061..ec4777ba7fd 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -940,19 +940,12 @@ public: BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override { - // Interrupt check interval - const MillisecondsDouble tick{1000}; - auto now{std::chrono::steady_clock::now()}; - auto deadline = now + timeout; - // std::chrono does not check against overflow - if (deadline < now) deadline = std::chrono::steady_clock::time_point::max(); + if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono { WAIT_LOCK(notifications().m_tip_block_mutex, lock); - while ((notifications().m_tip_block == uint256() || notifications().m_tip_block == current_tip) && !chainman().m_interrupt) { - now = std::chrono::steady_clock::now(); - if (now >= deadline) break; - notifications().m_tip_block_cv.wait_until(lock, std::min(deadline, now + tick)); - } + notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { + return (notifications().m_tip_block != current_tip && notifications().m_tip_block != uint256::ZERO) || chainman().m_interrupt; + }); } // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks. LOCK(::cs_main); diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index a39a20e2e2f..296b9c426dc 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -57,7 +57,9 @@ public: Mutex m_tip_block_mutex; std::condition_variable m_tip_block_cv GUARDED_BY(m_tip_block_mutex); //! The block for which the last blockTip notification was received for. - uint256 m_tip_block GUARDED_BY(m_tip_block_mutex); + //! The initial ZERO means that no block has been connected yet, which may + //! be true even long after startup, until shutdown. + uint256 m_tip_block GUARDED_BY(m_tip_block_mutex){uint256::ZERO}; private: const std::function& m_shutdown_request; From fa2e4439652172df27f14508eef078bd0ee2fca5 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 25 Sep 2024 11:57:17 +0200 Subject: [PATCH 6/7] refactor: Replace g_genesis_wait_cv with m_tip_block_cv They achieve the same, but the extra indirection over boost::signals2 is not needed. --- src/init.cpp | 41 +++++++---------------------------------- 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 433a2d07488..307a339aee0 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -685,21 +685,6 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddHiddenArgs(hidden_args); } -static bool fHaveGenesis = false; -static GlobalMutex g_genesis_wait_mutex; -static std::condition_variable g_genesis_wait_cv; - -static void BlockNotifyGenesisWait(const CBlockIndex* pBlockIndex) -{ - if (pBlockIndex != nullptr) { - { - LOCK(g_genesis_wait_mutex); - fHaveGenesis = true; - } - g_genesis_wait_cv.notify_all(); - } -} - #if HAVE_SYSTEM static void StartupNotify(const ArgsManager& args) { @@ -1616,7 +1601,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // ********************************************************* Step 7: load block chain node.notifications = std::make_unique(Assert(node.shutdown_request), node.exit_status, *Assert(node.warnings)); - ReadNotificationArgs(args, *node.notifications); + auto& kernel_notifications{*node.notifications}; + ReadNotificationArgs(args, kernel_notifications); // cache size calculations CacheSizes cache_sizes = CalculateCacheSizes(args, g_enabled_filter_types.size()); @@ -1768,15 +1754,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } } - // Either install a handler to notify us when genesis activates, or set fHaveGenesis directly. - // No locking, as this happens before any background thread is started. - boost::signals2::connection block_notify_genesis_wait_connection; - if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip() == nullptr)) { - block_notify_genesis_wait_connection = uiInterface.NotifyBlockTip_connect(std::bind(BlockNotifyGenesisWait, std::placeholders::_2)); - } else { - fHaveGenesis = true; - } - #if HAVE_SYSTEM const std::string block_notify = args.GetArg("-blocknotify", ""); if (!block_notify.empty()) { @@ -1821,15 +1798,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) }); // Wait for genesis block to be processed - { - WAIT_LOCK(g_genesis_wait_mutex, lock); - // We previously could hang here if shutdown was requested prior to - // ImportBlocks getting started, so instead we just wait on a timer to - // check ShutdownRequested() regularly. - while (!fHaveGenesis && !ShutdownRequested(node)) { - g_genesis_wait_cv.wait_for(lock, std::chrono::milliseconds(500)); - } - block_notify_genesis_wait_connection.disconnect(); + if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip() == nullptr)) { + WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock); + kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) { + return !kernel_notifications.m_tip_block.IsNull() || ShutdownRequested(node); + }); } if (ShutdownRequested(node)) { From fa22e5c430acaef9713d9a4b4b97bb3f4876f816 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 25 Sep 2024 15:42:51 +0200 Subject: [PATCH 7/7] refactor: Remove dead code that assumed tip == nullptr The tip is set after waiting for the genesis block. --- src/init.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 307a339aee0..bf268d418c6 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1811,17 +1811,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // ********************************************************* Step 12: start node - //// debug print int64_t best_block_time{}; { - LOCK(cs_main); + LOCK(chainman.GetMutex()); + const auto& tip{*Assert(chainman.ActiveTip())}; LogPrintf("block tree size = %u\n", chainman.BlockIndex().size()); - chain_active_height = chainman.ActiveChain().Height(); - best_block_time = chainman.ActiveChain().Tip() ? chainman.ActiveChain().Tip()->GetBlockTime() : chainman.GetParams().GenesisBlock().GetBlockTime(); + chain_active_height = tip.nHeight; + best_block_time = tip.GetBlockTime(); if (tip_info) { tip_info->block_height = chain_active_height; tip_info->block_time = best_block_time; - tip_info->verification_progress = GuessVerificationProgress(chainman.GetParams().TxData(), chainman.ActiveChain().Tip()); + tip_info->verification_progress = GuessVerificationProgress(chainman.GetParams().TxData(), &tip); } if (tip_info && chainman.m_best_header) { tip_info->header_height = chainman.m_best_header->nHeight;