wallet: disallow creating new or restoring to an unnamed (default) wallet #34269
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34269. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
test/functional/wallet_startup.py
Outdated
|
|
||
| self.log.info('New default wallet should load by default when there are no other wallets') | ||
| self.nodes[0].createwallet(wallet_name='', load_on_startup=False) | ||
| self.nodes[0].create_unnamed_wallet(load_on_startup=False) |
There was a problem hiding this comment.
why no just add a name to the wallet?
There was a problem hiding this comment.
This test is specifically regarding the unnamed wallet (referred to as the default wallet) and loading it automatically on startup if it's there by itself.
There was a problem hiding this comment.
Maybe worth a comment to make it easy to understand if you lack context?
There was a problem hiding this comment.
I think there's enough context if you hover over the create_unnamed_wallet helper? Or is there something else about this test that needs clarifying?
There was a problem hiding this comment.
Yes what create_unnamed_wallet does is clear, what is not clear is that the test needs an unnamed wallet. Because without context it is not clear that default wallet means unnamed.
That lack of context is what made me ask why not just add a name to the wallet.
Something like this might be enough:
self.log.info('New default (unnamed) wallet should load by default when there are no other wallets')There was a problem hiding this comment.
Because without context it is not clear that
defaultwallet meansunnamed.
Default wallet has always meant the unnamed wallet.
Use the utility function HandleWalletError to deal with wallet creation errors in createwallet.
5e3d561 to
1261089
Compare
There was a problem hiding this comment.
code lgtm tested ACK 1261089
Prior to this PR you could create or restore a wallet using an empty name "", now you cannot and it fails returning an error code.
$ ./build/bin/bitcoin-cli createwallet ""
error code: -8
error message:
Wallet name cannot be empty
$ ./build/bin/bitcoin-wallet -wallet='' create
Wallet name cannot be empty
test/functional/wallet_startup.py
Outdated
|
|
||
| self.log.info('New default wallet should load by default when there are no other wallets') | ||
| self.nodes[0].createwallet(wallet_name='', load_on_startup=False) | ||
| self.nodes[0].create_unnamed_wallet(load_on_startup=False) |
There was a problem hiding this comment.
Maybe worth a comment to make it easy to understand if you lack context?
|
utACK 1261089 |
| const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); | ||
|
|
||
| if (command == "create") { | ||
| if (name.empty()) { |
There was a problem hiding this comment.
Is this code reachable considering the condition already being checked above if ((command == "create" || command == "createfromdump") && !args.IsArgSet("-wallet")) { ?
There was a problem hiding this comment.
Yes, -wallet= is a valid way to set -wallet
There was a problem hiding this comment.
ACK 1261089
Tested and reviewed the code lgtm.
Now restorewallet does not work with empty name "".
$ /bitcoin-cli restorewallet "" walletbackup.dat
error code: -8
error message:
Wallet name cannot be empty
We've been moving in the direction that all wallets must have a name. Therefore, we shouldn't allow creating new unnamed wallets.
createwallet,restorewallet, and the wallet tool'screateandcreatefromdumpall now require the user to provide a non-empty wallet name when creating/restoring a wallet.The GUI is already enforcing this, but we were not enforcing it for RPCs or in the underlying
CreateWalletandRestoreWalletfunctions.Wallet migration does still need to be able to restore unnamed wallets, so there is a new argument to
RestoreWalletto explicitly allow that behavior for migration only.