Make GUI and CLI tools use the same datadir #27409

pull ryanofsky wants to merge 7 commits into bitcoin:master from ryanofsky:pr/1data changing 19 files +644 −115
  1. 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.conf1 which CLI tools ignore. This PR changes the GUI to instead store the datadir path at the default datadir location ~/.bitcoin2 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.


    1. strDataDir value is stored in .config/Bitcoin/Bitcoin-Qt.conf on linux, in property list files on macos, and in registry keys on windows. ↩︎

    2. The default datadir location is ~/.bitcoin on linux, ~/Library/Application Support/Bitcoin on macos, and %APPDATA%\Bitcoin on windows ↩︎

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

  3. 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
  4. DrahtBot added the label Needs rebase on Apr 3, 2023
  5. 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
  6. 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
  7. 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
  8. Merge remote-tracking branch 'origin/pull/27302/head' 8d777c0670
  9. 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
  10. 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
  11. 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
  12. ryanofsky force-pushed on Apr 12, 2023
  13. DrahtBot removed the label Needs rebase on Apr 12, 2023
  14. pinheadmz commented at 11:21 am on April 15, 2023: member
  15. DrahtBot commented at 10:23 am on April 21, 2023: contributor

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

  16. DrahtBot added the label Needs rebase on Apr 21, 2023
  17. 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?
  18. 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?

  19. 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
  20. 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.
  21. 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:
  22. 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.
  23. maflcko assigned ryanofsky on Jan 2, 2024
  24. 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.
  25. 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.
  26. maflcko commented at 9:16 am on June 28, 2024: member
    @ryanofsky What is the status of this?
  27. 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.

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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me