test: check backup from migratewallet can be successfully restored#28257
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
While I'm a concept ACK, I think that the approach isn't the best.
The code is walking through the node's directory, restoring all the backup files without checking if the restored wallet is correct or not.
The main point behind a backup test should be to assert that the restored wallet has the same balance and it watches the same scripts as before. Not only check that the backup file can be opened (the wallet could be empty or missing some information).
Also, test cases should be independent from each other.
Would suggest to test this inside the main test_basic() case. Using the migratewallet() return information to access the backup path (the command retrieves the backup path). And check that balance and watched scripts are the same prior and post migration.
brunoerg
force-pushed
the
2023-08-test-wallet-migration-restore
branch
2 times, most recently
from
e96b55c to
8182ea1
Compare
brunoerg
force-pushed
the
2023-08-test-wallet-migration-restore
branch
from
8182ea1 to
769f5b1
Compare
|
Thanks, @furszy. Initially, I was thinking about identifying all backup files into the node's directory, run Force-pushed changing the approach. I also moved it to |
|
ACK 769f5b1 |
|
lgtm ACK 769f5b1 |
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Post-merge ACK.
For the getwalletinfo['balance'] check, RPC getbalances['mine']['trusted'] should work as well.
…uccessfully restored 769f5b1 test: check backup from `migratewallet` can be successfully restored (brunoerg) Pull request description: `migratewallet` migrates the wallet to a descriptor one. During the process, it generates a backup file of the wallet in case of an incorrect migration. This PR adds test to check if the backup file can be successfully restored. ACKs for top commit: achow101: ACK 769f5b1 MarcoFalke: lgtm ACK 769f5b1 Tree-SHA512: 94c50b34fbd47c4d3cc34b94e9e7903bc233608c7f50f45c161669996fd5f5b7d8f9a4e6a3437b9151d66a76af833f3f1ca28e44ecb63b5a8f391f6d6be0e39f
7 participants
migratewalletmigrates the wallet to a descriptor one. During the process, it generates a backup file of the wallet in case of an incorrect migration. This PR adds test to check if the backup file can be successfully restored.