Migrate -dbcache setting from QSettings to settings.json

This is just the first of multiple settings that will be stored in the bitcoin
persistent setting file and shared with bitcoind, instead of being stored as Qt
settings backed by the windows registry or platform specific config files which
are ignored by bitcoind.

Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>

src/qt/bitcoin.cpp

@@ -259,9 +259,26 @@ void BitcoinApplication::createPaymentServer()
 }
 #endif
 
-void BitcoinApplication::createOptionsModel(bool resetSettings)
+bool BitcoinApplication::createOptionsModel(bool resetSettings)
 {
-    optionsModel = new OptionsModel(node(), this, resetSettings);
+    optionsModel = new OptionsModel(node(), this);
+    if (resetSettings) {
+        optionsModel->Reset();
+    }
+    bilingual_str error;
+    if (!optionsModel->Init(error)) {
+        fs::path settings_path;
+        if (gArgs.GetSettingsPath(&settings_path)) {
+            error += Untranslated("\n");
+            std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
+            error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
+            error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
+        }
+        InitError(error);
+        QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated));
+        return false;
+    }
+    return true;
 }
 
 void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
@@ -641,7 +658,9 @@ int GuiMain(int argc, char* argv[])
     app.createNode(*init);
 
     // Load GUI settings from QSettings
-    app.createOptionsModel(gArgs.GetBoolArg("-resetguisettings", false));
+    if (!app.createOptionsModel(gArgs.GetBoolArg("-resetguisettings", false))) {
+        return EXIT_FAILURE;
+    }
 
     if (did_show_intro) {
         // Store intro dialog settings other than datadir (network specific)

src/qt/bitcoin.h

@@ -47,7 +47,7 @@ public:
     /// parameter interaction/setup based on rules
     void parameterSetup();
     /// Create options model
-    void createOptionsModel(bool resetSettings);
+    [[nodiscard]] bool createOptionsModel(bool resetSettings);
     /// Initialize prune setting
     void InitPruneSetting(int64_t prune_MiB);
     /// Create main window

src/qt/optionsmodel.cpp

@@ -26,14 +26,41 @@
 #include <QStringList>
 #include <QVariant>
 
+#include <univalue.h>
+
 const char *DEFAULT_GUI_PROXY_HOST = "127.0.0.1";
 
 static const QString GetDefaultProxyAddress();
 
-OptionsModel::OptionsModel(interfaces::Node& node, QObject *parent, bool resetSettings) :
+/** Map GUI option ID to node setting name. */
+static const char* SettingName(OptionsModel::OptionID option)
+{
+    switch (option) {
+    case OptionsModel::DatabaseCache: return "dbcache";
+    default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
+    }
+}
+
+/** Call node.updateRwSetting() with Bitcoin 22.x workaround. */
+static void UpdateRwSetting(interfaces::Node& node, OptionsModel::OptionID option, const util::SettingsValue& value)
+{
+    if (value.isNum() && option == OptionsModel::DatabaseCache) {
+        // Write certain old settings as strings, even though they are numbers,
+        // because Bitcoin 22.x releases try to read these specific settings as
+        // strings in addOverriddenOption() calls at startup, triggering
+        // uncaught exceptions in UniValue::get_str(). These errors were fixed
+        // in later releases by https://github.com/bitcoin/bitcoin/pull/24498.
+        // If new numeric settings are added, they can be written as numbers
+        // instead of strings, because bitcoin 22.x will not try to read these.
+        node.updateRwSetting(SettingName(option), value.getValStr());
+    } else {
+        node.updateRwSetting(SettingName(option), value);
+    }
+}
+
+OptionsModel::OptionsModel(interfaces::Node& node, QObject *parent) :
     QAbstractListModel(parent), m_node{node}
 {
-    Init(resetSettings);
 }
 
 void OptionsModel::addOverriddenOption(const std::string &option)
@@ -42,11 +69,8 @@ void OptionsModel::addOverriddenOption(const std::string &option)
 }
 
 // Writes all missing QSettings with their default values
-void OptionsModel::Init(bool resetSettings)
+bool OptionsModel::Init(bilingual_str& error)
 {
-    if (resetSettings)
-        Reset();
-
     checkAndMigrate();
 
     QSettings settings;
@@ -98,7 +122,20 @@ void OptionsModel::Init(bool resetSettings)
 
     // These are shared with the core or have a command-line parameter
     // and we want command-line parameters to overwrite the GUI settings.
-    //
+    for (OptionID option : {DatabaseCache}) {
+        std::string setting = SettingName(option);
+        if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting);
+        try {
+            getOption(option);
+        } catch (const std::exception& e) {
+            // This handles exceptions thrown by univalue that can happen if
+            // settings in settings.json don't have the expected types.
+            error.original = strprintf("Could not read setting \"%s\", %s.", setting, e.what());
+            error.translated = tr("Could not read setting \"%1\", %2.").arg(QString::fromStdString(setting), e.what()).toStdString();
+            return false;
+        }
+    }
+
     // If setting doesn't exist create it with defaults.
     //
     // If gArgs.SoftSetArg() or gArgs.SoftSetBoolArg() return false we were overridden
@@ -111,11 +148,6 @@ void OptionsModel::Init(bool resetSettings)
         settings.setValue("nPruneSize", DEFAULT_PRUNE_TARGET_GB);
     SetPruneEnabled(settings.value("bPrune").toBool());
 
-    if (!settings.contains("nDatabaseCache"))
-        settings.setValue("nDatabaseCache", (qint64)nDefaultDbCache);
-    if (!gArgs.SoftSetArg("-dbcache", settings.value("nDatabaseCache").toString().toStdString()))
-        addOverriddenOption("-dbcache");
-
     if (!settings.contains("nThreadsScriptVerif"))
         settings.setValue("nThreadsScriptVerif", DEFAULT_SCRIPTCHECK_THREADS);
     if (!gArgs.SoftSetArg("-par", settings.value("nThreadsScriptVerif").toString().toStdString()))
@@ -222,6 +254,8 @@ void OptionsModel::Init(bool resetSettings)
     }
     m_use_embedded_monospaced_font = settings.value("UseEmbeddedMonospacedFont").toBool();
     Q_EMIT useEmbeddedMonospacedFontChanged(m_use_embedded_monospaced_font);
+
+    return true;
 }
 
 /** Helper function to copy contents from one QSettings to another.
@@ -356,6 +390,8 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
 
 QVariant OptionsModel::getOption(OptionID option) const
 {
+    auto setting = [&]{ return node().getPersistentSetting(SettingName(option)); };
+
     QSettings settings;
     switch (option) {
     case StartAtStartup:
@@ -420,7 +456,7 @@ QVariant OptionsModel::getOption(OptionID option) const
     case PruneSize:
         return settings.value("nPruneSize");
     case DatabaseCache:
-        return settings.value("nDatabaseCache");
+        return qlonglong(SettingToInt(setting(), nDefaultDbCache));
     case ThreadsScriptVerif:
         return settings.value("nThreadsScriptVerif");
     case Listen:
@@ -434,6 +470,9 @@ QVariant OptionsModel::getOption(OptionID option) const
 
 bool OptionsModel::setOption(OptionID option, const QVariant& value)
 {
+    auto changed = [&] { return value.isValid() && value != getOption(option); };
+    auto update = [&](const util::SettingsValue& value) { return UpdateRwSetting(node(), option, value); };
+
     bool successful = true; /* set to false on parse error */
     QSettings settings;
 
@@ -574,8 +613,8 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
         }
         break;
     case DatabaseCache:
-        if (settings.value("nDatabaseCache") != value) {
+        if (changed()) {
-            settings.setValue("nDatabaseCache", value);
+            update(static_cast<int64_t>(value.toLongLong()));
             setRestartRequired(true);
         }
         break;
@@ -654,4 +693,16 @@ void OptionsModel::checkAndMigrate()
     if (settings.contains("addrSeparateProxyTor") && settings.value("addrSeparateProxyTor").toString().endsWith("%2")) {
         settings.setValue("addrSeparateProxyTor", GetDefaultProxyAddress());
     }
+
+    // Migrate and delete legacy GUI settings that have now moved to <datadir>/settings.json.
+    auto migrate_setting = [&](OptionID option, const QString& qt_name) {
+        if (!settings.contains(qt_name)) return;
+        QVariant value = settings.value(qt_name);
+        if (node().getPersistentSetting(SettingName(option)).isNull()) {
+            setOption(option, value);
+        }
+        settings.remove(qt_name);
+    };
+
+    migrate_setting(DatabaseCache, "nDatabaseCache");
 }

src/qt/optionsmodel.h

@@ -13,6 +13,7 @@
 
 #include <assert.h>
 
+struct bilingual_str;
 namespace interfaces {
 class Node;
 }
@@ -41,7 +42,7 @@ class OptionsModel : public QAbstractListModel
     Q_OBJECT
 
 public:
-    explicit OptionsModel(interfaces::Node& node, QObject *parent = nullptr, bool resetSettings = false);
+    explicit OptionsModel(interfaces::Node& node, QObject *parent = nullptr);
 
     enum OptionID {
         StartAtStartup,         // bool
@@ -74,7 +75,7 @@ public:
         OptionIDRowCount,
     };
 
-    void Init(bool resetSettings = false);
+    bool Init(bilingual_str& error);
     void Reset();
 
     int rowCount(const QModelIndex & parent = QModelIndex()) const override;
@@ -120,6 +121,7 @@ private:
     bool fCoinControlFeatures;
     bool m_sub_fee_from_amount;
     bool m_enable_psbt_controls;
+
     /* settings that were overridden by command-line */
     QString strOverriddenByCommandLine;
 
@@ -128,6 +130,7 @@ private:
 
     // Check settings version and upgrade default values if required
     void checkAndMigrate();
+
 Q_SIGNALS:
     void displayUnitChanged(BitcoinUnit unit);
     void coinControlFeaturesChanged(bool);

src/qt/test/addressbooktests.cpp

@@ -128,6 +128,8 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
     // Initialize relevant QT models.
     std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));
     OptionsModel optionsModel(node);
+    bilingual_str error;
+    QVERIFY(optionsModel.Init(error));
     ClientModel clientModel(node, &optionsModel);
     WalletContext& context = *node.walletLoader().context();
     AddWallet(context, wallet);

src/qt/test/apptests.cpp

@@ -70,14 +70,9 @@ void AppTests::appTests()
     }
 #endif
 
-    fs::create_directories([] {
-        BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
-        return gArgs.GetDataDirNet() / "blocks";
-    }());
-
     qRegisterMetaType<interfaces::BlockAndHeaderTipInfo>("interfaces::BlockAndHeaderTipInfo");
     m_app.parameterSetup();
-    m_app.createOptionsModel(true /* reset settings */);
+    QVERIFY(m_app.createOptionsModel(true /* reset settings */));
     QScopedPointer<const NetworkStyle> style(NetworkStyle::instantiate(Params().NetworkIDString()));
     m_app.setupPlatformStyle();
     m_app.createWindow(style.data());

src/qt/test/optiontests.cpp

@@ -13,6 +13,40 @@
 
 #include <univalue.h>
 
+#include <fstream>
+
+OptionTests::OptionTests(interfaces::Node& node) : m_node(node)
+{
+    gArgs.LockSettings([&](util::Settings& s) { m_previous_settings = s; });
+}
+
+void OptionTests::init()
+{
+    // reset args
+    gArgs.LockSettings([&](util::Settings& s) { s = m_previous_settings; });
+    gArgs.ClearPathCache();
+}
+
+void OptionTests::migrateSettings()
+{
+    // Set legacy QSettings and verify that they get cleared and migrated to
+    // settings.json
+    QSettings settings;
+    settings.setValue("nDatabaseCache", 600);
+
+    settings.sync();
+
+    OptionsModel options{m_node};
+    bilingual_str error;
+    QVERIFY(options.Init(error));
+    QVERIFY(!settings.contains("nDatabaseCache"));
+
+    std::ifstream file(gArgs.GetDataDirNet() / "settings.json");
+    QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n"
+        "    \"dbcache\": \"600\"\n"
+        "}\n");
+}
+
 void OptionTests::integerGetArgBug()
 {
     // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Ensure
@@ -23,7 +57,8 @@ void OptionTests::integerGetArgBug()
         settings.rw_settings["prune"] = 3814;
     });
     gArgs.WriteSettingsFile();
-    OptionsModel{m_node};
+    bilingual_str error;
+    QVERIFY(OptionsModel{m_node}.Init(error));
     gArgs.LockSettings([&](util::Settings& settings) {
         settings.rw_settings.erase("prune");
     });
@@ -36,8 +71,6 @@ void OptionTests::parametersInteraction()
     // It was fixed via https://github.com/bitcoin-core/gui/pull/568.
     // With fListen=false in ~/.config/Bitcoin/Bitcoin-Qt.conf and all else left as default,
     // bitcoin-qt should set both -listen and -listenonion to false and start successfully.
-    gArgs.ClearPathCache();
-
     gArgs.LockSettings([&](util::Settings& s) {
         s.forced_settings.erase("listen");
         s.forced_settings.erase("listenonion");
@@ -48,7 +81,8 @@ void OptionTests::parametersInteraction()
     QSettings settings;
     settings.setValue("fListen", false);
 
-    OptionsModel{m_node};
+    bilingual_str error;
+    QVERIFY(OptionsModel{m_node}.Init(error));
 
     const bool expected{false};
 

src/qt/test/optiontests.h

@@ -6,6 +6,8 @@
 #define BITCOIN_QT_TEST_OPTIONTESTS_H
 
 #include <qt/optionsmodel.h>
+#include <univalue.h>
+#include <util/settings.h>
 
 #include <QObject>
 
@@ -13,14 +15,17 @@ class OptionTests : public QObject
 {
     Q_OBJECT
 public:
-    explicit OptionTests(interfaces::Node& node) : m_node(node) {}
+    explicit OptionTests(interfaces::Node& node);
 
 private Q_SLOTS:
+    void init(); // called before each test function execution.
+    void migrateSettings();
     void integerGetArgBug();
     void parametersInteraction();
 
 private:
     interfaces::Node& m_node;
+    util::Settings m_previous_settings;
 };
 
 #endif // BITCOIN_QT_TEST_OPTIONTESTS_H

src/qt/test/test_main.cpp

@@ -58,9 +58,10 @@ int main(int argc, char* argv[])
     // regtest params.
     //
     // All tests must use their own testing setup (if needed).
-    {
+    fs::create_directories([] {
         BasicTestingSetup dummy{CBaseChainParams::REGTEST};
-    }
+        return gArgs.GetDataDirNet() / "blocks";
+    }());
 
     std::unique_ptr<interfaces::Init> init = interfaces::MakeGuiInit(argc, argv);
     gArgs.ForceSetArg("-listen", "0");

src/qt/test/wallettests.cpp

@@ -185,6 +185,8 @@ void TestGUI(interfaces::Node& node)
     SendCoinsDialog sendCoinsDialog(platformStyle.get());
     TransactionView transactionView(platformStyle.get());
     OptionsModel optionsModel(node);
+    bilingual_str error;
+    QVERIFY(optionsModel.Init(error));
     ClientModel clientModel(node, &optionsModel);
     WalletContext& context = *node.walletLoader().context();
     AddWallet(context, wallet);