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

6db04be102 Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly (Ryan Ofsky)
213542b625 refactor: Add InitContext function to initialize NodeContext with global pointers (Ryan Ofsky)
feeb7b816a refactor: Remove calls to StartShutdown from KernelNotifications (Ryan Ofsky)
6824eecaf1 refactor: Remove call to StartShutdown from stop RPC (Ryan Ofsky)
1d92d89edb util: Get rid of uncaught exceptions thrown by SignalInterrupt class (Ryan Ofsky)
ba93966368 refactor: Remove call to ShutdownRequested from IndexWaitSynced (Ryan Ofsky)
42e5829d97 refactor: Remove call to ShutdownRequested from HTTPRequest (Ryan Ofsky)
73133c36aa refactor: Add NodeContext::shutdown member (Ryan Ofsky)
f4a8bd6e2f refactor: Remove call to StartShutdown from qt (Ryan Ofsky)
f0c73c1336 refactor: Remove call to ShutdownRequested from rpc/mining (Ryan Ofsky)
263b23f008 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 6db04be102
  TheCharlatan:
    Re-ACK 6db04be102
  maflcko:
    ACK 6db04be102 👗
  stickies-v:
    re-ACK 6db04be102

Tree-SHA512: 7a34cb69085f37e813c43bdaded1a0cbf6c53bd95fdde96f0cb45346127fc934604c43bccd3328231ca2f1faf712a7418d047ceabd22ef2dca3c32ebb659e634

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/chainstate.cpp

@@ -17,6 +17,7 @@
 #include <txdb.h>
 #include <uint256.h>
 #include <util/fs.h>
+#include <util/signalinterrupt.h>
 #include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
@@ -55,14 +56,14 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         }
     }
 
-    if (options.check_interrupt && options.check_interrupt()) return {ChainstateLoadStatus::INTERRUPTED, {}};
+    if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
 
     // LoadBlockIndex will load m_have_pruned if we've ever removed a
     // block file from disk.
     // Note that it also sets fReindex global based on the disk flag!
     // From here on, fReindex and options.reindex values may be different!
     if (!chainman.LoadBlockIndex()) {
-        if (options.check_interrupt && options.check_interrupt()) return {ChainstateLoadStatus::INTERRUPTED, {}};
+        if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
         return {ChainstateLoadStatus::FAILURE, _("Error loading block database")};
     }
 

src/node/chainstate.h

@@ -32,7 +32,6 @@ struct ChainstateLoadOptions {
     bool require_full_verification{true};
     int64_t check_blocks{DEFAULT_CHECKBLOCKS};
     int64_t check_level{DEFAULT_CHECKLEVEL};
-    std::function<bool()> check_interrupt;
     std::function<void()> coins_error_cb;
 };
 

src/node/context.h

@@ -50,6 +50,8 @@ struct NodeContext {
     std::unique_ptr<kernel::Context> 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> addrman;
     std::unique_ptr<CConnman> connman;
     std::unique_ptr<CTxMemPool> mempool;

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>
@@ -120,14 +120,16 @@ public:
     }
     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;
@@ -749,7 +751,7 @@ public:
     {
         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); }

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,6 +16,10 @@ class CBlockIndex;
 enum class SynchronizationState;
 struct bilingual_str;
 
+namespace util {
+class SignalInterrupt;
+} // namespace util
+
 namespace node {
 
 static constexpr int DEFAULT_STOPATHEIGHT{0};
@@ -23,7 +27,7 @@ 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 @@ public:
     //! 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;
 };