Merge bitcoin/bitcoin#34226: wallet: test: Relative wallet failed migration cleanup

eeaf28dbe0 wallet: test: Failed migration cleanup (David Gumberg)

Pull request description:

  Prior to https://github.com/bitcoin/bitcoin/pull/34156, an issue existed where if migration of a wallet with a relative pathname failed, the relatively specified path where the legacy wallet is would be deleted. This issue predates #32273, because the relative pathnames get stacked together, e.g. "../../", the copy conflict bug that caused migration to abort early instead of getting far enough to attempt clean-up that was fixed in #32273 is avoided.

  This is a functional test demonstrating that we handle failed migration clean-up correctly for relatively-named wallets. To see the issue, you can backport this test onto 29.x: https://github.com/davidgumberg/bitcoin/tree/2026-01-07-rel-migration-test-backport

  I've also added an absolute path failed migration cleanup test. WRT this and #34156, absolute paths exhibit similar behavior to unnamed wallets. Because of the name-conflict bug prior to #32273 an absolute-path migration would fail no matter what because migration would attempt to copy a file to a destination that already exists. But after #32273, absolute-path migration gets past there, and if it fails for some other reason, the same behavior that's fixed in #34156 occurs where the directory containing the wallet file is deleted.

ACKs for top commit:
  achow101:
    ACK eeaf28dbe0
  furszy:
    ACK eeaf28dbe0
  rkrux:
    lgtm ACK eeaf28dbe0

Tree-SHA512: ee366fe526d0328654a86c2e9e6f228ca81554c8f8a78c259fa7aab90f024f9e5694ecf3f1d188938355f4e6d351c5a6a8ad236701bdd0ce63005e5d42c15e15

test/functional/wallet_migration.py

@@ -706,37 +706,61 @@ class WalletMigrationTest(BitcoinTestFramework):
         wallet.unloadwallet()
         self.clear_default_wallet(backup_file=Path(res["backup_path"]))
 
-    def test_default_wallet_failure(self):
+    def test_migration_failure(self, wallet_name):
-        self.log.info("Test failure during unnamed (default) wallet migration")
+        is_default = wallet_name == ""
+        wallet_pretty_name = "unnamed (default)" if is_default else f'"{wallet_name}"'
+        self.log.info(f"Test failure during migration of wallet named: {wallet_pretty_name}")
+        # Preface, set up legacy wallet and unload it
         master_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name)
-        wallet = self.create_legacy_wallet("", blank=True)
+        wallet = self.create_legacy_wallet(wallet_name, blank=True)
         wallet.importaddress(master_wallet.getnewaddress(address_type="legacy"))
         wallet.unloadwallet()
 
-        # Create wallet directory with the watch-only name and a wallet file.
+        if os.path.isabs(wallet_name):
-        # Because the wallet dir exists, this will cause migration to fail.
+            old_path = master_path = Path(wallet_name)
-        watch_only_dir = self.master_node.wallets_path / "default_wallet_watchonly"
+        else:
+            old_path = self.old_node.wallets_path / wallet_name
+            master_path = self.master_node.wallets_path / wallet_name
+            os.makedirs(master_path, exist_ok=True)
+            shutil.copyfile(old_path / "wallet.dat", master_path / "wallet.dat")
+
+        # This will be the watch-only directory the migration tries to create,
+        # we make migration fail by placing a wallet.dat file there.
+        wo_prefix = wallet_name or "default_wallet"
+        # wo_prefix might have path characters in it, this corresponds with
+        # DoMigration().
+        wo_dirname = f"{wo_prefix}_watchonly"
+        watch_only_dir = self.master_node.wallets_path / wo_dirname
         os.mkdir(watch_only_dir)
-        shutil.copyfile(self.old_node.wallets_path / "wallet.dat", watch_only_dir / "wallet.dat")
+        shutil.copyfile(old_path / "wallet.dat", watch_only_dir / "wallet.dat")
 
         mocked_time = int(time.time())
         self.master_node.setmocktime(mocked_time)
-        shutil.copyfile(self.old_node.wallets_path / "wallet.dat", self.master_node.wallets_path / "wallet.dat")
+        assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, wallet_name)
-        assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, wallet_name="")
         self.master_node.setmocktime(0)
 
-        # Verify the /wallets/ path exists
+        # Verify the /wallets/ path exists.
         assert self.master_node.wallets_path.exists()
-        # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet'
-        backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak"
-        assert backup_path.exists()
-        # Verify the original unnamed wallet was restored
-        assert (self.master_node.wallets_path / "wallet.dat").exists()
-        # And verify it is still a BDB wallet
-        self.assert_is_bdb("")
 
-        # Test cleanup: clear default wallet for next test
+        # Verify both wallet paths exist.
-        self.clear_default_wallet(backup_path)
+        assert Path(old_path / "wallet.dat").exists()
+        assert Path(master_path / "wallet.dat").exists()
+
+        backup_prefix = "default_wallet" if is_default else os.path.basename(os.path.abspath(master_path))
+        backup_path = self.master_node.wallets_path / f"{backup_prefix}_{mocked_time}.legacy.bak"
+        assert backup_path.exists()
+
+        self.assert_is_bdb(wallet_name)
+
+        # Cleanup
+        if is_default:
+            self.clear_default_wallet(backup_path)
+        else:
+            backup_path.unlink()
+            Path(watch_only_dir / "wallet.dat").unlink()
+            Path(watch_only_dir).rmdir()
+            Path(master_path / "wallet.dat").unlink()
+            Path(old_path / "wallet.dat").unlink(missing_ok=True)
 
     def test_direct_file(self):
         self.log.info("Test migration of a wallet that is not in a wallet directory")
@@ -1654,7 +1678,16 @@ class WalletMigrationTest(BitcoinTestFramework):
         self.test_wallet_with_relative_path()
         self.test_wallet_with_path("path/to/mywallet/")
         self.test_wallet_with_path("path/that/ends/in/..")
-        self.test_default_wallet_failure()
+
+        migration_failure_cases = [
+            "",
+            "../",
+            os.path.abspath(self.master_node.datadir_path / "absolute_path"),
+            "normallynamedwallet"
+        ]
+        for wallet_name in migration_failure_cases:
+            self.test_migration_failure(wallet_name=wallet_name)
+
         self.test_default_wallet()
         self.test_default_wallet_watch_only()
         self.test_direct_file()