diff options
author | Ryan Ofsky <ryan@ofsky.org> | 2023-06-12 12:13:55 -0400 |
---|---|---|
committer | Ryan Ofsky <ryan@ofsky.org> | 2023-06-12 12:54:49 -0400 |
commit | c92fd638860c5b4477851fb3790bc8068f808d3e (patch) | |
tree | 063d1c5002af6f03f8a7a3a460314ee2eb3049dc | |
parent | 361a0c00b3fdac7149c027c2c69e4db60ee5aa86 (diff) | |
parent | 61c569ab6069d04079a0831468eb713983919636 (diff) | |
download | bitcoin-c92fd638860c5b4477851fb3790bc8068f808d3e.tar.xz |
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
-rw-r--r-- | src/bitcoind.cpp | 68 | ||||
-rw-r--r-- | src/index/base.cpp | 6 | ||||
-rw-r--r-- | src/init.cpp | 4 | ||||
-rw-r--r-- | src/init.h | 2 | ||||
-rw-r--r-- | src/interfaces/node.h | 3 | ||||
-rw-r--r-- | src/node/blockstorage.cpp | 3 | ||||
-rw-r--r-- | src/node/context.h | 3 | ||||
-rw-r--r-- | src/node/interfaces.cpp | 8 | ||||
-rw-r--r-- | src/qt/bitcoin.cpp | 11 | ||||
-rw-r--r-- | src/qt/bitcoin.h | 4 | ||||
-rw-r--r-- | src/shutdown.cpp | 7 | ||||
-rw-r--r-- | src/shutdown.h | 4 | ||||
-rwxr-xr-x | test/functional/feature_abortnode.py | 2 | ||||
-rwxr-xr-x | test/functional/test_framework/test_node.py | 16 |
14 files changed, 85 insertions, 56 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(); 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): """ |