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

Author: ryanofsky

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)) {
-            return InitError(Untranslated(strprintf("Specified data directory \"%s\" does not exist.", args.GetArg("-datadir", ""))));
-        }
-        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())));
+        if (auto error = common::InitConfig(args)) {
+            return InitError(error->message, error->details);
         }
 
         // 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();
-    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.setTextFormat(Qt::PlainText);
-        messagebox.setDefaultButton(QMessageBox::Reset);
-        switch (messagebox.exec()) {
-        case QMessageBox::Reset:
-            break;
-        case QMessageBox::Abort:
-            return false;
-        default:
-            assert(false);
-        }
-    }
-
-    errors.clear();
-    if (!gArgs.WriteSettingsFile(&errors)) {
-        std::string error = QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be written");
-        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.setTextFormat(Qt::PlainText);
-        messagebox.setDefaultButton(QMessageBox::Ok);
-        messagebox.exec();
+    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(details)));
+    messagebox.setTextFormat(Qt::PlainText);
+    messagebox.setDefaultButton(QMessageBox::Reset);
+    switch (messagebox.exec()) {
+    case QMessageBox::Reset:
         return false;
+    case QMessageBox::Abort:
+        return true;
+    default:
+        assert(false);
     }
-    return true;
+}
+
+static void ErrorSettingsWrite(const bilingual_str& error, const std::vector<std::string>& details)
+{
+    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(details)));
+    messagebox.setTextFormat(Qt::PlainText);
+    messagebox.setDefaultButton(QMessageBox::Ok);
+    messagebox.exec();
 }
 
 /* qDebug() message handler --> debug.log */
@@ -587,45 +570,30 @@ 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
-    if (!CheckDataDirOption(gArgs)) {
-        InitError(strprintf(Untranslated("Specified data directory \"%s\" does not exist."), 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;
-    }
-    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;
+    /// 6-7. Parse bitcoin.conf, determine network, switch to network specific
+    /// options, and create datadir and settings.json.
+    // - Do not call gArgs.GetDataDirNet() before this step finishes
+    // - Do not call Params() before this step
+    // - 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)) {
+        InitError(error->message, error->details);
+        if (error->status == common::ConfigStatus::FAILED_WRITE) {
+            // Show a custom error message to provide more information in the
+            // case of a datadir write error.
+            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)));
         }
-
-        /// 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
-
-        // Check for chain settings (Params() calls are only valid after this clause)
-        SelectParams(gArgs.GetChainName());
-    } catch(std::exception &e) {
-        InitError(Untranslated(strprintf("%s", e.what())));
-        QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(e.what()));
         return EXIT_FAILURE;
     }
 #ifdef ENABLE_WALLET
     // Parse URIs on command line
     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);