Migrate -upnp and -natpmp settings from QSettings to settings.json

This also effectively reverts 58e8364 from #18077, applying upnp and natpmp settings from the optionsmodel class instead of the optionsdialog class. This makes sense because model code, not view code is responsible for applying all other settings, and because leaving these settings half-applied in optionsmodel seems error prone and could lead to bugs. (These things were discussed a little in bitcoin/bitcoin#18077 (comment))
ryanofsky · Apr 18, 2022 · f854ab8 · f854ab8
1 parent 31c0219
commit f854ab8
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 22 deletions.
diff --git 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
@@ -41,6 +41,8 @@ static const char* SettingName(OptionsModel::OptionID option)
     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";
     default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
     }
 }
@@ -128,7 +130,8 @@ bool OptionsModel::Init(bilingual_str& error)
 
     // 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}) {
+    for (OptionID option : {DatabaseCache, ThreadsScriptVerif, SpendZeroConfChange, ExternalSignerPath, MapPortUPnP,
+                            MapPortNatpmp}) {
         std::string setting = SettingName(option);
         if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting);
         try {
@@ -164,18 +167,6 @@ bool OptionsModel::Init(bilingual_str& error)
 #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()};
@@ -389,13 +380,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
@@ -477,10 +468,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();
@@ -697,4 +694,6 @@ void OptionsModel::checkAndMigrate()
     migrate_setting(SpendZeroConfChange, "bSpendZeroConfChange");
     migrate_setting(ExternalSignerPath, "external_signer_path");
 #endif
+    migrate_setting(MapPortUPnP, "fUseUPnP");
+    migrate_setting(MapPortNatpmp, "fUseNatpmp");
 }
diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp
@@ -35,6 +35,7 @@ void OptionTests::migrateSettings()
     QSettings settings;
     settings.setValue("nDatabaseCache", 600);
     settings.setValue("nThreadsScriptVerif", 12);
+    settings.setValue("fUseUPnP", false);
 
     settings.sync();
 
@@ -43,6 +44,7 @@ void OptionTests::migrateSettings()
     QVERIFY(options.Init(error));
     QVERIFY(!settings.contains("nDatabaseCache"));
     QVERIFY(!settings.contains("nThreadsScriptVerif"));
+    QVERIFY(!settings.contains("fUseUPnP"));
 
     std::ifstream file(gArgs.GetDataDirNet() / "settings.json");
     QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n"