Migrate -listen and -server settings from QSettings to settings.json

This commit also removes the `node.args = nullptr` assignment in the Shutdown()
function. Clearing node.args there never made sense because it made the
Shutdown() function not idempotent, making it fragile and causing issues like
bitcoin/bitcoin#23186. Removing that assignment is
necessary in this commit to avoid a segfault in OptionTests during the new
node().initParameterInteraction() call added here. AppTests calls Shutdown()
which sets node.args to null, and OptionTests runs after AppTests and needs
node.args not to be null.

Author: ryanofsky

src/qt/optionsmodel.cpp

@@ -43,6 +43,8 @@ static const char* SettingName(OptionsModel::OptionID option)
     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";
     default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
     }
 }
@@ -131,7 +133,7 @@ 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, MapPortUPnP,
-                            MapPortNatpmp}) {
+                            MapPortNatpmp, Listen, Server}) {
         std::string setting = SettingName(option);
         if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting);
         try {
@@ -167,37 +169,6 @@ bool OptionsModel::Init(bilingual_str& error)
 #endif
 
     // Network
-    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);
@@ -438,9 +409,9 @@ QVariant OptionsModel::getOption(OptionID option) const
     case ThreadsScriptVerif:
         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();
     }
@@ -609,14 +580,14 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
         }
         break;
     case Listen:
-        if (settings.value("fListen") != value) {
-            settings.setValue("fListen", value);
+        if (changed()) {
+            update(value.toBool());
             setRestartRequired(true);
         }
         break;
     case Server:
-        if (settings.value("server") != value) {
-            settings.setValue("server", value);
+        if (changed()) {
+            update(value.toBool());
             setRestartRequired(true);
         }
         break;
@@ -696,4 +667,13 @@ void OptionsModel::checkAndMigrate()
 #endif
     migrate_setting(MapPortUPnP, "fUseUPnP");
     migrate_setting(MapPortNatpmp, "fUseNatpmp");
+    migrate_setting(Listen, "fListen");
+    migrate_setting(Server, "server");
+
+    // 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();
 }

src/qt/test/optiontests.cpp

@@ -36,6 +36,7 @@ void OptionTests::migrateSettings()
     settings.setValue("nDatabaseCache", 600);
     settings.setValue("nThreadsScriptVerif", 12);
     settings.setValue("fUseUPnP", false);
+    settings.setValue("fListen", false);
 
     settings.sync();
 
@@ -45,10 +46,12 @@ void OptionTests::migrateSettings()
     QVERIFY(!settings.contains("nDatabaseCache"));
     QVERIFY(!settings.contains("nThreadsScriptVerif"));
     QVERIFY(!settings.contains("fUseUPnP"));
+    QVERIFY(!settings.contains("fListen"));
 
     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"
         "    \"par\": \"12\"\n"
         "}\n");
 }