closes #27246
Replaces a SoftSetArg() called by the GUI before parsing bitcoin.conf with a new method InitDefaultDataDir(). This allows the GUI to look for the conf file in a user-specified, non-default location and still parse a datadir= setting from that file. Adds unit tests for argsman and also for Qt and its QSettings.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process. A summary of reviews will appear here.
No conflicts as of last run.
https://github.com/bitcoin/bitcoin/pull/27273/checks?check_run_id=12065322378
0********* Start testing of OptionTests *********
1Config: Using QtTest library 5.15.5, Qt 5.15.5 (x86_64-little_endian-lp64 static release build; by Clang 8.0.1 (tags/RELEASE_801/final)), debian 10
2PASS : OptionTests::initTestCase()
3PASS : OptionTests::migrateSettings()
4PASS : OptionTests::integerGetArgBug()
5PASS : OptionTests::parametersInteraction()
6PASS : OptionTests::extractFilter()
7QWARN : OptionTests::initDataDir() This plugin does not support propagateSizeHints()
8QWARN : OptionTests::initDataDir() This plugin does not support propagateSizeHints()
9
10=== Received signal at function time: 299728ms, total time: 300005ms, dumping stack ===
11=== End of stack trace ===
12QFATAL : OptionTests::initDataDir() Test function timed out
13FAIL! : OptionTests::initDataDir() Received a fatal error.
14 Loc: [Unknown file(0)]
15Totals: 5 passed, 1 failed, 0 skipped, 0 blacklisted, 300009ms
16********* Finished testing of OptionTests *********
17FAIL qt/test/test_bitcoin-qt (exit status: 134)
259@@ -260,8 +260,8 @@ bool Intro::showIfNeeded(bool& did_show_intro, int64_t& prune_MiB)
260 * override -datadir in the bitcoin.conf file in the default data directory
261 * (to be consistent with bitcoind behavior)
262 */
263- if(dataDir != GUIUtil::getDefaultDataDirectory()) {
In commit “system: allow GUI to initialize default data dir” (7a9b786e00a5275ba012bc37a3796d1d35491161)
This if statement no longer does anything and could be dropped. Just calling InitDefaultDataDir unconditionally would have the same effect and be easier to understand. Also the comment above needs to be updated. Could replace “Only override -datadir if different from the default, to make it possible […]” with “Set chosen directory as the default datadir. It is possible […]”
437@@ -438,6 +438,13 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
438 return path;
439 }
440
441+void ArgsManager::InitDefaultDataDir(const fs::path path)
442+{
443+ LOCK(cs_args);
444+ if (m_init_default_datadir_path.empty())
In commit “system: allow GUI to initialize default data dir” (7a9b786e00a5275ba012bc37a3796d1d35491161)
This if statment is unnecessary and causes InitDefaultDataDir to do nothing if it is called a second time. Would be saner to rename InitDefaultDataDir SetDefaultDataDir and just set the path unconditionally without unnecessary stickiness and no way of knowing whether the path was set.
I’m having second thoughts about this change, because while I do think the new behavior is simpler and better, the current behavior is not as crazy as I initially thought, and changing it now could cause problems.
I do think first of all that the PR could have clearer title and description to actually say what it does. For the PR title and description I would suggest:
gui: Make bitcoin.conf datadir reliably override the GUI datadir
Current bitcoin-qt behavior is to apply the datadir= setting in a bitcoin.conf file ONLY when the datadir setting picked in the GUI is the same as the default datadir location for the platform (~/.bitcoin on linux, ~/Library/Application Support/Bitcoin/ on macos, %APPDATA%\Bitcoin on windows). If the datadir picked in the GUI is set to any other value, the datadir= line in bitcoin.conf is ignored. The PR changes behavior to reliably make the bitcoin.conf datadir line take precedence over the GUI setting. It avoids the confusing behavior of datadir= setting in bitcoin.conf being ignored or sometimes applied depending on where the bitcoin.conf file is located.
Here are the reasons I no longer think this change would be good:
While the new behavior of always obeying the bitcoin.conf datadir= setting is simpler and easier to understand, I don’t think there is actually a use-case that requires it. The use-case for setting datadir= in bitcoin.conf generally is to be able to set an external storage location for wallet and block data, and this works fine now as long as bitcoin.conf is put in the default location. I doubt anybody really needs to configure the GUI to locate bitcoin.conf in a non-default second location, and then have that bitcoin.conf set a datadir in a third location. Most likely anybody who has done this did it by accident and would be better served by a warning about inconsistent datadir settings.
This change could cause compatibility problems. Some user could have a set a datadir= line in their bitcoin.conf file, and later moved and changed their datadir location in the GUI, forgetting about the datadir= line in bitcoin.conf file which the GUI would ignore. The change in this PR would suddenly cause the old bitcoin.conf datadir= line to come back to life, and wallets would appear missing, and blocks would be downloaded from scratch using an old storage location. I don’t know how likely it would be for this problem to happen, but it seems like it would be causing a lot of potential pain for no real benefit to users.
I do think code and test in the PR could be repurposed to take a more conservative approach, and just show a warning if a GUI bitcoin.conf datadir= line is being ignored. (If implementing this would suggest doing it in a new PR to avoid stale review comments and confusion between different approaches.)
- I doubt anybody really needs to configure the GUI to locate
bitcoin.confin a non-default second location, and then have thatbitcoin.confset a datadir in a third location.
Wasn’t that the user story in #27246 (comment) ?
Either way I see your point about the compatibility issues.
What actually might be more useful for that user or related cases is the option in the GUI to clear the QSettings and so the user can re-set the “initial directory” which “should” include a bitcoin.conf but doesn’t need to be the actual datdir or blocksdir. While testing this out on macOS I found it extremely hard to clear the plist data which is cached by OS and not easy to bust.
- I doubt anybody really needs to configure the GUI to locate
bitcoin.confin a non-default second location, and then have thatbitcoin.confset a datadir in a third location.Wasn’t that the user story in #27246 (comment) ?
Yes but I think a better alternative for that user would be to use -includeconf which works today (or symlinks) rather than datadir daisy chaining which doesn’t work and would need to be implemented, and could cause compatibility issues, and as far as we know would not be the simplest solution for the problem they are trying to solve.
What actually might be more useful for that user or related cases is the option in the GUI to clear the QSettings and so the user can re-set the “initial directory” which “should” include a bitcoin.conf but doesn’t need to be the actual datdir or blocksdir. While testing this out on macOS I found it extremely hard to clear the plist data which is cached by OS and not easy to bust.
This could be a good idea, but it would depend a lot on the specific details. There is -resetguisettings option which can help too.
strDataDir is a bit annoying and we should consider getting rid of it.