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

ishaanam · 2022-10-20T01:07:36Z

Wallet passphrases are needed to top up the keypool of encrypted wallets
during a rescan. The following RPCs need the passphrase when rescanning:
- importdescriptors
- rescanblockchain

The following RPCs use the information about whether or not the
passphrase is being used to ensure that full rescans are able to
take place (meaning the following RPCs should not be able to run
if a rescan requiring the wallet to be unlocked is taking place):
- walletlock
- encryptwallet
- walletpassphrasechange

m_relock_mutex is also introduced so that the passphrase is not
deleted from memory when the timeout provided in
walletpassphrase is up and the wallet is still rescanning.
Fixes #25702, #11249

Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions.

aureleoules

Changes look good, is there a reason it's in draft?
Also, is there a way to add a test for this?

ishaanam · 2022-10-21T13:29:17Z

Changes look good, is there a reason it's in draft?

Also, is there a way to add a test for this?

I made it a draft because I haven't written a test yet. I'll mark it as ready for review after I add some tests.

DrahtBot · 2022-10-23T22:57:50Z

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	achow101, hernanmarino, furszy
Concept ACK	pablomartin4btc, theStack

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:

#27068 (wallet: SecureString to allow null characters by john-moffett)
#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.

aureleoules · 2022-10-24T09:31:04Z

The test seems to create intermittent failures. I also cherry-picked the test on master and the results seem to be the same.
seq 100 | xargs -i -P 5 python test/functional/wallet_importdescriptors.py --descriptors will execute the test in parallel batches of 5.

ishaanam · 2022-10-24T18:34:49Z

The test seems to create intermittent failures. I also cherry-picked the test on master and the results seem to be the same. seq 100 | xargs -i -P 5 python test/functional/wallet_importdescriptors.py --descriptors will execute the test in parallel batches of 5.

Thanks for taking a look at this, that should be fixed now. There is still a problem of the passphrase timing out on certain systems before importdescriptors is even run, but I'm not sure how to fix that.

achow101

This could have a similar test for rescanblockchain as well.

src/wallet/rpc/encrypt.cpp

src/wallet/rpc/transactions.cpp

test/functional/wallet_importdescriptors.py

pablomartin4btc

Concept ACK, I see the point from @achow101 moving the wallet lock/ mutex to CWallet::Lock(), it looks like a better place but I'd need more knowledge to give a proper advice.

ishaanam · 2022-10-27T02:50:48Z

Thanks for the review @achow101, I have implemented your suggestions. I also made some slight modifications to the error messages in 8c16fdb.

This could have a similar test for rescanblockchain as well.

Yes, I will add one shortly.

w0xlt · 2022-10-30T19:55:26Z

src/wallet/wallet.h

    // Used to prevent concurrent calls to walletpassphrase RPC.
    Mutex m_unlock_mutex;
+    // Used to prevent deleting the passphrase from memory when it is still in use.
+    RecursiveMutex m_relock_mutex;


Using Mutex is preferable to RecursiveMutex.

Mutex isn't used here because sometimes m_relock_mutex has to be locked multiple time within a scope to prevent deadlocks. See: #26347 (comment)

The new mutex is now grabbed in CWallet::Lock and also in locations where cs_wallet would have been grabbed first in order to prevent a deadlock.

ishaanam · 2022-10-31T00:09:34Z

Added a test for rescanblockchain

theStack · 2023-01-03T13:11:59Z

Concept ACK

src/wallet/rpc/transactions.cpp

pablomartin4btc

re-ack b2e2829. I'll check again once you make the changes you commented to @furszy.

hernanmarino

Re ACK b2e2829

src/wallet/wallet.cpp

pablomartin4btc

re-ack fcaaa5fd905a21298bd69343c9756e32a70870e6 - fix lock ordering deadlock.

ishaanam · 2023-02-15T13:55:31Z

Rebased, I also increased the walletpassphrase timeout in both of the tests.

achow101 · 2023-02-15T18:10:32Z

ACK 6a5b348

hernanmarino

ACK 6a5b348

furszy

Tested ACK 6a5b348

No longer crashes due the deadlock.

…d for rescanning 6a5b348 test: test rescanning encrypted wallets (ishaanam) 493b813 wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam) 66a86eb wallet: keep track of when the passphrase is needed when rescanning (ishaanam) Pull request description: Wallet passphrases are needed to top up the keypool of encrypted wallets during a rescan. The following RPCs need the passphrase when rescanning: - `importdescriptors` - `rescanblockchain` The following RPCs use the information about whether or not the passphrase is being used to ensure that full rescans are able to take place (meaning the following RPCs should not be able to run if a rescan requiring the wallet to be unlocked is taking place): - `walletlock` - `encryptwallet` - `walletpassphrasechange` `m_relock_mutex` is also introduced so that the passphrase is not deleted from memory when the timeout provided in `walletpassphrase` is up and the wallet is still rescanning. Fixes bitcoin#25702, bitcoin#11249 Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions. ACKs for top commit: achow101: ACK 6a5b348 hernanmarino: ACK 6a5b348 furszy: Tested ACK 6a5b348 Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5

maflcko · 2023-03-02T16:03:29Z

test/functional/wallet_transactiontime_rescan.py

+            minernode.createwallet("encrypted_wallet", blank=True, passphrase="passphrase", descriptors=False)
+            encrypted_wallet = minernode.get_wallet_rpc("encrypted_wallet")
+
+            encrypted_wallet.walletpassphrase("passphrase", 1)


This fails for me when running in valgrind:

node0 2023-02-28T19:50:00.090808Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/request.cpp:179] [parse] [rpc] ThreadRPCServer method=walletpassphrase user=__cookie__ node0 2023-02-28T19:50:08.966047Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/server.cpp:560] [RPCRunLater] [rpc] queue run of timer lockwallet(encrypted_wallet) in 1 seconds (using HTTP) node0 2023-02-28T19:50:08.995457Z (mocktime: 2023-03-30T19:35:09Z) [http] [httpserver.cpp:239] [http_request_cb] [http] Received a POST request for /wallet/encrypted_wallet from 127.0.0.1:42774 node0 2023-02-28T19:50:08.998881Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.2] [rpc/request.cpp:179] [parse] [rpc] ThreadRPCServer method=sethdseed user=__cookie__ node0 2023-02-28T19:51:12.893784Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.2] [wallet/scriptpubkeyman.h:251] [WalletLogPrintf] [encrypted_wallet] keypool added 800 keys (400 internal), size=800 (400 internal) node0 2023-02-28T19:51:12.953236Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.2] [wallet/scriptpubkeyman.h:251] [WalletLogPrintf] [encrypted_wallet] LegacyScriptPubKeyMan::NewKeyPool rewrote keypool node0 2023-02-28T19:51:12.964195Z (mocktime: 2023-03-30T19:35:09Z) [http] [httpserver.cpp:239] [http_request_cb] [http] Received a POST request for /wallet/encrypted_wallet from 127.0.0.1:53062 node0 2023-02-28T19:51:12.966738Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.3] [rpc/request.cpp:179] [parse] [rpc] ThreadRPCServer method=sethdseed user=__cookie__ test 2023-02-28T19:51:12.976000Z TestFramework (ERROR): JSONRPC error Traceback (most recent call last): File "/root/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 134, in main self.run_test() File "/root/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_transactiontime_rescan.py", line 200, in run_test encrypted_wallet.sethdseed(seed=hd_seed) File "/root/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/root/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 149, in __call__ raise JSONRPCException(response['error'], status) test_framework.authproxy.JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first. (-13)

Maybe the timeout can be increased to 999999?

Also I wonder why you picked a timeout of 3 below, or why walletpassphrase needs to be called at all twice in a row?

If you want to confirm that the wallet can't be relocked while the rescan is in progress, wouldn't it be better to call walletlock rpc right after the rescan rpc?

Maybe the timeout can be increased to 999999?

Yes, the timeout before sethdseed can be increased to 999999, followed by running walletlock. I can open a follow-up PR for this.

Also I wonder why you picked a timeout of 3 below, or why walletpassphrase needs to be called at all twice in a row?

3 was used because it needed to be a number that was large enough so that the scan would begin, but small enough so that the timeout would occur while the rescan is in progress. However, this number can be too large or too small depending on the speed of the machine.

If you want to confirm that the wallet can't be relocked while the rescan is in progress, wouldn't it be better to call walletlock rpc right after the rescan rpc?

I don't completely understand this suggestion. Do you mean calling walletlock to ensure that the wallet is still unlocked? If so, I think there are two problems with that:

walletlock doesn't throw an error if the wallet is already locked

The wallet is supposed to re-lock immediately after the rescan, so the wallet might already be locked by the time walletlock is called, even if it did not lock during the rescan. However, using a batched RPC request might help with this.

Do you mean calling walletlock to ensure that the wallet is still unlocked?

No, I mean calling walletlock to lock the wallet. So the execution would look like:

-> walletpassphrase 99999 (start) <- walletpassphrase (end) -> rescanblockchain (start) -> walletlock (start) <- rescanblockchain (finish, release mutex) <- walletlock (take mutex, end)

Without the fix in this pull, walletlock might lock immediately and then likely cause rescanblockchain to fail?

Ok, I think I understand what you mean now. I have implemented this using multi-threading in #27199. However, for me this no longer fails without the fix.

However, for me this no longer fails without the fix.

For me, nothing fails. Unless I specifically add a manual sleep on the code before the fix:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index eed40f9462..d5b5af0d4c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1851,6 +1851,8 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc WalletLogPrintf("Rescan started from block %s... (%s)\n", start_block.ToString(), fast_rescan_filter ? "fast variant using block filters" : "slow variant inspecting all blocks"); + UninterruptibleSleep(4s); + fAbortRescan = false; ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), 0); // show rescan progress in GUI as dialog or on splashscreen, if rescan required on startup (e.g. due to corruption) uint256 tip_hash = WITH_LOCK(cs_wallet, return GetLastBlockHash());

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

…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

fanquake added the Wallet label Oct 20, 2022

aureleoules reviewed Oct 21, 2022

View reviewed changes

ishaanam force-pushed the rescanblockchain_unlock_wallet branch 2 times, most recently from 679b803 to c0a4c88 Compare October 23, 2022 20:53

ishaanam marked this pull request as ready for review October 23, 2022 20:53

DrahtBot mentioned this pull request Oct 24, 2022

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

Merged

ishaanam force-pushed the rescanblockchain_unlock_wallet branch from c0a4c88 to 25b8736 Compare October 24, 2022 17:50

achow101 reviewed Oct 24, 2022

View reviewed changes

src/wallet/rpc/encrypt.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/transactions.cpp Show resolved Hide resolved

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

pablomartin4btc reviewed Oct 25, 2022

View reviewed changes

ishaanam force-pushed the rescanblockchain_unlock_wallet branch 2 times, most recently from 13a7c62 to 4741e68 Compare October 27, 2022 02:45

DrahtBot mentioned this pull request Oct 27, 2022

RPC/Wallet: Access wallets via interfaces::Wallet #26082

Closed

ishaanam force-pushed the rescanblockchain_unlock_wallet branch 2 times, most recently from 83bd450 to fdc794d Compare October 30, 2022 18:05

w0xlt reviewed Oct 30, 2022

View reviewed changes

ishaanam force-pushed the rescanblockchain_unlock_wallet branch 2 times, most recently from f9b41b9 to b2e2829 Compare October 30, 2022 23:16

furszy reviewed Jan 20, 2023

View reviewed changes

src/wallet/rpc/transactions.cpp Show resolved Hide resolved

pablomartin4btc reviewed Jan 25, 2023

View reviewed changes

hernanmarino reviewed Jan 25, 2023

View reviewed changes

furszy reviewed Feb 4, 2023

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

ishaanam force-pushed the rescanblockchain_unlock_wallet branch from b2e2829 to fcaaa5f Compare February 6, 2023 15:06

pablomartin4btc reviewed Feb 6, 2023

View reviewed changes

test: test rescanning encrypted wallets

6a5b348

ishaanam force-pushed the rescanblockchain_unlock_wallet branch from dcda4fb to 6a5b348 Compare February 15, 2023 04:53

DrahtBot removed the Needs rebase label Feb 15, 2023

DrahtBot requested a review from hernanmarino February 15, 2023 18:10

hernanmarino reviewed Feb 16, 2023

View reviewed changes

furszy approved these changes Feb 21, 2023

View reviewed changes

DrahtBot requested a review from furszy February 21, 2023 15:26

achow101 merged commit 80f4979 into bitcoin:master Feb 21, 2023

maflcko reviewed Mar 2, 2023

View reviewed changes

ishaanam mentioned this pull request Mar 4, 2023

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

Merged

Ronnie6464 approved these changes May 28, 2023

View reviewed changes

This comment was marked as spam.

Sign in to view

bitcoin deleted a comment from Ronnie6464 May 28, 2023

bitcoin locked as spam and limited conversation to collaborators May 28, 2023

bitcoin unlocked this conversation May 28, 2023

bitcoin locked as resolved and limited conversation to collaborators May 28, 2023

bitcoin deleted a comment from Ronnie6464 May 28, 2023

ishaanam deleted the rescanblockchain_unlock_wallet branch November 30, 2023 03:17

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

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

Conversation

ishaanam commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Oct 21, 2022

Uh oh!

DrahtBot commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

aureleoules commented Oct 24, 2022

Uh oh!

ishaanam commented Oct 24, 2022

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Oct 27, 2022

Uh oh!

w0xlt Oct 30, 2022

Choose a reason for hiding this comment

Uh oh!

ishaanam Oct 30, 2022

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Oct 31, 2022

Uh oh!

theStack commented Jan 3, 2023

Uh oh!

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Feb 15, 2023

Uh oh!

achow101 commented Feb 15, 2023

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

furszy 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!

maflcko Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

ishaanam Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

ishaanam Mar 4, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

ishaanam commented Oct 20, 2022 •

edited

Loading

DrahtBot commented Oct 23, 2022 •

edited

Loading

furszy left a comment •

edited

Loading