The crash occurs because we assume the cached scripts structure will not be empty, but it can be empty for watch-only wallets that start blank.
This also adds test coverage for standalone imported keys, which were also crashing
because pubkey imports are treated the same way as hex script imports through
importaddress().
Testing Notes: This can be verified by cherry-picking and running any of the test commits on master. It will crash there but pass on this branch.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31374.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/33553306813</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
1071 | + expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_desc] 1072 | + 1073 | + migrated_desc = [] 1074 | + for desc_info in wo_wallet.listdescriptors()['descriptors']: 1075 | + desc = desc_info['desc'] 1076 | + if pubkey.hex() in desc:
In bdd42ed8de8a92ae3e69e7b2dfceae7330f32676: I think it can be simplified. See:
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 928ab4acb4..06bc4e1fbb 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -1070,12 +1070,7 @@ class WalletMigrationTest(BitcoinTestFramework):
wpkh_desc = descsum_create(f'wpkh({pubkey_hex})')
expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_desc]
- migrated_desc = []
- for desc_info in wo_wallet.listdescriptors()['descriptors']:
- desc = desc_info['desc']
- if pubkey.hex() in desc:
- migrated_desc.append(desc)
-
+ migrated_desc = [item['desc'] for item in wo_wallet.listdescriptors()['descriptors']]
# Verify all expected descriptors were migrated
assert_equal(expected_descs, migrated_desc)
wallet.unloadwallet()
I don't see why we need to check if pubkey is into the descriptors, we expect all of them to have it.
I don't see why we need to check if pubkey is into the descriptors, we expect all of them to have it.
Thanks. I just blindly re-used the code I applied for the first check. Suggestion applied, and also inlined the first check too.
1013 | @@ -1012,6 +1014,18 @@ def check_comments(): 1014 | 1015 | wallet.unloadwallet() 1016 | 1017 | + def test_migrate_simple_watch_only(self): 1018 | + self.log.info("Test migrating a watch-only p2pk script") 1019 | + wallet = self.create_legacy_wallet("bare_p2pk", blank=True) 1020 | + privkey = ECKey() 1021 | + privkey.generate() 1022 | + pubkey = privkey.get_pubkey()
in a65117610de13f0de7db5f9dac380d047085d862: could use the generate_keypair helper already in this commit, so you don't have to import ECKey (and get rid of .get_bytes() calls below):
_, pubkey = generate_keypair()
yeah sure, applied. Thanks!
1093 | @@ -1050,7 +1094,7 @@ def run_test(self): 1094 | self.test_preserve_tx_extra_info() 1095 | self.test_blank() 1096 | self.test_migrate_simple_watch_only() 1097 | - 1098 | + self.test_manual_keys_import()
nit: empty line lost here
fixed
1023 | + p2pk_script = key_to_p2pk_script(pubkey.get_bytes()) 1024 | + wallet.importaddress(address=p2pk_script.hex()) 1025 | + res = wallet.migratewallet() 1026 | + wo_wallet = self.nodes[0].get_wallet_rpc(res['watchonly_name']) 1027 | + assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.get_bytes().hex()})')) 1028 | + wallet.unloadwallet()
nit: for sake of completeness, could also unload wo_wallet here (as done in the next manual keys import subtests)
sure, applied. Thanks
ACK 8c68ba50eb97b4ffef9a359892f2f46a89453e9a
ACK 8c68ba50eb97b4ffef9a359892f2f46a89453e9a
ACK 8c68ba50eb97b4ffef9a359892f2f46a89453e9a
The crash occurs because we assume the cached scripts
structure will not be empty, but it can be empty when
the legacy wallet contained only watch-only and
solvable but not spendable scripts
reACK 080e7ec110b0e40e56b9aad608cd144d75b971f7
1027 | + privkey, pubkey = generate_keypair(wif=True) 1028 | + wallet.importprivkey(privkey=privkey, label="hi", rescan=False) 1029 | + wallet.unloadwallet() 1030 | + 1031 | + # Now migrate wallet in the latest node 1032 | + res = self.master_node.migratewallet(self.old_node.wallets_path / "import_privkeys")
In 080e7ec110b0e40e56b9aad608cd144d75b971f7 "test: add coverage for migrating standalone imported keys"
Use self.migrate_and_get_rpc
Ups.. reimplemented the wheel.
1015 | + wallet.importaddress(address=p2pk_script.hex()) 1016 | + # Migrate wallet in the latest node 1017 | + res, _ = self.migrate_and_get_rpc("bare_p2pk") 1018 | + wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name']) 1019 | + assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})')) 1020 | + wo_wallet.unloadwallet()
nit: could still unload the migrated wallet here by using the second return value of migrate_and_get_rpc (but OTOH, the unloading is not done consistently in other sub-tests anyway, so maybe material for a different PR...)
nit: could still unload the migrated wallet here by using the second return value of
migrate_and_get_rpc(but OTOH, the unloading is not done consistently in other sub-tests anyway, so maybe material for a different PR...)
Actually, I didn't unload it on purpose. Aligned this PR to #31423 expected outcome. Short description: This test creates a blank legacy wallet, imports a watch-only P2PK script, and executes migration --> This scenario shouldn't create a brand-new spendable wallet as it is doing right now. The pre-migration wallet is a watch-only wallet with no key material.
1019 | @@ -1019,6 +1020,50 @@ def test_migrate_simple_watch_only(self): 1020 | assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})')) 1021 | wo_wallet.unloadwallet() 1022 | 1023 | + def test_manual_keys_import(self): 1024 | + self.log.info("Test migrating standalone private keys") 1025 | + wallet = self.create_legacy_wallet("import_privkeys", blank=True) 1026 | + privkey, pubkey = generate_keypair(wif=True) 1027 | + wallet.importprivkey(privkey=privkey, label="hi", rescan=False)
nit, seems there is no need for a label here
wallet.importprivkey(privkey=privkey, rescan=False)
re-ACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
Left two nits (not worth to tackle if you don't need to retouch for other reasons imho).
reACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
1034 | + pubkey_hex = pubkey.hex() 1035 | + pk_desc = descsum_create(f'pk([{key_origin}]{pubkey_hex})') 1036 | + pkh_desc = descsum_create(f'pkh([{key_origin}]{pubkey_hex})') 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]
It was surprising to me that the migrated wallet would make all of these individual descriptors when it could make a single comb(). Further investigation reveals that there is a minor bug in the handling of non-HD keys that would result in the separate descriptors rather than the combo().
Semantically, these 4 descriptors are the same as combo(), so this would never have caused any funds loss, however it is unexpected and should be fixed in a followup.
1055 | + # As we imported the pubkey only, there will be no key origin in the following descriptors 1056 | + pk_desc = descsum_create(f'pk({pubkey_hex})') 1057 | + pkh_desc = descsum_create(f'pkh({pubkey_hex})') 1058 | + sh_wpkh_desc = descsum_create(f'sh(wpkh({pubkey_hex}))') 1059 | + wpkh_desc = descsum_create(f'wpkh({pubkey_hex})') 1060 | + expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_desc]
Similar to the above, however this is more expected since watchonly pubkeys are not being specially treated to produce combo() descriptors, but we should probably make combo() for them as well.
Edit: On further thought, this appears to not be as possible since we can have watchonly pubkeys but not be watching all of the combo() output scripts for them.
Edit: On further thought, this appears to not be as possible since we can have watchonly pubkeys but not be watching all of the combo() output scripts for them.
~Not sure how this could be possible, but I think it would be better to add the combo and track more scripts instead of fewer. Importing a private key and a public key should behave the same at the "scan for X level". The only difference should be that one lets you spend the script, while the other doesn't.~
nvm, just checked the code. importpubkey does not save the key to disk. Only scripts created from the pubkey are stored.
ACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
Removed the label as this isn't being backported.
this isn't being backported.
Why not?
this isn't being backported.
Why not?
It was unclean, and making it work seemed to be a somewhat large change.
