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>

10 files changed, 146 insertions, 34 deletions

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 53fbac0106..a7003c95ab 100644
--- a/src/qt/bitcoin.cpp
+++ b/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)
diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h
index 7a6aa5cdc9..9ad37ca6c9 100644
--- a/src/qt/bitcoin.h
+++ b/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
diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
index 612d3009c1..6f61afed54 100644
--- a/src/qt/optionsmodel.cpp
+++ b/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) {
-            settings.setValue("nDatabaseCache", value);
+        if (changed()) {
+            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");
 }
diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h
index 92f80ecf21..9f8d17b305 100644
--- a/src/qt/optionsmodel.h
+++ b/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);
diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp
index ededde4da9..bc9dbae44a 100644
--- a/src/qt/test/addressbooktests.cpp
+++ b/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);
diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
index 9648ef6188..6fc7a52435 100644
--- a/src/qt/test/apptests.cpp
+++ b/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());
diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp
index 2ef9230c59..fb8b78f66e 100644
--- a/src/qt/test/optiontests.cpp
+++ b/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};
 
diff --git a/src/qt/test/optiontests.h b/src/qt/test/optiontests.h
index 257a0b65be..286d785572 100644
--- a/src/qt/test/optiontests.h
+++ b/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
diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp
index aeedd92834..de23f51c92 100644
--- a/src/qt/test/test_main.cpp
+++ b/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");
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
index bc06f0f23b..7671bfc739 100644
--- a/src/qt/test/wallettests.cpp
+++ b/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);