wallet: test: Relative wallet failed migration cleanup

davidgumberg commented at 1:01 am on January 8, 2026: contributor

Prior to #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.

DrahtBot added the label Wallet on Jan 8, 2026

DrahtBot commented at 1:01 am on January 8, 2026: 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/34226.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, achow101, rkrux
Approach ACK	w0xlt

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

w0xlt commented at 2:02 am on January 8, 2026: contributor

Approach ACK

DrahtBot added the label CI failed on Jan 8, 2026

achow101 referenced this in commit 78f48e7ba1 on Jan 8, 2026

achow101 referenced this in commit 5a1e79d881 on Jan 8, 2026

achow101 referenced this in commit 814e20ae57 on Jan 8, 2026

DrahtBot removed the label CI failed on Jan 8, 2026

in test/functional/wallet_migration.py:719 in 6ca68838d2 outdated

745+        wallet.importaddress(master_wallet.getnewaddress(address_type="legacy"))
746+
747+        # Unload legacy wallet, see migrate_and_get_rpc
748+        self.old_node.unloadwallet(wallet_name)
749+        self.old_node.loadwallet(wallet_name)
750+        self.old_node.unloadwallet(wallet_name)

furszy commented at 3:58 am on January 8, 2026:

no need to reload the wallet. The unload is to flush BDB to disk.

davidgumberg commented at 6:30 pm on January 8, 2026:

Fixed, thanks

furszy commented at 4:08 am on January 8, 2026: member

Why not generalize test_default_wallet_failure and call it three times with a different wallet name each time? The tests seem to be pretty much the same, just with a different input name.

achow101 referenced this in commit 8d81e635a0 on Jan 8, 2026

achow101 referenced this in commit 1bfcac1417 on Jan 8, 2026

in test/functional/wallet_migration.py:764 in ed73420b06 outdated

822+        os.unlink(backup_path)
823+        shutil.rmtree(absolute_path)
824+        shutil.rmtree(watch_only_dir)
825+
826+
827+

rkrux commented at 1:54 pm on January 8, 2026:

 0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
 1index 8f61d96b1e..501454f5f7 100755
 2--- a/test/functional/wallet_migration.py
 3+++ b/test/functional/wallet_migration.py
 4@@ -823,8 +823,6 @@ class WalletMigrationTest(BitcoinTestFramework):
 5         shutil.rmtree(absolute_path)
 6         shutil.rmtree(watch_only_dir)
 7 
 8-
 9-
10     def test_direct_file(self):
11         self.log.info("Test migration of a wallet that is not in a wallet directory")
12         wallet = self.create_legacy_wallet("plainfile")

davidgumberg commented at 6:29 pm on January 8, 2026:

Thanks, fixed.

in test/functional/wallet_migration.py:776 in ed73420b06

771+        assert os.path.exists(watch_only_dir / "wallet.dat")
772+
773+        # Check backup file exists.
774+        backup_prefix = os.path.basename(os.path.abspath(wallet_path.parent))
775+        backup_path = os.path.join(self.master_node.wallets_path, f"{backup_prefix}_{mocked_time}.legacy.bak")
776+        assert os.path.exists(backup_path)

rkrux commented at 1:57 pm on January 8, 2026:

There are commonalities in what we check when the migration either succeeds or fails. I suggested in PR #34221 to use migrate_and_get_rpc more in case of errors as well here in this comment: #34221#pullrequestreview-3639390846

We might be able to avoid setting mock time in the tests and checking for backup (& original) files in every test case.

davidgumberg commented at 6:30 pm on January 8, 2026:

I think this comment is partially addressed by the recent update to this branch where I refactor the three test cases to use one function, but I am happy to do further refactoring here or in a follow-up if you think it would improve things.

rkrux commented at 9:01 am on January 9, 2026:

I feel the latest code is fine for now because the commits in the PR will be part of the backport. I will raise a separate PR later trying to move the common steps in the migrate_and_get_rpc function, and see if it helps in overall readability.

rkrux commented at 1:58 pm on January 8, 2026: contributor

Agree with the addition of these test cases and also with the suggestion of reducing duplication: #34226#pullrequestreview-3637562202

davidgumberg force-pushed on Jan 8, 2026

davidgumberg commented at 6:32 pm on January 8, 2026: contributor

Rebased to address @furszy’s suggestion above of having one test function called with three different name arguments, a fair bit of special-casing is going on in the function, but I think I’ve got it as general as it can be, and I think it is still a win over the initial implementation with three separate test functions.

achow101 commented at 6:32 pm on January 8, 2026: member

Needs rebase

davidgumberg force-pushed on Jan 8, 2026

davidgumberg commented at 6:37 pm on January 8, 2026: contributor

Needs rebase

Oops, fixed.

in test/functional/wallet_migration.py:750 in f8c366dfd5

756         # Verify the /wallets/ path exists
757         assert self.master_node.wallets_path.exists()
758-        # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet'
759-        backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak"
760+
761+        # Verify the /wallets/ path exists

achow101 commented at 6:44 pm on January 8, 2026:

This is the wrong comment.

davidgumberg commented at 7:07 pm on January 8, 2026:

Fixed, thanks.

in test/functional/wallet_migration.py:769 in f8c366dfd5

781+            backup_path.unlink()
782+            Path(watch_only_dir / "wallet.dat").unlink()
783+            Path(watch_only_dir).rmdir()
784+            if is_absolute:
785+                os.unlink(os.path.join(wallet_name, "wallet.dat"))
786+            else:

achow101 commented at 6:47 pm on January 8, 2026:

I think this special case is unnecessary. You can do unlink(missing_ok=True) to avoid an error on the second one.

davidgumberg commented at 7:08 pm on January 8, 2026:

Fixed, thanks for catching this.

in test/functional/wallet_migration.py:1692 in f8c366dfd5

1685@@ -1654,7 +1686,10 @@ def run_test(self):
1686         self.test_wallet_with_relative_path()
1687         self.test_wallet_with_path("path/to/mywallet/")
1688         self.test_wallet_with_path("path/that/ends/in/..")
1689-        self.test_default_wallet_failure()
1690+        self.test_migration_failure("")
1691+        self.test_migration_failure("../")
1692+        abspath = os.path.abspath(self.master_node.datadir_path / "absolute_path")
1693+        self.test_migration_failure(abspath)

achow101 commented at 6:48 pm on January 8, 2026:

Would prefer to inline this since abspath is never reused.

davidgumberg commented at 7:07 pm on January 8, 2026:

Done, thank you.

davidgumberg force-pushed on Jan 8, 2026

DrahtBot added the label CI failed on Jan 8, 2026

davidgumberg commented at 7:09 pm on January 8, 2026: contributor

Rebased to address @achow101’s feedback.

in test/functional/wallet_migration.py:724 in dc1dc67395

725+        if is_absolute:
726+            old_path_prefix  = Path('/')
727+            master_path_prefix = Path('/')
728+        else:
729+            old_path_prefix = self.old_node.wallets_path
730+            master_path_prefix = self.master_node.wallets_path

furszy commented at 7:34 pm on January 8, 2026:

0old_path_prefix = Path() if is_absolute else self.old_node.wallets_path
1master_path_prefix = Path() if is_absolute else self.master_node.wallets_path

davidgumberg commented at 7:44 pm on January 8, 2026:

I considered doing it this way, although longer I think the current version is more legible, but if you insist I’ll give in.

furszy commented at 7:49 pm on January 8, 2026:

The only thing I’m strong changing it is the Path('/') for the empty one. Having the root directory path there is very concerning to me. The other change is merely cosmetic.

davidgumberg commented at 8:54 pm on January 8, 2026:

Good point, in latest push I’ve avoided Path('/'), and the solution here looks a bit different now:

https://github.com/bitcoin/bitcoin/blob/ccf72b2082bc020120c6ef587c8799b8d7253980/test/functional/wallet_migration.py#L727-L736

in test/functional/wallet_migration.py:733 in dc1dc67395

734+        wo_dirname = f"{wo_prefix}_watchonly"
735+
736+        if is_absolute:
737+            watch_only_dir = Path(wallet_name) / ".." / wo_dirname
738+        else:
739+            watch_only_dir = self.master_node.wallets_path / wallet_name / wo_dirname

furszy commented at 7:36 pm on January 8, 2026:

this could be:

0base_dir = Path(wallet_name).parent if is_absolute else self.master_node.wallets_path / wallet_name
1watch_only_dir = base_dir / wo_dirname

And you could use base_dir in other places as well. It should simplify the code a bit.

davidgumberg commented at 8:52 pm on January 8, 2026:

I found a solution that I think is better, the way I was doing this before would not work if you had a normally named wallet, now I just add the wo_prefix and “_watchonly” together like strings, this corresponds with how DoMigration() does it:

https://github.com/bitcoin/bitcoin/blob/6c3fb719d1a8f34352e6518e28eda0f46ce54ce7/src/wallet/wallet.cpp#L4134

This reproduces all the strange path behaviors of the migration code in the test code without some of the special casing I had before.

in test/functional/wallet_migration.py:1686 in dc1dc67395

1682@@ -1654,7 +1683,9 @@ def run_test(self):
1683         self.test_wallet_with_relative_path()
1684         self.test_wallet_with_path("path/to/mywallet/")
1685         self.test_wallet_with_path("path/that/ends/in/..")
1686-        self.test_default_wallet_failure()
1687+        self.test_migration_failure("")

furszy commented at 7:38 pm on January 8, 2026:

0        self.test_migration_failure(wallet_name="")

Same for the other two.

davidgumberg commented at 8:56 pm on January 8, 2026:

I’ve made an orthogonal but related change of putting the cases into a list and looping through the list, let me know what you think:

https://github.com/bitcoin/bitcoin/blob/ccf72b2082bc020120c6ef587c8799b8d7253980/test/functional/wallet_migration.py#L1682-L1689

DrahtBot removed the label CI failed on Jan 8, 2026

davidgumberg force-pushed on Jan 8, 2026

DrahtBot added the label CI failed on Jan 8, 2026

davidgumberg commented at 8:58 pm on January 8, 2026: contributor

Pushed to also add a test that checks the normal case, this revealed that the way I was finding wo_dir was not as general as it could be, and the latest push simplifies the test quite a bit I think.

I also made an orthogonal change of putting the cases in a list and looping through them, I think this fits nicely with / suggests a future refactor where we generalize some of our other migration tests and run them all through a list of interesting wallet names / paths.

in test/functional/wallet_migration.py:1688 in ccf72b2082

1684+            "",
1685+            "../",
1686+            os.path.abspath(self.master_node.datadir_path / "absolute_path"),
1687+            "normallynamedwallet"
1688+        ]
1689+        for wallet_name in migration_failure_cases: self.test_migration_failure(wallet_name=wallet_name)

achow101 commented at 9:45 pm on January 8, 2026:

The linter does not like this being one line.

davidgumberg commented at 10:01 pm on January 8, 2026:

Oops, fixed

achow101 referenced this in commit fd33e2dd98 on Jan 8, 2026

achow101 referenced this in commit 9a888e676d on Jan 8, 2026

wallet: test: Failed migration cleanup

Refactor a common way to perform the failed migration test that exists
for default wallets, and add relative-path wallets and absolute-path
wallets.

eeaf28dbe0

davidgumberg force-pushed on Jan 8, 2026

furszy commented at 10:49 pm on January 8, 2026: member

Thanks for the changes, looking much better now.

in test/functional/wallet_migration.py:763 in eeaf28dbe0

776+        else:
777+            backup_path.unlink()
778+            Path(watch_only_dir / "wallet.dat").unlink()
779+            Path(watch_only_dir).rmdir()
780+            Path(master_path / "wallet.dat").unlink()
781+            Path(old_path / "wallet.dat").unlink(missing_ok=True)

furszy commented at 10:51 pm on January 8, 2026:

nano nit:

0backup_path.unlink()
1for file in [watch_only_dir, master_path, old_path]:
2    (file / "wallet.dat").unlink(missing_ok=True)
3watch_only_dir.rmdir()

furszy commented at 11:04 pm on January 8, 2026: member

ACK eeaf28dbe0e09819ab0e95bb7762b29536bdeef6

DrahtBot removed the label CI failed on Jan 8, 2026

achow101 commented at 11:53 pm on January 8, 2026: member

ACK eeaf28dbe0e09819ab0e95bb7762b29536bdeef6

in test/functional/wallet_migration.py:753 in eeaf28dbe0

766-        # And verify it is still a BDB wallet
767-        self.assert_is_bdb("")
768 
769-        # Test cleanup: clear default wallet for next test
770-        self.clear_default_wallet(backup_path)
771+        self.assert_is_bdb(wallet_name)

rkrux commented at 8:59 am on January 9, 2026:

https://github.com/bitcoin/bitcoin/blob/6c3fb719d1a8f34352e6518e28eda0f46ce54ce7/test/functional/wallet_migration.py#L70-L75

It took me some time to convince myself that why is this file self.master_node.wallets_path / wallet_name / self.wallet_data_filename even opened for absolute path wallet name because of the absolute path wallet name residing outside the wallets_path dir. From the logs on my machine:

0assert_is_bdb!
1self.master_node.wallets_path: /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_yfvb1rj7/node0/regtest/wallets
2wallet_name: /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_yfvb1rj7/node0/absolute_path
3self.master_node.wallets_path / wallet_name / self.wallet_data_filename:  /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_yfvb1rj7/node0/absolute_path/wallet.dat
4Within open()!

It’s because of this caveat in pathlib: https://docs.python.org/3/library/pathlib.html#:~:text=If%20a%20segment%20is%20an%20absolute%20path%2C%20all%20previous%20segments%20are%20ignored

0If a segment is an absolute path, all previous segments are ignored (like [os.path.join()](https://docs.python.org/3/library/os.path.html#os.path.join)):
1
2>>> PurePath('/etc', '/usr', 'lib64')
3PurePosixPath('/usr/lib64')
4>>> PureWindowsPath('c:/Windows', 'd:bar')
5PureWindowsPath('d:bar')

rkrux commented at 9:00 am on January 9, 2026: contributor

lgtm ACK eeaf28dbe0e09819ab0e95bb7762b29536bdeef6

I also tested this with an absolute path wallet name outside the node dir like below:

 0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
 1index 45ebd435cd..07c14f2935 100755
 2--- a/test/functional/wallet_migration.py
 3+++ b/test/functional/wallet_migration.py
 4@@ -1679,11 +1679,14 @@ class WalletMigrationTest(BitcoinTestFramework):
 5         self.test_wallet_with_path("path/to/mywallet/")
 6         self.test_wallet_with_path("path/that/ends/in/..")
 7 
 8+        troublesomedirpath = f"{self.master_node.wallets_path}/../../../troublesomedir"
 9+        os.mkdir(troublesomedirpath)
10         migration_failure_cases = [
11             "",
12             "../",
13             os.path.abspath(self.master_node.datadir_path / "absolute_path"),
14-            "normallynamedwallet"
15+            "normallynamedwallet",
16+            os.path.abspath(troublesomedirpath)
17         ]
18         for wallet_name in migration_failure_cases:
19             self.test_migration_failure(wallet_name=wallet_name)

fanquake merged this on Jan 9, 2026

fanquake closed this on Jan 9, 2026

fanquake commented at 10:13 am on January 9, 2026: member

Backported to 30.x in #34229.

fanquake referenced this in commit 1dae0027cd on Jan 9, 2026

wallet: test: Relative wallet failed migration cleanup #34226

Code Coverage & Benchmarks

Reviews