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

Sjors · 2018-03-29T19:01:12Z

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 · 2018-03-29T20:25:55Z

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).

Sjors · 2018-03-30T08:49:34Z

@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 · 2018-03-30T10:24:24Z

@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 · 2018-03-30T15:03:54Z

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 · 2018-04-03T16:08:30Z

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

jonasschnelli · 2018-04-04T12:24:56Z

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 · 2018-04-04T13:06:01Z

@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 · 2018-04-17T15:03:31Z

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

Sjors · 2018-05-15T11:09:45Z

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

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

ryanofsky · 2018-10-06T02:05:44Z

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 · 2018-10-09T01:46:24Z

@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 · 2018-11-10T12:11:41Z

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.

src/qt/optionsmodel.cpp

src/util/system.cpp

practicalswift · 2018-11-11T12:34:04Z

src/qt/optionsmodel.cpp

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


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

That results in error: redefinition of 'prune'.

src/qt/optionsmodel.cpp

DrahtBot · 2018-11-13T20:03:02Z

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.

Sjors · 2018-11-20T13:46:45Z

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

…is software itself

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.

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.

DrahtBot · 2019-10-29T15:26:55Z

Needs rebase

jonasschnelli · 2020-05-29T07:00:20Z

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

Sjors · 2020-05-29T08:57:47Z

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

Sjors · 2020-06-26T16:30:05Z

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

Rspigler · 2020-08-19T02:40:31Z

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

Sjors · 2020-08-22T11:05:51Z

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.

laanwj added the GUI label Mar 29, 2018

Sjors mentioned this pull request Mar 30, 2018

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

Closed

Sjors force-pushed the 2018/03/bitcoin-conf-rw branch 4 times, most recently from 3f4d27f to 518f630 Compare April 3, 2018 16:03

Sjors changed the title ~~WIP [qt] move QSettings to bitcoin.conf where possible~~ [qt] move QSettings to bitcoin.conf where possible Apr 3, 2018

Sjors mentioned this pull request Apr 30, 2018

[qt] OptionsDialog: add prune setting #13043

Merged

maflcko added the Needs rebase label Jun 6, 2018

Sjors mentioned this pull request Jul 31, 2018

More intuitive GUI settings behavior when -proxy is set #13818

Closed

Sjors changed the title ~~[qt] move QSettings to bitcoin.conf where possible~~ [qt] move QSettings to bitcoin_rw.conf where possible Nov 10, 2018

Sjors force-pushed the 2018/03/bitcoin-conf-rw branch from 518f630 to decad6a Compare November 10, 2018 12:11

DrahtBot removed the Needs rebase label Nov 10, 2018

practicalswift reviewed Nov 11, 2018

View reviewed changes

DrahtBot added the Needs rebase label Nov 19, 2018

Sjors force-pushed the 2018/03/bitcoin-conf-rw branch from decad6a to 09e150a Compare January 15, 2019 19:34

meshcollider reopened this Oct 14, 2019

Sjors force-pushed the 2018/03/bitcoin-conf-rw branch from ec0d14e to 4a72dcd Compare October 15, 2019 08:19

DrahtBot added Needs rebase and removed Needs rebase labels Oct 15, 2019

maflcko removed the Up for grabs label Oct 16, 2019

ryanofsky mentioned this pull request Oct 22, 2019

qt: Fix text display when state of prune button is changed #17035

Closed

luke-jr and others added 6 commits October 29, 2019 02:06

util: Support prepending configs in ReadConfigStream

11dbd1a

util: SelectBaseParams in ReadConfigFiles, before getting final datadir

bef8e8e

util: ReadConfigFiles flag to make everything network-specific

33df129

Add new bitcoin_rw.conf file that is used for settings modified by th…

956a76c

…is software itself

Add havePruned() to Node interface

31acbd1

[gui] settings: remove command line override label

14de2ee

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.

Sjors force-pushed the 2018/03/bitcoin-conf-rw branch 3 times, most recently from 1c03dc1 to 3142a95 Compare October 29, 2019 11:11

DrahtBot removed the Needs rebase label Oct 29, 2019

Sjors added 3 commits October 29, 2019 14:44

[wallet] move constants to walletconstants.h

b372aa5

[doc] clarify control flow in AppTests

b950472

Sjors force-pushed the 2018/03/bitcoin-conf-rw branch from 3142a95 to b950472 Compare October 29, 2019 13:44

DrahtBot added the Needs rebase label Oct 29, 2019

jonasschnelli closed this May 29, 2020

ryanofsky mentioned this pull request Oct 4, 2021

Do not require restart if overridden option is modified bitcoin-core/gui#440

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

Sjors commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 30, 2018

Uh oh!

Sjors commented Mar 30, 2018

Uh oh!

Sjors commented Apr 3, 2018

Uh oh!

jonasschnelli commented Apr 4, 2018

Uh oh!

Sjors commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Apr 17, 2018

Uh oh!

Sjors commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 6, 2018

Uh oh!

Sjors commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 10, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift Nov 11, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Nov 20, 2018

Uh oh!

DrahtBot commented Oct 29, 2019

Uh oh!

jonasschnelli commented May 29, 2020

Uh oh!

Sjors commented May 29, 2020

Uh oh!

Sjors commented Jun 26, 2020

Uh oh!

Rspigler commented Aug 19, 2020

Uh oh!

Sjors commented Aug 22, 2020

Uh oh!

Uh oh!

Sjors commented Mar 29, 2018 •

edited

Loading

laanwj commented Mar 29, 2018 •

edited

Loading

Sjors commented Mar 30, 2018 •

edited

Loading

Sjors commented Apr 4, 2018 •

edited

Loading

Sjors commented May 15, 2018 •

edited

Loading

Sjors commented Oct 9, 2018 •

edited

Loading

DrahtBot commented Nov 13, 2018 •

edited

Loading