wallet: fix crash during watch-only wallet migration #31374
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/31374. 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. |
furszy
force-pushed
the
2024_migration_watch-only_crash_fix
branch
from
982ce98 to
ea9528f
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
furszy
force-pushed
the
2024_migration_watch-only_crash_fix
branch
2 times, most recently
from
0d5bf47 to
b63cd86
Compare
furszy
force-pushed
the
2024_migration_watch-only_crash_fix
branch
2 times, most recently
from
97ddf4a to
8c68ba5
Compare
|
ACK 8c68ba50eb97b4ffef9a359892f2f46a89453e9a |
brunoerg
left a comment
brunoerg
left a comment
There was a problem hiding this comment.
ACK 8c68ba50eb97b4ffef9a359892f2f46a89453e9a
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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
furszy
force-pushed
the
2024_migration_watch-only_crash_fix
branch
from
8c68ba5 to
080e7ec
Compare
|
Rebased due to a silent conflict with the recently merged #31248. The tests now use the previous release node to create the wallet and the latest version node to migrate it. Ready to go. |
brunoerg
left a comment
brunoerg
left a comment
There was a problem hiding this comment.
reACK 080e7ec110b0e40e56b9aad608cd144d75b971f7
| pk_desc = descsum_create(f'pk([{key_origin}]{pubkey_hex})') | ||
| pkh_desc = descsum_create(f'pkh([{key_origin}]{pubkey_hex})') | ||
| sh_wpkh_desc = descsum_create(f'sh(wpkh([{key_origin}]{pubkey_hex}))') | ||
| wpkh_desc = descsum_create(f'wpkh([{key_origin}]{pubkey_hex})') | ||
| expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_desc] |
There was a problem hiding this comment.
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.
| pk_desc = descsum_create(f'pk({pubkey_hex})') | ||
| pkh_desc = descsum_create(f'pkh({pubkey_hex})') | ||
| sh_wpkh_desc = descsum_create(f'sh(wpkh({pubkey_hex}))') | ||
| wpkh_desc = descsum_create(f'wpkh({pubkey_hex})') | ||
| expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_desc] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
62b2d23 wallet: Migrate non-HD keys to combo() descriptor (Ava Chow) Pull request description: 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 (comment) ACKs for top commit: laanwj: Concept and code review ACK 62b2d23 brunoerg: code review ACK 62b2d23 furszy: Nice catch. ACK 62b2d23 theStack: ACK 62b2d23 rkrux: tACK 62b2d23 Tree-SHA512: 86a80b7dcc1598ab18068a2572ff4b4920b233178b760f7b76c5b21a9e6608005ac872f90e082a8f99b51daab0b049e73e4bee5b8e0b537d56ed0d34122a1f49
|
Removed the label as this isn't being backported. |
Why not? |
It was unclean, and making it work seemed to be a somewhat large change. |
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.