From 52ecb6547dd2b59643483e43531d1201941c0df0 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: [PATCH] Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt
 directly

This change is mostly a refectoring that removes some code and gets rid of an
unnecessary layer of indirection after #27861

But it is not a pure refactoring since StartShutdown, AbortShutdown, and
WaitForShutdown functions used to abort on failure, and the replacement code
logs or returns errors instead.
---
 src/Makefile.am                |  2 --
 src/bitcoind.cpp               |  6 +---
 src/init.cpp                   | 55 +++++++++++++++++++++++-----------
 src/init.h                     |  4 ++-
 src/kernel/context.cpp         |  6 ----
 src/kernel/context.h           | 15 ----------
 src/node/interfaces.cpp        | 11 +++----
 src/shutdown.cpp               | 43 --------------------------
 src/shutdown.h                 | 25 ----------------
 src/test/util/setup_common.cpp |  1 -
 10 files changed, 47 insertions(+), 121 deletions(-)
 delete mode 100644 src/shutdown.cpp
 delete mode 100644 src/shutdown.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 8905c0ad1cd25..e54823600a732 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -270,7 +270,6 @@ BITCOIN_CORE_H = \
   script/sign.h \
   script/signingprovider.h \
   script/solver.h \
-  shutdown.h \
   signet.h \
   streams.h \
   support/allocators/pool.h \
@@ -456,7 +455,6 @@ libbitcoin_node_a_SOURCES = \
   rpc/signmessage.cpp \
   rpc/txoutproof.cpp \
   script/sigcache.cpp \
-  shutdown.cpp \
   signet.cpp \
   timedata.cpp \
   torcontrol.cpp \
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index ccdc628a0fd36..4f0a816388e7f 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -20,7 +20,6 @@
 #include <node/context.h>
 #include <node/interface_ui.h>
 #include <noui.h>
-#include <shutdown.h>
 #include <util/check.h>
 #include <util/exception.h>
 #include <util/strencodings.h>
@@ -185,7 +184,6 @@ static bool AppInit(NodeContext& node)
         }
 
         node.kernel = std::make_unique<kernel::Context>();
-        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
@@ -273,9 +271,7 @@ MAIN_FUNCTION
     if (ProcessInitCommands(args)) return EXIT_SUCCESS;
 
     // Start application
-    if (AppInit(node)) {
-        WaitForShutdown();
-    } else {
+    if (!AppInit(node) || !Assert(node.shutdown)->wait()) {
         node.exit_status = EXIT_FAILURE;
     }
     Interrupt(node);
diff --git a/src/init.cpp b/src/init.cpp
index 8e8439aaae3f3..3f5ce3eba7737 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -66,7 +66,6 @@
 #include <rpc/util.h>
 #include <scheduler.h>
 #include <script/sigcache.h>
-#include <shutdown.h>
 #include <sync.h>
 #include <timedata.h>
 #include <torcontrol.h>
@@ -176,9 +175,15 @@ static fs::path GetPidFile(const ArgsManager& args)
     }
 }
 
+static std::optional<util::SignalInterrupt> g_shutdown;
+
 void InitContext(NodeContext& node)
 {
+    assert(!g_shutdown);
+    g_shutdown.emplace();
+
     node.args = &gArgs;
+    node.shutdown = &*g_shutdown;
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -192,11 +197,9 @@ void InitContext(NodeContext& node)
 // The network-processing threads are all part of a thread group
 // created by AppInit() or the Qt main() function.
 //
-// A clean exit happens when StartShutdown() or the SIGTERM
-// signal handler sets ShutdownRequested(), which makes main thread's
-// WaitForShutdown() interrupts the thread group.
-// And then, WaitForShutdown() makes all other on-going threads
-// in the thread group join the main thread.
+// A clean exit happens when the SignalInterrupt object is triggered, which
+// makes main thread's SignalInterrupt::wait() call return, and join all other
+// ongoing threads in the thread group to the main thread.
 // Shutdown() is then called to clean up database connections, and stop other
 // threads that should only be stopped after the main network-processing
 // threads have exited.
@@ -206,6 +209,11 @@ void InitContext(NodeContext& node)
 // shutdown thing.
 //
 
+bool ShutdownRequested(node::NodeContext& node)
+{
+    return bool{*(Assert(node.shutdown))};
+}
+
 #if HAVE_SYSTEM
 static void ShutdownNotify(const ArgsManager& args)
 {
@@ -373,7 +381,9 @@ void Shutdown(NodeContext& node)
 #ifndef WIN32
 static void HandleSIGTERM(int)
 {
-    StartShutdown();
+    // Return value is intentionally ignored because there is not a better way
+    // of handling this failure in a signal handler.
+    (void)(*Assert(g_shutdown))();
 }
 
 static void HandleSIGHUP(int)
@@ -383,7 +393,10 @@ static void HandleSIGHUP(int)
 #else
 static BOOL WINAPI consoleCtrlHandler(DWORD dwCtrlType)
 {
-    StartShutdown();
+    if (!(*Assert(g_shutdown))()) {
+        LogPrintf("Error: failed to send shutdown signal on Ctrl-C\n");
+        return false;
+    }
     Sleep(INFINITE);
     return true;
 }
@@ -1145,11 +1158,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }, std::chrono::minutes{1});
 
     // Check disk space every 5 minutes to avoid db corruption.
-    node.scheduler->scheduleEvery([&args]{
+    node.scheduler->scheduleEvery([&args, &node]{
         constexpr uint64_t min_disk_space = 50 << 20; // 50 MB
         if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) {
             LogPrintf("Shutting down due to lack of disk space!\n");
-            StartShutdown();
+            if (!(*Assert(node.shutdown))()) {
+                LogPrintf("Error: failed to send shutdown signal after disk space check\n");
+            }
         }
     }, std::chrono::minutes{5});
 
@@ -1493,7 +1508,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }
     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));
 
-    for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
+    for (bool fLoaded = false; !fLoaded && !ShutdownRequested(node);) {
         node.mempool = std::make_unique<CTxMemPool>(mempool_opts);
 
         node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown), chainman_opts, blockman_opts);
@@ -1560,7 +1575,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             return InitError(error);
         }
 
-        if (!fLoaded && !ShutdownRequested()) {
+        if (!fLoaded && !ShutdownRequested(node)) {
             // first suggest a reindex
             if (!options.reindex) {
                 bool fRet = uiInterface.ThreadSafeQuestion(
@@ -1569,7 +1584,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
                     "", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);
                 if (fRet) {
                     fReindex = true;
-                    AbortShutdown();
+                    if (!Assert(node.shutdown)->reset()) {
+                        LogPrintf("Internal error: failed to reset shutdown signal.\n");
+                    }
                 } else {
                     LogPrintf("Aborted block database rebuild. Exiting.\n");
                     return false;
@@ -1583,7 +1600,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // As LoadBlockIndex can take several minutes, it's possible the user
     // requested to kill the GUI during the last operation. If so, exit.
     // As the program has not fully started yet, Shutdown() is possibly overkill.
-    if (ShutdownRequested()) {
+    if (ShutdownRequested(node)) {
         LogPrintf("Shutdown requested. Exiting.\n");
         return false;
     }
@@ -1704,7 +1721,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         ImportBlocks(chainman, vImportFiles);
         if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
             LogPrintf("Stopping after block import\n");
-            StartShutdown();
+            if (!(*Assert(node.shutdown))()) {
+                LogPrintf("Error: failed to send shutdown signal after finishing block import\n");
+            }
             return;
         }
 
@@ -1724,16 +1743,16 @@ 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 StartShutdown() is called prior to
+        // 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()) {
+        while (!fHaveGenesis && !ShutdownRequested(node)) {
             g_genesis_wait_cv.wait_for(lock, std::chrono::milliseconds(500));
         }
         block_notify_genesis_wait_connection.disconnect();
     }
 
-    if (ShutdownRequested()) {
+    if (ShutdownRequested(node)) {
         return false;
     }
 
diff --git a/src/init.h b/src/init.h
index da8255cda3579..ead5f5e0d2195 100644
--- a/src/init.h
+++ b/src/init.h
@@ -26,8 +26,10 @@ namespace node {
 struct NodeContext;
 } // namespace node
 
-/** Initialize node context variables. */
+/** Initialize node context shutdown and args variables. */
 void InitContext(node::NodeContext& node);
+/** Return whether node shutdown was requested. */
+bool ShutdownRequested(node::NodeContext& node);
 
 /** Interrupt threads */
 void Interrupt(node::NodeContext& node);
diff --git a/src/kernel/context.cpp b/src/kernel/context.cpp
index 3f4a423531cdc..c60f1638d119b 100644
--- a/src/kernel/context.cpp
+++ b/src/kernel/context.cpp
@@ -14,12 +14,8 @@
 
 
 namespace kernel {
-Context* g_context;
-
 Context::Context()
 {
-    assert(!g_context);
-    g_context = this;
     std::string sha256_algo = SHA256AutoDetect();
     LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
     RandomInit();
@@ -29,8 +25,6 @@ Context::Context()
 Context::~Context()
 {
     ECC_Stop();
-    assert(g_context);
-    g_context = nullptr;
 }
 
 } // namespace kernel
diff --git a/src/kernel/context.h b/src/kernel/context.h
index e74e0a6f08f63..ff4df20473e7c 100644
--- a/src/kernel/context.h
+++ b/src/kernel/context.h
@@ -18,24 +18,9 @@ namespace kernel {
 //! State stored directly in this struct should be simple. More complex state
 //! should be stored to std::unique_ptr members pointing to opaque types.
 struct Context {
-    //! Interrupt object that can be used to stop long-running kernel operations.
-    util::SignalInterrupt interrupt;
-
-    //! Declare default constructor and destructor that are not inline, so code
-    //! instantiating the kernel::Context struct doesn't need to #include class
-    //! definitions for all the unique_ptr members.
     Context();
     ~Context();
 };
-
-//! Global pointer to kernel::Context for legacy code. New code should avoid
-//! using this, and require state it needs to be passed to it directly.
-//!
-//! Having this pointer is useful because it allows state be moved out of global
-//! variables into the kernel::Context struct before all global references to
-//! that state are removed. This allows the global references to be removed
-//! incrementally, instead of all at once.
-extern Context* g_context;
 } // namespace kernel
 
 #endif // BITCOIN_KERNEL_CONTEXT_H
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index d1c483d3a801d..209e2be7204a3 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -39,13 +39,13 @@
 #include <primitives/transaction.h>
 #include <rpc/protocol.h>
 #include <rpc/server.h>
-#include <shutdown.h>
 #include <support/allocators/secure.h>
 #include <sync.h>
 #include <txmempool.h>
 #include <uint256.h>
 #include <univalue.h>
 #include <util/check.h>
+#include <util/signalinterrupt.h>
 #include <util/translation.h>
 #include <validation.h>
 #include <validationinterface.h>
@@ -99,7 +99,6 @@ class NodeImpl : public Node
         if (!AppInitParameterInteraction(args())) return false;
 
         m_context->kernel = std::make_unique<kernel::Context>();
-        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;
@@ -121,14 +120,16 @@ class NodeImpl : public Node
     }
     void startShutdown() override
     {
-        StartShutdown();
+        if (!(*Assert(Assert(m_context)->shutdown))()) {
+            LogPrintf("Error: 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();
         }
     }
-    bool shutdownRequested() override { return ShutdownRequested(); }
+    bool shutdownRequested() override { return ShutdownRequested(*Assert(m_context)); };
     bool isSettingIgnored(const std::string& name) override
     {
         bool ignored = false;
@@ -747,7 +748,7 @@ class ChainImpl : public Chain
     {
         return chainman().IsInitialBlockDownload();
     }
-    bool shutdownRequested() override { return ShutdownRequested(); }
+    bool shutdownRequested() override { return ShutdownRequested(m_node); }
     void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
     void initWarning(const bilingual_str& message) override { InitWarning(message); }
     void initError(const bilingual_str& message) override { InitError(message); }
diff --git a/src/shutdown.cpp b/src/shutdown.cpp
deleted file mode 100644
index 9e0b2693e5b3e..0000000000000
--- a/src/shutdown.cpp
+++ /dev/null
@@ -1,43 +0,0 @@
-// Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2022 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#include <shutdown.h>
-
-#include <kernel/context.h>
-#include <logging.h>
-#include <util/check.h>
-#include <util/signalinterrupt.h>
-
-#include <assert.h>
-#include <system_error>
-
-void StartShutdown()
-{
-    if (!Assert(kernel::g_context)->interrupt()) {
-        LogPrintf("Sending shutdown token failed\n");
-        assert(0);
-    }
-}
-
-void AbortShutdown()
-{
-    if (!Assert(kernel::g_context)->interrupt.reset()) {
-        LogPrintf("Reading shutdown token failed\n");
-        assert(0);
-    }
-}
-
-bool ShutdownRequested()
-{
-    return bool{Assert(kernel::g_context)->interrupt};
-}
-
-void WaitForShutdown()
-{
-    if (!Assert(kernel::g_context)->interrupt.wait()) {
-        LogPrintf("Reading shutdown token failed\n");
-        assert(0);
-    }
-}
diff --git a/src/shutdown.h b/src/shutdown.h
deleted file mode 100644
index 2d6ace8d9368f..0000000000000
--- a/src/shutdown.h
+++ /dev/null
@@ -1,25 +0,0 @@
-// Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2021 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#ifndef BITCOIN_SHUTDOWN_H
-#define BITCOIN_SHUTDOWN_H
-
-/** Request shutdown of the application. */
-void StartShutdown();
-
-/** Clear shutdown flag. Only use this during init (before calling WaitForShutdown in any
- * thread), or in the unit tests. Calling it in other circumstances will cause a race condition.
- */
-void AbortShutdown();
-
-/** Returns true if a shutdown is requested, false otherwise. */
-bool ShutdownRequested();
-
-/** Wait for StartShutdown to be called in any thread. This can only be used
- * from a single thread.
- */
-void WaitForShutdown();
-
-#endif // BITCOIN_SHUTDOWN_H
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index acabdb8509d4a..985b815a3a242 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -40,7 +40,6 @@
 #include <rpc/server.h>
 #include <scheduler.h>
 #include <script/sigcache.h>
-#include <shutdown.h>
 #include <streams.h>
 #include <test/util/net.h>
 #include <test/util/random.h>