Merge bitcoin/bitcoin#35445: wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated

ec2adf3c51 test: Check miniscript descriptor h and apostrophe equivalence (w0xlt)
a2d001b57c test: Enforce descriptor reimport is an update (Ava Chow)
e2b2f1c5c6 descriptor: Rename DescriptorID to CompatDescriptorHash (Ava Chow)
6ad31c062c test: Add 31.0 to wallet backwards compatibility test (Ava Chow)
2a6c53371b wallet, spkm: Treat Descriptor ID as an opaque SPKM ID (Ava Chow)
62e826fa76 wallet: Update WalletDescriptor from another one instead of overwriting (Ava Chow)
1113f7590e wallet, export: Include descriptor cache when exporting descriptors (Ava Chow)
9fc7b2618b spkm: Remove DescriptorSPKM constructor that doesn't take a descriptor (Ava Chow)
770ff64bd7 test: Add v30.2 and Miniscript to wallet backwards compatibility test (Ava Chow)
35d6a60dbf descriptor: Add ToCanonicalString (Ava Chow)
1d87af26ce descriptors: Remove default StringType from PubkeyProvider::ToString() (Ava Chow)
1c7f9aaf75 miniscript: Don't use StringType::COMPAT (Ava Chow)

Pull request description:

  Since keys in Miniscript expressions were not correctly handling `StringType::COMPAT` when generating the Descriptor ID, in order to keep compatibility with previous versions, we need to continue to handle that enum incorrectly when computing the ID.

  Given that this it the second time that we have had this issue, this PR also drops the concept of Descriptor ID being something that we can validate. Instead, the ID read in from the database is treated as an opaque blob that is used only to tie together the records related to a particular SPKM. It is instead treated as a ScriptPubKeyMan ID and users of it must be retrieving the ID from somewhere rather than computing it from a descriptor. The check of comparing the read ID to the computed ID is removed so that all previously created wallets can be read.

  To clarify that the ID is not actually an ID, the function `DescriptorID` is renamed to `CompatDescriptorHash` and it is still used to generate the SPKM ID that is written to the database.

  The ID was additionally being used to determine whether a descriptor is equal to another descriptor. This was used only by `importdescriptors` and `createwalletdescriptor`. These uses have been changed to do a string comparison rather than computing a hash and comparing the hashes. This removes the need to rely on `CompatDescriptorHash`.

  The only caveat is that previously the hash was being used to do a map lookup in `m_spk_managers`, but this is now changed to use `std::find_if`. The lookup complexity changes from logarithmic to linear, which may be really bad for wallets with a lot of descriptors, e.g. migrated formerly non-HD wallets. I think in general though, the tradeoff is okay, and neither of these functions purport to be performant, especially as `importdescriptors` may also do a rescan which can take a long time. However, if that is a concern, an additional map of `CompatDescriptorHash` to DescriptorSPKM can be added.

  Lastly, the wallet backwards compatibility test is updated to have 30.2 and 31.0 nodes, and a wallet with miniscript expressions. This exercises both creating wallets in previous versions and making sure they load in master, and making new wallets on master and checking whether they load, depending on the version.

  Fixes #35432

ACKs for top commit:
  pseudoramdom:
    ACK ec2adf3c51
  davidgumberg:
    crACK ec2adf3c51
  w0xlt:
    ACK ec2adf3c51

Tree-SHA512: a32995c171b829a874cfd1bb03adde46fd8737322d5c44bc2ff27eff1ea8742c16c7ea1bb6fdc0fb2b89d0f11919850383799c3af126e7f55fe0878a8f1a7024

test/functional/wallet_backwards_compatibility.py

@@ -36,11 +36,13 @@ LAST_KEYPOOL_INDEX = 9 # Index of the last derived address with the keypool size
 class BackwardsCompatibilityTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
-        self.num_nodes = 8
+        self.num_nodes = 10
         # Add new version after each release:
         self.extra_args = [
             ["-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to mine blocks. noban for immediate tx relay
             ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to receive coins, swap wallets, etc
+            ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v31.0
+            ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v30.2
             ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v25.0
             ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v24.0.1
             ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v23.0
@@ -58,6 +60,8 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
             None,
             None,
+            310000,
+            300200,
             250000,
             240001,
             230000,
@@ -269,7 +273,8 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         node_v21 = self.nodes[self.num_nodes - 2]
         node_v20 = self.nodes[self.num_nodes - 1] # bdb only
 
-        legacy_nodes = self.nodes[2:] # Nodes that support legacy wallets
+        previous_nodes = self.nodes[2:] # All previous version nodes
+        legacy_nodes = self.nodes[-6:] # Nodes that support legacy wallets
         descriptors_nodes = self.nodes[2:-1] # Nodes that support descriptor wallets
 
         self.generatetoaddress(node_miner, COINBASE_MATURITY + 1, node_miner.getnewaddress())
@@ -323,13 +328,23 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         assert info['private_keys_enabled']
         assert_equal(info['keypoolsize'], 0)
 
+        node_master.createwallet(wallet_name="miniscript")
+        wallet = node_master.get_wallet_rpc("miniscript")
+        miniscript_desc = "wsh(or_b(pk([deadbeef/0h/1h/2h]tprv8ZgxMBicQKsPerQj6m35no46amfKQdjY7AhLnmatHYXs8S4MTgeZYkWAn4edSGwwL3vkSiiGqSZQrmy5D3P5gBoqgvYP2fCUpBwbKTMTAkL/3h/*),s:pk([beefdead/4h/5h]tpubD6NzVbkrYhZ4YU9vM1s53UhD75UyJatx8EMzMZ3VUjR2FciNfLLkAw6a4pWACChzobTseNqdWk4G7ZdBqRDLtLSACKykTScmqibb1ZrCvJu/6/7/*)))"
+        miniscript_apos = miniscript_desc.replace("[beefdead/4h/5h]", "[beefdead/4'/5']")
+        assert miniscript_apos != miniscript_desc
+        for desc in [miniscript_desc, miniscript_apos]:
+            res = wallet.importdescriptors([{"desc": descsum_create(desc), "timestamp":"now"}])
+            assert_equal(res[0]["success"], True)
+
         # Unload wallets and copy to older nodes:
         node_master_wallets_dir = node_master.wallets_path
         node_master.unloadwallet("w1")
         node_master.unloadwallet("w2")
         node_master.unloadwallet("w3")
+        node_master.unloadwallet("miniscript")
 
-        for node in legacy_nodes:
+        for node in previous_nodes:
             # Copy wallets to previous version
             for wallet in os.listdir(node_master_wallets_dir):
                 dest = node.wallets_path / wallet
@@ -341,12 +356,23 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         # since we can no longer create legacy wallets.
         for node in descriptors_nodes:
             self.log.info(f"- {node.version}")
-            for wallet_name in ["w1", "w2", "w3"]:
+            for wallet_name in ["w1", "w2", "w3", "miniscript"]:
                 if self.major_version_less_than(node, 22) and wallet_name == "w1":
                     # Descriptor wallets created after 0.21 have taproot descriptors which 0.21 does not support, tested below
                     continue
+                if self.major_version_less_than(node, 24) and wallet_name == "miniscript":
+                    # Miniscript was introduced in 24.0
+                    continue
                 # Also try to reopen on master after opening on old
                 for n in [node, node_master]:
+                    # 31.0 has a descriptor id calculation incompatibility.
+                    # Miniscript descriptors imported into node versions other than 31.0 will
+                    # result in wallets that cannot be loaded into 31.0.
+                    # These wallets will emit a "Wallet corrupted" error.
+                    if wallet_name == "miniscript" and n.version == 310000:
+                        assert_raises_rpc_error(-4, "Wallet corrupted", n.loadwallet, wallet_name)
+                        continue
+
                     n.loadwallet(wallet_name)
                     wallet = n.get_wallet_rpc(wallet_name)
                     info = wallet.getwalletinfo()
@@ -368,9 +394,15 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
                     elif wallet_name == "w2":
                         assert_equal(info['private_keys_enabled'], False)
                         assert_equal(info['keypoolsize'], 0)
-                    else:
+                    elif wallet_name == "w3":
                         assert_equal(info['private_keys_enabled'], True)
                         assert_equal(info['keypoolsize'], 0)
+                    elif wallet_name == "miniscript":
+                        for desc in wallet.listdescriptors()["descriptors"]:
+                            if desc["desc"].startswith("wsh(or_b(pk"):
+                                break
+                        else:
+                            assert False, "Did not find miniscript descriptor"
 
                     # Copy back to master
                     wallet.unloadwallet()
@@ -405,6 +437,10 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
             hdkeypath = addr_info["hdkeypath"].replace("'", "h")
             pubkey = addr_info["pubkey"]
 
+            if self.major_version_at_least(node, 24):
+                res = wallet_prev.importdescriptors([{"desc": descsum_create(miniscript_desc), "timestamp":"now"}])
+                assert_equal(res[0]["success"], True)
+
             # Make a backup of the wallet file
             backup_path = os.path.join(self.options.tmpdir, f"{wallet_name}.dat")
             wallet_prev.backupwallet(backup_path)
@@ -429,6 +465,13 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
             descriptor = f"wpkh([{info['hdmasterfingerprint']}{hdkeypath[1:]}]{pubkey})"
             assert_equal(info["desc"], descsum_create(descriptor))
 
+            if self.major_version_at_least(node, 24):
+                for desc in wallet.listdescriptors()["descriptors"]:
+                    if desc["desc"].startswith("wsh(or_b(pk"):
+                        break
+                else:
+                    assert False, "Did not find miniscript descriptor"
+
             # Make backup so the wallet can be copied back to old node
             down_wallet_name = f"re_down_{node.version}"
             down_backup_path = os.path.join(self.options.tmpdir, f"{down_wallet_name}.dat")

test/functional/wallet_importdescriptors.py

@@ -345,11 +345,14 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         assert_equal(w1.getwalletinfo()['keypoolsize'], 0)
 
         self.log.info("Test can import same descriptor with public key twice")
+        list_descs = w1.listdescriptors()
         self.test_importdesc(import_request, success=True)
+        assert_equal(list_descs, w1.listdescriptors())
 
         self.log.info("Test can update descriptor label")
         self.test_importdesc({**import_request, "label": "Updated label"}, success=True)
         test_address(w1, key.p2pkh_addr, solvable=True, ismine=True, labels=["Updated label"])
+        assert_equal(list_descs, w1.listdescriptors())
 
         self.log.info("Internal addresses cannot have labels")
         self.test_importdesc({**import_request, "internal": True},
@@ -615,7 +618,9 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         self.log.info("Check we can change next_index")
         # go back and forth with next_index
         for i in [4, 0, 2, 1, 3]:
-            self.test_importdesc({'desc': descsum_create('wpkh([80002067/0h/0h]' + xpub + '/*)'),
+            # Sometimes use h, sometimes use ' for hardened indicator
+            hard = "'" if i & 2 == 0 else "h"
+            self.test_importdesc({'desc': descsum_create(f'wpkh([80002067/0{hard}/0{hard}]' + xpub + '/*)'),
                                   'active': True,
                                   'range': [0, 9],
                                   'next_index': i,
@@ -624,6 +629,24 @@ class ImportDescriptorsTest(BitcoinTestFramework):
                                  success=True)
             assert_equal(w1.getnewaddress('', 'bech32'), addresses[i])
 
+        self.log.info("Equivalent Miniscript descriptors should not be duplicated")
+        self.nodes[1].createwallet(wallet_name="wminiscript", disable_private_keys=True, blank=True)
+        wminiscript = self.nodes[1].get_wallet_rpc("wminiscript")
+        miniscript_request = {
+            'active': True,
+            'range': [0, 9],
+            'timestamp': 'now',
+        }
+        self.test_importdesc({
+            **miniscript_request,
+            'desc': descsum_create(f"wsh(and_v(v:pk([80002067/0h/0h]{xpub}/*),older(1)))"),
+        }, success=True, wallet=wminiscript)
+        self.test_importdesc({
+            **miniscript_request,
+            'desc': descsum_create(f"wsh(and_v(v:pk([80002067/0'/0']{xpub}/*),older(1)))"),
+        }, success=True, wallet=wminiscript)
+        assert_equal(len(wminiscript.listdescriptors()["descriptors"]), 1)
+
         # Check active=False default
         self.log.info('Check imported descriptors are not active by default')
         self.test_importdesc({'desc': descsum_create('pkh([12345678/1h]' + xpub + '/*)'),

test/get_previous_releases.py

@@ -91,6 +91,24 @@ SHA256_SUMS = {
     "866a4b703a2095301151c17dcc753e19e4dba61ec68d19709ec4f81ff4320103": {"tag": "v28.2", "archive": "bitcoin-28.2-x86_64-apple-darwin.tar.gz"},
     "98add5f220c01b387343b70edeb6273403fe081e22cd85fda132704cdcaa98aa": {"tag": "v28.2", "archive": "bitcoin-28.2-x86_64-linux-gnu.tar.gz"},
     "da0869639c323bbf6f264f1829083b9514e10179b90c34b09d8cbcab8a1897e3": {"tag": "v28.2", "archive": "bitcoin-28.2-win64.zip"},
+
+    "73e76c14edc79808a0511c744d102ffbb494807ee90cbcba176568243254b532": {"tag": "v30.2", "archive": "bitcoin-30.2-aarch64-linux-gnu.tar.gz"},
+    "d510542842318ea34d87cb2c93d6a7fe091dcac2e8684460be2b3c44843fb502": {"tag": "v30.2", "archive": "bitcoin-30.2-arm-linux-gnueabihf.tar.gz"},
+    "c2ecab62891de22228043815cb6211549a32272be3d5d052ff19847d3420bd10": {"tag": "v30.2", "archive": "bitcoin-30.2-arm64-apple-darwin.tar.gz"},
+    "db8803f11f8259794864b8b0d2ef8a1a27d01a5943ff4f525bc26a325031fa87": {"tag": "v30.2", "archive": "bitcoin-30.2-powerpc64-linux-gnu.tar.gz"},
+    "b0302e4d9579d19a9a501f1278e5d2c56d33fd9583040f34802d8567a1f81ace": {"tag": "v30.2", "archive": "bitcoin-30.2-riscv64-linux-gnu.tar.gz"},
+    "99d5cee9b9c37be506396c30837a4b98e320bfea71c474d6120a7e8eb6075c7b": {"tag": "v30.2", "archive": "bitcoin-30.2-x86_64-apple-darwin.tar.gz"},
+    "6aa7bb4feb699c4c6262dd23e4004191f6df7f373b5d5978b5bcdd4bb72f75d8": {"tag": "v30.2", "archive": "bitcoin-30.2-x86_64-linux-gnu.tar.gz"},
+    "0d7e1f16f8823aa26d29b44855ff6dbac11c03d75631a6c1d2ea5fab3a84fdf8": {"tag": "v30.2", "archive": "bitcoin-30.2-win64.zip"},
+
+    "4de1d568dedd48604f75132421bc0abeca432639589b49a3909c81db3a813112": {"tag": "v31.0", "archive": "bitcoin-31.0-aarch64-linux-gnu.tar.gz"},
+    "8c19d007bfc73502625095ea4073af3a98ceb722d500556ab173bac5bcadd0d6": {"tag": "v31.0", "archive": "bitcoin-31.0-arm-linux-gnueabihf.tar.gz"},
+    "a2d7a13b4da53d4a3e4c517f3a0269e2429813417bb320d3b268993cfdc545d0": {"tag": "v31.0", "archive": "bitcoin-31.0-arm64-apple-darwin.tar.gz"},
+    "1d9c865aa0ccf675fc068e79d9fa57a5a70b59132fca38bb322a7d44ce2f0ff2": {"tag": "v31.0", "archive": "bitcoin-31.0-powerpc64-linux-gnu.tar.gz"},
+    "7ece4ea365bba9b2008b27f0717ef6a518598a572edaa2815e775faadc53c136": {"tag": "v31.0", "archive": "bitcoin-31.0-riscv64-linux-gnu.tar.gz"},
+    "56824dd705bc2a3b22d42e8aa02ed53498d491ff7c2c8aa96831333871887ead": {"tag": "v31.0", "archive": "bitcoin-31.0-x86_64-apple-darwin.tar.gz"},
+    "d3e4c58a35b1d0a97a457462c94f55501ad167c660c245cb1ffa565641c65074": {"tag": "v31.0", "archive": "bitcoin-31.0-x86_64-linux-gnu.tar.gz"},
+    "82fd2c504a0f20a31d4d13bd407783d6fc7bf17622d0ce85228a9b92694e03f0": {"tag": "v31.0", "archive": "bitcoin-31.0-win64.zip"},
 }