Merge bitcoin-core/gui#602: Unify bitcoin-qt and bitcoind persistent settings

e47c6c76561807d30cff3c2e5372ea83c91a3677 Reset settings.json when GUI options are reset (Ryan Ofsky)
99ccc02b652cf67a3aec66371fcb6bbe737571a7 Add release notes about unified bitcoin-qt and bitcoind persistent settings (Ryan Ofsky)
504b06b1dec9d9329c83b13c7c36ca710ebcd349 Migrate -lang setting from QSettings to settings.json (Ryan Ofsky)
9a016a3c07d4becf0651ef58c7160180c5f25a0c Migrate -prune setting from QSettings to settings.json (Ryan Ofsky)
f067e1943361b7bfa78a423528759e9edffa7482 Migrate -proxy and -onion settings from QSettings to settings.json (Ryan Ofsky)
a09e3b7cf29c3b1fd320badbed32275e0aa83cda Migrate -listen and -server settings from QSettings to settings.json (Ryan Ofsky)
d2ada6e63583cad91e92b49c4dbf8c7ff086a758 Migrate -upnp and -natpmp settings from QSettings to settings.json (Ryan Ofsky)
1dc4fc29c1086420b7dc51b20c0b7a18fecb4462 Migrate -spendzeroconfchange and -signer settings from QSettings to settings.json (Ryan Ofsky)
a7ef6d5975a5f40b90b2709b32a00647bd2bd5a3 Migrate -par setting from QSettings to settings.json (Ryan Ofsky)
284f339de68905131331f7fdb4c0b945c9a1b8cd Migrate -dbcache setting from QSettings to settings.json (Ryan Ofsky)

Pull request description:

  If a setting like pruning, port mapping, or a network proxy is enabled in the GUI, it will now be stored in the bitcoin persistent setting file in the datadir and shared with bitcoind, instead of being stored as Qt settings which end up in the the windows registry or platform specific config files and are ignored by bitcoind.

  This PR has been split off from bitcoin/bitcoin#15936 so some review of these commits previously took place in that PR.

ACKs for top commit:
  furszy:
    Code review ACK e47c6c76
  hebasto:
    ACK e47c6c76561807d30cff3c2e5372ea83c91a3677

Tree-SHA512: 076ea7c7efe67805b4a357113bfe1643dce364d0032774106de59566a0ed5771d57a5923920085e03d686beb34b98114bd278555dfdf8bb7af0b778b0f35b7d2

12 files changed, 392 insertions, 245 deletions

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 53fbac0106..bd6a1f5eca 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)
@@ -327,7 +344,7 @@ void BitcoinApplication::parameterSetup()
 
 void BitcoinApplication::InitPruneSetting(int64_t prune_MiB)
 {
-    optionsModel->SetPruneTargetGB(PruneMiBtoGB(prune_MiB), true);
+    optionsModel->SetPruneTargetGB(PruneMiBtoGB(prune_MiB));
 }
 
 void BitcoinApplication::requestInitialize()
@@ -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/forms/optionsdialog.ui b/src/qt/forms/optionsdialog.ui
index 5438811aff..582f02132a 100644
--- a/src/qt/forms/optionsdialog.ui
+++ b/src/qt/forms/optionsdialog.ui
@@ -896,7 +896,7 @@
         <item>
          <widget class="QLabel" name="overriddenByCommandLineInfoLabel">
           <property name="text">
-           <string>Options set in this dialog are overridden by the command line or in the configuration file:</string>
+           <string>Options set in this dialog are overridden by the command line:</string>
           </property>
           <property name="textFormat">
            <enum>Qt::PlainText</enum>
diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index e6ff43a379..f3c3af10e0 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsdialog.cpp
@@ -26,7 +26,6 @@
 #include <QIntValidator>
 #include <QLocale>
 #include <QMessageBox>
-#include <QSettings>
 #include <QSystemTrayIcon>
 #include <QTimer>
 
@@ -56,10 +55,6 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
 #ifndef USE_NATPMP
     ui->mapPortNatpmp->setEnabled(false);
 #endif
-    connect(this, &QDialog::accepted, [this](){
-        QSettings settings;
-        model->node().mapPort(settings.value("fUseUPnP").toBool(), settings.value("fUseNatpmp").toBool());
-    });
 
     ui->proxyIp->setEnabled(false);
     ui->proxyPort->setEnabled(false);
diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
index 612d3009c1..0b4359a917 100644
--- a/src/qt/optionsmodel.cpp
+++ b/src/qt/optionsmodel.cpp
@@ -19,6 +19,7 @@
 #include <txdb.h>       // for -dbcache defaults
 #include <util/string.h>
 #include <validation.h> // For DEFAULT_SCRIPTCHECK_THREADS
+#include <wallet/wallet.h> // For DEFAULT_SPEND_ZEROCONF_CHANGE
 
 #include <QDebug>
 #include <QLatin1Char>
@@ -26,14 +27,99 @@
 #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";
+    case OptionsModel::ThreadsScriptVerif: return "par";
+    case OptionsModel::SpendZeroConfChange: return "spendzeroconfchange";
+    case OptionsModel::ExternalSignerPath: return "signer";
+    case OptionsModel::MapPortUPnP: return "upnp";
+    case OptionsModel::MapPortNatpmp: return "natpmp";
+    case OptionsModel::Listen: return "listen";
+    case OptionsModel::Server: return "server";
+    case OptionsModel::PruneSize: return "prune";
+    case OptionsModel::Prune: return "prune";
+    case OptionsModel::ProxyIP: return "proxy";
+    case OptionsModel::ProxyPort: return "proxy";
+    case OptionsModel::ProxyUse: return "proxy";
+    case OptionsModel::ProxyIPTor: return "onion";
+    case OptionsModel::ProxyPortTor: return "onion";
+    case OptionsModel::ProxyUseTor: return "onion";
+    case OptionsModel::Language: return "lang";
+    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 ||
+         option == OptionsModel::ThreadsScriptVerif ||
+         option == OptionsModel::Prune ||
+         option == OptionsModel::PruneSize)) {
+        // 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);
+    }
+}
+
+//! Convert enabled/size values to bitcoin -prune setting.
+static util::SettingsValue PruneSetting(bool prune_enabled, int prune_size_gb)
+{
+    assert(!prune_enabled || prune_size_gb >= 1); // PruneSizeGB and ParsePruneSizeGB never return less
+    return prune_enabled ? PruneGBtoMiB(prune_size_gb) : 0;
+}
+
+//! Get pruning enabled value to show in GUI from bitcoin -prune setting.
+static bool PruneEnabled(const util::SettingsValue& prune_setting)
+{
+    // -prune=1 setting is manual pruning mode, so disabled for purposes of the gui
+    return SettingToInt(prune_setting, 0) > 1;
+}
+
+//! Get pruning size value to show in GUI from bitcoin -prune setting. If
+//! pruning is not enabled, just show default recommended pruning size (2GB).
+static int PruneSizeGB(const util::SettingsValue& prune_setting)
+{
+    int value = SettingToInt(prune_setting, 0);
+    return value > 1 ? PruneMiBtoGB(value) : DEFAULT_PRUNE_TARGET_GB;
+}
+
+//! Parse pruning size value provided by user in GUI or loaded from QSettings
+//! (windows registry key or qt .conf file). Smallest value that the GUI can
+//! display is 1 GB, so round up if anything less is parsed.
+static int ParsePruneSizeGB(const QVariant& prune_size)
+{
+    return std::max(1, prune_size.toInt());
+}
+
+struct ProxySetting {
+    bool is_set;
+    QString ip;
+    QString port;
+};
+static ProxySetting ParseProxyString(const std::string& proxy);
+static std::string ProxyString(bool is_set, QString ip, QString port);
+
+OptionsModel::OptionsModel(interfaces::Node& node, QObject *parent) :
     QAbstractListModel(parent), m_node{node}
 {
-    Init(resetSettings);
 }
 
 void OptionsModel::addOverriddenOption(const std::string &option)
@@ -42,10 +128,17 @@ 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();
+    // Initialize display settings from stored settings.
+    m_prune_size_gb = PruneSizeGB(node().getPersistentSetting("prune"));
+    ProxySetting proxy = ParseProxyString(SettingToString(node().getPersistentSetting("proxy"), GetDefaultProxyAddress().toStdString()));
+    m_proxy_ip = proxy.ip;
+    m_proxy_port = proxy.port;
+    ProxySetting onion = ParseProxyString(SettingToString(node().getPersistentSetting("onion"), GetDefaultProxyAddress().toStdString()));
+    m_onion_ip = onion.ip;
+    m_onion_port = onion.port;
+    language = QString::fromStdString(SettingToString(node().getPersistentSetting("lang"), ""));
 
     checkAndMigrate();
 
@@ -98,130 +191,43 @@ 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, ThreadsScriptVerif, SpendZeroConfChange, ExternalSignerPath, MapPortUPnP,
+                            MapPortNatpmp, Listen, Server, Prune, ProxyUse, ProxyUseTor, Language}) {
+        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
-    // by command-line and show this in the UI.
 
     // Main
-    if (!settings.contains("bPrune"))
-        settings.setValue("bPrune", false);
-    if (!settings.contains("nPruneSize"))
-        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()))
-        addOverriddenOption("-par");
-
     if (!settings.contains("strDataDir"))
         settings.setValue("strDataDir", GUIUtil::getDefaultDataDirectory());
 
     // Wallet
 #ifdef ENABLE_WALLET
-    if (!settings.contains("bSpendZeroConfChange"))
-        settings.setValue("bSpendZeroConfChange", true);
-    if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
-        addOverriddenOption("-spendzeroconfchange");
-
-    if (!settings.contains("external_signer_path"))
-        settings.setValue("external_signer_path", "");
-
-    if (!gArgs.SoftSetArg("-signer", settings.value("external_signer_path").toString().toStdString())) {
-        addOverriddenOption("-signer");
-    }
-
     if (!settings.contains("SubFeeFromAmount")) {
         settings.setValue("SubFeeFromAmount", false);
     }
     m_sub_fee_from_amount = settings.value("SubFeeFromAmount", false).toBool();
 #endif
 
-    // Network
-    if (!settings.contains("fUseUPnP"))
-        settings.setValue("fUseUPnP", DEFAULT_UPNP);
-    if (!gArgs.SoftSetBoolArg("-upnp", settings.value("fUseUPnP").toBool()))
-        addOverriddenOption("-upnp");
-
-    if (!settings.contains("fUseNatpmp")) {
-        settings.setValue("fUseNatpmp", DEFAULT_NATPMP);
-    }
-    if (!gArgs.SoftSetBoolArg("-natpmp", settings.value("fUseNatpmp").toBool())) {
-        addOverriddenOption("-natpmp");
-    }
-
-    if (!settings.contains("fListen"))
-        settings.setValue("fListen", DEFAULT_LISTEN);
-    const bool listen{settings.value("fListen").toBool()};
-    if (!gArgs.SoftSetBoolArg("-listen", listen)) {
-        addOverriddenOption("-listen");
-    } else if (!listen) {
-        // We successfully set -listen=0, thus mimic the logic from InitParameterInteraction():
-        // "parameter interaction: -listen=0 -> setting -listenonion=0".
-        //
-        // Both -listen and -listenonion default to true.
-        //
-        // The call order is:
-        //
-        // InitParameterInteraction()
-        //     would set -listenonion=0 if it sees -listen=0, but for bitcoin-qt with
-        //     fListen=false -listen is 1 at this point
-        //
-        // OptionsModel::Init()
-        //     (this method) can flip -listen from 1 to 0 if fListen=false
-        //
-        // AppInitParameterInteraction()
-        //     raises an error if -listen=0 and -listenonion=1
-        gArgs.SoftSetBoolArg("-listenonion", false);
-    }
-
-    if (!settings.contains("server")) {
-        settings.setValue("server", false);
-    }
-    if (!gArgs.SoftSetBoolArg("-server", settings.value("server").toBool())) {
-        addOverriddenOption("-server");
-    }
-
-    if (!settings.contains("fUseProxy"))
-        settings.setValue("fUseProxy", false);
-    if (!settings.contains("addrProxy"))
-        settings.setValue("addrProxy", GetDefaultProxyAddress());
-    // Only try to set -proxy, if user has enabled fUseProxy
-    if ((settings.value("fUseProxy").toBool() && !gArgs.SoftSetArg("-proxy", settings.value("addrProxy").toString().toStdString())))
-        addOverriddenOption("-proxy");
-    else if(!settings.value("fUseProxy").toBool() && !gArgs.GetArg("-proxy", "").empty())
-        addOverriddenOption("-proxy");
-
-    if (!settings.contains("fUseSeparateProxyTor"))
-        settings.setValue("fUseSeparateProxyTor", false);
-    if (!settings.contains("addrSeparateProxyTor"))
-        settings.setValue("addrSeparateProxyTor", GetDefaultProxyAddress());
-    // Only try to set -onion, if user has enabled fUseSeparateProxyTor
-    if ((settings.value("fUseSeparateProxyTor").toBool() && !gArgs.SoftSetArg("-onion", settings.value("addrSeparateProxyTor").toString().toStdString())))
-        addOverriddenOption("-onion");
-    else if(!settings.value("fUseSeparateProxyTor").toBool() && !gArgs.GetArg("-onion", "").empty())
-        addOverriddenOption("-onion");
-
     // Display
-    if (!settings.contains("language"))
-        settings.setValue("language", "");
-    if (!gArgs.SoftSetArg("-lang", settings.value("language").toString().toStdString()))
-        addOverriddenOption("-lang");
-
-    language = settings.value("language").toString();
-
     if (!settings.contains("UseEmbeddedMonospacedFont")) {
         settings.setValue("UseEmbeddedMonospacedFont", "true");
     }
     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.
@@ -245,6 +251,9 @@ static void BackupSettings(const fs::path& filename, const QSettings& src)
 
 void OptionsModel::Reset()
 {
+    // Backup and reset settings.json
+    node().resetSettings();
+
     QSettings settings;
 
     // Backup old settings to chain-specific datadir for troubleshooting
@@ -273,21 +282,15 @@ int OptionsModel::rowCount(const QModelIndex & parent) const
     return OptionIDRowCount;
 }
 
-struct ProxySetting {
-    bool is_set;
-    QString ip;
-    QString port;
-};
-
-static ProxySetting GetProxySetting(QSettings &settings, const QString &name)
+static ProxySetting ParseProxyString(const QString& proxy)
 {
     static const ProxySetting default_val = {false, DEFAULT_GUI_PROXY_HOST, QString("%1").arg(DEFAULT_GUI_PROXY_PORT)};
     // Handle the case that the setting is not set at all
-    if (!settings.contains(name)) {
+    if (proxy.isEmpty()) {
         return default_val;
     }
     // contains IP at index 0 and port at index 1
-    QStringList ip_port = GUIUtil::SplitSkipEmptyParts(settings.value(name).toString(), ":");
+    QStringList ip_port = GUIUtil::SplitSkipEmptyParts(proxy, ":");
     if (ip_port.size() == 2) {
         return {true, ip_port.at(0), ip_port.at(1)};
     } else { // Invalid: return default
@@ -295,39 +298,42 @@ static ProxySetting GetProxySetting(QSettings &settings, const QString &name)
     }
 }
 
-static void SetProxySetting(QSettings &settings, const QString &name, const ProxySetting &ip_port)
+static ProxySetting ParseProxyString(const std::string& proxy)
 {
-    settings.setValue(name, QString{ip_port.ip + QLatin1Char(':') + ip_port.port});
+    return ParseProxyString(QString::fromStdString(proxy));
 }
 
-static const QString GetDefaultProxyAddress()
+static std::string ProxyString(bool is_set, QString ip, QString port)
 {
-    return QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST).arg(DEFAULT_GUI_PROXY_PORT);
+    return is_set ? QString(ip + ":" + port).toStdString() : "";
 }
 
-void OptionsModel::SetPruneEnabled(bool prune, bool force)
+static const QString GetDefaultProxyAddress()
 {
-    QSettings settings;
-    settings.setValue("bPrune", prune);
-    const int64_t prune_target_mib = PruneGBtoMiB(settings.value("nPruneSize").toInt());
-    std::string prune_val = prune ? ToString(prune_target_mib) : "0";
-    if (force) {
-        gArgs.ForceSetArg("-prune", prune_val);
-        return;
-    }
-    if (!gArgs.SoftSetArg("-prune", prune_val)) {
-        addOverriddenOption("-prune");
-    }
+    return QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST).arg(DEFAULT_GUI_PROXY_PORT);
 }
 
-void OptionsModel::SetPruneTargetGB(int prune_target_gb, bool force)
+void OptionsModel::SetPruneTargetGB(int prune_target_gb)
 {
-    const bool prune = prune_target_gb > 0;
-    if (prune) {
-        QSettings settings;
-        settings.setValue("nPruneSize", prune_target_gb);
+    const util::SettingsValue cur_value = node().getPersistentSetting("prune");
+    const util::SettingsValue new_value = PruneSetting(prune_target_gb > 0, prune_target_gb);
+
+    m_prune_size_gb = prune_target_gb;
+
+    // Force setting to take effect. It is still safe to change the value at
+    // this point because this function is only called after the intro screen is
+    // shown, before the node starts.
+    node().forceSetting("prune", new_value);
+
+    // Update settings.json if value configured in intro screen is different
+    // from saved value. Avoid writing settings.json if bitcoin.conf value
+    // doesn't need to be overridden.
+    if (PruneEnabled(cur_value) != PruneEnabled(new_value) ||
+        PruneSizeGB(cur_value) != PruneSizeGB(new_value)) {
+        // Call UpdateRwSetting() instead of setOption() to avoid setting
+        // RestartRequired flag
+        UpdateRwSetting(node(), Prune, new_value);
     }
-    SetPruneEnabled(prune, force);
 }
 
 // read QSettings values and return them
@@ -356,6 +362,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:
@@ -366,13 +374,13 @@ QVariant OptionsModel::getOption(OptionID option) const
         return fMinimizeToTray;
     case MapPortUPnP:
 #ifdef USE_UPNP
-        return settings.value("fUseUPnP");
+        return SettingToBool(setting(), DEFAULT_UPNP);
 #else
         return false;
 #endif // USE_UPNP
     case MapPortNatpmp:
 #ifdef USE_NATPMP
-        return settings.value("fUseNatpmp");
+        return SettingToBool(setting(), DEFAULT_NATPMP);
 #else
         return false;
 #endif // USE_NATPMP
@@ -381,25 +389,25 @@ QVariant OptionsModel::getOption(OptionID option) const
 
     // default proxy
     case ProxyUse:
-        return settings.value("fUseProxy", false);
+        return ParseProxyString(SettingToString(setting(), "")).is_set;
     case ProxyIP:
-        return GetProxySetting(settings, "addrProxy").ip;
+        return m_proxy_ip;
     case ProxyPort:
-        return GetProxySetting(settings, "addrProxy").port;
+        return m_proxy_port;
 
     // separate Tor proxy
     case ProxyUseTor:
-        return settings.value("fUseSeparateProxyTor", false);
+        return ParseProxyString(SettingToString(setting(), "")).is_set;
     case ProxyIPTor:
-        return GetProxySetting(settings, "addrSeparateProxyTor").ip;
+        return m_onion_ip;
     case ProxyPortTor:
-        return GetProxySetting(settings, "addrSeparateProxyTor").port;
+        return m_onion_port;
 
 #ifdef ENABLE_WALLET
     case SpendZeroConfChange:
-        return settings.value("bSpendZeroConfChange");
+        return SettingToBool(setting(), wallet::DEFAULT_SPEND_ZEROCONF_CHANGE);
     case ExternalSignerPath:
-        return settings.value("external_signer_path");
+        return QString::fromStdString(SettingToString(setting(), ""));
     case SubFeeFromAmount:
         return m_sub_fee_from_amount;
 #endif
@@ -408,7 +416,7 @@ QVariant OptionsModel::getOption(OptionID option) const
     case ThirdPartyTxUrls:
         return strThirdPartyTxUrls;
     case Language:
-        return settings.value("language");
+        return QString::fromStdString(SettingToString(setting(), ""));
     case UseEmbeddedMonospacedFont:
         return m_use_embedded_monospaced_font;
     case CoinControlFeatures:
@@ -416,17 +424,17 @@ QVariant OptionsModel::getOption(OptionID option) const
     case EnablePSBTControls:
         return settings.value("enable_psbt_controls");
     case Prune:
-        return settings.value("bPrune");
+        return PruneEnabled(setting());
     case PruneSize:
-        return settings.value("nPruneSize");
+        return m_prune_size_gb;
     case DatabaseCache:
-        return settings.value("nDatabaseCache");
+        return qlonglong(SettingToInt(setting(), nDefaultDbCache));
     case ThreadsScriptVerif:
-        return settings.value("nThreadsScriptVerif");
+        return qlonglong(SettingToInt(setting(), DEFAULT_SCRIPTCHECK_THREADS));
     case Listen:
-        return settings.value("fListen");
+        return SettingToBool(setting(), DEFAULT_LISTEN);
     case Server:
-        return settings.value("server");
+        return SettingToBool(setting(), false);
     default:
         return QVariant();
     }
@@ -434,6 +442,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;
 
@@ -451,10 +462,16 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
         settings.setValue("fMinimizeToTray", fMinimizeToTray);
         break;
     case MapPortUPnP: // core option - can be changed on-the-fly
-        settings.setValue("fUseUPnP", value.toBool());
+        if (changed()) {
+            update(value.toBool());
+            node().mapPort(value.toBool(), getOption(MapPortNatpmp).toBool());
+        }
         break;
     case MapPortNatpmp: // core option - can be changed on-the-fly
-        settings.setValue("fUseNatpmp", value.toBool());
+        if (changed()) {
+            update(value.toBool());
+            node().mapPort(getOption(MapPortUPnP).toBool(), value.toBool());
+        }
         break;
     case MinimizeOnClose:
         fMinimizeOnClose = value.toBool();
@@ -463,66 +480,66 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
 
     // default proxy
     case ProxyUse:
-        if (settings.value("fUseProxy") != value) {
-            settings.setValue("fUseProxy", value.toBool());
+        if (changed()) {
+            update(ProxyString(value.toBool(), m_proxy_ip, m_proxy_port));
             setRestartRequired(true);
         }
         break;
-    case ProxyIP: {
-        auto ip_port = GetProxySetting(settings, "addrProxy");
-        if (!ip_port.is_set || ip_port.ip != value.toString()) {
-            ip_port.ip = value.toString();
-            SetProxySetting(settings, "addrProxy", ip_port);
-            setRestartRequired(true);
+    case ProxyIP:
+        if (changed()) {
+            m_proxy_ip = value.toString();
+            if (getOption(ProxyUse).toBool()) {
+                update(ProxyString(true, m_proxy_ip, m_proxy_port));
+                setRestartRequired(true);
+            }
         }
-    }
-    break;
-    case ProxyPort: {
-        auto ip_port = GetProxySetting(settings, "addrProxy");
-        if (!ip_port.is_set || ip_port.port != value.toString()) {
-            ip_port.port = value.toString();
-            SetProxySetting(settings, "addrProxy", ip_port);
-            setRestartRequired(true);
+        break;
+    case ProxyPort:
+        if (changed()) {
+            m_proxy_port = value.toString();
+            if (getOption(ProxyUse).toBool()) {
+                update(ProxyString(true, m_proxy_ip, m_proxy_port));
+                setRestartRequired(true);
+            }
         }
-    }
-    break;
+        break;
 
     // separate Tor proxy
     case ProxyUseTor:
-        if (settings.value("fUseSeparateProxyTor") != value) {
-            settings.setValue("fUseSeparateProxyTor", value.toBool());
+        if (changed()) {
+            update(ProxyString(value.toBool(), m_onion_ip, m_onion_port));
             setRestartRequired(true);
         }
         break;
-    case ProxyIPTor: {
-        auto ip_port = GetProxySetting(settings, "addrSeparateProxyTor");
-        if (!ip_port.is_set || ip_port.ip != value.toString()) {
-            ip_port.ip = value.toString();
-            SetProxySetting(settings, "addrSeparateProxyTor", ip_port);
-            setRestartRequired(true);
+    case ProxyIPTor:
+        if (changed()) {
+            m_onion_ip = value.toString();
+            if (getOption(ProxyUseTor).toBool()) {
+                update(ProxyString(true, m_onion_ip, m_onion_port));
+                setRestartRequired(true);
+            }
         }
-    }
-    break;
-    case ProxyPortTor: {
-        auto ip_port = GetProxySetting(settings, "addrSeparateProxyTor");
-        if (!ip_port.is_set || ip_port.port != value.toString()) {
-            ip_port.port = value.toString();
-            SetProxySetting(settings, "addrSeparateProxyTor", ip_port);
-            setRestartRequired(true);
+        break;
+    case ProxyPortTor:
+        if (changed()) {
+            m_onion_port = value.toString();
+            if (getOption(ProxyUseTor).toBool()) {
+                update(ProxyString(true, m_onion_ip, m_onion_port));
+                setRestartRequired(true);
+            }
         }
-    }
-    break;
+        break;
 
 #ifdef ENABLE_WALLET
     case SpendZeroConfChange:
-        if (settings.value("bSpendZeroConfChange") != value) {
-            settings.setValue("bSpendZeroConfChange", value);
+        if (changed()) {
+            update(value.toBool());
             setRestartRequired(true);
         }
         break;
     case ExternalSignerPath:
-        if (settings.value("external_signer_path") != value.toString()) {
-            settings.setValue("external_signer_path", value.toString());
+        if (changed()) {
+            update(value.toString().toStdString());
             setRestartRequired(true);
         }
         break;
@@ -542,8 +559,8 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
         }
         break;
     case Language:
-        if (settings.value("language") != value) {
-            settings.setValue("language", value);
+        if (changed()) {
+            update(value.toString().toStdString());
             setRestartRequired(true);
         }
         break;
@@ -562,38 +579,36 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
         settings.setValue("enable_psbt_controls", m_enable_psbt_controls);
         break;
     case Prune:
-        if (settings.value("bPrune") != value) {
-            settings.setValue("bPrune", value);
+        if (changed()) {
+            update(PruneSetting(value.toBool(), m_prune_size_gb));
             setRestartRequired(true);
         }
         break;
     case PruneSize:
-        if (settings.value("nPruneSize") != value) {
-            settings.setValue("nPruneSize", value);
-            setRestartRequired(true);
+        if (changed()) {
+            m_prune_size_gb = ParsePruneSizeGB(value);
+            if (getOption(Prune).toBool()) {
+                update(PruneSetting(true, m_prune_size_gb));
+                setRestartRequired(true);
+            }
         }
         break;
     case DatabaseCache:
-        if (settings.value("nDatabaseCache") != value) {
-            settings.setValue("nDatabaseCache", value);
+        if (changed()) {
+            update(static_cast<int64_t>(value.toLongLong()));
             setRestartRequired(true);
         }
         break;
     case ThreadsScriptVerif:
-        if (settings.value("nThreadsScriptVerif") != value) {
-            settings.setValue("nThreadsScriptVerif", value);
+        if (changed()) {
+            update(static_cast<int64_t>(value.toLongLong()));
             setRestartRequired(true);
         }
         break;
     case Listen:
-        if (settings.value("fListen") != value) {
-            settings.setValue("fListen", value);
-            setRestartRequired(true);
-        }
-        break;
     case Server:
-        if (settings.value("server") != value) {
-            settings.setValue("server", value);
+        if (changed()) {
+            update(value.toBool());
             setRestartRequired(true);
         }
         break;
@@ -654,4 +669,49 @@ 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()) {
+            if (option == ProxyIP) {
+                ProxySetting parsed = ParseProxyString(value.toString());
+                setOption(ProxyIP, parsed.ip);
+                setOption(ProxyPort, parsed.port);
+            } else if (option == ProxyIPTor) {
+                ProxySetting parsed = ParseProxyString(value.toString());
+                setOption(ProxyIPTor, parsed.ip);
+                setOption(ProxyPortTor, parsed.port);
+            } else {
+                setOption(option, value);
+            }
+        }
+        settings.remove(qt_name);
+    };
+
+    migrate_setting(DatabaseCache, "nDatabaseCache");
+    migrate_setting(ThreadsScriptVerif, "nThreadsScriptVerif");
+#ifdef ENABLE_WALLET
+    migrate_setting(SpendZeroConfChange, "bSpendZeroConfChange");
+    migrate_setting(ExternalSignerPath, "external_signer_path");
+#endif
+    migrate_setting(MapPortUPnP, "fUseUPnP");
+    migrate_setting(MapPortNatpmp, "fUseNatpmp");
+    migrate_setting(Listen, "fListen");
+    migrate_setting(Server, "server");
+    migrate_setting(PruneSize, "nPruneSize");
+    migrate_setting(Prune, "bPrune");
+    migrate_setting(ProxyIP, "addrProxy");
+    migrate_setting(ProxyUse, "fUseProxy");
+    migrate_setting(ProxyIPTor, "addrSeparateProxyTor");
+    migrate_setting(ProxyUseTor, "fUseSeparateProxyTor");
+    migrate_setting(Language, "language");
+
+    // In case migrating QSettings caused any settings value to change, rerun
+    // parameter interaction code to update other settings. This is particularly
+    // important for the -listen setting, which should cause -listenonion, -upnp,
+    // and other settings to default to false if it was set to false.
+    // (https://github.com/bitcoin-core/gui/issues/567).
+    node().initParameterInteraction();
 }
diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h
index 92f80ecf21..42b89c5029 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;
@@ -98,8 +99,7 @@ public:
     const QString& getOverriddenByCommandLine() { return strOverriddenByCommandLine; }
 
     /* Explicit setters */
-    void SetPruneEnabled(bool prune, bool force = false);
-    void SetPruneTargetGB(int prune_target_gb, bool force = false);
+    void SetPruneTargetGB(int prune_target_gb);
 
     /* Restart flag helper */
     void setRestartRequired(bool fRequired);
@@ -120,6 +120,16 @@ private:
     bool fCoinControlFeatures;
     bool m_sub_fee_from_amount;
     bool m_enable_psbt_controls;
+
+    //! In-memory settings for display. These are stored persistently by the
+    //! bitcoin node but it's also nice to store them in memory to prevent them
+    //! getting cleared when enable/disable toggles are used in the GUI.
+    int m_prune_size_gb;
+    QString m_proxy_ip;
+    QString m_proxy_port;
+    QString m_onion_ip;
+    QString m_onion_port;
+
     /* settings that were overridden by command-line */
     QString strOverriddenByCommandLine;
 
@@ -128,6 +138,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 3b7a40438b..581735263d 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..3bd0af19ad 100644
--- a/src/qt/test/optiontests.cpp
+++ b/src/qt/test/optiontests.cpp
@@ -13,6 +13,63 @@
 
 #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.setValue("nThreadsScriptVerif", 12);
+    settings.setValue("fUseUPnP", false);
+    settings.setValue("fListen", false);
+    settings.setValue("bPrune", true);
+    settings.setValue("nPruneSize", 3);
+    settings.setValue("fUseProxy", true);
+    settings.setValue("addrProxy", "proxy:123");
+    settings.setValue("fUseSeparateProxyTor", true);
+    settings.setValue("addrSeparateProxyTor", "onion:234");
+
+    settings.sync();
+
+    OptionsModel options{m_node};
+    bilingual_str error;
+    QVERIFY(options.Init(error));
+    QVERIFY(!settings.contains("nDatabaseCache"));
+    QVERIFY(!settings.contains("nThreadsScriptVerif"));
+    QVERIFY(!settings.contains("fUseUPnP"));
+    QVERIFY(!settings.contains("fListen"));
+    QVERIFY(!settings.contains("bPrune"));
+    QVERIFY(!settings.contains("nPruneSize"));
+    QVERIFY(!settings.contains("fUseProxy"));
+    QVERIFY(!settings.contains("addrProxy"));
+    QVERIFY(!settings.contains("fUseSeparateProxyTor"));
+    QVERIFY(!settings.contains("addrSeparateProxyTor"));
+
+    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"
+        "    \"listen\": false,\n"
+        "    \"onion\": \"onion:234\",\n"
+        "    \"par\": \"12\",\n"
+        "    \"proxy\": \"proxy:123\",\n"
+        "    \"prune\": \"2861\"\n"
+        "}\n");
+}
+
 void OptionTests::integerGetArgBug()
 {
     // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Ensure
@@ -23,7 +80,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 +94,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 +104,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);