Skip to content

Conversation

@furszy
Copy link
Member

@furszy furszy commented Jan 7, 2026

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.

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.
@DrahtBot DrahtBot added the Tests label Jan 7, 2026
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 7, 2026

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 achow101 added this to the 30.2 milestone Jan 7, 2026
@achow101
Copy link
Member

achow101 commented Jan 7, 2026

ACK cbf0bd3

@bensig
Copy link
Contributor

bensig commented Jan 7, 2026

ACK cbf0bd3
test skipped but change is straightforward

@fanquake fanquake merged commit 8d5700a into bitcoin:master Jan 8, 2026
27 checks passed
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 8, 2026
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.

Github-Pull: bitcoin#34221
Rebased-From: cbf0bd3
@fanquake
Copy link
Member

fanquake commented Jan 8, 2026

Backported to 30.x in #34229.

Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Working Diff
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")

fanquake added a commit that referenced this pull request Jan 9, 2026
04a996b doc: update manual pages for v30.2 (fanquake)
ed355b8 build: bump version to v30.2 (fanquake)
6c98d68 doc: update release notes for v30.2 (fanquake)
6d86b32 guix: Fix `osslsigncode` tests (Hennadii Stepanov)
1dae002 wallet: test: Failed migration cleanup (David Gumberg)
9e59047 test: migration, avoid backup name mismatch in default_wallet_failure (furszy)

Pull request description:

  Backports:
  * #34221
  * #34226
  * #34227

ACKs for top commit:
  hebasto:
    ACK 04a996b, I have reviewed the code and it looks OK.
  willcl-ark:
    ACK 04a996b
  marcofleon:
    ACK 04a996b

Tree-SHA512: 3389b9b629dcb920186b383353fd386bb757967d224e0267501b5e2083dc1e6cba051df6ef646de05c0e58fd43c9f549b9175eefb77fed1fe9ab7d1648b2d9e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants