test: fix race condition in encrypted wallet rescan tests #27199

ishaanam · 2023-03-04T22:30:54Z

DrahtBot · 2023-03-05T01:14:07Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake · 2023-03-05T09:43:26Z

Is this a draft because you haven't tested under Valgrind yet? Could add to the PR description what needs doing.

fanquake · 2023-03-05T21:47:40Z

Worked for me under a recent Valgrind run.

ishaanam · 2023-03-06T02:54:38Z

Is this a draft because you haven't tested under Valgrind yet?

Yes, it was.

Worked for me under a recent Valgrind run.

Great, I will mark it as ready for review.

maflcko · 2023-03-06T09:12:38Z

test/functional/wallet_transactiontime_rescan.py

Any reason to use the cli, when the normal rpc can be used directly?

Also, I wonder if this introduces another race where the actual rescan in t.start() is scheduled after this call. Avoiding this race might be ugly, but I think it could be possible by using assert_debug_log with a timeout value to sync on the rescan actually starting?

Any reason to use the cli, when the normal rpc can be used directly?

When I use the normal RPC directly, I get a CannotSendRequest error.

Also, I wonder if this introduces another race where the actual rescan in t.start() is scheduled after this call.

Yes, it does. I have used assert_debug_log with a timeout of 5 to fix this.

test/functional/wallet_importdescriptors.py

fanquake · 2023-03-11T09:56:57Z

Tested 308ea03d95786db674e8cce3e78a56b498cf119a rebased on master (c7f1d95) and I'm no-longer seeing the issue in #27229: wallet_importdescriptors.py --descriptors | ✓ Passed | 652 s

maflcko

Calling the cli constructor does not dispatch the rpc.

Otherwise this looks really nice.

I've tested this with https://github.com/bitcoin/bitcoin/pull/26347/commits/6a5b348f2e526f048d0b448b01f6c4ab608569af#r1133856319

test/functional/wallet_transactiontime_rescan.py

test/functional/wallet_importdescriptors.py

maflcko

looks nice, but there is a bug ACK 308ea03d95786db674e8cce3e78a56b498cf119a 🤠

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: looks nice, but there is a bug ACK 308ea03d95786db674e8cce3e78a56b498cf119a 🤠
dsqqD3bytDg6ZUqhxFkcRuKux2dfw85m+3T2vlhaQUQ3b4l8EkXyTYeSWzPpfL9zx9/SGYpqZiYJTY2x8tWODA==

maflcko · 2023-03-15T12:54:16Z

Didn't re-test. Please advise maintainers if you want this merged or work on coverage for walletpassphrase a bit more (#27199 (comment))

review ACK 78cd77e17719d56366c483d5508cad7b0c5f1b5c 🏟

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 78cd77e17719d56366c483d5508cad7b0c5f1b5c 🏟
ubuVZMdV4ZjmXyRCN11JWS3wS1zS8LnQWZZ+XwYwYFJYNWZjDxuYeDZs+qThasZ8axW4+4HoP8NXTP6XZDDECQ==

maflcko · 2023-03-16T09:52:59Z

nice re-ACK dbeca79 🚜

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice re-ACK dbeca792a9980085d00be0f9d78187ca3a4d7cdc  🚜
JFDIenhS3ChvBHnr3YgwQFTqW9fJwU4I3gVtjnBatedP3Ch3CrUVWjy/1q1ieBV/KSwz3iW9aEPDNbR0SaAGBA==

maflcko · 2023-03-16T10:15:04Z

Also, tested with my diff from #26347 (comment)

achow101 · 2023-03-16T21:19:43Z

ACK dbeca79

…can tests dbeca79 test: fix race condition in encrypted wallet rescan tests (ishaanam) Pull request description: This fixes bitcoin#26347 (comment) ACKs for top commit: MarcoFalke: nice re-ACK dbeca79 🚜 achow101: ACK dbeca79 Tree-SHA512: 7127254ac0274b5bc8ba0242736e77464acbf1f6e3f6af098b4e47742124c336cd67dffdb385e1e8dbd3a8ae74acd073c99e82fa35c44a615fd7d22b29a0daf7

mzumsande

saw two fails of wallet_transactiontime_rescan.py --legacy-wallet today, both on master:
https://cirrus-ci.com/task/6620294484328448
https://cirrus-ci.com/task/4914078427119616

fanquake · 2023-03-21T09:36:26Z

@mzumsande see also #27282.

stickies-v · 2023-03-23T14:09:34Z

test/functional/wallet_importdescriptors.py

+        self.nodes[0].cli("-rpcwallet=encrypted_wallet").walletpassphrase("passphrase", 1)
+
+        try:
+            self.nodes[0].cli("-rpcwallet=encrypted_wallet").walletlock()


It seems (?) we want this test to fail if walletlock succeeds, but that's not currently the case - is that on purpose? Apologies if I'm misunderstanding, I didn't dive super deep yet so feel free to keep your answer brief.

Edit: I suppose the main difficulty here is that walletlock can succeed if the rescan has already finished by the time walletlock is called, but then I think maybe the test needs a different setup?

I suppose the main difficulty here is that walletlock can succeed if the rescan has already finished by the time walletlock is called

Yes, this issue here is that the rescan could have finished. For more context see: #27199 (comment)

then I think maybe the test needs a different setup?

It would be great if there was a way that this test could behave the same regardless of speed. We haven't been able to find a way to make that happen yet.

ishaanam mentioned this pull request Mar 4, 2023

wallet: ensure the wallet is unlocked when needed for rescanning #26347

Merged

DrahtBot mentioned this pull request Mar 5, 2023

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

fanquake requested a review from maflcko March 5, 2023 09:42

ishaanam marked this pull request as ready for review March 6, 2023 02:54

maflcko reviewed Mar 6, 2023

View reviewed changes

DrahtBot added the Tests label Mar 6, 2023

ishaanam force-pushed the rescanblockchain_test_fix branch from a4278ad to 92777b3 Compare March 6, 2023 15:47

maflcko reviewed Mar 10, 2023

View reviewed changes

test/functional/wallet_importdescriptors.py Outdated Show resolved Hide resolved

maflcko mentioned this pull request Mar 10, 2023

Issue with wallet_importdescriptors.py --descriptors under valgrind #27229

Closed

ishaanam force-pushed the rescanblockchain_test_fix branch from 92777b3 to 308ea03 Compare March 10, 2023 16:41

maflcko suggested changes Mar 13, 2023

View reviewed changes

test/functional/wallet_transactiontime_rescan.py Outdated Show resolved Hide resolved

test/functional/wallet_importdescriptors.py Outdated Show resolved Hide resolved

maflcko reviewed Mar 13, 2023

View reviewed changes

ishaanam force-pushed the rescanblockchain_test_fix branch from 308ea03 to 78cd77e Compare March 15, 2023 03:36

test: fix race condition in encrypted wallet rescan tests

dbeca79

ishaanam force-pushed the rescanblockchain_test_fix branch from 78cd77e to dbeca79 Compare March 15, 2023 21:28

maflcko assigned fanquake and achow101 Mar 16, 2023

achow101 merged commit db03248 into bitcoin:master Mar 16, 2023

mzumsande reviewed Mar 20, 2023

View reviewed changes

maflcko mentioned this pull request Mar 21, 2023

test: wallet_importdescriptors.py --descriptors failure #27282

Closed

stickies-v reviewed Mar 23, 2023

View reviewed changes

ishaanam deleted the rescanblockchain_test_fix branch November 30, 2023 03:16

bitcoin locked and limited conversation to collaborators Nov 29, 2024

test: fix race condition in encrypted wallet rescan tests #27199

test: fix race condition in encrypted wallet rescan tests #27199

Conversation

ishaanam commented Mar 4, 2023

Uh oh!

DrahtBot commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented Mar 5, 2023

Uh oh!

fanquake commented Mar 5, 2023

Uh oh!

ishaanam commented Mar 6, 2023

Uh oh!

maflcko Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

ishaanam Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Mar 11, 2023

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 15, 2023

Uh oh!

maflcko commented Mar 16, 2023

Uh oh!

maflcko commented Mar 16, 2023

Uh oh!

achow101 commented Mar 16, 2023

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Mar 21, 2023

Uh oh!

stickies-v Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishaanam Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Mar 5, 2023 •

edited

Loading

maflcko left a comment •

edited

Loading

stickies-v Mar 23, 2023 •

edited

Loading

ishaanam Mar 23, 2023 •

edited

Loading