From 31eca93a9eb8e54f856d3f558aa3c831ef181d37 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Fri, 7 Jul 2023 12:53:31 -0400
Subject: [PATCH] kernel: Remove StartShutdown calls from validation code

This change drops the last kernel dependency on shutdown.cpp. It also adds new
hooks for libbitcoinkernel applications to be able to interrupt kernel
operations when the chain tip changes.

This is a refactoring that does not affect behavior. (Looking at the code it
can appear like the new break statement in the ActivateBestChain function is a
change in behavior, but actually the previous StartShutdown call was indirectly
triggering a break before, because it was causing m_chainman.m_interrupt to be
true. The new code just makes the break more obvious.)
---
 src/Makefile.am                      |  1 -
 src/bitcoin-chainstate.cpp           |  3 ++-
 src/init.cpp                         |  2 ++
 src/kernel/chainstatemanager_opts.h  |  2 --
 src/kernel/notifications_interface.h | 15 ++++++++++++++-
 src/node/chainstatemanager_args.cpp  |  2 --
 src/node/kernel_notifications.cpp    | 13 ++++++++++++-
 src/node/kernel_notifications.h      | 11 ++++++++++-
 src/validation.cpp                   | 20 +++++++++++++++-----
 src/validation.h                     |  2 --
 10 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 4e9c161c57..e1ae049b15 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -956,7 +956,6 @@ libbitcoinkernel_la_SOURCES = \
   script/script_error.cpp \
   script/sigcache.cpp \
   script/standard.cpp \
-  shutdown.cpp \
   signet.cpp \
   support/cleanse.cpp \
   support/lockedpool.cpp \
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 45fd239e20..580a6badd6 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -81,9 +81,10 @@ int main(int argc, char* argv[])
     class KernelNotifications : public kernel::Notifications
     {
     public:
-        void blockTip(SynchronizationState, CBlockIndex&) override
+        kernel::InterruptResult blockTip(SynchronizationState, CBlockIndex&) override
         {
             std::cout << "Block tip changed" << std::endl;
+            return {};
         }
         void headerTip(SynchronizationState, int64_t height, int64_t timestamp, bool presync) override
         {
diff --git a/src/init.cpp b/src/init.cpp
index 2f4eec060b..f726fe54ca 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -123,6 +123,7 @@ using node::CacheSizes;
 using node::CalculateCacheSizes;
 using node::DEFAULT_PERSIST_MEMPOOL;
 using node::DEFAULT_PRINTPRIORITY;
+using node::DEFAULT_STOPATHEIGHT;
 using node::fReindex;
 using node::KernelNotifications;
 using node::LoadChainstate;
@@ -1408,6 +1409,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // ********************************************************* Step 7: load block chain
 
     node.notifications = std::make_unique<KernelNotifications>(node.exit_status);
+    ReadNotificationArgs(args, *node.notifications);
     fReindex = args.GetBoolArg("-reindex", false);
     bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false);
     ChainstateManager::Options chainman_opts{
diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
index 035a913d10..917f7d226c 100644
--- a/src/kernel/chainstatemanager_opts.h
+++ b/src/kernel/chainstatemanager_opts.h
@@ -21,7 +21,6 @@ class CChainParams;
 
 static constexpr bool DEFAULT_CHECKPOINTS_ENABLED{true};
 static constexpr auto DEFAULT_MAX_TIP_AGE{24h};
-static constexpr int DEFAULT_STOPATHEIGHT{0};
 
 namespace kernel {
 
@@ -46,7 +45,6 @@ struct ChainstateManagerOpts {
     DBOptions coins_db{};
     CoinsViewOptions coins_view{};
     Notifications& notifications;
-    int stop_at_height{DEFAULT_STOPATHEIGHT};
 };
 
 } // namespace kernel
diff --git a/src/kernel/notifications_interface.h b/src/kernel/notifications_interface.h
index e596a144a8..c5e77b0df9 100644
--- a/src/kernel/notifications_interface.h
+++ b/src/kernel/notifications_interface.h
@@ -9,12 +9,25 @@
 
 #include <cstdint>
 #include <string>
+#include <variant>
 
 class CBlockIndex;
 enum class SynchronizationState;
 
 namespace kernel {
 
+//! Result type for use with std::variant to indicate that an operation should be interrupted.
+struct Interrupted{};
+
+//! Simple result type for functions that need to propagate an interrupt status and don't have other return values.
+using InterruptResult = std::variant<std::monostate, Interrupted>;
+
+template <typename T>
+bool IsInterrupted(const T& result)
+{
+    return std::holds_alternative<kernel::Interrupted>(result);
+}
+
 /**
  * A base class defining functions for notifying about certain kernel
  * events.
@@ -24,7 +37,7 @@ class Notifications
 public:
     virtual ~Notifications(){};
 
-    virtual void blockTip(SynchronizationState state, CBlockIndex& index) {}
+    [[nodiscard]] virtual InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) { return {}; }
     virtual void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {}
     virtual void progress(const bilingual_str& title, int progress_percent, bool resume_possible) {}
     virtual void warning(const bilingual_str& warning) {}
diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
index a7f7303348..87d9238c18 100644
--- a/src/node/chainstatemanager_args.cpp
+++ b/src/node/chainstatemanager_args.cpp
@@ -37,8 +37,6 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
 
     if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};
 
-    if (auto value{args.GetIntArg("-stopatheight")}) opts.stop_at_height = *value;
-
     ReadDatabaseArgs(args, opts.block_tree_db);
     ReadDatabaseArgs(args, opts.coins_db);
     ReadCoinsViewArgs(args, opts.coins_view);
diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp
index 0be7d51afb..7224127c72 100644
--- a/src/node/kernel_notifications.cpp
+++ b/src/node/kernel_notifications.cpp
@@ -8,6 +8,7 @@
 #include <config/bitcoin-config.h>
 #endif
 
+#include <chain.h>
 #include <common/args.h>
 #include <common/system.h>
 #include <kernel/context.h>
@@ -57,9 +58,14 @@ static void DoWarning(const bilingual_str& warning)
 
 namespace node {
 
-void KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index)
+kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index)
 {
     uiInterface.NotifyBlockTip(state, &index);
+    if (m_stop_at_height && index.nHeight >= m_stop_at_height) {
+        StartShutdown();
+        return kernel::Interrupted{};
+    }
+    return {};
 }
 
 void KernelNotifications::headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync)
@@ -87,4 +93,9 @@ void KernelNotifications::fatalError(const std::string& debug_message, const bil
     node::AbortNode(m_exit_status, debug_message, user_message, m_shutdown_on_fatal_error);
 }
 
+void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications)
+{
+    if (auto value{args.GetIntArg("-stopatheight")}) notifications.m_stop_at_height = *value;
+}
+
 } // namespace node
diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h
index d35b6ecca5..b2dfc03398 100644
--- a/src/node/kernel_notifications.h
+++ b/src/node/kernel_notifications.h
@@ -11,17 +11,21 @@
 #include <cstdint>
 #include <string>
 
+class ArgsManager;
 class CBlockIndex;
 enum class SynchronizationState;
 struct bilingual_str;
 
 namespace node {
+
+static constexpr int DEFAULT_STOPATHEIGHT{0};
+
 class KernelNotifications : public kernel::Notifications
 {
 public:
     KernelNotifications(std::atomic<int>& exit_status) : m_exit_status{exit_status} {}
 
-    void blockTip(SynchronizationState state, CBlockIndex& index) override;
+    [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override;
 
     void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) override;
 
@@ -33,11 +37,16 @@ public:
 
     void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) override;
 
+    //! Block height after which blockTip notification will return Interrupted{}, if >0.
+    int m_stop_at_height{DEFAULT_STOPATHEIGHT};
     //! Useful for tests, can be set to false to avoid shutdown on fatal error.
     bool m_shutdown_on_fatal_error{true};
 private:
     std::atomic<int>& m_exit_status;
 };
+
+void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications);
+
 } // namespace node
 
 #endif // BITCOIN_NODE_KERNEL_NOTIFICATIONS_H
diff --git a/src/validation.cpp b/src/validation.cpp
index be6afbd320..5bf8bd70e2 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -37,7 +37,6 @@
 #include <reverse_iterator.h>
 #include <script/script.h>
 #include <script/sigcache.h>
-#include <shutdown.h>
 #include <signet.h>
 #include <tinyformat.h>
 #include <txdb.h>
@@ -3172,13 +3171,17 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
                 GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);
 
                 // Always notify the UI if a new block tip was connected
-                m_chainman.GetNotifications().blockTip(GetSynchronizationState(fInitialDownload), *pindexNewTip);
+                if (kernel::IsInterrupted(m_chainman.GetNotifications().blockTip(GetSynchronizationState(fInitialDownload), *pindexNewTip))) {
+                    // Just breaking and returning success for now. This could
+                    // be changed to bubble up the kernel::Interrupted value to
+                    // the caller so the caller could distinguish between
+                    // completed and interrupted operations.
+                    break;
+                }
             }
         }
         // When we reach this point, we switched to a new tip (stored in pindexNewTip).
 
-        if (m_chainman.StopAtHeight() && pindexNewTip && pindexNewTip->nHeight >= m_chainman.StopAtHeight()) StartShutdown();
-
         if (WITH_LOCK(::cs_main, return m_disabled)) {
             // Background chainstate has reached the snapshot base block, so exit.
             break;
@@ -3369,7 +3372,14 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 
     // Only notify about a new block tip if the active chain was modified.
     if (pindex_was_in_chain) {
-        m_chainman.GetNotifications().blockTip(GetSynchronizationState(IsInitialBlockDownload()), *to_mark_failed->pprev);
+        // Ignoring return value for now, this could be changed to bubble up
+        // kernel::Interrupted value to the caller so the caller could
+        // distinguish between completed and interrupted operations. It might
+        // also make sense for the blockTip notification to have an enum
+        // parameter indicating the source of the tip change so hooks can
+        // distinguish user-initiated invalidateblock changes from other
+        // changes.
+        (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(IsInitialBlockDownload()), *to_mark_failed->pprev);
     }
     return true;
 }
diff --git a/src/validation.h b/src/validation.h
index 49860f2d6a..af8ceb5dfa 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -23,7 +23,6 @@
 #include <policy/packages.h>
 #include <policy/policy.h>
 #include <script/script_error.h>
-#include <shutdown.h>
 #include <sync.h>
 #include <txdb.h>
 #include <txmempool.h> // For CTxMemPool::cs
@@ -970,7 +969,6 @@ public:
     const arith_uint256& MinimumChainWork() const { return *Assert(m_options.minimum_chain_work); }
     const uint256& AssumedValidBlock() const { return *Assert(m_options.assumed_valid_block); }
     kernel::Notifications& GetNotifications() const { return m_options.notifications; };
-    int StopAtHeight() const { return m_options.stop_at_height; };
 
     /**
      * Alias for ::cs_main.