From 9ddf7e03a35592617a016418fd320cc93c8d1abd Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 29 May 2023 18:56:58 -0300 Subject: move ThreadImport ABC error to use AbortNode 'StartShutdown' should only be used for user requested shutdowns. Internal errors that cause a shutdown should use 'AbortNode'. --- src/node/blockstorage.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 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; } } -- cgit v1.2.3 From 3c06926cf21dcca3074ef51506f556b2286c299b Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Wed, 7 Jun 2023 00:04:57 +0200 Subject: refactor: index: use `AbortNode` in fatal error helper Deduplicates code in the `FatalError` template function by using `AbortNode` which does the exact same thing if called without any user message (i.e. without second parameter specified). The template is still kept for ease-of-use w.r.t. not having to call `tfm::format(...)` at the call-side each time, and also to keep the diff minimal. --- src/index/base.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) 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 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) -- cgit v1.2.3 From 3b2c61e8198bcefb1c2343caff1d705951026cc4 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 20 May 2023 10:51:17 -0300 Subject: Return EXIT_FAILURE on post-init fatal errors 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. Co-authored-by: Ryan Ofsky --- src/bitcoind.cpp | 6 ++++-- src/init.cpp | 4 ++-- src/init.h | 2 +- src/node/context.h | 3 +++ src/node/interfaces.cpp | 2 +- src/shutdown.cpp | 7 ++++++- src/shutdown.h | 4 +++- test/functional/feature_abortnode.py | 2 +- test/functional/test_framework/test_node.py | 16 +++++++++++----- 9 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index aefb130e9c..8982d0a316 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -169,7 +169,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) // 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; } @@ -238,6 +238,8 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF); if (fRet) { WaitForShutdown(); + } else { + node.exit_status = EXIT_FAILURE; } Interrupt(node); Shutdown(node); @@ -264,5 +266,5 @@ MAIN_FUNCTION // Connect bitcoind signal handlers noui_connect(); - return (AppInit(node, argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE); + return AppInit(node, argc, argv) ? node.exit_status.load() : EXIT_FAILURE; } 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 g_enabled_filter_types; std::terminate(); }; -bool AppInitBasicSetup(const ArgsManager& args) +bool AppInitBasicSetup(const ArgsManager& args, std::atomic& 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& exit_status); /** * Initialization: parameter interaction. * @note This can be done before daemonization. Do not call Shutdown() if this function fails. 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 +#include #include +#include #include #include #include @@ -65,6 +67,7 @@ struct NodeContext { std::unique_ptr scheduler; std::function rpc_interruption_point = [] {}; std::unique_ptr notifications; + std::atomic 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 6e39ccf34e..bf42167c2f 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -92,7 +92,7 @@ public: 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(); 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 #include +#include #include #include @@ -20,6 +21,8 @@ #include #endif +static std::atomic* 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& exit_status) { + g_exit_status = &exit_status; #ifndef WIN32 std::optional 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 // For bilingual_str +#include + /** 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& exit_status); /** Request shutdown of the application. */ void StartShutdown(); diff --git a/test/functional/feature_abortnode.py b/test/functional/feature_abortnode.py index fa1bb6506a..586722aa65 100755 --- a/test/functional/feature_abortnode.py +++ b/test/functional/feature_abortnode.py @@ -40,7 +40,7 @@ class AbortNodeTest(BitcoinTestFramework): # 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 index 51bd697e81..4466bd544f 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -365,7 +365,7 @@ class TestNode(): 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,8 +377,13 @@ class TestNode(): 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 @@ -386,8 +391,9 @@ class TestNode(): 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): """ -- cgit v1.2.3 From 4927167f855f8ed3bbf6d2766f61229f742e632a Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 8 Jun 2023 12:16:23 -0300 Subject: gui: return EXIT_FAILURE on post-init fatal errors --- src/interfaces/node.h | 3 +++ src/node/interfaces.cpp | 6 +++++- src/qt/bitcoin.cpp | 11 ++++------- src/qt/bitcoin.h | 4 ---- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 7e87d5a523..f074214569 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/interfaces.cpp b/src/node/interfaces.cpp index bf42167c2f..f741bd6d03 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -89,6 +89,7 @@ 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 { @@ -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 #include +#include #include #include #include @@ -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 shutdownWindow; SplashScreen* m_splash = nullptr; -- cgit v1.2.3 From 61c569ab6069d04079a0831468eb713983919636 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 9 Jun 2023 11:05:11 -0300 Subject: refactor: decouple early return commands from AppInit Cleaned up the init flow to make it more obvious when the 'exit_status' value will and won't be returned. This is because it was confusing that `AppInit` was returning true under two different circumstances: 1) When bitcoind was launched only to retrieve the "-help" or "-version" information. In this case, the app was not initialized. 2) When the user triggers a shutdown. In this case, the app was fully initialized. --- src/bitcoind.cpp | 68 ++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 8982d0a316..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,17 +171,6 @@ 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 @@ -236,14 +243,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) } #endif SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF); - if (fRet) { - WaitForShutdown(); - } else { - node.exit_status = EXIT_FAILURE; - } - Interrupt(node); - Shutdown(node); - return fRet; } @@ -266,5 +265,22 @@ MAIN_FUNCTION // Connect bitcoind signal handlers noui_connect(); - return AppInit(node, argc, argv) ? node.exit_status.load() : 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; } -- cgit v1.2.3