rpc: Prevent unloading a wallet when rescanning #26618

aureleoules · 2022-12-01T16:09:13Z

This PR prevents a user from unloading a wallet if it is currently rescanning.

To test:

./src/bitcoin-cli -testnet -named createwallet wallet_name=wo disable_private_keys=true
./src/bitcoin-cli -testnet -rpcwallet=wo importdescriptors '[{
	  "desc": "addr(mmcuW74MyJUZuLnWXGQLoNXPrS9RbFz6gD)#tpnrahgc",
      "timestamp": 0,
      "active": false,
      "internal": false,
      "next": 0
}]'
./src/bitcoin-cli -testnet unloadwallet wo
error code: -4
error message:
Wallet is currently rescanning. Abort existing rescan or wait.

DrahtBot · 2022-12-01T16:09:16Z

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	w0xlt, kouloumos, promag, achow101
Concept ACK	luke-jr

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

stickies-v

Current behaviour seems to be that unloadwallet waits until the scan is finished. Changing that by throwing could affect downstream users that are expecting unloadwallet to just wait. I think the behaviour in this PR is better if it were implemented like that from the start. I'm not sure if it's worth changing though? ~0 on this for now.

ishaanam

Current behaviour seems to be that unloadwallet waits until the scan is finished.

This is what I have observed as well. Even so, I think that it would be better if instead of waiting for the rescan to finish (which could take hours), CWallet::AbortRescan is called from unloadwallet. This seems better because unloadwallet should ideally be able to unload the wallet relatively quickly regardless of the processes that the wallet is running. A warning could be given when a rescan has been aborted by unloadwallet so that a user is not surprised.

aureleoules · 2022-12-03T10:28:27Z

Changing that by throwing could affect downstream users that are expecting unloadwallet to just wait.

Do you know any software that would use this behavior? I understand this could affect users but this seems a bit hacky.

A warning could be given when a rescan has been aborted by unloadwallet so that a user is not surprised.

I don't really have an opinion on this. But an argument for keeping the warning in 8198c76a3008094fefbbae1b11e5d5673a993158 is that an user may not realize that the wallet is still rescanning and wouldn't want to abort it when trying to unload.
As a compromise we could add a 'force' argument to unloadwallet to abort the rescan if there is one.

w0xlt · 2022-12-03T20:06:45Z

As a compromise we could add a 'force' argument to unloadwallet to abort the rescan if there is one.

I think this is a good approach.

luke-jr · 2022-12-07T01:34:34Z

Agree with a force option (named only / options Object).

But also, we probably shouldn't return from unloadwallet until the wallet is actually unloaded fully, independently of whether that's due to a rescan or otherwise.

I guess this PR is strictly better than the current behaviour, though, so Concept ACK.

aureleoules · 2022-12-07T14:09:53Z

I added the named force option that will abort the rescan in progress.
https://github.com/bitcoin/bitcoin/compare/8198c76a3008094fefbbae1b11e5d5673a993158..2567ffc5f1850af7b97c3faf166c187dff65cd4c

promag

Tiny concept NACK.

I think unloadwallet should just fail if a scan is in progress. We already have that behavior/warning elsewhere: Wallet is currently rescanning. Abort existing rescan or wait. I understand it can be seen as a breaking change, but makes more sense than what happened in #26463.

Regardless of the above, the current code is still not ideal, for instance, a scan can be initiated after IsScanning check if importdescriptors is called concurrently. I think you should use WalletRescanReserver because it allows checking if a rescan is in progress and at the same time prevent concurrent scans.

A test case would be nice.

src/wallet/rpc/wallet.cpp

ziggie1984 · 2022-12-08T00:01:30Z

Thanks for fixing this issue. I like the approach more where we just fail the unloadwallet call but the force flag is also a good way.

a scan can be initiated after IsScanning check if importdescriptors is called concurrently.

Definitly important to prevent bitcoind from crashing. I was able to bring bitcoind down when unloading the wallet (blocking in the background) deleting the walletdir and then importing the descriptors with the same wallet name again (an then there was a dependence that one sync finished before the other). So would be important if we would not allow loading a wallet which is currently in rescan (at least not with the same name).

src/wallet/rpc/wallet.cpp

aureleoules · 2022-12-08T15:16:02Z

I have decided to remove to force option and only leave a warning in case there is a rescan. To me it's simpler and faster to type abortrescan than options={"force": true} and the warning is consistent with other RPCs.

I think you should use WalletRescanReserver because it allows checking if a rescan is in progress and at the same time prevent concurrent scans.

Done, thanks.

promag

Concept ACK, unloadwallet fails if a rescan is in progress. Needs:

release notes
test, not sure how
follow-up in the GUI

src/wallet/rpc/wallet.cpp

aureleoules · 2022-12-08T15:47:06Z

Thanks, @promag. I added release notes.

I think the test should be a follow-up as there are many other instances where this warning is not tested.

promag · 2022-12-08T16:08:39Z

I think the test should be a follow-up as there are many other instances where this warning is not tested.

The general rule of thumb is that tests are added when there are changes.

aureleoules · 2022-12-08T16:34:21Z

The general rule of thumb is that tests are added when there are changes.

I agree but the RPC error Wallet is currently rescanning. Abort existing rescan or wait has not been tested in other RPCs either and if a test were to be added, the other RPCs should be tested too and that may be out of scope.

grep -nrIi "Wallet is currently rescanning. Abort existing rescan or wait" ./src
src/wallet/rpc/backup.cpp:159:            throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
src/wallet/rpc/backup.cpp:254:        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
src/wallet/rpc/backup.cpp:447:        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
src/wallet/rpc/backup.cpp:516:        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
src/wallet/rpc/backup.cpp:1351:        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
src/wallet/rpc/backup.cpp:1658:        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
src/wallet/rpc/transactions.cpp:872:        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
src/wallet/rpc/wallet.cpp:444:            throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");

grep -nrIi "Wallet is currently rescanning. Abort existing rescan or wait" ./test
<empty>

I'll look into adding a test though.

promag · 2022-12-08T16:36:20Z

My suggestion is to test the just introduced error, the other are indeed out of scope.

ziggie1984 · 2022-12-08T23:02:07Z

Is it conceptionally possible to abort the rescan, if the wallet_dir is deleted (manually with rm -f wallet_dir)? My tests showed me that bitcoind will happily rescan further ? Or is it somehow a user-misbehaving we should not take into account ?

promag · 2022-12-08T23:07:31Z

@ziggie1984 user-misbehaving

luke-jr · 2022-12-09T03:53:07Z

Yeah, I would think deleting data files out from under the software should be expected to crash ;)

aureleoules · 2022-12-14T19:43:21Z

I added a test in #26700 @promag.

w0xlt

ACK 109cbb8

kouloumos

ACK 109cbb8
The implementation is consistent with how the WalletRescanReserver is used in the rest of the related RPCs as pointed out in #26618 (review) .
(check the rest of the RPCs with git grep -n "Wallet is currently rescanning. Abort existing rescan or wait")

I would also prefer to see a test as part of this PR, but looking at the linked draft PR implementation it seems that the relative complexity to include a test (other test cases missing, refactor of an existing test file) justifies a follow-up.

promag

ACK 109cbb8

achow101 · 2023-01-09T21:18:26Z

ACK 109cbb8

…canning 109cbb8 doc: Add release notes for #26618 (Aurèle Oulès) b13902d rpc: Prevent unloading a wallet when rescanning (Aurèle Oulès) Pull request description: Fixes #26463. This PR prevents a user from unloading a wallet if it is currently rescanning. To test: ```bash ./src/bitcoin-cli -testnet -named createwallet wallet_name=wo disable_private_keys=true ./src/bitcoin-cli -testnet -rpcwallet=wo importdescriptors '[{ "desc": "addr(mmcuW74MyJUZuLnWXGQLoNXPrS9RbFz6gD)#tpnrahgc", "timestamp": 0, "active": false, "internal": false, "next": 0 }]' ./src/bitcoin-cli -testnet unloadwallet wo error code: -4 error message: Wallet is currently rescanning. Abort existing rescan or wait. ACKs for top commit: achow101: ACK 109cbb8 w0xlt: ACK bitcoin/bitcoin@109cbb8 kouloumos: ACK 109cbb8 promag: ACK 109cbb8 Tree-SHA512: 15fdddf4cf9f3fa08f52069fe4a25a76e04a55bb2586b031bfb0393dce7f175dcdb52823e132a7dff6a894539beeb980a1aad2a792790be036be6977628149b2

achow101 · 2023-01-09T22:02:41Z

This was merged

109cbb8 doc: Add release notes for bitcoin#26618 (Aurèle Oulès) b13902d rpc: Prevent unloading a wallet when rescanning (Aurèle Oulès) Pull request description: Fixes bitcoin#26463. This PR prevents a user from unloading a wallet if it is currently rescanning. To test: ```bash ./src/bitcoin-cli -testnet -named createwallet wallet_name=wo disable_private_keys=true ./src/bitcoin-cli -testnet -rpcwallet=wo importdescriptors '[{ "desc": "addr(mmcuW74MyJUZuLnWXGQLoNXPrS9RbFz6gD)#tpnrahgc", "timestamp": 0, "active": false, "internal": false, "next": 0 }]' ./src/bitcoin-cli -testnet unloadwallet wo error code: -4 error message: Wallet is currently rescanning. Abort existing rescan or wait. ACKs for top commit: achow101: ACK 109cbb8 w0xlt: ACK bitcoin@109cbb8 kouloumos: ACK 109cbb8 promag: ACK 109cbb8 Tree-SHA512: 15fdddf4cf9f3fa08f52069fe4a25a76e04a55bb2586b031bfb0393dce7f175dcdb52823e132a7dff6a894539beeb980a1aad2a792790be036be6977628149b2

DrahtBot added the RPC/REST/ZMQ label Dec 1, 2022

stickies-v reviewed Dec 2, 2022

View reviewed changes

ishaanam reviewed Dec 3, 2022

View reviewed changes

aureleoules force-pushed the 2022-11-unloadwallet-with-rescan branch from 8198c76 to bee026e Compare December 7, 2022 14:08

aureleoules force-pushed the 2022-11-unloadwallet-with-rescan branch from bee026e to 2567ffc Compare December 7, 2022 14:11

promag reviewed Dec 7, 2022

View reviewed changes

luke-jr suggested changes Dec 7, 2022

View reviewed changes

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

furszy reviewed Dec 8, 2022

View reviewed changes

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

aureleoules force-pushed the 2022-11-unloadwallet-with-rescan branch from 2567ffc to 3f31949 Compare December 8, 2022 15:11

promag suggested changes Dec 8, 2022

View reviewed changes

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

aureleoules force-pushed the 2022-11-unloadwallet-with-rescan branch from 3f31949 to 8aa324e Compare December 8, 2022 15:41

aureleoules added 2 commits December 8, 2022 16:45

rpc: Prevent unloading a wallet when rescanning

b13902d

doc: Add release notes for bitcoin#26618

109cbb8

aureleoules force-pushed the 2022-11-unloadwallet-with-rescan branch from 8aa324e to 109cbb8 Compare December 8, 2022 15:45

aureleoules mentioned this pull request Dec 14, 2022

test: Wallet interactions with rescanning wallet #26700

Closed

w0xlt approved these changes Dec 16, 2022

View reviewed changes

kouloumos reviewed Jan 9, 2023

View reviewed changes

promag reviewed Jan 9, 2023

View reviewed changes

achow101 closed this Jan 9, 2023

aureleoules deleted the 2022-11-unloadwallet-with-rescan branch January 12, 2023 11:51

ryanofsky mentioned this pull request Jan 20, 2023

refactor: Use util::Result class for wallet loading #25722

Draft

bitcoin locked and limited conversation to collaborators Jan 12, 2024

rpc: Prevent unloading a wallet when rescanning #26618

rpc: Prevent unloading a wallet when rescanning #26618

Uh oh!

Conversation

aureleoules commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules commented Dec 3, 2022

Uh oh!

w0xlt commented Dec 3, 2022

Uh oh!

luke-jr commented Dec 7, 2022

Uh oh!

aureleoules commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ziggie1984 commented Dec 8, 2022

Uh oh!

Uh oh!

aureleoules commented Dec 8, 2022

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aureleoules commented Dec 8, 2022

Uh oh!

promag commented Dec 8, 2022

Uh oh!

aureleoules commented Dec 8, 2022

Uh oh!

promag commented Dec 8, 2022

Uh oh!

ziggie1984 commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Dec 8, 2022

Uh oh!

luke-jr commented Dec 9, 2022

Uh oh!

aureleoules commented Dec 14, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 9, 2023

Uh oh!

achow101 commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

aureleoules commented Dec 1, 2022 •

edited

Loading

DrahtBot commented Dec 1, 2022 •

edited

Loading

aureleoules commented Dec 7, 2022 •

edited

Loading

ziggie1984 commented Dec 8, 2022 •

edited

Loading