Merge bitcoin/bitcoin#29124: test: Test that migration automatically repairs corrupted metadata with doubled derivation path

c7e2b9e264 tests: Test migration cleans up bad inactive chain derivation path (Ava Chow)

Pull request description:

  A bug in 0.21.x and 22.x resulted in some wallets having invalid derivation paths that are the concatenation of two derivation paths. These appear only when inactive hd chains are topped up.

  Since key metadata is a legacy wallet only record, migrating legacy wallets to descriptor wallets will fix this issue as all key metadata records are deleted. The derivation path information is derived on-the-fly from the descriptor that is produced for the inactive hd chain.

  Thus we only need a test to verify that the derivation paths are good, and that all key metadata records are deleted from the migrated wallet.

ACKs for top commit:
  murchandamus:
    re-ACK c7e2b9e264 via range-diff:
  rkrux:
    re-ACK c7e2b9e264
  furszy:
    utACK c7e2b9e264

Tree-SHA512: 3117c4a43798972109fe2d3539341a8b69db70c6457fcabdd019e6044834dc4b17212abbc006d7b8008f560dce4b7856142b057981b9404f406d58fa0955cbd9

test/functional/wallet_backwards_compatibility.py

@@ -26,6 +26,7 @@ from test_framework.util import (
     assert_raises_rpc_error,
 )
 
+LAST_KEYPOOL_INDEX = 9 # Index of the last derived address with the keypool size of 10
 
 class BackwardsCompatibilityTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -38,7 +39,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v25.0
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v24.0.1
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v23.0
-            ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v22.0
+            ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1", f"-keypool={LAST_KEYPOOL_INDEX + 1}"], # v22.0
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v0.21.0
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v0.20.1
         ]
@@ -81,6 +82,85 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         node_major, _, _ = self.split_version(node)
         return node_major >= major
 
+    def test_v22_inactivehdchain_path(self):
+        self.log.info("Testing inactive hd chain bad derivation path cleanup")
+        # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
+        # Make sure that this is being automatically cleaned up by migration
+        node_master = self.nodes[1]
+        node_v22 = self.nodes[self.num_nodes - 5]
+        wallet_name = "bad_deriv_path"
+        node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
+        bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)
+
+        # Make a dump of the wallet to get an unused address
+        dump_path = node_v22.wallets_path / f"{wallet_name}.dump"
+        bad_deriv_wallet.dumpwallet(dump_path)
+        addr = None
+        seed = None
+        with open(dump_path, encoding="utf8") as f:
+            for line in f:
+                if f"hdkeypath=m/0'/0'/{LAST_KEYPOOL_INDEX}'" in line:
+                    addr = line.split(" ")[4].split("=")[1]
+                elif " hdseed=1 " in line:
+                    seed = line.split(" ")[0]
+        assert addr is not None
+        assert seed is not None
+        # Rotate seed and unload
+        bad_deriv_wallet.sethdseed()
+        bad_deriv_wallet.unloadwallet()
+        # Receive at addr to trigger inactive chain topup on next load
+        self.nodes[0].sendtoaddress(addr, 1)
+        self.generate(self.nodes[0], 1, sync_fun=self.no_op)
+        self.sync_all(nodes=[self.nodes[0], node_master, node_v22])
+        node_v22.loadwallet(wallet_name)
+
+        # Dump again to find bad hd keypath
+        bad_deriv_path = f"m/0'/0'/{LAST_KEYPOOL_INDEX}'/0'/0'/{LAST_KEYPOOL_INDEX + 1}'"
+        good_deriv_path = f"m/0h/0h/{LAST_KEYPOOL_INDEX + 1}h"
+        os.unlink(dump_path)
+        bad_deriv_wallet.dumpwallet(dump_path)
+        bad_path_addr = None
+        with open(dump_path, encoding="utf8") as f:
+            for line in f:
+                if f"hdkeypath={bad_deriv_path}" in line:
+                    bad_path_addr = line.split(" ")[4].split("=")[1]
+        assert bad_path_addr is not None
+        assert_equal(bad_deriv_wallet.getaddressinfo(bad_path_addr)["hdkeypath"], bad_deriv_path)
+
+        # Verify that this bad derivation path addr is actually at m/0'/0'/10' by making a new wallet with the same seed but larger keypool
+        node_v22.createwallet(wallet_name="path_verify", descriptors=False, blank=True)
+        verify_wallet = node_v22.get_wallet_rpc("path_verify")
+        verify_wallet.sethdseed(True, seed)
+        # Bad addr is after keypool, so need to generate it by refilling
+        verify_wallet.keypoolrefill(LAST_KEYPOOL_INDEX + 2)
+        assert_equal(verify_wallet.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path.replace("h", "'"))
+
+        # Migrate with master
+        # Since all keymeta records are now deleted after migration, the derivation path
+        # should now be correct as it is derived on-the-fly from the inactive hd chain's descriptor
+        backup_path = node_v22.wallets_path / f"{wallet_name}.bak"
+        bad_deriv_wallet.backupwallet(backup_path)
+        wallet_dir_master = os.path.join(node_master.wallets_path, wallet_name)
+        os.makedirs(wallet_dir_master, exist_ok=True)
+        shutil.copy(backup_path, os.path.join(wallet_dir_master, "wallet.dat"))
+        node_master.migratewallet(wallet_name)
+        bad_deriv_wallet_master = node_master.get_wallet_rpc(wallet_name)
+        assert_equal(bad_deriv_wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path)
+        bad_deriv_wallet_master.unloadwallet()
+
+        # If we have sqlite3, verify that there are no keymeta records
+        try:
+            import sqlite3
+            wallet_db = node_master.wallets_path / wallet_name / "wallet.dat"
+            conn = sqlite3.connect(wallet_db)
+            with conn:
+                # Retrieve all records that have the "keymeta" prefix. The remaining key data varies for each record.
+                keymeta_rec = conn.execute("SELECT value FROM main where key >= x'076b65796d657461' AND key < x'076b65796d657462'").fetchone()
+                assert_equal(keymeta_rec, None)
+            conn.close()
+        except ImportError:
+            self.log.warning("sqlite3 module not available, skipping lack of keymeta records check")
+
     def run_test(self):
         node_miner = self.nodes[0]
         node_master = self.nodes[1]
@@ -300,5 +380,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
             # Legacy wallets are no longer supported. Trying to load these should result in an error
             assert_raises_rpc_error(-18, "The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC)", node_master.restorewallet, wallet_name, backup_path)
 
+        self.test_v22_inactivehdchain_path()
+
 if __name__ == '__main__':
     BackwardsCompatibilityTest(__file__).main()