kernel: Add fatalError method to notifications

TheCharlatan · ryanofsky · TheCharlatan · commit 6eb33bd0c21b · 2023-06-28T09:52:33.000+02:00
FatalError replaces what previously was the AbortNode function in
shutdown.cpp.

This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.

Co-authored-by: Russell Yanofsky &lt;russ@yanofsky.org&gt;
Co-authored-by: TheCharlatan &lt;seb.kung@gmail.com&gt;
diff --git a/src/Makefile.am b/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 \
@@ -400,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 \
@@ -935,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 \
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
@@ -101,7 +101,11 @@ int main(int argc, char* argv[])
         {
             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>();
 
diff --git a/src/index/base.cpp b/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 FatalErrorf(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)
diff --git a/src/index/base.h b/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};
diff --git a/src/init.cpp b/src/init.cpp
@@ -812,8 +812,6 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status)
     // Enable heap terminate-on-corruption
     HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0);
 #endif
-    InitShutdownState(exit_status);
-
     if (!SetupNetworking()) {
         return InitError(Untranslated("Initializing networking failed."));
     }
@@ -986,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(),
@@ -1410,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{
diff --git a/src/kernel/notifications_interface.h b/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 {
 
@@ -35,6 +36,15 @@ class Notifications
     //! 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
 
diff --git a/src/node/abort.cpp b/src/node/abort.cpp
@@ -0,0 +1,27 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <node/abort.h>
+
+#include <logging.h>
+#include <node/interface_ui.h>
+#include <shutdown.h>
+#include <util/translation.h>
+#include <warnings.h>
+
+#include <atomic>
+#include <cstdlib>
+#include <string>
+
+namespace node {
+
+void AbortNode(std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message, bool shutdown)
+{
+    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);
+    exit_status.store(EXIT_FAILURE);
+    if (shutdown) StartShutdown();
+}
+} // namespace node
diff --git a/src/node/abort.h b/src/node/abort.h
@@ -0,0 +1,17 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_NODE_ABORT_H
+#define BITCOIN_NODE_ABORT_H
+
+#include <util/translation.h>
+
+#include <atomic>
+#include <string>
+
+namespace node {
+void AbortNode(std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message = {}, bool shutdown = true);
+} // namespace node
+
+#endif // BITCOIN_NODE_ABORT_H
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
@@ -659,7 +659,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;
@@ -683,7 +684,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;
@@ -725,7 +726,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
@@ -843,7 +844,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();
         }
     }
@@ -927,7 +928,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
         for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
             BlockValidationState state;
             if (!chainstate->ActivateBestChain(state, nullptr)) {
-                AbortNode(strprintf("Failed to connect best block (%s)", state.ToString()));
+                chainman.GetNotifications().fatalError(strprintf("Failed to connect best block (%s)", state.ToString()));
                 return;
             }
         }
diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp
@@ -10,8 +10,12 @@
 
 #include <common/args.h>
 #include <common/system.h>
+#include <kernel/context.h>
+#include <logging.h>
+#include <node/abort.h>
 #include <node/interface_ui.h>
 #include <shutdown.h>
+#include <util/check.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/translation.h>
@@ -75,7 +79,12 @@ void KernelNotifications::warning(const bilingual_str& warning)
 
 void KernelNotifications::flushError(const std::string& debug_message)
 {
-    AbortNode(debug_message);
+    AbortNode(m_exit_status, debug_message);
+}
+
+void KernelNotifications::fatalError(const std::string& debug_message, const bilingual_str& user_message)
+{
+    node::AbortNode(m_exit_status, debug_message, user_message, m_shutdown_on_fatal_error);
 }
 
 } // namespace node
diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h
@@ -7,6 +7,7 @@
 
 #include <kernel/notifications_interface.h>
 
+#include <atomic>
 #include <cstdint>
 #include <string>
 
@@ -18,6 +19,8 @@ namespace node {
 class KernelNotifications : public kernel::Notifications
 {
 public:
+    KernelNotifications(std::atomic<int>& exit_status) : m_exit_status{exit_status} {}
+
     void blockTip(SynchronizationState state, CBlockIndex& index) override;
 
     void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) override;
@@ -27,6 +30,13 @@ class KernelNotifications : public kernel::Notifications
     void warning(const bilingual_str& warning) override;
 
     void flushError(const std::string& debug_message) 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};
+private:
+    std::atomic<int>& m_exit_status;
 };
 } // namespace node
 
diff --git a/src/shutdown.cpp b/src/shutdown.cpp
@@ -5,39 +5,13 @@
 
 #include <shutdown.h>
 
-#if defined(HAVE_CONFIG_H)
-#include <config/bitcoin-config.h>
-#endif
-
 #include <kernel/context.h>
 #include <logging.h>
-#include <node/interface_ui.h>
 #include <util/check.h>
 #include <util/signalinterrupt.h>
-#include <warnings.h>
-
-#include <atomic>
-#include <cassert>
-
-static std::atomic<int>* g_exit_status{nullptr};
 
-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);
-    Assert(g_exit_status)->store(EXIT_FAILURE);
-    StartShutdown();
-    return false;
-}
-
-void InitShutdownState(std::atomic<int>& exit_status)
-{
-    g_exit_status = &exit_status;
-}
+#include <assert.h>
+#include <system_error>
 
 void StartShutdown()
 {
diff --git a/src/shutdown.h b/src/shutdown.h
@@ -6,18 +6,6 @@
 #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.
- */
-void InitShutdownState(std::atomic<int>& exit_status);
-
 /** Request shutdown of the application. */
 void StartShutdown();
 
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
@@ -23,7 +23,7 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
 {
     const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)};
-    KernelNotifications notifications{};
+    KernelNotifications notifications{m_node.exit_status};
     const BlockManager::Options blockman_opts{
         .chainparams = *params,
         .blocks_dir = m_args.GetBlocksDirPath(),
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -184,7 +184,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
 
     m_cache_sizes = CalculateCacheSizes(m_args);
 
-    m_node.notifications = std::make_unique<KernelNotifications>();
+    m_node.notifications = std::make_unique<KernelNotifications>(m_node.exit_status);
 
     const ChainstateManager::Options chainman_opts{
         .chainparams = chainparams,
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
diff --git a/src/validation.cpp b/src/validation.cpp
diff --git a/src/validation.h b/src/validation.h
diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py

Original file line number	Diff line number	Diff line change
`@@ -101,7 +101,11 @@ int main(int argc, char* argv[])`
`101`	`101`	`{`
`102`	`102`	`std::cerr << "Error flushing block data to disk: " << debug_message << std::endl;`
`103`	`103`	`}`
`104`		`-`
	`104`	`+ void fatalError(const std::string& debug_message, const bilingual_str& user_message) override`
	`105`	`+ {`
	`106`	`+ std::cerr << "Error: " << debug_message << std::endl;`
	`107`	`+ std::cerr << (user_message.empty() ? "A fatal internal error occurred." : user_message.original) << std::endl;`
	`108`	`+ }`
`105`	`109`	`};`
`106`	`110`	`auto notifications = std::make_unique<KernelNotifications>();`
`107`	`111`
Original file line number	Diff line number	Diff line change
`@@ -659,7 +659,8 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne`
`659`	`659`	`bool out_of_space;`
`660`	`660`	`size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space);`
`661`	`661`	`if (out_of_space) {`
`662`		`- return AbortNode("Disk space is too low!", _("Disk space is too low!"));`
	`662`	`+ m_opts.notifications.fatalError("Disk space is too low!", _("Disk space is too low!"));`
	`663`	`+ return false;`
`663`	`664`	`}`
`664`	`665`	`if (bytes_allocated != 0 && IsPruneMode()) {`
`665`	`666`	`m_check_for_pruning = true;`
`@@ -683,7 +684,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP`
`683`	`684`	`bool out_of_space;`
`684`	`685`	`size_t bytes_allocated = UndoFileSeq().Allocate(pos, nAddSize, out_of_space);`
`685`	`686`	`if (out_of_space) {`
`686`		`- return AbortNode(state, "Disk space is too low!", _("Disk space is too low!"));`
	`687`	`+ return FatalError(m_opts.notifications, state, "Disk space is too low!", _("Disk space is too low!"));`
`687`	`688`	`}`
`688`	`689`	`if (bytes_allocated != 0 && IsPruneMode()) {`
`689`	`690`	`m_check_for_pruning = true;`
`@@ -725,7 +726,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid`
`725`	`726`	`return error("ConnectBlock(): FindUndoPos failed");`
`726`	`727`	`}`
`727`	`728`	`if (!UndoWriteToDisk(blockundo, _pos, block.pprev->GetBlockHash(), GetParams().MessageStart())) {`
`728`		`- return AbortNode(state, "Failed to write undo data");`
	`729`	`+ return FatalError(m_opts.notifications, state, "Failed to write undo data");`
`729`	`730`	`}`
`730`	`731`	`// rev files are written in block height order, whereas blk files are written as blocks come in (often out of order)`
`731`	`732`	`// we want to flush the rev (undo) file once we've written the last block, which is indicated by the last height`
`@@ -843,7 +844,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CCha`
`843`	`844`	`}`
`844`	`845`	`if (!position_known) {`
`845`	`846`	`if (!WriteBlockToDisk(block, blockPos, GetParams().MessageStart())) {`
`846`		`- AbortNode("Failed to write block");`
	`847`	`+ m_opts.notifications.fatalError("Failed to write block");`
`847`	`848`	`return FlatFilePos();`
`848`	`849`	`}`
`849`	`850`	`}`
`@@ -927,7 +928,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile`
`927`	`928`	`for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {`
`928`	`929`	`BlockValidationState state;`
`929`	`930`	`if (!chainstate->ActivateBestChain(state, nullptr)) {`
`930`		`- AbortNode(strprintf("Failed to connect best block (%s)", state.ToString()));`
	`931`	`+ chainman.GetNotifications().fatalError(strprintf("Failed to connect best block (%s)", state.ToString()));`
`931`	`932`	`return;`
`932`	`933`	`}`
`933`	`934`	`}`