interfaces: Expose settings.json methods to GUI #15936

Is this needed, given that no version was released with this setting?

Not strictly needed, but it would be strange to remove this because removing it would treat this setting differently than all the other settings, it causes no harm, it is one line of code, removing it would introduce strange behaviors trying to revert and test this PR, and removing it would introduce a bug in the PR if there is any release before this PR is merged.

nit:

0files), and the GUI settings will now be used if bitcoind is started
1subsequently, rather than ignored.

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

nit:

Thanks, updated

This can be marked as resolved. I cannot do that for some reason even though I opened it.

Is not the author of a suggestion the best one to judge whether it is resolved or not, github ~!(@*(*~!=&/_*

The LocaleIndependentAtoi()’s argument is const std::string&, no need to get the C-string:

0    return LocaleIndependentAtoi<int>(value.get_str());

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

The LocaleIndependentAtoi()’s argument is const std::string&, no need to get the C-string:

Thanks, updated

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

get_str() will throw if the value is not a string (typ != VSTR). I think it is strange to return "" for a boolean false value and throw for a boolean true. Will also throw for integers.

Added comment. In general, I want this function to throw if an setting is saved with the wrong type, because our code should write to settings.json file with proper types, and wrong types are an indication of a corrupt or invalid settings file.

The isNull() and isFalse() cases are special just because ArgsManager treats them as special. Null values are used by ArgsManager to indicate unspecified settings values. False settings are settings that have been specified, but explicitly negated. There are some settings that don’t make sense to negate, which is why we have DISALLOW_NEGATION flag, but the general approach is allow negations, and treat a negated -nolist as [], treat -nostring as "", -noint as 0, -nobool as false.

The ToQString method is used for -proxy -onion and -lang settings and returning empty string for these does make sense when they are negated. For example, if no -lang value was specified you might want to use operating system language for translation, where if -nolang were specified you might want to disable translation.

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

Using QVariant::fromValue instead of QString::fromStdString would make this less clear, I think. Or I am not seeing the benefit. fromValue is a template method that doesn’t have clearly documented behavior, while fromStdString is clearly documented to accept the UTF8 strings that get_str returns, and is already very commonly used in our Qt code.

Ok, leave it as is then.

I mentioned it because this function has a return type QVariant and now it returns QString. This, I guess, implicitly calls the QVariant(const QString &string) contructor. Where is QString::fromStdString() documented? Is the constructor QVariant(const QString &string) documented too? I do not see any comments in Qt headers.

re: #15936 (review)

Where is QString::fromStdString() documented? Is the constructor QVariant(const QString &string) documented too? I do not see any comments in Qt headers.

Thanks, to answer question I think I just googled “fromStdString” and was looking at https://doc.qt.io/qt-5/qstring.html#fromStdString which describes the locale conversion. There also seem to be around ~100 other uses of fromStdString in our codebase, so probably developers will be familiar with it.

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

Is it not possible to use variant.Type() and ditch the second argument to this function QVariant::Type type?

I want to make sure settings are written to settings.json with correct types. I rearranged the functions and expanded the comment to make the design goals more clear.

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

Here is the reverse of the above false->empty-string conversion. This code converts empty string to boolean false. This looks strange. Maybe in some cases we would like to keep the empty string as an empty string?

You’re right this was just strange, and I don’t know what motivated it. This was only used for the ExternalSignerPath option which was a QLineEdit string and the Language option which was QValueComboBox also always set string values. There would no reason in either case to turn empty strings into booleans.

style:

0bool OptionsModel::setData(const QModelIndex& index, const QVariant& value, int role)

In commit “refactor: Add OptionsModel getOption/setOption methods” (87c655ab07b2bddf50bfdc3fcceabd3ef46e436a)

re: #15936 (review)

style:

This line is actually just moving and I didn’t change it. Would prefer not to change it here to avoid interfering with git diff --color-moved

style:

0    bool setOption(OptionID option, const QVariant& value);

In commit “refactor: Add OptionsModel getOption/setOption methods” (87c655ab07b2bddf50bfdc3fcceabd3ef46e436a)

re: #15936 (review)

style:

Thanks, updated

node() would assert if the node is not created. This adds an implicit ordering dependency of these methods in src/qt/bitcoin.cpp:

0    app.createNode(*init);
1    app.createOptionsModel(gArgs.GetBoolArg("-resetguisettings", false));

Not so nice, but I guess it is ok. It will crash immediately if the order if violated, can’t remain unnoticed if somebody bricks it in the future.

PS what about calling createNode() at the beginning of createOptionsModel()? The above is actually the only place where either method is called (ignoring tests).

In commit “refactor: Pass interfaces::Node references to OptionsModel constructor” (ae9a0987317e23d54c72d82787ef9690a2736595)

re: #15936 (review)

node() would assert if the node is not created.

It doesn’t seem like a problem to me that app.createOptionsModel() and app.createSpashScreen() methods will fail if app.createNode() is not called before them. These are just normal function preconditions which are checked by asserts. If it is a problem, it’s not a new one: the existing app.createWindow() and app.baseInitialize() functions both fail in exactly the same way as app.createOptionsModel() does if m_node is null.

If you have ideas about how to clean up this code generally, I’d be happy to review a PR. But I wouldn’t want to make createOptionsModel a special case here different from the other create methods.

Ok, leave it as is for the purposes of this PR.

There was some subtle bug recently caused by some not-obvious dependency during startup, but the difference with this code here is that here we have an assert(), so it will not remain unnoticed.

In commit “refactor: Pass interfaces::Node references to OptionsModel constructor” (ae9a0987317e23d54c72d82787ef9690a2736595)

re: #15936 (review)

I think this comment is not correct anymore? The settings are no more in QSettings, right?

Expanded comment in later commit. GUI-only settings are still stored in QSettings, but shared bitcoind/bitcoin-qt settings will come from ArgsManager after this.

equivalent, but shorter/simpler:

0    m_splash = new SplashScreen(networkStyle);
1    m_splash->setNode(node());

In commit “refactor: Pass interfaces::Node references to OptionsModel constructor” (ae9a0987317e23d54c72d82787ef9690a2736595)

re: #15936 (review)

equivalent, but shorter/simpler:

Thanks! Updated this

style, here and elsewhere, it must be without spaces for some automated tool to pick it up:

0        /*ignore_nonpersistent=*/true, /*get_chain_name=*/false);

In commit “settings: Add update/getPersistent/isIgnored methods” (ea8c5cb67ac2aef7b24a4d5765edaca2fa84207a)

re: #15936 (review)

style, here and elsewhere, it must be without spaces for some automated tool to pick it up:

Thanks, updated new & changed lines

this is the same as:

0    m_prune_size_gb = prune_target_gb;

since new_value is derived from prune_target_gb. I think there is no point to convert the GB (prune_target_gb) to MB (new_value) only to convert back to GB for m_prune_size_gb?

re: #15936 (review)

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

I think there is no point to convert the GB (prune_target_gb) to MB (new_value) only to convert back to GB for m_prune_size_gb?

Nice suggestion, updated

This, in isolation, is (wrongly) ignoring the fact that values in [2, 550) are not allowed (MIN_DISK_SPACE_FOR_BLOCK_FILES – 550 MiB). init.cpp would refuse to start with such a value.

In context, this is only called with values entered in the GUI which is accepting integer GB only, so it is impossible to enter e.g. 100 MiB. Or it could come from settings.json where the value is stored in MiB and the user may have edited it (they are not supposed to). I think it is ok because even if <550 is entered in settings.json the startup would gracefully fail with the proper message from init.cpp.

re: #15936 (review)

This, in isolation, is (wrongly) ignoring the fact that values in [2, 550) are not allowed

This is all technically correct, and I agree with everything you wrote except for the word “wrongly”. I would say this is rightly not depending on details of the pruning implementation, and only commenting on the one special manual pruning case that does need to be handled here. Still would be open to suggestions to clarify this or maybe point to the -prune argument documentation for more background.

nit: use doxygen’s @throws

0//! [@throws](/bitcoin-bitcoin/contributor/throws/) std::runtime_error if it has non-string type like integer, bool, array, or object.

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

nit: use doxygen’s @throws

Thanks, changed.

I was about to suggest to remove the is_set member and use std::optional, but I noticed that none of the callers of ParseProxyString() bothers to check the is_set member, so it is set-but-never-used. Thus it can be removed:

0struct ProxySetting {
1    QString ip;
2    QString port;
3};
4static ProxySetting ParseProxyString(const QString& proxy);

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

I was about to suggest to remove the is_set member and use std::optional, but I noticed that none of the callers of ParseProxyString() bothers to check the is_set member, so it is set-but-never-used. Thus it can be removed.

Nice catch. This struct definition is just moving not changing, but you’re right I wasn’t taking advantage of the is_used member in getOption code, and it is nicer to do that.

If the is_set member is removed, then this becomes:

0    static const ProxySetting default_val = {DEFAULT_GUI_PROXY_HOST, QString("%1").arg(DEFAULT_GUI_PROXY_PORT)};

and also, a few lines below:

0-return {true, ip_port.at(0), ip_port.at(1)};
1+return {ip_port.at(0), ip_port.at(1)};

In commit “Unify bitcoin-qt and bitcoind persistent settings” (8d50aba09faafa2329751a30d5ce6f63afdc52d0)

re: #15936 (review)

If the is_set member is removed, then this becomes:

Thanks, now make better use of the is_set member and ParseProxyString function.

This was ok before because the supplied SettingsValue to GetBoolArg() was always of type string (VSTR) (right?). But now this code may be called with values coming from settings.json and thus it could be number as well which would trigger get_str() to throw.

For example, if "listen": false in settings.json is changed to "listen": 0, then bitcoin-qt crashes at startup:

0$ bitcoin-qt
1Abort trap (core dumped)
2$

This looks easy to overcome:

0-    return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.get_str());
1+    return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.getValStr());

(putting many things in one line makes a diff harder to read)

re: #15936 (review)

This was ok before because the supplied SettingsValue to GetBoolArg() was always of type string (VSTR) (right?). But now this code may be called with values coming from settings.json and thus it could be number as well which would trigger get_str() to throw.

You are right and the problem is even broader than described, because in general settings.json file is supposed to be strongly typed. If a settings.json file ever has string for an int value, or a int for a bool value, or vice versa, etc it means the file is corrupt and bitcoin should not continue running using a configuration that may be buggy or incomplete.

But of course it shouldn’t abort with an uncaught exception either, so I added code to handle this with more clarity.

Related: There are tests for what’s expected to happen when you set an integer in the settings.json file here:

https://github.com/bitcoin/bitcoin/blob/cf0a8b9c4870cc88254a757286140d9632e7b70c/src/test/getarg_tests.cpp#L99-L104

nit: toInt() returns int, thus this would limit dbcache to max 2^31. It is in megabytes, but still, int64_t is used for it elsewhere:

0            node().updateSetting("dbcache", static_cast<int64_t>(value.toLongLong()));

re: #15936 (review)

nit: toInt() returns int, thus this would limit dbcache to max 2^31. It is in megabytes, but still, int64_t is used for it elsewhere:

Good catch. I’ve cleaned up the integer conversions here and elsewhere.

re: #15936 (review)

Why is it necessary to call mapPort() from here? In the previous code it was not called from here.

Added a commit message comment about this, and also removed other mapPort call this is supposed to replace. When this PR was first opened, mapPort() was called here, but 58e8364dcdc4e57b0caac09f8402e6535301de9b from #18077 removed it, adding an external call in OptionsDialog. I think it’s a good thing to move the mapPort() call back here, because for all other options, calling setOption completely applies the setting (either updating state immediately or calling restartRequired) instead of half-applying it.

It is strange to migrate two different options (ProxyIP and ProxyUse) into the same option (proxy). What happens under the hood is that node().updateSetting("proxy", ...) is called 3 times (when it can/should be called 1 time) with the last call relying on m_proxy_ip and m_proxy_port being set already, so the order in which migrate_setting() is called matters. This is not obvious.

This would resolve itself if the “use proxy” GUI option is stored explicitly in settings.json.

re : #15936 (review)

It is strange to migrate two different options (ProxyIP and ProxyUse) into the same option (proxy). What happens under the hood is that node().updateSetting("proxy", ...) is called 3 times (when it can/should be called 1 time) with the last call relying on m_proxy_ip and m_proxy_port being set already, so the order in which migrate_setting() is called matters. This is not obvious.

This would resolve itself if the “use proxy” GUI option is stored explicitly in settings.json.

This is all true, but intentional. The migration code is pretty dumb and inefficient like you described, but it is also brief and self-contained. The PR is intentionally not adding any new settings fields to settings.json that current versions of bitcoind and bitcoin-qt don’t understand, and intentionally not making any behavior changes to bitcoind, only bitcoin-qt. It’s only trying to migrate existing settings values, so node and wallet settings are stored completely within the datadir, and current and new versions of bitcoind and bitcoin-qt all share the same configurations instead of seeing different realities.

I could probably add some comments to the migration code if it’s not clear enough, but I think it’s ok for the code to be inefficient, and even ok for it to have a bit of extra complexity, if that is the tradeoff for keeping the settings format unchanged, and keeping bitcoind unchanged here.

Alright, if this PR settles as it is (to store “use proxy” implicitly), then this is ok IMO. If it is decided to store “use proxy” explicitly then this will be changed anyway.

Can be marked as resolved.

This migration may silently drop configs from .config/Bitcoin/Bitcoin-Qt.conf with unexpected effects. Before this PR, values in Bitcoin-Qt.conf were overriding values from bitcoin.conf. So, for example, if one had:

bitcoin.conf: listen=1 Bitcoin-Qt.conf : fListen=false

then bitcoin-qt would not be listening and “Allow incoming connections” will be unchecked in the GUI (correct).

However, once the migration happens, it would leave the configs in this state:

bitcoin.conf: listen=1 Bitcoin-Qt.conf : (no fListen is present) settings.json: (no listen is present)

which will cause bitcoin-qt to start listening, silently, when it was not listening before.

re: #15936 (review)

This migration may silently drop configs from .config/Bitcoin/Bitcoin-Qt.conf with unexpected effects. Before this PR, values in Bitcoin-Qt.conf were overriding values from bitcoin.conf. So, for example, if one had:

bitcoin.conf: listen=1 Bitcoin-Qt.conf : fListen=false

then bitcoin-qt would not be listening and “Allow incoming connections” will be unchecked in the GUI (correct).

However, once the migration happens, it would leave the configs in this state:

bitcoin.conf: listen=1 Bitcoin-Qt.conf : (no fListen is present) settings.json: (no listen is present)

which will cause bitcoin-qt to start listening, silently, when it was not listening before.

I don’t think this is describing the previous behavior accurately. With listen=1 in bitcoin.conf it should be listening before and after this PR, no matter what is in Bitcoin-Qt.conf , because QSettings have lower priority than bitcoin.conf (now and previously).

If you try setting fListen=false before this PR, you should see that bitcoin is listening, and there is an “Options set in this dialog are overridden by the command line or in the configuration file: -listen=1”

If you try setting fListen=false after this PR, you should see that bitcoin is still listening and the setting is “migrated” by being deleted, since it was already being ignored anyway.

This PR does add the ability for new GUI setting changes to override bitcoin.conf settings, (because by design settings.json has a higher priority than bitcoin.conf), but migration is one-time occurrence, and existing migrated settings are interpreted with the same lower priority before and after this PR.

The goal for this PR, assuming this PR is merged before 0.24, is that if you switch back and forth between 0.23 and 0.24 you should never see settings changed or interpreted differently unless you actually intervene and edit something to change them.

Thanks for the elaborate explanation! I was wrong, sorry for the noise.

Can be marked as resolved.

(continuing the proxy discussion here so that it is in its own thread, not in the main PR comments and so that it can be closed when resolved eventually)

This applies to the options that have a user-input value and an additional checkbox to disable them: prune, proxy and onion. Naturally there are two things per option - the value typed by the user and the boolean checkbox (denoting enabled/disabled).

This PR uses a dummy value in settings.json to denote the “disabled” state: 0 for prune and "" for proxy. The problem with this approach is that this loses the value typed by the user. This is surprising and annoying (having to re-enter it after temporary disabling it). Also, it is not consistent behavior:

• closing the config window and re-opening it => the values are still there, greyed out
• restarting bitcoin-qt => the values are deleted

Firefox for example has a proxy-config and it acts in a consistent way - the values, when disabled, are preserved (greyed out) even after restart. IMO this is less surprising.

I would suggest to save the “use prune”, “use proxy” and “use proxy for tor” as explicit boolean entities in settings.json and preserve the user-entered values in prune, proxy and onion.

Actually, for prune the behavior is even a bit more confusing:

• enter 5 in the GUI config (some arbitrary value)
• disable the checkbox (5 remains in the box, greyed out)
• restart
• enable the checkbox
• surprise! the value is not 5, but 2

re: #15936 (review)

(continuing the proxy discussion here so that it is in its own thread, not in the main PR comments and so that it can be closed when resolved eventually)

I’ll try to implement a patch which implements the previous behavior you seem to like. But honestly, I think storing unused settings values after a restart is just as likely to harm users as to help them. And even if it does help some users I think it make application behavior more opaque and settings format more complicated to keep around unused settings.

I think it’s ok to clear pruning or proxy values after a users disables pruning or proxying and then restarts. I agree with you that if the user frequently toggles pruning or proxying off and on through the GUI, this may be less convenient. But probably in the more common case, when a user is turning pruning or proxying on for the first time, or turning it on after a long interval, the default values provided by the GUI (2GB for pruning and tor for the proxy), are more likely to be useful and actually work than previous values which may be old or completely invalid or never worked. Our proxy setting is different from firefox’s proxy setting because we actually do provide a default working value using tor, where firefox just has a blank default value that would never be worth resetting to.

Also our UI is very simple, and the default values that will be applied when pruning or proxying are enabled are clearly visible, so I feel like your description exaggerates the amount of surprise or confusion they could cause. I’d agree that that it would be bad and confusing behavior to reset the proxy and values to defaults if a user only toggled the settings off and without restarting to apply the settings. But once you’re restarted with pruning or proxying disabled, I don’t think it’s essential to show old pruning and proxy values the next time you want to enable them. Especially since we have good defaults and don’t have a good way of knowing if the old values are currently valid or were ever valid.

I’ll try to implement a patch which implements the previous behavior you seem to like.

Hmm, I shouldn’t be forcing my view here. I could be nuts and totally disconnected from realit:space_invader: Maybe at this point it makes sense to poke other reviewers for opinion. Anyone?

I think it’s ok to clear pruning or proxy values after a users disables pruning or proxying and then restarts.

I think it would be less surprising and more obvious what is going on if we clear the values after the user disables the option. Like, immediately after the click on the checkbox. Then the behavior will be consistent between “close & re-open the config dialog box” and “close & re-open the app”.

To summarize, there are 3 approaches for when an option is disabled (sorted in order of my preference):

1. (this is how it works in master, before this PR) Preserve the value (greyed out) through config dialog re-open and through app restart. To achieve this, store an explicit use_proxy: false in settings.json and keep the proxy address in proxy: "...". This means that a config which bitcoind does not recognize will appear in settings.json which will require a tweak here: https://github.com/bitcoin/bitcoin/blob/b297b945f7610772434817181ad12067b2832565/src/util/system.cpp#L569

2. Delete the value immediately after the checkbox is unchecked. This will make it obvious that the value is gone and will be consistent wrt open/close config dialog and open/close app.

3. (this is how this PR works (99b4ce266f62afc8a37575527d82ac6048f50f75)) Preserve the value (greyed out) through config dialog re-open, but delete it through an app restart.

re: #15936 (review)

I’ll try to implement a patch which implements the previous behavior you seem to like.

Hmm, I shouldn’t be forcing my view here. I could be nuts and totally disconnected from realitspace_invader Maybe at this point it makes sense to poke other reviewers for opinion. Anyone?

As a proof of concept I implemented the behavior you requested here: ~https://github.com/ryanofsky/bitcoin/commit/0f29583a903fd8ffa69abe7ae005acac340faa5c~. UPDATE: 4e86ab5dfc9bf40502aca0f9e3a4690f6f5dfc19 is a better version that incorporates #24830 bugfix.

I don’t think that behavior is nuts but I think the alternative is perfectly fine and each behavior has advantages and disadvantages. For purpose of this PR I’m preferring behavior that has simplest possible implementation, and is completely backwards compatible (i.e, existing bitcoin-qt and bitcoind versions will correctly interpret settings.json files written by new bitcoin-qt versions). As commit https://github.com/ryanofsky/bitcoin/commit/4e86ab5dfc9bf40502aca0f9e3a4690f6f5dfc19 demonstrates, our options for the future are open here. We don’t need to fixate on this one minor behavior. We can start off with the simplest behavior, and change it if there is any desire for more complicated solutions in the future.

I think it would be less surprising and more obvious what is going on if we clear the values after the user disables the option. Like, immediately after the click on the checkbox. Then the behavior will be consistent between “close & re-open the config dialog box” and “close & re-open the app”.

IIUC this will require modifying OptionsDialog as well as OptionsModel, and will make interactions between the classes more involved. It may be also be worse for the user if it clears settings before restart while they are still in use. But again, the differences between all these behaviors are small and I don’t think any actual user will have much reason to care about them. I’m just trying to start off with the simplest possible implementation and mental model.

To achieve this, store an explicit use_proxy: false in settings.json and keep the proxy address in proxy: "...". This means that a config which bitcoind does not recognize will appear in settings.json which will require a tweak

I’m opposed to changing the setting.json format in this way, or any way that will cause previous versions of bitcoin-qt and bitcoind to not interpret settings correctly. Doing this is not necessary to get the behavior you are requesting with the storing previous values (as https://github.com/ryanofsky/bitcoin/commit/4e86ab5dfc9bf40502aca0f9e3a4690f6f5dfc19 demonstrates). If we do want to change the settnngs.json in a backwards incompatible way in the future, I think we can do that, but it’s not a line I want to be cross here in this PR.

re: #15936 (review)

I opened https://github.com/bitcoin-core/gui/issues/596 to continue discussion from this thread and try to resolve this

This diff is now in commit ce530d566f Migrate -prune setting from QSettings to settings.json, but it has to be amended to commit dd3d146f72 Migrate -proxy and -onion settings from QSettings to settings.json

 0--- a/src/qt/optionsmodel.cpp
 1+++ b/src/qt/optionsmodel.cpp
 2@@ -338,21 +364,21 @@ QVariant OptionsModel::getOption(OptionID option) const
 3 #endif // USE_NATPMP
 4     case MinimizeOnClose:
 5         return fMinimizeOnClose;
 6 
 7     // default proxy
 8     case ProxyUse:
 9-        return ParseProxyString(SettingToString(node().getPersistentSetting("proxy"), "")).is_set;
10+        return ParseProxyString(SettingToString(setting(), "")).is_set;
11     case ProxyIP:
12         return m_proxy_ip;
13     case ProxyPort:
14         return m_proxy_port;
15 
16     // separate Tor proxy
17     case ProxyUseTor:
18-        return ParseProxyString(SettingToString(node().getPersistentSetting("onion"), "")).is_set;
19+        return ParseProxyString(SettingToString(setting(), "")).is_set;
20     case ProxyIPTor:
21         return m_onion_ip;
22     case ProxyPortTor:
23         return m_onion_port;
24 
25 #ifdef ENABLE_WALLET
26@@ -430,56 +456,56 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
27         settings.setValue("fMinimizeOnClose", fMinimizeOnClose);
28         break;
29 
30     // default proxy
31     case ProxyUse:
32         if (changed()) {
33-            node().updateSetting("proxy", ProxyString(value.toBool(), m_proxy_ip, m_proxy_port));
34+            update(ProxyString(value.toBool(), m_proxy_ip, m_proxy_port));
35             setRestartRequired(true);
36         }
37         break;
38     case ProxyIP:
39         if (changed()) {
40             m_proxy_ip = value.toString();
41             if (getOption(ProxyUse).toBool()) {
42-                node().updateSetting("proxy", ProxyString(true, m_proxy_ip, m_proxy_port));
43+                update(ProxyString(true, m_proxy_ip, m_proxy_port));
44                 setRestartRequired(true);
45             }
46         }
47         break;
48     case ProxyPort:
49         if (changed()) {
50             m_proxy_port = value.toString();
51             if (getOption(ProxyUse).toBool()) {
52-                node().updateSetting("proxy", ProxyString(true, m_proxy_ip, m_proxy_port));
53+                update(ProxyString(true, m_proxy_ip, m_proxy_port));
54                 setRestartRequired(true);
55             }
56         }
57         break;
58 
59     // separate Tor proxy
60     case ProxyUseTor:
61         if (changed()) {
62-            node().updateSetting("onion", ProxyString(value.toBool(), m_onion_ip, m_onion_port));
63+            update(ProxyString(value.toBool(), m_onion_ip, m_onion_port));
64             setRestartRequired(true);
65         }
66         break;
67     case ProxyIPTor:
68         if (changed()) {
69             m_onion_ip = value.toString();
70             if (getOption(ProxyUseTor).toBool()) {
71-                node().updateSetting("onion", ProxyString(true, m_onion_ip, m_onion_port));
72+                update(ProxyString(true, m_onion_ip, m_onion_port));
73                 setRestartRequired(true);
74             }
75         }
76         break;
77     case ProxyPortTor:
78         if (changed()) {
79             m_onion_port = value.toString();
80             if (getOption(ProxyUseTor).toBool()) {
81-                node().updateSetting("onion", ProxyString(true, m_onion_ip, m_onion_port));
82+                update(ProxyString(true, m_onion_ip, m_onion_port));
83                 setRestartRequired(true);
84             }
85         }
86         break;
87 
88 #ifdef ENABLE_WALLET

re: #15936 (review)

This diff is now in commit ce530d5 Migrate -prune setting from QSettings to settings.json, but it has to be amended to commit dd3d146 Migrate -proxy and -onion settings from QSettings to settings.json diff

Good catch. Now moved changes to the right commit.

Given that m_node is always initialized in the constructor, it can never be nullptr/uninitialized. It is better to make this explicit by changing the type of the member variable from pointer to reference. Here is a patch to amend into f384d4d935 refactor: Pass interfaces::Node references to OptionsModel constructor:

 0diff --git i/src/qt/optionsmodel.cpp w/src/qt/optionsmodel.cpp
 1index a83c61ffcc..0bde4407ca 100644
 2--- i/src/qt/optionsmodel.cpp
 3+++ w/src/qt/optionsmodel.cpp
 4@@ -91,13 +91,13 @@ struct ProxySetting {
 5     QString port;
 6 };
 7 static ProxySetting ParseProxyString(const std::string& proxy);
 8 static std::string ProxyString(bool is_set, QString ip, QString port);
 9 
10 OptionsModel::OptionsModel(interfaces::Node& node, QObject *parent) :
11-    QAbstractListModel(parent), m_node(&node)
12+    QAbstractListModel(parent), m_node{node}
13 {
14 }
15 
16 void OptionsModel::addOverriddenOption(const std::string &option)
17 {
18     strOverriddenByCommandLine += QString::fromStdString(option) + "=" + QString::fromStdString(gArgs.GetArg(option, "")) + " ";
19diff --git i/src/qt/optionsmodel.h w/src/qt/optionsmodel.h
20index 4ee0d77a8e..5a4da8a72d 100644
21--- i/src/qt/optionsmodel.h
22+++ w/src/qt/optionsmodel.h
23@@ -102,16 +102,16 @@ public:
24     void SetPruneTargetGB(int prune_target_gb);
25 
26     /* Restart flag helper */
27     void setRestartRequired(bool fRequired);
28     bool isRestartRequired() const;
29 
30-    interfaces::Node& node() const { assert(m_node); return *m_node; }
31+    interfaces::Node& node() const { return m_node; }
32 
33 private:
34-    interfaces::Node* m_node = nullptr;
35+    interfaces::Node& m_node;
36     /* Qt-only settings */
37     bool m_show_tray_icon;
38     bool fMinimizeToTray;
39     bool fMinimizeOnClose;
40     QString language;
41     BitcoinUnit m_display_bitcoin_unit;

re: #15936 (review)

Given that m_node is always initialized in the constructor, it can never be nullptr/uninitialized. It is better to make this explicit by changing the type of the member variable from pointer to reference. Here is a patch to amend into f384d4d refactor: Pass interfaces::Node references to OptionsModel constructor:

Makes sense. Added this change.

It turns out that now this is the only place where the constructor of SplashScreen is called and it is immediately followed by an unconditional call to setNode(). So it would be better to pass the node to the constructor and ditch the setNode() method. This also allows to make the SplashSceen::m_node member a reference, so that it is obvious that it will always be initialized.

Here is a diff to amend into f384d4d9350f94043518d1cba98f4054cd54b325 refactor: Pass interfaces::Node references to OptionsModel constructor or maybe that deserves a separate commit or maybe even a followup PR:

  0diff --git i/src/qt/bitcoin.cpp w/src/qt/bitcoin.cpp
  1index 324f1c7ce0..ad6a7a129d 100644
  2--- i/src/qt/bitcoin.cpp
  3+++ w/src/qt/bitcoin.cpp
  4@@ -292,14 +292,13 @@ void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
  5     });
  6 }
  7 
  8 void BitcoinApplication::createSplashScreen(const NetworkStyle *networkStyle)
  9 {
 10     assert(!m_splash);
 11-    m_splash = new SplashScreen(networkStyle);
 12-    m_splash->setNode(node());
 13+    m_splash = new SplashScreen(node(), networkStyle);
 14     // We don't hold a direct pointer to the splash screen after creation, but the splash
 15     // screen will take care of deleting itself when finish() happens.
 16     m_splash->show();
 17     connect(this, &BitcoinApplication::splashFinished, m_splash, &SplashScreen::finish);
 18     connect(this, &BitcoinApplication::requestedShutdown, m_splash, &QWidget::close);
 19 }
 20diff --git i/src/qt/splashscreen.cpp w/src/qt/splashscreen.cpp
 21index a4dfffa387..de99e36ad0 100644
 22--- i/src/qt/splashscreen.cpp
 23+++ w/src/qt/splashscreen.cpp
 24@@ -24,14 +24,14 @@
 25 #include <QCloseEvent>
 26 #include <QPainter>
 27 #include <QRadialGradient>
 28 #include <QScreen>
 29 
 30 
 31-SplashScreen::SplashScreen(const NetworkStyle* networkStyle)
 32-    : QWidget(), curAlignment(0)
 33+SplashScreen::SplashScreen(interfaces::Node& node, const NetworkStyle* networkStyle)
 34+    : QWidget(), curAlignment(0), m_node{node}
 35 {
 36     // set reference point, paddings
 37     int paddingRight            = 50;
 38     int paddingTop              = 50;
 39     int titleVersionVSpace      = 17;
 40     int titleCopyrightVSpace    = 40;
 41@@ -127,31 +127,25 @@ SplashScreen::SplashScreen(const NetworkStyle* networkStyle)
 42     setFixedSize(r.size());
 43     move(QGuiApplication::primaryScreen()->geometry().center() - r.center());
 44 
 45     installEventFilter(this);
 46 
 47     GUIUtil::handleCloseWindowShortcut(this);
 48-}
 49 
 50-SplashScreen::~SplashScreen()
 51-{
 52-    if (m_node) unsubscribeFromCoreSignals();
 53+    subscribeToCoreSignals();
 54 }
 55 
 56-void SplashScreen::setNode(interfaces::Node& node)
 57+SplashScreen::~SplashScreen()
 58 {
 59-    assert(!m_node);
 60-    m_node = &node;
 61-    subscribeToCoreSignals();
 62-    if (m_shutdown) m_node->startShutdown();
 63+    unsubscribeFromCoreSignals();
 64 }
 65 
 66 void SplashScreen::shutdown()
 67 {
 68     m_shutdown = true;
 69-    if (m_node) m_node->startShutdown();
 70+    m_node.startShutdown();
 71 }
 72 
 73 bool SplashScreen::eventFilter(QObject * obj, QEvent * ev) {
 74     if (ev->type() == QEvent::KeyPress) {
 75         QKeyEvent *keyEvent = static_cast<QKeyEvent *>(ev);
 76         if (keyEvent->key() == Qt::Key_Q) {
 77@@ -189,22 +183,22 @@ static void ShowProgress(SplashScreen *splash, const std::string &title, int nPr
 78             strprintf("\n%d", nProgress) + "%");
 79 }
 80 
 81 void SplashScreen::subscribeToCoreSignals()
 82 {
 83     // Connect signals to client
 84-    m_handler_init_message = m_node->handleInitMessage(std::bind(InitMessage, this, std::placeholders::_1));
 85-    m_handler_show_progress = m_node->handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3));
 86-    m_handler_init_wallet = m_node->handleInitWallet([this]() { handleLoadWallet(); });
 87+    m_handler_init_message = m_node.handleInitMessage(std::bind(InitMessage, this, std::placeholders::_1));
 88+    m_handler_show_progress = m_node.handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3));
 89+    m_handler_init_wallet = m_node.handleInitWallet([this]() { handleLoadWallet(); });
 90 }
 91 
 92 void SplashScreen::handleLoadWallet()
 93 {
 94 #ifdef ENABLE_WALLET
 95     if (!WalletModel::isWalletEnabled()) return;
 96-    m_handler_load_wallet = m_node->walletLoader().handleLoadWallet([this](std::unique_ptr<interfaces::Wallet> wallet) {
 97+    m_handler_load_wallet = m_node.walletLoader().handleLoadWallet([this](std::unique_ptr<interfaces::Wallet> wallet) {
 98         m_connected_wallet_handlers.emplace_back(wallet->handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2, false)));
 99         m_connected_wallets.emplace_back(std::move(wallet));
100     });
101 #endif
102 }
103 
104diff --git i/src/qt/splashscreen.h w/src/qt/splashscreen.h
105index c14fc521a7..e9879e835b 100644
106--- i/src/qt/splashscreen.h
107+++ w/src/qt/splashscreen.h
108@@ -25,15 +25,14 @@ class Wallet;
109  */
110 class SplashScreen : public QWidget
111 {
112     Q_OBJECT
113 
114 public:
115-    explicit SplashScreen(const NetworkStyle *networkStyle);
116+    explicit SplashScreen(interfaces::Node& node, const NetworkStyle* networkStyle);
117     ~SplashScreen();
118-    void setNode(interfaces::Node& node);
119 
120 protected:
121     void paintEvent(QPaintEvent *event) override;
122     void closeEvent(QCloseEvent *event) override;
123 
124 public Q_SLOTS:
125@@ -59,13 +58,13 @@ private:
126 
127     QPixmap pixmap;
128     QString curMessage;
129     QColor curColor;
130     int curAlignment;
131 
132-    interfaces::Node* m_node = nullptr;
133+    interfaces::Node& m_node;
134     bool m_shutdown = false;
135     std::unique_ptr<interfaces::Handler> m_handler_init_message;
136     std::unique_ptr<interfaces::Handler> m_handler_show_progress;
137     std::unique_ptr<interfaces::Handler> m_handler_init_wallet;
138     std::unique_ptr<interfaces::Handler> m_handler_load_wallet;
139     std::list<std::unique_ptr<interfaces::Wallet>> m_connected_wallets;

re: #15936 (review)

It turns out that now this is the only place where the constructor of SplashScreen is called and it is immediately followed by an unconditional call to setNode(). So it would be better to pass the node to the constructor and ditch the setNode() method. This also allows to make the SplashSceen::m_node member a reference, so that it is obvious that it will always be initialized.

Thanks, applied this change

The only way to throw from getOption() is if SettingName() (called from getOption()) throws. However, SettingName(option) (same argument) is also called just before this try/catch without try/catch. So, if SettingName() is going to throw it will do so before this try/catch and so this code will never be reached.

To trigger that SettingName() to throw the source code must be modified. Cannot be triggered by wrong user input or corrupted config file.

re: #15936 (review)

To trigger that SettingName() to throw the source code must be modified. Cannot be triggered by wrong user input or corrupted config file.

I added a comment to clarify, but this is not the case! The catch is supposed to deal with runtime_error exceptions thrown by univalue, related to the ones you reported #15936 (review). You can trigger it by adding a nonsense line like "upnp": 37 to the settings file.

02ce0badbe3fd50a508b1c189082c91bff420c60, style nit

0    if (!GetSettingsPath(&path, /*temp=*/false, backup) || !GetSettingsPath(&path_tmp, /*temp=*/true, backup)) {