test: check `scanning` field from `getwalletinfo` #31768

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2025-01-test-wallet-scan changing 1 files +8 −0

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

ACK e7f5955

confirms the duration of rescanning to be greater than 0

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

test: check `scanning` field from `getwalletinfo`

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

bb0879ddab

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.

brunoerg force-pushed on Feb 4, 2025
maflcko commented at 3:17 PM on February 4, 2025: member

lgtm ACK bb0879ddabc8b3a7253bc774d23b842937d18015
DrahtBot requested review from Prabhat1308 on Feb 4, 2025
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).
BrandonOdiwuor commented at 4:23 PM on February 11, 2025: contributor

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

re-ACK bb0879d

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?

            scanning = wallet_info.get("scanning")
            if scanning:
                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

achow101 commented at 12:14 AM on February 14, 2025: member

ACK bb0879ddabc8b3a7253bc774d23b842937d18015
achow101 merged this on Feb 14, 2025
achow101 closed this on Feb 14, 2025
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.
```
progress = wallet_info["scanning"]["progress"]
assert_approx(progress,0,1)
```
brunoerg deleted the branch on Feb 17, 2025

Contributors

Labels

Linked (view graph)

#31893 test: remove scanning check on `wallet_importdescriptors`

test: check `scanning` field from `getwalletinfo` #31768

Code Coverage & Benchmarks

Reviews