wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases

achow101 commented at 10:07 pm on December 13, 2024: member

The legacy wallet IsMine() is essentially a black box that would tell us whether the wallet is watching an output script. In order to migrate legacy wallets to descriptor wallets, we need to be able to compute all of the output scripts that a legacy wallet would watch. The original approach for this was to understand IsMine() and write a function which would be its inverse. This was partially done in the original migration code, and attempted to be completed in #30328. However, further analysis of IsMine() has continued to reveal additional edge cases which make writing an inverse function increasingly difficult to verify correctness.

This PR instead changes migration to utilize IsMine() to produce the output scripts by first computing a superset of all of the output scripts that IsMine() would watch and testing each script against IsMine() to filter for the ones that actually are watched. The superset is constructed by computing all possible output scripts for the keys and scripts in the wallet - for keys, every key could be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH; for scripts, every script could be an output script, the redeemScript of a P2SH, the witnessScript of a P2WSH, and the witnessScript of a P2SH-P2WSH.

Additionally, the legacy wallet can contain scripts that are redeemScripts and witnessScripts, while not watching for any output script utilizing that script. These are known as solvable scripts and are migrated to a separate “solvables” wallet. The previous approach to identifying these solvables was similar to identifying output scripts - finding known solvable conditions and computing the scripts. However, this also can miss scripts, so the solvables are now identified in a manner similar to the output scripts but using the function CanProvide(). Using the same superset as before, all output scripts which are ISMINE_NO are put through CanProvide() which will perform a dummy signing and then a key lookup to determine whether the legacy wallet could provide any solving data for the output script. The scripts that pass will have their descriptors inferred and the script included in the solvables wallet.

The main downside of this approach is that IsMine() and CanProvide() can no longer be deleted. They will need to be refactored to be migration only code instead in #28710.

Lastly, I’ve added 2 test cases for the edge cases that prompted this change of approach. In particular, miniscript witnessScripts and rawtr() output scripts are solvable and signable in a legacy wallet, although never ISMINE_SPENDABLE.

DrahtBot commented at 10:07 pm on December 13, 2024: contributor

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/31495.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	sipa

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)
#28710 (Remove the legacy wallet and BDB dependency 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.

DrahtBot added the label Wallet on Dec 13, 2024

test: Extra verification that migratewallet migrates 88b67c95cb

achow101 force-pushed on Dec 13, 2024

tests: Test migration of additional P2WSH scripts 9c21b8f581

legacy spkm: Move CanProvide to LegacyDataSPKM

This function will be needed in migration

22f6cdd440

legacy spkm: use IsMine() to extract watched output scripts

Instead of (partially) trying to reverse IsMine() to get the output
scripts that a LegacySPKM would track, we can preserve it in migration
only code and utilize it to get an accurate set of output scripts.

This is accomplished by computing a set of output script candidates from
map(Crypted)Keys, mapScripts, and setWatchOnly. This candidate set is an
upper bound on the scripts tracked by the wallet. Then IsMine() is used
to filter to the exact output scripts that LegacySPKM would track.

By changing GetScriptPubKeys() this way, we can avoid complexities in
reversing IsMine() and get a more complete set of output scripts.

637a4ce9d3

migration: Skip descriptors which do not parse

InferDescriptors can sometimes make descriptors which are actually
invalid and cannot be parsed. Detect and skip such descriptors by doing
a Parse() check before adding the descriptor to the wallet.

5ed8464c0b

wallet migration: Determine Solvables with CanProvide

LegacySPKM would determine whether it could provide any script data to a
transaction through the use of the CanProvide function. Instead of
partially reversing signing logic to figure out the output scripts of
solvable things, we use the same candidate set approach in
GetScriptPubKeys() and instead filter the candidate set first for
things that are ISMINE_NO, and second with CanProvide(). This should
give a more accurate solvables wallet.

22ec1dc31f

achow101 force-pushed on Dec 13, 2024

sipa commented at 7:46 pm on December 14, 2024: member

Concept ACK

test: Test migration of miniscript in legacy wallets 850962d007

test: Test migration of taproot output scripts 9b85a94dc3

test: Test migration of a solvable script with no privkeys

The legacy wallet will be able to solve output scripts where the
redeemScript or witnessScript is known, but does not know any of the
private keys involved in that script. These should be migrated to the
solvables wallet.

d8608d140c

achow101 force-pushed on Dec 16, 2024

wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases #31495

Code Coverage & Benchmarks

Reviews

Conflicts