From 284f339de68905131331f7fdb4c0b945c9a1b8cd Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 01/10] 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 --- src/qt/bitcoin.cpp | 25 ++++++++-- src/qt/bitcoin.h | 2 +- src/qt/optionsmodel.cpp | 81 ++++++++++++++++++++++++++------ src/qt/optionsmodel.h | 7 ++- src/qt/test/addressbooktests.cpp | 2 + src/qt/test/apptests.cpp | 7 +-- src/qt/test/optiontests.cpp | 42 +++++++++++++++-- src/qt/test/optiontests.h | 7 ++- src/qt/test/test_main.cpp | 5 +- src/qt/test/wallettests.cpp | 2 + 10 files changed, 146 insertions(+), 34 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 53fbac01060..a7003c95ab7 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 7a6aa5cdc93..9ad37ca6c97 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 612d3009c1b..6f61afed54c 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -26,14 +26,41 @@ #include #include +#include + 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(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 /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 92f80ecf21d..9f8d17b305c 100644 --- a/src/qt/optionsmodel.h +++ b/src/qt/optionsmodel.h @@ -13,6 +13,7 @@ #include +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 ededde4da9e..bc9dbae44aa 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 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 9648ef6188e..6fc7a524358 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"); m_app.parameterSetup(); - m_app.createOptionsModel(true /* reset settings */); + QVERIFY(m_app.createOptionsModel(true /* reset settings */)); QScopedPointer 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 2ef9230c598..fb8b78f66ee 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -13,6 +13,40 @@ #include +#include + +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(file), std::istreambuf_iterator()).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 257a0b65bef..286d7855722 100644 --- a/src/qt/test/optiontests.h +++ b/src/qt/test/optiontests.h @@ -6,6 +6,8 @@ #define BITCOIN_QT_TEST_OPTIONTESTS_H #include +#include +#include #include @@ -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 aeedd928343..de23f51c92a 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 init = interfaces::MakeGuiInit(argc, argv); gArgs.ForceSetArg("-listen", "0"); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index bc06f0f23be..7671bfc739d 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); From a7ef6d5975a5f40b90b2709b32a00647bd2bd5a3 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 02/10] Migrate -par setting from QSettings to settings.json --- src/qt/optionsmodel.cpp | 20 +++++++++----------- src/qt/test/optiontests.cpp | 5 ++++- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 6f61afed54c..b42dbf40df8 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -37,6 +37,7 @@ static const char* SettingName(OptionsModel::OptionID option) { switch (option) { case OptionsModel::DatabaseCache: return "dbcache"; + case OptionsModel::ThreadsScriptVerif: return "par"; default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option)); } } @@ -44,7 +45,9 @@ 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) { - if (value.isNum() && option == OptionsModel::DatabaseCache) { + if (value.isNum() && + (option == OptionsModel::DatabaseCache || + option == OptionsModel::ThreadsScriptVerif)) { // 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 @@ -122,7 +125,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}) { + for (OptionID option : {DatabaseCache, ThreadsScriptVerif}) { std::string setting = SettingName(option); if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting); try { @@ -147,12 +150,6 @@ bool OptionsModel::Init(bilingual_str& error) if (!settings.contains("nPruneSize")) settings.setValue("nPruneSize", DEFAULT_PRUNE_TARGET_GB); SetPruneEnabled(settings.value("bPrune").toBool()); - - 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()); @@ -458,7 +455,7 @@ QVariant OptionsModel::getOption(OptionID option) const case DatabaseCache: return qlonglong(SettingToInt(setting(), nDefaultDbCache)); case ThreadsScriptVerif: - return settings.value("nThreadsScriptVerif"); + return qlonglong(SettingToInt(setting(), DEFAULT_SCRIPTCHECK_THREADS)); case Listen: return settings.value("fListen"); case Server: @@ -619,8 +616,8 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value) } break; case ThreadsScriptVerif: - if (settings.value("nThreadsScriptVerif") != value) { - settings.setValue("nThreadsScriptVerif", value); + if (changed()) { + update(static_cast(value.toLongLong())); setRestartRequired(true); } break; @@ -705,4 +702,5 @@ void OptionsModel::checkAndMigrate() }; migrate_setting(DatabaseCache, "nDatabaseCache"); + migrate_setting(ThreadsScriptVerif, "nThreadsScriptVerif"); } diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index fb8b78f66ee..fa3fe031ecd 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -33,6 +33,7 @@ void OptionTests::migrateSettings() // settings.json QSettings settings; settings.setValue("nDatabaseCache", 600); + settings.setValue("nThreadsScriptVerif", 12); settings.sync(); @@ -40,10 +41,12 @@ void OptionTests::migrateSettings() bilingual_str error; QVERIFY(options.Init(error)); QVERIFY(!settings.contains("nDatabaseCache")); + QVERIFY(!settings.contains("nThreadsScriptVerif")); std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); QCOMPARE(std::string(std::istreambuf_iterator(file), std::istreambuf_iterator()).c_str(), "{\n" - " \"dbcache\": \"600\"\n" + " \"dbcache\": \"600\",\n" + " \"par\": \"12\"\n" "}\n"); } From 1dc4fc29c1086420b7dc51b20c0b7a18fecb4462 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 03/10] Migrate -spendzeroconfchange and -signer settings from QSettings to settings.json --- src/qt/optionsmodel.cpp | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index b42dbf40df8..ba5c5e03e99 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -19,6 +19,7 @@ #include // for -dbcache defaults #include #include // For DEFAULT_SCRIPTCHECK_THREADS +#include // For DEFAULT_SPEND_ZEROCONF_CHANGE #include #include @@ -38,6 +39,8 @@ 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"; default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option)); } } @@ -125,7 +128,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}) { + for (OptionID option : {DatabaseCache, ThreadsScriptVerif, SpendZeroConfChange, ExternalSignerPath}) { std::string setting = SettingName(option); if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting); try { @@ -155,18 +158,6 @@ bool OptionsModel::Init(bilingual_str& error) // 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); } @@ -430,9 +421,9 @@ QVariant OptionsModel::getOption(OptionID option) const #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 @@ -551,14 +542,14 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value) #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; @@ -703,4 +694,8 @@ void OptionsModel::checkAndMigrate() migrate_setting(DatabaseCache, "nDatabaseCache"); migrate_setting(ThreadsScriptVerif, "nThreadsScriptVerif"); +#ifdef ENABLE_WALLET + migrate_setting(SpendZeroConfChange, "bSpendZeroConfChange"); + migrate_setting(ExternalSignerPath, "external_signer_path"); +#endif } From d2ada6e63583cad91e92b49c4dbf8c7ff086a758 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 18 Apr 2022 19:19:02 -0400 Subject: [PATCH 04/10] Migrate -upnp and -natpmp settings from QSettings to settings.json This also effectively reverts 58e8364dcdc4e57b0caac09f8402e6535301de9b 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 https://github.com/bitcoin/bitcoin/pull/18077#discussion_r560381734) --- src/qt/optionsdialog.cpp | 5 ----- src/qt/optionsmodel.cpp | 33 ++++++++++++++++----------------- src/qt/test/optiontests.cpp | 2 ++ 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp index e6ff43a3792..f3c3af10e08 100644 --- a/src/qt/optionsdialog.cpp +++ b/src/qt/optionsdialog.cpp @@ -26,7 +26,6 @@ #include #include #include -#include #include #include @@ -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 ba5c5e03e99..52327dd9f93 100644 --- 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 { @@ -165,18 +168,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()}; @@ -390,13 +381,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 @@ -478,10 +469,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(); @@ -698,4 +695,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 index fa3fe031ecd..d84dc524cdf 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -34,6 +34,7 @@ void OptionTests::migrateSettings() QSettings settings; settings.setValue("nDatabaseCache", 600); settings.setValue("nThreadsScriptVerif", 12); + settings.setValue("fUseUPnP", false); settings.sync(); @@ -42,6 +43,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(file), std::istreambuf_iterator()).c_str(), "{\n" From a09e3b7cf29c3b1fd320badbed32275e0aa83cda Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 05/10] Migrate -listen and -server settings from QSettings to settings.json --- src/qt/optionsmodel.cpp | 57 +++++++++++-------------------------- src/qt/test/optiontests.cpp | 3 ++ 2 files changed, 19 insertions(+), 41 deletions(-) diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 52327dd9f93..82da3415161 100644 --- a/src/qt/optionsmodel.cpp +++ b/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 { @@ -168,37 +170,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); @@ -439,9 +410,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(); } @@ -610,14 +581,9 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value) } 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; @@ -697,4 +663,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(); } diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index d84dc524cdf..57b462c50b7 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -35,6 +35,7 @@ void OptionTests::migrateSettings() settings.setValue("nDatabaseCache", 600); settings.setValue("nThreadsScriptVerif", 12); settings.setValue("fUseUPnP", false); + settings.setValue("fListen", false); settings.sync(); @@ -44,10 +45,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(file), std::istreambuf_iterator()).c_str(), "{\n" " \"dbcache\": \"600\",\n" + " \"listen\": false,\n" " \"par\": \"12\"\n" "}\n"); } From f067e1943361b7bfa78a423528759e9edffa7482 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 06/10] Migrate -proxy and -onion settings from QSettings to settings.json --- src/qt/optionsmodel.cpp | 167 +++++++++++++++++++----------------- src/qt/optionsmodel.h | 8 ++ src/qt/test/optiontests.cpp | 12 ++- 3 files changed, 109 insertions(+), 78 deletions(-) diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 82da3415161..238727e6697 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -45,6 +45,12 @@ static const char* SettingName(OptionsModel::OptionID option) case OptionsModel::MapPortNatpmp: return "natpmp"; case OptionsModel::Listen: return "listen"; case OptionsModel::Server: return "server"; + 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"; default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option)); } } @@ -68,6 +74,14 @@ static void UpdateRwSetting(interfaces::Node& node, OptionsModel::OptionID optio } } +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} { @@ -81,6 +95,14 @@ void OptionsModel::addOverriddenOption(const std::string &option) // Writes all missing QSettings with their default values bool OptionsModel::Init(bilingual_str& error) { + // Initialize display settings from stored settings. + 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; + checkAndMigrate(); QSettings settings; @@ -133,7 +155,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, Listen, Server}) { + MapPortNatpmp, Listen, Server, ProxyUse, ProxyUseTor}) { std::string setting = SettingName(option); if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting); try { @@ -169,28 +191,6 @@ bool OptionsModel::Init(bilingual_str& error) m_sub_fee_from_amount = settings.value("SubFeeFromAmount", false).toBool(); #endif - // Network - - 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", ""); @@ -257,21 +257,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 @@ -279,9 +273,14 @@ 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 std::string ProxyString(bool is_set, QString ip, QString port) +{ + return is_set ? QString(ip + ":" + port).toStdString() : ""; } static const QString GetDefaultProxyAddress() @@ -367,19 +366,19 @@ 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: @@ -458,55 +457,55 @@ 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: @@ -650,7 +649,17 @@ void OptionsModel::checkAndMigrate() if (!settings.contains(qt_name)) return; QVariant value = settings.value(qt_name); if (node().getPersistentSetting(SettingName(option)).isNull()) { - setOption(option, value); + 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); }; @@ -665,6 +674,10 @@ void OptionsModel::checkAndMigrate() migrate_setting(MapPortNatpmp, "fUseNatpmp"); migrate_setting(Listen, "fListen"); migrate_setting(Server, "server"); + migrate_setting(ProxyIP, "addrProxy"); + migrate_setting(ProxyUse, "fUseProxy"); + migrate_setting(ProxyIPTor, "addrSeparateProxyTor"); + migrate_setting(ProxyUseTor, "fUseSeparateProxyTor"); // In case migrating QSettings caused any settings value to change, rerun // parameter interaction code to update other settings. This is particularly diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h index 9f8d17b305c..ca07a268e8e 100644 --- a/src/qt/optionsmodel.h +++ b/src/qt/optionsmodel.h @@ -122,6 +122,14 @@ private: 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. + 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; diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index 57b462c50b7..443c9f2a93b 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -36,6 +36,10 @@ void OptionTests::migrateSettings() settings.setValue("nThreadsScriptVerif", 12); settings.setValue("fUseUPnP", false); settings.setValue("fListen", false); + settings.setValue("fUseProxy", true); + settings.setValue("addrProxy", "proxy:123"); + settings.setValue("fUseSeparateProxyTor", true); + settings.setValue("addrSeparateProxyTor", "onion:234"); settings.sync(); @@ -46,12 +50,18 @@ void OptionTests::migrateSettings() QVERIFY(!settings.contains("nThreadsScriptVerif")); QVERIFY(!settings.contains("fUseUPnP")); QVERIFY(!settings.contains("fListen")); + 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(file), std::istreambuf_iterator()).c_str(), "{\n" " \"dbcache\": \"600\",\n" " \"listen\": false,\n" - " \"par\": \"12\"\n" + " \"onion\": \"onion:234\",\n" + " \"par\": \"12\",\n" + " \"proxy\": \"proxy:123\"\n" "}\n"); } From 9a016a3c07d4becf0651ef58c7160180c5f25a0c Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 07/10] Migrate -prune setting from QSettings to settings.json --- src/qt/bitcoin.cpp | 2 +- src/qt/optionsmodel.cpp | 101 ++++++++++++++++++++++++------------ src/qt/optionsmodel.h | 4 +- src/qt/test/optiontests.cpp | 7 ++- 4 files changed, 76 insertions(+), 38 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index a7003c95ab7..bd6a1f5ecaa 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -344,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() diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 238727e6697..39e79a2a53a 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -45,6 +45,8 @@ static const char* SettingName(OptionsModel::OptionID option) 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"; @@ -60,7 +62,9 @@ static void UpdateRwSetting(interfaces::Node& node, OptionsModel::OptionID optio { if (value.isNum() && (option == OptionsModel::DatabaseCache || - option == OptionsModel::ThreadsScriptVerif)) { + 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 @@ -74,6 +78,36 @@ static void UpdateRwSetting(interfaces::Node& node, OptionsModel::OptionID optio } } +//! 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; @@ -96,6 +130,7 @@ 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; @@ -155,7 +190,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, Listen, Server, ProxyUse, ProxyUseTor}) { + MapPortNatpmp, Listen, Server, Prune, ProxyUse, ProxyUseTor}) { std::string setting = SettingName(option); if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting); try { @@ -175,11 +210,6 @@ bool OptionsModel::Init(bilingual_str& error) // 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("strDataDir")) settings.setValue("strDataDir", GUIUtil::getDefaultDataDirectory()); @@ -288,29 +318,27 @@ static const QString GetDefaultProxyAddress() return QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST).arg(DEFAULT_GUI_PROXY_PORT); } -void OptionsModel::SetPruneEnabled(bool prune, bool force) +void OptionsModel::SetPruneTargetGB(int prune_target_gb) { - 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"); - } -} + const util::SettingsValue cur_value = node().getPersistentSetting("prune"); + const util::SettingsValue new_value = PruneSetting(prune_target_gb > 0, prune_target_gb); -void OptionsModel::SetPruneTargetGB(int prune_target_gb, bool force) -{ - const bool prune = prune_target_gb > 0; - if (prune) { - QSettings settings; - settings.setValue("nPruneSize", 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 @@ -401,9 +429,9 @@ 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 qlonglong(SettingToInt(setting(), nDefaultDbCache)); case ThreadsScriptVerif: @@ -556,15 +584,18 @@ 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: @@ -674,6 +705,8 @@ void OptionsModel::checkAndMigrate() 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"); diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h index ca07a268e8e..42b89c50293 100644 --- a/src/qt/optionsmodel.h +++ b/src/qt/optionsmodel.h @@ -99,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); @@ -125,6 +124,7 @@ private: //! 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; diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index 443c9f2a93b..3bd0af19ada 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -36,6 +36,8 @@ void OptionTests::migrateSettings() 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); @@ -50,6 +52,8 @@ void OptionTests::migrateSettings() 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")); @@ -61,7 +65,8 @@ void OptionTests::migrateSettings() " \"listen\": false,\n" " \"onion\": \"onion:234\",\n" " \"par\": \"12\",\n" - " \"proxy\": \"proxy:123\"\n" + " \"proxy\": \"proxy:123\",\n" + " \"prune\": \"2861\"\n" "}\n"); } From 504b06b1dec9d9329c83b13c7c36ca710ebcd349 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 08/10] Migrate -lang setting from QSettings to settings.json --- src/qt/forms/optionsdialog.ui | 2 +- src/qt/optionsmodel.cpp | 21 +++++++-------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/qt/forms/optionsdialog.ui b/src/qt/forms/optionsdialog.ui index 5438811aff7..582f02132a8 100644 --- a/src/qt/forms/optionsdialog.ui +++ b/src/qt/forms/optionsdialog.ui @@ -896,7 +896,7 @@ - Options set in this dialog are overridden by the command line or in the configuration file: + Options set in this dialog are overridden by the command line: Qt::PlainText diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 39e79a2a53a..1b2bcefe94e 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -53,6 +53,7 @@ static const char* SettingName(OptionsModel::OptionID option) 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)); } } @@ -137,6 +138,7 @@ bool OptionsModel::Init(bilingual_str& error) 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(); @@ -190,7 +192,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, Listen, Server, Prune, ProxyUse, ProxyUseTor}) { + MapPortNatpmp, Listen, Server, Prune, ProxyUse, ProxyUseTor, Language}) { std::string setting = SettingName(option); if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting); try { @@ -205,9 +207,6 @@ bool OptionsModel::Init(bilingual_str& error) } // 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("strDataDir")) @@ -222,13 +221,6 @@ bool OptionsModel::Init(bilingual_str& error) #endif // 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"); } @@ -421,7 +413,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: @@ -564,8 +556,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; @@ -711,6 +703,7 @@ void OptionsModel::checkAndMigrate() 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 From 99ccc02b652cf67a3aec66371fcb6bbe737571a7 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 09/10] Add release notes about unified bitcoin-qt and bitcoind persistent settings If a bitcoind 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 and shared with bitcoind, instead of being stored as Qt settings backed by the windows registry or platform specific config files. --- doc/release-notes-15936.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 doc/release-notes-15936.md diff --git a/doc/release-notes-15936.md b/doc/release-notes-15936.md new file mode 100644 index 00000000000..90c0413b9a8 --- /dev/null +++ b/doc/release-notes-15936.md @@ -0,0 +1,15 @@ +GUI changes +----------- + +Configuration changes made in the bitcoin GUI (such as the pruning setting, +proxy settings, UPNP preferences) are now saved to `/settings.json` +file rather than to the Qt settings backend (windows registry or unix desktop +config files), so these settings will now apply to bitcoind, instead of being +ignored. + +Also, the interaction between GUI settings and `bitcoin.conf` settings is +simplified. Settings from `bitcoin.conf` are now displayed normally in the GUI +settings dialog, instead of in a separate warning message ("Options set in this +dialog are overridden by the configuration file: -setting=value"). And these +settings can now be edited because `settings.json` values take precedence over +`bitcoin.conf` values. From e47c6c76561807d30cff3c2e5372ea83c91a3677 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 16 May 2022 14:37:00 -0400 Subject: [PATCH 10/10] Reset settings.json when GUI options are reset Clear settings.json file and save settings.json.bak file when "Reset Options" GUI button is pressed or -resetguisettings command line option is used. --- src/qt/optionsmodel.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 1b2bcefe94e..0b4359a9179 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -251,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