New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read/write bitcoin_rw.conf for exposing shared Daemon/GUI options in the GUI #7510

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@luke-jr
Member

luke-jr commented Feb 11, 2016

This gives GUI options for maxuploadtarget and peerbloomfilters, while retaining the user-set values in the Daemon as well (unlike current GUI options). This is done by modifying a bitcoin_rw.conf which is loaded in addition to bitcoin.conf (before, because that's how our precedence is setup). There's about 200 LOC for making these updates, but it is all well-tested, and the worst-case scenario doesn't affect the user-managed bitcoin.conf file.

The ugliest part having to special-case additions to the OptionsModel stuff. It would be nice to avoid that, but I don't see any way to do so as long as an enum is being used as the interface there.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 11, 2016

Member

We've discussed this on IRC.

I'm not a big fan of adding yet another options source. The GUI already has command line, bitcoin.conf and QSettings - in that order of precedence. This has caused many complications and bugs in the past, especially during initialization, only now it seems to have cooled down a bit so I'm not really happy to stir the ants' nest again (we don't have a test suite for this part).

Remember this all has to be backwards compatible. We can't just drop QSettings support. If you remember 0.5.x, the client used to store GUI settings in the wallet. It took long enough until we were able to finally drop that functionality.

I'm happy that you're adding options to the GUI, but please don't couple it to this.

Member

laanwj commented Feb 11, 2016

We've discussed this on IRC.

I'm not a big fan of adding yet another options source. The GUI already has command line, bitcoin.conf and QSettings - in that order of precedence. This has caused many complications and bugs in the past, especially during initialization, only now it seems to have cooled down a bit so I'm not really happy to stir the ants' nest again (we don't have a test suite for this part).

Remember this all has to be backwards compatible. We can't just drop QSettings support. If you remember 0.5.x, the client used to store GUI settings in the wallet. It took long enough until we were able to finally drop that functionality.

I'm happy that you're adding options to the GUI, but please don't couple it to this.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 11, 2016

Member

This doesn't change initialisation at all, it uses the same code as bitcoin.conf for that. For now, everything currently in QSettings remains in QSettings (although upgrading them wouldn't be very difficult or risky, it's not really an immediate goal).

QSettings is not really a good solution since bitcoind won't use it (I can't think of any use case where you'd want these kind of settings different depending on whether you use Daemon vs GUI).

I think I'm going to need to make this aspect of the design be take-it-or-leave-it. It seems more than reasonable to let it sit a while and get any bugs ironed out in LJR/Knots before tackling it in Core.

Member

luke-jr commented Feb 11, 2016

This doesn't change initialisation at all, it uses the same code as bitcoin.conf for that. For now, everything currently in QSettings remains in QSettings (although upgrading them wouldn't be very difficult or risky, it's not really an immediate goal).

QSettings is not really a good solution since bitcoind won't use it (I can't think of any use case where you'd want these kind of settings different depending on whether you use Daemon vs GUI).

I think I'm going to need to make this aspect of the design be take-it-or-leave-it. It seems more than reasonable to let it sit a while and get any bugs ironed out in LJR/Knots before tackling it in Core.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 11, 2016

Member

QSettings is not really a good solution since bitcoind won't use it

That's certainly a valid argument for using our own system instead of QSettings for core settings.

If you add a test suite that tests interaction between various parameter sources, as well as backwards compatibility, I'd feel better about this. I just don't want to introduce a new source of stress.

Member

laanwj commented Feb 11, 2016

QSettings is not really a good solution since bitcoind won't use it

That's certainly a valid argument for using our own system instead of QSettings for core settings.

If you add a test suite that tests interaction between various parameter sources, as well as backwards compatibility, I'd feel better about this. I just don't want to introduce a new source of stress.

@laanwj laanwj added the GUI label Feb 11, 2016

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 11, 2016

Member

Conversation with @laanwj on IRC re settings enum: Goal should be to move this into util, not to introduce strings to GUI. Makes more sense to do it as part of RPC setconfig (#7289), rather than rwconf.

Member

luke-jr commented Feb 11, 2016

Conversation with @laanwj on IRC re settings enum: Goal should be to move this into util, not to introduce strings to GUI. Makes more sense to do it as part of RPC setconfig (#7289), rather than rwconf.

luke-jr added some commits Feb 13, 2016

Use network-specific directory for bitcoin_rw.conf
This necessarily loads bitcoin_rw.conf after bitcoin.conf, but takes care to allow the former to override options set only by the latter (eg, not ones set on the command line).

@laanwj laanwj added the Feature label Feb 16, 2016

@@ -1,6 +1,7 @@
* banlist.dat: stores the IPs/Subnets of banned nodes
* bitcoin.conf: contains configuration settings for bitcoind or bitcoin-qt
* bitcoin_rw.conf: contains configuration settings modified by bitcoind or bitcoin-qt: since Knots 0.12.0

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 19, 2016

Member

What is "Knots 0.12.0"?

@jonasschnelli

jonasschnelli Feb 19, 2016

Member

What is "Knots 0.12.0"?

This comment has been minimized.

@rebroad

rebroad Sep 25, 2016

Contributor

@jonasschnelli Google seems to be quite good at answering this question :)

@rebroad

rebroad Sep 25, 2016

Contributor

@jonasschnelli Google seems to be quite good at answering this question :)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 19, 2016

Member

Agree with @laanwj that we should not couple additional GUI settings with the different persistent store file.

QSettings works nice and is platform independent, but – I agree – users might have problems to locate the file (to delete or inspect/change) and I agree that keeping all bitcoin related config file in the bitcoin-datadir would be good.

I don't have a strong opinion on that, one one hand, I would like the fact to keep all files together (would also be possible with setting the QSettings path [http://doc.qt.io/qt-4.8/qsettings.html#setPath]?), on the other hand, adding more code of that type (that needs maintenance) should be avoided.

Member

jonasschnelli commented Feb 19, 2016

Agree with @laanwj that we should not couple additional GUI settings with the different persistent store file.

QSettings works nice and is platform independent, but – I agree – users might have problems to locate the file (to delete or inspect/change) and I agree that keeping all bitcoin related config file in the bitcoin-datadir would be good.

I don't have a strong opinion on that, one one hand, I would like the fact to keep all files together (would also be possible with setting the QSettings path [http://doc.qt.io/qt-4.8/qsettings.html#setPath]?), on the other hand, adding more code of that type (that needs maintenance) should be avoided.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 5, 2016

Member

@luke-jr: Are you interested to open a PR with just the maxuploadtarget GUI option? I think that one would be uncontroversial.

Member

jonasschnelli commented Apr 5, 2016

@luke-jr: Are you interested to open a PR with just the maxuploadtarget GUI option? I think that one would be uncontroversial.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Aug 24, 2016

Member

Needs to be closed or rebased. I think closing would make more sense since this seems to be controversial.

Member

jonasschnelli commented Aug 24, 2016

Needs to be closed or rebased. I think closing would make more sense since this seems to be controversial.

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Sep 25, 2016

Contributor

Not familiar with QConfig, but if it's Qt specific when it ought to influence bitcoind then I'd say the sooner it's removed the better. If this pull helps with that process then, utACK.

Contributor

rebroad commented Sep 25, 2016

Not familiar with QConfig, but if it's Qt specific when it ought to influence bitcoind then I'd say the sooner it's removed the better. If this pull helps with that process then, utACK.

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Sep 25, 2016

Contributor

@luke-jr Can this pull include an example bitcoin_rw.conf file or is it parsed in the same way as bitcoin.conf? If the latter then why the new code to parse it (can't it use the same code used to parse bitcoin.conf)?

Contributor

rebroad commented Sep 25, 2016

@luke-jr Can this pull include an example bitcoin_rw.conf file or is it parsed in the same way as bitcoin.conf? If the latter then why the new code to parse it (can't it use the same code used to parse bitcoin.conf)?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 25, 2016

Member

@rebroad bitcoin_rw.conf is parsed the same. The difference is that this file is modified by the code itself when the user changes options in the GUI, rather than needing to be modified manually by the user.

Member

luke-jr commented Sep 25, 2016

@rebroad bitcoin_rw.conf is parsed the same. The difference is that this file is modified by the code itself when the user changes options in the GUI, rather than needing to be modified manually by the user.

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Sep 25, 2016

Contributor

@rebroad Given that #7289 allows config changes during run-time - we might want to permit some of those changes to persist between runs - something like this pull (are there other options on offer) do seem to be needed - therefore ACK from me (unless a better alternative is coming along soon).

Contributor

rebroad commented Sep 25, 2016

@rebroad Given that #7289 allows config changes during run-time - we might want to permit some of those changes to persist between runs - something like this pull (are there other options on offer) do seem to be needed - therefore ACK from me (unless a better alternative is coming along soon).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 18, 2016

Member

Closing for now because it seems to be controversial.

Member

jonasschnelli commented Oct 18, 2016

Closing for now because it seems to be controversial.

Sjors added a commit to Sjors/bitcoin that referenced this pull request Mar 29, 2018

[qt] move QSettings to bitcoin.conf where possible
When QT launches, move the following QSettings to
bitcoin.conf:

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

Based on @luke-jr's #11082 and #7510

@Sjors Sjors referenced this pull request Mar 29, 2018

Open

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

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment