Skip to content

wallet: Cleanup accidental encryption keys in watchonly wallets #28724

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
fanquake merged 3 commits into from
Jan 10, 2025
+106 −0
Merged

wallet: Cleanup accidental encryption keys in watchonly wallets #28724

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
813a16a
wallet: Add HasCryptedKeys
achow101 Oct 24, 2023
2b9279b
wallet: Remove unused encryption keys from watchonly wallets
achow101 Oct 24, 2023
69e95c2
tests: Test cleanup of mkeys from wallets without privkeys
achow101 Oct 24, 2023
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
Prev Previous commit
Next Next commit
wallet: Remove unused encryption keys from watchonly wallets 
Due to a bug in earlier versions, some wallets without private keys may
have an encryption key. This encryption key is unused and can lead to
confusing behavior elsewhere. When such wallets are detected, those
encryption keys will now be deleted from the wallet. For safety, we only
do this to wallets which have private keys disabled, have encryption keys,
and definitely do not have encrypted keys.
  • Loading branch information
@achow101
achow101 committed Oct 16, 2024
commit 2b9279b50a3682ab97308888b4f272d3ae379811
20 changes: 20 additions & 0 deletions src/wallet/walletdb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,11 @@ bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)
return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true);
}

bool WalletBatch::EraseMasterKey(unsigned int id)
{
return EraseIC(std::make_pair(DBKeys::MASTER_KEY, id));
}

bool WalletBatch::WriteCScript(const uint160& hash, const CScript& redeemScript)
{
return WriteIC(std::make_pair(DBKeys::CSCRIPT, hash), redeemScript, false);
Expand Down Expand Up @@ -1241,6 +1246,21 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
result = DBErrors::CORRUPT;
}

// Since it was accidentally possible to "encrypt" a wallet with private keys disabled, we should check if this is
// such a wallet and remove the encryption key records to avoid any future issues.
// Although wallets without private keys should not have *ckey records, we should double check that.
// Removing the mkey records is only safe if there are no *ckey records.
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) {

@willcl-ark willcl-ark Nov 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I notice that nothing in this block requires any locks (even though cs_wallet will be locked at this stage in this function).

For my own understanding, is this because the db operations, i.e. writing to the on-disk file, have their synchronisation handled by the db's transaction system?

I see for example in LoadEncryptionKey that we lock cs_wallet to load the keys into the wallet, which makes intuitive sense. But it feels that conversely there should be something asserting the lock is held here too, as we remove encryption keys from the wallet? Or at least when pwallet->mapMasterKeys.clear(); is called.

Perhaps it doesn't matter as it's done during loading from the db, so nothing else will have started to access the Wallet?

@achow101 achow101 Nov 1, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The lock is already being held from the top of the function.

pwallet->WalletLogPrintf("Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys.\n");

@willcl-ark willcl-ark Nov 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was wondering if if could be a good idea to log the removed keys at this point as a backup (safe-ish seeing as we are deleting), but I'm struggling to think of a scenario where a user with this type of wallet might somehow need these keys again...

@achow101 achow101 Nov 30, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think it would be useful to log them.

@furszy furszy Nov 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm still not sure about #28142 but, if we ever do something like that, we could have an encrypted wallet without encrypted private keys. So, might want to change the "without private keys" to "without any encrypted record".

@achow101 achow101 Nov 30, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this is fine as is. It can be updated in the future if it becomes outdated.

for (const auto& [id, _] : pwallet->mapMasterKeys) {
if (!EraseMasterKey(id)) {
pwallet->WalletLogPrintf("Error: Unable to remove extraneous encryption key '%u'. Wallet corrupt.\n", id);
return DBErrors::CORRUPT;
}
}
pwallet->mapMasterKeys.clear();
}

return result;
}

Expand Down
1 change: 1 addition & 0 deletions src/wallet/walletdb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ class WalletBatch
bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata &keyMeta);
bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector<unsigned char>& vchCryptedSecret, const CKeyMetadata &keyMeta);
bool WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey);
bool EraseMasterKey(unsigned int id);

bool WriteCScript(const uint160& hash, const CScript& redeemScript);

Expand Down