refactor: Split up NodeContext shutdown_signal and shutdown_request

Instead of having a single NodeContext::shutdown member that is used both to
request shutdowns and check if they have been requested, use separate members
for each. Benefits of this change:

1. Should make code a little clearer and easier to search because it is easier
   to see which parts of code are triggering shutdowns and which parts are just
   checking to see if they were triggered.

2. Makes it possible for init.cpp to specify additional code to run when a
   shutdown is requested, like signalling the m_tip_block_cv condition variable.

Motivation for this change was to remove hacky NodeContext argument and
m_tip_block_cv access from the StopRPC function, so StopRPC can just be
concerned with RPC functionality, not other node functionality.

Author: ryanofsky

src/bitcoind.cpp

@@ -274,7 +274,7 @@ MAIN_FUNCTION
     if (ProcessInitCommands(args)) return EXIT_SUCCESS;
 
     // Start application
-    if (!AppInit(node) || !Assert(node.shutdown)->wait()) {
+    if (!AppInit(node) || !Assert(node.shutdown_signal)->wait()) {
         node.exit_status = EXIT_FAILURE;
     }
     Interrupt(node);

src/index/base.cpp

@@ -31,7 +31,7 @@ template <typename... Args>
 void BaseIndex::FatalErrorf(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
 {
     auto message = tfm::format(fmt, args...);
-    node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message), m_chain->context()->warnings.get());
+    node::AbortNode(m_chain->context()->shutdown_request, m_chain->context()->exit_status, Untranslated(message), m_chain->context()->warnings.get());
 }
 
 CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)

src/init.cpp

@@ -207,7 +207,14 @@ void InitContext(NodeContext& node)
     g_shutdown.emplace();
 
     node.args = &gArgs;
-    node.shutdown = &*g_shutdown;
+    node.shutdown_signal = &*g_shutdown;
+    node.shutdown_request = [&node] {
+        assert(node.shutdown_signal);
+        if (!(*node.shutdown_signal)()) return false;
+        // Wake any threads that may be waiting for the tip to change.
+        if (node.notifications) WITH_LOCK(node.notifications->m_tip_block_mutex, node.notifications->m_tip_block_cv.notify_all());
+        return true;
+    };
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -235,7 +242,7 @@ void InitContext(NodeContext& node)
 
 bool ShutdownRequested(node::NodeContext& node)
 {
-    return bool{*Assert(node.shutdown)};
+    return bool{*Assert(node.shutdown_signal)};
 }
 
 #if HAVE_SYSTEM
@@ -286,7 +293,7 @@ void Shutdown(NodeContext& node)
 
     StopHTTPRPC();
     StopREST();
-    StopRPC(&node);
+    StopRPC();
     StopHTTPServer();
     for (const auto& client : node.chain_clients) {
         client->flush();
@@ -707,7 +714,7 @@ static void StartupNotify(const ArgsManager& args)
 static bool AppInitServers(NodeContext& node)
 {
     const ArgsManager& args = *Assert(node.args);
-    if (!InitHTTPServer(*Assert(node.shutdown))) {
+    if (!InitHTTPServer(*Assert(node.shutdown_signal))) {
         return false;
     }
     StartRPC();
@@ -1216,7 +1223,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
     };
     Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
     try {
-        node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown), chainman_opts, blockman_opts);
+        node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown_signal), chainman_opts, blockman_opts);
     } catch (std::exception& e) {
         return {ChainstateLoadStatus::FAILURE_FATAL, strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what())};
     }
@@ -1327,7 +1334,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         constexpr uint64_t min_disk_space = 50 << 20; // 50 MB
         if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) {
             LogError("Shutting down due to lack of disk space!\n");
-            if (!(*Assert(node.shutdown))()) {
+            if (!(Assert(node.shutdown_request))()) {
                 LogError("Failed to send shutdown signal after disk space check\n");
             }
         }
@@ -1608,7 +1615,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 7: load block chain
 
-    node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status, *Assert(node.warnings));
+    node.notifications = std::make_unique<KernelNotifications>(Assert(node.shutdown_request), node.exit_status, *Assert(node.warnings));
     ReadNotificationArgs(args, *node.notifications);
 
     // cache size calculations
@@ -1649,7 +1656,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             return false;
         }
         do_reindex = true;
-        if (!Assert(node.shutdown)->reset()) {
+        if (!Assert(node.shutdown_signal)->reset()) {
             LogError("Internal error: failed to reset shutdown signal.\n");
         }
         std::tie(status, error) = InitAndLoadChainstate(
@@ -1794,7 +1801,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         ImportBlocks(chainman, vImportFiles);
         if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
             LogPrintf("Stopping after block import\n");
-            if (!(*Assert(node.shutdown))()) {
+            if (!(Assert(node.shutdown_request))()) {
                 LogError("Failed to send shutdown signal after finishing block import\n");
             }
             return;

src/node/abort.cpp

@@ -15,12 +15,12 @@
 
 namespace node {
 
-void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings)
+void AbortNode(const std::function<bool()>& shutdown_request, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings)
 {
     if (warnings) warnings->Set(Warning::FATAL_INTERNAL_ERROR, message);
     InitError(_("A fatal internal error occurred, see debug.log for details: ") + message);
     exit_status.store(EXIT_FAILURE);
-    if (shutdown && !(*shutdown)()) {
+    if (shutdown_request && !shutdown_request()) {
         LogError("Failed to send shutdown signal\n");
     };
 }

src/node/abort.h

@@ -6,16 +6,13 @@
 #define BITCOIN_NODE_ABORT_H
 
 #include <atomic>
+#include <functional>
 
 struct bilingual_str;
 
-namespace util {
-class SignalInterrupt;
-} // namespace util
-
 namespace node {
 class Warnings;
-void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings);
+void AbortNode(const std::function<bool()>& shutdown_request, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings);
 } // namespace node
 
 #endif // BITCOIN_NODE_ABORT_H

src/node/context.h

@@ -59,8 +59,10 @@ struct NodeContext {
     std::unique_ptr<ECC_Context> ecc_context;
     //! Init interface for initializing current process and connecting to other processes.
     interfaces::Init* init{nullptr};
+    //! Function to request a shutdown.
+    std::function<bool()> shutdown_request;
     //! Interrupt object used to track whether node shutdown was requested.
-    util::SignalInterrupt* shutdown{nullptr};
+    util::SignalInterrupt* shutdown_signal{nullptr};
     std::unique_ptr<AddrMan> addrman;
     std::unique_ptr<CConnman> connman;
     std::unique_ptr<CTxMemPool> mempool;

src/node/interfaces.cpp

@@ -134,13 +134,15 @@ class NodeImpl : public Node
     }
     void startShutdown() override
     {
-        if (!(*Assert(Assert(m_context)->shutdown))()) {
+        NodeContext& ctx{*Assert(m_context)};
+        if (!(Assert(ctx.shutdown_request))()) {
             LogError("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(m_context);
+            StopRPC();
         }
     }
     bool shutdownRequested() override { return ShutdownRequested(*Assert(m_context)); };

src/node/kernel_notifications.cpp

@@ -58,7 +58,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state
 
     uiInterface.NotifyBlockTip(state, &index);
     if (m_stop_at_height && index.nHeight >= m_stop_at_height) {
-        if (!m_shutdown()) {
+        if (!m_shutdown_request()) {
             LogError("Failed to send shutdown signal after reaching stop height\n");
         }
         return kernel::Interrupted{};
@@ -90,12 +90,12 @@ void KernelNotifications::warningUnset(kernel::Warning id)
 
 void KernelNotifications::flushError(const bilingual_str& message)
 {
-    AbortNode(&m_shutdown, m_exit_status, message, &m_warnings);
+    AbortNode(m_shutdown_request, m_exit_status, message, &m_warnings);
 }
 
 void KernelNotifications::fatalError(const bilingual_str& message)
 {
-    node::AbortNode(m_shutdown_on_fatal_error ? &m_shutdown : nullptr,
+    node::AbortNode(m_shutdown_on_fatal_error ? m_shutdown_request : nullptr,
                     m_exit_status, message, &m_warnings);
 }
 

src/node/kernel_notifications.h

@@ -13,6 +13,7 @@
 
 #include <atomic>
 #include <cstdint>
+#include <functional>
 
 class ArgsManager;
 class CBlockIndex;
@@ -23,10 +24,6 @@ namespace kernel {
 enum class Warning;
 } // namespace kernel
 
-namespace util {
-class SignalInterrupt;
-} // namespace util
-
 namespace node {
 
 class Warnings;
@@ -35,8 +32,8 @@ static constexpr int DEFAULT_STOPATHEIGHT{0};
 class KernelNotifications : public kernel::Notifications
 {
 public:
-    KernelNotifications(util::SignalInterrupt& shutdown, std::atomic<int>& exit_status, node::Warnings& warnings)
-        : m_shutdown(shutdown), m_exit_status{exit_status}, m_warnings{warnings} {}
+    KernelNotifications(const std::function<bool()>& shutdown_request, std::atomic<int>& exit_status, node::Warnings& warnings)
+        : m_shutdown_request(shutdown_request), m_exit_status{exit_status}, m_warnings{warnings} {}
 
     [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override EXCLUSIVE_LOCKS_REQUIRED(!m_tip_block_mutex);
 
@@ -63,7 +60,7 @@ class KernelNotifications : public kernel::Notifications
     uint256 m_tip_block GUARDED_BY(m_tip_block_mutex);
 
 private:
-    util::SignalInterrupt& m_shutdown;
+    const std::function<bool()>& m_shutdown_request;
     std::atomic<int>& m_exit_status;
     node::Warnings& m_warnings;
 };

src/rpc/server.cpp

@@ -169,7 +169,7 @@ static RPCHelpMan stop()
 {
     // Event loop will exit after current HTTP requests have been handled, so
     // this reply will get back to the client.
-    CHECK_NONFATAL((*CHECK_NONFATAL(EnsureAnyNodeContext(jsonRequest.context).shutdown))());
+    CHECK_NONFATAL((CHECK_NONFATAL(EnsureAnyNodeContext(jsonRequest.context).shutdown_request))());
     if (jsonRequest.params[0].isNum()) {
         UninterruptibleSleep(std::chrono::milliseconds{jsonRequest.params[0].getInt<int>()});
     }
@@ -294,7 +294,7 @@ void InterruptRPC()
     });
 }
 
-void StopRPC(const std::any& context)
+void StopRPC()
 {
     static std::once_flag g_rpc_stop_flag;
     // This function could be called twice if the GUI has been started with -server=1.
@@ -303,9 +303,6 @@ void StopRPC(const std::any& context)
         LogDebug(BCLog::RPC, "Stopping RPC\n");
         WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear());
         DeleteAuthCookie();
-        node::NodeContext& node = EnsureAnyNodeContext(context);
-        // The notifications interface doesn't exist between initialization step 4a and 7.
-        if (node.notifications) WITH_LOCK(node.notifications->m_tip_block_mutex, node.notifications->m_tip_block_cv.notify_all());
         LogDebug(BCLog::RPC, "RPC stopped.\n");
     });
 }

src/rpc/server.h

@@ -9,7 +9,6 @@
 #include <rpc/request.h>
 #include <rpc/util.h>
 
-#include <any>
 #include <functional>
 #include <map>
 #include <stdint.h>
@@ -173,7 +172,7 @@ extern CRPCTable tableRPC;
 
 void StartRPC();
 void InterruptRPC();
-void StopRPC(const std::any& context);
+void StopRPC();
 UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors);
 
 #endif // BITCOIN_RPC_SERVER_H

src/test/blockfilter_index_tests.cpp

@@ -144,7 +144,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, *Assert(m_node.shutdown));
+    IndexWaitSynced(filter_index, *Assert(m_node.shutdown_signal));
 
     // Check that filter index has all blocks that were in the chain before it started.
     {

src/test/blockmanager_tests.cpp

@@ -28,13 +28,13 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
 {
     const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)};
-    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)};
+    KernelNotifications notifications{Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)};
     const BlockManager::Options blockman_opts{
         .chainparams = *params,
         .blocks_dir = m_args.GetBlocksDirPath(),
         .notifications = notifications,
     };
-    BlockManager blockman{*Assert(m_node.shutdown), blockman_opts};
+    BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts};
     // simulate adding a genesis block normally
     BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0).nPos, BLOCK_SERIALIZATION_HEADER_SIZE);
     // simulate what happens during reindex
@@ -135,13 +135,13 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
 
 BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
 {
-    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)};
+    KernelNotifications notifications{Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)};
     node::BlockManager::Options blockman_opts{
         .chainparams = Params(),
         .blocks_dir = m_args.GetBlocksDirPath(),
         .notifications = notifications,
     };
-    BlockManager blockman{*Assert(m_node.shutdown), blockman_opts};
+    BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts};
 
     // Test blocks with no transactions, not even a coinbase
     CBlock block1;

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, *Assert(m_node.shutdown));
+    IndexWaitSynced(coin_stats_index, *Assert(m_node.shutdown_signal));
 
     // 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, *Assert(m_node.shutdown));
+        IndexWaitSynced(index, *Assert(m_node.shutdown_signal));
         std::shared_ptr<const CBlock> new_block;
         CBlockIndex* new_block_index = nullptr;
         {

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, *Assert(m_node.shutdown));
+    IndexWaitSynced(txindex, *Assert(m_node.shutdown_signal));
 
     // Check that txindex excludes genesis block transactions.
     const CBlock& genesis_block = Params().GenesisBlock();

src/test/util/setup_common.cpp

@@ -104,7 +104,8 @@ static void ExitFailure(std::string_view str_err)
 BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
     : m_args{}
 {
-    m_node.shutdown = &m_interrupt;
+    m_node.shutdown_signal = &m_interrupt;
+    m_node.shutdown_request = [this]{ return m_interrupt(); };
     m_node.args = &gArgs;
     std::vector<const char*> arguments = Cat(
         {
@@ -224,7 +225,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts)
 
     m_cache_sizes = CalculateCacheSizes(m_args);
 
-    m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings));
+    m_node.notifications = std::make_unique<KernelNotifications>(Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings));
 
     m_make_chainman = [this, &chainparams, opts] {
         Assert(!m_node.chainman);
@@ -245,7 +246,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts)
             .blocks_dir = m_args.GetBlocksDirPath(),
             .notifications = chainman_opts.notifications,
         };
-        m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown), chainman_opts, blockman_opts);
+        m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts);
         LOCK(m_node.chainman->GetMutex());
         m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<BlockTreeDB>(DBParams{
             .path = m_args.GetDataDirNet() / "blocks" / "index",

src/test/validation_chainstatemanager_tests.cpp

@@ -382,7 +382,7 @@ struct SnapshotTestSetup : TestChain100Setup {
             LOCK(::cs_main);
             chainman.ResetChainstates();
             BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0);
-            m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings));
+            m_node.notifications = std::make_unique<KernelNotifications>(Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings));
             const ChainstateManager::Options chainman_opts{
                 .chainparams = ::Params(),
                 .datadir = chainman.m_options.datadir,
@@ -397,7 +397,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>(*Assert(m_node.shutdown), chainman_opts, blockman_opts);
+            m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts);
         }
         return *Assert(m_node.chainman);
     }