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, theStack, brunoerg

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

achow101 force-pushed on Dec 13, 2024

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

Concept ACK

achow101 force-pushed on Dec 16, 2024

in test/functional/wallet_migration.py:1117 in 9c21b8f581 outdated

1112+        # In order to get the wsh(pkh()) into only mapScripts and not setWatchOnly, we need to utilize
1113+        # importmulti and wrap the wsh(pkh()) inside of a sh(). This will insert the sh(wsh(pkh())) into
1114+        # setWatchOnly but not the wsh(pkh()).
1115+        # Furthermore, migration should not migrate the wsh(pkh()) if the key is uncompressed.
1116+        comp_eckey = ECKey()
1117+        comp_eckey.generate(compressed=True)

theStack commented at 5:37 pm on January 5, 2025:

seems like the picked commit 9c21b8f581542aee0f599f1d8996ac97071bdf6f is out-dated, as this sub-test was already changed to use generate_keypair in #30328 (see e.g. #30328 (review))

achow101 commented at 7:44 pm on January 6, 2025:

Ah, fixed.

in src/wallet/scriptpubkeyman.cpp:1958 in 850962d007 outdated

1954@@ -1955,6 +1955,7 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
1955         std::string desc_str;
1956         bool watchonly = !desc->ToPrivateString(*this, desc_str);
1957         if (watchonly && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
1958+            WalletLogPrintf("%s\n", desc->ToString());

theStack commented at 6:56 pm on January 5, 2025:

in commit 850962d0074683cccbe0f63c864a70e0f31a8caf: is this a left-over from debugging or introduced intentionally?

achow101 commented at 7:44 pm on January 6, 2025:

Removed

theStack commented at 7:16 pm on January 5, 2025: contributor

Concept ACK

achow101 force-pushed on Jan 6, 2025

DrahtBot added the label CI failed on Jan 6, 2025

DrahtBot removed the label CI failed on Jan 7, 2025

in test/functional/wallet_migration.py:115 in 88b67c95cb outdated

111@@ -111,7 +112,9 @@ def migrate_and_get_rpc(self, wallet_name, **kwargs):
112         # Migrate, checking that rescan does not occur
113         with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
114             migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
115-        return migrate_info, self.master_node.get_wallet_rpc(wallet_name)
116+        rpc = self.master_node.get_wallet_rpc(wallet_name)

Sjors commented at 3:54 pm on January 13, 2025:

88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b nit: wallet = would be more consistent

achow101 commented at 11:25 pm on January 20, 2025:

Done

in test/functional/wallet_migration.py:1054 in 9452f06a02 outdated

1063@@ -1063,6 +1064,151 @@ def test_manual_keys_import(self):
1064         assert_equal(expected_descs, migrated_desc)
1065         wo_wallet.unloadwallet()
1066 
1067+    def test_p2wsh(self):
1068+        self.log.info("Test that non-multisig P2WSH output scripts are migrated")
1069+        def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name)

Sjors commented at 4:00 pm on January 13, 2025:

9452f06a0283299c420a52f6b7f59f484cb60ca3 nit: maybe call it funding_wallet =, or default = for consistency with test_basic. Alternatively you could introduce a helper self.fund(address, amount)

achow101 commented at 11:10 pm on January 20, 2025:

def_wallet is a common name used for several other test cases, I don’t think it’s necessary to rename it.

I don’t think a self.fund() helper would be all that useful considering that this is basically 2 lines, and several tests still need to access the default wallet anyways outside of funding the test wallet.

in test/functional/wallet_migration.py:1068 in 9452f06a02 outdated

1075+        pkh_script = key_to_p2pkh_script(pubkey).hex()
1076+        wsh_pkh_script = script_to_p2wsh_script(pkh_script).hex()
1077+        wsh_pkh_addr = script_to_p2wsh(pkh_script)
1078+
1079+        wallet.importaddress(address=pkh_script, p2sh=False)
1080+        wallet.importaddress(address=wsh_pkh_script, p2sh=False)

Sjors commented at 5:09 pm on January 13, 2025:

9452f06a0283299c420a52f6b7f59f484cb60ca3: maybe add a comment to explain what’s going on here. In particular why pkh_script needs to be imported, even though the legacy wallet tracks a pkh(pubkey) for every address you generate.

IIUC:

normally the legacy wallet doesn’t consider the wsh(pkh(pubkey)) variant
It only has pkh(pubkey), wpkh(pubkey) and sh(wpkh(pukey)
- but pkh(pubkey) is not in mapScripts (?)
The two importaddress calls add pkh(pubkey) and wsh(pkh(pubkey)) scripts
- now pkh(pubkey) is in mapScripts (?)
The reason this doesn’t end up in the seperate watch-only wallet, i.e. ismine and solvable below are true and iswatchonly is false:
- magic in the legacy IsMine when asked about wsh(pkh(pubkey))
- it goes through case TxoutType::WITNESS_V0_SCRIPTHASH:
- which looks for the subscript pkh(pubkey), only in mapScripts
- and then recurses into LegacyWalletIsMineInner for pkh(pubkey)
  - which identifies it as a PUBKEYHASH, which finds pubkey in mapKeys and so we know it’s spendable

brunoerg commented at 4:38 pm on January 16, 2025:

I had same doubt. I understand that the generated address would be watch-only if we do not import as it’s done in the test.

achow101 commented at 11:25 pm on January 20, 2025:

Added a comment.

Sjors commented at 5:16 pm on January 13, 2025: member

Some questions in the form of feedback…

brunoerg commented at 11:40 am on January 16, 2025: contributor

Concept ACK

in test/functional/wallet_migration.py:118 in 88b67c95cb outdated

111@@ -111,7 +112,9 @@ def migrate_and_get_rpc(self, wallet_name, **kwargs):
112         # Migrate, checking that rescan does not occur
113         with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
114             migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
115-        return migrate_info, self.master_node.get_wallet_rpc(wallet_name)
116+        rpc = self.master_node.get_wallet_rpc(wallet_name)
117+        assert_equal(rpc.getwalletinfo()["descriptors"], True)

brunoerg commented at 2:20 pm on January 16, 2025:

88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b: Since you’re checking the “descriptors” field into migrate_and_get_rpc, you could also assert the wallet is sqlite and then remove the duplicated code along the test. e.g.:

0_, multisig0 = self.migrate_and_get_rpc("multisig0")
1assert_equal(multisig0.getwalletinfo()["descriptors"], True)
2self.assert_is_sqlite("multisig0")

0_, multisig1 = self.migrate_and_get_rpc("multisig1")
1assert_equal(multisig1.getwalletinfo()["descriptors"], True)
2self.assert_is_sqlite("multisig1")

achow101 commented at 11:25 pm on January 20, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:1935 in 0f0ce579ff outdated

1930+        // Re-parse the descriptors to detect that, and skip any that do not parse.
1931+        {
1932+            std::string desc_str = desc->ToString();
1933+            FlatSigningProvider parsed_keys;
1934+            std::string parse_error;
1935+            std::vector<std::unique_ptr<Descriptor>> parsed_descs = Parse(desc_str, parsed_keys, parse_error, false);

brunoerg commented at 4:43 pm on January 16, 2025:

0f0ce579fffe533534dc117c854ebef3f277f18e: nit: No need to set require_checksum. It’s false by default.

achow101 commented at 11:25 pm on January 20, 2025:

Done

in test/functional/wallet_migration.py:1252 in 56d5a6a495 outdated

1247+        send_res = wallet.send(outputs=[{some_keys_addr: 1},{all_keys_addr: 0.75}], include_watching=True, change_address=def_wallet.getnewaddress())
1248+        assert_equal(send_res["complete"], True)
1249+        self.generate(self.old_node, 6)
1250+        assert_equal(wallet.getbalances()["watchonly"]["trusted"], 1.75)
1251+
1252+        res, wallet = self.migrate_and_get_rpc("miniscript")

brunoerg commented at 5:55 pm on January 16, 2025:

56d5a6a495bfa5da540e9832443ce8f130c4c960: nit: res is not being used.

achow101 commented at 11:25 pm on January 20, 2025:

Removed

in test/functional/wallet_migration.py:1334 in 3474524ee5 outdated

1342+        addr_info = wallet.getaddressinfo(addr)
1343+        assert_equal(addr_info["ismine"], False)
1344+        assert "hex" not in addr_info
1345+
1346+        # The multisig address should be in the solvables wallet
1347+        addr_info = solvables.getaddressinfo(addr)

brunoerg commented at 9:02 pm on January 17, 2025:

3474524ee56dc3d0d009fe1462092ae5d6cbd775: It should be in the solvables wallet, so we could check it in getaddressinfo:

0@@ -1346,6 +1346,7 @@ class WalletMigrationTest(BitcoinTestFramework):
1         # The multisig address should be in the solvables wallet
2         addr_info = solvables.getaddressinfo(addr)
3         assert_equal(addr_info["ismine"], True)
4+        assert_equal(addr_info['solvable'], True)
5         assert "hex" in addr_info

achow101 commented at 11:25 pm on January 20, 2025:

Added

achow101 force-pushed on Jan 20, 2025

test: Extra verification that migratewallet migrates c39b3cfcd1

tests: Test migration of additional P2WSH scripts 9c0d5856d2

legacy spkm: Move CanProvide to LegacyDataSPKM

This function will be needed in migration

afea1f991b

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.

936fe5899f

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.

7d199d062d

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.

ca0564e02c

test: Test migration of miniscript in legacy wallets e41f47597d

test: Test migration of taproot output scripts ee598f68fe

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.

feb96d8c1c

achow101 force-pushed on Jan 21, 2025

DrahtBot commented at 0:04 am on January 21, 2025: contributor

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

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.

DrahtBot added the label CI failed on Jan 21, 2025

DrahtBot removed the label CI failed on Jan 21, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts