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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
62+void UpdateSetting(interfaces::Node& node, OptionsModel::OptionID option, const util::SettingsValue& value)
63+{
64+ // Write int settings that Bitcoin 22.x releases try read as strings at
65+ // startup as strings instead of numbers, to prevent uncaught exceptions in
66+ // addOverriddenOption() thrown by UniValue::get_str(). These errors were
67+ // fixed in later releases by https://github.com/bitcoin/bitcoin/pull/24498.
nit: some rewording
0 // Write int settings as strings because Bitcoin 22.x and earlier try to read everything as strings at
1 // startup. This prevents uncaught exceptions in
2 // addOverriddenOption() thrown by UniValue::get_str(). These errors were
3 // fixed in later releases by https://github.com/bitcoin/bitcoin/pull/24498.
re: #602 (review)
nit: some rewording
Thanks, used some of this wording, but some things were not correct, or might imply the wrong thing. AFAIK, this should only affect 22.x releases not 22.x and earlier releases because those releases were not reading the settings.json file. Also the intention is not to write all int settings as strings, only 3 specific int settings because of specific calls:
0addOverriddenOption("-dbcache");
1addOverriddenOption("-par");
2addOverriddenOption("-prune");
in https://github.com/bitcoin/bitcoin/blob/22.x/src/qt/optionsmodel.cpp#L40
Also since 22.x seems to be an active branch I backported the fix to it in https://github.com/bitcoin/bitcoin/pull/25180 in case there is another release. Probably need to keep this workaround anyway, but it might make it easier to get rid of in the future
Rebased db64d21fa932fcf2b6d82117adb86a1059e0c920 -> 60a9db7f58e4ece13bfb607ce914f94e518a2635 (pr/qtsopt.1 -> pr/qtsopt.2, compare) after bitcoin/bitcoin#24830 with no other changes.
bitcoin/bitcoin#24830 doesn’t interact with this PR directly, but @Rspigler mentioned retesting this in https://github.com/bitcoin/bitcoin/pull/15936#issuecomment-1132422382, and testing this PR is annoying without bitcoin/bitcoin#24830
54+ case OptionsModel::ProxyIPTor: return "onion";
55+ case OptionsModel::ProxyPortTor: return "onion";
56+ case OptionsModel::ProxyUseTor: return "onion";
57+ case OptionsModel::Language: return "lang";
58+ default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
59+ };
;
33
34 static const QString GetDefaultProxyAddress();
35
36-OptionsModel::OptionsModel(QObject *parent, bool resetSettings) :
37- QAbstractListModel(parent)
38+const char* SettingName(OptionsModel::OptionID option)
The comment from some of the previous incarnations was useful:
/* Map GUI option ID to node setting name */
SettingName() is only used in optionsmodel.cpp. It should be static.
re: #602 (review)
The comment from some of the previous incarnations was useful:
/* Map GUI option ID to node setting name */
SettingName()is only used inoptionsmodel.cpp. It should bestatic.
Thanks applied both suggestions
57+ case OptionsModel::Language: return "lang";
58+ default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
59+ };
60+}
61+
62+void UpdateSetting(interfaces::Node& node, OptionsModel::OptionID option, const util::SettingsValue& value)
optionsmodel.cpp. Should be static.
ACK 60a9db7f58e4ece13bfb607ce914f94e518a2635
I feel strongly that the vanishing disabled values after restarting should be resolved, but that can be done as a followup, before 24.x.
Thanks for reviewing!
Rebased 60a9db7f58e4ece13bfb607ce914f94e518a2635 -> 9ae2f6ff0f780e34b40768a5d6097704859a7928 (pr/qtsopt.2 -> pr/qtsopt.3, compare) with suggested fixes on top of updated base PRs
re: #602#pullrequestreview-979858810
I feel strongly that the vanishing disabled values after restarting should be resolved, but that can be done as a followup, before 24.x.
Since we talked about multiple solutions for this in #596, I posted #603 as one possible solution. I really do like simplicity of not keeping unused values across restarts. If I set a temporary ssh proxy value one day while I’m using bad wifi, I don’t want that value sticking forever after I disabled the proxy, and confusing me months later or making it more difficult to set up tor. But #596 discusses this issue, #603 is one possible solution, and others solutions should be possible too.
tACK 9ae2f6ff0f780e34b40768a5d6097704859a7928
Now the GUI changes settings.json (instead of just settings.json affecting the GUI; it’s a two way street).
One thing I noticed is, setting prune=x,000 always results in x+1,000. For example, prune=2 -> prune=3 GB.
bitcoin.conf does also now change the GUI, with settings.json taking precedence. bitcoin.conf is never written to by the GUI or settings.json, as discussed here.
Also, main settings dialog has changed appropriately: “Options set in this dialog are overridden by the command line”. (It used to say …“or also in the configuration file”). (It is not overridden by the config file, since it should be matching the config file).
One thing I noticed:
If settings.json is empty, and bitcoin.conf is filled with settings; I open bitcoin-qt and the settings are reflected there. I disable and re-enable those settings, close and open bitcoin-qt, shouldn’t those settings be written in settings.json? They are not.
re: #602 (comment)
One thing I noticed is, setting
prune=x,000always results in x+1,000. For example, prune=2 -> prune=3 GB.
This can happen because -prune setting is specified in MiB (2^20 bytes) while GUI setting is specified in GB (10^9 bytes). So in various cases rounded values will be displayed. Like if you have prune=600 in bitcoin.conf, the gui will display pruning size of 1GB. But as soon as you change settings in the GUI, it will add "prune": 953 (1GB) or 1907 (2GB) or 2861 (3GB), etc to the settings.json file and whatever value is in bitcoin.conf will be ignored. If you edit the settings.json file manually, whatever value you specify will be used, but the GUI options dialog will convert it to GB for display. You should let me know if you see anything more strange than this, but hopefully behavior should be sensible in every case.
If
settings.jsonis empty, andbitcoin.confis filled with settings; I open bitcoin-qt and the settings are reflected there. I disable and re-enable those settings, close and open bitcoin-qt, shouldn’t those settings be written insettings.json? They are not.
This is intentional. Only settings that a user actually changes will be written to settings.json. If all settings were copied to settings.json, then bitcoin.conf would stop having any effect after settings.json was written. Intention is for bitcoin.conf to be a source of settings from a packager or sysadmin, and settings.json to contain customizations from the user. User settings have precedence, but system settings take effect otherwise.
pr/qtsopt.3 -> pr/qtsopt.4, compare) after #600 merge
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 <matiasfurszyfer@protonmail.com>
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)
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.
Clear settings.json file and save settings.json.bak file when "Reset Options"
GUI button is pressed or -resetguisettings command line option is used.
pr/qtsopt.4 -> pr/qtsopt.5, compare) after base PR’s #601 and https://github.com/bitcoin/bitcoin/pull/15936 were merged (no other changes or conflicts)
270+ bilingual_str error;
271+ if (!optionsModel->Init(error)) {
272+ fs::path settings_path;
273+ if (gArgs.GetSettingsPath(&settings_path)) {
274+ error += Untranslated("\n");
275+ error += strprintf(_("Settings file %s might be corrupt or invalid."), fs::quoted(fs::PathToString(settings_path)));
Could
_()be avoided in the GUI code?See #454.
I’m looking at approach taken in #454, but it doesn’t seem to deal with template strings that have parameters. If I need to generalize that approach and use QObject::tr() method to format "Settings file %1 might be corrupt" template for the GUI, and strprintf to format Settings file %s might be corrupt" template for debug.log`, that will be make the code more repetitive and make the translated/original code paths diverge more.
I wonder why _() should be avoided in GUI code if handles bilingual strings well and we want bilingual strings for an init error? Is there a technical reason why _() does not work? Would be be difficult to fix?
I wonder why
_()should be avoided in GUI code if handles bilingual strings well and we want bilingual strings for an init error? Is there a technical reason why_()does not work? Would be be difficult to fix?
The reason is that the qt sub-directory is not parsed by the share/qt/extract_strings_qt.py script, and it results with missed translatable strings.
Besides #454, there were an alternative approach in bitcoin/bitcoin#22764.
re: #602 (review)
Besides #454, there were an alternative approach in bitcoin/bitcoin#22764.
Thanks! I commented on https://github.com/bitcoin/bitcoin/pull/22764, and will try to remove _() from this PR with whatever repetition is needed so this PR is not blocked waiting for that one.
Updated 689dd6588c102ad4dc9e06e7465bd8d3acec273a -> ac5fb0107489f5b04559627bbb4eeee698c7e987 (pr/qtsopt.5 -> pr/qtsopt.6, compare) removing _() from qt code as suggested #602 (review). Maybe will change it back depending how discussion goes in https://github.com/bitcoin/bitcoin/pull/22764#issuecomment-1142564272
Changes look like:
0--- a/src/qt/bitcoin.cpp
1+++ b/src/qt/bitcoin.cpp
2@@ -270,7 +270,9 @@ bool BitcoinApplication::createOptionsModel(bool resetSettings)
3 fs::path settings_path;
4 if (gArgs.GetSettingsPath(&settings_path)) {
5 error += Untranslated("\n");
6- error += strprintf(_("Settings file %s might be corrupt or invalid."), fs::quoted(fs::PathToString(settings_path)));
7+ std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
8+ error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
9+ error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
10 }
11 InitError(error);
12 QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated));
13diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
14index 2c15bfd45e3..d8d39abe6bd 100644
15--- a/src/qt/optionsmodel.cpp
16+++ b/src/qt/optionsmodel.cpp
17@@ -200,7 +200,8 @@ bool OptionsModel::Init(bilingual_str& error)
18 } catch (const std::exception& e) {
19 // This handles exceptions thrown by univalue that can happen if
20 // settings in settings.json don't have the expected types.
21- error = strprintf(_("Could not read setting \"%s\", %s."), setting, e.what());
22+ error.original = strprintf("Could not read setting \"%s\", %s.", setting, e.what());
23+ error.translated = tr("Could not read setting \"%1\", %2.").arg(QString::fromStdString(setting), e.what()).toStdString();
24 return false;
25 }
26 }
273+ if (gArgs.GetSettingsPath(&settings_path)) {
274+ error += Untranslated("\n");
275+ std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
276+ error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
277+ error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
278+ }
What are expected scenarios which run this code?
Asking because settings.json file I/O errors are handled in the InitSettings() function.
re: #602 (review)
What are expected scenarios which run this code?
Asking because
settings.jsonfile I/O errors are handled in theInitSettings()function.
This is triggered if invalid data was written to the setting.json file (from a bug or from the user editing the file). SettingTo{Bool,Int,String} functions can throw exceptions, OptionsModel::Init function will catch them and turn them into an error string, and this code log the error and show an error dialog. OptionsModel::Init right now basically just returns false if an array or object value was found where a normal value was expected, but it could return other errors in other cases as code changes.
198+ if (node().isSettingIgnored(setting)) addOverriddenOption("-" + setting);
199+ try {
200+ getOption(option);
201+ } catch (const std::exception& e) {
202+ // This handles exceptions thrown by univalue that can happen if
203+ // settings in settings.json don't have the expected types.
Should the following settings.json
0{
1 "dbcache": "qwerty"
2}
throw an exception?
re: #602 (review)
Should the following
settings.json0{ 1 "dbcache": "qwerty" 2}throw an exception?
Probably not here, but maybe in the future it should raise an init error. https://github.com/bitcoin/bitcoin/pull/16545 tries to add validation flags to make this easier. It is better to handle validation in shared bitcoind code not GUI code specifically, I think. This PR is only trying to add more settings to settings.json file, not change the way the file is interpreted.
46@@ -47,7 +47,7 @@ class BitcoinApplication: public QApplication
47 /// parameter interaction/setup based on rules
48 void parameterSetup();
49 /// Create options model
50- void createOptionsModel(bool resetSettings);
51+ bool createOptionsModel(bool resetSettings);
nit
0 [[nodiscard]] bool createOptionsModel(bool resetSettings);
with this patch:
0--- a/src/qt/test/apptests.cpp
1+++ b/src/qt/test/apptests.cpp
2@@ -72,7 +72,7 @@ void AppTests::appTests()
3
4 qRegisterMetaType<interfaces::BlockAndHeaderTipInfo>("interfaces::BlockAndHeaderTipInfo");
5 m_app.parameterSetup();
6- m_app.createOptionsModel(true /* reset settings */);
7+ QVERIFY(m_app.createOptionsModel(true /* reset settings */));
8 QScopedPointer<const NetworkStyle> style(NetworkStyle::instantiate(Params().NetworkIDString()));
9 m_app.setupPlatformStyle();
10 m_app.createWindow(style.data());
re: #602 (review)
Thanks, added nodiscard
pr/qtsopt.6 -> pr/qtsopt.7, compare), just adding nodiscard
275+ std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
276+ error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
277+ error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
278+ }
279+ InitError(error);
280+ QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated));
In 6afa080c:
This is going to open two dialogs, one after the other. Is that intended?
First one:
InitError signals ThreadSafeMessageBox which is captured by BitcoinGUI::message function, which opens an error dialog.
Second one:
QMessageBox::critical opens a critical message box with the given text in a standalone window.
re: #602 (review)
This wouldn’t be intended, but I don’t think this problem happens in practice, or at least I can’t reproduce it. I can trigger the error with settings.json file:
0{
1 "dbcache": [
2 ]
3}
In this case, InitError just logs an error without creating a message box because app.createWindow has not been called yet. After that the QMessageBox::critical call actually shows a message box.
There is a bunch of other in code the src/qt/bitcoin.cpp file following the pattern of calling InitError then QMessageBox::critical. It might be something that can be simplified in the future, but I think it is all correct and not buggy.
22+
23+void OptionTests::resetArgs()
24+{
25+ gArgs.LockSettings([&](util::Settings& s) { s = m_previous_settings; });
26+ gArgs.ClearPathCache();
27+}
In 6afa080c:
nit: could use Qt framework’s function init() for this.
It’s a private slot that Qt calls before running each function (example: https://github.com/furszy/bitcoin-core/commit/5b63a5c63cb5a74bd19eb6e8ef2a24ff2a8df8e9)
re: #602 (review)
nit: could use Qt framework’s function
init()for this
Great suggestion! Added your patch
18@@ -19,6 +19,7 @@
19 #include <txdb.h> // for -dbcache defaults
20 #include <util/string.h>
21 #include <validation.h> // For DEFAULT_SCRIPTCHECK_THREADS
22+#include <wallet/wallet.h> // For DEFAULT_SPEND_ZEROCONF_CHANGE
Not for this PR but better to make the options model call getter functions that return these default values from the node and wallet interfaces instead of accessing them through direct includes.
I think that it’s going against the multi-processes separation goal.
re: #602 (review)
Not for this PR but I would rather make the options model call getter functions that return these default values from the node and wallet interfaces instead of accessing them through direct includes.
I think that it’s going against the multi-processes separation goal.
I think the PR helps with this goal by at least getting node and gui to read settings from the same sources, but reading settings still requires dealing with unset settings and knowing default values. If settings API were better it would know about default values, but at least having a set of shared constants prevents values from getting out of sync.
yeah for sure, this PR is great as is. I was thinking more broadly.
GUI architecture wise, I think that only the interfaces should have access to validation.h and wallet.h. And going further, ideally only wallet model (one per active wallet) should have access to the wallet interface.
592- if (settings.value("server") != value) {
593- settings.setValue("server", value);
594+ if (changed()) {
595+ update(value.toBool());
596 setRestartRequired(true);
597 }
in 01f51429:
nit: maybe unify it?
0case Listen:
1case Server:
2 if (changed()) {
3 update(value.toBool());
4 setRestartRequired(true);
5 }
re: #602 (review)
nit: maybe unify it?
Made this consolidation. Some other consolidations are possible too, but it looks like those require changing settings organization, or cause more complicated diffs, or interfere with followup #603, so in these cases I might put off deduping for a followup instead of trying to do it at the same time as other changes.
Code reviewed bc6a5621 (except the prune commit). Left few comments, nothing big, not blocking.
Plus a small point about 01f51429:
The commit message talks about node.args = nullptr removal but that is not happening there.
Thanks for the review!
Updated bc6a56217b6d799c124cd4ec9881f572545fb0e7 -> e47c6c76561807d30cff3c2e5372ea83c91a3677 (pr/qtsopt.7 -> pr/qtsopt.8, compare) with suggested changes
re: #602#pullrequestreview-1001310497
The commit message talks about
node.args = nullptrremoval but that is not happening there.
Thanks, this was obsolete, dropped it
338-{
339- const bool prune = prune_target_gb > 0;
340- if (prune) {
341- QSettings settings;
342- settings.setValue("nPruneSize", prune_target_gb);
343+ const util::SettingsValue cur_value = node().getPersistentSetting("prune");
In 9a016a3c:
tiny nit:
Better to try to not use hardcoded strings, we could now do:
auto prune_name = SettingName(Prune);
(same for line 331)
Code review ACK e47c6c76
With two, non-blocking, points for e47c6c76:
When the user presses “Reset Options”, the triggered dialog is not telling the user that a backup file will be created.
Maybe the backup file should have a different name: settings1.json.bak, settings2.json.bak, settings3.json.bak etc. If there is an existing backup? (otherwise the previous file is being overwritten)