wallet: Cleanup accidental encryption keys in watchonly wallets #28724
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28724. 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. |
achow101
force-pushed
the
cleanup-accidental-watchonly-mkeys
branch
from
47ce145 to
aaa6b70
Compare
laanwj
left a comment
laanwj
left a comment
There was a problem hiding this comment.
Code review ACK aaa6b709d164052c8b2496cbedccf8ccf48c4aa2
willcl-ark
left a comment
willcl-ark
left a comment
There was a problem hiding this comment.
ACK aaa6b709d164052c8b2496cbedccf8ccf48c4aa2
I left a few comments, but they're mainly thoughts I had and questions for my own understanding.
Also checked the test manually with --legacy-wallet and --descriptors and lightly inspected the dumps to ensure the records were being removed:
log
$ test/functional/wallet_encryption.py
2023-11-01T11:51:31.078000Z TestFramework (INFO): PRNG seed is: 196471712041506435
2023-11-01T11:51:31.079000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_hm1fob6j
2023-11-01T11:51:35.739000Z TestFramework (INFO): Check a timeout less than the limit
2023-11-01T11:51:35.850000Z TestFramework (INFO): Check a timeout greater than the limit
2023-11-01T11:51:36.579000Z TestFramework (INFO): Test that wallets without private keys cannot be encrypted
2023-11-01T11:51:36.595000Z TestFramework (INFO): Test that encryption keys in wallets without privkeys are removed
{'walletname': 'noprivs', 'walletversion': 169900, 'format': 'sqlite', 'balance': Decimal('0E-8'), 'unconfirmed_balance': Decimal('0E-8'), 'immature_balance': Decimal('0E-8'), 'txcount': 0, 'keypoolsize': 0, 'keypoolsize_hd_internal': 0, 'paytxfee': Decimal('0E-8'), 'private_keys_enabled': False, 'avoid_reuse': False, 'scanning': False, 'descriptors': True, 'external_signer': False, 'blank': False, 'lastprocessedblock': {'hash': '0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206', 'height': 0}}
orig_dump=['BITCOIN_CORE_WALLET_DUMP,1\n', 'format,sqlite\n', '0776657273696f6e,4c1e0400\n', '0a6d696e76657273696f6e,ac970200\n', '0962657374626c6f636b,8011010000\n', '1262657374626c6f636b5f6e6f6d65726b6c65,801101000106226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f\n', '05666c616773,0400000005000000\n', 'checksum,058c1f6af6fcbbf22056c1f690d7b9fa27ec2a37dec2b0bcadca89977666634f\n']
after_dump=['BITCOIN_CORE_WALLET_DUMP,1\n', 'format,sqlite\n', '0776657273696f6e,4c1e0400\n', '0a6d696e76657273696f6e,ac970200\n', '0962657374626c6f636b,8011010000\n', '1262657374626c6f636b5f6e6f6d65726b6c65,801101000106226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f\n', '05666c616773,0400000005000000\n', '046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n', 'checksum,e63f7200466a4d2d6e05b505be87b148a96ae57702590f0fc284fbbeac332036\n']
Result of set(orig_dump) ^ set(after_dump):
{'046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n', 'checksum,058c1f6af6fcbbf22056c1f690d7b9fa27ec2a37dec2b0bcadca89977666634f\n', 'checksum,e63f7200466a4d2d6e05b505be87b148a96ae57702590f0fc284fbbeac332036\n'}
2023-11-01T11:51:37.068000Z TestFramework (INFO): Stopping nodes
2023-11-01T11:51:37.223000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_hm1fob6j on exit
2023-11-01T11:51:37.223000Z TestFramework (INFO): Tests successful
src/wallet/walletdb.cpp
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The lock is already being held from the top of the function.
src/wallet/walletdb.cpp
Outdated
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
I don't think it would be useful to log them.
src/wallet/walletdb.cpp
Outdated
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
I think this is fine as is. It can be updated in the future if it becomes outdated.
There was a problem hiding this comment.
As the process of reading wallet records from db disregards unknown record types, what if the encryption key is ever used for something else?. Wouldn't that mean that opening such wallet on any previous version (post this PR) would erase the encryption key forever?
The erase process can only verify the private keys disabled flag and whether there is any crypted key or not. But it cannot verify if the encryption key was used for something else.
Guessing that in the case of adding a new type of encrypted record in the future for private keys disabled wallets, we would be forced to add downgrade protection if we merge this PR?
Yes, but I think it's unlikely such usage of encryption keys would be added, and if it were, it could still be added in a way that does not interact with this PR. For example,although #28142 allows for encryption of records regardless of the wallet flags, it does this at the database level and uses separate records, so this PR doesn't affect it at all. |
achow101
force-pushed
the
cleanup-accidental-watchonly-mkeys
branch
from
aaa6b70 to
e975301
Compare
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.
|
utACK 69e95c2. I can't imagine any way that this could affect operations later. The key isn't used, so deleting it can't hurt backups, and with or without encryption key no ckey records can be added later. Is that correct? |
That's correct. ckey is a private key record, and wallets without private keys definitionally do not have private keys. No private keys can be generated or imported to such wallets. |
|
The new code in and |
The root cause of the issue is that the |
That's pretty surprising, because
It seems kinda unlikely they'd do that if the option was incompatible with the dynamic linker. |
The underlying issue with supported NetBSD releases is that When linking with However, linking with This limitation has been removed in NetBSD/src@acf7fb3 and will be included in the upcoming 11.0 release. |
An earlier version allowed users to create watchonly wallets (wallets without private keys) that were "encrypted". Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as bitcoin-core/gui#772.
We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those encryption keys thereby avoiding any issues that may result from this unexpected situation.