Non-HD keys do not have an HD seed ID associated with them, so if this value is the null value (all 0s), then we should not perform any seed ID comparison that would result in excluding the keys from combo() migration.
This changes the migration of non-HD wallets (or blank wallets with imported private keys) to make a single combo() descriptors for the non-HD/imported keys, rather than pk(), pkh(), sh(wpkh()), and wpkh() descriptors for the keys.
Implements #31374 (review)
Non-HD keys in legacy wallets without a HD seed ID were being migrated
to separate pk(), pkh(), sh(wpkh()), and wpkh() descriptors for each key.
These could be more compactly represented as combo() descriptors, so
migration should make combo() for them.
It is possible that existing non-HD wallets that were migrated, or
wallets that started blank and had private keys imported into them have
run into this issue. However, as the 4 descriptors produce the same output
scripts as the single combo(), so any previously migrated wallets are
not missing any output scripts. The only observable difference should be
performance related, and the wallet size on disk.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31452.
See 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.
Using a combo descriptor when possible reduces the amount of descriptors in the migrated wallet, while functionally equivalent to P2PK + P2PKH + P2WPKH + P2SH-P2WPKH. This fixes an oversight where it’s possible, but wasn’t done before.
As the result is equivalent, it’s not necessary to take any steps for existing wallets (though a hypothetical “wallet optimization” tool could do this).
Concept and code review ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
1040+ combo_desc = descsum_create(f"combo([{key_origin}]{pubkey_hex})")
1041
1042 # Verify all expected descriptors were migrated
1043 migrated_desc = [item['desc'] for item in wallet.listdescriptors()['descriptors'] if pubkey.hex() in item['desc']]
1044- assert_equal(expected_descs, migrated_desc)
1045+ assert_equal([combo_desc], migrated_desc)
listdescriptors(private=True)
Nice catch. ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
Non-blocking comment. For the sake of mental peace, it might worth verifying that non-hd very old releases stored scripts after importing standalone private keys. Because if they calculated them in-memory after load, this would pose a different problem.
For the sake of mental peace, it might worth verifying that non-hd very old releases stored scripts after importing standalone private keys. Because if they calculated them in-memory after load, this would pose a different problem.
I don’t think it particularly matters since loading a legacy wallet for migration results in calculating all of those scripts in memory anyways and putting them into mapScripts.
For the sake of mental peace, it might worth verifying that non-hd very old releases stored scripts after importing standalone private keys. Because if they calculated them in-memory after load, this would pose a different problem.
I don’t think it particularly matters since loading a legacy wallet for migration results in calculating all of those scripts in memory anyways and putting them into
mapScripts.
Are you sure?
The loading procedure calls LegacyDataSPKM::LoadKey, which only calls LegacyDataSPKM::AddKeyPubKeyInner, which only calls FillableSigningProvider::AddKeyPubKey which stores the key in mapKeys and calls FillableSigningProvider::ImplicitlyLearnRelatedKeyScripts, which only generates and stores the p2wpkh witness program in mapScripts and nothing else. Am I missing something?
Ok, following my last comment. After discussing it with @theStack for a while.
The first part of the LegacyDataSPKM::GetScriptPubKeys() method calculates the p2pkh and p2pk scripts from the private key. That’s how they end up being migrated. They should not be appearing in mapScript.
Anyway.. all good.
1031@@ -1032,15 +1032,11 @@ def test_manual_keys_import(self):
1032 # There should be descriptors containing the imported key for: pk(), pkh(), sh(wpkh()), wpkh()
1037- sh_wpkh_desc = descsum_create(f'sh(wpkh([{key_origin}]{pubkey_hex}))')
1038- wpkh_desc = descsum_create(f'wpkh([{key_origin}]{pubkey_hex})')
1039- expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_desc]
1040+ combo_desc = descsum_create(f"combo([{key_origin}]{pubkey_hex})")
1041
1042 # Verify all expected descriptors were migrated
tACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
I’ve been debugging with the wallet_migration.py test a bit. gethdkeys is available only for descriptor wallets.
listdescriptors give a good enough idea for the newer wallets.