Check for private keys disabled before attempting unlock #773
Conversation
Before trying to unlock a wallet, first check if it has private keys disabled. If so, there is no need to unlock. Note that such wallets are not expected to occur in typical usage. However bugs in previous versions allowed such wallets to be created, and so we need to handle them.
|
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. |
hebasto
changed the title
| // (encryption keys are present, but not actually doing anything). | ||
| // To avoid issues with such wallets, check if the wallet has private keys disabled, and if so, return a context | ||
| // that indicates the wallet is not encrypted. | ||
| if (m_wallet->privateKeysDisabled()) { |
There was a problem hiding this comment.
It's a pity that getEncryptionStatus() == NoKeys does not work here.
Is it worth to warn the user when they load a watch-only wallet with an encryption keys?
There was a problem hiding this comment.
It's a pity that
getEncryptionStatus() == NoKeysdoes not work here.
It could, but #631 states that NoKeys is not returned for encrypted watchonly so that other users of getEncryptionStatus() get this status correctly. Perhaps that isn't necessary, but I wanted to keep this change targeted for just this specific issue and changing getEncryptionStatus()'s behavior could have unanticipated side effects.
Is it worth to warn the user when they load a watch-only wallet with an encryption keys?
bitcoin/bitcoin#28724 proposes to just delete them for the user on loading.
It's not a regression, although I think it would make sense to be backported. |
|
cc @furszy |
There was a problem hiding this comment.
Doesn't mean that the migration process will also be blocked when this happen? --> look at this line
getEncryptionStatus() should be returning Locked, which here would trigger the askpassphrase dialog in the same way described on #772.
Also, what is the expectation here, merge and backport this PR without backporting #28724? Because, after #28724, this isn't a problem anymore.
That's a good point. Perhaps
28724 is a much scarier change since it deletes things from the wallet database and I wasn't sure if people were comfortable with that. If we move forward with it, then that can supersede this PR. |
I'm going to merge this to get this bugfix into 27.0 as bitcoin/bitcoin#28724 remains untagged. |
Before trying to unlock a wallet, first check if it has private keys disabled. If so, there is no need to unlock.
Note that such wallets are not expected to occur in typical usage. However bugs in previous versions allowed such wallets to be created, and so we need to handle them.
Fixes #772
For some additional context, see #631