wallet: Create named SQLite wallet files instead of wallet directories #20260

achow101 · 2020-10-28T21:46:32Z

Wallet directories were introduced because BDB has issues with shared database environments. Since SQLite does not have database environments it is not necessary to have these directories. So instead, we go back to the original multiwallet plan of having just wallet files named with the desired wallet name. Note that this configuration is technically supported with BDB wallets although new wallets would not be created like this. For SQLite, this is both supported and the way that wallets are created.

Depends on #18788 for tests.

Removes the use of dumpprivkey so that descriptor wallets can pass on this. Also does a few descriptor wallet specific changes due to different IsMine semantics.

Import things into a separate watchonly wallet to work with descriptor wallets.

Existing import* RPCs are disabled for descriptor wallets, so only do these tests for legacy wallets.

Generate a privkey in the test framework instead of using dumpprivkey so that descriptor wallets work in this test.

If using descriptor wallets, use importdescriptors instead of importmulti.

…lance.py Imports and watchonly behavior are legacy wallet only, so make them only run when the test is in legacy wallet mode.

Create and import the multisig into a separate watchonly wallet so that feature_nulldummy.py works with descriptor wallets. blocktools.create_raw_transaction is also updated to use multiple nodes and wallets and to use PSBT so that this test passes.

Descriptor wallets output slightly different information in the wallet tool, so check that output when in descriptor wallet mode.

dumpprivkey and watchonly behavior don't work with descriptor wallets. Test for multisigs is modified to not rely on watchonly behavior for those multisigs. This has a side effect of removing listunspent, but that's not the target of this test, so that's fine.

…wallet.py sethdseed and importmulti are not available for descriptor wallets, so when doing descriptor wallet tests, use importdescriptors instead. Also changes some output to match what descriptor wallets will return.

…ypes.py addmultisigaddress is not available in descriptor wallets, so only run these when testing legacy wallets

The traditional multisig workflow doesn't work with descriptor wallets so make these tests legacy wallet only.

…ptors Although legacy wallet is still the default, for future use, add a --legacy-wallet option to the test framework. Additional tests for descriptor wallets have been enabled with the --descriptors option. Tests that must be legacy wallet only are being started with --legacy-wallet. Even though this option does not currently do anything, this will be helpful in the future when descriptor wallets become the default.

Some tests are legacy wallet only (and make legacy wallets) so they shouldn't be run when doing descriptor wallet tests.

Like with BDB, we allow users to specify the wallet file directly, rather than just a wallet dir name.

When --dsecriptors, we expect the wallets to be descriptor wallets, and the reverse when not (or --legacy-wallet). So updated wallet_multiwallet.py to do that.

It isn't necessary to have wallet directories for sqlite wallets, so we should just make single files in the wallets dir.

ryanofsky · 2020-10-28T22:55:29Z

I think wallets should keep being directories. Few reasons:

Aesthetics, usability, and the road to ambiguity. People are used to file names having extensions, and directory names not having extensions. If you create wallet named "My Wallet" in the GUI, or run bitcoin-cli createwallet "My Wallet", there's nothing unusual about seeing a directory called "My Wallet" in <walletdir>. If "My Wallet" is created as a file not a directory, then you have an unusual looking file with no extension. So then there is a reason to call bitcoin-cli createwallet with a name that looks more like a filename: mywallet.dat, mywallet.db mywallet.wallet. And then the GUI starts showing lists of wallets like mywallet1.wallet and mywallet2.wallet and RPC endpoints looks like /wallet/mywallet1.wallet and /wallet/mywallet2.wallet. And then to improve ergonomics, somebody writes a PR to strip extensions, so GUI is more user-friendly or so RPC endpoint names are less verbose. And then simple mapping of wallet name == RPC endpoint name == GUI display name is broken, so now there are multiple mappings, ambiguity, more confusion, and everything is more complicated and stupid.
Backup safety. The claim that "Since SQLite does not have database environments it is not necessary to have these directories" is ambiguous and not accurate as far as I know. The sqlite implementation does not require periodic flushing to consolidate log files, but that doesn't mean that sqlite doesn't create log files. If you create a <walletname> wallet, sqlite will create a <walletname>.wallet-journal file, so if you have a backup script that is periodically backing up the <walletdir>/<walletname> path, it can miss the journal file. If the backup script wants to include the journal file, it needs to know about wallet database layout. It is better if wallet is just a directory, so the backup script can back up the directory without having to care about database layout. If script wants to care about the layout, it can still do that and exclude things, but the simple non-invasive approach will include data instead of excluding it.
Future extensibility. It's nice if wallets are directories instead of files, so there is a place to add new files associated with the wallet. I have PRs that let wallets run as separate processes. These wallet processes will want to run RPC servers. These RPC servers will need a place to put cookie files. A logical place would be inside the wallet directory. Similarly with future per-wallet debug logs, per-wallet settings files, per-wallet socket endpoints. If you have wallet directory, there is a natural place to put all these things. Users and external tools will be able to locate all data associated with a wallet without having to make assumptions about layout.

DrahtBot · 2020-10-29T02:24:33Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: ignore (but warn) on duplicate -wallet parameters #20199 (Ignoring (but warn) on dublicate -wallet parameters by jonasschnelli)
wallet: Make -wallet setting not create wallets #20186 (wallet: Make -wallet setting not create wallets by ryanofsky)
wallet: Unify wallet directory lock error message #20094 (wallet: Unify wallet directory lock error message by hebasto)
Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks #19502 (Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr)
Refactor: Improve setup_clean_chain semantics #19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013 (test: add v0.20.1 to backwards compatibility test by Sjors)
tests: Update more tests to work with descriptor wallets #18788 (tests: Update more tests to work with descriptor wallets by achow101)
Fix crashes and infinite loop in ListWalletDir() #18095 (Fix crashes and infinite loop in ListWalletDir() by uhliksk)
External signer support - Wallet Box edition #16546 (External signer support - Wallet Box edition by Sjors)

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.

hebasto · 2020-10-29T04:53:33Z

I agree with @ryanofsky's reasons, especially with "Future extensibility" and "Backup safety".

As for wallet names that looks like <filename><dot><extension>, they could cause user confusion and fund loss because some OSes hide extensions from users.

sipa · 2020-10-29T05:05:08Z

@ryanofsky I considered being able to move to non-directory wallets an advantage of sqlite, but you give good arguments for sticking with directories.

DrahtBot · 2020-10-29T14:19:13Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

achow101 · 2020-10-29T18:54:44Z

@ryanofsky makes a convincing argument. Closing.

achow101 added 25 commits October 28, 2020 16:48

Update wallet_importprunedfunds to avoid dumpprivkey

0bc23ef

Removes the use of dumpprivkey so that descriptor wallets can pass on this. Also does a few descriptor wallet specific changes due to different IsMine semantics.

Use a separate watchonly wallet in rpc_fundrawtransaction.py

90dfb1f

Import things into a separate watchonly wallet to work with descriptor wallets.

Make import tests in wallet_listtransactions.py legacy wallet only

2a1619b

Existing import* RPCs are disabled for descriptor wallets, so only do these tests for legacy wallets.

Avoid dumpprivkey in wallet_listsinceblock.py

f337bd8

Generate a privkey in the test framework instead of using dumpprivkey so that descriptor wallets work in this test.

Use importdescriptors for descriptor wallets in wallet_bumpfee.py

93a59c6

If using descriptor wallets, use importdescriptors instead of importmulti.

Move import and watchonly tests to be legacy wallet only in wallet_ba…

6e0ad04

…lance.py Imports and watchonly behavior are legacy wallet only, so make them only run when the test is in legacy wallet mode.

Add descriptor wallet output to tool_wallet.py

f27138a

Descriptor wallets output slightly different information in the wallet tool, so check that output when in descriptor wallet mode.

Add script equivalent of functions in address.py

6b98833

Do addmultisigaddress tests in legacy wallet mode in wallet_address_t…

9ece42a

…ypes.py addmultisigaddress is not available in descriptor wallets, so only run these when testing legacy wallets

Make raw multisig tests legacy wallet only in rpc_rawtransaction.py

e83a076

The traditional multisig workflow doesn't work with descriptor wallets so make these tests legacy wallet only.

Ensure a legacy wallet for BDB format check

e4c9e4c

Update wallet_labels.py to not require descriptors=False

e8e49d4

Disable some tests for tool_wallet when descriptors

1b49997

Some tests are legacy wallet only (and make legacy wallets) so they shouldn't be run when doing descriptor wallet tests.

Update feature_backwards_compatibility for descriptor wallets

57b3bfb

Avoid creating legacy wallets in wallet_importdescriptors.py

20676bf

Allow direct sqlite db

f0d058d

Like with BDB, we allow users to specify the wallet file directly, rather than just a wallet dir name.

Add wallet_util functions for checking file magic

26c3376

tests: have wallet_multiwallet.py actually make the correct wallet types

154c46d

When --dsecriptors, we expect the wallets to be descriptor wallets, and the reverse when not (or --legacy-wallet). So updated wallet_multiwallet.py to do that.

Update feature_backwards_compatibility.py to handle single sqlite files

b118f2d

Use a helper function to get wallet file paths

80bd5e6

Change sqlite wallets to not use wallet directories

4b48854

It isn't necessary to have wallet directories for sqlite wallets, so we should just make single files in the wallets dir.

DrahtBot added the Wallet label Oct 28, 2020

This was referenced Oct 29, 2020

wallet: ignore (but warn) on duplicate -wallet parameters #20199

Merged

wallet: Make -wallet setting not create wallets #20186

Merged

This was referenced Oct 29, 2020

wallet: Unify wallet directory lock error message #20094

Closed

Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks #19502

Merged

This was referenced Oct 29, 2020

Refactor: Improve setup_clean_chain semantics #19168

Closed

wallettool: Add dump and createfromdump commands #19137

Merged

DrahtBot added the Needs rebase label Oct 29, 2020

achow101 closed this Oct 29, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Create named SQLite wallet files instead of wallet directories #20260

wallet: Create named SQLite wallet files instead of wallet directories #20260

Uh oh!

achow101 commented Oct 28, 2020

Uh oh!

ryanofsky commented Oct 28, 2020

Uh oh!

DrahtBot commented Oct 29, 2020

Uh oh!

hebasto commented Oct 29, 2020

Uh oh!

sipa commented Oct 29, 2020

Uh oh!

DrahtBot commented Oct 29, 2020

Uh oh!

achow101 commented Oct 29, 2020

Uh oh!

Uh oh!

wallet: Create named SQLite wallet files instead of wallet directories #20260

wallet: Create named SQLite wallet files instead of wallet directories #20260

Uh oh!

Conversation

achow101 commented Oct 28, 2020

Uh oh!

ryanofsky commented Oct 28, 2020

Uh oh!

DrahtBot commented Oct 29, 2020

Conflicts

Uh oh!

hebasto commented Oct 29, 2020

Uh oh!

sipa commented Oct 29, 2020

Uh oh!

DrahtBot commented Oct 29, 2020

Uh oh!

achow101 commented Oct 29, 2020

Uh oh!

Uh oh!