kernel: Add fatalError method to notifications

FatalError replaces what previously was the AbortNode function in
shutdown.cpp.

This commit is part of the libbitcoinkernel project and removes the
shutdown's and, more generally, the kernel library's dependency on
interface_ui with an options method. By removing interface_ui from the
kernel library, its dependency on boost is reduced to just
boost::multi_index.

Also add shutdown switch in node::KernelNotifications. This simplifies
testing of validation methods that may normally trigger a shutdown on a
fatal error by leaving out the call to StartShutdown.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>

src/Makefile.am

@@ -939,7 +939,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 \

src/bitcoin-chainstate.cpp

@@ -90,7 +90,11 @@ int main(int argc, char* argv[])
 
     class KernelNotifications : public kernel::Notifications
     {
+    private:
+        std::atomic<bool>& m_shutdown_requested;
+
     public:
+        KernelNotifications(std::atomic<bool>& shutdown_requested) : m_shutdown_requested{shutdown_requested} {}
         void blockTip(SynchronizationState, CBlockIndex&) override
         {
             std::cout << "Block tip changed" << std::endl;
@@ -107,8 +111,14 @@ int main(int argc, char* argv[])
         {
             std::cout << "Warning: " << warning.original << std::endl;
         }
+        void fatalError(const std::string& debug_message, const bilingual_str& user_message) override
+        {
+            m_shutdown_requested = true;
+            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>();
+    auto notifications = std::make_unique<KernelNotifications>(shutdown_requested);
 
     // SETUP: Chainstate
     const ChainstateManager::Options chainman_opts{
@@ -122,6 +132,7 @@ int main(int argc, char* argv[])
         .chainparams = chainman_opts.chainparams,
         .blocks_dir = gArgs.GetBlocksDirPath(),
         .shutdown_requested = chainman_opts.shutdown_requested,
+        .notifications = chainman_opts.notifications,
     };
     ChainstateManager chainman{chainman_opts, blockman_opts};
 

src/init.cpp

@@ -1037,6 +1037,7 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
             .chainparams = chainman_opts_dummy.chainparams,
             .blocks_dir = args.GetBlocksDirPath(),
             .shutdown_requested = chainman_opts_dummy.shutdown_requested,
+            .notifications = chainman_opts_dummy.notifications,
         };
         auto blockman_result{ApplyArgsManOptions(args, blockman_opts_dummy)};
         if (!blockman_result) {
@@ -1453,6 +1454,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         .chainparams = chainman_opts.chainparams,
         .blocks_dir = args.GetBlocksDirPath(),
         .shutdown_requested = chainman_opts.shutdown_requested,
+        .notifications = chainman_opts.notifications,
     };
     Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
 

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 <atomic>
@@ -27,6 +28,7 @@ struct BlockManagerOpts {
     bool stop_after_block_import{DEFAULT_STOPAFTERBLOCKIMPORT};
     const fs::path blocks_dir;
     std::atomic<bool>& shutdown_requested;
+    Notifications& notifications;
 };
 
 } // namespace kernel

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,12 @@ 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 fatal error notification is sent to notify the user and start
+    //! shutting down if an error happens 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.
+    virtual void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) {}
 };
 } // namespace kernel
 

src/node/blockstorage.cpp

@@ -529,7 +529,7 @@ void BlockManager::FlushUndoFile(int block_file, bool finalize)
 {
     FlatFilePos undo_pos_old(block_file, m_blockfile_info[block_file].nUndoSize);
     if (!UndoFileSeq().Flush(undo_pos_old, finalize)) {
-        AbortNode("Flushing undo file to disk failed. This is likely the result of an I/O error.");
+        m_opts.notifications.fatalError("Flushing undo file to disk failed. This is likely the result of an I/O error.");
     }
 }
 
@@ -548,7 +548,7 @@ void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo)
 
     FlatFilePos block_pos_old(m_last_blockfile, m_blockfile_info[m_last_blockfile].nSize);
     if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) {
-        AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error.");
+        m_opts.notifications.fatalError("Flushing block file to disk failed. This is likely the result of an I/O error.");
     }
     // we do not always flush the undo file, as the chain tip may be lagging behind the incoming blocks,
     // e.g. during IBD or a sync after a node going offline
@@ -660,7 +660,8 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
         bool out_of_space;
         size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space);
         if (out_of_space) {
-            return AbortNode("Disk space is too low!", _("Disk space is too low!"));
+            m_opts.notifications.fatalError("Disk space is too low!", _("Disk space is too low!"));
+            return false;
         }
         if (bytes_allocated != 0 && IsPruneMode()) {
             m_check_for_pruning = true;
@@ -684,7 +685,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP
     bool out_of_space;
     size_t bytes_allocated = UndoFileSeq().Allocate(pos, nAddSize, out_of_space);
     if (out_of_space) {
-        return AbortNode(state, "Disk space is too low!", _("Disk space is too low!"));
+        return FatalError(m_opts.notifications, state, "Disk space is too low!", _("Disk space is too low!"));
     }
     if (bytes_allocated != 0 && IsPruneMode()) {
         m_check_for_pruning = true;
@@ -726,7 +727,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid
             return error("ConnectBlock(): FindUndoPos failed");
         }
         if (!UndoWriteToDisk(blockundo, _pos, block.pprev->GetBlockHash(), GetParams().MessageStart())) {
-            return AbortNode(state, "Failed to write undo data");
+            return FatalError(m_opts.notifications, state, "Failed to write undo data");
         }
         // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order)
         // we want to flush the rev (undo) file once we've written the last block, which is indicated by the last height
@@ -844,7 +845,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CCha
     }
     if (!position_known) {
         if (!WriteBlockToDisk(block, blockPos, GetParams().MessageStart())) {
-            AbortNode("Failed to write block");
+            m_opts.notifications.fatalError("Failed to write block");
             return FlatFilePos();
         }
     }

src/node/chainstate.cpp

@@ -207,7 +207,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) {
         LogPrintf("[snapshot] cleaning up unneeded background chainstate, then reinitializing\n");
         if (!chainman.ValidatedSnapshotCleanup()) {
-            AbortNode("Background chainstate cleanup failed unexpectedly.");
+            chainman.GetNotifications().fatalError("Background chainstate cleanup failed unexpectedly.");
         }
 
         // Because ValidatedSnapshotCleanup() has torn down chainstates with

src/node/kernel_notifications.cpp

@@ -10,7 +10,9 @@
 
 #include <common/args.h>
 #include <common/system.h>
+#include <logging.h>
 #include <node/interface_ui.h>
+#include <shutdown.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/translation.h>
@@ -72,4 +74,12 @@ void KernelNotifications::warning(const bilingual_str& warning)
     DoWarning(warning);
 }
 
+void KernelNotifications::fatalError(const std::string& debug_message, const bilingual_str& user_message)
+{
+    SetMiscWarning(Untranslated(debug_message));
+    LogPrintf("*** %s\n", debug_message);
+    InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message);
+    if (m_shutdown_on_fatal_error) StartShutdown();
+}
+
 } // namespace node

src/node/kernel_notifications.h

@@ -25,6 +25,11 @@ class KernelNotifications : public kernel::Notifications
     void progress(const bilingual_str& title, int progress_percent, bool resume_possible) override;
 
     void warning(const bilingual_str& warning) override;
+
+    void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) override;
+
+    //! Useful for tests, can be set to false to avoid shutdown on fatal error.
+    bool m_shutdown_on_fatal_error{true};
 };
 } // namespace node
 

src/shutdown.cpp

@@ -10,28 +10,14 @@
 #endif
 
 #include <logging.h>
-#include <node/interface_ui.h>
 #include <util/tokenpipe.h>
-#include <warnings.h>
 
 #include <assert.h>
 #include <atomic>
 #ifdef WIN32
 #include <condition_variable>
 #endif
 
-bool AbortNode(const std::string& strMessage, bilingual_str user_message)
-{
-    SetMiscWarning(Untranslated(strMessage));
-    LogPrintf("*** %s\n", strMessage);
-    if (user_message.empty()) {
-        user_message = _("A fatal internal error occurred, see debug.log for details");
-    }
-    InitError(user_message);
-    StartShutdown();
-    return false;
-}
-
 static std::atomic<bool> fRequestShutdown(false);
 #ifdef WIN32
 /** On windows it is possible to simply use a condition variable. */

src/shutdown.h

@@ -6,13 +6,8 @@
 #ifndef BITCOIN_SHUTDOWN_H
 #define BITCOIN_SHUTDOWN_H
 
-#include <util/translation.h> // For bilingual_str
-
 #include <atomic>
 
-/** Abort with a message */
-bool AbortNode(const std::string& strMessage, bilingual_str user_message = bilingual_str{});
-
 /** Initialize shutdown state. This must be called before using either StartShutdown(),
  * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe.
  */

src/test/blockmanager_tests.cpp

@@ -5,15 +5,17 @@
 #include <chainparams.h>
 #include <node/blockstorage.h>
 #include <node/context.h>
+#include <node/kernel_notifications.h>
 #include <shutdown.h>
 #include <util/chaintype.h>
 #include <validation.h>
 
 #include <boost/test/unit_test.hpp>
 #include <test/util/setup_common.h>
 
-using node::BlockManager;
 using node::BLOCK_SERIALIZATION_HEADER_SIZE;
+using node::BlockManager;
+using node::KernelNotifications;
 using node::MAX_BLOCKFILE_SIZE;
 
 // use BasicTestingSetup here for the data directory configuration, setup, and cleanup
@@ -22,10 +24,12 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
 {
     const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)};
+    KernelNotifications notifications{};
     const BlockManager::Options blockman_opts{
         .chainparams = *params,
         .blocks_dir = m_args.GetBlocksDirPath(),
         .shutdown_requested = GetRequestShutdownGlobal(),
+        .notifications = notifications,
     };
     BlockManager blockman{blockman_opts};
     CChain chain {};

src/test/util/setup_common.cpp

@@ -198,6 +198,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
         .chainparams = chainman_opts.chainparams,
         .blocks_dir = m_args.GetBlocksDirPath(),
         .shutdown_requested = chainman_opts.shutdown_requested,
+        .notifications = chainman_opts.notifications,
     };
     m_node.chainman = std::make_unique<ChainstateManager>(chainman_opts, blockman_opts);
     m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<CBlockTreeDB>(DBParams{

src/test/validation_chainstatemanager_tests.cpp

@@ -392,6 +392,7 @@ struct SnapshotTestSetup : TestChain100Setup {
                 .chainparams = chainman_opts.chainparams,
                 .blocks_dir = m_args.GetBlocksDirPath(),
                 .shutdown_requested = chainman_opts.shutdown_requested,
+                .notifications = chainman_opts.notifications,
             };
             // For robustness, ensure the old manager is destroyed before creating a
             // new one.
@@ -566,7 +567,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
     auto db_cache_before_complete = active_cs.m_coinsdb_cache_size_bytes;
 
     SnapshotCompletionResult res;
-    auto mock_shutdown = [](bilingual_str msg) {};
+    m_node.notifications->m_shutdown_on_fatal_error = false;
 
     fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir();
     BOOST_CHECK(fs::exists(snapshot_chainstate_dir));
@@ -576,8 +577,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
     const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(),
         return chainman.ActiveTip()->GetBlockHash());
 
-    res = WITH_LOCK(::cs_main,
-        return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
+    res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
     BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS);
 
     WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated()));
@@ -593,8 +593,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
     BOOST_CHECK_EQUAL(all_chainstates[0], &active_cs);
 
     // Trying completion again should return false.
-    res = WITH_LOCK(::cs_main,
-        return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
+    res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
     BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SKIPPED);
 
     // The invalid snapshot path should not have been used.
@@ -647,7 +646,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
     Chainstate& validation_chainstate = *std::get<0>(chainstates);
     ChainstateManager& chainman = *Assert(m_node.chainman);
     SnapshotCompletionResult res;
-    auto mock_shutdown = [](bilingual_str msg) {};
+    m_node.notifications->m_shutdown_on_fatal_error = false;
 
     // Test tampering with the IBD UTXO set with an extra coin to ensure it causes
     // snapshot completion to fail.
@@ -663,8 +662,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
     fs::path snapshot_chainstate_dir = gArgs.GetDataDirNet() / "chainstate_snapshot";
     BOOST_CHECK(fs::exists(snapshot_chainstate_dir));
 
-    res = WITH_LOCK(::cs_main,
-        return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
+    res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
     BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::HASH_MISMATCH);
 
     auto all_chainstates = chainman.GetAll();