Deduplicate bitcoind and bitcoin-qt init code

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

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

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/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>
@@ -150,17 +151,8 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
     std::any context{&node};
     try
     {
-        if (!CheckDataDirOption(args)) {
+        if (auto error = common::InitConfig(args)) {
-            return InitError(Untranslated(strprintf("Specified data directory \"%s\" does not exist.", args.GetArg("-datadir", ""))));
+            return InitError(error->message, error->details);
-        }
-        if (!args.ReadConfigFiles(error, true)) {
-            return InitError(Untranslated(strprintf("Error reading configuration file: %s", 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", e.what())));
         }
 
         // Error out when loose non-argument tokens are encountered on command line
@@ -170,11 +162,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
             }
         }
 
-        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

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/qt/bitcoin.cpp

@@ -9,6 +9,7 @@
 #include <qt/bitcoin.h>
 
 #include <chainparams.h>
+#include <common/init.h>
 #include <init.h>
 #include <interfaces/handler.h>
 #include <interfaces/init.h>
@@ -165,54 +166,36 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans
     }
 }
 
-static bool InitSettings()
+static bool ErrorSettingsRead(const bilingual_str& error, const std::vector<std::string>& details)
 {
-    gArgs.EnsureDataDir();
+    QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort);
-    if (!gArgs.GetSettingsPath()) {
-        return true; // Do nothing if settings file disabled.
-    }
-
-    std::vector<std::string> errors;
-    if (!gArgs.ReadSettingsFile(&errors)) {
-        std::string error = QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be read");
-        std::string error_translated = QCoreApplication::translate("bitcoin-core", error.c_str()).toStdString();
-        InitError(Untranslated(strprintf("%s:\n%s", error, MakeUnorderedList(errors))));
-
-        QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error_translated)), QMessageBox::Reset | QMessageBox::Abort);
     /*: Explanatory text shown on startup when the settings file cannot be read.
       Prompts user to make a choice between resetting or aborting. */
     messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?"));
-        messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors)));
+    messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(details)));
     messagebox.setTextFormat(Qt::PlainText);
     messagebox.setDefaultButton(QMessageBox::Reset);
     switch (messagebox.exec()) {
     case QMessageBox::Reset:
-            break;
-        case QMessageBox::Abort:
         return false;
+    case QMessageBox::Abort:
+        return true;
     default:
         assert(false);
     }
 }
 
-    errors.clear();
+static void ErrorSettingsWrite(const bilingual_str& error, const std::vector<std::string>& details)
-    if (!gArgs.WriteSettingsFile(&errors)) {
+{
-        std::string error = QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be written");
+    QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok);
-        std::string error_translated = QCoreApplication::translate("bitcoin-core", error.c_str()).toStdString();
-        InitError(Untranslated(strprintf("%s:\n%s", error, MakeUnorderedList(errors))));
-
-        QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error_translated)), QMessageBox::Ok);
     /*: Explanatory text shown on startup when the settings file could not be written.
         Prompts user to check that we have the ability to write to the file.
         Explains that the user has the option of running without a settings file.*/
     messagebox.setInformativeText(QObject::tr("A fatal error occurred. Check that settings file is writable, or try running with -nosettings."));
-        messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors)));
+    messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(details)));
     messagebox.setTextFormat(Qt::PlainText);
     messagebox.setDefaultButton(QMessageBox::Ok);
     messagebox.exec();
-        return false;
-    }
-    return true;
 }
 
 /* qDebug() message handler --> debug.log */
@@ -587,34 +570,23 @@ int GuiMain(int argc, char* argv[])
     // Gracefully exit if the user cancels
     if (!Intro::showIfNeeded(did_show_intro, prune_MiB)) return EXIT_SUCCESS;
 
-    /// 6a. Determine availability of data directory
+    /// 6-7. Parse bitcoin.conf, determine network, switch to network specific
-    if (!CheckDataDirOption(gArgs)) {
+    /// options, and create datadir and settings.json.
-        InitError(strprintf(Untranslated("Specified data directory \"%s\" does not exist."), gArgs.GetArg("-datadir", "")));
+    // - Do not call gArgs.GetDataDirNet() before this step finishes
-        QMessageBox::critical(nullptr, PACKAGE_NAME,
-            QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", ""))));
-        return EXIT_FAILURE;
-    }
-    try {
-        /// 6b. Parse bitcoin.conf
-        /// - Do not call gArgs.GetDataDirNet() before this step finishes
-        if (!gArgs.ReadConfigFiles(error, true)) {
-            InitError(strprintf(Untranslated("Error reading configuration file: %s"), error));
-            QMessageBox::critical(nullptr, PACKAGE_NAME,
-                QObject::tr("Error: Cannot parse configuration file: %1.").arg(QString::fromStdString(error)));
-            return EXIT_FAILURE;
-        }
-
-        /// 7. Determine network (and switch to network specific options)
     // - Do not call Params() before this step
-        // - Do this after parsing the configuration file, as the network can be switched there
     // - QSettings() will use the new application name after this, resulting in network-specific settings
     // - Needs to be done before createOptionsModel
-
+    if (auto error = common::InitConfig(gArgs, ErrorSettingsRead)) {
-        // Check for chain settings (Params() calls are only valid after this clause)
+        InitError(error->message, error->details);
-        SelectParams(gArgs.GetChainName());
+        if (error->status == common::ConfigStatus::FAILED_WRITE) {
-    } catch(std::exception &e) {
+            // Show a custom error message to provide more information in the
-        InitError(Untranslated(strprintf("%s", e.what())));
+            // case of a datadir write error.
-        QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(e.what()));
+            ErrorSettingsWrite(error->message, error->details);
+        } else if (error->status != common::ConfigStatus::ABORTED) {
+            // Show a generic message in other cases, and no additional error
+            // message in the case of a read error if the user decided to abort.
+            QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(QString::fromStdString(error->message.translated)));
+        }
         return EXIT_FAILURE;
     }
 #ifdef ENABLE_WALLET
@@ -622,10 +594,6 @@ int GuiMain(int argc, char* argv[])
     PaymentServer::ipcParseCommandLine(argc, argv);
 #endif
 
-    if (!InitSettings()) {
-        return EXIT_FAILURE;
-    }
-
     QScopedPointer<const NetworkStyle> networkStyle(NetworkStyle::instantiate(Params().NetworkIDString()));
     assert(!networkStyle.isNull());
     // Allow for separate UI settings for testnets

src/util/system.cpp

@@ -438,27 +438,6 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
     return path;
 }
 
-void ArgsManager::EnsureDataDir() const
-{
-    /**
-     * "/wallets" subdirectories are created in all **new**
-     * datadirs, because wallet code will create new wallets in the "wallets"
-     * subdirectory only if exists already, otherwise it will create them in
-     * the top-level datadir where they could interfere with other files.
-     * Wallet init code currently avoids creating "wallets" directories itself
-     * for backwards compatibility, but this be changed in the future and
-     * wallet code here could go away (#16220).
-     */
-    auto path{GetDataDir(false)};
-    if (!fs::exists(path)) {
-        fs::create_directories(path / "wallets");
-    }
-    path = GetDataDir(true);
-    if (!fs::exists(path)) {
-        fs::create_directories(path / "wallets");
-    }
-}
-
 void ArgsManager::ClearPathCache()
 {
     LOCK(cs_args);
@@ -502,25 +481,6 @@ bool ArgsManager::IsArgSet(const std::string& strArg) const
     return !GetSetting(strArg).isNull();
 }
 
-bool ArgsManager::InitSettings(std::string& error)
-{
-    EnsureDataDir();
-    if (!GetSettingsPath()) {
-        return true; // Do nothing if settings file disabled.
-    }
-
-    std::vector<std::string> errors;
-    if (!ReadSettingsFile(&errors)) {
-        error = strprintf("Failed loading settings file:\n%s\n", MakeUnorderedList(errors));
-        return false;
-    }
-    if (!WriteSettingsFile(&errors)) {
-        error = strprintf("Failed saving settings file:\n%s\n", MakeUnorderedList(errors));
-        return false;
-    }
-    return true;
-}
-
 bool ArgsManager::GetSettingsPath(fs::path* filepath, bool temp, bool backup) const
 {
     fs::path settings = GetPathArg("-settings", BITCOIN_SETTINGS_FILENAME);

src/util/system.h

@@ -434,13 +434,6 @@ protected:
      */
     std::optional<unsigned int> GetArgFlags(const std::string& name) const;
 
-    /**
-     * Read and update settings file with saved settings. This needs to be
-     * called after SelectParams() because the settings file location is
-     * network-specific.
-     */
-    bool InitSettings(std::string& error);
-
     /**
      * Get settings file path, or return false if read-write settings were
      * disabled with -nosettings.
@@ -480,12 +473,6 @@ protected:
      */
     void LogArgs() const;
 
-    /**
-     * If datadir does not exist, create it along with wallets/
-     * subdirectory(s).
-     */
-    void EnsureDataDir() const;
-
 private:
     /**
      * Get data directory path