From 263b23f0082c60516acced1b03abb8e4d8f9ee46 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Remove call to ShutdownRequested from chainstate init

Use chainman.m_interrupt object instead

There is no change in behavior in this commit
---
 src/bitcoin-chainstate.cpp | 1 -
 src/init.cpp               | 1 -
 src/node/chainstate.cpp    | 5 +++--
 src/node/chainstate.h      | 1 -
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 31edb86ce2..f241bdae4d 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -132,7 +132,6 @@ int main(int argc, char* argv[])
     cache_sizes.coins_db = 2 << 22;
     cache_sizes.coins = (450 << 20) - (2 << 20) - (2 << 22);
     node::ChainstateLoadOptions options;
-    options.check_interrupt = [] { return false; };
     auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options);
     if (status != node::ChainstateLoadStatus::SUCCESS) {
         std::cerr << "Failed to load Chain state from your datadir." << std::endl;
diff --git a/src/init.cpp b/src/init.cpp
index ac52b34fc5..8bff75ec91 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1515,7 +1515,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."),
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index eb1994177a..bf1fc06b0b 100644
--- a/src/node/chainstate.cpp
+++ b/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")};
     }
 
diff --git a/src/node/chainstate.h b/src/node/chainstate.h
index 2e35035c28..a6e9a0331b 100644
--- a/src/node/chainstate.h
+++ b/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;
 };
 
-- 
cgit v1.2.3


From f0c73c1336bee74fe2d58474ac36bca28c219e85 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Remove call to ShutdownRequested from rpc/mining

Use chainman.m_interrupt object instead

There is no change in behavior in this commit
---
 src/rpc/mining.cpp | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 3d80656507..15a7792c36 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -27,7 +27,6 @@
 #include <script/descriptor.h>
 #include <script/script.h>
 #include <script/signingprovider.h>
-#include <shutdown.h>
 #include <timedata.h>
 #include <txmempool.h>
 #include <univalue.h>
@@ -129,11 +128,11 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
     block_out.reset();
     block.hashMerkleRoot = BlockMerkleRoot(block);
 
-    while (max_tries > 0 && block.nNonce < std::numeric_limits<uint32_t>::max() && !CheckProofOfWork(block.GetHash(), block.nBits, chainman.GetConsensus()) && !ShutdownRequested()) {
+    while (max_tries > 0 && block.nNonce < std::numeric_limits<uint32_t>::max() && !CheckProofOfWork(block.GetHash(), block.nBits, chainman.GetConsensus()) && !chainman.m_interrupt) {
         ++block.nNonce;
         --max_tries;
     }
-    if (max_tries == 0 || ShutdownRequested()) {
+    if (max_tries == 0 || chainman.m_interrupt) {
         return false;
     }
     if (block.nNonce == std::numeric_limits<uint32_t>::max()) {
@@ -154,7 +153,7 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
 static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& mempool, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
 {
     UniValue blockHashes(UniValue::VARR);
-    while (nGenerate > 0 && !ShutdownRequested()) {
+    while (nGenerate > 0 && !chainman.m_interrupt) {
         std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler{chainman.ActiveChainstate(), &mempool}.CreateNewBlock(coinbase_script));
         if (!pblocktemplate.get())
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
-- 
cgit v1.2.3


From f4a8bd6e2f03e786a84dd7763d1c04665e6371f2 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Remove call to StartShutdown from qt

Use interfaces::Node object instead.

There is a minor change in behavior in this commit, because the new code calls
InterruptRPC() and StopRPC() when previous code did not do this.  But this
should be a good thing since it makes sense to interrupt RPC when the system is
shutting down, and it is better for the GUI shut down in a consistent way
regardless of how the shutdown is triggered.
---
 src/qt/bitcoin.cpp            | 5 ++++-
 src/qt/bitcoingui.h           | 2 +-
 src/qt/test/apptests.cpp      | 1 -
 src/qt/winshutdownmonitor.cpp | 3 +--
 src/qt/winshutdownmonitor.h   | 6 ++++++
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index a9a9b39bf4..33c305f0d4 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -661,7 +661,10 @@ int GuiMain(int argc, char* argv[])
     app.installEventFilter(new GUIUtil::LabelOutOfFocusEventFilter(&app));
 #if defined(Q_OS_WIN)
     // Install global event filter for processing Windows session related Windows messages (WM_QUERYENDSESSION and WM_ENDSESSION)
-    qApp->installNativeEventFilter(new WinShutdownMonitor());
+    // Note: it is safe to call app.node() in the lambda below despite the fact
+    // that app.createNode() hasn't been called yet, because native events will
+    // not be processed until the Qt event loop is executed.
+    qApp->installNativeEventFilter(new WinShutdownMonitor([&app] { app.node().startShutdown(); }));
 #endif
     // Install qDebug() message handler to route to debug.log
     qInstallMessageHandler(DebugMessageHandler);
diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h
index 6fdc4c60d8..ba91eeb1ff 100644
--- a/src/qt/bitcoingui.h
+++ b/src/qt/bitcoingui.h
@@ -315,7 +315,7 @@ public Q_SLOTS:
     /** Simply calls showNormalIfMinimized(true) */
     void toggleHidden();
 
-    /** called by a timer to check if ShutdownRequested() has been set **/
+    /** called by a timer to check if shutdown has been requested */
     void detectShutdown();
 
     /** Show progress dialog e.g. for verifychain */
diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
index b05009965f..9007b183aa 100644
--- a/src/qt/test/apptests.cpp
+++ b/src/qt/test/apptests.cpp
@@ -11,7 +11,6 @@
 #include <qt/bitcoingui.h>
 #include <qt/networkstyle.h>
 #include <qt/rpcconsole.h>
-#include <shutdown.h>
 #include <test/util/setup_common.h>
 #include <validation.h>
 
diff --git a/src/qt/winshutdownmonitor.cpp b/src/qt/winshutdownmonitor.cpp
index 97a9ec318c..0b5278c192 100644
--- a/src/qt/winshutdownmonitor.cpp
+++ b/src/qt/winshutdownmonitor.cpp
@@ -5,7 +5,6 @@
 #include <qt/winshutdownmonitor.h>
 
 #if defined(Q_OS_WIN)
-#include <shutdown.h>
 
 #include <windows.h>
 
@@ -25,7 +24,7 @@ bool WinShutdownMonitor::nativeEventFilter(const QByteArray &eventType, void *pM
            {
                // Initiate a client shutdown after receiving a WM_QUERYENDSESSION and block
                // Windows session end until we have finished client shutdown.
-               StartShutdown();
+               m_shutdown_fn();
                *pnResult = FALSE;
                return true;
            }
diff --git a/src/qt/winshutdownmonitor.h b/src/qt/winshutdownmonitor.h
index 72655da3da..78f287637f 100644
--- a/src/qt/winshutdownmonitor.h
+++ b/src/qt/winshutdownmonitor.h
@@ -8,6 +8,7 @@
 #ifdef WIN32
 #include <QByteArray>
 #include <QString>
+#include <functional>
 
 #include <windef.h> // for HWND
 
@@ -16,11 +17,16 @@
 class WinShutdownMonitor : public QAbstractNativeEventFilter
 {
 public:
+    WinShutdownMonitor(std::function<void()> shutdown_fn) : m_shutdown_fn{std::move(shutdown_fn)} {}
+
     /** Implements QAbstractNativeEventFilter interface for processing Windows messages */
     bool nativeEventFilter(const QByteArray &eventType, void *pMessage, long *pnResult) override;
 
     /** Register the reason for blocking shutdown on Windows to allow clean client exit */
     static void registerShutdownBlockReason(const QString& strReason, const HWND& mainWinId);
+
+private:
+    std::function<void()> m_shutdown_fn;
 };
 #endif
 
-- 
cgit v1.2.3


From 73133c36aa9cc09546eabac18d0ea35274dd5d72 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Add NodeContext::shutdown member

Add NodeContext::shutdown variable and start using it to replace the
kernel::Context::interrupt variable. The latter can't easily be removed right
away but will be removed later in this PR.

Moving the interrupt object from the kernel context to the node context
increases flexibility of the kernel API so it is possible to use multiple
interrupt objects, or avoid creating one if one is not needed. It will also
allow getting rid of the kernel::g_context global later in this PR, replacing
it with a private SignalInterrupt instance in init.cpp

There is no change in behavior in this commit outside of unit tests. In unit
tests there should be no visible change either, but internally now each test
has its own interrupt variable so the variable will be automatically reset
between tests.
---
 src/bitcoin-chainstate.cpp                      | 3 ++-
 src/bitcoind.cpp                                | 1 +
 src/init.cpp                                    | 2 +-
 src/node/context.h                              | 2 ++
 src/node/interfaces.cpp                         | 1 +
 src/test/blockmanager_tests.cpp                 | 2 +-
 src/test/util/setup_common.cpp                  | 3 ++-
 src/test/util/setup_common.h                    | 1 +
 src/test/validation_chainstatemanager_tests.cpp | 2 +-
 9 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index f241bdae4d..ee8b0a44c5 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -125,7 +125,8 @@ int main(int argc, char* argv[])
         .blocks_dir = abs_datadir / "blocks",
         .notifications = chainman_opts.notifications,
     };
-    ChainstateManager chainman{kernel_context.interrupt, chainman_opts, blockman_opts};
+    util::SignalInterrupt interrupt;
+    ChainstateManager chainman{interrupt, chainman_opts, blockman_opts};
 
     node::CacheSizes cache_sizes;
     cache_sizes.block_tree_db = 2 << 20;
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index e2224befef..ccdc628a0f 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -185,6 +185,7 @@ 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
diff --git a/src/init.cpp b/src/init.cpp
index 8bff75ec91..dfc05ebb67 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1485,7 +1485,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
         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
diff --git a/src/node/context.h b/src/node/context.h
index dae900ff7f..4f3b640b2d 100644
--- a/src/node/context.h
+++ b/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;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index b5691f0e57..088497399a 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -99,6 +99,7 @@ public:
         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;
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
index e1b31c20f4..8a777ee5ff 100644
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -33,7 +33,7 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
         .blocks_dir = m_args.GetBlocksDirPath(),
         .notifications = notifications,
     };
-    BlockManager blockman{m_node.kernel->interrupt, blockman_opts};
+    BlockManager blockman{*Assert(m_node.shutdown), blockman_opts};
     // simulate adding a genesis block normally
     BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE);
     // simulate what happens during reindex
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index bc639da4dd..4b700b18d6 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -102,6 +102,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vecto
     : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
       m_args{}
 {
+    m_node.shutdown = &m_interrupt;
     m_node.args = &gArgs;
     std::vector<const char*> arguments = Cat(
         {
@@ -194,7 +195,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
         .blocks_dir = m_args.GetBlocksDirPath(),
         .notifications = chainman_opts.notifications,
     };
-    m_node.chainman = std::make_unique<ChainstateManager>(m_node.kernel->interrupt, chainman_opts, blockman_opts);
+    m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown), chainman_opts, blockman_opts);
     m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<BlockTreeDB>(DBParams{
         .path = m_args.GetDataDirNet() / "blocks" / "index",
         .cache_bytes = static_cast<size_t>(m_cache_sizes.block_tree_db),
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index c3a4df28bf..9ff4c372a5 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -47,6 +47,7 @@ static constexpr CAmount CENT{1000000};
  * This just configures logging, data dir and chain parameters.
  */
 struct BasicTestingSetup {
+    util::SignalInterrupt m_interrupt;
     node::NodeContext m_node; // keep as first member to be destructed last
 
     explicit BasicTestingSetup(const ChainType chainType = ChainType::MAIN, const std::vector<const char*>& extra_args = {});
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6969822ad7..6c81cd618b 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -394,7 +394,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<ChainstateManager>(m_node.kernel->interrupt, chainman_opts, blockman_opts);
+            m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown), chainman_opts, blockman_opts);
         }
         return *Assert(m_node.chainman);
     }
-- 
cgit v1.2.3


From 42e5829d9710ebebda5de356fab01dd7c149d5fa Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Remove call to ShutdownRequested from HTTPRequest

Pass HTTP server an interrupt object instead of having it depend on shutdown.h
and global shutdown state.

There is no change in behavior in this commit.
---
 src/httpserver.cpp             | 13 +++++++------
 src/httpserver.h               |  9 +++++++--
 src/init.cpp                   |  3 ++-
 src/test/fuzz/http_request.cpp |  4 +++-
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 4c14c5b939..e26ea6a596 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -15,9 +15,9 @@
 #include <netbase.h>
 #include <node/interface_ui.h>
 #include <rpc/protocol.h> // For HTTP status codes
-#include <shutdown.h>
 #include <sync.h>
 #include <util/check.h>
+#include <util/signalinterrupt.h>
 #include <util/strencodings.h>
 #include <util/threadnames.h>
 #include <util/translation.h>
@@ -284,7 +284,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
             }
         }
     }
-    std::unique_ptr<HTTPRequest> hreq(new HTTPRequest(req));
+    auto hreq{std::make_unique<HTTPRequest>(req, *static_cast<const util::SignalInterrupt*>(arg))};
 
     // Early address-based allow check
     if (!ClientAllowed(hreq->GetPeer())) {
@@ -425,7 +425,7 @@ static void libevent_log_cb(int severity, const char *msg)
     LogPrintLevel(BCLog::LIBEVENT, level, "%s\n", msg);
 }
 
-bool InitHTTPServer()
+bool InitHTTPServer(const util::SignalInterrupt& interrupt)
 {
     if (!InitHTTPAllowList())
         return false;
@@ -454,7 +454,7 @@ bool InitHTTPServer()
     evhttp_set_timeout(http, gArgs.GetIntArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT));
     evhttp_set_max_headers_size(http, MAX_HEADERS_SIZE);
     evhttp_set_max_body_size(http, MAX_SIZE);
-    evhttp_set_gencb(http, http_request_cb, nullptr);
+    evhttp_set_gencb(http, http_request_cb, (void*)&interrupt);
 
     if (!HTTPBindAddresses(http)) {
         LogPrintf("Unable to bind any endpoint for RPC server\n");
@@ -579,7 +579,8 @@ void HTTPEvent::trigger(struct timeval* tv)
     else
         evtimer_add(ev, tv); // trigger after timeval passed
 }
-HTTPRequest::HTTPRequest(struct evhttp_request* _req, bool _replySent) : req(_req), replySent(_replySent)
+HTTPRequest::HTTPRequest(struct evhttp_request* _req, const util::SignalInterrupt& interrupt, bool _replySent)
+    : req(_req), m_interrupt(interrupt), replySent(_replySent)
 {
 }
 
@@ -639,7 +640,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
 void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
 {
     assert(!replySent && req);
-    if (ShutdownRequested()) {
+    if (m_interrupt) {
         WriteHeader("Connection", "close");
     }
     // Send event to main http thread to send reply message
diff --git a/src/httpserver.h b/src/httpserver.h
index 036a39a023..9a49877f09 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -9,6 +9,10 @@
 #include <optional>
 #include <string>
 
+namespace util {
+class SignalInterrupt;
+} // namespace util
+
 static const int DEFAULT_HTTP_THREADS=4;
 static const int DEFAULT_HTTP_WORKQUEUE=16;
 static const int DEFAULT_HTTP_SERVER_TIMEOUT=30;
@@ -21,7 +25,7 @@ class HTTPRequest;
 /** Initialize HTTP server.
  * Call this before RegisterHTTPHandler or EventBase().
  */
-bool InitHTTPServer();
+bool InitHTTPServer(const util::SignalInterrupt& interrupt);
 /** Start HTTP server.
  * This is separate from InitHTTPServer to give users race-condition-free time
  * to register their handlers between InitHTTPServer and StartHTTPServer.
@@ -57,10 +61,11 @@ class HTTPRequest
 {
 private:
     struct evhttp_request* req;
+    const util::SignalInterrupt& m_interrupt;
     bool replySent;
 
 public:
-    explicit HTTPRequest(struct evhttp_request* req, bool replySent = false);
+    explicit HTTPRequest(struct evhttp_request* req, const util::SignalInterrupt& interrupt, bool replySent = false);
     ~HTTPRequest();
 
     enum RequestMethod {
diff --git a/src/init.cpp b/src/init.cpp
index dfc05ebb67..1c4c79aa86 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -690,8 +690,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))
diff --git a/src/test/fuzz/http_request.cpp b/src/test/fuzz/http_request.cpp
index 9928c4a1ab..f13f1c72a5 100644
--- a/src/test/fuzz/http_request.cpp
+++ b/src/test/fuzz/http_request.cpp
@@ -7,6 +7,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/signalinterrupt.h>
 #include <util/strencodings.h>
 
 #include <event2/buffer.h>
@@ -47,7 +48,8 @@ FUZZ_TARGET(http_request)
         return;
     }
 
-    HTTPRequest http_request{evreq, true};
+    util::SignalInterrupt interrupt;
+    HTTPRequest http_request{evreq, interrupt, true};
     const HTTPRequest::RequestMethod request_method = http_request.GetRequestMethod();
     (void)RequestMethodString(request_method);
     (void)http_request.GetURI();
-- 
cgit v1.2.3


From ba93966368d3aaa426b97837ef475ec5aa612f5f Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Remove call to ShutdownRequested from IndexWaitSynced

Use the node interrupt object instead.

There is no change in behavior in this commit.
---
 src/test/blockfilter_index_tests.cpp | 2 +-
 src/test/coinstatsindex_tests.cpp    | 4 ++--
 src/test/txindex_tests.cpp           | 2 +-
 src/test/util/index.cpp              | 6 +++---
 src/test/util/index.h                | 5 ++++-
 5 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
index 97ea5cfbf3..a9009948ee 100644
--- a/src/test/blockfilter_index_tests.cpp
+++ b/src/test/blockfilter_index_tests.cpp
@@ -143,7 +143,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);
+    IndexWaitSynced(filter_index, *Assert(m_node.shutdown));
 
     // Check that filter index has all blocks that were in the chain before it started.
     {
diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
index 50f3f7d833..cc1ec49d41 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);
+    IndexWaitSynced(coin_stats_index, *Assert(m_node.shutdown));
 
     // 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);
+        IndexWaitSynced(index, *Assert(m_node.shutdown));
         std::shared_ptr<const CBlock> new_block;
         CBlockIndex* new_block_index = nullptr;
         {
diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp
index 9fa59bab57..e2432a4718 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);
+    IndexWaitSynced(txindex, *Assert(m_node.shutdown));
 
     // Check that txindex excludes genesis block transactions.
     const CBlock& genesis_block = Params().GenesisBlock();
diff --git a/src/test/util/index.cpp b/src/test/util/index.cpp
index e653d5dbf0..cfeba35756 100644
--- a/src/test/util/index.cpp
+++ b/src/test/util/index.cpp
@@ -5,16 +5,16 @@
 #include <test/util/index.h>
 
 #include <index/base.h>
-#include <shutdown.h>
 #include <util/check.h>
+#include <util/signalinterrupt.h>
 #include <util/time.h>
 
-void IndexWaitSynced(const BaseIndex& index)
+void IndexWaitSynced(const BaseIndex& index, const util::SignalInterrupt& interrupt)
 {
     while (!index.BlockUntilSyncedToCurrentChain()) {
         // Assert shutdown was not requested to abort the test, instead of looping forever, in case
         // there was an unexpected error in the index that caused it to stop syncing and request a shutdown.
-        Assert(!ShutdownRequested());
+        Assert(!interrupt);
 
         UninterruptibleSleep(100ms);
     }
diff --git a/src/test/util/index.h b/src/test/util/index.h
index 95309f6273..a3bd1dddc3 100644
--- a/src/test/util/index.h
+++ b/src/test/util/index.h
@@ -6,8 +6,11 @@
 #define BITCOIN_TEST_UTIL_INDEX_H
 
 class BaseIndex;
+namespace util {
+class SignalInterrupt;
+} // namespace util
 
 /** Block until the index is synced to the current chain */
-void IndexWaitSynced(const BaseIndex& index);
+void IndexWaitSynced(const BaseIndex& index, const util::SignalInterrupt& interrupt);
 
 #endif // BITCOIN_TEST_UTIL_INDEX_H
-- 
cgit v1.2.3


From 1d92d89edbb1812dc353084c62772ebb1024d632 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: util: Get rid of uncaught exceptions thrown by SignalInterrupt class

Replace exceptions thrown by signal and wait methods with [[nodiscard]] return
values.

This is mostly a refactoring, but there is a slight change of behavior if
AbortShutdown function fails. The original behavior which was unintentionally
changed in #27861 is restored, so it now triggers an assert failure again
instead of throwing an exception. (The AbortShutdown function is only ever
called in the the GUI version of Bitcoin Core when corruption is detected on
loading and the user tries to reindex.)

Problems with using exceptions were pointed out by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.
---
 src/shutdown.cpp             | 13 ++++++-------
 src/util/signalinterrupt.cpp | 15 +++++++++------
 src/util/signalinterrupt.h   |  6 +++---
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/shutdown.cpp b/src/shutdown.cpp
index fc18ccd207..9e0b2693e5 100644
--- a/src/shutdown.cpp
+++ b/src/shutdown.cpp
@@ -15,9 +15,7 @@
 
 void StartShutdown()
 {
-    try {
-        Assert(kernel::g_context)->interrupt();
-    } catch (const std::system_error&) {
+    if (!Assert(kernel::g_context)->interrupt()) {
         LogPrintf("Sending shutdown token failed\n");
         assert(0);
     }
@@ -25,7 +23,10 @@ void StartShutdown()
 
 void AbortShutdown()
 {
-    Assert(kernel::g_context)->interrupt.reset();
+    if (!Assert(kernel::g_context)->interrupt.reset()) {
+        LogPrintf("Reading shutdown token failed\n");
+        assert(0);
+    }
 }
 
 bool ShutdownRequested()
@@ -35,9 +36,7 @@ bool ShutdownRequested()
 
 void WaitForShutdown()
 {
-    try {
-        Assert(kernel::g_context)->interrupt.wait();
-    } catch (const std::system_error&) {
+    if (!Assert(kernel::g_context)->interrupt.wait()) {
         LogPrintf("Reading shutdown token failed\n");
         assert(0);
     }
diff --git a/src/util/signalinterrupt.cpp b/src/util/signalinterrupt.cpp
index c551ba8044..f36f3dbf9e 100644
--- a/src/util/signalinterrupt.cpp
+++ b/src/util/signalinterrupt.cpp
@@ -30,15 +30,16 @@ SignalInterrupt::operator bool() const
     return m_flag;
 }
 
-void SignalInterrupt::reset()
+bool SignalInterrupt::reset()
 {
     // Cancel existing interrupt by waiting for it, this will reset condition flags and remove
     // the token from the pipe.
-    if (*this) wait();
+    if (*this && !wait()) return false;
     m_flag = false;
+    return true;
 }
 
-void SignalInterrupt::operator()()
+bool SignalInterrupt::operator()()
 {
 #ifdef WIN32
     std::unique_lock<std::mutex> lk(m_mutex);
@@ -52,13 +53,14 @@ void SignalInterrupt::operator()()
         // Write an arbitrary byte to the write end of the pipe.
         int res = m_pipe_w.TokenWrite('x');
         if (res != 0) {
-            throw std::ios_base::failure("Could not write interrupt token");
+            return false;
         }
     }
 #endif
+    return true;
 }
 
-void SignalInterrupt::wait()
+bool SignalInterrupt::wait()
 {
 #ifdef WIN32
     std::unique_lock<std::mutex> lk(m_mutex);
@@ -66,9 +68,10 @@ void SignalInterrupt::wait()
 #else
     int res = m_pipe_r.TokenRead();
     if (res != 'x') {
-        throw std::ios_base::failure("Did not read expected interrupt token");
+        return false;
     }
 #endif
+    return true;
 }
 
 } // namespace util
diff --git a/src/util/signalinterrupt.h b/src/util/signalinterrupt.h
index ca02feda91..605d124206 100644
--- a/src/util/signalinterrupt.h
+++ b/src/util/signalinterrupt.h
@@ -30,9 +30,9 @@ class SignalInterrupt
 public:
     SignalInterrupt();
     explicit operator bool() const;
-    void operator()();
-    void reset();
-    void wait();
+    [[nodiscard]] bool operator()();
+    [[nodiscard]] bool reset();
+    [[nodiscard]] bool wait();
 
 private:
     std::atomic<bool> m_flag;
-- 
cgit v1.2.3


From 6824eecaf1e74624cf149ed20abd9145c49d614a Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Remove call to StartShutdown from stop RPC

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 returns an RPC error instead.
---
 src/rpc/server.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 7adc28f416..9fa5ea7b0f 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -8,9 +8,11 @@
 #include <common/args.h>
 #include <common/system.h>
 #include <logging.h>
+#include <node/context.h>
+#include <rpc/server_util.h>
 #include <rpc/util.h>
-#include <shutdown.h>
 #include <sync.h>
+#include <util/signalinterrupt.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/time.h>
@@ -179,7 +181,7 @@ static RPCHelpMan stop()
 {
     // Event loop will exit after current HTTP requests have been handled, so
     // this reply will get back to the client.
-    StartShutdown();
+    CHECK_NONFATAL((*CHECK_NONFATAL(EnsureAnyNodeContext(jsonRequest.context).shutdown))());
     if (jsonRequest.params[0].isNum()) {
         UninterruptibleSleep(std::chrono::milliseconds{jsonRequest.params[0].getInt<int>()});
     }
-- 
cgit v1.2.3


From feeb7b816affa790e02e7ba0780c4ef33d2310ff Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: 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.
---
 src/index/base.cpp                              |  3 +--
 src/init.cpp                                    |  2 +-
 src/node/abort.cpp                              |  8 +++++---
 src/node/abort.h                                |  6 +++++-
 src/node/kernel_notifications.cpp               | 10 ++++++----
 src/node/kernel_notifications.h                 |  7 ++++++-
 src/test/blockmanager_tests.cpp                 |  6 +++---
 src/test/util/setup_common.cpp                  |  2 +-
 src/test/validation_chainstatemanager_tests.cpp |  2 +-
 9 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/src/index/base.cpp b/src/index/base.cpp
index 8474d01c41..bcfe7215be 100644
--- a/src/index/base.cpp
+++ b/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)
diff --git a/src/init.cpp b/src/init.cpp
index 1c4c79aa86..11679f2e4a 100644
--- a/src/init.cpp
+++ b/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);
diff --git a/src/node/abort.cpp b/src/node/abort.cpp
index a554126b86..1bdc91670d 100644
--- a/src/node/abort.cpp
+++ b/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
diff --git a/src/node/abort.h b/src/node/abort.h
index d6bb0c14d5..28d021cc78 100644
--- a/src/node/abort.h
+++ b/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
diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp
index 7224127c72..1fd3bad296 100644
--- a/src/node/kernel_notifications.cpp
+++ b/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)
diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h
index b2dfc03398..38d8600ac6 100644
--- a/src/node/kernel_notifications.h
+++ b/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;
 };
 
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
index 8a777ee5ff..d7ac0bf823 100644
--- a/src/test/blockmanager_tests.cpp
+++ b/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;
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 4b700b18d6..a111ea5997 100644
--- a/src/test/util/setup_common.cpp
+++ b/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,
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6c81cd618b..368ba8bee4 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/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,
-- 
cgit v1.2.3


From 213542b625a6a4885fcbdfe236629a5f381eeb05 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: refactor: Add InitContext function to initialize NodeContext with
 global pointers

Having InitContext() avoids the need to add duplicate code to src/init/*.cpp
files in the next commit. It also lets these files avoid referencing global
variables like gArgs.

There is no change in behavior in this commit.
---
 src/init.cpp              | 4 ++++
 src/init.h                | 3 +++
 src/init/bitcoin-gui.cpp  | 4 ++--
 src/init/bitcoin-node.cpp | 4 ++--
 src/init/bitcoin-qt.cpp   | 4 ++--
 src/init/bitcoind.cpp     | 4 ++--
 6 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/init.cpp b/src/init.cpp
index 11679f2e4a..cbef3c7db8 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -192,6 +192,10 @@ static void RemovePidFile(const ArgsManager& args)
     }
 }
 
+void InitContext(NodeContext& node)
+{
+    node.args = &gArgs;
+}
 
 //////////////////////////////////////////////////////////////////////////////
 //
diff --git a/src/init.h b/src/init.h
index f27d6120ef..da8255cda3 100644
--- a/src/init.h
+++ b/src/init.h
@@ -26,6 +26,9 @@ namespace node {
 struct NodeContext;
 } // namespace node
 
+/** Initialize node context variables. */
+void InitContext(node::NodeContext& node);
+
 /** Interrupt threads */
 void Interrupt(node::NodeContext& node);
 void Shutdown(node::NodeContext& node);
diff --git a/src/init/bitcoin-gui.cpp b/src/init/bitcoin-gui.cpp
index ddbdaa6cd0..aceff1e40f 100644
--- a/src/init/bitcoin-gui.cpp
+++ b/src/init/bitcoin-gui.cpp
@@ -2,7 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <common/args.h>
+#include <init.h>
 #include <interfaces/chain.h>
 #include <interfaces/echo.h>
 #include <interfaces/init.h>
@@ -23,7 +23,7 @@ class BitcoinGuiInit : public interfaces::Init
 public:
     BitcoinGuiInit(const char* arg0) : m_ipc(interfaces::MakeIpc(EXE_NAME, arg0, *this))
     {
-        m_node.args = &gArgs;
+        InitContext(m_node);
         m_node.init = this;
     }
     std::unique_ptr<interfaces::Node> makeNode() override { return interfaces::MakeNode(m_node); }
diff --git a/src/init/bitcoin-node.cpp b/src/init/bitcoin-node.cpp
index b04596f986..97b8dc1161 100644
--- a/src/init/bitcoin-node.cpp
+++ b/src/init/bitcoin-node.cpp
@@ -2,7 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <common/args.h>
+#include <init.h>
 #include <interfaces/chain.h>
 #include <interfaces/echo.h>
 #include <interfaces/init.h>
@@ -25,7 +25,7 @@ public:
         : m_node(node),
           m_ipc(interfaces::MakeIpc(EXE_NAME, arg0, *this))
     {
-        m_node.args = &gArgs;
+        InitContext(m_node);
         m_node.init = this;
     }
     std::unique_ptr<interfaces::Node> makeNode() override { return interfaces::MakeNode(m_node); }
diff --git a/src/init/bitcoin-qt.cpp b/src/init/bitcoin-qt.cpp
index dd5826d982..3003a8fde1 100644
--- a/src/init/bitcoin-qt.cpp
+++ b/src/init/bitcoin-qt.cpp
@@ -2,7 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <common/args.h>
+#include <init.h>
 #include <interfaces/chain.h>
 #include <interfaces/echo.h>
 #include <interfaces/init.h>
@@ -20,7 +20,7 @@ class BitcoinQtInit : public interfaces::Init
 public:
     BitcoinQtInit()
     {
-        m_node.args = &gArgs;
+        InitContext(m_node);
         m_node.init = this;
     }
     std::unique_ptr<interfaces::Node> makeNode() override { return interfaces::MakeNode(m_node); }
diff --git a/src/init/bitcoind.cpp b/src/init/bitcoind.cpp
index 210608370c..b5df764017 100644
--- a/src/init/bitcoind.cpp
+++ b/src/init/bitcoind.cpp
@@ -2,7 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <common/args.h>
+#include <init.h>
 #include <interfaces/chain.h>
 #include <interfaces/echo.h>
 #include <interfaces/init.h>
@@ -22,7 +22,7 @@ class BitcoindInit : public interfaces::Init
 public:
     BitcoindInit(NodeContext& node) : m_node(node)
     {
-        m_node.args = &gArgs;
+        InitContext(m_node);
         m_node.init = this;
     }
     std::unique_ptr<interfaces::Node> makeNode() override { return interfaces::MakeNode(m_node); }
-- 
cgit v1.2.3


From 6db04be102807ee0120981a9b8de62a55439dabb Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 17:32:54 -0400
Subject: 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 df56ac7090..5e7ca6788b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -271,7 +271,6 @@ BITCOIN_CORE_H = \
   script/sign.h \
   script/signingprovider.h \
   script/solver.h \
-  shutdown.h \
   signet.h \
   streams.h \
   support/allocators/pool.h \
@@ -459,7 +458,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 ccdc628a0f..4f0a816388 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 cbef3c7db8..a48cc1b162 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,9 +191,15 @@ 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;
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -208,11 +213,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 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.
@@ -222,6 +225,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)
 {
@@ -386,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)
@@ -396,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;
 }
@@ -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});
 
@@ -1487,7 +1502,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);
@@ -1554,7 +1569,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(
@@ -1563,7 +1578,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;
@@ -1577,7 +1594,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;
     }
@@ -1698,7 +1715,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;
         }
 
@@ -1718,16 +1737,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 da8255cda3..ead5f5e0d2 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 3f4a423531..c60f1638d1 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 e74e0a6f08..ff4df20473 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 088497399a..2e547e8e2c 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 @@ public:
         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 @@ 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;
@@ -750,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); }
diff --git a/src/shutdown.cpp b/src/shutdown.cpp
deleted file mode 100644
index 9e0b2693e5..0000000000
--- 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 2d6ace8d93..0000000000
--- 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 a111ea5997..8789e86196 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>
-- 
cgit v1.2.3