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

ryanofsky · ryanofsky · commit c92fd638860c · 2023-06-12T12:54:49.000-04:00
61c569a refactor: decouple early return commands from AppInit (furszy) 4927167 gui: return EXIT_FAILURE on post-init fatal errors (furszy) 3b2c61e Return EXIT_FAILURE on post-init fatal errors (furszy) 3c06926 refactor: index: use `AbortNode` in fatal error helper (Sebastian Falbesoner) 9ddf7e0 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 61c569a ryanofsky: Code review ACK 61c569a pinheadmz: ACK 61c569a theStack: Code-review ACK 61c569a Tree-SHA512: 18a59c3acc1c6d12cbc74a20a401e89659740c6477fccb59070c9f97922dfe588468e9e5eef56c5f395762187c34179a5e3954aa5b844787fa13da2e666c63d3
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -80,6 +80,9 @@ class Node
     //! 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
@@ -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
@@ -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
@@ -89,10 +89,11 @@ class NodeImpl : public Node
     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 @@ class NodeImpl : public Node
     }
     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
@@ -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
@@ -62,9 +62,6 @@ class BitcoinApplication: public QApplication
     /// 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 @@ public Q_SLOTS:
     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
@@ -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
@@ -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();
diff --git a/test/functional/feature_abortnode.py b/test/functional/feature_abortnode.py
@@ -40,7 +40,7 @@ def run_test(self):
 
             # Check that node0 aborted
             self.log.info("Waiting for crash")
-            self.nodes[0].wait_until_stopped(timeout=5)
+            self.nodes[0].wait_until_stopped(timeout=5, expect_error=True)
         self.log.info("Node crashed - now verifying restart fails")
         self.nodes[0].assert_start_raises_init_error()
 
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
@@ -365,7 +365,7 @@ def stop_node(self, expected_stderr='', *, wait=0, wait_until_stopped=True):
         if wait_until_stopped:
             self.wait_until_stopped()
 
-    def is_node_stopped(self):
+    def is_node_stopped(self, expected_ret_code=None):
         """Checks whether the node has stopped.
 
         Returns True if the node has stopped. False otherwise.
@@ -377,17 +377,23 @@ def is_node_stopped(self):
             return False
 
         # process has stopped. Assert that it didn't return an error code.
-        assert return_code == 0, self._node_msg(
-            "Node returned non-zero exit code (%d) when stopping" % return_code)
+        # unless 'expected_ret_code' is provided.
+        if expected_ret_code is not None:
+            assert return_code == expected_ret_code, self._node_msg(
+                "Node returned unexpected exit code (%d) vs (%d) when stopping" % (return_code, expected_ret_code))
+        else:
+            assert return_code == 0, self._node_msg(
+                "Node returned non-zero exit code (%d) when stopping" % return_code)
         self.running = False
         self.process = None
         self.rpc_connected = False
         self.rpc = None
         self.log.debug("Node stopped")
         return True
 
-    def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT):
-        wait_until_helper(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
+    def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT, expect_error=False):
+        expected_ret_code = 1 if expect_error else None  # Whether node shutdown return EXIT_FAILURE or EXIT_SUCCESS
+        wait_until_helper(lambda: self.is_node_stopped(expected_ret_code=expected_ret_code), timeout=timeout, timeout_factor=self.timeout_factor)
 
     def replace_in_config(self, replacements):
         """

Original file line number	Diff line number	Diff line change
`@@ -33,11 +33,7 @@ constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s};`
`33`	`33`	`template <typename... Args>`
`34`	`34`	`static void FatalError(const char* fmt, const Args&... args)`
`35`	`35`	`{`
`36`		`- std::string strMessage = tfm::format(fmt, args...);`
`37`		`- SetMiscWarning(Untranslated(strMessage));`
`38`		`- LogPrintf("*** %s\n", strMessage);`
`39`		`- InitError(_("A fatal internal error occurred, see debug.log for details"));`
`40`		`- StartShutdown();`
	`36`	`+ AbortNode(tfm::format(fmt, args...));`
`41`	`37`	`}`
`42`	`38`
`43`	`39`	`CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)`
Original file line number	Diff line number	Diff line change
`@@ -928,8 +928,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile`
`928`	`928`	`for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {`
`929`	`929`	`BlockValidationState state;`
`930`	`930`	`if (!chainstate->ActivateBestChain(state, nullptr)) {`
`931`		`- LogPrintf("Failed to connect best block (%s)\n", state.ToString());`
`932`		`- StartShutdown();`
	`931`	`+ AbortNode(strprintf("Failed to connect best block (%s)", state.ToString()));`
`933`	`932`	`return;`
`934`	`933`	`}`
`935`	`934`	`}`
Original file line number	Diff line number	Diff line change
`@@ -89,10 +89,11 @@ class NodeImpl : public Node`
`89`	`89`	`void initLogging() override { InitLogging(args()); }`
`90`	`90`	`void initParameterInteraction() override { InitParameterInteraction(args()); }`
`91`	`91`	`bilingual_str getWarnings() override { return GetWarnings(true); }`
	`92`	`+ int getExitStatus() override { return Assert(m_context)->exit_status.load(); }`
`92`	`93`	`uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }`
`93`	`94`	`bool baseInitialize() override`
`94`	`95`	`{`
`95`		`- if (!AppInitBasicSetup(args())) return false;`
	`96`	`+ if (!AppInitBasicSetup(args(), Assert(context())->exit_status)) return false;`
`96`	`97`	`if (!AppInitParameterInteraction(args(), /use_syscall_sandbox=/false)) return false;`
`97`	`98`
`98`	`99`	`m_context->kernel = std::make_unique<kernel::Context>();`
`@@ -105,7 +106,10 @@ class NodeImpl : public Node`
`105`	`106`	`}`
`106`	`107`	`bool appInitMain(interfaces::BlockAndHeaderTipInfo* tip_info) override`
`107`	`108`	`{`
`108`		`- return AppInitMain(*m_context, tip_info);`
	`109`	`+ if (AppInitMain(*m_context, tip_info)) return true;`
	`110`	`+ // Error during initialization, set exit status before continue`
	`111`	`+ m_context->exit_status.store(EXIT_FAILURE);`
	`112`	`+ return false;`
`109`	`113`	`}`
`110`	`114`	`void appShutdown() override`
`111`	`115`	`{`