Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: fix crash during watch-only wallet migration #31374

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

furszy
Copy link
Member

@furszy furszy commented Nov 26, 2024

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 26, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31374.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theStack, brunoerg, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31423 (wallet: migration, don't create spendable wallet from a watch-only legacy wallet by furszy)
  • #31420 (test: implements helper functions for unit conversion by wfzyx)
  • #30328 (wallet: Remove IsMine from migration code by achow101)

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 furszy force-pushed the 2024_migration_watch-only_crash_fix branch from 982ce98 to ea9528f Compare November 26, 2024 16:46
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/33553306813

Hints

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.

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bdd42ed

I verified that wallet_migration test fails on master. Also, I ran the spkm_migration fuzz target (from #29694) for over 24 hours and did not got any crash.

@furszy furszy force-pushed the 2024_migration_watch-only_crash_fix branch 2 times, most recently from 0d5bf47 to b63cd86 Compare December 4, 2024 15:36
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will run the fuzz test from #29694 on top during the night and ACK tomorrow if there's no crash. Only left some test nits below.

test/functional/wallet_migration.py Outdated Show resolved Hide resolved
test/functional/wallet_migration.py Outdated Show resolved Hide resolved
test/functional/wallet_migration.py Outdated Show resolved Hide resolved
@furszy furszy force-pushed the 2024_migration_watch-only_crash_fix branch 2 times, most recently from 97ddf4a to 8c68ba5 Compare December 5, 2024 02:55
@achow101
Copy link
Member

achow101 commented Dec 5, 2024

ACK 8c68ba5

@DrahtBot DrahtBot requested review from theStack and brunoerg December 5, 2024 19:42
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8c68ba5

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8c68ba5

Verified

This commit was signed with the committer’s verified signature.
furszy Matias Furszyfer
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 furszy force-pushed the 2024_migration_watch-only_crash_fix branch from 8c68ba5 to 080e7ec Compare December 6, 2024 17:08
@furszy
Copy link
Member Author

furszy commented Dec 6, 2024

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.

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 080e7ec

@DrahtBot DrahtBot requested review from achow101 and theStack December 6, 2024 17:18
@DrahtBot DrahtBot requested a review from achow101 December 6, 2024 19:06

Verified

This commit was signed with the committer’s verified signature.
furszy Matias Furszyfer

Verified

This commit was signed with the committer’s verified signature.
furszy Matias Furszyfer
@furszy furszy force-pushed the 2024_migration_watch-only_crash_fix branch from 080e7ec to cdd207c Compare December 6, 2024 19:13
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK cdd207c

Left two nits (not worth to tackle if you don't need to retouch for other reasons imho).

res, _ = self.migrate_and_get_rpc("bare_p2pk")
wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name'])
assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})'))
wo_wallet.unloadwallet()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Member Author

@furszy furszy Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.log.info("Test migrating standalone private keys")
wallet = self.create_legacy_wallet("import_privkeys", blank=True)
privkey, pubkey = generate_keypair(wif=True)
wallet.importprivkey(privkey=privkey, label="hi", rescan=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, seems there is no need for a label here

Suggested change
wallet.importprivkey(privkey=privkey, label="hi", rescan=False)
wallet.importprivkey(privkey=privkey, rescan=False)

@DrahtBot DrahtBot requested a review from brunoerg December 8, 2024 23:01
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK cdd207c

Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cdd207c

Comment on lines +1035 to +1039
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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1056 to +1060
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]
Copy link
Member

@achow101 achow101 Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@furszy furszy Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@achow101 achow101 merged commit 9039d8f into bitcoin:master Dec 9, 2024
18 checks passed
@furszy furszy deleted the 2024_migration_watch-only_crash_fix branch December 9, 2024 20:14
achow101 added a commit to achow101/bitcoin that referenced this pull request Dec 9, 2024
…tion

cdd207c test: add coverage for migrating standalone imported keys (furszy)
297a876 test: add coverage for migrating watch-only script (furszy)
932cd1e wallet: fix crash during watch-only wallet migration (furszy)

Pull request description:

  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.
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Dec 11, 2024

Unverified

This user has not yet uploaded their public signing key.
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

Github-Pull: bitcoin#31374
Rebased-From: 932cd1e
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Dec 11, 2024

Unverified

This user has not yet uploaded their public signing key.
Github-Pull: bitcoin#31374
Rebased-From: 297a876
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Dec 11, 2024

Unverified

This user has not yet uploaded their public signing key.
Github-Pull: bitcoin#31374
Rebased-From: cdd207c
fanquake added a commit that referenced this pull request Dec 13, 2024

Verified

This commit was signed with the committer’s verified signature.
fanquake fanquake
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
@fanquake
Copy link
Member

Removed the label as this isn't being backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants