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.
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31374.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | brunoerg |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33553306813
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.
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:
0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
1index 928ab4acb4..06bc4e1fbb 100755
2--- a/test/functional/wallet_migration.py
3+++ b/test/functional/wallet_migration.py
4@@ -1070,12 +1070,7 @@ class WalletMigrationTest(BitcoinTestFramework):
5 wpkh_desc = descsum_create(f'wpkh({pubkey_hex})')
6 expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_desc]
7
8- migrated_desc = []
9- for desc_info in wo_wallet.listdescriptors()['descriptors']:
10- desc = desc_info['desc']
11- if pubkey.hex() in desc:
12- migrated_desc.append(desc)
13-
14+ migrated_desc = [item['desc'] for item in wo_wallet.listdescriptors()['descriptors']]
15 # Verify all expected descriptors were migrated
16 assert_equal(expected_descs, migrated_desc)
17 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.