Merge #603: Add settings.json prune-prev, proxy-prev, onion-prev settings

9d3127b Add settings.json prune-prev, proxy-prev, onion-prev settings (Ryan Ofsky)

Pull request description:

  With #602, if proxy and pruning settings are disabled in the GUI and the GUI is restarted, proxy and prune values are not stored anywhere. So if these settings are enabled in the future, default values will be shown, not previous values.

  This PR stores previous values so they will preserved across restarts. I'm not sure I like this behavior because showing default values seems simpler and safer to me. Previous values may just have been set temporarily and may have never actually worked, and it adds some code complexity to store them.

  This PR is one way of resolving #596. Other solutions are possible and could be implemented as alternatives.

ACKs for top commit:
  hebasto:
    ACK 9d3127b, tested on Ubuntu 22.04.
  vasild:
    ACK 9d3127b
  jarolrod:
    tACK 9d3127b

Tree-SHA512: 1778d1819443490c880cfd5c1711d9c5ac75ea3ee8440e2f0ced81d293247163a78ae8aba6027215110aec6533bd7dc6472aeead6796bfbd51bf2354e28f24a9

Author: hebasto

src/qt/optionsmodel.cpp

@@ -59,7 +59,7 @@ static const char* SettingName(OptionsModel::OptionID option)
 }
 
 /** Call node.updateRwSetting() with Bitcoin 22.x workaround. */
-static void UpdateRwSetting(interfaces::Node& node, OptionsModel::OptionID option, const util::SettingsValue& value)
+static void UpdateRwSetting(interfaces::Node& node, OptionsModel::OptionID option, const std::string& suffix, const util::SettingsValue& value)
 {
     if (value.isNum() &&
         (option == OptionsModel::DatabaseCache ||
@@ -73,9 +73,9 @@ static void UpdateRwSetting(interfaces::Node& node, OptionsModel::OptionID optio
         // 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());
+        node.updateRwSetting(SettingName(option) + suffix, value.getValStr());
     } else {
-        node.updateRwSetting(SettingName(option), value);
+        node.updateRwSetting(SettingName(option) + suffix, value);
     }
 }
 
@@ -131,13 +131,6 @@ void OptionsModel::addOverriddenOption(const std::string &option)
 bool OptionsModel::Init(bilingual_str& error)
 {
     // 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();
@@ -320,8 +313,6 @@ void OptionsModel::SetPruneTargetGB(int 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.
@@ -334,7 +325,12 @@ void OptionsModel::SetPruneTargetGB(int prune_target_gb)
         PruneSizeGB(cur_value) != PruneSizeGB(new_value)) {
         // Call UpdateRwSetting() instead of setOption() to avoid setting
         // RestartRequired flag
-        UpdateRwSetting(node(), Prune, new_value);
+        UpdateRwSetting(node(), Prune, "", new_value);
+    }
+
+    // Keep previous pruning size, if pruning was disabled.
+    if (PruneEnabled(cur_value)) {
+        UpdateRwSetting(node(), Prune, "-prev", PruneEnabled(new_value) ? util::SettingsValue{} : cur_value);
     }
 }
 
@@ -362,9 +358,9 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
     return successful;
 }
 
-QVariant OptionsModel::getOption(OptionID option) const
+QVariant OptionsModel::getOption(OptionID option, const std::string& suffix) const
 {
-    auto setting = [&]{ return node().getPersistentSetting(SettingName(option)); };
+    auto setting = [&]{ return node().getPersistentSetting(SettingName(option) + suffix); };
 
     QSettings settings;
     switch (option) {
@@ -391,19 +387,30 @@ QVariant OptionsModel::getOption(OptionID option) const
 
     // default proxy
     case ProxyUse:
+    case ProxyUseTor:
         return ParseProxyString(SettingToString(setting(), "")).is_set;
     case ProxyIP:
-        return m_proxy_ip;
+    case ProxyIPTor: {
+        ProxySetting proxy = ParseProxyString(SettingToString(setting(), ""));
+        if (proxy.is_set) {
+            return proxy.ip;
+        } else if (suffix.empty()) {
+            return getOption(option, "-prev");
+        } else {
+            return ParseProxyString(GetDefaultProxyAddress().toStdString()).ip;
+        }
+    }
     case ProxyPort:
-        return m_proxy_port;
-
-    // separate Tor proxy
-    case ProxyUseTor:
-        return ParseProxyString(SettingToString(setting(), "")).is_set;
-    case ProxyIPTor:
-        return m_onion_ip;
-    case ProxyPortTor:
-        return m_onion_port;
+    case ProxyPortTor: {
+        ProxySetting proxy = ParseProxyString(SettingToString(setting(), ""));
+        if (proxy.is_set) {
+            return proxy.port;
+        } else if (suffix.empty()) {
+            return getOption(option, "-prev");
+        } else {
+            return ParseProxyString(GetDefaultProxyAddress().toStdString()).port;
+        }
+    }
 
 #ifdef ENABLE_WALLET
     case SpendZeroConfChange:
@@ -428,7 +435,9 @@ QVariant OptionsModel::getOption(OptionID option) const
     case Prune:
         return PruneEnabled(setting());
     case PruneSize:
-        return m_prune_size_gb;
+        return PruneEnabled(setting()) ? PruneSizeGB(setting()) :
+               suffix.empty()          ? getOption(option, "-prev") :
+                                         DEFAULT_PRUNE_TARGET_GB;
     case DatabaseCache:
         return qlonglong(SettingToInt(setting(), nDefaultDbCache));
     case ThreadsScriptVerif:
@@ -444,10 +453,10 @@ QVariant OptionsModel::getOption(OptionID option) const
     }
 }
 
-bool OptionsModel::setOption(OptionID option, const QVariant& value)
+bool OptionsModel::setOption(OptionID option, const QVariant& value, const std::string& suffix)
 {
-    auto changed = [&] { return value.isValid() && value != getOption(option); };
-    auto update = [&](const util::SettingsValue& value) { return UpdateRwSetting(node(), option, value); };
+    auto changed = [&] { return value.isValid() && value != getOption(option, suffix); };
+    auto update = [&](const util::SettingsValue& value) { return UpdateRwSetting(node(), option, suffix, value); };
 
     bool successful = true; /* set to false on parse error */
     QSettings settings;
@@ -485,52 +494,60 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
     // default proxy
     case ProxyUse:
         if (changed()) {
-            update(ProxyString(value.toBool(), m_proxy_ip, m_proxy_port));
-            setRestartRequired(true);
+            if (suffix.empty() && !value.toBool()) setOption(option, true, "-prev");
+            update(ProxyString(value.toBool(), getOption(ProxyIP).toString(), getOption(ProxyPort).toString()));
+            if (suffix.empty() && value.toBool()) UpdateRwSetting(node(), option, "-prev", {});
+            if (suffix.empty()) setRestartRequired(true);
         }
         break;
     case ProxyIP:
         if (changed()) {
-            m_proxy_ip = value.toString();
-            if (getOption(ProxyUse).toBool()) {
-                update(ProxyString(true, m_proxy_ip, m_proxy_port));
-                setRestartRequired(true);
+            if (suffix.empty() && !getOption(ProxyUse).toBool()) {
+                setOption(option, value, "-prev");
+            } else {
+                update(ProxyString(true, value.toString(), getOption(ProxyPort).toString()));
             }
+            if (suffix.empty() && getOption(ProxyUse).toBool()) 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);
+            if (suffix.empty() && !getOption(ProxyUse).toBool()) {
+                setOption(option, value, "-prev");
+            } else {
+                update(ProxyString(true, getOption(ProxyIP).toString(), value.toString()));
             }
+            if (suffix.empty() && getOption(ProxyUse).toBool()) setRestartRequired(true);
         }
         break;
 
     // separate Tor proxy
     case ProxyUseTor:
         if (changed()) {
-            update(ProxyString(value.toBool(), m_onion_ip, m_onion_port));
-            setRestartRequired(true);
+            if (suffix.empty() && !value.toBool()) setOption(option, true, "-prev");
+            update(ProxyString(value.toBool(), getOption(ProxyIPTor).toString(), getOption(ProxyPortTor).toString()));
+            if (suffix.empty() && value.toBool()) UpdateRwSetting(node(), option, "-prev", {});
+            if (suffix.empty()) setRestartRequired(true);
         }
         break;
     case ProxyIPTor:
         if (changed()) {
-            m_onion_ip = value.toString();
-            if (getOption(ProxyUseTor).toBool()) {
-                update(ProxyString(true, m_onion_ip, m_onion_port));
-                setRestartRequired(true);
+            if (suffix.empty() && !getOption(ProxyUseTor).toBool()) {
+                setOption(option, value, "-prev");
+            } else {
+                update(ProxyString(true, value.toString(), getOption(ProxyPortTor).toString()));
             }
+            if (suffix.empty() && getOption(ProxyUseTor).toBool()) 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);
+            if (suffix.empty() && !getOption(ProxyUseTor).toBool()) {
+                setOption(option, value, "-prev");
+            } else {
+                update(ProxyString(true, getOption(ProxyIPTor).toString(), value.toString()));
             }
+            if (suffix.empty() && getOption(ProxyUseTor).toBool()) setRestartRequired(true);
         }
         break;
 
@@ -584,17 +601,20 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
         break;
     case Prune:
         if (changed()) {
-            update(PruneSetting(value.toBool(), m_prune_size_gb));
-            setRestartRequired(true);
+            if (suffix.empty() && !value.toBool()) setOption(option, true, "-prev");
+            update(PruneSetting(value.toBool(), getOption(PruneSize).toInt()));
+            if (suffix.empty() && value.toBool()) UpdateRwSetting(node(), option, "-prev", {});
+            if (suffix.empty()) setRestartRequired(true);
         }
         break;
     case PruneSize:
         if (changed()) {
-            m_prune_size_gb = ParsePruneSizeGB(value);
-            if (getOption(Prune).toBool()) {
-                update(PruneSetting(true, m_prune_size_gb));
-                setRestartRequired(true);
+            if (suffix.empty() && !getOption(Prune).toBool()) {
+                setOption(option, value, "-prev");
+            } else {
+                update(PruneSetting(true, ParsePruneSizeGB(value)));
             }
+            if (suffix.empty() && getOption(Prune).toBool()) setRestartRequired(true);
         }
         break;
     case DatabaseCache:

src/qt/optionsmodel.h

@@ -82,8 +82,8 @@ class OptionsModel : public QAbstractListModel
     int rowCount(const QModelIndex & parent = QModelIndex()) const override;
     QVariant data(const QModelIndex & index, int role = Qt::DisplayRole) const override;
     bool setData(const QModelIndex & index, const QVariant & value, int role = Qt::EditRole) override;
-    QVariant getOption(OptionID option) const;
-    bool setOption(OptionID option, const QVariant& value);
+    QVariant getOption(OptionID option, const std::string& suffix="") const;
+    bool setOption(OptionID option, const QVariant& value, const std::string& suffix="");
     /** Updates current unit in memory, settings and emits displayUnitChanged(new_unit) signal */
     void setDisplayUnit(const QVariant& new_unit);
 
@@ -123,15 +123,6 @@ class OptionsModel : public QAbstractListModel
     bool m_enable_psbt_controls;
     bool m_mask_values;
 
-    //! 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;