wallet, tests: Expand and test when the blank wallet flag should be un/set #25634
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
7e297eb to
4c936c8
Compare
|
Concept ACK |
| 8000 assert_equal(expected, wallet.listdescriptors()) | ||
| assert expected["descriptors"][0] in wallet.listdescriptors()["descriptors"] | ||
|
|
||
| self.log.info('Test list private descriptors with encrypted wallet') | ||
| assert_raises_rpc_error(-13, 'Please enter the wallet passphrase with walletpassphrase first.', wallet.listdescriptors, True) | ||
| wallet.walletpassphrase(passphrase="pass", timeout=1000000) | ||
| assert_equal(expected_private, wallet.listdescriptors(True)) | ||
| assert expected_private["descriptors"][0] in wallet.listdescriptors(True)["descriptors"] |
There was a problem hiding this comment.
In 44e30c5 "wallet: Ensure that the blank wallet flag is unset after imports"
It is not clear to me how the other changes in this PR result in this behavior change. Especially since it appears to only be happening once a wallet is encrypted. After the changes in this PR, running listdescriptors before a wallet is encrypted produces drastically different results than when it is run afterwards.
listdescriptors before the wallet is encrypted:
{'wallet_name': 'w2', 'descriptors': [{'desc': "wpkh([80002067/84'/1'/0']tpubDCMVLhErorrAGfApiJSJzEKwqeaf2z3NrkVMxgYQjZLzMjXMBeRw2muGNYbvaekAE8rUFLftyEar4LdrG2wXyyTJQZ26zptmeTEjPTaATts/0/*)#yz2uu6mw", 'timestamp': 1296688602, 'active': False, 'range': [0, 0], 'next': 0}]}
listdescriptors after the wallet is encrypted:
{'wallet_name': 'w2', 'descriptors': [{'desc': "wpkh([80002067/84'/1'/0']tpubDCMVLhErorrAGfApiJSJzEKwqeaf2z3NrkVMxgYQjZLzMjXMBeRw2muGNYbvaekAE8rUFLftyEar4LdrG2wXyyTJQZ26zptmeTEjPTaATts/0/*)#yz2uu6mw", 'timestamp': 1296688602, 'active': False, 'range': [0, 0], 'next': 0}, {'desc': "pkh([5dc1e209/44'/1'/0']tpubDDfjWivrLDGEU679nu8YgTDPUALX9bUppvdXXk19TDAaecSNxMH8vPZBeRRjaDqKUYgEd3UzZWS4rt5qsAWQW2qktupQZR1MnSnAZ7L85Cn/0/*)#rx0cg4z2", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "wpkh([5dc1e209/84'/1'/0']tpubDD42XKtjCmM9u18pwcFj5VNPoh4QZn6fDuHq5ypo5ZvZrsMguRRd5YvDjKX6BLKz393V9QpeouE95eHeaugv3FM6LVqTo9f2phTPQvw65xy/0/*)#q3r4f8hn", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "pkh([5dc1e209/44'/1'/0']tpubDDfjWivrLDGEU679nu8YgTDPUALX9bUppvdXXk19TDAaecSNxMH8vPZBeRRjaDqKUYgEd3UzZWS4rt5qsAWQW2qktupQZR1MnSnAZ7L85Cn/1/*)#jj2e4qjj", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}, {'desc': "sh(wpkh([5dc1e209/49'/1'/0']tpubDCVSqsf6kfUFDo28Vq3Lsrt9FXVqtohxWFxufkxubWT4Kh9z5RfmcxZmgKPBsWvboazFWNqnA9YHmQNrtfCvxTU8mch4yzxcMshqCWXtt9u/0/*))#8s9y4eu7", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "tr([5dc1e209/86'/1'/0']tpubDDMfjHg7VtGGjDiXv3ohgm7NTpm6ue5XnXdJBx9NZ8PgrSdLAQ1DjnYnxNiQKJcB6ZaoQo7WjZo7iTXVzdBgW38KM5BRsJJzXtDUSSjehRU/0/*)#60q6ankn", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "tr([5dc1e209/86'/1'/0']tpubDDMfjHg7VtGGjDiXv3ohgm7NTpm6ue5XnXdJBx9NZ8PgrSdLAQ1DjnYnxNiQKJcB6ZaoQo7WjZo7iTXVzdBgW38KM5BRsJJzXtDUSSjehRU/1/*)#tm9mqxxt", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}, {'desc': "sh(wpkh([5dc1e209/49'/1'/0']tpubDCVSqsf6kfUFDo28Vq3Lsrt9FXVqtohxWFxufkxubWT4Kh9z5RfmcxZmgKPBsWvboazFWNqnA9YHmQNrtfCvxTU8mch4yzxcMshqCWXtt9u/1/*))#j3tjdxfp", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}, {'desc': "wpkh([5dc1e209/84'/1'/0']tpubDD42XKtjCmM9u18pwcFj5VNPoh4QZn6fDuHq5ypo5ZvZrsMguRRd5YvDjKX6BLKz393V9QpeouE95eHeaugv3FM6LVqTo9f2phTPQvw65xy/1/*)#39x55j8t", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}]}
There was a problem hiding this comment.
Yes. This seems like a bug. I was able to trace it back to encryption function
src/wallet/wallet.cpp:829
// If we are using descriptors, make new descriptors with a new seed
if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
SetupDescriptorScriptPubKeyMans();
@achow101 why do we want to regenerate descriptors on encryption?
There was a problem hiding this comment.
Hmm, yes, I'm not sure whether this is a bug as it the behavior is (mostly) expected.
We always want to rotate keys after encryption because the old keys were written to disk unencrypted and thus may have been compromised. There may also be backups of the unencrypted keys. So if the user has opted into encryption, we want to make sure any keys they use from then on were born encrypted and known to not have been stored anywhere unencrypted. We do the same for the legacy wallet as well.
The question here is whether this behavior is expected for formerly blank wallets.
There was a problem hiding this comment.
I personally wouldn't expect to see new descriptors after encrypting wallet with an imported descriptor. But I also understand why rotating descriptors is generally good.
I don't think distinction of "formerly blank wallets" is good enough. The same problem can happen with non-blank wallet. Should we rotate user imported descriptors in non-blank wallets? The current code would rotate it as well.
It seems like we need to track which descriptors are automatically generated and which are manually imported. Of course, the problem is that we can't backfill the information.
There was a problem hiding this comment.
Should we rotate user imported descriptors in non-blank wallets? The current code would rotate it as well.
Yes. This is considered an important aspect of encryption as it ensures that addresses used afterwards did not have their private keys written unencrypted. It is also a well documented behavior in both the RPC and GUI.
There was a problem hiding this comment.
This is considered an important aspect of encryption as it ensures that addresses used afterwards did not have their private keys written unencrypted.
This makes sense, but in this case shouldn't new private keys only be generated for the descriptors that already exist in the wallet (imported or automatically generated) instead of adding additional descriptors to the wallet that the user did not ask for? Even if rotating keys upon encryption is well documented, adding additional descriptors does not appear to be.
There was a problem hiding this comment.
This makes sense, but in this case shouldn't new private keys only be generated for the descriptors that already exist in the wallet (imported or automatically generated) instead of adding additional descriptors to the wallet that the user did not ask for? Even if rotating keys upon encryption is well documented, adding additional descriptors does not appear to be.
This can get rather complicated since users can import descriptors that don't match the same pattern as our automatically generated ones, and producing ones that look similar except for the keys might be more confusing than ones that are altogether different? At least for non-single key things, it would be very obvious that the descriptor has changed.
This is also perhaps an argument for always encrypting wallets with a default passphrase which would completely sidestep that issue.
There was a problem hiding this comment.
Seeing as most of the solutions to this issue/confusion are likely out of the scope of this PR, I think that these changes should be fine with the addition of documentation about this behavior change related to importing descriptors before encrypting a wallet, since previously no new descriptors would be added.
There was a problem hiding this comment.
Change to unset the blank flag when writing a descriptor is now reverted, so I think this makes BLANK flag documentation misleading, and could be fixed with:
--- a/src/wallet/walletutil.h
+++ b/src/wallet/walletutil.h
@@ -53,10 +53,18 @@ enum WalletFlags : uint64_t {
//! Flag set when a wallet contains no HD seed and no private keys, scripts,
//! addresses, and other watch only things, and is therefore "blank."
//!
- //! The only function this flag serves is to distinguish a blank wallet from
+ //! The main function this flag serves is to distinguish a blank wallet from
//! a newly created wallet when the wallet database is loaded, to avoid
//! initialization that should only happen on first run.
//!
+ //! A secondary function of this flag, which applies to descriptor wallets
+ //! only, is to serve as an ongoing indication that descriptors in the
+ //! wallet should be created manually, and that the wallet should not
+ //! generate automatically generate new descriptors if it is later
+ //! encrypted. To support this behavior, descriptor wallets unlike legacy
+ //! wallets do not automatically unset the BLANK flag when things are
+ //! imported.
+ //!
//! This flag is also a mandatory flag to prevent previous versions of
//! bitcoin from opening the wallet, thinking it was newly created, and
//! then improperly reinitializing it.
There was a problem hiding this comment.
I've added a commit with this comment.
|
Thinking about this I've got a question: |
No, that's how we determine whether the wallet was just created. Without the blank flag, we don't have a way of distinguishing between "blank" and "newly created". |
Hmm, wouldn't be sufficient to differentiate blank from newly created wallets by providing a flag to the Sounds pretty odd this blank flag storage requirement. We could just add a runtime parameter that says "if we are loading any wallet from disk, don't create any new seed nor new SPKMs, just load the file into a wallet". Then, only if the context is the |
Going forward, probably. But we still need the flag for downgrade protection. Older software determines whether a wallet is new by observing that there's nothing in it. So if a blank wallet (without the flag) were loaded into the older software, it would think the wallet was just created and generate a new seed for it. Since the flag is one of the mandatory flags, older software that see that flag (or the version number that added support for the flag) will not load or modify the wallet. We unset the flag once the wallet contains things because it is no longer needed for downgrade protection. Once the wallet has keys or descriptors, older software will not automatically generate anything for that wallet. The other reason it's implemented this way is because at the time it was introduced, we would create a wallet specified with Perhaps there needs to be some distinction between "this wallet needs to start out without any automatically generated things" and "this wallet must only have things the user adds explicitly". The original reason blank was added was to support born-encrypted wallets. It was a way to make sure that a newly created wallet would not have any initial keys so that the user (or the creation process itself) can encrypt the wallet and generate keys automatically after encryption. However some users probably want to have wallets where the only things in their wallets are things that are added explicitly, e.g. only imports, or only |
4c936c8 to
3931196
Compare
3931196 to
23403d0
Compare
| wallet.importwallet(wallet_dump_path) | ||
| assert_equal(wallet.getwalletinfo()["blank"], False) |
There was a problem hiding this comment.
You are explicitly expecting this behavior, but I’m kinda surprised that when we import a wallet that was explicitly created “blank” after the import it is now no longer blank. As far as I can tell, no keys were created, unless the import automatically does that. I’m curious why this is the expected behavior (although since you seem to explicitly expect that behavior, I don’t think there is an issue here).
There was a problem hiding this comment.
We determine whether a wallet is new by checking whether it has any keys or scripts contained in it. If it is "new", then the wallet will be setup to have the automatically generated things. The blank flag is used only to indicate to that code that the wallet is intentionally blank and should not be setup as a new wallet. Once something has been added to the wallet, it's no longer blank and the setup code will no longer recognize it as being "new", so the flag should be removed. It's not intended as something that should live beyond that, it doesn't just mean that the wallet was created as blank, but rather that it is currently blank and that state is intentional.
There was a problem hiding this comment.
I see, thanks for the explanation
There was a problem hiding this comment.
I have a different perspective. The flag was provided during wallet creation and captures the intent of the user.
The blank flag is used only to indicate to that code that the wallet is intentionally blank and should not be setup as a new wallet.
I'd like to take this one step further and argue that "The blank flag is used only to indicate to that code that the wallet is intentionally blank and the user is responsible for managing the keys in this wallet manually." If user created wallet intentionally blank so the software doesn't touch the keys, does the intention go away after the keys are imported in the wallet? I don't think so.
Once something has been added to the wallet, it's no longer blank and the setup code will no longer recognize it as being "new", so the flag should be removed.
We shouldn't casually flip between just blank and intentionally blank. Yes, it's not longer blank, but it's still intentionally blank because this was and is the intent of the user. I think the word blank is just misleading here. We're better off replacing intentionally blank with manual.
|
@achow101 could you please add release notes detailing how encrypting the wallet will now rotate the imported descriptors in a descriptor wallet? Other than that this looks good. |
23403d0 to
abc328c
Compare
fdd7a21 to
cdba23d
Compare
I've done that in bitcoin-core/gui#739
Dropped it. |
|
reACK cdba23d Appreciate the perseverance in addressing all the comments and arriving at consensus. Thank for working on this. |
Agreed, thanks @achow101. I do think the end result is better, but that was a very long process... and the PR description is now slightly out of date too 😕. I'm also wondering if the earlier discussion about adding a "manual" flag (#25634 (comment)) is still relevant. Is the blank flag already acting like manual flag for descriptor wallets now, or are there other effects setting a manual flag would have for descriptor wallets? (I assume we don't care about adding a manual flag for legacy wallets.) |
|
I edited the PR description to drop the second paragraph #25634 (comment):
|
|
I went ahead and merged this despite latest version only having 2 acks, because previous versions had more acks, and the only significant change dropping a commit (37dd054 "Set blank when disable private keys in createwallet"). If anybody disagrees with dropping that, we can bring it back in another PR, but I thought it'd be good to merge the changes that seemed to have full agreement. |
…llet flag should be un/set cdba23d wallet: Document blank flag use in descriptor wallets (Ryan Ofsky) 4331020 wallet: Ensure that the blank wallet flag is unset after imports (Andrew Chow) e9379f1 rpc, wallet: Include information about blank flag (Andrew Chow) Pull request description: The `blank` wallet flag is used to indicate that the wallet intentionally does not have any keys, scripts, or descriptors, and it prevents the automatic generation of those things for such a wallet. Once the wallet contains any of those data, it is unnecessary, and possibly incorrect, to have `blank` set. This PR fixes a few places where this was not properly happening. It also adds a test for this unset behavior. ACKs for top commit: S3RK: reACK cdba23d ryanofsky: Code review ACK cdba23d. Only change since last review is dropping the commit which makes createwallet RPC set BLANK flag automatically when DISABLE_PRIVATE_KEYS flag is set Tree-SHA512: 85bc2a9754df0531575d5c8f4ad7e8f38dcd50083dc29b3283dacf56feae842e81f34654c5e1781f2dadb0560ff80e454bbc8ca3b2d1fab1b236499ae9abd7da
Unless I'm missing something, the
Now when |
As far as I know, difference was introduced when descriptor wallets were introduced, so there would not be a need for release notes. But it would be good to make sure RPCs are properly documented, so if you encrypt a wallet it is clear in what cases new keys will be generated. Also if we have any documentation contrasting legacy wallets and descriptor wallets we could have it mention this difference. I don't think it would be good for documentation to make a lot of references to the BLANK flag specifically. Better to just be document when keys will and won't be generated, and only mention about flag if it actually helps clarify behavior. |
There was a problem hiding this comment.
post-merge re-ACK with dropping 3rd commit (which only had 2 ACKs and there was the debate on: setting BLANK flag on RPC automatically when DISABLE_PRIVATE_KEYS flag is set), going to review at the GUI's follow-up on this by @achow101 that would fix the inconsistency there.
This allows us to test that the blank flag is being set appropriately. Github-Pull: bitcoin#25634 Rebased-From: e9379f1
Github-Pull: bitcoin#25634 Rebased-From: 4331020
The
blankwallet flag is used to indicate that the wallet intentionally does not have any keys, scripts, or descriptors, and it prevents the automatic generation of those things for such a wallet. Once the wallet contains any of those data, it is unnecessary, and possibly incorrect, to haveblankset. This PR fixes a few places where this was not properly happening. It also adds a test for this unset behavior.