Skip to content

Check for private keys disabled before attempting unlock #773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hebasto merged 1 commit into from
Feb 12, 2024
Merged

Check for private keys disabled before attempting unlock #773

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/qt/walletmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,13 @@ void WalletModel::unsubscribeFromCoreSignals()
// WalletModel::UnlockContext implementation
WalletModel::UnlockContext WalletModel::requestUnlock()
{
// Bugs in earlier versions may have resulted in wallets with private keys disabled to become "encrypted"
// (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()) {
Copy link
Copy Markdown
Member

@hebasto hebasto Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

@achow101 achow101 Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

bitcoin/bitcoin#28724 proposes to just delete them for the user on loading.

return UnlockContext(this, /*valid=*/true, /*relock=*/false);
}
bool was_locked = getEncryptionStatus() == Locked;
if(was_locked)
{
Expand Down