Merge #16366: init: Use InitError for all errors in bitcoind/qt

fa6f402bde Call node->initError instead of InitError from GUI code (Russell Yanofsky)
fad2502240 init: Use InitError for all errors in bitcoind/qt (MarcoFalke)

Pull request description:

  Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again:

  ```sh
  BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args

ACKs for top commit:
  hebasto:
    ACK fa6f402bde
  ryanofsky:
    utACK fa6f402bde. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node `InitError` function and global variables from GUI code.

Tree-SHA512: bd19e08dcea4019dfe40356bc5c63cb583cefed54b6c9dcfb82f1b5b00308d8e2b363549afcaea5e93bf83864dbe0917400c3b70f43a8a5bdff45c9cd34cc294

src/bitcoind.cpp

@@ -11,15 +11,14 @@
 #include <clientversion.h>
 #include <compat.h>
 #include <fs.h>
-#include <interfaces/chain.h>
 #include <init.h>
+#include <interfaces/chain.h>
 #include <noui.h>
 #include <shutdown.h>
+#include <ui_interface.h>
+#include <util/strencodings.h>
 #include <util/system.h>
 #include <util/threadnames.h>
-#include <util/strencodings.h>
-
-#include <stdio.h>
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 
@@ -70,8 +69,7 @@ static bool AppInit(int argc, char* argv[])
     SetupServerArgs();
     std::string error;
     if (!gArgs.ParseParameters(argc, argv, error)) {
-        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error.c_str());
+        return InitError(strprintf("Error parsing command line arguments: %s\n", error));
-        return false;
     }
 
     // Process help and version before taking care about datadir
@@ -96,26 +94,22 @@ static bool AppInit(int argc, char* argv[])
     {
         if (!fs::is_directory(GetDataDir(false)))
         {
-            tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "").c_str());
+            return InitError(strprintf("Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "")));
-            return false;
         }
         if (!gArgs.ReadConfigFiles(error, true)) {
-            tfm::format(std::cerr, "Error reading configuration file: %s\n", error.c_str());
+            return InitError(strprintf("Error reading configuration file: %s\n", error));
-            return false;
         }
         // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
         try {
             SelectParams(gArgs.GetChainName());
         } catch (const std::exception& e) {
-            tfm::format(std::cerr, "Error: %s\n", e.what());
+            return InitError(strprintf("%s\n", e.what()));
-            return false;
         }
 
         // Error out when loose non-argument tokens are encountered on command line
         for (int i = 1; i < argc; i++) {
             if (!IsSwitchChar(argv[i][0])) {
-                tfm::format(std::cerr, "Error: Command line contains unexpected token '%s', see bitcoind -h for a list of options.\n", argv[i]);
+                return InitError(strprintf("Command line contains unexpected token '%s', see bitcoind -h for a list of options.\n", argv[i]));
-                return false;
             }
         }
 
@@ -146,19 +140,17 @@ static bool AppInit(int argc, char* argv[])
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 #endif
-            tfm::format(std::cout, "Bitcoin server starting\n");
+            tfm::format(std::cout, PACKAGE_NAME "daemon starting\n");
 
             // Daemonize
             if (daemon(1, 0)) { // don't chdir (1), do close FDs (0)
-                tfm::format(std::cerr, "Error: daemon() failed: %s\n", strerror(errno));
+                return InitError(strprintf("daemon() failed: %s\n", strerror(errno)));
-                return false;
             }
 #if defined(MAC_OSX)
 #pragma GCC diagnostic pop
 #endif
 #else
-            tfm::format(std::cerr, "Error: -daemon is not supported on this operating system\n");
+            return InitError("-daemon is not supported on this operating system\n");
-            return false;
 #endif // HAVE_DECL_DAEMON
         }
         // Lock data directory after daemonization

src/interfaces/node.cpp

@@ -54,6 +54,7 @@ class NodeImpl : public Node
 {
 public:
     NodeImpl() { m_interfaces.chain = MakeChain(); }
+    void initError(const std::string& message) override { InitError(message); }
     bool parseParameters(int argc, const char* const argv[], std::string& error) override
     {
         return gArgs.ParseParameters(argc, argv, error);

src/interfaces/node.h

@@ -38,6 +38,9 @@ class Node
 public:
     virtual ~Node() {}
 
+    //! Send init error.
+    virtual void initError(const std::string& message) = 0;
+
     //! Set command line arguments.
     virtual bool parseParameters(int argc, const char* const argv[], std::string& error) = 0;
 

src/qt/bitcoin.cpp

@@ -10,8 +10,8 @@
 #include <qt/bitcoingui.h>
 
 #include <chainparams.h>
-#include <qt/clientmodel.h>
 #include <fs.h>
+#include <qt/clientmodel.h>
 #include <qt/guiconstants.h>
 #include <qt/guiutil.h>
 #include <qt/intro.h>
@@ -30,15 +30,12 @@
 #include <interfaces/handler.h>
 #include <interfaces/node.h>
 #include <noui.h>
-#include <util/threadnames.h>
 #include <ui_interface.h>
 #include <uint256.h>
 #include <util/system.h>
+#include <util/threadnames.h>
 
 #include <memory>
-#include <stdint.h>
-
-#include <boost/thread.hpp>
 
 #include <QApplication>
 #include <QDebug>
@@ -462,8 +459,11 @@ int GuiMain(int argc, char* argv[])
     SetupUIArgs();
     std::string error;
     if (!node->parseParameters(argc, argv, error)) {
+        node->initError(strprintf("Error parsing command line arguments: %s\n", error));
+        // Create a message box, because the gui has neither been created nor has subscribed to core signals
         QMessageBox::critical(nullptr, PACKAGE_NAME,
-            QObject::tr("Error parsing command line arguments: %1.").arg(QString::fromStdString(error)));
+            // message can not be translated because translations have not been initialized
+            QString::fromStdString("Error parsing command line arguments: %1.").arg(QString::fromStdString(error)));
         return EXIT_FAILURE;
     }
 
@@ -499,11 +499,13 @@ int GuiMain(int argc, char* argv[])
     /// - Do not call GetDataDir(true) before this step finishes
     if (!fs::is_directory(GetDataDir(false)))
     {
+        node->initError(strprintf("Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "")));
         QMessageBox::critical(nullptr, PACKAGE_NAME,
             QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", ""))));
         return EXIT_FAILURE;
     }
     if (!node->readConfigFiles(error)) {
+        node->initError(strprintf("Error reading configuration file: %s\n", error));
         QMessageBox::critical(nullptr, PACKAGE_NAME,
             QObject::tr("Error: Cannot parse configuration file: %1.").arg(QString::fromStdString(error)));
         return EXIT_FAILURE;
@@ -519,6 +521,7 @@ int GuiMain(int argc, char* argv[])
     try {
         node->selectParams(gArgs.GetChainName());
     } catch(std::exception &e) {
+        node->initError(strprintf("%s\n", e.what()));
         QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(e.what()));
         return EXIT_FAILURE;
     }

src/qt/bitcoin.h

@@ -11,7 +11,6 @@
 
 #include <QApplication>
 #include <memory>
-#include <vector>
 
 class BitcoinGUI;
 class ClientModel;

src/qt/bitcoingui.cpp

@@ -36,9 +36,6 @@
 #include <ui_interface.h>
 #include <util/system.h>
 
-#include <iostream>
-#include <memory>
-
 #include <QAction>
 #include <QApplication>
 #include <QComboBox>

test/functional/feature_config_args.py

@@ -22,7 +22,7 @@ class ConfArgsTest(BitcoinTestFramework):
             conf.write('includeconf={}\n'.format(inc_conf_file_path))
 
         self.nodes[0].assert_start_raises_init_error(
-            expected_msg='Error parsing command line arguments: Invalid parameter -dash_cli',
+            expected_msg='Error: Error parsing command line arguments: Invalid parameter -dash_cli',
             extra_args=['-dash_cli=1'],
         )
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
@@ -33,7 +33,7 @@ class ConfArgsTest(BitcoinTestFramework):
 
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('-dash=1\n')
-        self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 1: -dash=1, options in configuration file must be specified without leading -')
+        self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 1: -dash=1, options in configuration file must be specified without leading -')
 
         with open(inc_conf_file_path, 'w', encoding='utf8') as conf:
             conf.write("wallet=foo\n")
@@ -46,19 +46,19 @@ class ConfArgsTest(BitcoinTestFramework):
 
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('nono\n')
-        self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 1: nono, if you intended to specify a negated option, use nono=1 instead')
+        self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 1: nono, if you intended to specify a negated option, use nono=1 instead')
 
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('server=1\nrpcuser=someuser\nrpcpassword=some#pass')
-        self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 3, using # in rpcpassword can be ambiguous and should be avoided')
+        self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 3, using # in rpcpassword can be ambiguous and should be avoided')
 
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('server=1\nrpcuser=someuser\nmain.rpcpassword=some#pass')
-        self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 3, using # in rpcpassword can be ambiguous and should be avoided')
+        self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 3, using # in rpcpassword can be ambiguous and should be avoided')
 
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('server=1\nrpcuser=someuser\n[main]\nrpcpassword=some#pass')
-        self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 4, using # in rpcpassword can be ambiguous and should be avoided')
+        self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 4, using # in rpcpassword can be ambiguous and should be avoided')
 
         inc_conf_file2_path = os.path.join(self.nodes[0].datadir, 'include2.conf')
         with open(os.path.join(self.nodes[0].datadir, 'bitcoin.conf'), 'a', encoding='utf-8') as conf:

test/functional/feature_includeconf.py

@@ -43,7 +43,7 @@ class IncludeConfTest(BitcoinTestFramework):
 
         self.log.info("-includeconf cannot be used as command-line arg")
         self.stop_node(0)
-        self.nodes[0].assert_start_raises_init_error(extra_args=["-includeconf=relative2.conf"], expected_msg="Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=relative2.conf")
+        self.nodes[0].assert_start_raises_init_error(extra_args=["-includeconf=relative2.conf"], expected_msg="Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=relative2.conf")
 
         self.log.info("-includeconf cannot be used recursively. subversion should end with 'main; relative)/'")
         with open(os.path.join(self.options.tmpdir, "node0", "relative.conf"), "a", encoding="utf8") as f:
@@ -59,11 +59,11 @@ class IncludeConfTest(BitcoinTestFramework):
         # Commented out as long as we ignore invalid arguments in configuration files
         #with open(os.path.join(self.options.tmpdir, "node0", "relative.conf"), "w", encoding="utf8") as f:
         #    f.write("foo=bar\n")
-        #self.nodes[0].assert_start_raises_init_error(expected_msg="Error reading configuration file: Invalid configuration value foo")
+        #self.nodes[0].assert_start_raises_init_error(expected_msg="Error: Error reading configuration file: Invalid configuration value foo")
 
         self.log.info("-includeconf cannot be invalid path")
         os.remove(os.path.join(self.options.tmpdir, "node0", "relative.conf"))
-        self.nodes[0].assert_start_raises_init_error(expected_msg="Error reading configuration file: Failed to include configuration file relative.conf")
+        self.nodes[0].assert_start_raises_init_error(expected_msg="Error: Error reading configuration file: Failed to include configuration file relative.conf")
 
         self.log.info("multiple -includeconf args can be used from the base config file. subversion should end with 'main; relative; relative2)/'")
         with open(os.path.join(self.options.tmpdir, "node0", "relative.conf"), "w", encoding="utf8") as f: