kernel: Remove StartShutdown calls from validation code

ryanofsky · ryanofsky · commit 09938a41d904 · 2023-07-07T16:23:33.000-04:00
This change drops the last kernel dependency on shutdown.cpp. It also adds new
hooks for libbitcoinkernel applications to be able to stop kernel operations
after blocks are imported and each time the chain tip changes.

This is a refactoring that mostly does not affect behavior, except it does
change the behavior of the -stopatheight option in a small way:

- Previously the -stopatheight option would shut down the node at or above the
  specified height. It would not stop connecting blocks after the specified
  height if they were already downloaded.

- Now, the -stopatheight option will shut down the node exactly at the
  specified height, without validating and connecting the downloaded blocks
  after it. Since blocks above the specified height are no longer validated, it
  means -stopatheight option is only guaranteed to return a valid chain with
  the specified height. But it is possible for the resulting chain to not be the
  most-work chain, if the blocks above the height which are not validated turn
  out to actually be invalid.
diff --git a/doc/release-notes-28048.md b/doc/release-notes-28048.md
@@ -0,0 +1 @@
+- The `-stopatheight` option will now stop exactly at the specified height, rather than at or above the specified height. Since the node shuts down as soon as it find a valid chain at the specified height, it is possible for the resulting chain to no longer be the most-work chain, if some of the blocks above the specified height are invalid.
diff --git 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
@@ -81,9 +81,15 @@ int main(int argc, char* argv[])
     class KernelNotifications : public kernel::Notifications
     {
     public:
-        void blockTip(SynchronizationState, CBlockIndex&) override
+        kernel::InterruptResult blocksImported() override
+        {
+            std::cout << "Block import finished" << std::endl;
+            return {};
+        }
+        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
@@ -114,7 +114,8 @@
 #include <zmq/zmqrpc.h>
 #endif
 
-using kernel::DEFAULT_STOPAFTERBLOCKIMPORT;
+using node::DEFAULT_STOPAFTERBLOCKIMPORT;
+using node::DEFAULT_STOPATHEIGHT;
 using kernel::DumpMempool;
 using kernel::ValidationCacheSizes;
 
@@ -566,7 +567,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-deprecatedrpc=<method>", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the given height in the main chain (default: %u)", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-stopatheight", strprintf("Shut down the node after validating and attaching a block greater or equal to the given height to the chain. This option is mostly useful for benchmarking. (default: %u)", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-limitancestorcount=<n>", strprintf("Do not accept transactions if number of in-mempool ancestors is <n> or more (default: %u)", DEFAULT_ANCESTOR_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-limitancestorsize=<n>", strprintf("Do not accept transactions whose size with all in-mempool ancestors exceeds <n> kilobytes (default: %u)", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-limitdescendantcount=<n>", strprintf("Do not accept transactions if any ancestor would have <n> or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
@@ -1409,6 +1410,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/blockmanager_opts.h b/src/kernel/blockmanager_opts.h
@@ -14,8 +14,6 @@ class CChainParams;
 
 namespace kernel {
 
-static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
-
 /**
  * An options struct for `BlockManager`, more ergonomically referred to as
  * `BlockManager::Options` due to the using-declaration in `BlockManager`.
@@ -24,7 +22,6 @@ struct BlockManagerOpts {
     const CChainParams& chainparams;
     uint64_t prune_target{0};
     bool fast_prune{false};
-    bool stop_after_block_import{DEFAULT_STOPAFTERBLOCKIMPORT};
     const fs::path blocks_dir;
     Notifications& notifications;
 };
diff --git 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
@@ -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 indiciate 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,8 @@ class Notifications
 public:
     virtual ~Notifications(){};
 
-    virtual void blockTip(SynchronizationState state, CBlockIndex& index) {}
+    [[nodiscard]] virtual InterruptResult blocksImported() { return {}; }
+    [[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/blockmanager_args.cpp b/src/node/blockmanager_args.cpp
@@ -32,7 +32,6 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op
     opts.prune_target = nPruneTarget;
 
     if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
-    if (auto value{args.GetBoolArg("-stopafterblockimport")}) opts.stop_after_block_import = *value;
 
     return {};
 }
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
@@ -933,9 +933,11 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
             }
         }
 
-        if (chainman.m_blockman.StopAfterBlockImport()) {
+        if (kernel::IsInterrupted(chainman.GetNotifications().blocksImported())) {
             LogPrintf("Stopping after block import\n");
-            StartShutdown();
+            // Just returning void 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.
             return;
         }
     } // End scope of ImportingNow
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
@@ -214,8 +214,6 @@ class BlockManager
 
     [[nodiscard]] bool LoadingBlocks() const { return m_importing || fReindex; }
 
-    [[nodiscard]] bool StopAfterBlockImport() const { return m_opts.stop_after_block_import; }
-
     /** Calculate the amount of disk space the block & undo files currently use */
     uint64_t CalculateCurrentUsage();
 
diff --git 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
@@ -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,23 @@ static void DoWarning(const bilingual_str& warning)
 
 namespace node {
 
-void KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index)
+kernel::InterruptResult KernelNotifications::blocksImported()
+{
+    if (m_stop_after_block_import) {
+        StartShutdown();
+        return kernel::Interrupted{};
+    }
+    return {};
+}
+
+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 +102,10 @@ 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.GetBoolArg("-stopafterblockimport")}) notifications.m_stop_after_block_import = *value;
+    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
@@ -11,17 +11,24 @@
 #include <cstdint>
 #include <string>
 
+class ArgsManager;
 class CBlockIndex;
 enum class SynchronizationState;
 struct bilingual_str;
 
 namespace node {
+
+static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
+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 blocksImported() 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 +40,18 @@ class KernelNotifications : public kernel::Notifications
 
     void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) override;
 
+    //! Whether or not to return Interrupted{} after blocksImported notification.
+    bool m_stop_after_block_import{DEFAULT_STOPAFTERBLOCKIMPORT};
+    //! 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
@@ -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,10 @@ 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.
+        (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(IsInitialBlockDownload()), *to_mark_failed->pprev);
     }
     return true;
 }
diff --git 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 @@ 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.

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+- The `-stopatheight` option will now stop exactly at the specified height, rather than at or above the specified height. Since the node shuts down as soon as it find a valid chain at the specified height, it is possible for the resulting chain to no longer be the most-work chain, if some of the blocks above the specified height are invalid.
Original file line number	Diff line number	Diff line change
`@@ -81,9 +81,15 @@ int main(int argc, char* argv[])`
`81`	`81`	`class KernelNotifications : public kernel::Notifications`
`82`	`82`	`{`
`83`	`83`	`public:`
`84`		`- void blockTip(SynchronizationState, CBlockIndex&) override`
	`84`	`+ kernel::InterruptResult blocksImported() override`
	`85`	`+ {`
	`86`	`+ std::cout << "Block import finished" << std::endl;`
	`87`	`+ return {};`
	`88`	`+ }`
	`89`	`+ kernel::InterruptResult blockTip(SynchronizationState, CBlockIndex&) override`
`85`	`90`	`{`
`86`	`91`	`std::cout << "Block tip changed" << std::endl;`
	`92`	`+ return {};`
`87`	`93`	`}`
`88`	`94`	`void headerTip(SynchronizationState, int64_t height, int64_t timestamp, bool presync) override`
`89`	`95`	`{`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,6 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op`
`32`	`32`	`opts.prune_target = nPruneTarget;`
`33`	`33`
`34`	`34`	`if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;`
`35`		`- if (auto value{args.GetBoolArg("-stopafterblockimport")}) opts.stop_after_block_import = *value;`
`36`	`35`
`37`	`36`	`return {};`
`38`	`37`	`}`
Original file line number	Diff line number	Diff line change
`@@ -933,9 +933,11 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile`
`933`	`933`	`}`
`934`	`934`	`}`
`935`	`935`
`936`		`- if (chainman.m_blockman.StopAfterBlockImport()) {`
	`936`	`+ if (kernel::IsInterrupted(chainman.GetNotifications().blocksImported())) {`
`937`	`937`	`LogPrintf("Stopping after block import\n");`
`938`		`- StartShutdown();`
	`938`	`+ // Just returning void for now. This could be changed to bubble up`
	`939`	`+ // the kernel::Interrupted value to the caller so the caller could`
	`940`	`+ // distinguish between completed and interrupted operations.`
`939`	`941`	`return;`
`940`	`942`	`}`
`941`	`943`	`} // End scope of ImportingNow`