refactor: Remove calls to StartShutdown from KernelNotifications

Use SignalInterrupt object instead. There is a slight change in behavior here
because the previous StartShutdown code used to abort on failure and the
new code logs errors instead.

Author: ryanofsky

src/index/base.cpp

@@ -13,7 +13,6 @@
 #include <node/context.h>
 #include <node/database_args.h>
 #include <node/interface_ui.h>
-#include <shutdown.h>
 #include <tinyformat.h>
 #include <util/thread.h>
 #include <util/translation.h>
@@ -32,7 +31,7 @@ template <typename... Args>
 void BaseIndex::FatalErrorf(const char* fmt, const Args&... args)
 {
     auto message = tfm::format(fmt, args...);
-    node::AbortNode(m_chain->context()->exit_status, message);
+    node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, message);
 }
 
 CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)

src/init.cpp

@@ -1432,7 +1432,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 7: load block chain
 
-    node.notifications = std::make_unique<KernelNotifications>(node.exit_status);
+    node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status);
     ReadNotificationArgs(args, *node.notifications);
     fReindex = args.GetBoolArg("-reindex", false);
     bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false);

src/node/abort.cpp

@@ -6,7 +6,7 @@
 
 #include <logging.h>
 #include <node/interface_ui.h>
-#include <shutdown.h>
+#include <util/signalinterrupt.h>
 #include <util/translation.h>
 #include <warnings.h>
 
@@ -16,12 +16,14 @@
 
 namespace node {
 
-void AbortNode(std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message, bool shutdown)
+void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message)
 {
     SetMiscWarning(Untranslated(debug_message));
     LogPrintf("*** %s\n", debug_message);
     InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message);
     exit_status.store(EXIT_FAILURE);
-    if (shutdown) StartShutdown();
+    if (shutdown && !(*shutdown)()) {
+        LogPrintf("Error: failed to send shutdown signal\n");
+    };
 }
 } // namespace node

src/node/abort.h

@@ -10,8 +10,12 @@
 #include <atomic>
 #include <string>
 
+namespace util {
+class SignalInterrupt;
+} // namespace util
+
 namespace node {
-void AbortNode(std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message = {}, bool shutdown = true);
+void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message = {});
 } // namespace node
 
 #endif // BITCOIN_NODE_ABORT_H

src/node/kernel_notifications.cpp

@@ -15,7 +15,6 @@
 #include <logging.h>
 #include <node/abort.h>
 #include <node/interface_ui.h>
-#include <shutdown.h>
 #include <util/check.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -62,7 +61,9 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state
 {
     uiInterface.NotifyBlockTip(state, &index);
     if (m_stop_at_height && index.nHeight >= m_stop_at_height) {
-        StartShutdown();
+        if (!m_shutdown()) {
+            LogPrintf("Error: failed to send shutdown signal after reaching stop height\n");
+        }
         return kernel::Interrupted{};
     }
     return {};
@@ -85,12 +86,13 @@ void KernelNotifications::warning(const bilingual_str& warning)
 
 void KernelNotifications::flushError(const std::string& debug_message)
 {
-    AbortNode(m_exit_status, debug_message);
+    AbortNode(&m_shutdown, m_exit_status, debug_message);
 }
 
 void KernelNotifications::fatalError(const std::string& debug_message, const bilingual_str& user_message)
 {
-    node::AbortNode(m_exit_status, debug_message, user_message, m_shutdown_on_fatal_error);
+    node::AbortNode(m_shutdown_on_fatal_error ? &m_shutdown : nullptr,
+                    m_exit_status, debug_message, user_message);
 }
 
 void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications)

src/node/kernel_notifications.h

@@ -16,14 +16,18 @@ class CBlockIndex;
 enum class SynchronizationState;
 struct bilingual_str;
 
+namespace util {
+class SignalInterrupt;
+} // namespace util
+
 namespace node {
 
 static constexpr int DEFAULT_STOPATHEIGHT{0};
 
 class KernelNotifications : public kernel::Notifications
 {
 public:
-    KernelNotifications(std::atomic<int>& exit_status) : m_exit_status{exit_status} {}
+    KernelNotifications(util::SignalInterrupt& shutdown, std::atomic<int>& exit_status) : m_shutdown(shutdown), m_exit_status{exit_status} {}
 
     [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override;
 
@@ -42,6 +46,7 @@ class KernelNotifications : public kernel::Notifications
     //! Useful for tests, can be set to false to avoid shutdown on fatal error.
     bool m_shutdown_on_fatal_error{true};
 private:
+    util::SignalInterrupt& m_shutdown;
     std::atomic<int>& m_exit_status;
 };
 

src/test/blockmanager_tests.cpp

@@ -27,7 +27,7 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
 {
     const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)};
-    KernelNotifications notifications{m_node.exit_status};
+    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status};
     const BlockManager::Options blockman_opts{
         .chainparams = *params,
         .blocks_dir = m_args.GetBlocksDirPath(),
@@ -134,13 +134,13 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
 
 BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
 {
-    KernelNotifications notifications{m_node.exit_status};
+    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status};
     node::BlockManager::Options blockman_opts{
         .chainparams = Params(),
         .blocks_dir = m_args.GetBlocksDirPath(),
         .notifications = notifications,
     };
-    BlockManager blockman{m_node.kernel->interrupt, blockman_opts};
+    BlockManager blockman{*Assert(m_node.shutdown), blockman_opts};
 
     // Test blocks with no transactions, not even a coinbase
     CBlock block1;

src/test/util/setup_common.cpp

@@ -180,7 +180,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
 
     m_cache_sizes = CalculateCacheSizes(m_args);
 
-    m_node.notifications = std::make_unique<KernelNotifications>(m_node.exit_status);
+    m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status);
 
     const ChainstateManager::Options chainman_opts{
         .chainparams = chainparams,

src/test/validation_chainstatemanager_tests.cpp

@@ -379,7 +379,7 @@ struct SnapshotTestSetup : TestChain100Setup {
             LOCK(::cs_main);
             chainman.ResetChainstates();
             BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0);
-            m_node.notifications = std::make_unique<KernelNotifications>(m_node.exit_status);
+            m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status);
             const ChainstateManager::Options chainman_opts{
                 .chainparams = ::Params(),
                 .datadir = chainman.m_options.datadir,