Merge bitcoin/bitcoin#28051: Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly

6db04be102807ee0120981a9b8de62a55439dabb Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly (Ryan Ofsky)
213542b625a6a4885fcbdfe236629a5f381eeb05 refactor: Add InitContext function to initialize NodeContext with global pointers (Ryan Ofsky)
feeb7b816affa790e02e7ba0780c4ef33d2310ff refactor: Remove calls to StartShutdown from KernelNotifications (Ryan Ofsky)
6824eecaf1e74624cf149ed20abd9145c49d614a refactor: Remove call to StartShutdown from stop RPC (Ryan Ofsky)
1d92d89edbb1812dc353084c62772ebb1024d632 util: Get rid of uncaught exceptions thrown by SignalInterrupt class (Ryan Ofsky)
ba93966368d3aaa426b97837ef475ec5aa612f5f refactor: Remove call to ShutdownRequested from IndexWaitSynced (Ryan Ofsky)
42e5829d9710ebebda5de356fab01dd7c149d5fa refactor: Remove call to ShutdownRequested from HTTPRequest (Ryan Ofsky)
73133c36aa9cc09546eabac18d0ea35274dd5d72 refactor: Add NodeContext::shutdown member (Ryan Ofsky)
f4a8bd6e2f03e786a84dd7763d1c04665e6371f2 refactor: Remove call to StartShutdown from qt (Ryan Ofsky)
f0c73c1336bee74fe2d58474ac36bca28c219e85 refactor: Remove call to ShutdownRequested from rpc/mining (Ryan Ofsky)
263b23f0082c60516acced1b03abb8e4d8f9ee46 refactor: Remove call to ShutdownRequested from chainstate init (Ryan Ofsky)

Pull request description:

  This change drops `shutdown.h` and `shutdown.cpp` files, replacing them with a `NodeContext::shutdown` member which is used to trigger shutdowns directly. This gets rid of an unnecessary layer of indirection, and allows getting rid of the `kernel::g_context` global.

  Additionally, this PR tries to improve error handling of `SignalInterrupt` code by marking relevant methods `[[nodiscard]]` to avoid the possibility of uncaught exceptions mentioned https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.

  Behavior is changing In a few cases which are noted in individual commit messages. Particularly: GUI code more consistently interrupts RPCs when it is shutting down, shutdown state no longer persists between unit tests, the stop RPC now returns an RPC error if requesting shutdown fails instead of aborting, and other failed shutdown calls now log errors instead of aborting.

  This PR is a net reduction in lines of code, but in some cases the explicit error handling and lack of global shutdown functions do make it more verbose. The verbosity can be seen as good thing if it discourages more code from directly triggering shutdowns, and instead encourages code to return errors or send notifications that could be translated into shutdowns. Probably a number of existing shutdown calls could just be replaced by better error handling.

ACKs for top commit:
  achow101:
    ACK 6db04be102807ee0120981a9b8de62a55439dabb
  TheCharlatan:
    Re-ACK 6db04be102807ee0120981a9b8de62a55439dabb
  maflcko:
    ACK 6db04be102807ee0120981a9b8de62a55439dabb 👗
  stickies-v:
    re-ACK 6db04be102807ee0120981a9b8de62a55439dabb

Tree-SHA512: 7a34cb69085f37e813c43bdaded1a0cbf6c53bd95fdde96f0cb45346127fc934604c43bccd3328231ca2f1faf712a7418d047ceabd22ef2dca3c32ebb659e634

1 files changed, 45 insertions, 22 deletions

diff --git a/src/init.cpp b/src/init.cpp
index d1a0ca1c3b..481d5d398d 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>
@@ -192,6 +191,16 @@ static void RemovePidFile(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;
+}
 
 //////////////////////////////////////////////////////////////////////////////
 //
@@ -204,11 +213,9 @@ static void RemovePidFile(const ArgsManager& args)
 // 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 the 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.
@@ -218,6 +225,11 @@ static void RemovePidFile(const ArgsManager& args)
 // shutdown thing.
 //
 
+bool ShutdownRequested(node::NodeContext& node)
+{
+    return bool{*Assert(node.shutdown)};
+}
+
 #if HAVE_SYSTEM
 static void ShutdownNotify(const ArgsManager& args)
 {
@@ -382,7 +394,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)
@@ -392,7 +406,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;
 }
@@ -690,8 +707,9 @@ static bool AppInitServers(NodeContext& node)
     const ArgsManager& args = *Assert(node.args);
     RPCServer::OnStarted(&OnRPCStarted);
     RPCServer::OnStopped(&OnRPCStopped);
-    if (!InitHTTPServer())
+    if (!InitHTTPServer(*Assert(node.shutdown))) {
         return false;
+    }
     StartRPC();
     node.rpc_interruption_point = RpcInterruptionPoint;
     if (!StartHTTPRPC(&node))
@@ -1141,11 +1159,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});
 
@@ -1432,7 +1452,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);
@@ -1483,10 +1503,10 @@ 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>(node.kernel->interrupt, chainman_opts, blockman_opts);
+        node.chainman = std::make_unique<ChainstateManager>(*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
@@ -1516,7 +1536,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
         options.check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL);
         options.require_full_verification = args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel");
-        options.check_interrupt = ShutdownRequested;
         options.coins_error_cb = [] {
             uiInterface.ThreadSafeMessageBox(
                 _("Error reading from database, shutting down."),
@@ -1551,7 +1570,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(
@@ -1560,7 +1579,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;
@@ -1574,7 +1595,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;
     }
@@ -1695,7 +1716,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;
         }
 
@@ -1715,16 +1738,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;
     }