wallet: Fix wallet directory initialization #28514

BrandonOdiwuor · 2023-09-21T06:52:51Z

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 .

DrahtBot · 2023-09-21T06:52:54Z

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.

maflcko · 2023-09-21T11:03:33Z

Looks like the tests fail?

BrandonOdiwuor · 2023-09-26T10:09:53Z

@MarcoFalke not sure why this test is failing, since it passes locally

maflcko · 2023-09-27T07:43:47Z

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.

fanquake · 2023-10-02T10:59:31Z

cc @ryanofsky @achow101

ryanofsky · 2023-10-11T10:52:21Z

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:

bitcoin/src/qt/intro.cpp

Line 245 in bf32ae3

TryCreateDirectories(GUIUtil::QStringToPath(dataDir) / "wallets");

, 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 Multiprocess bitcoin #10102 WalletInit wallet/init.cpp functions are run in the node process, not the wallet process.

BrandonOdiwuor · 2023-10-17T06:00:41Z

Accidentally closed the PR

BrandonOdiwuor · 2023-10-19T10:25:39Z

@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

src/qt/intro.cpp

ryanofsky · 2023-10-20T15:17:52Z

test/functional/wallet_listtransactions.py

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

@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

ryanofsky · 2023-10-20T15:18:00Z

test/functional/wallet_reorgsrestore.py

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

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

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 bitcoin#16220

sedited

Concept ACK

DrahtBot · 2024-04-05T07:48:52Z

Could rebase for fresh CI, if still relevant?

achow101 · 2024-04-09T14:49:37Z

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.

DrahtBot added the CI failed label Sep 21, 2023

BrandonOdiwuor force-pushed the fix-wallet-dir-init branch 3 times, most recently from ac74173 to 3b7d0e9 Compare September 25, 2023 18:30

DrahtBot mentioned this pull request Sep 26, 2023

test: Use pathlib over os path #28392

Merged

BrandonOdiwuor force-pushed the fix-wallet-dir-init branch from 3b7d0e9 to 4544d03 Compare September 26, 2023 08:43

BrandonOdiwuor changed the title ~~Fix wallet directory initialization~~ Wallet: Fix wallet directory initialization Sep 26, 2023

BrandonOdiwuor changed the title ~~Wallet: Fix wallet directory initialization~~ wallet: Fix wallet directory initialization Sep 26, 2023

BrandonOdiwuor force-pushed the fix-wallet-dir-init branch 3 times, most recently from 4544d03 to 9c3b79e Compare September 27, 2023 11:28

DrahtBot added Wallet and removed CI failed labels Sep 27, 2023

0xETHengineer approved these changes Sep 27, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 29, 2023

Multiprocess bitcoin #10102

Draft

BrandonOdiwuor mentioned this pull request Oct 11, 2023

odd behaviour of GetDataDir creating wallets/ subdirectory #16220

Closed

DrahtBot added the Needs rebase label Oct 11, 2023

BrandonOdiwuor closed this Oct 17, 2023

BrandonOdiwuor force-pushed the fix-wallet-dir-init branch from 9c3b79e to 76d8957 Compare October 17, 2023 05:44

BrandonOdiwuor reopened this Oct 17, 2023

BrandonOdiwuor force-pushed the fix-wallet-dir-init branch from e37e60f to 6b4ad10 Compare October 17, 2023 06:09

DrahtBot added CI failed and removed Needs rebase labels Oct 17, 2023

BrandonOdiwuor force-pushed the fix-wallet-dir-init branch from 6b4ad10 to 17b4955 Compare October 18, 2023 13:48

DrahtBot removed the CI failed label Oct 18, 2023

0xETHengineer approved these changes Oct 19, 2023

View reviewed changes

ryanofsky reviewed Oct 20, 2023

View reviewed changes

BrandonOdiwuor force-pushed the fix-wallet-dir-init branch from 17b4955 to dfe62e9 Compare October 21, 2023 12:18

BrandonOdiwuor requested a review from ryanofsky October 21, 2023 12:23

DrahtBot added CI failed and removed CI failed labels Oct 21, 2023

sedited reviewed Oct 31, 2023

View reviewed changes

DrahtBot added the CI failed label Jan 17, 2024

DrahtBot mentioned this pull request Mar 13, 2024

scripted-diff: Use LogInfo over LogPrintf #29641

Merged

achow101 closed this Apr 9, 2024

bitcoin locked and limited conversation to collaborators Apr 9, 2025

wallet: Fix wallet directory initialization #28514

wallet: Fix wallet directory initialization #28514

Uh oh!

Conversation

BrandonOdiwuor commented Sep 21, 2023

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented Sep 21, 2023

Uh oh!

BrandonOdiwuor commented Sep 26, 2023

Uh oh!

maflcko commented Sep 27, 2023

Uh oh!

fanquake commented Oct 2, 2023

Uh oh!

ryanofsky commented Oct 11, 2023

Uh oh!

BrandonOdiwuor commented Oct 17, 2023

Uh oh!

BrandonOdiwuor commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanofsky Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 5, 2024

Uh oh!

achow101 commented Apr 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Sep 21, 2023 •

edited

Loading

BrandonOdiwuor commented Oct 19, 2023 •

edited

Loading