wallet: test: Relative wallet failed migration cleanup #34226
Conversation
|
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/34226. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
|
Approach ACK |
Github-Pull: bitcoin#34226 Rebased-From: 6ca6883
Github-Pull: bitcoin#34226 Rebased-From: 6ca6883
Github-Pull: bitcoin#34226 Rebased-From: 6ca6883
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
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.
Github-Pull: bitcoin#34226 Rebased-From: 6ca6883
Github-Pull: bitcoin#34226 Rebased-From: 6ca6883
rkrux
left a comment
rkrux
left a comment
There was a problem hiding this comment.
Agree with the addition of these test cases and also with the suggestion of reducing duplication: #34226 (review)
test/functional/wallet_migration.py
Outdated
| mocked_time = int(time.time()) | ||
| self.master_node.setmocktime(mocked_time) | ||
| assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, wallet_name) | ||
| self.master_node.setmocktime(0) | ||
|
|
||
| # Verify the original wallet was not deleted | ||
| assert os.path.exists(wallet_path) | ||
| # And verify it is still a BDB wallet | ||
| self.assert_is_bdb(wallet_name) | ||
| # Verify the watchonly wallet was not deleted | ||
| assert os.path.exists(watch_only_dir / "wallet.dat") | ||
|
|
||
| # Check backup file exists. | ||
| backup_prefix = os.path.basename(os.path.abspath(wallet_path.parent)) | ||
| backup_path = os.path.join(self.master_node.wallets_path, f"{backup_prefix}_{mocked_time}.legacy.bak") | ||
| assert os.path.exists(backup_path) |
There was a problem hiding this comment.
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 (review)
We might be able to avoid setting mock time in the tests and checking for backup (& original) files in every test case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
davidgumberg
force-pushed
the
2026-01-07-relative-path-migration-failure
branch
2 times, most recently
from
a9657ab to
a4df0ea
Compare
|
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. |
|
Needs rebase |
davidgumberg
force-pushed
the
2026-01-07-relative-path-migration-failure
branch
from
a4df0ea to
f8c366d
Compare
Oops, fixed. |
davidgumberg
force-pushed
the
2026-01-07-relative-path-migration-failure
branch
from
f8c366d to
dc1dc67
Compare
|
Rebased to address @achow101's feedback. |
test/functional/wallet_migration.py
Outdated
| self.test_wallet_with_path("path/to/mywallet/") | ||
| self.test_wallet_with_path("path/that/ends/in/..") | ||
| self.test_default_wallet_failure() | ||
| self.test_migration_failure("") |
There was a problem hiding this comment.
| self.test_migration_failure("") | |
| self.test_migration_failure(wallet_name="") |
Same for the other two.
There was a problem hiding this comment.
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:
bitcoin/test/functional/wallet_migration.py
Lines 1682 to 1689 in ccf72b2
davidgumberg
force-pushed
the
2026-01-07-relative-path-migration-failure
branch
2 times, most recently
from
c6ca03e to
ccf72b2
Compare
|
Pushed to also add a test that checks the normal case, this revealed that the way I was finding 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. |
Github-Pull: bitcoin#34226 Rebased-From: 6ca6883
Github-Pull: bitcoin#34226 Rebased-From: 6ca6883
Refactor a common way to perform the failed migration test that exists for default wallets, and add relative-path wallets and absolute-path wallets.
davidgumberg
force-pushed
the
2026-01-07-relative-path-migration-failure
branch
from
ccf72b2 to
eeaf28d
Compare
|
Thanks for the changes, looking much better now. |
| backup_path.unlink() | ||
| Path(watch_only_dir / "wallet.dat").unlink() | ||
| Path(watch_only_dir).rmdir() | ||
| Path(master_path / "wallet.dat").unlink() | ||
| Path(old_path / "wallet.dat").unlink(missing_ok=True) |
There was a problem hiding this comment.
nano nit:
backup_path.unlink()
for file in [watch_only_dir, master_path, old_path]:
(file / "wallet.dat").unlink(missing_ok=True)
watch_only_dir.rmdir()
|
ACK eeaf28d |
There was a problem hiding this comment.
lgtm ACK eeaf28d
I also tested this with an absolute path wallet name outside the node dir like below:
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 45ebd435cd..07c14f2935 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -1679,11 +1679,14 @@ class WalletMigrationTest(BitcoinTestFramework):
self.test_wallet_with_path("path/to/mywallet/")
self.test_wallet_with_path("path/that/ends/in/..")
+ troublesomedirpath = f"{self.master_node.wallets_path}/../../../troublesomedir"
+ os.mkdir(troublesomedirpath)
migration_failure_cases = [
"",
"../",
os.path.abspath(self.master_node.datadir_path / "absolute_path"),
- "normallynamedwallet"
+ "normallynamedwallet",
+ os.path.abspath(troublesomedirpath)
]
for wallet_name in migration_failure_cases:
self.test_migration_failure(wallet_name=wallet_name)
|
|
||
| # Test cleanup: clear default wallet for next test | ||
| self.clear_default_wallet(backup_path) | ||
| self.assert_is_bdb(wallet_name) |
There was a problem hiding this comment.
bitcoin/test/functional/wallet_migration.py
Lines 70 to 75 in 6c3fb71
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:
assert_is_bdb!
self.master_node.wallets_path: /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_yfvb1rj7/node0/regtest/wallets
wallet_name: /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_yfvb1rj7/node0/absolute_path
self.master_node.wallets_path / wallet_name / self.wallet_data_filename: /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_yfvb1rj7/node0/absolute_path/wallet.dat
Within 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
If 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)):
>>> PurePath('/etc', '/usr', 'lib64')
PurePosixPath('/usr/lib64')
>>> PureWindowsPath('c:/Windows', 'd:bar')
PureWindowsPath('d:bar')
|
Backported to 30.x in #34229. |
Refactor a common way to perform the failed migration test that exists for default wallets, and add relative-path wallets and absolute-path wallets. Github-Pull: bitcoin#34226 Rebased-From: eeaf28d
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
76cdeb7 wallet: test: Failed migration cleanup (David Gumberg) 9405e91 test: coverage for migration failure when last sync is beyond prune height (furszy) 5e8ad98 wallet: migration, fix watch-only and solvables wallets names (furszy) a7e2d10 wallet: improve post-migration logging (furszy) 9ea84c0 test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy) 833848e test: add coverage for unnamed wallet migration failure (furszy) a074d36 wallet: fix unnamed wallet migration failure (furszy) d91f56e wallet: RestoreWallet failure, erase only what was created (furszy) cc324aa wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow) 01c04d3 wallet: introduce method to return all db created files (furszy) abaf1e3 refactor: remove sqlite dir path back-and-forth conversion (furszy) Pull request description: Backports: * #34215 * #34156 * #34226 * 2 required commits from #31423 Note that this backport is unclean and several changes have to be made to most commits to accommodate BDB and the differences in migration cleanup behavior. ACKs for top commit: furszy: Code review ACK 76cdeb7 brunoerg: light code review ACK 76cdeb7 + backported the functional tests without the fixes and all of them failed accordingly. glozow: light review ACK 76cdeb7. Tree-SHA512: 432268117783fc9a221d895a6f6601b6a2a5031c76d1443cf804cc1d486b40fcded982409d548acd1c01a13c7b378b840fcc3fbe823d6ba5ffc4ebe017d4e13c
7 participants
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.