tests: cover abortrescan() in-flight True path with dynamic-tail retry #33131

1. 
   cedwies commented at 3:01 pm on August 3, 2025: none

   This PR adds a test in wallet_transactiontime_rescan.py. Previously we only tested that abortrescan() returns False when no rescan was running. This change verifies that it returns True and actually interrupts a running scan.

   How it works:

   1. spawn a daemon background thread that rescans a tail of the wallet’s chain.
   2. aggressively poll every 10 ms (for up to 5 s) until we see the node start scanning
   3. Call ‘abortrescan()’ and assert to True
   4. Threads are joined with short timeouts so they can’t hang teardown

   Dynamic tail: If the tail of the wallets chain is too small, by the time we see “scanning=True”, the scan might already be finished (so no scan is active, and abortrescan() will be ‘False’. A fixed sized tail caused issues in ~10% of test runs (the dynamic tail was tested 200 times without an error). By starting with 10 blocks and doubling for up to 5 times, we can ensure that a suitable tail length is found so that:

   1. The node sets scanning=true long enough for our tight (10 ms) polls to catch it
   2. But still finishes in a few seconds so the test stays fast.

   I’d love feedback if you have ideas for a simpler or more reliable way design this test, or if you spot any edge cases this might miss.

2. tests: cover abortrescan() in-flight True path with dynamic-tail retry

   Extend wallet_transactiontime_rescan.py to exercise the “scan in progress” branch of
   abortrescan(). Retry rescanning an initial 10-block tail—doubling on failure up to
   full chain—until getwalletinfo().scanning is observed, then call abortrescan()
   and assert it returns True. Threads are daemonized and joined with timeouts to avoid
   hangs, with a maximum of 5 attempts to prevent infinite loops. This change has been
   stress-tested 100× with no flakes.

   f516aa8e12
3. DrahtBot added the label Tests on Aug 3, 2025
4. 
   DrahtBot commented at 3:01 pm on August 3, 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/33131.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

5. DrahtBot added the label CI failed on Aug 3, 2025
6. in test/functional/wallet_transactiontime_rescan.py:168 in f516aa8e12 outdated

   163+        # RPC handle dedicated to polling getwalletinfo() and abortrescan()
   164+        poll_rpc = AuthServiceProxy(restorenode.url + '/wallet/wo')
   165+
   166+        # Get current block height and start with up to 10‐block tail.
   167+        height = restorewo_wallet.getblockcount()
   168+        tail = min(10, height)
   

   ---

   ---

   

   maflcko commented at 8:34 am on August 4, 2025:

   why not just use the maximum in the first try?

   ---

   

   cedwies commented at 11:30 am on August 4, 2025:

   When I first wrote the test, I had repeated issues on MacOS. the wallet’s scanning flag sometimes didn’t turn true quickly enough when I fired a full rescan. Too quickly, I convinced myself that the issue is with the tail length (I honestly never questioned it anymore until your question).

   Upon inspection: Yes, I will fix it (which simplifies the code a lot).

7. 
   cedwies commented at 11:33 am on August 4, 2025: none

   Also, I will block the test on t.join(). (no timeout), so the background rescan thread is guaranteed to exit before the test returns (which I think causes the CI cleanup step from failing with “OSError: directory not empty”).
8. simplified test with full rescan and blocking join

   Switched from dynamic tail, to directly using a full rescan.
   We now wait for the thread to be joined without a timeout.
   The test only proceeds when the threads were joined.

   5d61c6dab0
9. cedwies force-pushed on Aug 4, 2025
10. in test/functional/wallet_transactiontime_rescan.py:189 in 5d61c6dab0

    184+        # Ensure that the rescan started within 10 seconds
    185+        assert_greater_than(deadline, time.time())
    186+
    187+        # Abort and require True
    188+        result = poll_rpc.abortrescan()
    189+        assert_equal(result, True)
    

    ---

    ---

    

    maflcko commented at 7:38 am on August 5, 2025:

    This is racy and will intermittently fail. You can test this by adding a time.sleep(…) before the abort.

    ---

    

    cedwies commented at 9:16 am on August 5, 2025:

    Are you referring to the scan finishing between “scanning=True” and “abortrescan()”? I have thought about this, but I am not sure how to best solve the issue. If we saw “scanning=True” but “abortrescan()=False” we could assume a race and just try again (e.g. up to 5 times). But maybe this blows up the test suite?

    ---

    

    maflcko commented at 9:22 am on August 5, 2025:

    Are you referring to the scan finishing between “scanning=True” and “abortrescan()”?

    yes

    and just try again (e.g. up to 5 times).

    This will still fail, if it fails 5 times

    ---

    

    cedwies commented at 11:41 am on August 6, 2025:

    I currently don’t see a way around this. Any thoughts on how one could solve this problem?

    ---

    

    maflcko commented at 8:11 am on August 7, 2025:

    I am sure on Linux you could halt execution of a process (e.g. by adding a gdb breakpoint), but I am not sure if this is possible on Windows, or if this something worth trying.

    Could mark as draft while CI is red?

    ---

    

    cedwies commented at 9:01 pm on August 7, 2025:

    Thanks for the suggestion, I will look into this. Could work in theory, but my guess would be issues with portability, too. And maybe lack robustness. I will dig into this, and thank you for the review so far.
11. cedwies marked this as a draft on Aug 7, 2025

Contributors
cedwies DrahtBot maflcko

Labels
Tests CI failed