test: check backup from `migratewallet` can be successfully restored #28257

1. 
   brunoerg commented at 12:46 PM on August 11, 2023: contributor

   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.

2. 
   DrahtBot commented at 12:46 PM on August 11, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, MarcoFalke
   Concept ACK	furszy

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Aug 11, 2023
4. 
   furszy commented at 1:53 PM on August 11, 2023: member

   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.

5. brunoerg force-pushed on Aug 11, 2023
6. brunoerg force-pushed on Aug 11, 2023
7. test: check backup from `migratewallet` can be successfully restored 769f5b15f2
8. brunoerg force-pushed on Aug 11, 2023
9. 
   brunoerg commented at 7:48 PM on August 11, 2023: contributor

   Thanks, @furszy. Initially, I was thinking about identifying all backup files into the node's directory, run migratewallet with them and check the result. However, I agree it's better to go beyond by checking wallet's balance and other stuff prior and post migration.

   Force-pushed changing the approach. I also moved it to test_basic().

10. 
    achow101 commented at 7:31 PM on August 15, 2023: member

    ACK 769f5b15f207ce6d1067672ea5e195541c97de6b

11. fanquake requested review from furszy on Aug 16, 2023
12. 
    maflcko commented at 10:30 AM on August 16, 2023: member

    lgtm ACK 769f5b15f207ce6d1067672ea5e195541c97de6b

13. fanquake merged this on Aug 16, 2023
14. fanquake closed this on Aug 16, 2023

    ---

15. 
    jonatack commented at 8:27 PM on August 16, 2023: member

    Post-merge ACK.

    For the getwalletinfo['balance'] check, RPC getbalances['mine']['trusted'] should work as well.

16. sidhujag referenced this in commit 11f6aa775b on Aug 17, 2023
17. bitcoin locked this on Dec 5, 2024

Review Requested
furszy