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
ACK	sipa, brunoerg, rkrux
Concept ACK	theStack
Stale ACK	Sjors

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:1341 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

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

in test/functional/wallet_migration.py:1175 in 9c0d5856d2 outdated

1170+            assert "embedded" in addr_info
1171+
1172+        # Fund those output scripts
1173+        def_wallet.send([{comp_wsh_pkh_addr: 1}] + [{k: i + 1} for i, k in enumerate(invalid_addrs)])
1174+        self.generate(self.nodes[0], 6)
1175+        assert_equal(wallet.getbalances()["mine"]["trusted"], 1)

Sjors commented at 11:00 am on January 21, 2025:

9c0d5856d2065119232bb4ca651391ed829a99d4: maybe add a comment here that coins sent to invalid addresses won’t show up in the destination wallet.

You can also add:

0assert_equal(wallet.getbalances()["watchonly"]["trusted"], 0)

And maybe:

0assert_equal(len(wallet.listtransactions(include_watchonly=True)), 1)

achow101 commented at 7:20 pm on January 21, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:1708 in 936fe5899f outdated

1710-    // All keys have at least P2PK and P2PKH
1711-    for (const auto& key_pair : mapKeys) {
1712-        const CPubKey& pub = key_pair.second.GetPubKey();
1713-        spks.insert(GetScriptForRawPubKey(pub));
1714-        spks.insert(GetScriptForDestination(PKHash(pub)));
1715+    // For every private key in the wallet, there should be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH

Sjors commented at 11:28 am on January 21, 2025:

936fe5899f9086f99a9a7f05584f467add2bc45b: should or could? IIUC if we migrate a pre-segwit wallet, then the IsMine() check below will throw out P2WPKH and P2SH-P2WPKH. But the migrated wallet will watch those addresses anyway, because it uses a combo() descriptor.

If so, it would be good to note that.

achow101 commented at 6:34 pm on January 21, 2025:

Should, because loading a legacy wallet will add the P2WPKH script to mapScripts for all keys, regardless of pre or post segwit. This is done in memory only, on loading.

Sjors commented at 9:52 am on January 22, 2025:

Ok, I suppose we could drop the code that does that eventually, but not here.

in src/wallet/scriptpubkeyman.cpp:1725 in 936fe5899f outdated

1759-                    spks.insert(ms_spk);
1760-                }
1761-            }
1762-        }
1763+    // mapScripts contains all redeemScripts and witnessScripts. Therefore each script in it has
1764+    // itself, P2SH, P2WSH, and P2SH-P2WSH as a candidate.

Sjors commented at 11:45 am on January 21, 2025:

936fe5899f9086f99a9a7f05584f467add2bc45b: it seems worth noting here that we’ll add some p2sh-of-p2sh scripts, but that those will get filtered, so we don’t bother with a script.IsPayToScriptHash() check.

Ditto for the segwit and multisig checks that are now removed, I think it’s worth mentioning that are intentionally added here for simplicity, and then filtered.

achow101 commented at 7:20 pm on January 21, 2025:

Expanded the comment a bit.

in src/wallet/scriptpubkeyman.cpp:1939 in 7d199d062d outdated

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);
1936+            if (parsed_descs.empty()) {

Sjors commented at 11:52 am on January 21, 2025:

7d199d062d8794cde46631e61f2c116b9e37f573: why is it safe to not abort the migration over this?

Also, won’t this hit the assert(spks.size() == 0); added by the next commit?

theStack commented at 5:36 pm on January 21, 2025:

Also, won’t this hit the assert(spks.size() == 0); added by the next commit?

Was also wondering about this. IIUC, we can’t directly continue here, but would need execute the “// Remove the scriptPubKeys from our current set” loop below for also for skipped output scripts, as otherwise the set isn’t empty and the assertion throws. Would be good to have test coverage for the skipping scenario, but I presume that isn’t possible, as we’d need to intentionally modify the behaviour of InferDescriptor only for the test (to re-introduce past bugs that caused this)?

achow101 commented at 7:20 pm on January 21, 2025:

Changed this to also delete the offending script from spks.

Sjors commented at 10:01 am on January 22, 2025:

But then the script just disappears, shouldn’t we warn about that or even abort?

Sjors commented at 11:04 am on February 3, 2025:

Ah I misunderstood the comment. It’s not referring to “past bugs” that somehow left bad scripts in the wallet, but rather it’s saying that InferDescriptor is potentially unreliable.

in src/wallet/scriptpubkeyman.cpp:2014 in ca0564e02c outdated

2016         uint64_t creation_time = 0;
2017-        const auto& it = m_script_metadata.find(CScriptID(script));
2018-        if (it != m_script_metadata.end()) {
2019-            creation_time = it->second.nCreateTime;
2020+        const auto& mit = m_script_metadata.find(CScriptID(script));
2021+        if (mit != m_script_metadata.end()) {

Sjors commented at 12:23 pm on January 21, 2025:

ca0564e02c72ea57d89ec37f0f4948e6a5d0096e: you’re renaming it to mit for consistency with above? Not sure if it’s helpful though.

achow101 commented at 7:20 pm on January 21, 2025:

Reverted

in src/wallet/scriptpubkeyman.cpp:1997 in ca0564e02c outdated

2028-            CScript sh_spk = GetScriptForDestination(ScriptHash(script));
2029-            CTxDestination witdest = WitnessV0ScriptHash(script);
2030-            CScript witprog = GetScriptForDestination(witdest);
2031-            CScript sh_wsh_spk = GetScriptForDestination(ScriptHash(witprog));
2032-
2033-            // We only want the multisigs that we have not already seen, i.e. they are not watchonly and not spendable

Sjors commented at 12:26 pm on January 21, 2025:

ca0564e02c72ea57d89ec37f0f4948e6a5d0096e: I think it’s worth preserving some of this comment, if only to explain to future readers why we went for the GetCandidateScriptPubKeys() | CanProvide approach.

Perhaps in CanProvide?

achow101 commented at 7:19 pm on January 21, 2025:

I don’t think any part of this comment is relevant to the current code.

in test/functional/wallet_migration.py:1208 in e41f47597d outdated

1195@@ -1196,6 +1196,60 @@ def test_disallowed_p2wsh(self):
1196 
1197         wallet.unloadwallet()
1198 
1199+    def test_miniscript(self):
1200+        # It turns out that due to how signing logic works, legacy wallets that have valid miniscript witnessScripts
1201+        # and the private keys for them can still sign and spend them, even though output scripts involving them
1202+        # as a witnessScript would not be detected as ISMINE_SPENDABLE.

Sjors commented at 12:36 pm on January 21, 2025:

e41f47597d86022d5a9947ee0b08151d4336cffc: Is this only intended for users who called importmulti on a legacy wallet? Or could those scripts have ended up in legacy wallets in other ways?

If the latter is the case, do you mean that only modern node versions with miniscript support can sign them? Or could pre-miniscript nodes spend them as well? Might be worth illustrating with an older node (< v25?), if the miniscript wallet here can be loaded by that older version.

achow101 commented at 6:43 pm on January 21, 2025:

I believe the only way is through importmulti.

Versions without miniscript cannot sign for miniscript. I feel like that’s obvious and we don’t need a test for that.

Sjors commented at 10:00 am on January 22, 2025:

That’s not going to obvious for most people looking at this in the future, so I think it’s worth a comment (a test might be overkill).

in test/functional/wallet_migration.py:1209 in e41f47597d outdated

1195@@ -1196,6 +1196,60 @@ def test_disallowed_p2wsh(self):
1196 
1197         wallet.unloadwallet()
1198 
1199+    def test_miniscript(self):
1200+        # It turns out that due to how signing logic works, legacy wallets that have valid miniscript witnessScripts
1201+        # and the private keys for them can still sign and spend them, even though output scripts involving them
1202+        # as a witnessScript would not be detected as ISMINE_SPENDABLE.
1203+        self.log.info("Test migration of a legacy wallet containing miniscript")

Sjors commented at 12:39 pm on January 21, 2025:

e41f47597d86022d5a9947ee0b08151d4336cffc: “containing scripts matching miniscript” / “containing scripts imported from miniscript”?

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

Miniscript is a subset of script. Definitionally, a miniscript is also a script, so this sentence is correct.

Sjors commented at 9:58 am on January 22, 2025:

Miniscript is a language for writing (a subset of) Bitcoin Scripts

https://bitcoin.sipa.be/miniscript/

So when I read “containing miniscript” I interpret that as containing a descriptor using the miniscript language. But these are legacy wallets, which don’t have descriptors, so they can only contain script itself.

Sjors commented at 12:48 pm on January 21, 2025: member

Concept ACK

Mostly happy with feb96d8c1c4dfcdf087250a55f893aed0c768398

achow101 force-pushed on Jan 21, 2025

DrahtBot added the label CI failed on Jan 21, 2025

DrahtBot removed the label CI failed on Jan 21, 2025

achow101 added this to the milestone 29.0 on Jan 23, 2025

in test/functional/wallet_migration.py:135 in c39b3cfcd1 outdated

135@@ -132,10 +136,6 @@ def test_basic(self):
136         # Note: migration could take a while.
137         _, basic0 = self.migrate_and_get_rpc("basic0")
138 
139-        # Verify created descriptors

sipa commented at 5:33 pm on January 23, 2025:

In commit “test: Extra verification that migratewallet migrates”

Why are these test lines removed?

achow101 commented at 10:42 pm on January 31, 2025:

These checks all occur inside of migrate_and_get_rpc now so they’re redundant.

in src/wallet/scriptpubkeyman.h:305 in 5087481527 outdated

301@@ -302,6 +302,9 @@ class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider
302     virtual bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey);
303     bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
304 
305+    // Helper function to retrieve a set of all output scripts that may be relevant to this LegacyDataSPKM

sipa commented at 5:45 pm on January 23, 2025:

In commit “legacy spkm: use IsMine() to extract watched output scripts”

Maybe explicitly mention this is a conservative superset of the actually relevant output scripts.

achow101 commented at 0:36 am on February 1, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:1932 in 525458f38c outdated

1927@@ -1928,6 +1928,21 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
1928 
1929         // InferDescriptor as that will get us all the solving info if it is there
1930         std::unique_ptr<Descriptor> desc = InferDescriptor(spk, *GetSolvingProvider(spk));
1931+
1932+        // Past bugs in InferDescriptor has caused it to create descriptors which cannot be re-parsed

sipa commented at 5:47 pm on January 23, 2025:

In commit “migration: Skip descriptors which do not parse”

Grammar nit: has -> have. Add . at end of sentence.

achow101 commented at 0:36 am on February 1, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:1996 in 0944298372 outdated

1991@@ -1992,10 +1992,26 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
1992         }
1993     }
1994 
1995-    // Multisigs are special. They don't show up as ISMINE_SPENDABLE unless they are in a P2SH
1996-    // So we have to check if any of our scripts are a multisig and if so, add the P2SH
1997-    for (const auto& script_pair : mapScripts) {
1998-        const CScript script = script_pair.second;
1999+    // Make sure that we have accounted for all scriptPubKeys
2000+    assert(spks.size() == 0);

sipa commented at 6:44 pm on January 23, 2025:

In commit “wallet migration: Determine Solvables with CanProvide”

Could this use CHECK_NONFATAL to give an RPC error in case of failure, rather than crashing the process?

achow101 commented at 0:36 am on February 1, 2025:

I changed it to Assume and added an early return here so that the failed migration is cleaned up.

in test/functional/wallet_migration.py:1252 in c429ba3c3a outdated

1246+        # The miniscript with all keys should be in the migrated wallet
1247+        assert_equal(wallet.getbalances()["mine"], {"trusted": 0.75, "untrusted_pending": 0, "immature": 0})
1248+        assert_equal(wallet.getaddressinfo(all_keys_addr)["ismine"], True)
1249+        assert_equal(wallet.getaddressinfo(some_keys_addr)["ismine"], False)
1250+
1251+        # The miniscript with some keys should be in the watchonly wallet

sipa commented at 6:52 pm on January 23, 2025:

In commit “test: Test migration of miniscript in legacy wallets”, is it possible to also construct an example involving miniscript that ends up in the solvables wallet?

achow101 commented at 0:36 am on February 1, 2025:

I think it’s possible, but I can’t figure it out right now.

achow101 force-pushed on Feb 1, 2025

DrahtBot added the label CI failed on Feb 1, 2025

in src/wallet/scriptpubkeyman.cpp:1932 in 6eddf8ea9f outdated

1927@@ -1928,6 +1928,21 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
1928 
1929         // InferDescriptor as that will get us all the solving info if it is there
1930         std::unique_ptr<Descriptor> desc = InferDescriptor(spk, *GetSolvingProvider(spk));
1931+
1932+        // Past bugs in InferDescriptor hav caused it to create descriptors which cannot be re-parsed.

Sjors commented at 10:47 am on February 3, 2025:

In 6eddf8ea9fce23b3e6d1c7e73d7f39e839beb3b5 “migration: Skip descriptors which do not parse” typo: hav

Sjors commented at 11:06 am on February 3, 2025: member

ACK d5e28457a099cd546e757984043f28ba9f6689b1 modulo fixup squash

DrahtBot requested review from theStack on Feb 3, 2025

DrahtBot requested review from brunoerg on Feb 3, 2025

DrahtBot requested review from sipa on Feb 3, 2025

DrahtBot removed the label CI failed on Feb 3, 2025

in test/functional/wallet_migration.py:1163 in e920a15aae outdated

1158+        comp_wsh_pkh_addr_info = wallet.getaddressinfo(comp_wsh_pkh_addr)
1159+        assert_equal(comp_wsh_pkh_addr_info["ismine"], True)
1160+        assert_equal(comp_wsh_pkh_addr_info["iswatchonly"], False)
1161+        assert "embedded" in comp_wsh_pkh_addr_info
1162+
1163+        # The invalid addresses are invalid, so the legcy wallet should not detect them as ismine,

rkrux commented at 10:48 am on February 5, 2025:

legacy

achow101 commented at 6:24 pm on February 10, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:2001 in 810b465f59 outdated

2000+    if (!Assume(spks.size() == 0)) {
2001+        LogPrintf(STR_INTERNAL_BUG("Error: Some output scripts were not migrated.\n"));
2002+        return std::nullopt;
2003+    }
2004+
2005+    // Legacy wallets can also contains scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for

rkrux commented at 12:25 pm on February 5, 2025:

contain

achow101 commented at 6:24 pm on February 10, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:1726 in 68e0b1306d outdated

1760-                }
1761-            }
1762-        }
1763+    // mapScripts contains all redeemScripts and witnessScripts. Therefore each script in it has
1764+    // itself, P2SH, P2WSH, and P2SH-P2WSH as a candidate.
1765+    // Invalid scripts such as P2SH-P2SH and P2WSH-P2SH, among others, will be added as candidates.

rkrux commented at 12:28 pm on February 5, 2025:

// Invalid scripts such as P2SH-P2SH and P2WSH-P2SH, among others, will be added as candidates.

In the below function, I notice script itself, P2SH, P2WSH, P2SH-P2WSH being added only. These invalid scripts could be added as candidates because the underlying script could be a P2SH?

With this script nesting involved, it makes me want to read the logic of IsMine to see how it handles it!

achow101 commented at 5:58 pm on February 10, 2025:

These invalid scripts could be added as candidates because the underlying script could be a P2SH?

Yes

in src/wallet/scriptpubkeyman.cpp:1996 in 810b465f59 outdated

1991@@ -1992,10 +1992,29 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
1992         }
1993     }
1994 
1995-    // Multisigs are special. They don't show up as ISMINE_SPENDABLE unless they are in a P2SH
1996-    // So we have to check if any of our scripts are a multisig and if so, add the P2SH
1997-    for (const auto& script_pair : mapScripts) {
1998-        const CScript script = script_pair.second;
1999+    // Make sure that we have accounted for all scriptPubKeys
2000+    if (!Assume(spks.size() == 0)) {

rkrux commented at 12:37 pm on February 5, 2025:

Nit:

0- if (!Assume(spks.size() == 0)) {
1+ if (!Assume(spks.empty())) {

achow101 commented at 6:24 pm on February 10, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:2005 in 810b465f59 outdated

2004+
2005+    // Legacy wallets can also contains scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for
2006+    // but can provide script data to a PSBT spending them. These "solvable" output scripts will need to
2007+    // be put into the separate "solvables" wallet.
2008+    // These can be detected by going through the entire candidate output scripts, finding the ISMINE_NO scripts,
2009+    // and checking CanProvide() which will dummy sign.

rkrux commented at 12:43 pm on February 5, 2025:

Thanks for this comment, very helpful!

in src/wallet/scriptpubkeyman.cpp:2034 in 810b465f59 outdated

2061-            if (desc->IsSolvable()) {
2062-                std::unique_ptr<Descriptor> wsh_desc = InferDescriptor(witprog, *GetSolvingProvider(witprog));
2063-                out.solvable_descs.emplace_back(wsh_desc->ToString(), creation_time);
2064-                std::unique_ptr<Descriptor> sh_wsh_desc = InferDescriptor(sh_wsh_spk, *GetSolvingProvider(sh_wsh_spk));
2065-                out.solvable_descs.emplace_back(sh_wsh_desc->ToString(), creation_time);
2066+        // Past bugs in InferDescriptor has caused it to create descriptors which cannot be re-parsed

rkrux commented at 12:54 pm on February 5, 2025:

have

achow101 commented at 6:24 pm on February 10, 2025:

Done

in test/functional/wallet_migration.py:1221 in eb6c32ec54 outdated

1215+        some_keys_priv_desc = descsum_create(f"wsh(or_b(pk({privkey}),s:pk(029ffbe722b147f3035c87cb1c60b9a5947dd49c774cc31e94773478711a929ac0)))")
1216+        some_keys_addr = self.master_node.deriveaddresses(some_keys_priv_desc)[0]
1217+
1218+        # Make a descriptor where we have all of the keys. This will stay in the migrated wallet
1219+        all_keys_priv_desc = descsum_create(f"wsh(and_v(v:pk({privkey}),1))")
1220+        all_keys_addr = self.master_node.deriveaddresses(all_keys_priv_desc)[0]

rkrux commented at 1:13 pm on February 5, 2025:

Although not a bug but for sanity shouldn’t these deriveaddresses calls be done in the old_node because the wallet has been created on old_node & has not been migrated at this stage?

achow101 commented at 5:59 pm on February 10, 2025:

deriveaddresses is not a wallet RPC.

rkrux commented at 12:12 pm on February 13, 2025:

Not sure if I was clear earlier. I am suggesting this change:

0some_keys_addr = self.old_node.deriveaddresses(some_keys_priv_desc)[0]
1all_keys_addr = self.old_node.deriveaddresses(all_keys_priv_desc)[0]

Reason being that these are imported in the wallet down below, a wallet that has been created on old_node. Same reasoning for the suggestion in the next comment for the test_taproot test.

achow101 commented at 3:42 pm on February 13, 2025:

It doesn’t materially make a difference. Deriving a descriptor from either one will result in the same descriptor, or it’s a bug.

in test/functional/wallet_migration.py:1276 in a3ff90dbbc outdated

1271+        tr_desc = descsum_create(f"tr({privkey})")
1272+        tr_addr = self.master_node.deriveaddresses(tr_desc)[0]
1273+        tr_spk = self.master_node.validateaddress(tr_addr)["scriptPubKey"]
1274+        tr_script_desc = descsum_create(f"tr(9ffbe722b147f3035c87cb1c60b9a5947dd49c774cc31e94773478711a929ac0,pk({privkey}))")
1275+        tr_script_addr = self.master_node.deriveaddresses(tr_script_desc)[0]
1276+        tr_script_spk = self.master_node.validateaddress(tr_script_addr)["scriptPubKey"]

rkrux commented at 1:56 pm on February 5, 2025:

self.old_node?

achow101 commented at 5:59 pm on February 10, 2025:

validateaddress is not a wallet RPC.

in test/functional/wallet_migration.py:1294 in a3ff90dbbc outdated

1288+
1289+        # Double check that the rawtr can be spent by the legacy wallet
1290+        send_res = wallet.send(outputs=[{rawtr_addr: 0.5}], include_watching=True, change_address=def_wallet.getnewaddress(), inputs=[{"txid": txid, "vout": rawtr_vout}])
1291+        assert_equal(send_res["complete"], True)
1292+        self.generate(self.old_node, 6)
1293+        assert_equal(wallet.getbalances()["watchonly"]["trusted"], 5.5)

rkrux commented at 2:07 pm on February 5, 2025:

Nit: Can add a corresponding call before sending the transaction that checks the balance is 6.

rkrux commented at 2:32 pm on February 5, 2025:

Can also add below check because there is one later down (with only 1 getbalances call though).

assert_equal(wallet.getbalances()["mine"]["trusted"], 0)

achow101 commented at 6:24 pm on February 10, 2025:

Done

in test/functional/wallet_migration.py:1289 in a3ff90dbbc outdated

1284+        rawtr_vout = find_vout_for_address(self.master_node, txid, rawtr_addr)
1285+        tr_vout = find_vout_for_address(self.master_node, txid, tr_addr)
1286+        tr_script_vout = find_vout_for_address(self.master_node, txid, tr_script_addr)
1287+        self.generate(self.master_node, 6)
1288+
1289+        # Double check that the rawtr can be spent by the legacy wallet

rkrux commented at 2:14 pm on February 5, 2025:

Where was the first check?

achow101 commented at 6:25 pm on February 10, 2025:

Removed “double”.

in test/functional/wallet_migration.py:1298 in a3ff90dbbc outdated

1291+        assert_equal(send_res["complete"], True)
1292+        self.generate(self.old_node, 6)
1293+        assert_equal(wallet.getbalances()["watchonly"]["trusted"], 5.5)
1294+
1295+        # Check that the tr() cannot be spent by the legacy wallet
1296+        send_res = wallet.send(outputs=[{def_wallet.getnewaddress(): 4}], include_watching=True, inputs=[{"txid": txid, "vout": tr_vout}, {"txid": txid, "vout": tr_script_vout}])

rkrux commented at 2:27 pm on February 5, 2025:

I can understand why tr_script_vout can’t be spent because it contains another key that the wallet doesn’t have the private key for. Why can’t the tr_vout be spent? IIUC, there’s just 1 internal key with no script path.

achow101 commented at 6:06 pm on February 10, 2025:

The legacy wallet does not know that it has the internal key for this output, nor does it know any of the other taproot spend data. Therefore, it is unable to sign the input as it does not know which key to use.

rkrux commented at 12:15 pm on February 13, 2025:

Aah right, thanks.

in test/functional/wallet_migration.py:1260 in a3ff90dbbc outdated

1255@@ -1255,6 +1256,62 @@ def test_miniscript(self):
1256         assert_equal(watchonly.getaddressinfo(some_keys_addr)["ismine"], True)
1257         assert_equal(watchonly.getaddressinfo(all_keys_addr)["ismine"], False)
1258 
1259+    def test_taproot(self):
1260+        # It turns out that due to how signing logic works, legacy wallets that have the private key for a Taproot

rkrux commented at 2:29 pm on February 5, 2025:

It turns out that due to how signing logic works,

Curious how did you end up finding this?

Sjors commented at 3:48 pm on February 5, 2025:

I think there was an earlier bugfix PR / issue around this, but I can’t find it.

achow101 commented at 6:07 pm on February 10, 2025:

Walking through the signing logic with every possible script type I could think of, and then contriving a test.

in test/functional/wallet_migration.py:1307 in a3ff90dbbc outdated

1302+        assert_equal(wallet.getbalances()["mine"], {"trusted": 0.5, "untrusted_pending": 0, "immature": 0})
1303+        assert_equal(wallet.getaddressinfo(rawtr_addr)["ismine"], True)
1304+        assert_equal(wallet.getaddressinfo(tr_addr)["ismine"], False)
1305+        assert_equal(wallet.getaddressinfo(tr_script_addr)["ismine"], False)
1306+
1307+        # The miniscript with some keys should be in the watchonly wallet

rkrux commented at 2:35 pm on February 5, 2025:

This is the comment of the test_miniscript test in the previous commit. Below can be added instead.

0The tr() should be in the watchonly wallet

achow101 commented at 6:25 pm on February 10, 2025:

Done

in test/functional/wallet_migration.py:1319 in aa49b03b2a outdated

1311@@ -1312,6 +1312,35 @@ def test_taproot(self):
1312         assert_equal(watchonly.getaddressinfo(tr_addr)["ismine"], True)
1313         assert_equal(watchonly.getaddressinfo(tr_script_addr)["ismine"], True)
1314 
1315+    def test_solvable_no_privs(self):
1316+        self.log.info("Test migrating a multisig that we do not have any private keys for")
1317+        wallet = self.create_legacy_wallet("multisig_noprivs")
1318+
1319+        privkey, pubkey = generate_keypair(compressed=True, wif=True)

rkrux commented at 2:42 pm on February 5, 2025:

0- privkey, pubkey = generate_keypair(compressed=True, wif=True)
1+ _, pubkey = generate_keypair(compressed=True, wif=True)

Besides the fact that the privkey is not used in the test, also to relate to the test name “we do not have any private keys for”.

achow101 commented at 6:25 pm on February 10, 2025:

Done

in test/functional/wallet_migration.py:1343 in aa49b03b2a outdated

1336+        assert "hex" not in addr_info
1337+
1338+        # The multisig address should be in the solvables wallet
1339+        addr_info = solvables.getaddressinfo(addr)
1340+        assert_equal(addr_info["ismine"], True)
1341+        assert_equal(addr_info["solvable"], True)

rkrux commented at 2:47 pm on February 5, 2025:

Does it make sense to check for iswatchonly: false as well to make the migration assertions more robust?

Sjors commented at 3:41 pm on February 5, 2025:

The solvables wallet itself is watch-only, so I think iswatchonly should be true?

rkrux commented at 7:34 am on February 6, 2025:

Hmm the getaddressinfo for this address displays it as false.

0{'address': 'bcrt1q4dsf6kwgrraj6v6zvcr4a4cwlrfdvmnd0mh8yqcgpzrnryep5l5s4fr46d', 'scriptPubKey': '0020ab609d59c818fb2d334266075ed70ef8d2d66e6d7eee7203080887319321a7e9', 'ismine': True, 'solvable': True, 'desc': 'wsh(multi(1,[978e2eb7]0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#str4m9tu', 'parent_desc': 'wsh(multi(1,0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#qqala45t', 'iswatchonly': False, 'isscript': True, 'iswitness': True, 'witness_version': 0, 'witness_program': 'ab609d59c818fb2d334266075ed70ef8d2d66e6d7eee7203080887319321a7e9', 'script': 'multisig', 'hex': '51210284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef851ae', 'sigsrequired': 1, 'pubkeys': ['0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8'], 'ischange': False, 'labels': ['']}

Sjors commented at 7:38 am on February 6, 2025:

I’m not sure how iswatchonly is supposed to behave in watch-only descriptor wallets. cc @achow101

achow101 commented at 3:54 pm on February 6, 2025:

Descriptor wallets never show iswatchonly for individual addresses. They are only ismine or not. That’s that whole point. The watchonly distinction lives at the wallet level, not that address level.

Sjors commented at 7:42 am on February 7, 2025:

Right, but shouldn’t getaddressinfo hide the field entirely then? (obviously not in this PR)

rkrux commented at 2:52 pm on February 5, 2025: none

Build and migration tests successful at d5e28457a099cd546e757984043f28ba9f6689b1.

Utilising isMine() instead of reverse engineering looks to me a cautious approach to take, Concept ACK. Great PR and enough context to go through for which I will review the PR again.

tests: Test migration of additional P2WSH scripts b1ab927bbf

legacy spkm: Move CanProvide to LegacyDataSPKM

This function will be needed in migration

b777e84cd7

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.

440ea1ab63

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.

fa1b7cd6e2

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.

e8c3efc7d8

test: Test migration of miniscript in legacy wallets 1eb9a2a39f

test: Test migration of taproot output scripts 17f01b0795

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.

af76664b12

in test/functional/wallet_migration.py:365 in c39b3cfcd1 outdated

350@@ -361,8 +351,6 @@ def test_other_watchonly(self):
351 
352         # Migrate
353         _, imports0 = self.migrate_and_get_rpc("imports0")
354-        assert_equal(imports0.getwalletinfo()["descriptors"], True)
355-        self.assert_is_sqlite("imports0")

davidgumberg commented at 8:53 pm on February 6, 2025:

nit: if retouching, could also drop the checks below in test_no_privkeys for watchonly0 and watchonly1 (R411, R442)

 0@@ -408,9 +408,7 @@ class WalletMigrationTest(BitcoinTestFramework):
 1         _, watchonly0 = self.migrate_and_get_rpc("watchonly0")
 2         assert_equal("watchonly0_watchonly" in self.master_node.listwallets(), False)
 3         info = watchonly0.getwalletinfo()
 4-        assert_equal(info["descriptors"], True)
 5         assert_equal(info["private_keys_enabled"], False)
 6-        self.assert_is_sqlite("watchonly0")
 7
 8         # Migrating a wallet with pubkeys added to the keypool
 9         self.log.info("Test migration of a pure watchonly wallet with pubkeys in keypool")
10@@ -439,9 +437,7 @@ class WalletMigrationTest(BitcoinTestFramework):
11
12         _, watchonly1 = self.migrate_and_get_rpc("watchonly1")
13         info = watchonly1.getwalletinfo()
14-        assert_equal(info["descriptors"], True)
15         assert_equal(info["private_keys_enabled"], False)
16-        self.assert_is_sqlite("watchonly1")
17         # After migrating, the "keypool" is empty
18         assert_raises_rpc_error(-4, "Error: This wallet has no available keys", watchonly1.getnewaddress)

achow101 commented at 6:09 pm on February 10, 2025:

Going to leave this as is since migrate_and_get_rpc does not check the descriptor-ness of the watchonly and solvables wallets.

achow101 force-pushed on Feb 10, 2025

sipa commented at 9:58 pm on February 10, 2025: member

Code review ACK af76664b12d8611b606a7e755a103a20542ee539; I did not review the tests in detail.

DrahtBot requested review from Sjors on Feb 10, 2025

DrahtBot requested review from rkrux on Feb 10, 2025

brunoerg approved

brunoerg commented at 7:42 pm on February 12, 2025: contributor

code review ACK af76664b12d8611b606a7e755a103a20542ee539

rkrux commented at 12:40 pm on February 13, 2025: none

ACK af76664b12d8611b606a7e755a103a20542ee539

I reviewed range-diff:

0git range-diff d5e28457a099cd546e757984043f28ba9f6689b1...af76664b12d8611b606a7e755a103a20542ee539

Newer changes are incorporating previous suggestions such as fixing comments, adding couple more assertions in the functional tests, getting rid of unused private key in test. Clarified my previous suggestion.

in src/wallet/scriptpubkeyman.cpp:1997 in e8c3efc7d8 outdated

1996-    // So we have to check if any of our scripts are a multisig and if so, add the P2SH
1997-    for (const auto& script_pair : mapScripts) {
1998-        const CScript script = script_pair.second;
1999+    // Make sure that we have accounted for all scriptPubKeys
2000+    if (!Assume(spks.empty())) {
2001+        LogPrintf("%s\n", STR_INTERNAL_BUG("Error: Some output scripts were not migrated.\n"));

theStack commented at 5:23 pm on February 13, 2025:

nit:

0        LogPrintf("%s\n", STR_INTERNAL_BUG("Error: Some output scripts were not migrated."));

(the expansion in StrFormatInternalBug already includes a new-line after the message)

glozow merged this on Feb 13, 2025

glozow closed this on Feb 13, 2025

in src/wallet/scriptpubkeyman.cpp:2006 in e8c3efc7d8 outdated

2005+    // Legacy wallets can also contain scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for
2006+    // but can provide script data to a PSBT spending them. These "solvable" output scripts will need to
2007+    // be put into the separate "solvables" wallet.
2008+    // These can be detected by going through the entire candidate output scripts, finding the ISMINE_NO scripts,
2009+    // and checking CanProvide() which will dummy sign.
2010+    for (const CScript& script : GetCandidateScriptPubKeys()) {

theStack commented at 5:36 pm on February 13, 2025:

nit: rather than calculating the candidate scriptPubKey set a second time, could create a non-modifiable copy the first time above at std::unordered_set<CScript, SaltedSipHasher> spks{GetScriptPubKeys()}; (probably only relevant for huge wallets, if it’s noticable at all)

theStack commented at 5:38 pm on February 13, 2025: contributor

post-merge code-review ACK af76664b12d8611b606a7e755a103a20542ee539

nit for functional test commit c39b3cfcd1bc5002aa06d1b79c948ce94f3ec8dc: there are still instances of self.migrate_and_get_rpc calls with is-descriptor/sqlite-checks after that can now be removed (e.g. in the test_no_privkeys subtest).

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

Code Coverage & Benchmarks

Reviews

Conflicts