Merge bitcoin/bitcoin#27861: kernel: Rm ShutdownRequested and AbortNode from validation code.

6eb33bd kernel: Add fatalError method to notifications (TheCharlatan)
7320db9 kernel: Add flushError method to notifications (TheCharlatan)
3fa9094 scripted-diff: Rename FatalError to FatalErrorf (TheCharlatan)
edb55e2 kernel: Pass interrupt reference to chainman (TheCharlatan)
e2d680a util: Add SignalInterrupt class and use in shutdown.cpp (TheCharlatan)

Pull request description:

  Get rid of all `ShutdownRequested` calls in validation code by introducing an interrupt object that applications can use to cancel long-running kernel operations.

  Replace all `AbortNode` calls in validation code with new fatal error and flush error notifications so kernel applications can be notified about failures and choose how to handle them.

  ---

  This pull request is part of the `libbitcoinkernel` project bitcoin/bitcoin#27587 https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".

  The pull request mostly allows dropping the kernel dependency on shutdown.cpp. The only dependency left after this is a `StartShutdown` call which will be removed in followup PR bitcoin/bitcoin#27711. This PR also drops the last reference to the `uiInterface` global in kernel code. The process of moving the `uiInterface` out of the kernel was started in bitcoin/bitcoin#27636.

  This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent and less reliable on global mutable state. The set of patches contained here was originally proposed by @ryanofsky [here](bitcoin/bitcoin#27711 (comment)).

ACKs for top commit:
  achow101:
    light ACK 6eb33bd
  hebasto:
    ACK 6eb33bd, I have reviewed the code and it looks OK.
  ryanofsky:
    Code review ACK 6eb33bd. No changes since last review other than rebase.

Tree-SHA512: 7d2d05fa4805428a09466d43c11ae32946cbb25aa5e741b1eec9cd142e4de4bb311e13ebf1bb125ae490c9d08274f2d56c93314e10f3d69e7fec7445e504987c

Author: ryanofsky

src/Makefile.am

@@ -205,6 +205,7 @@ BITCOIN_CORE_H = \
   netbase.h \
   netgroup.h \
   netmessagemaker.h \
+  node/abort.h \
   node/blockmanager_args.h \
   node/blockstorage.h \
   node/caches.h \
@@ -310,6 +311,7 @@ BITCOIN_CORE_H = \
   util/readwritefile.h \
   util/result.h \
   util/serfloat.h \
+  util/signalinterrupt.h \
   util/sock.h \
   util/spanparsing.h \
   util/string.h \
@@ -399,6 +401,7 @@ libbitcoin_node_a_SOURCES = \
   net.cpp \
   net_processing.cpp \
   netgroup.cpp \
+  node/abort.cpp \
   node/blockmanager_args.cpp \
   node/blockstorage.cpp \
   node/caches.cpp \
@@ -733,6 +736,7 @@ libbitcoin_util_a_SOURCES = \
   util/moneystr.cpp \
   util/rbf.cpp \
   util/readwritefile.cpp \
+  util/signalinterrupt.cpp \
   util/thread.cpp \
   util/threadinterrupt.cpp \
   util/threadnames.cpp \
@@ -933,7 +937,6 @@ libbitcoinkernel_la_SOURCES = \
   logging.cpp \
   node/blockstorage.cpp \
   node/chainstate.cpp \
-  node/interface_ui.cpp \
   node/utxo_snapshot.cpp \
   policy/feerate.cpp \
   policy/fees.cpp \
@@ -972,6 +975,7 @@ libbitcoinkernel_la_SOURCES = \
   util/moneystr.cpp \
   util/rbf.cpp \
   util/serfloat.cpp \
+  util/signalinterrupt.cpp \
   util/strencodings.cpp \
   util/string.cpp \
   util/syserror.cpp \

src/bitcoin-chainstate.cpp

@@ -97,6 +97,15 @@ int main(int argc, char* argv[])
         {
             std::cout << "Warning: " << warning.original << std::endl;
         }
+        void flushError(const std::string& debug_message) override
+        {
+            std::cerr << "Error flushing block data to disk: " << debug_message << std::endl;
+        }
+        void fatalError(const std::string& debug_message, const bilingual_str& user_message) override
+        {
+            std::cerr << "Error: " << debug_message << std::endl;
+            std::cerr << (user_message.empty() ? "A fatal internal error occurred." : user_message.original) << std::endl;
+        }
     };
     auto notifications = std::make_unique<KernelNotifications>();
 
@@ -112,8 +121,9 @@ int main(int argc, char* argv[])
     const node::BlockManager::Options blockman_opts{
         .chainparams = chainman_opts.chainparams,
         .blocks_dir = abs_datadir / "blocks",
+        .notifications = chainman_opts.notifications,
     };
-    ChainstateManager chainman{chainman_opts, blockman_opts};
+    ChainstateManager chainman{kernel_context.interrupt, chainman_opts, blockman_opts};
 
     node::CacheSizes cache_sizes;
     cache_sizes.block_tree_db = 2 << 20;

src/index/base.cpp

@@ -8,6 +8,7 @@
 #include <interfaces/chain.h>
 #include <kernel/chain.h>
 #include <logging.h>
+#include <node/abort.h>
 #include <node/blockstorage.h>
 #include <node/context.h>
 #include <node/database_args.h>
@@ -30,9 +31,10 @@ constexpr auto SYNC_LOG_INTERVAL{30s};
 constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s};
 
 template <typename... Args>
-static void FatalError(const char* fmt, const Args&... args)
+void BaseIndex::FatalErrorf(const char* fmt, const Args&... args)
 {
-    AbortNode(tfm::format(fmt, args...));
+    auto message = tfm::format(fmt, args...);
+    node::AbortNode(m_chain->context()->exit_status, message);
 }
 
 CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)
@@ -197,7 +199,7 @@ void BaseIndex::ThreadSync()
                     break;
                 }
                 if (pindex_next->pprev != pindex && !Rewind(pindex, pindex_next->pprev)) {
-                    FatalError("%s: Failed to rewind index %s to a previous chain tip",
+                    FatalErrorf("%s: Failed to rewind index %s to a previous chain tip",
                                __func__, GetName());
                     return;
                 }
@@ -221,14 +223,14 @@ void BaseIndex::ThreadSync()
             CBlock block;
             interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex);
             if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *pindex)) {
-                FatalError("%s: Failed to read block %s from disk",
+                FatalErrorf("%s: Failed to read block %s from disk",
                            __func__, pindex->GetBlockHash().ToString());
                 return;
             } else {
                 block_info.data = &block;
             }
             if (!CustomAppend(block_info)) {
-                FatalError("%s: Failed to write block %s to index database",
+                FatalErrorf("%s: Failed to write block %s to index database",
                            __func__, pindex->GetBlockHash().ToString());
                 return;
             }
@@ -294,7 +296,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
     const CBlockIndex* best_block_index = m_best_block_index.load();
     if (!best_block_index) {
         if (pindex->nHeight != 0) {
-            FatalError("%s: First block connected is not the genesis block (height=%d)",
+            FatalErrorf("%s: First block connected is not the genesis block (height=%d)",
                        __func__, pindex->nHeight);
             return;
         }
@@ -312,7 +314,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
             return;
         }
         if (best_block_index != pindex->pprev && !Rewind(best_block_index, pindex->pprev)) {
-            FatalError("%s: Failed to rewind index %s to a previous chain tip",
+            FatalErrorf("%s: Failed to rewind index %s to a previous chain tip",
                        __func__, GetName());
             return;
         }
@@ -325,7 +327,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
         // processed, and the index object being safe to delete.
         SetBestBlockIndex(pindex);
     } else {
-        FatalError("%s: Failed to write block %s to index",
+        FatalErrorf("%s: Failed to write block %s to index",
                    __func__, pindex->GetBlockHash().ToString());
         return;
     }
@@ -345,7 +347,7 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator)
     }
 
     if (!locator_tip_index) {
-        FatalError("%s: First block (hash=%s) in locator was not found",
+        FatalErrorf("%s: First block (hash=%s) in locator was not found",
                    __func__, locator_tip_hash.ToString());
         return;
     }

src/index/base.h

@@ -94,6 +94,9 @@ class BaseIndex : public CValidationInterface
 
     virtual bool AllowPrune() const = 0;
 
+    template <typename... Args>
+    void FatalErrorf(const char* fmt, const Args&... args);
+
 protected:
     std::unique_ptr<interfaces::Chain> m_chain;
     Chainstate* m_chainstate{nullptr};

src/init.cpp

@@ -812,10 +812,6 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status)
     // Enable heap terminate-on-corruption
     HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0);
 #endif
-    if (!InitShutdownState(exit_status)) {
-        return InitError(Untranslated("Initializing wait-for-shutdown state failed."));
-    }
-
     if (!SetupNetworking()) {
         return InitError(Untranslated("Initializing networking failed."));
     }
@@ -988,7 +984,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 
     // Also report errors from parsing before daemonization
     {
-        KernelNotifications notifications{};
+        kernel::Notifications notifications{};
         ChainstateManager::Options chainman_opts_dummy{
             .chainparams = chainparams,
             .datadir = args.GetDataDirNet(),
@@ -1001,6 +997,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         BlockManager::Options blockman_opts_dummy{
             .chainparams = chainman_opts_dummy.chainparams,
             .blocks_dir = args.GetBlocksDirPath(),
+            .notifications = chainman_opts_dummy.notifications,
         };
         auto blockman_result{ApplyArgsManOptions(args, blockman_opts_dummy)};
         if (!blockman_result) {
@@ -1411,7 +1408,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 7: load block chain
 
-    node.notifications = std::make_unique<KernelNotifications>();
+    node.notifications = std::make_unique<KernelNotifications>(node.exit_status);
     fReindex = args.GetBoolArg("-reindex", false);
     bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false);
     ChainstateManager::Options chainman_opts{
@@ -1425,6 +1422,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     BlockManager::Options blockman_opts{
         .chainparams = chainman_opts.chainparams,
         .blocks_dir = args.GetBlocksDirPath(),
+        .notifications = chainman_opts.notifications,
     };
     Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
 
@@ -1464,7 +1462,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>(chainman_opts, blockman_opts);
+        node.chainman = std::make_unique<ChainstateManager>(node.kernel->interrupt, chainman_opts, blockman_opts);
         ChainstateManager& chainman = *node.chainman;
 
         node::ChainstateLoadOptions options;

src/kernel/blockmanager_opts.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H
 #define BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H
 
+#include <kernel/notifications_interface.h>
 #include <util/fs.h>
 
 #include <cstdint>
@@ -25,6 +26,7 @@ struct BlockManagerOpts {
     bool fast_prune{false};
     bool stop_after_block_import{DEFAULT_STOPAFTERBLOCKIMPORT};
     const fs::path blocks_dir;
+    Notifications& notifications;
 };
 
 } // namespace kernel

src/kernel/context.cpp

@@ -14,9 +14,12 @@
 
 
 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();
@@ -26,6 +29,8 @@ Context::Context()
 Context::~Context()
 {
     ECC_Stop();
+    assert(g_context);
+    g_context = nullptr;
 }
 
 } // namespace kernel

src/kernel/context.h

@@ -5,6 +5,8 @@
 #ifndef BITCOIN_KERNEL_CONTEXT_H
 #define BITCOIN_KERNEL_CONTEXT_H
 
+#include <util/signalinterrupt.h>
+
 #include <memory>
 
 namespace kernel {
@@ -16,12 +18,24 @@ 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

src/kernel/mempool_persist.cpp

@@ -9,13 +9,13 @@
 #include <logging.h>
 #include <primitives/transaction.h>
 #include <serialize.h>
-#include <shutdown.h>
 #include <streams.h>
 #include <sync.h>
 #include <txmempool.h>
 #include <uint256.h>
 #include <util/fs.h>
 #include <util/fs_helpers.h>
+#include <util/signalinterrupt.h>
 #include <util/time.h>
 #include <validation.h>
 
@@ -95,7 +95,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
             } else {
                 ++expired;
             }
-            if (ShutdownRequested())
+            if (active_chainstate.m_chainman.m_interrupt)
                 return false;
         }
         std::map<uint256, CAmount> mapDeltas;

src/kernel/notifications_interface.h

@@ -5,12 +5,13 @@
 #ifndef BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H
 #define BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H
 
+#include <util/translation.h>
+
 #include <cstdint>
 #include <string>
 
 class CBlockIndex;
 enum class SynchronizationState;
-struct bilingual_str;
 
 namespace kernel {
 
@@ -27,6 +28,23 @@ class Notifications
     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) {}
+
+    //! The flush error notification is sent to notify the user that an error
+    //! occurred while flushing block data to disk. Kernel code may ignore flush
+    //! errors that don't affect the immediate operation it is trying to
+    //! perform. Applications can choose to handle the flush error notification
+    //! by logging the error, or notifying the user, or triggering an early
+    //! shutdown as a precaution against causing more errors.
+    virtual void flushError(const std::string& debug_message) {}
+
+    //! The fatal error notification is sent to notify the user when an error
+    //! occurs in kernel code that can't be recovered from. After this
+    //! notification is sent, whatever function triggered the error should also
+    //! return an error code or raise an exception. Applications can choose to
+    //! handle the fatal error notification by logging the error, or notifying
+    //! the user, or triggering an early shutdown as a precaution against
+    //! causing more errors.
+    virtual void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) {}
 };
 } // namespace kernel