system: allow GUI to initialize default data dir #27273

pinheadmz · 2023-03-16T22:02:47Z

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.

DrahtBot · 2023-03-16T22:02:49Z

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

fanquake · 2023-03-17T09:34:02Z

https://github.com/bitcoin/bitcoin/pull/27273/checks?check_run_id=12065322378

********* Start testing of OptionTests *********
Config: 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
PASS   : OptionTests::initTestCase()
PASS   : OptionTests::migrateSettings()
PASS   : OptionTests::integerGetArgBug()
PASS   : OptionTests::parametersInteraction()
PASS   : OptionTests::extractFilter()
QWARN  : OptionTests::initDataDir() This plugin does not support propagateSizeHints()
QWARN  : OptionTests::initDataDir() This plugin does not support propagateSizeHints()

=== Received signal at function time: 299728ms, total time: 300005ms, dumping stack ===
=== End of stack trace ===
QFATAL : OptionTests::initDataDir() Test function timed out
FAIL!  : OptionTests::initDataDir() Received a fatal error.
   Loc: [Unknown file(0)]
Totals: 5 passed, 1 failed, 0 skipped, 0 blacklisted, 300009ms
********* Finished testing of OptionTests *********
FAIL qt/test/test_bitcoin-qt (exit status: 134)

ryanofsky

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

ryanofsky · 2023-03-24T14:10:50Z

src/qt/intro.cpp

@@ -260,8 +260,8 @@ bool Intro::showIfNeeded(bool& did_show_intro, int64_t& prune_MiB)
     * override -datadir in the bitcoin.conf file in the default data directory
     * (to be consistent with bitcoind behavior)
     */
-    if(dataDir != GUIUtil::getDefaultDataDirectory()) {


In commit "system: allow GUI to initialize default data dir" (7a9b786)

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 [...]"

ryanofsky · 2023-03-24T14:17:13Z

src/util/system.cpp

+void ArgsManager::InitDefaultDataDir(const fs::path path)
+{
+    LOCK(cs_args);
+    if (m_init_default_datadir_path.empty())


In commit "system: allow GUI to initialize default data dir" (7a9b786)

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.

pinheadmz · 2023-03-24T15:54:06Z

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.

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.

ryanofsky · 2023-03-24T16:15:41Z

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.

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.

pinheadmz · 2023-03-28T15:09:53Z

Closing for now, strDataDir is a bit annoying and we should consider getting rid of it.

Currently if a you choose a non-default datadir in the GUI intro screen, the datadir is ignored by CLI tools. This means `bitcoin-cli` and `bitcoin-wallet` will try to use the wrong datadir and show errors if they are called without a -datadir arguments, and `bitcoind` will appear to work but use a different datadir, not loading the same wallets and settings, and downloading blocks into the wrong place. There are also more subtle inconsistencies between GUI and CLI selection of datadirs such as bitcoin#27273 where GUI might ignore a datadir= line in a bitcoin.conf that CLI tools would apply. This PR gets rid of inconsistencies between GUI and CLI tools and makes them use the same datadir setting by default. It is followup to bitcoin-core/gui#602 which made GUI and CLI tools use the same `-dbcache`, `-par`, `-spendzeroconfchange`, `-signer`, `-upnp`, `-natpmp`, `-listen`, `-server`, `-prune`, `-proxy`, `-onion`, and `-lang` settings as long as they loaded the same datadir. The reason for GUI and CLI tools using inconsistent datadirs, is that GUI stores the datadir path in a `strDataDir` field in `.config/Bitcoin/Bitcoin-Qt.conf`[^1] which CLI tools ignore. This PR changes the GUI to instead store the datadir path at the default datadir location `~/.bitcoin`[^2] as a symlink that CLI tools will already follow, or as a text file if the filesystem does not support creating symlinks. If upgrading from a previous version of the GUI and there is only a GUI datadir, the `strDataDir` setting will be automatically migrated to a symlink so CLI tools will start using it as well. If CLI and GUI tools are currently using different default datadirs, the GUI will show a prompt allowing either of the datadirs to be loaded and optionally set as the common default going forward.

pinheadmz force-pushed the conf-file branch 3 times, most recently from 3636af3 to c589b66 Compare March 20, 2023 14:58

pinheadmz mentioned this pull request Mar 20, 2023

Error: specified data directory "\\IP.Ad.re.ss\release\Folder"does not exist #25868

Closed

pinheadmz force-pushed the conf-file branch 3 times, most recently from 6f7fd82 to 25ee539 Compare March 21, 2023 01:18

bitcoin deleted a comment from crystalshay2es Mar 21, 2023

pinheadmz marked this pull request as ready for review March 21, 2023 13:53

pinheadmz mentioned this pull request Mar 21, 2023

Bitcoin ignores datadir and blocksdir parameter in .conf #27246

Closed

1 task

system: allow GUI to initialize default data dir

7a9b786

pinheadmz force-pushed the conf-file branch from 25ee539 to 7a9b786 Compare March 22, 2023 18:58

DrahtBot mentioned this pull request Mar 23, 2023

system: cache config file path before potentially updating datadir #27303

Closed

ryanofsky reviewed Mar 24, 2023

View reviewed changes

pinheadmz closed this Mar 28, 2023

bitcoin locked and limited conversation to collaborators Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

system: allow GUI to initialize default data dir #27273

system: allow GUI to initialize default data dir #27273

Uh oh!

pinheadmz commented Mar 16, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Mar 16, 2023 •

edited

Loading

Uh oh!

fanquake commented Mar 17, 2023

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

ryanofsky Mar 24, 2023

Uh oh!

ryanofsky Mar 24, 2023

Uh oh!

pinheadmz commented Mar 24, 2023

Uh oh!

ryanofsky commented Mar 24, 2023

Uh oh!

pinheadmz commented Mar 28, 2023

Uh oh!

Uh oh!

system: allow GUI to initialize default data dir #27273

system: allow GUI to initialize default data dir #27273

Uh oh!

Conversation

pinheadmz commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented Mar 17, 2023

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Mar 24, 2023

Uh oh!

ryanofsky commented Mar 24, 2023

Uh oh!

pinheadmz commented Mar 28, 2023

Uh oh!

Uh oh!

pinheadmz commented Mar 16, 2023 •

edited

Loading

DrahtBot commented Mar 16, 2023 •

edited

Loading

ryanofsky left a comment •

edited

Loading