Merge bitcoin/bitcoin#27150: Deduplicate bitcoind and bitcoin-qt init code

802cc1e Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c Add InitError(error, details) overload (Ryan Ofsky)
3db2874 Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df9 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)

Pull request description:

  Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.

  Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073

  There are a few minor changes in behavior:

  - In bitcoin-qt, when there is a problem reading the configuration file, the GUI error text has changed from "Error: Cannot parse configuration file:" to "Error reading configuration file:" to be consistent with bitcoind.
  - In bitcoind, when there is a problem reading the settings.json file, the error text has changed from "Failed loading settings file" to "Settings file could not be read" to be consistent with bitcoin-qt.
  - In bitcoind, when there is a problem writing the settings.json file, the error text has changed from "Failed saving settings file" to "Settings file could not be written" to be consistent with bitcoin-qt.
  - In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read), there is an normal error dialog showing "Error: filesystem error: status: Permission denied [.../settings.json]", instead of an uncaught exception.

ACKs for top commit:
  Sjors:
    Light review ACK 802cc1e
  TheCharlatan:
    ACK 802cc1e
  achow101:
    ACK 802cc1e

Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981

Author: achow101

ci/test/06_script_b.sh

@@ -42,6 +42,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
   ( CI_EXEC run-clang-tidy -quiet "${MAKEJOBS}" ) | grep -C5 "error"
   export P_CI_DIR="${BASE_BUILD_DIR}/bitcoin-$HOST/"
   CI_EXEC "python3 ${DIR_IWYU}/include-what-you-use/iwyu_tool.py"\
+          " src/common/init.cpp"\
           " src/common/url.cpp"\
           " src/compat"\
           " src/dbwrapper.cpp"\

src/Makefile.am

@@ -134,6 +134,7 @@ BITCOIN_CORE_H = \
   clientversion.h \
   coins.h \
   common/bloom.h \
+  common/init.h \
   common/run_command.h \
   common/url.h \
   compat/assumptions.h \
@@ -640,6 +641,7 @@ libbitcoin_common_a_SOURCES = \
   chainparams.cpp \
   coins.cpp \
   common/bloom.cpp \
+  common/init.cpp \
   common/interfaces.cpp \
   common/run_command.cpp \
   compressor.cpp \

src/Makefile.test.include

@@ -147,6 +147,7 @@ BITCOIN_TESTS =\
   test/timedata_tests.cpp \
   test/torcontrol_tests.cpp \
   test/transaction_tests.cpp \
+  test/translation_tests.cpp \
   test/txindex_tests.cpp \
   test/txpackage_tests.cpp \
   test/txreconciliation_tests.cpp \

src/bitcoind.cpp

@@ -9,6 +9,7 @@
 
 #include <chainparams.h>
 #include <clientversion.h>
+#include <common/init.h>
 #include <common/url.h>
 #include <compat/compat.h>
 #include <init.h>
@@ -120,7 +121,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
     SetupServerArgs(args);
     std::string error;
     if (!args.ParseParameters(argc, argv, error)) {
-        return InitError(Untranslated(strprintf("Error parsing command line arguments: %s\n", error)));
+        return InitError(Untranslated(strprintf("Error parsing command line arguments: %s", error)));
     }
 
     // Process help and version before taking care about datadir
@@ -150,31 +151,17 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
     std::any context{&node};
     try
     {
-        if (!CheckDataDirOption(args)) {
-            return InitError(Untranslated(strprintf("Specified data directory \"%s\" does not exist.\n", args.GetArg("-datadir", ""))));
-        }
-        if (!args.ReadConfigFiles(error, true)) {
-            return InitError(Untranslated(strprintf("Error reading configuration file: %s\n", error)));
-        }
-        // Check for chain settings (Params() calls are only valid after this clause)
-        try {
-            SelectParams(args.GetChainName());
-        } catch (const std::exception& e) {
-            return InitError(Untranslated(strprintf("%s\n", e.what())));
+        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.\n", argv[i])));
+                return InitError(Untranslated(strprintf("Command line contains unexpected token '%s', see bitcoind -h for a list of options.", argv[i])));
             }
         }
 
-        if (!args.InitSettings(error)) {
-            InitError(Untranslated(error));
-            return false;
-        }
-
         // -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
@@ -210,7 +197,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
                 }
                 break;
             case -1: // Error happened.
-                return InitError(Untranslated(strprintf("fork_daemon() failed: %s\n", SysErrorString(errno))));
+                return InitError(Untranslated(strprintf("fork_daemon() failed: %s", SysErrorString(errno))));
             default: { // Parent: wait and exit.
                 int token = daemon_ep.TokenRead();
                 if (token) { // Success
@@ -222,7 +209,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
             }
             }
 #else
-            return InitError(Untranslated("-daemon is not supported on this operating system\n"));
+            return InitError(Untranslated("-daemon is not supported on this operating system"));
 #endif // HAVE_DECL_FORK
         }
         // Lock data directory after daemonization

src/common/init.cpp

@@ -0,0 +1,74 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <common/init.h>
+#include <chainparams.h>
+#include <fs.h>
+#include <tinyformat.h>
+#include <util/system.h>
+#include <util/translation.h>
+
+#include <algorithm>
+#include <exception>
+#include <optional>
+
+namespace common {
+std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn settings_abort_fn)
+{
+    try {
+        if (!CheckDataDirOption(args)) {
+            return ConfigError{ConfigStatus::FAILED, strprintf(_("Specified data directory \"%s\" does not exist."), args.GetArg("-datadir", ""))};
+        }
+        std::string error;
+        if (!args.ReadConfigFiles(error, true)) {
+            return ConfigError{ConfigStatus::FAILED, strprintf(_("Error reading configuration file: %s"), error)};
+        }
+
+        // Check for chain settings (Params() calls are only valid after this clause)
+        SelectParams(args.GetChainName());
+
+        // Create datadir if it does not exist.
+        const auto base_path{args.GetDataDirBase()};
+        if (!fs::exists(base_path)) {
+            // When creating a *new* datadir, also create a "wallets" subdirectory,
+            // whether or not the wallet is enabled now, so if the wallet is enabled
+            // in the future, it will use the "wallets" subdirectory for creating
+            // and listing wallets, rather than the top-level directory where
+            // wallets could be mixed up with other files. For backwards
+            // compatibility, wallet code will use the "wallets" subdirectory only
+            // if it already exists, but never create it itself. There is discussion
+            // in https://github.com/bitcoin/bitcoin/issues/16220 about ways to
+            // change wallet code so it would no longer be necessary to create
+            // "wallets" subdirectories here.
+            fs::create_directories(base_path / "wallets");
+        }
+        const auto net_path{args.GetDataDirNet()};
+        if (!fs::exists(net_path)) {
+            fs::create_directories(net_path / "wallets");
+        }
+
+        // Create settings.json if -nosettings was not specified.
+        if (args.GetSettingsPath()) {
+            std::vector<std::string> details;
+            if (!args.ReadSettingsFile(&details)) {
+                const bilingual_str& message = _("Settings file could not be read");
+                if (!settings_abort_fn) {
+                    return ConfigError{ConfigStatus::FAILED, message, details};
+                } else if (settings_abort_fn(message, details)) {
+                    return ConfigError{ConfigStatus::ABORTED, message, details};
+                } else {
+                    details.clear(); // User chose to ignore the error and proceed.
+                }
+            }
+            if (!args.WriteSettingsFile(&details)) {
+                const bilingual_str& message = _("Settings file could not be written");
+                return ConfigError{ConfigStatus::FAILED_WRITE, message, details};
+            }
+        }
+    } catch (const std::exception& e) {
+        return ConfigError{ConfigStatus::FAILED, Untranslated(e.what())};
+    }
+    return {};
+}
+} // namespace common

src/common/init.h

@@ -0,0 +1,39 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_COMMON_INIT_H
+#define BITCOIN_COMMON_INIT_H
+
+#include <util/translation.h>
+
+#include <functional>
+#include <optional>
+#include <string>
+#include <vector>
+
+class ArgsManager;
+
+namespace common {
+enum class ConfigStatus {
+    FAILED,       //!< Failed generically.
+    FAILED_WRITE, //!< Failed to write settings.json
+    ABORTED,      //!< Aborted by user
+};
+
+struct ConfigError {
+    ConfigStatus status;
+    bilingual_str message{};
+    std::vector<std::string> details{};
+};
+
+//! Callback function to let the user decide whether to abort loading if
+//! settings.json file exists and can't be parsed, or to ignore the error and
+//! overwrite the file.
+using SettingsAbortFn = std::function<bool(const bilingual_str& message, const std::vector<std::string>& details)>;
+
+/* Read config files, and create datadir and settings.json if they don't exist. */
+std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn settings_abort_fn = nullptr);
+} // namespace common
+
+#endif // BITCOIN_COMMON_INIT_H

src/index/base.cpp

@@ -35,7 +35,7 @@ static void FatalError(const char* fmt, const Args&... args)
     std::string strMessage = tfm::format(fmt, args...);
     SetMiscWarning(Untranslated(strMessage));
     LogPrintf("*** %s\n", strMessage);
-    AbortError(_("A fatal internal error occurred, see debug.log for details"));
+    InitError(_("A fatal internal error occurred, see debug.log for details"));
     StartShutdown();
 }
 

src/node/interface_ui.cpp

@@ -4,6 +4,7 @@
 
 #include <node/interface_ui.h>
 
+#include <util/string.h>
 #include <util/translation.h>
 
 #include <boost/signals2/optional_last_value.hpp>
@@ -62,6 +63,18 @@ bool InitError(const bilingual_str& str)
     return false;
 }
 
+bool InitError(const bilingual_str& str, const std::vector<std::string>& details)
+{
+    // For now just flatten the list of error details into a string to pass to
+    // the base InitError overload. In the future, if more init code provides
+    // error details, the details could be passed separately from the main
+    // message for rich display in the GUI. But currently the only init
+    // functions which provide error details are ones that run during early init
+    // before the GUI uiInterface is registered, so there's no point passing
+    // main messages and details separately to uiInterface yet.
+    return InitError(details.empty() ? str : strprintf(Untranslated("%s:\n%s"), str, MakeUnorderedList(details)));
+}
+
 void InitWarning(const bilingual_str& str)
 {
     uiInterface.ThreadSafeMessageBox(str, "", CClientUIInterface::MSG_WARNING);

src/node/interface_ui.h

@@ -116,7 +116,7 @@ void InitWarning(const bilingual_str& str);
 
 /** Show error message **/
 bool InitError(const bilingual_str& str);
-constexpr auto AbortError = InitError;
+bool InitError(const bilingual_str& str, const std::vector<std::string>& details);
 
 extern CClientUIInterface uiInterface;