wallet: Create named SQLite wallet files instead of wallet directories

achow101 commented at 9:46 pm on October 28, 2020: member

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.

Update wallet_importprunedfunds to avoid dumpprivkey

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.

0bc23ef35e

Use a separate watchonly wallet in rpc_fundrawtransaction.py

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

90dfb1ff10

Make import tests in wallet_listtransactions.py legacy wallet only

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

2a1619b141

Avoid dumpprivkey in wallet_listsinceblock.py

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

f337bd8e51

Use importdescriptors for descriptor wallets in wallet_bumpfee.py

If using descriptor wallets, use importdescriptors instead of
importmulti.

93a59c652d

Move import and watchonly tests to be legacy wallet only in wallet_balance.py

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

6e0ad0405e

Use separate watchonly wallet for multisig in feature_nulldummy.py

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.

11ae325b33

Add descriptor wallet output to tool_wallet.py

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

f27138a342

Add script equivalent of functions in address.py 6b988335be

Avoid dumpprivkey and watchonly behavior in rpc_signrawtransaction.py

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.

de5ec3fcd2

Use importdescriptors when in descriptor wallet mode in wallet_createwallet.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.

1d54a4a9fa

Do addmultisigaddress tests in legacy wallet mode in wallet_address_types.py

addmultisigaddress is not available in descriptor wallets, so only run
these when testing legacy wallets

9ece42aa5b

Make raw multisig tests legacy wallet only in rpc_rawtransaction.py

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

e83a07688a

Ensure a legacy wallet for BDB format check e4c9e4c7a0

tests: Add a --legacy-wallet that is mutually exclusive with --descriptors

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.

39ec3201bf

Update wallet_labels.py to not require descriptors=False e8e49d4384

Disable some tests for tool_wallet when descriptors

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

1b4999702f

Update feature_backwards_compatibility for descriptor wallets 57b3bfbf76

Avoid creating legacy wallets in wallet_importdescriptors.py 20676bf432

Allow direct sqlite db

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

f0d058d2fb

Add wallet_util functions for checking file magic 26c3376d2d

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

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.

154c46ddff

Update feature_backwards_compatibility.py to handle single sqlite files b118f2d052

Use a helper function to get wallet file paths 80bd5e6e90

Change sqlite wallets to not use wallet directories

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

4b4885403d

ryanofsky commented at 10:55 pm on October 28, 2020: member

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 added the label Wallet on Oct 28, 2020

DrahtBot commented at 2:24 am on October 29, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20199 (Ignoring (but warn) on dublicate -wallet parameters by jonasschnelli)
#20186 (wallet: Make -wallet setting not create wallets by ryanofsky)
#20094 (wallet: Unify wallet directory lock error message by hebasto)
#19502 (Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr)
#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#19137 (wallettool: Add dump and createfromdump commands by achow101)
#19013 (test: add v0.20.1 to backwards compatibility test by Sjors)
#18788 (tests: Update more tests to work with descriptor wallets by achow101)
#18095 (Fix crashes and infinite loop in ListWalletDir() by uhliksk)
#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 commented at 4:53 am on October 29, 2020: member

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 commented at 5:05 am on October 29, 2020: member

@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 added the label Needs rebase on Oct 29, 2020

DrahtBot commented at 2:19 pm on October 29, 2020: member

🐙 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 commented at 6:54 pm on October 29, 2020: member

@ryanofsky makes a convincing argument. Closing.

achow101 closed this on Oct 29, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts