Check for private keys disabled before attempting unlock

achow101 commented at 9:25 pm on October 24, 2023: member

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

gui: Check for private keys disabled before attempting unlock

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.

517c7f9cba

DrahtBot commented at 9:25 pm on October 24, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label CI failed on Oct 24, 2023

hebasto renamed this:
~~gui: Check for private keys disabled before attempting unlock~~
Check for private keys disabled before attempting unlock
on Oct 25, 2023

in src/qt/walletmodel.cpp:456 in 517c7f9cba

448@@ -449,6 +449,13 @@ void WalletModel::unsubscribeFromCoreSignals()
449 // WalletModel::UnlockContext implementation
450 WalletModel::UnlockContext WalletModel::requestUnlock()
451 {
452+    // Bugs in earlier versions may have resulted in wallets with private keys disabled to become "encrypted"
453+    // (encryption keys are present, but not actually doing anything).
454+    // To avoid issues with such wallets, check if the wallet has private keys disabled, and if so, return a context
455+    // that indicates the wallet is not encrypted.
456+    if (m_wallet->privateKeysDisabled()) {

hebasto commented at 10:35 am on October 25, 2023:

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?

achow101 commented at 2:26 pm on October 25, 2023:

It’s a pity that getEncryptionStatus() == NoKeys does 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?

https://github.com/bitcoin/bitcoin/pull/28724 proposes to just delete them for the user on loading.

hebasto approved

hebasto commented at 10:41 am on October 25, 2023: member

ACK 517c7f9cba306292e12e166b9dbc6c0838f05b27, I have reviewed the code and it looks OK.

Do we want to backport this PR into the 26.x branch?

hebasto added the label Wallet on Oct 25, 2023

DrahtBot removed the label CI failed on Oct 25, 2023

achow101 commented at 2:24 pm on October 25, 2023: member

Do we want to backport this PR into the 26.x branch?

It’s not a regression, although I think it would make sense to be backported.

hebasto commented at 2:36 pm on October 25, 2023: member

cc @furszy

furszy commented at 7:51 pm on October 25, 2023: member

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.

achow101 commented at 8:01 pm on October 25, 2023: member

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.

That’s a good point. Perhaps getEncryptionStatus() should just move to returning NoKeys for all private key disabled wallets.

Also, what is the expectation here, merge and backport this PR without backporting #28724? Because, after #28724, this isn’t a problem anymore.

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.

BrandonOdiwuor commented at 9:57 am on November 17, 2023: contributor

ACK 517c7f9cba306292e12e166b9dbc6c0838f05b27

looks good to me

DrahtBot added the label CI failed on Jan 14, 2024

hebasto commented at 2:09 pm on February 12, 2024: member

Also, what is the expectation here, merge and backport this PR without backporting #28724? Because, after #28724, this isn’t a problem anymore.

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 https://github.com/bitcoin/bitcoin/pull/28724 remains untagged.

hebasto merged this on Feb 12, 2024

hebasto closed this on Feb 12, 2024

Check for private keys disabled before attempting unlock #773

Reviews