test: migration, avoid backup name mismatch in default_wallet

furszy commented at 9:41 pm on January 7, 2026: member

This is a possible test failure, pushing it in case the CI starts complaining. The change affects only test code; no cpp logic is involved.

The test_default_wallet_failure migration test calls the function migrate_and_get_rpc(), which sets the mock time internally. But, at the same time, the test already caches the mock time value, to later use it in the backup existence check. Setting the mock time twice can lead to a name mismatch during the mentioned check (diff timestamp == diff backup names), which could cause the test to fail.

The fix is very simple, just need to call the migration RPC directly. Since the test expects the migration to fail, migrate_and_get_rpc() is unnecessary here. I’m surprised the CI hasn’t complained about this yet.

test: migration, avoid backup name mismatch in default_wallet_failure

The test calls migrate_and_get_rpc(), which sets mock time internally.
The caller caches a mock time value and later relies on it to predict the
backup filename, so setting the mock time again could cause a naming
mismatch.

Fix this by calling the migration RPC directly. Since the test expects the
migration to fail, migrate_and_get_rpc() is unnecessary here.

cbf0bd35bb

DrahtBot added the label Tests on Jan 7, 2026

DrahtBot commented at 9:41 pm on January 7, 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/34221.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, bensig

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

achow101 added this to the milestone 30.2 on Jan 7, 2026

achow101 added the label Needs backport (30.x) on Jan 7, 2026

achow101 commented at 10:06 pm on January 7, 2026: member

ACK cbf0bd35bbf312f3b13d92d281d7112e4b43b9c3

bensig commented at 10:13 pm on January 7, 2026: contributor

ACK cbf0bd35bbf312f3b13d92d281d7112e4b43b9c3 test skipped but change is straightforward

fanquake merged this on Jan 8, 2026

fanquake closed this on Jan 8, 2026

fanquake referenced this in commit 9e59047a7e on Jan 8, 2026

fanquake removed the label Needs backport (30.x) on Jan 8, 2026

fanquake commented at 10:25 am on January 8, 2026: member

Backported to 30.x in #34229.

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

These changes lgtm but I was thinking along the lines of PR #33104 that we can do basic sanity checking both in the cases of success and error in the migrate_and_get_rpc function itself. And do the checks common to both success and error case in a finally block. This cleans up the callers of migrate_and_get_rpc a bit.

  0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
  1index e90c48fa6b..3738376158 100755
  2--- a/test/functional/wallet_migration.py
  3+++ b/test/functional/wallet_migration.py
  4@@ -130,53 +130,67 @@ class WalletMigrationTest(BitcoinTestFramework):
  5             if w["name"] == wallet_name:
  6                 assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
  7 
  8-        # migratewallet uses current time in naming the backup file, set a mock time
  9-        # to check that this works correctly.
 10-        mocked_time = int(time.time())
 11-        self.master_node.setmocktime(mocked_time)
 12-        # Migrate, checking that rescan does not occur
 13-        with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
 14-            migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
 15-        self.master_node.setmocktime(0)
 16-        # Update wallet name in case the initial wallet was completely migrated to a watch-only wallet
 17-        # (in which case the wallet name would be suffixed by the 'watchonly' term)
 18-        migrated_wallet_name = migrate_info['wallet_name']
 19-        wallet = self.master_node.get_wallet_rpc(migrated_wallet_name)
 20-        wallet_info = wallet.getwalletinfo()
 21-        assert_equal(wallet_info["descriptors"], True)
 22-        self.assert_is_sqlite(migrated_wallet_name)
 23-        # Always verify the backup path exist after migration
 24-        assert os.path.exists(migrate_info['backup_path'])
 25-        if wallet_name == "":
 26-            backup_prefix = "default_wallet"
 27-        else:
 28-            backup_prefix = os.path.basename(os.path.realpath(self.old_node.wallets_path / wallet_name))
 29-
 30-        backup_filename = f"{backup_prefix}_{mocked_time}.legacy.bak"
 31-        expected_backup_path = self.master_node.wallets_path / backup_filename
 32-        assert_equal(str(expected_backup_path), migrate_info['backup_path'])
 33-        assert {"name": backup_filename} not in self.master_node.listwalletdir()["wallets"]
 34-
 35-        # Open the wallet with sqlite and verify that the wallet has the last hardened cache flag
 36-        # set and the last hardened cache entries
 37-        def check_last_hardened(conn):
 38-            flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone()
 39-            flags = int.from_bytes(flags_rec[0], byteorder="little")
 40-
 41-            # All wallets should have the upgrade flag set
 42-            assert_equal(bool(flags & (1 << 2)), True)
 43-
 44-            # Fetch all records with the walletdescriptorlhcache prefix
 45-            # if the wallet has private keys and is not blank
 46-            if wallet_info["private_keys_enabled"] and not wallet_info["blank"]:
 47-                lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'walletdescriptorlhcache').hex()}' AND key < x'{ser_string(b'walletdescriptorlhcachf').hex()}'").fetchall()
 48-                assert_greater_than(len(lh_cache_recs), 0)
 49-
 50-        inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
 51-        wallet.backupwallet(inspect_path)
 52-        self.inspect_sqlite_db(inspect_path, check_last_hardened)
 53-
 54-        return migrate_info, wallet
 55+        migrate_info = {}
 56+        try:
 57+            # migratewallet uses current time in naming the backup file, set a mock time
 58+            # to check that this works correctly.
 59+            mocked_time = int(time.time())
 60+            self.master_node.setmocktime(mocked_time)
 61+            # Migrate, checking that rescan does not occur
 62+            with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
 63+                migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
 64+
 65+            # Update wallet name in case the initial wallet was completely migrated to a watch-only wallet
 66+            # (in which case the wallet name would be suffixed by the 'watchonly' term)
 67+            migrated_wallet_name = migrate_info['wallet_name']
 68+            wallet = self.master_node.get_wallet_rpc(migrated_wallet_name)
 69+            wallet_info = wallet.getwalletinfo()
 70+            assert_equal(wallet_info["descriptors"], True)
 71+            self.assert_is_sqlite(migrated_wallet_name)
 72+
 73+            # Open the wallet with sqlite and verify that the wallet has the last hardened cache flag
 74+            # set and the last hardened cache entries
 75+            def check_last_hardened(conn):
 76+                flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone()
 77+                flags = int.from_bytes(flags_rec[0], byteorder="little")
 78+
 79+                # All wallets should have the upgrade flag set
 80+                assert_equal(bool(flags & (1 << 2)), True)
 81+
 82+                # Fetch all records with the walletdescriptorlhcache prefix
 83+                # if the wallet has private keys and is not blank
 84+                if wallet_info["private_keys_enabled"] and not wallet_info["blank"]:
 85+                    lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'walletdescriptorlhcache').hex()}' AND key < x'{ser_string(b'walletdescriptorlhcachf').hex()}'").fetchall()
 86+                    assert_greater_than(len(lh_cache_recs), 0)
 87+
 88+            inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
 89+            wallet.backupwallet(inspect_path)
 90+            self.inspect_sqlite_db(inspect_path, check_last_hardened)
 91+            return migrate_info, wallet
 92+
 93+        except Exception as e:
 94+            # Verify the original legacy wallet was restored in case of error
 95+            assert (self.master_node.wallets_path / wallet_name / "wallet.dat").exists()
 96+            # And verify it is still a BDB wallet
 97+            self.assert_is_bdb(wallet_name)
 98+            raise e
 99+
100+        finally:
101+            self.master_node.setmocktime(0)
102+
103+            # Always verify the backup path exists after migration, even in failure cases
104+            if wallet_name == "":
105+                backup_prefix = "default_wallet"
106+            else:
107+                backup_prefix = os.path.basename(os.path.realpath(self.old_node.wallets_path / wallet_name))
108+
109+            backup_filename = f"{backup_prefix}_{mocked_time}.legacy.bak"
110+            assert self.master_node.wallets_path.exists() # Verify the /wallets/ path exists
111+            assert {"name": backup_filename} not in self.master_node.listwalletdir()["wallets"]
112+            if migrate_info:
113+                expected_backup_path = self.master_node.wallets_path / backup_filename
114+                assert_equal(str(expected_backup_path), migrate_info['backup_path'])
115+                assert os.path.exists(migrate_info['backup_path'])
116 
117     def test_basic(self):
118         default = self.master_node.get_wallet_rpc(self.default_wallet_name)
119@@ -543,7 +557,7 @@ class WalletMigrationTest(BitcoinTestFramework):
120         assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", self.migrate_and_get_rpc, "encrypted")
121 
122         # Use the RPC directly on the master node for the rest of these checks
123-        self.master_node.bumpmocktime(1) # Prevents filename duplication on wallet backups which is a problem on Windows
124+        self.master_node.setmocktime(int(time.time()))
125         assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", self.master_node.migratewallet, "encrypted", "badpass")
126 
127         self.master_node.bumpmocktime(1) # Prevents filename duplication on wallet backups which is a problem on Windows
128@@ -660,13 +674,18 @@ class WalletMigrationTest(BitcoinTestFramework):
129 
130         assert_equal(bals, wallet.getbalances())
131 
132-    def clear_default_wallet(self, backup_file):
133+    def clear_default_wallet(self, backup_file=None):
134         # Test cleanup: Clear unnamed default wallet for subsequent tests
135         (self.old_node.wallets_path / "wallet.dat").unlink()
136         (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True)
137         shutil.rmtree(self.master_node.wallets_path / "default_wallet_watchonly", ignore_errors=True)
138         shutil.rmtree(self.master_node.wallets_path / "default_wallet_solvables", ignore_errors=True)
139-        backup_file.unlink()
140+        if backup_file:
141+            backup_file.unlink()
142+        else:
143+            # Delete all legacy backup files in the directory if file not passed
144+            for file_path in self.master_node.wallets_path.glob(".legacy.bak"):
145+                file_path.unlink()
146 
147     def test_default_wallet(self):
148         self.log.info("Test migration of the wallet named as the empty string")
149@@ -717,24 +736,10 @@ class WalletMigrationTest(BitcoinTestFramework):
150         watch_only_dir = self.master_node.wallets_path / "default_wallet_watchonly"
151         os.mkdir(watch_only_dir)
152         shutil.copyfile(self.old_node.wallets_path / "wallet.dat", watch_only_dir / "wallet.dat")
153-
154-        mocked_time = int(time.time())
155-        self.master_node.setmocktime(mocked_time)
156         assert_raises_rpc_error(-4, "Failed to create database", self.migrate_and_get_rpc, "")
157-        self.master_node.setmocktime(0)
158-
159-        # Verify the /wallets/ path exists
160-        assert self.master_node.wallets_path.exists()
161-        # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet'
162-        backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak"
163-        assert backup_path.exists()
164-        # Verify the original unnamed wallet was restored
165-        assert (self.master_node.wallets_path / "wallet.dat").exists()
166-        # And verify it is still a BDB wallet
167-        self.assert_is_bdb("")
168 
169         # Test cleanup: clear default wallet for next test
170-        self.clear_default_wallet(backup_path)
171+        self.clear_default_wallet()
172 
173     def test_direct_file(self):
174         self.log.info("Test migration of a wallet that is not in a wallet directory")

test: migration, avoid backup name mismatch in default_wallet_failure #34221

Code Coverage & Benchmarks

Reviews