test: check scanning field from getwalletinfo #31768

1. 
   brunoerg commented at 7:01 pm on January 30, 2025: contributor

   During a rescan, check that getwalletinfo returns properly information (the scanning field) about it.
2. 
   DrahtBot commented at 7:01 pm on January 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/31768.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, arejula27, BrandonOdiwuor, Prabhat1308, achow101

   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 Jan 30, 2025
4. brunoerg marked this as a draft on Jan 30, 2025
5. DrahtBot added the label CI failed on Jan 30, 2025
6. brunoerg force-pushed on Jan 30, 2025
7. brunoerg marked this as ready for review on Jan 30, 2025
8. DrahtBot removed the label CI failed on Jan 30, 2025
9. Prabhat1308 approved
10. 
    Prabhat1308 commented at 11:39 pm on February 1, 2025: none

    ACK e7f5955

    confirms the duration of rescanning to be greater than 0

11. test: check `scanning` field from `getwalletinfo`

    During a rescan, check that `getwalletinfo` returns
    properly information (the scanning field) about it.

    bb0879ddab
12. in test/functional/wallet_importdescriptors.py:709 in e7f5955ee0 outdated

    705@@ -705,6 +706,7 @@ def run_test(self):
    706             except JSONRPCException as e:
    707                 assert e.error["code"] == -4 and "Error: the wallet is currently being used to rescan the blockchain for related transactions. Please call `abortrescan` before changing the passphrase." in e.error["message"]
    708 
    709+            assert_greater_than(self.nodes[0].cli("-rpcwallet=encrypted_wallet").getwalletinfo()["scanning"]["duration"], 0)
    

    ---

    ---

    

    maflcko commented at 9:09 am on February 4, 2025:

    Won’t this race with importing and intermittently fail?

    ---

    

    brunoerg commented at 2:02 pm on February 4, 2025:

    You’re right, could happen. I just added a try/except around it, now it checks the duration is greater than 0 or if the field is not returned into getwalletinfo response.
13. brunoerg force-pushed on Feb 4, 2025
14. 
    maflcko commented at 3:17 pm on February 4, 2025: member

    lgtm ACK bb0879ddabc8b3a7253bc774d23b842937d18015
15. DrahtBot requested review from Prabhat1308 on Feb 4, 2025
16. 
    arejula27 commented at 7:09 pm on February 5, 2025: none

    ACK bb0879d

    The test looks good as it improves the verification of an RPC call field. However, the verified field produces different values depending on the scan situation:

    “scanning” : { (json object) current scanning details, or false if no scan is in progress>     “duration” : n, (numeric) elapsed seconds since scan start     “progress” : n (numeric) scanning progress percentage [0.0, 1.0] },

    I believe this PR would be more complete if it also verified the false case—for example, when importing the p2pkh descriptor for the first time (around line 108).

17. 
    BrandonOdiwuor commented at 4:23 pm on February 11, 2025: contributor

    Code Review ACK bb0879ddabc8b3a7253bc774d23b842937d18015
18. 
    Prabhat1308 commented at 7:55 pm on February 11, 2025: none

    re-ACK bb0879d
19. in test/functional/wallet_importdescriptors.py:714 in bb0879ddab

    705@@ -705,6 +706,13 @@ def run_test(self):
    706             except JSONRPCException as e:
    707                 assert e.error["code"] == -4 and "Error: the wallet is currently being used to rescan the blockchain for related transactions. Please call `abortrescan` before changing the passphrase." in e.error["message"]
    708 
    709+            wallet_info = self.nodes[0].cli("-rpcwallet=encrypted_wallet").getwalletinfo()
    710+            try:
    711+                duration = wallet_info["scanning"]["duration"]
    712+                assert_greater_than(duration, 0)
    713+            except Exception:
    714+                assert "scanning" not in wallet_info
    

    ---

    ---

    

    hodlinator commented at 1:16 pm on February 13, 2025:

    nit: Seems a bit heavy-handed to use exceptions, even if it’s following the pattern of code above?

    0            scanning = wallet_info.get("scanning")
    1            if scanning:
    2                assert_greater_than(scanning["duration"], 0)
    

    Could also break out encryped_cli = self.nodes[0].cli("-rpcwallet=encrypted_wallet") for the entire block now that we are using it for a 4th time, if you re-touch.

    ---

    

    brunoerg commented at 5:36 pm on February 13, 2025:

    Good suggestion, but I will leave as is for now to not invalidate the reviews. Thanks.

    ---

    

    maflcko commented at 7:20 am on February 17, 2025:

    https://github.com/bitcoin/bitcoin/issues/31881
20. 
    achow101 commented at 0:14 am on February 14, 2025: member

    ACK bb0879ddabc8b3a7253bc774d23b842937d18015
21. achow101 merged this on Feb 14, 2025
22. achow101 closed this on Feb 14, 2025

    ---

23. 
    janb84 commented at 11:03 am on February 14, 2025: none

    Post-merge ACK bb0879d

    Small improvement suggestion; also testing that the progress is between 0 and 1.

    0progress = wallet_info["scanning"]["progress"]
    1assert_approx(progress,0,1)
    

24. brunoerg deleted the branch on Feb 17, 2025

Contributors
brunoerg DrahtBot Prabhat1308 maflcko arejula27 BrandonOdiwuor hodlinator achow101 janb84

Labels
Tests