[qt] move QSettings to bitcoin_rw.conf where possible

Sjors commented at 7:01 pm on March 29, 2018: member

When QT launches, move the following QSettings to bitcoin_rw.conf (added in #11082):

-lang (language) -dbcache (nDatabaseCache) -par (nThreadsScriptVerif) -prune (bPrune,bPruneSize) -spendzeroconfchange (bSpendZeroConfChange) -upnp (fUseUPnP) -listen (fListen) -proxy (fUseProxy) -onion (fUseSeparateProxyTor)

~This is a step towards being able to configure prune from the GUI (#6461). ~ (already done )

There’s potentially a safety benefit in storing listen, proxy and onion in bitcoin_rw.conf in case the user ever decides to run bitcoind.

laanwj commented at 8:25 pm on March 29, 2018: member

Please, never write to bitcoin.conf. It is exceptionally bad form for a daemon to write to its configuration file. If you want to do this, I’d suggest adding an additional file (in the datadir) for read/write settings (as @luke-jr did AFAIK).

laanwj added the label GUI on Mar 29, 2018

Sjors commented at 8:49 am on March 30, 2018: member

@laanwj two files is indeed what @luke-jr did, but I don’t see how that changes anything. It’s critical that bitcoind uses this second file (for settings like prune) and treats it the same as bitcoin.conf. So by the same reasoning it shouldn’t write to that second file.

Also note that only QT needs this ability, so I could refactor it such that bitcoind daemon only has read-only access.

I thought of a completely different approach to handle the prune case, so we might be able to kick this shared settings can down the road. Will open a PR for later.

laanwj commented at 10:24 am on March 30, 2018: member

@laanwj two files is indeed what @luke-jr did, but I don’t see how that changes anything.

One thing is permissions. The data directory is guaranteed writable. The configuration file might be -conf=/etc/bitcoind.conf.

Also note that only QT needs this ability, so I could refactor it such that bitcoind daemon only has read-only access.

Unlike QSettings the second file would be used by both the GUI and the daemon.

And it might be useful to edit settings in bitcoind at real-time as well. The GUI shouldn’t really be special there, that gets in the way of the whole idea of unifying the settings mechanism.

Sjors commented at 3:03 pm on March 30, 2018: member

If bitcoin.conf is not writeable, because the user configured a custom read-only directory, the UI elements that change settings this way could be disabled.

And it might be useful to edit settings in bitcoind at real-time as well.

Indeed.

I thought having two separate files leads to a more complicated implementation than just having one, but I didn’t remove that much from the original code so far. It helps that settings can be overridden by repeating them in the new file.

So in that case it might more sense to get #11082 merged, in which case this PR just adds QSettings migration code on top of that.

Sjors force-pushed on Apr 3, 2018

Sjors renamed this:
~~WIP [qt] move QSettings to bitcoin.conf where possible~~
[qt] move QSettings to bitcoin.conf where possible
on Apr 3, 2018

Sjors commented at 4:08 pm on April 3, 2018: member

I pushed a new version that uses bitcoin_rw.conf. I could probably add some helper functions to make the change more compact.

jonasschnelli commented at 12:24 pm on April 4, 2018: contributor

I’m not sure if moving the QT settings from pure QT format in the QT chosen file to the bitcoind compatible bitcoin_rw.conf format reduces the complexity. Configuration file complexity can be harmful (-datadir, eventually once config files per network, includes, etc.).

I also think we could expose prune to the QT layer without sharing the configuration value with bitcoind.

Users using bitcoind & QT with the same settings and datadir do probably know the ramification of GUI only settings. A warning when enabling prune in QT (once implemented) that it will not automatically be enabled in daemon mode seems sufficient to me.

Sjors commented at 1:06 pm on April 4, 2018: member

@jonasschnelli in addition to a warning, we can even make bitcoind actually handle QT’s prune elegantly, see my suggestion here.

For settings other than prune there might be similar workarounds on a case by case basis (other than warning).

Sjors commented at 3:03 pm on April 17, 2018: member

This and #11082 need a likely non-trivial rebase after #11862. I’m going to try the other approach first though.

Sjors commented at 11:09 am on May 15, 2018: member

#11862 has been merged. I’ll rebase this once #11082 is rebased.

QT doesn’t have any concept (yet) of network specific configuration though.

MarcoFalke added the label Needs rebase on Jun 6, 2018

ryanofsky commented at 2:05 am on October 6, 2018: member

I like this approach. I think it’d be a simplification to stop using the windows registry and other config backends in bitcoin-qt. And it seems like it would be less surprising and error prone if bitcoin-qt and bitcoind used the same settings, instead of slightly different but overlapping sources of settings like they do now, so I don’t get the objection in #12833 (comment).

One thing I don’t understand about the implementation though is why the “Active command-line options that override above options” display is removed. Will command line settings not override config file settings anymore with this change? I think it would be bad if that were the case.

Sjors commented at 1:46 am on October 9, 2018: member

@ryanofsky wrote:

Will command line settings not override config file settings anymore with this change?

They will. The phrase “active command-line options” actually referred to both bitcoin.conf and command line options, either of which can override QT settings, as well as each other.

Afaik QT can’t tell the difference between settings that came from bitcoin.conf and those that came from a command line option, which is why I removed that text. It’s also pretty difficult in practice to launch QT with command line options, involving editing desktop shortcuts etc.

Sjors renamed this:
~~[qt] move QSettings to bitcoin.conf where possible~~
[qt] move QSettings to bitcoin_rw.conf where possible
on Nov 10, 2018

Sjors force-pushed on Nov 10, 2018

Sjors commented at 12:11 pm on November 10, 2018: member

Rebased and added pruning to the list of imported settings.

I changed the meaning of disabling prune in the GUI to mean stop further pruning, instead undo previous pruning. It just sets prune=1, which avoids forcing the user to re-download the chain. We could later add a button to re-download the whole chain.

DrahtBot removed the label Needs rebase on Nov 10, 2018

in src/qt/optionsmodel.cpp:410 in decad6aa96 outdated

408+            } else {
409+              // When user disables pruning, set prune=1 and store last used prune
410+              // size in settings.
411+              nPruneSizeMB = gArgs.GetArg("-prune", (qint64)nDefaultDbCache);
412+              settings.setValue("nPruneSize", nPruneSizeMB / 1000);
413+              // TODO: check if pruning actually occured, otherwise prune=0 is safe too

practicalswift commented at 12:23 pm on November 11, 2018:

“Occurred” :-)

in src/qt/optionsmodel.cpp:403 in decad6aa96 outdated

401-                settings.setValue("bPrune", value);
402-                setRestartRequired(true);
403+            if (value.toBool()) {
404+              // When user enables pruning, store the prune size:
405+              nPruneSizeMB = settings.value("nPruneSize").toInt() * 1000;
406+              gArgs.ModifyRWConfigFile("prune",std::to_string(nPruneSizeMB));

practicalswift commented at 12:25 pm on November 11, 2018:

Nit: Missing whitespace after , :-)

in src/util/system.cpp:1042 in decad6aa96 outdated

1037+        current_char = stream.get();
1038+        if (current_char == std::char_traits<char>::eof()) {
1039+            return false;
1040+        }
1041+        result.clear();
1042+        result.push_back(char(current_char));

practicalswift commented at 12:27 pm on November 11, 2018:

Nit: I believe (char)current_char is the preferred form for casting the fundamental types in the project :-)

Sjors commented at 7:24 pm on January 15, 2019:

That’s upstream: #11082

in src/util/system.cpp:1048 in decad6aa96 outdated

1043+        while (current_char != '\n') {
1044+            current_char = stream.get();
1045+            if (current_char == std::char_traits<char>::eof()) {
1046+                break;
1047+            }
1048+            result.push_back(char(current_char));

practicalswift commented at 12:27 pm on November 11, 2018:

Same here :-)

in src/qt/optionsmodel.cpp:187 in decad6aa96 outdated

170@@ -243,6 +171,7 @@ static const QString GetDefaultProxyAddress()
171 // read QSettings values and return them
172 QVariant OptionsModel::data(const QModelIndex & index, int role) const
173 {
174+    int64_t prune = -1;

practicalswift commented at 12:34 pm on November 11, 2018:

The scope of prune can be limited to the scope of case Prune and case PruneSize below :-)

Sjors commented at 7:28 pm on January 15, 2019:

That results in error: redefinition of 'prune'.

in src/qt/optionsmodel.cpp:286 in decad6aa96 outdated

282@@ -340,8 +283,16 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
283 
284         // default proxy
285         case ProxyUse:
286-            if (settings.value("fUseProxy") != value) {
287-                settings.setValue("fUseProxy", value.toBool());
288+            if (value.toBool() ^ gArgs.GetArg("-proxy", "") != "") {

practicalswift commented at 12:36 pm on November 11, 2018:

Add parentheses to make precedence obvious :-)

in src/qt/optionsmodel.cpp:328 in decad6aa96 outdated

325 
326         // separate Tor proxy
327         case ProxyUseTor:
328-            if (settings.value("fUseSeparateProxyTor") != value) {
329-                settings.setValue("fUseSeparateProxyTor", value.toBool());
330+            if (value.toBool() ^ gArgs.GetArg("-onion", "") != "") {

practicalswift commented at 12:36 pm on November 11, 2018:

Same here :-)

in src/qt/optionsmodel.cpp:432 in decad6aa96 outdated

434             }
435             break;
436         case DatabaseCache:
437-            if (settings.value("nDatabaseCache") != value) {
438-                settings.setValue("nDatabaseCache", value);
439+            if ((int64_t)gArgs.GetArg("-dbcache", (qint64)nDefaultDbCache) != value) {

practicalswift commented at 12:37 pm on November 11, 2018:

Is this cast to int64_t needed?

in src/qt/optionsmodel.cpp:439 in decad6aa96 outdated

443             }
444             break;
445         case ThreadsScriptVerif:
446-            if (settings.value("nThreadsScriptVerif") != value) {
447-                settings.setValue("nThreadsScriptVerif", value);
448+            if ((int64_t)gArgs.GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS) != value) {

practicalswift commented at 12:37 pm on November 11, 2018:

Is this cast to int64_t needed?

DrahtBot commented at 8:03 pm on November 13, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16673 (Relog configuration args on debug.log rotation by LarryRuane)
#15934 (Merge settings one place instead of five places by ryanofsky)
#15454 (Remove the automatic creation and loading of the default wallet by achow101)
#14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
#13818 (More intuitive GUI settings behavior when -proxy is set by Sjors)
#13339 (wallet: Replace %w by wallet name in -walletnotify script by promag)

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.

DrahtBot added the label Needs rebase on Nov 19, 2018

Sjors commented at 1:46 pm on November 20, 2018: member

I’ll rebase and fix code issues after the next upstream PR rebase.

Sjors force-pushed on Jan 15, 2019

Sjors force-pushed on Feb 12, 2019

DrahtBot removed the label Needs rebase on Feb 12, 2019

DrahtBot added the label Needs rebase on Feb 21, 2019

DrahtBot added the label Up for grabs on Sep 30, 2019

DrahtBot commented at 1:02 pm on September 30, 2019: member

DrahtBot closed this on Sep 30, 2019

ryanofsky commented at 2:04 pm on September 30, 2019: member

Interesting! Apparently I commented on this PR and then forgot it existed, because I reimplemented the same thing six months later in #15936

meshcollider reopened this on Oct 14, 2019

Sjors force-pushed on Oct 15, 2019

DrahtBot removed the label Needs rebase on Oct 15, 2019

DrahtBot added the label Needs rebase on Oct 16, 2019

MarcoFalke removed the label Up for grabs on Oct 16, 2019

util: Support prepending configs in ReadConfigStream 11dbd1a465

util: SelectBaseParams in ReadConfigFiles, before getting final datadir bef8e8eff6

util: ReadConfigFiles flag to make everything network-specific 33df12917f

Add new bitcoin_rw.conf file that is used for settings modified by this software itself 956a76cc85

Add havePruned() to Node interface 31acbd16ec

[gui] settings: remove command line override label

addOverriddenOption becomes an empty function to keep the diff simple, due to the many if statements without brackets. It will be removed in the next commit.

14de2eee8a

Sjors force-pushed on Oct 29, 2019

DrahtBot removed the label Needs rebase on Oct 29, 2019

[wallet] move constants to walletconstants.h b372aa546d

[qt] move QSettings to bitcoin_rw.conf where possible

When QT launches, move the following QSettings to
bitcoin_rw.conf:

-lang (language)
-dbcache (nDatabaseCache)
-par (nThreadsScriptVerif)
-prune (bPrune,nPruneSize)
-spendzeroconfchange (bSpendZeroConfChange)
-upnp (fUseUPnP)
-listen (fListen)
-proxy (fUseProxy)
-onion (fUseSeparateProxyTor)

Disabling pruning in the GUI now sets prune=1 if the chain is pruned, otherwise prune=0.

3d33e1f334

[doc] clarify control flow in AppTests b9504728d4

Sjors force-pushed on Oct 29, 2019

DrahtBot added the label Needs rebase on Oct 29, 2019

DrahtBot commented at 3:26 pm on October 29, 2019: member

jonasschnelli commented at 7:00 am on May 29, 2020: contributor

Closing for now since this is dependant on a dormant PR #11082

jonasschnelli closed this on May 29, 2020

Sjors commented at 8:57 am on May 29, 2020: member

I’ll nag to reopen this once 11082 is rebased.

Sjors commented at 4:30 pm on June 26, 2020: member

@jonasschnelli 11082 is rebased, can you re-open this?

Rspigler commented at 2:40 am on August 19, 2020: contributor

@Sjors are you still working on this/asking this to be opened, with #15935 being merged and #15936 being discussed?

Sjors commented at 11:05 am on August 22, 2020: member

I have to read up on the alternative approach that was merged. If I build on top of that, I’ll make a new PR. For now, consider this up for grabs.

DrahtBot locked this on Feb 15, 2022

[qt] move QSettings to bitcoin_rw.conf where possible #12833

Conflicts