test: Perform backup filename checks in migrate_and_get_rpc in wallet_migration.py #33104

1. 
   achow101 commented at 5:17 pm on July 30, 2025: member

   Some test cases were unnecessarily checking the backup filename, which involved setting the mocktime before migrate_and_get_rpc. However, this could cause a failure if the test was slow since migrate_and_get_rpc also sets the mocktime. Since it also already checks that the backup file is named correctly, there’s no need for those tests to also do their own mocktime and filename check.

   The CI failure can be reproduced locally by adding a sleep to migrate_and_get_rpc:

    0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
    1index 704204425c7..e87a6100623 100755
    2--- a/test/functional/wallet_migration.py
    3+++ b/test/functional/wallet_migration.py
    4@@ -129,6 +129,7 @@ class WalletMigrationTest(BitcoinTestFramework):
    5                 assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
    6 
    7         # Mock time so that we can check the backup filename.
    8+        time.sleep(1)
    9         mocked_time = int(time.time())
   10         self.master_node.setmocktime(mocked_time)
   11         # Migrate, checking that rescan does not occur
   

   Fixes #33096

2. DrahtBot added the label Tests on Jul 30, 2025
3. 
   DrahtBot commented at 5:17 pm on July 30, 2025: contributor

   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/33104.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	pablomartin4btc, fjahr, Sammie05, rkrux
   Stale ACK	enirox001

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

4. in test/functional/wallet_migration.py:598 in 95c11728f4 outdated

   595@@ -595,10 +596,7 @@ def test_wallet_with_relative_path(self):
   596 
   597         # migratewallet uses current time in naming the backup file, set a mock time
   598         # to check that this works correctly.
   

   ---

   ---

   

   rkrux commented at 8:37 am on July 31, 2025:

   This comment should be removed as migrate_and_get_rpc does this internally now.

   ---

   

   fjahr commented at 2:53 pm on July 31, 2025:

   Could also move this one into migrate_and_get_rpc and replace the one there since this one is a bit more detailed.

   ---

   

   achow101 commented at 5:46 pm on July 31, 2025:

   Moved to migrate_and_get_rpc.
5. in test/functional/wallet_migration.py:681 in 95c11728f4 outdated

   680@@ -685,8 +681,6 @@ def test_default_wallet(self):
   681         assert_equal(str(backup_path), res['backup_path'])
   

   ---

   ---

   

   rkrux commented at 8:43 am on July 31, 2025:

   Can consider the following removal because migrate_and_get_rpc seems to be checking for these more or less.

   0- backup_path = self.master_node.wallets_path / backup_filename
   1- assert backup_path.exists()
   2- assert_equal(str(backup_path), res['backup_path'])
   

   https://github.com/bitcoin/bitcoin/blob/8283af13fe869defce1a93fc0935d0b0244edd45/test/functional/wallet_migration.py#L144-L152

   ---

   

   achow101 commented at 5:47 pm on July 31, 2025:

   That’s a good point. Moving the listwalletdir check into migrate_and_get_rpc also lets us drop the part where this test needs to pass in the mocktime too.
6. rkrux approved
7. 
   rkrux commented at 8:51 am on July 31, 2025: contributor

   tACK 95c11728f423e1c655439f9248a314f083ef68ef

   I did notice the error mentioned in #33096 for the first time yesterday and it didn’t appear again on rerunning.

   The fix in itself seems fine to me. Prior to this PR, the test_default_wallet subtest was setting mock time twice (one directly and another indirectly via the migrate_and_get_rpc function), which was confusing too. This fix streamlines the flow in addition to the fix.

   Suggested couple changes but can be done later as the issue could be time sensitive because of multiple CI failures.

8. pablomartin4btc approved
9. 
   pablomartin4btc commented at 3:29 pm on July 31, 2025: member

   ACK 95c11728f423e1c655439f9248a314f083ef68ef

   Since now migrate_and_get_rpc allows to the mocked_time to be passed, as it was already validating the timestamp in the backup filename within the function, wouldn’t make sense anymore to do the validation again outside the function.

10. 
    enirox001 commented at 4:31 pm on July 31, 2025: contributor

    Confirmed this fixes the timing race condition. The test was failing due to different timestamps being captured in the test vs inside migrate_and_get_rpc(), causing backup filename mismatches in slow environments.

    Tested locally - reproduces the failure without the fix and passes with it. Good solution that maintains backward compatibility while centralizing mocktime management.

11. DrahtBot requested review from enirox001 on Jul 31, 2025
12. 
    enirox001 commented at 4:37 pm on July 31, 2025: contributor

    ACK 95c1172
13. 
    fjahr commented at 5:09 pm on July 31, 2025: contributor

    tACK 95c11728f423e1c655439f9248a314f083ef68ef
14. test: Perform backup filename checks in migrate_and_get_rpc

    Some test cases were unnecessarily checking the backup filename, which
    involved setting the mocktime before `migrate_and_get_rpc`. However,
    this could cause a failure if the test was slow since
    `migrate_and_get_rpc` also sets the mocktime. Since it also already
    checks that the backup file is named correctly, there's no need for
    those tests to also do their own mocktime and filename check.

    4b80147feb
15. achow101 force-pushed on Jul 31, 2025
16. achow101 renamed this:
    test: Use the same mocktime when migrating in wallet_migration.py
    test: Perform backup filename checks in migrate_and_get_rpc in wallet_migration.py
    on Jul 31, 2025
17. pablomartin4btc approved
18. 
    pablomartin4btc commented at 8:20 pm on July 31, 2025: member

    utACK 4b80147feb97300e92e1f940b8d989a0af331e06
19. DrahtBot requested review from rkrux on Jul 31, 2025
20. DrahtBot requested review from enirox001 on Jul 31, 2025
21. DrahtBot requested review from fjahr on Jul 31, 2025
22. 
    fjahr commented at 9:17 pm on July 31, 2025: contributor

    reACK 4b80147feb97300e92e1f940b8d989a0af331e06
23. 
    Sammie05 commented at 2:23 am on August 1, 2025: none

    tACK 4b80147 Confirmed the test passes and mocktime is handled consistently within migrate_and_get_rpc.

    Reproduced the failure when mocktime is set redundantly and confirmed the fix removes the race. Clean change, and no unnecessary test logic outside the migration function. Good to go from my end.

24. rkrux approved
25. 
    rkrux commented at 8:31 am on August 1, 2025: contributor

    ACK 4b80147feb97300e92e1f940b8d989a0af331e06

    Thanks for incorporating suggestions.

26. fanquake merged this on Aug 1, 2025
27. fanquake closed this on Aug 1, 2025

    ---

Contributors
achow101 DrahtBot rkrux fjahr pablomartin4btc enirox001 Sammie05

Review Requested
fjahr enirox001

Labels
Tests