wallet: Fix wallet directory initialization #28514

pull BrandonOdiwuor wants to merge 1 commits into bitcoin:master from BrandonOdiwuor:fix-wallet-dir-init changing 9 files +83 −22
  1. BrandonOdiwuor commented at 6:52 am on September 21, 2023: contributor

    Fixes #16220, see #16220 (comment)

    • util/system datadir code should not care about wallets or look for or create any wallet paths
    • If -walletdir is specified, wallet code should use that directory to locate and list wallets.
    • If -walletdir is specified and path is not a directory, wallet init should return a fatal error and refuse to start
    • If -walletdir is not specified, wallet code should use /wallets as the directory to locate and list wallets.
    • If -walletdir is not specified and /wallets does not exist, wallet init code should create it
      • Before creating /wallets wallet should call ListWalletDir ListDatabases on
      • If list is empty wallet should create /wallets as a directory, otherwise it should create it as a symlink pointing to .
  2. DrahtBot commented at 6:52 am on September 21, 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.

    Type Reviewers
    Concept ACK TheCharlatan

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

    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. DrahtBot added the label CI failed on Sep 21, 2023
  4. maflcko commented at 11:03 am on September 21, 2023: member
    Looks like the tests fail?
  5. BrandonOdiwuor force-pushed on Sep 21, 2023
  6. BrandonOdiwuor force-pushed on Sep 25, 2023
  7. BrandonOdiwuor force-pushed on Sep 25, 2023
  8. BrandonOdiwuor force-pushed on Sep 26, 2023
  9. BrandonOdiwuor renamed this:
    Fix wallet directory initialization
    Wallet: Fix wallet directory initialization
    on Sep 26, 2023
  10. BrandonOdiwuor renamed this:
    Wallet: Fix wallet directory initialization
    wallet: Fix wallet directory initialization
    on Sep 26, 2023
  11. BrandonOdiwuor commented at 10:09 am on September 26, 2023: contributor
    @MarcoFalke not sure why this test is failing, since it passes locally Screenshot from 2023-09-26 13-07-20
  12. maflcko commented at 7:43 am on September 27, 2023: member
    The CI task that failed is named “no wallet”. This means, to reproduce the failure, you will have to compile without a wallet and run the test.
  13. BrandonOdiwuor force-pushed on Sep 27, 2023
  14. BrandonOdiwuor force-pushed on Sep 27, 2023
  15. BrandonOdiwuor force-pushed on Sep 27, 2023
  16. DrahtBot removed the label CI failed on Sep 27, 2023
  17. DrahtBot added the label Wallet on Sep 27, 2023
  18. 0xETHengineer approved
  19. fanquake commented at 10:59 am on October 2, 2023: member
    cc @ryanofsky @achow101
  20. ryanofsky commented at 10:52 am on October 11, 2023: contributor

    This seems promising, but it isn’t exactly implementing what’s described in #16220 (comment) or the PR description so it has a different set of advantages and disadvantages.

    The difference between code and PR description is that code is not implementing: “If list is empty wallet should create <datadir>/wallets as a directory, otherwise it should create it as a symlink pointing to .”. Because of this, the PR can’t simplify the GetWalletDir function so it still needs to check existence <datadir>/wallets and can’t just return it unconditionally.

    Maybe this is ok, and the PR is still worthwhile, because it at least it simplifies the node code, even if it doesn’t simplify the wallet code, and it makes the separation cleaner because node code no longer explicitly needs to create the “wallets” directory and can avoid creating it if the wallet is disabled.

    Another consequence of not creating "wallets" -> "." symlink is that startup might be a little slower in a data directory that has wallets in the top level, since ListDatabases().empty() will be checked every time on startup, instead of just one time when symlink doesn’t exist.

    Other notes about the PR:

    • It is suspicious why wallet functional tests need to be changed. This change should be backwards compatible so it is surprising that tests should have to change in order to pass. It would be good to have an explanation for that.
    • I’m not sure if we currently have a test explicitly written to cover loading a datadir with no wallets/ subdirectory and wallets in the top-level directory. If there is not an existing test, probably one should be written to make sure code in this PR doesn’t break and wallets can still be loaded there.
    • This PR isn’t removing qt code that creates the “wallets” directory: https://github.com/bitcoin/bitcoin/blob/bf32ae3969833c767b2c88d4c7f609a569c7effe/src/qt/intro.cpp#L245, which should no longer be necessary, after the other changes.
    • I think it would be better if this PR did not add a g_wallet_init_interface.InitWalletDir function, and instead just changed existing VerifyWallets function which is already determining the wallet directory. Getting rid of it would be nicer just to have fewer init functions, but also this code doesn’t belong in WalletInit class because with #10102 WalletInit wallet/init.cpp functions are run in the node process, not the wallet process.
  21. DrahtBot added the label Needs rebase on Oct 11, 2023
  22. BrandonOdiwuor closed this on Oct 17, 2023
  23. BrandonOdiwuor force-pushed on Oct 17, 2023
  24. BrandonOdiwuor commented at 6:00 am on October 17, 2023: contributor
    Accidentally closed the PR
  25. BrandonOdiwuor reopened this on Oct 17, 2023
  26. BrandonOdiwuor force-pushed on Oct 17, 2023
  27. DrahtBot added the label CI failed on Oct 17, 2023
  28. DrahtBot removed the label Needs rebase on Oct 17, 2023
  29. BrandonOdiwuor force-pushed on Oct 18, 2023
  30. DrahtBot removed the label CI failed on Oct 18, 2023
  31. 0xETHengineer approved
  32. BrandonOdiwuor commented at 10:25 am on October 19, 2023: contributor

    @ryanofsky

    I have updated the PR and moved the wallet directory initialization code to VerifyWallets()

    I have also added tests for the various init scenarios and removed the remaining TryCreateDirectories from QT

    The tests changed in this PR are a reflection of the changes in the logic of the wallet dir init code as suggested, such as the wallet/ dir currently being created even if the data dir is not empty or the mainnet wallet/ dir only being created when running mainnet

    I removed the symlink code (see screenshot attached) since it makes tests in wallet_multiwallet.py fail

    Screenshot from 2023-10-18 16-46-18

  33. in src/qt/intro.cpp:252 in 17b4955e4b outdated 
    247-                break;
248-            } catch (const fs::filesystem_error&) {
249-                QMessageBox::critical(nullptr, PACKAGE_NAME,
250-                    tr("Error: Specified data directory \"%1\" cannot be created.").arg(dataDir));
251-                /* fall through, back to choosing screen */
252-            }
    ryanofsky commented at 3:16 pm on October 20, 2023:

    In commit “Fix wallet directory initialization” (17b4955e4b1e4f02f00a948b432f18caa9b250a5)

    Is it a mistake to delete this whole block of code? I’d expect only line 245 (TryCreateDirectories("wallets")) to be deleted

    BrandonOdiwuor commented at 12:23 pm on October 21, 2023:
    fixed
  34. in test/functional/wallet_listtransactions.py:237 in 17b4955e4b outdated 
    233@@ -234,8 +234,8 @@ def run_externally_generated_address_test(self):
234         # refill keypool otherwise the second node wouldn't recognize addresses generated on the first nodes
235         self.nodes[0].keypoolrefill(1000)
236         self.stop_nodes()
237-        wallet0 = os.path.join(self.nodes[0].chain_path, self.default_wallet_name, "wallet.dat")
    ryanofsky commented at 3:17 pm on October 20, 2023:

    In commit “Fix wallet directory initialization” (17b4955e4b1e4f02f00a948b432f18caa9b250a5)

    Is there a reason this test is changing? I’d expect the change to be backwards compatible so existing tests should not need to change

    BrandonOdiwuor commented at 12:22 pm on October 21, 2023:
    @ryanofsky This is because of the change in wallet dir init logic. The wallet/ dir is created when a new node is started and no wallet is detected in data_dir
  35. in test/functional/wallet_reorgsrestore.py:91 in 17b4955e4b outdated 
    87@@ -88,7 +88,7 @@ def run_test(self):
88         # Node0 wallet file is loaded on longest sync'ed node1
89         self.stop_node(1)
90         self.nodes[0].backupwallet(self.nodes[0].datadir_path / 'wallet.bak')
91-        shutil.copyfile(self.nodes[0].datadir_path / 'wallet.bak', self.nodes[1].chain_path / self.default_wallet_name / self.wallet_data_filename)
    ryanofsky commented at 3:18 pm on October 20, 2023:

    In commit “Fix wallet directory initialization” (17b4955e4b1e4f02f00a948b432f18caa9b250a5)

    Is there a reason this test is changing? I’d expect the change to be backwards compatible so existing tests should not need to change

    BrandonOdiwuor commented at 12:23 pm on October 21, 2023:
    This is also because of the change in the wallet dir init logic. The wallet/ dir is created when a new node is started and no wallet is detected in data_dir
  36. Fix wallet directory initialization 
    util/system datadir code should not care about wallets or create any wallet paths
If -walletdir is specified, wallet init code should use that directory
If -walletdir is specified and path is not a directory, return a fatal error
If -walletdir is not specified, use <datadir>/wallets if it is directory.
If -walletdir is not specified and <datadir>/wallets does not exist:
<datadir>/wallets wallet should call ListWalletDir ListDatabases on <datadir>
If list is empty wallet should create <datadir>/wallets as a directory,
otherwise it should create it as a symlink pointing to .
discussion at https://github.com/bitcoin/bitcoin/issues/16220
    dfe62e9acc
  37. BrandonOdiwuor force-pushed on Oct 21, 2023
  38. BrandonOdiwuor requested review from ryanofsky on Oct 21, 2023
  39. DrahtBot added the label CI failed on Oct 21, 2023
  40. DrahtBot removed the label CI failed on Oct 25, 2023
  41. TheCharlatan commented at 2:17 pm on October 31, 2023: contributor
    Concept ACK
  42. DrahtBot added the label CI failed on Jan 17, 2024
  43. DrahtBot commented at 7:48 am on April 5, 2024: contributor
    Could rebase for fresh CI, if still relevant?
  44. achow101 commented at 2:49 pm on April 9, 2024: member

    The PR didn’t seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs.

    Closing due to lack of interest.

  45. achow101 closed this on Apr 9, 2024


BrandonOdiwuor DrahtBot maflcko 0xETHengineer fanquake ryanofsky TheCharlatan achow101


ryanofsky

Labels
Wallet CI failed

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-09-20 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me