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

1. 
   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.

2. 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
3. DrahtBot added the label Tests on Jan 7, 2026
4. 
   DrahtBot commented at 9:41 PM on January 7, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34221.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, bensig

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

5. achow101 added this to the milestone 30.2 on Jan 7, 2026
6. achow101 added the label Needs backport (30.x) on Jan 7, 2026
7. 
   achow101 commented at 10:06 PM on January 7, 2026: member

   ACK cbf0bd35bbf312f3b13d92d281d7112e4b43b9c3

8. 
   bensig commented at 10:13 PM on January 7, 2026: contributor

   ACK cbf0bd35bbf312f3b13d92d281d7112e4b43b9c3 test skipped but change is straightforward

9. fanquake merged this on Jan 8, 2026
10. fanquake closed this on Jan 8, 2026

    ---

11. fanquake referenced this in commit 9e59047a7e on Jan 8, 2026
12. fanquake removed the label Needs backport (30.x) on Jan 8, 2026
13. 
    fanquake commented at 10:25 AM on January 8, 2026: member

    Backported to 30.x in #34229.

14. 
    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.

    <details open> <summary>Working Diff</summary>

    diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
    index e90c48fa6b..3738376158 100755
    --- a/test/functional/wallet_migration.py
    +++ b/test/functional/wallet_migration.py
    @@ -130,53 +130,67 @@ class WalletMigrationTest(BitcoinTestFramework):
                 if w["name"] == wallet_name:
                     assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
     
    -        # migratewallet uses current time in naming the backup file, set a mock time
    -        # to check that this works correctly.
    -        mocked_time = int(time.time())
    -        self.master_node.setmocktime(mocked_time)
    -        # Migrate, checking that rescan does not occur
    -        with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
    -            migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
    -        self.master_node.setmocktime(0)
    -        # Update wallet name in case the initial wallet was completely migrated to a watch-only wallet
    -        # (in which case the wallet name would be suffixed by the 'watchonly' term)
    -        migrated_wallet_name = migrate_info['wallet_name']
    -        wallet = self.master_node.get_wallet_rpc(migrated_wallet_name)
    -        wallet_info = wallet.getwalletinfo()
    -        assert_equal(wallet_info["descriptors"], True)
    -        self.assert_is_sqlite(migrated_wallet_name)
    -        # Always verify the backup path exist after migration
    -        assert os.path.exists(migrate_info['backup_path'])
    -        if wallet_name == "":
    -            backup_prefix = "default_wallet"
    -        else:
    -            backup_prefix = os.path.basename(os.path.realpath(self.old_node.wallets_path / wallet_name))
    -
    -        backup_filename = f"{backup_prefix}_{mocked_time}.legacy.bak"
    -        expected_backup_path = self.master_node.wallets_path / backup_filename
    -        assert_equal(str(expected_backup_path), migrate_info['backup_path'])
    -        assert {"name": backup_filename} not in self.master_node.listwalletdir()["wallets"]
    -
    -        # Open the wallet with sqlite and verify that the wallet has the last hardened cache flag
    -        # set and the last hardened cache entries
    -        def check_last_hardened(conn):
    -            flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone()
    -            flags = int.from_bytes(flags_rec[0], byteorder="little")
    -
    -            # All wallets should have the upgrade flag set
    -            assert_equal(bool(flags & (1 << 2)), True)
    -
    -            # Fetch all records with the walletdescriptorlhcache prefix
    -            # if the wallet has private keys and is not blank
    -            if wallet_info["private_keys_enabled"] and not wallet_info["blank"]:
    -                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()
    -                assert_greater_than(len(lh_cache_recs), 0)
    -
    -        inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
    -        wallet.backupwallet(inspect_path)
    -        self.inspect_sqlite_db(inspect_path, check_last_hardened)
    -
    -        return migrate_info, wallet
    +        migrate_info = {}
    +        try:
    +            # migratewallet uses current time in naming the backup file, set a mock time
    +            # to check that this works correctly.
    +            mocked_time = int(time.time())
    +            self.master_node.setmocktime(mocked_time)
    +            # Migrate, checking that rescan does not occur
    +            with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
    +                migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
    +
    +            # Update wallet name in case the initial wallet was completely migrated to a watch-only wallet
    +            # (in which case the wallet name would be suffixed by the 'watchonly' term)
    +            migrated_wallet_name = migrate_info['wallet_name']
    +            wallet = self.master_node.get_wallet_rpc(migrated_wallet_name)
    +            wallet_info = wallet.getwalletinfo()
    +            assert_equal(wallet_info["descriptors"], True)
    +            self.assert_is_sqlite(migrated_wallet_name)
    +
    +            # Open the wallet with sqlite and verify that the wallet has the last hardened cache flag
    +            # set and the last hardened cache entries
    +            def check_last_hardened(conn):
    +                flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone()
    +                flags = int.from_bytes(flags_rec[0], byteorder="little")
    +
    +                # All wallets should have the upgrade flag set
    +                assert_equal(bool(flags & (1 << 2)), True)
    +
    +                # Fetch all records with the walletdescriptorlhcache prefix
    +                # if the wallet has private keys and is not blank
    +                if wallet_info["private_keys_enabled"] and not wallet_info["blank"]:
    +                    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()
    +                    assert_greater_than(len(lh_cache_recs), 0)
    +
    +            inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
    +            wallet.backupwallet(inspect_path)
    +            self.inspect_sqlite_db(inspect_path, check_last_hardened)
    +            return migrate_info, wallet
    +
    +        except Exception as e:
    +            # Verify the original legacy wallet was restored in case of error
    +            assert (self.master_node.wallets_path / wallet_name / "wallet.dat").exists()
    +            # And verify it is still a BDB wallet
    +            self.assert_is_bdb(wallet_name)
    +            raise e
    +
    +        finally:
    +            self.master_node.setmocktime(0)
    +
    +            # Always verify the backup path exists after migration, even in failure cases
    +            if wallet_name == "":
    +                backup_prefix = "default_wallet"
    +            else:
    +                backup_prefix = os.path.basename(os.path.realpath(self.old_node.wallets_path / wallet_name))
    +
    +            backup_filename = f"{backup_prefix}_{mocked_time}.legacy.bak"
    +            assert self.master_node.wallets_path.exists() # Verify the /wallets/ path exists
    +            assert {"name": backup_filename} not in self.master_node.listwalletdir()["wallets"]
    +            if migrate_info:
    +                expected_backup_path = self.master_node.wallets_path / backup_filename
    +                assert_equal(str(expected_backup_path), migrate_info['backup_path'])
    +                assert os.path.exists(migrate_info['backup_path'])
     
         def test_basic(self):
             default = self.master_node.get_wallet_rpc(self.default_wallet_name)
    @@ -543,7 +557,7 @@ class WalletMigrationTest(BitcoinTestFramework):
             assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", self.migrate_and_get_rpc, "encrypted")
     
             # Use the RPC directly on the master node for the rest of these checks
    -        self.master_node.bumpmocktime(1) # Prevents filename duplication on wallet backups which is a problem on Windows
    +        self.master_node.setmocktime(int(time.time()))
             assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", self.master_node.migratewallet, "encrypted", "badpass")
     
             self.master_node.bumpmocktime(1) # Prevents filename duplication on wallet backups which is a problem on Windows
    @@ -660,13 +674,18 @@ class WalletMigrationTest(BitcoinTestFramework):
     
             assert_equal(bals, wallet.getbalances())
     
    -    def clear_default_wallet(self, backup_file):
    +    def clear_default_wallet(self, backup_file=None):
             # Test cleanup: Clear unnamed default wallet for subsequent tests
             (self.old_node.wallets_path / "wallet.dat").unlink()
             (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True)
             shutil.rmtree(self.master_node.wallets_path / "default_wallet_watchonly", ignore_errors=True)
             shutil.rmtree(self.master_node.wallets_path / "default_wallet_solvables", ignore_errors=True)
    -        backup_file.unlink()
    +        if backup_file:
    +            backup_file.unlink()
    +        else:
    +            # Delete all legacy backup files in the directory if file not passed
    +            for file_path in self.master_node.wallets_path.glob(".legacy.bak"):
    +                file_path.unlink()
     
         def test_default_wallet(self):
             self.log.info("Test migration of the wallet named as the empty string")
    @@ -717,24 +736,10 @@ class WalletMigrationTest(BitcoinTestFramework):
             watch_only_dir = self.master_node.wallets_path / "default_wallet_watchonly"
             os.mkdir(watch_only_dir)
             shutil.copyfile(self.old_node.wallets_path / "wallet.dat", watch_only_dir / "wallet.dat")
    -
    -        mocked_time = int(time.time())
    -        self.master_node.setmocktime(mocked_time)
             assert_raises_rpc_error(-4, "Failed to create database", self.migrate_and_get_rpc, "")
    -        self.master_node.setmocktime(0)
    -
    -        # 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
    -        self.clear_default_wallet(backup_path)
    +        self.clear_default_wallet()
     
         def test_direct_file(self):
             self.log.info("Test migration of a wallet that is not in a wallet directory")
    
    

    </details>

Milestone
30.2