Skip to content
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

qt: Add -guisettingsdir option #17636

Closed
wants to merge 1 commit into from

Conversation

emilengler
Copy link
Contributor

@emilengler emilengler commented Nov 30, 2019

Short Description

This PR makes it possible to change the Qt relevant config files in a custom directory

Motivation

There are multiple usecases where it could be useful, for example when you use Bitcoin Core on a USB stick, you can keep the config files there as well. My motivation was that I don't need to move my production bitcoin-qt config file to a different place because I don't want that an unstable bulid corrupts my datadir. Currently I need to delete my config or reset the datadir path every time I switch between productive to development. With this I can have a config directory for my productive use and one for my development use.

Showcase

Having a directory called /home/emil/testconf and running bitcoin-qt will result in this hier: /home/emil/testconf/Bitcoin/Bitcoin-Qt.conf. The Bitcoin subdirectory is a feature, not a bug to make it possible to keep other Qt config files there without colliding. If the the user has no permissions, it won't save anything, if the dir does not exists it will be created.

@fanquake fanquake added the GUI label Nov 30, 2019
@fanquake fanquake requested a review from ryanofsky Nov 30, 2019
Copy link
Member

@hebasto hebasto left a comment

Concept ACK 96a1caf

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
@@ -466,6 +468,10 @@ int GuiMain(int argc, char* argv[])
QApplication::setOrganizationName(QAPP_ORG_NAME);
QApplication::setOrganizationDomain(QAPP_ORG_DOMAIN);
QApplication::setApplicationName(QAPP_APP_NAME_DEFAULT);
if (gArgs.IsArgSet("-guisettings") && gArgs.GetArg("-guisettings", "") != "") {
Copy link

@paymog paymog Nov 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to test this logic in UTs?

Copy link
Contributor Author

@emilengler emilengler Nov 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'm not an expert with the test framework but we could run bitcoin-qt --guisettings=/a/custom/path/ and see if /a/custom/path/Bitcoin/Bitcoin-Qt.conf exists.
The problem with the Qt tests is that they test the actual GUI like clicking buttons. This belongs more to a startup test, don't know if this exists

src/qt/bitcoin.cpp Show resolved Hide resolved
@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Dec 1, 2019

I prefer #15936 in the long run. Maybe this makes sense as an intermediary step... but unsure (could also be a maintenance burden since this needs to go away with #15936 very likely). Pinging @ryanofsky.

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Dec 1, 2019

@jonasschnelli #15936 is something different. -guisettings is also intended to be the Qt relevant equivalent of bitcoind's -conf

Copy link
Member

@promag promag left a comment

I understand the OP motivation, not sure if the new argument is the best. Specifying a directory gives the impression that multiple files could be stored there.

How about supporting specifying the filename like =my_settings_1.ini? This could be an absolute path or relative to datadir?

doc/files.md Outdated Show resolved Hide resolved
@@ -466,6 +468,10 @@ int GuiMain(int argc, char* argv[])
QApplication::setOrganizationName(QAPP_ORG_NAME);
QApplication::setOrganizationDomain(QAPP_ORG_DOMAIN);
QApplication::setApplicationName(QAPP_APP_NAME_DEFAULT);
if (gArgs.GetArg("-guisettings", "") != "") {
QSettings::setDefaultFormat(QSettings::IniFormat);
QSettings::setPath(QSettings::IniFormat, QSettings::UserScope, QString::fromStdString(gArgs.GetArg("-guisettings", "")));
Copy link
Member

@promag promag Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have some check, could be an invalid or read only path and then it wouldn't work as expected.

Copy link
Contributor Author

@emilengler emilengler Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that is that important? If the path does not exists it will create the directory recursively, if the user has no permission it will fallback to the default. Don't know if it is that much required to check for writing privileges.

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Dec 1, 2019

Concept ACK. This seems at least marginally useful and I don't see drawbacks. It could use better documentation and error checking though. Documentation: should specified path be a file or directory path, or are both accepted? Error checking: it'd be nice to trigger InitErrors if specified location can't be opened or isn't writeable. It'd also be nice to trigger an init error if guisettings=path is specified in the bitcoin.conf file since right now it looks like that is silently ignored.

I'm also not sure if there are caveats to using QSettings::IniFormat instead of QSettings::NativeFormat. According to https://doc.qt.io/qt-5/qsettings.html#Format-enum, "Note that type information is not preserved when reading settings from INI files; all values will be returned as QString." So it'd be worth looking into whether thing there might be subtle bugs from using this option. I do see some types used in my current config file like MainWindowGeometry=@ByteArray(...)

re: #17636 (comment)

This PR is compatible with #15936, but #15936 will make it less practically useful because #15936 moves shared bitcoind / bitcoin-qt settings like pruning and proxy settings out of gui-only QSettings storage which is ignored by bitcoind into shared datadir storage used by both bitcoind and bitcoin-qt.

But even after #15936, QSettings storage does not go away. It still used for GUI-only settings like window positions, coin control modes, and the default datadir location. Longer term I think it makes sense to store as many settings as possible in the datadir and avoid using QSettings. But I think QSettings storage might never go away entirely, because it does give GUI users a simple way to choose a custom datadir location, and obviously the datadir location can't be stored in the datadir.

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Dec 2, 2019

I understand the OP motivation, not sure if the new argument is the best. Specifying a directory gives the impression that multiple files could be stored there.

That's a design choice. Other files should be stored there like Bitcoin-Qt-regtest.conf (forgot the actual filename, but I mean the other chain configs).

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Dec 2, 2019

It'd also be nice to trigger an init error if guisettings=path is specified in the bitcoin.conf file since right now it looks like that is silently ignored.

Is it even possible to add GUI related stuff to the bitcoin.conf?
Also the -guisettings parsing needs to be done as one of the first operations. As the translation system is going to access QSettings for the language string.

I'm also not sure if there are caveats to using QSettings::IniFormat instead of QSettings::NativeFormat. According to https://doc.qt.io/qt-5/qsettings.html#Format-enum, "Note that type information is not preserved when reading settings from INI files; all values will be returned as QString." So it'd be worth looking into whether thing there might be subtle bugs from using this option. I do see some types used in my current config file like MainWindowGeometry=@bytearray(...)

On UNIX system it just works fine as QSettings::IniFormat is the QSettings::NativeFormat. If you specify -guisettings, it will always use QSettings::IniFormat as it is nearly impossible to store the windows registry in one portable file.

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Dec 2, 2019

It'd also be nice to trigger an init error if guisettings=path is specified in the bitcoin.conf file since right now it looks like that is silently ignored.

Is it even possible to add GUI related stuff to the bitcoin.conf?

No, but that isn't obvious. I'm suggesting it should trigger an init error unless that's difficult, because most command line arguments do work in the config file and the fact that this is setting is silently ignored is just a quirk of the current implementation, and not something I was expecting, or would expect a user to know about.

I think it would be pretty easy to add an error with a new COMMAND_LINE_ONLY flag to complement existing DEBUG_ONLY and NETWORK_ONLY flags:

bitcoin/src/util/system.h

Lines 141 to 147 in 35eda63

DEBUG_ONLY = 0x100,
/* Some options would cause cross-contamination if values for
* mainnet were used while running on regtest/testnet (or vice-versa).
* Setting them as NETWORK_ONLY ensures that sharing a config file
* between mainnet and regtest/testnet won't cause problems due to these
* parameters by accident. */
NETWORK_ONLY = 0x200,

I'm also not sure if there are caveats to using QSettings::IniFormat instead of QSettings::NativeFormat. According to https://doc.qt.io/qt-5/qsettings.html#Format-enum, "Note that type information is not preserved when reading settings from INI files; all values will be returned as QString." So it'd be worth looking into whether thing there might be subtle bugs from using this option. I do see some types used in my current config file like MainWindowGeometry=@bytearray(...)

On UNIX system it just works fine as QSettings::IniFormat is the QSettings::NativeFormat. If you specify -guisettings, it will always use QSettings::IniFormat as it is nearly impossible to store the windows registry in one portable file.

I'll take your word for it but this seems to contradict the documentation which mentions differences between IniFormat and NativeFormat on unix, like the fact that they use different file extensions, and warns about IniFormat returning everything as a strings. But if there's no issue that's great.

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Dec 2, 2019

@ryanofsky Indeed the file extensions are different but the files are the same so it doesn't matter:

$ sha256sum testdir/Bitcoin/Bitcoin-Qt.ini
d00cb5f6ef6183e9752e56bbd13a95b31f39d7ba5a82933449d8bdf2576060f4  testdir/Bitcoin/Bitcoin-Qt.ini
 $ sha256sum .config/Bitcoin/Bitcoin-Qt.conf
d00cb5f6ef6183e9752e56bbd13a95b31f39d7ba5a82933449d8bdf2576060f4  .config/Bitcoin/Bitcoin-Qt.conf

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Dec 2, 2019

@ryanofsky Indeed the file extensions are different but the files are the same so it doesn't matter:

Question is how values are read, not really how they are written. e.g. is @bytearray(...) read as an actual byte array or as the string "@bytearray(...)"

@@ -393,6 +394,7 @@ WId BitcoinApplication::getMainWinId() const
static void SetupUIArgs()
{
gArgs.AddArg("-choosedatadir", strprintf("Choose data directory on startup (default: %u)", DEFAULT_CHOOSE_DATADIR), ArgsManager::ALLOW_ANY, OptionsCategory::GUI);
gArgs.AddArg("-guisettings=<path>", "Choose a custom data directory especially for the Qt Settings", ArgsManager::ALLOW_ANY, OptionsCategory::GUI);
Copy link
Member

@laanwj laanwj Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that due to initialization order constraints, this option can only be provided on the command line.
It won't work in the configuration file because at that time, QSettings was already parsed.

Copy link
Contributor Author

@emilengler emilengler Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are wrong.
Take a look at src/qt/bitcoin.cpp line 451.
There you'll see that SetupUIArgs() is executed before the PR feature.

Copy link
Contributor

@ryanofsky ryanofsky Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #17636 (comment)

I concluded the same as Wladimir based on the fact that readConfigFiles is called after GetArg("-guisettings"), so presumably it could not affect the result of the earlier call.

@laanwj
Copy link
Member

@laanwj laanwj commented Dec 3, 2019

Concept ~0 on this. If there are actual users that have use for this, I'm okay with it (it doesn't add that much complexity), but, it's use is pretty constrained and this is heavily initialization-order dependent, and yet another thing to test when testing setting reading.

@laanwj
Copy link
Member

@laanwj laanwj commented Dec 3, 2019

This definitely needs a test of some kind.

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Dec 4, 2019

@laanwj Maybe I overlooked it but I haven't found any Qt tests which test CLI args. Is it really that worth to implement a complete new system just for that?

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Is it really that worth to implement a complete new system just for that?

I think it would be great to have a new test for this, not just for this feature but also because the framework could be extended to verify other init behavior. But you are right that there isn't currently an easy way to write tests covering the GuiMain function, so I personally don't think one needs to be added in this PR.

Perhaps the easiest test to write would be a python test invoking bitcoin-qt with QT_QPA_PLATFORM=minimal and -guisettings=path, calling the stop rpc right away, and checking for the ini file in the filesystem. I don't think it would be too hard to write a C++ unit test either, but it'd probably require breaking up the GuiMain function which might be overkill here.

My main feedback for this PR is still to add better error checking and documentation: #17636 (comment)

@@ -393,6 +394,7 @@ WId BitcoinApplication::getMainWinId() const
static void SetupUIArgs()
{
gArgs.AddArg("-choosedatadir", strprintf("Choose data directory on startup (default: %u)", DEFAULT_CHOOSE_DATADIR), ArgsManager::ALLOW_ANY, OptionsCategory::GUI);
gArgs.AddArg("-guisettings=<path>", "Choose a custom data directory especially for the Qt Settings", ArgsManager::ALLOW_ANY, OptionsCategory::GUI);
Copy link
Contributor

@ryanofsky ryanofsky Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #17636 (comment)

I concluded the same as Wladimir based on the fact that readConfigFiles is called after GetArg("-guisettings"), so presumably it could not affect the result of the earlier call.

@Talkless
Copy link

@Talkless Talkless commented Dec 6, 2019

Concept ACK. Looks useful.

Built & tested a bit - changing values to switch between two .ini files does change language, window position, listening, etc.

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
@emilengler emilengler force-pushed the 2019-11-guisettings branch 2 times, most recently from d76d202 to 5266efa Compare Dec 18, 2019
@emilengler emilengler changed the title qt: Add -guisettings option qt: Add -guisettingsdir option Dec 18, 2019
@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Dec 23, 2019

Push, as I changed the title and added the dir suffix

@Talkless
Copy link

@Talkless Talkless commented Feb 4, 2020

notice this somehow

I doubt this is good enough. Consider this setup:

$ fgrep bPrune ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf 
bPrune=false
$ fgrep bPrune /tmp/myconfig/Bitcoin/Bitcoin-Qt-testnet.ini 
bPrune=true

Now imagine myconfig dir "disappears", like, forgot to plug-in USB drive, network drive not mounted, or whatever. Bitoinc-Qt will silently start downloading full chain due to old config, possibly filling users drive...

Consider taking @ryanofsky suggestion about using QSettings::sync().

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Feb 4, 2020

Now imagine myconfig dir "disappears", like, forgot to plug-in USB drive, network drive not mounted, or whatever. Bitoinc-Qt will silently start downloading full chain due to old config, possibly filling users drive...

No, bitcoin-qt doesn't necessarily know where the datadir is and the intro screen opens again. Also I wouldn't call this a direct issue of this PR...

@Talkless
Copy link

@Talkless Talkless commented Feb 5, 2020

datadir

This PR is not about data dir (~/.bitcoin), but Qt settings dir (~/.config/Bitcoin). If you remove data dir - yes, wizard is show. But not when -guisettingsdir is missing. Try this:

mkdir /tmp/myconfig
bicoin-qt -testnet -guisettingsdir=/tmp/mysettings
# ... enable pruning in `Settings -> Options` window ..
# ... quit bitcoin-qt ...
mv /tmp/mysettings /tmp/mysettings_removed
bicoin-qt -testnet -guisettingsdir=/tmp/mysettings
# ... go to `Options` again, see pruning quietly disabled...

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Feb 5, 2020

I was talking about the Qt settings. The data dir path is set in the Qt settings. Sorry if you understood it wrong

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Feb 6, 2020

utACK 5266efa

@Talkless
Copy link

@Talkless Talkless commented Feb 6, 2020

The data dir path is set in the Qt settings

It doesn't matter, you can remove strDataDir=/home/vincas/.bitcoin line from it and it will still use this path as default.

Anyways, I am not talking theoretically, I ACTAULLY tested your code and reproduced issue when invalid -guisettingsdir path makes pruning disabled (it's by default disabled, empty QSettings results in default), which is unexpected by user, and results your blockchain copy a full, unpruned archive, using up space.

If I set invalid -blocksdir=/tmp/foo/bar, I get hard error.
If I set invalid -datadir=/tmp/foo/bar, I get hard error.
If I set invalid -walletdir=/tmp/foo/bar, I get hard error.
If I set invalid -guisettingsdir=/tmp/foo/bar, I DO NOT get hard error. This is incossistency, dangerous, and so wrong.

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Feb 6, 2020

Anyways, I am not talking theoretically, I ACTAULLY tested your code and reproduced issue when invalid -guisettingsdir path makes pruning disabled (it's by default disabled, empty QSettings results in default), which is unexpected by user, and results your blockchain copy a full, unpruned archive, using up space.

I bet that 99.99% percent of users will notice it if the blockchain gets re-downloaded (modaloverlay) opens again, etc.

@Talkless
Copy link

@Talkless Talkless commented Feb 10, 2020

I bet that 99.99% percent of users will notice

bitcoin-qt shows me that overlay every day I launch it, as it has to keep up after being offline for the night. I just instinctively close window to minimize it without bothering much...

Anyway, this is only one of possible surprises user can get once -guisettingsdir is set to invalid path.

@Talkless
Copy link

@Talkless Talkless commented Feb 10, 2020

Tested, Approach NACK, due to insufficient error handling which is inconsistent when compared to -blocksdir/-datadir/-walletdir command line options, and IMHO authors arguments keeping that way are not strong enough.

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Mar 5, 2020

From IRC

would be good to get ryanofsky's ack

IMO adding this with no error checking is adding a slightly useful feature that's a big footgun in practice. I don't want to block this change if other people want it, but would just again recommend error checking: #17636 (comment) so there aren't silent failures and silently discarded settings with this

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Mar 20, 2020

Rebased on 5bf45fe,
will now work ryanofskys suggestion

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Mar 20, 2020

The client now checks if the custom config file is accessible. If not, it returns an error. If it is not accessible but it is the first time you run bitcoin-qt (datadir does not exists), it will create the config at that location. If this fails it also returns an error.

std::fstream f;
f.open(settingspath.c_str());
// File could not be opened
if (f.fail()) {
Copy link

@Talkless Talkless Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use QFile? In that case would be more "straightforward" - no need to convert QString to std::string.

// File could not be opened
if (f.fail()) {
QMessageBox::critical(nullptr, PACKAGE_NAME,
QObject::tr("Error: Cannot open config file at \"%1\"").arg(QString::fromStdString(qt_settings_path)));
Copy link

@Talkless Talkless Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO that message could be more concrete - that config file is a bit goo vague. Maybe mentioning GUI settings file would be more informative (since it's for -guisettingsdir).

Copy link
Member

@luke-jr luke-jr left a comment

If it is not accessible but it is the first time you run bitcoin-qt (datadir does not exists), it will create the config at that location.

I don't see code for this...?

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Apr 6, 2020

Thanks for your review @luke-jr

I don't see code for this...?

This is covered by the Qt library itself

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 6, 2020

This is covered by the Qt library itself

Qt doesn't know if -datadir exists or not...

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented May 29, 2020

Is it intentional that changing the guisettingsdir to an empty folder, once you have a datadir setup (say after IBD), will not work?

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented May 29, 2020

Tested a bit and does not work as expected:

Tried on an existing node/datadir (where I previously created a folder /tmp/gui):

./src/qt/bitcoin-qt --regtest --guisettingsdir=/tmp/gui

Leads to:
Bildschirmfoto 2020-05-29 um 10 10 37

Same for creating a new datadir:

./src/qt/bitcoin-qt --regtest --datadir=/tmp/node --guisettingsdir=/tmp/gui

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented May 29, 2020

@jonasschnelli Ok, I will have a look at this somewhere this weekend

@fanquake
Copy link
Member

@fanquake fanquake commented Jul 15, 2020

Progress here seems to have stalled, and this doesn't yet seem to work as intended. There have also been multiple requests for tests, and none have been added. I'm going to close this and suggest re-opening your changes in the gui repo if you are still interested in working on them, as aside from files.md this is contained to qt code.

@fanquake fanquake closed this Jul 15, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet