Merge bitcoin/bitcoin#27708: Return EXIT_FAILURE on post-init fatal errors

61c569ab6069d04079a0831468eb713983919636 refactor: decouple early return commands from AppInit (furszy)
4927167f855f8ed3bbf6d2766f61229f742e632a gui: return EXIT_FAILURE on post-init fatal errors (furszy)
3b2c61e8198bcefb1c2343caff1d705951026cc4 Return EXIT_FAILURE on post-init fatal errors (furszy)
3c06926cf21dcca3074ef51506f556b2286c299b refactor: index: use `AbortNode` in fatal error helper (Sebastian Falbesoner)
9ddf7e03a35592617a016418fd320cc93c8d1abd move ThreadImport ABC error to use AbortNode (furszy)

Pull request description:

  It seems odd to return `EXIT_SUCCESS` when the node aborted execution due a fatal internal error
  or any post-init problem that triggers an unrequested shutdown.

  e.g. blocks or coins db I/O errors, disconnect block failure, failure during thread import (external
  blocks loading process error), among others.

ACKs for top commit:
  TheCharlatan:
    ACK 61c569ab6069d04079a0831468eb713983919636
  ryanofsky:
    Code review ACK 61c569ab6069d04079a0831468eb713983919636
  pinheadmz:
    ACK 61c569ab6069d04079a0831468eb713983919636
  theStack:
    Code-review ACK 61c569ab6069d04079a0831468eb713983919636

Tree-SHA512: 18a59c3acc1c6d12cbc74a20a401e89659740c6477fccb59070c9f97922dfe588468e9e5eef56c5f395762187c34179a5e3954aa5b844787fa13da2e666c63d3

12 files changed, 73 insertions, 50 deletions

diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index aefb130e9c..c561f9aa14 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -112,20 +112,30 @@ int fork_daemon(bool nochdir, bool noclose, TokenPipeEnd& endpoint)
 
 #endif
 
-static bool AppInit(NodeContext& node, int argc, char* argv[])
+static bool ParseArgs(ArgsManager& args, int argc, char* argv[])
 {
-    bool fRet = false;
-
-    util::ThreadSetInternalName("init");
-
     // If Qt is used, parameters/bitcoin.conf are parsed in qt/bitcoin.cpp's main()
-    ArgsManager& args = *Assert(node.args);
     SetupServerArgs(args);
     std::string error;
     if (!args.ParseParameters(argc, argv, error)) {
         return InitError(Untranslated(strprintf("Error parsing command line arguments: %s", error)));
     }
 
+    if (auto error = common::InitConfig(args)) {
+        return InitError(error->message, error->details);
+    }
+
+    // Error out when loose non-argument tokens are encountered on command line
+    for (int i = 1; i < argc; i++) {
+        if (!IsSwitchChar(argv[i][0])) {
+            return InitError(Untranslated(strprintf("Command line contains unexpected token '%s', see bitcoind -h for a list of options.", argv[i])));
+        }
+    }
+    return true;
+}
+
+static bool ProcessInitCommands(ArgsManager& args)
+{
     // Process help and version before taking care about datadir
     if (HelpRequested(args) || args.IsArgSet("-version")) {
         std::string strUsage = PACKAGE_NAME " version " + FormatFullVersion() + "\n";
@@ -142,6 +152,14 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
         return true;
     }
 
+    return false;
+}
+
+static bool AppInit(NodeContext& node)
+{
+    bool fRet = false;
+    ArgsManager& args = *Assert(node.args);
+
 #if HAVE_DECL_FORK
     // Communication with parent after daemonizing. This is used for signalling in the following ways:
     // - a boolean token is sent when the initialization process (all the Init* functions) have finished to indicate
@@ -153,23 +171,12 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
     std::any context{&node};
     try
     {
-        if (auto error = common::InitConfig(args)) {
-            return InitError(error->message, error->details);
-        }
-
-        // Error out when loose non-argument tokens are encountered on command line
-        for (int i = 1; i < argc; i++) {
-            if (!IsSwitchChar(argv[i][0])) {
-                return InitError(Untranslated(strprintf("Command line contains unexpected token '%s', see bitcoind -h for a list of options.", argv[i])));
-            }
-        }
-
         // -server defaults to true for bitcoind but not for the GUI so do this here
         args.SoftSetBoolArg("-server", true);
         // Set this early so that parameter interactions go to console
         InitLogging(args);
         InitParameterInteraction(args);
-        if (!AppInitBasicSetup(args)) {
+        if (!AppInitBasicSetup(args, node.exit_status)) {
             // InitError will have been called with detailed error, which ends up on console
             return false;
         }
@@ -236,12 +243,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
     }
 #endif
     SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF);
-    if (fRet) {
-        WaitForShutdown();
-    }
-    Interrupt(node);
-    Shutdown(node);
-
     return fRet;
 }
 
@@ -264,5 +265,22 @@ MAIN_FUNCTION
     // Connect bitcoind signal handlers
     noui_connect();
 
-    return (AppInit(node, argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE);
+    util::ThreadSetInternalName("init");
+
+    // Interpret command line arguments
+    ArgsManager& args = *Assert(node.args);
+    if (!ParseArgs(args, argc, argv)) return EXIT_FAILURE;
+    // Process early info return commands such as -help or -version
+    if (ProcessInitCommands(args)) return EXIT_SUCCESS;
+
+    // Start application
+    if (AppInit(node)) {
+        WaitForShutdown();
+    } else {
+        node.exit_status = EXIT_FAILURE;
+    }
+    Interrupt(node);
+    Shutdown(node);
+
+    return node.exit_status;
 }
diff --git a/src/index/base.cpp b/src/index/base.cpp
index 3f91910db2..a713be3480 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -33,11 +33,7 @@ constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s};
 template <typename... Args>
 static void FatalError(const char* fmt, const Args&... args)
 {
-    std::string strMessage = tfm::format(fmt, args...);
-    SetMiscWarning(Untranslated(strMessage));
-    LogPrintf("*** %s\n", strMessage);
-    InitError(_("A fatal internal error occurred, see debug.log for details"));
-    StartShutdown();
+    AbortNode(tfm::format(fmt, args...));
 }
 
 CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)
diff --git a/src/init.cpp b/src/init.cpp
index e8c804b9a7..38e1dbb4a2 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -800,7 +800,7 @@ std::set<BlockFilterType> g_enabled_filter_types;
     std::terminate();
 };
 
-bool AppInitBasicSetup(const ArgsManager& args)
+bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status)
 {
     // ********************************************************* Step 1: setup
 #ifdef _MSC_VER
@@ -814,7 +814,7 @@ bool AppInitBasicSetup(const ArgsManager& args)
     // Enable heap terminate-on-corruption
     HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0);
 #endif
-    if (!InitShutdownState()) {
+    if (!InitShutdownState(exit_status)) {
         return InitError(Untranslated("Initializing wait-for-shutdown state failed."));
     }
 
diff --git a/src/init.h b/src/init.h
index 8c5b2e77d3..fabb4f66d6 100644
--- a/src/init.h
+++ b/src/init.h
@@ -38,7 +38,7 @@ void InitParameterInteraction(ArgsManager& args);
  *  @note This can be done before daemonization. Do not call Shutdown() if this function fails.
  *  @pre Parameters should be parsed and config file should be read.
  */
-bool AppInitBasicSetup(const ArgsManager& args);
+bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status);
 /**
  * Initialization: parameter interaction.
  * @note This can be done before daemonization. Do not call Shutdown() if this function fails.
diff --git a/src/interfaces/node.h b/src/interfaces/node.h
index 479c585b88..3f8df57124 100644
--- a/src/interfaces/node.h
+++ b/src/interfaces/node.h
@@ -80,6 +80,9 @@ public:
     //! Get warnings.
     virtual bilingual_str getWarnings() = 0;
 
+    //! Get exit status.
+    virtual int getExitStatus() = 0;
+
     // Get log flags.
     virtual uint32_t getLogCategories() = 0;
 
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index b7afa8a7c3..1368ae6f6d 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -928,8 +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)) {
-                LogPrintf("Failed to connect best block (%s)\n", state.ToString());
-                StartShutdown();
+                AbortNode(strprintf("Failed to connect best block (%s)", state.ToString()));
                 return;
             }
         }
diff --git a/src/node/context.h b/src/node/context.h
index 9532153cdb..91b68fa5bb 100644
--- a/src/node/context.h
+++ b/src/node/context.h
@@ -7,7 +7,9 @@
 
 #include <kernel/context.h>
 
+#include <atomic>
 #include <cassert>
+#include <cstdlib>
 #include <functional>
 #include <memory>
 #include <vector>
@@ -65,6 +67,7 @@ struct NodeContext {
     std::unique_ptr<CScheduler> scheduler;
     std::function<void()> rpc_interruption_point = [] {};
     std::unique_ptr<KernelNotifications> notifications;
+    std::atomic<int> exit_status{EXIT_SUCCESS};
 
     //! Declare default constructor and destructor that are not inline, so code
     //! instantiating the NodeContext struct doesn't need to #include class
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 57c0d302fd..94b607b1de 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -89,10 +89,11 @@ public:
     void initLogging() override { InitLogging(args()); }
     void initParameterInteraction() override { InitParameterInteraction(args()); }
     bilingual_str getWarnings() override { return GetWarnings(true); }
+    int getExitStatus() override { return Assert(m_context)->exit_status.load(); }
     uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
     bool baseInitialize() override
     {
-        if (!AppInitBasicSetup(args())) return false;
+        if (!AppInitBasicSetup(args(), Assert(context())->exit_status)) return false;
         if (!AppInitParameterInteraction(args(), /*use_syscall_sandbox=*/false)) return false;
 
         m_context->kernel = std::make_unique<kernel::Context>();
@@ -105,7 +106,10 @@ public:
     }
     bool appInitMain(interfaces::BlockAndHeaderTipInfo* tip_info) override
     {
-        return AppInitMain(*m_context, tip_info);
+        if (AppInitMain(*m_context, tip_info)) return true;
+        // Error during initialization, set exit status before continue
+        m_context->exit_status.store(EXIT_FAILURE);
+        return false;
     }
     void appShutdown() override
     {
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index e33753f5e7..8f45af9485 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -9,6 +9,7 @@
 #include <qt/bitcoin.h>
 
 #include <chainparams.h>
+#include <node/context.h>
 #include <common/args.h>
 #include <common/init.h>
 #include <common/system.h>
@@ -397,9 +398,7 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead
 {
     qDebug() << __func__ << ": Initialization result: " << success;
 
-    // Set exit result.
-    returnValue = success ? EXIT_SUCCESS : EXIT_FAILURE;
-    if(success) {
+    if (success) {
         delete m_splash;
         m_splash = nullptr;
 
@@ -653,7 +652,6 @@ int GuiMain(int argc, char* argv[])
         app.InitPruneSetting(prune_MiB);
     }
 
-    int rv = EXIT_SUCCESS;
     try
     {
         app.createWindow(networkStyle.data());
@@ -666,10 +664,9 @@ int GuiMain(int argc, char* argv[])
             WinShutdownMonitor::registerShutdownBlockReason(QObject::tr("%1 didn't yet exit safely…").arg(PACKAGE_NAME), (HWND)app.getMainWinId());
 #endif
             app.exec();
-            rv = app.getReturnValue();
         } else {
             // A dialog with detailed error will have been shown by InitError()
-            rv = EXIT_FAILURE;
+            return EXIT_FAILURE;
         }
     } catch (const std::exception& e) {
         PrintExceptionContinue(&e, "Runaway exception");
@@ -678,5 +675,5 @@ int GuiMain(int argc, char* argv[])
         PrintExceptionContinue(nullptr, "Runaway exception");
         app.handleRunawayException(QString::fromStdString(app.node().getWarnings().translated));
     }
-    return rv;
+    return app.node().getExitStatus();
 }
diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h
index 9174e23de6..9622c9d57d 100644
--- a/src/qt/bitcoin.h
+++ b/src/qt/bitcoin.h
@@ -62,9 +62,6 @@ public:
     /// Request core initialization
     void requestInitialize();
 
-    /// Get process return value
-    int getReturnValue() const { return returnValue; }
-
     /// Get window identifier of QMainWindow (BitcoinGUI)
     WId getMainWinId() const;
 
@@ -104,7 +101,6 @@ private:
     PaymentServer* paymentServer{nullptr};
     WalletController* m_wallet_controller{nullptr};
 #endif
-    int returnValue{0};
     const PlatformStyle* platformStyle{nullptr};
     std::unique_ptr<QWidget> shutdownWindow;
     SplashScreen* m_splash = nullptr;
diff --git a/src/shutdown.cpp b/src/shutdown.cpp
index 2fffc0663c..d70017d734 100644
--- a/src/shutdown.cpp
+++ b/src/shutdown.cpp
@@ -11,6 +11,7 @@
 
 #include <logging.h>
 #include <node/interface_ui.h>
+#include <util/check.h>
 #include <util/tokenpipe.h>
 #include <warnings.h>
 
@@ -20,6 +21,8 @@
 #include <condition_variable>
 #endif
 
+static std::atomic<int>* g_exit_status{nullptr};
+
 bool AbortNode(const std::string& strMessage, bilingual_str user_message)
 {
     SetMiscWarning(Untranslated(strMessage));
@@ -28,6 +31,7 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message)
         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;
 }
@@ -44,8 +48,9 @@ static TokenPipeEnd g_shutdown_r;
 static TokenPipeEnd g_shutdown_w;
 #endif
 
-bool InitShutdownState()
+bool InitShutdownState(std::atomic<int>& exit_status)
 {
+    g_exit_status = &exit_status;
 #ifndef WIN32
     std::optional<TokenPipe> pipe = TokenPipe::Make();
     if (!pipe) return false;
diff --git a/src/shutdown.h b/src/shutdown.h
index 07a8315788..c119bee96f 100644
--- a/src/shutdown.h
+++ b/src/shutdown.h
@@ -8,13 +8,15 @@
 
 #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.
  */
-bool InitShutdownState();
+bool InitShutdownState(std::atomic<int>& exit_status);
 
 /** Request shutdown of the application. */
 void StartShutdown();