-
Notifications
You must be signed in to change notification settings - Fork 38.7k
test: migration, avoid backup name mismatch in default_wallet_failure #34221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34221. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
|
ACK cbf0bd3 |
|
ACK cbf0bd3 |
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
|
Backported to 30.x in #34229. |
rkrux
left a comment
There was a problem hiding this 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")
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
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_failuremigration test calls the functionmigrate_and_get_rpc(), which sets the mock time internally. But, at thesame 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()isunnecessary here. I'm surprised the CI hasn't complained about this yet.