Merge bitcoin/bitcoin#28048: kernel: Remove StartShutdown calls from validation code

31eca93 kernel: Remove StartShutdown calls from validation code (Ryan Ofsky)

Pull request description:

  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 change is mostly a refactoring, but does slightly change `-stopatheight` behavior (see release note and commit message)

ACKs for top commit:
  TheCharlatan:
    ACK 31eca93
  furszy:
    Concept and light review ACK 31eca93
  hebasto:
    ACK 31eca93, I have reviewed the code and it looks OK.
  MarcoFalke:
    lgtm ACK 31eca93 🕷

Tree-SHA512: e26928436bcde658e842b1f92e9c24b1ce91031fb63b41aafccf3130bfff532b75338a269a2bb7558bff2973913f17b97a00fec3e7e0588e2ce44de097142047

Author: achow101

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 @@ class KernelNotifications : public kernel::Notifications
 
     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 for: 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 for: 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 @@ class ChainstateManager
     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.