Make GUI and CLI tools use the same datadir

ryanofsky commented at 4:21 pm on April 3, 2023: contributor

This is based on #27302. The non-base commits are:

Currently if your choose a non-default datadir in the GUI intro screen, the datadir is ignored by CLI tools.

This PR makes GUI and CLI tools the same datadir setting by default. It is followup to https://github.com/bitcoin-core/gui/pull/602 which made GUI and CLI tools use the same settings as long as they loaded the same datadir.

The reason GUI and CLI tools use inconsistent datadirs is that GUI stores the datadir path in a strDataDir field in .config/Bitcoin/Bitcoin-Qt.conf¹ which CLI tools ignore. This PR changes the GUI to instead store the datadir path at the default datadir location ~/.bitcoin² as a symlink that CLI tools will already follow, or as a text file if the filesystem does not support 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 upgrading and GUI and CLI tools are using different datadirs, the GUI will show a prompt allowing either of the datadirs to be loaded on startup, with an option to set one as the default going forward.

strDataDir value is stored in .config/Bitcoin/Bitcoin-Qt.conf on linux, in property list files on macos, and in registry keys on windows. ↩︎
The default datadir location is ~/.bitcoin on linux, ~/Library/Application Support/Bitcoin on macos, and %APPDATA%\Bitcoin on windows ↩︎

DrahtBot commented at 4:21 pm on April 3, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/149 (Intro: Have user choose assumevalid by luke-jr)
#27491 (refactor: Move chain constants to the util library by TheCharlatan)
#27419 (move-only: Extract common/args from util/system by TheCharlatan)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

ryanofsky commented at 4:22 pm on April 3, 2023: contributor

This is a draft PR and not fully implemented. Just opening it to share progress since it’s taking longer than I expected to implement correctly

DrahtBot added the label Needs rebase on Apr 3, 2023

lint: Fix lint-format-strings false positives when format specifiers have argument positions

Do not error on valid format specifications like strprintf("arg2=%2$s arg1=%1$s arg2=%2$s", arg1, arg2);

Needed to avoid lint error in upcoming commit: https://cirrus-ci.com/task/4755032734695424?logs=lint#L221

Additionally tested with python -m doctest test/lint/run-lint-format-strings.py

05e864a1f1

init: Error if ignored bitcoin.conf file is found

Show an error on startup if a bitcoin datadir that is being used contains a
`bitcoin.conf` file that is ignored. There are two cases where this could
happen:

- One case reported in
  https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043
  happens when a bitcoin.conf file in the default datadir (e.g.
  $HOME/.bitcoin/bitcoin.conf) has a "datadir=/path" line that sets different
  datadir containing a second bitcoin.conf file. Currently the second
  bitcoin.conf file is ignored with no warning.

- Another way this could happen is if a -conf= command line argument points
  to a configuration file with a "datadir=/path" line and that specified path
  contains a bitcoin.conf file, which is currently ignored.

This change only adds an error message and doesn't change anything about way
settings are applied. It also doesn't trigger errors if there are redundant
-datadir or -conf settings pointing at the same configuration file, only if
they are pointing at different files and one file is being ignored.

385b3e5f97

bugfix: Fix incorrect debug.log config file path

Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
https://github.com/bitcoin/bitcoin/pull/27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>

641b66c1fa

Merge remote-tracking branch 'origin/pull/27302/head' 8d777c0670

bitcoin-wallet: make bitcoin-wallet tool load config file

Currently bitcoin-wallet accepts a -datadir argument but ignores and config
file in that directory. This changes it to load the config file for consistency
with other cli tools, and in case it contains any significant settings.

Also update various tests using the data directory to call ReadConfigFiles.

Motivation for these changes is to prevent errors after the next commit makes
it at assert error to get the datadir path without reading the configuration
first.

ae5fc323b2

init: Allow bitcoin default datadir to point at an external datadir

It's always been possible for bitcoin default datadirs ($HOME/.bitcoin,
$HOME/Library/Application Support/Bitcoin, %APPDATA%\Bitcoin) to point at an
external locations using symlinks, but not all filesystems support symlinks, so
add extra support for pointing at external locations using text file.

This feature is used in the following commit to allow the bitcoin GUI to select
a default datadir location that will also be treated as the default datadir for
CLI tools. Currently when a custom data is set in the GUI, CLI tools ignore it
by default.

b091d9b076

Make GUI and CLI tools use the same datadir

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 #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
https://github.com/bitcoin-core/gui/pull/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.

5855150efa

ryanofsky force-pushed on Apr 12, 2023

DrahtBot removed the label Needs rebase on Apr 12, 2023

pinheadmz commented at 11:21 am on April 15, 2023: member

This PR probably closes https://github.com/bitcoin/bitcoin/issues/8106

DrahtBot commented at 10:23 am on April 21, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Apr 21, 2023

luke-jr commented at 10:21 pm on June 22, 2023: member

Won’t this change/break putting the bitcoin.conf in the GUI-selected datadir?

ryanofsky commented at 10:36 pm on June 22, 2023: contributor

Won’t this change/break putting the bitcoin.conf in the GUI-selected datadir?

No, I definitely need to update this and it may contain bugs. But do you see something here that would cause that to happen?

luke-jr commented at 11:02 pm on June 22, 2023: member

No, I didn’t get to the code yet - was just commenting from the description of this PR and #27302

DrahtBot commented at 0:34 am on September 20, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

in test/functional/feature_config_args.py:120 in 8d777c0670 outdated

112@@ -108,6 +113,41 @@ def test_config_file_parser(self):
113         with open(inc_conf_file2_path, 'w', encoding='utf-8') as conf:
114             conf.write('')  # clear
115 
116+    def test_config_file_log(self):
117+        # Disable this test for windows currently because trying to override
118+        # the default datadir through the environment does not seem to work.
119+        if sys.platform == "win32":
120+            return

pinheadmz commented at 12:17 pm on October 3, 2023:

Confirmed, doesn’t work :-( https://learn.microsoft.com/en-us/windows/win32/shell/csidl#remarks

DrahtBot commented at 0:37 am on January 1, 2024: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

maflcko assigned ryanofsky on Jan 2, 2024

DrahtBot commented at 0:28 am on March 31, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:03 am on June 28, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

maflcko commented at 9:16 am on June 28, 2024: member

@ryanofsky What is the status of this?

ryanofsky commented at 2:34 pm on June 28, 2024: contributor

re: #27409 (comment)

@ryanofsky What is the status of this?

It’s a change I’d really like to get back to, but is not ready now. The last time I worked on it, which was months ago, I started writing a Qt test for it but there are so many permutations of configurations (presence/absense of different data directory locations) that I was struggling to write it. I would like to get back to this at some point and I do have work in progress, but I don’t think I will have an update on it soon, given other things I’m working on.

DrahtBot commented at 1:30 am on September 25, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

Make GUI and CLI tools use the same datadir #27409

Code Coverage

Reviews

Conflicts