rpc: Prevent unloading a wallet when rescanning #26618

aureleoules commented at 4:09 PM on December 1, 2022: member

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 commented at 4:09 PM on December 1, 2022: contributor

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.

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

stickies-v commented at 4:43 PM on December 2, 2022: contributor

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 commented at 1:05 AM on December 3, 2022: contributor

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 commented at 10:28 AM on December 3, 2022: member

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 commented at 8:06 PM on December 3, 2022: contributor

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 commented at 1:34 AM on December 7, 2022: member

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 force-pushed on Dec 7, 2022

aureleoules commented at 2:09 PM on December 7, 2022: member

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

aureleoules force-pushed on Dec 7, 2022

promag commented at 8:28 PM on December 7, 2022: member

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.

in src/wallet/rpc/wallet.cpp:415 in 2567ffc5f1 outdated

 411 | @@ -412,6 +412,7 @@ static RPCHelpMan unloadwallet()
 412 |                  {
 413 |                      {"wallet_name", RPCArg::Type::STR, RPCArg::DefaultHint{"the wallet name from the RPC endpoint"}, "The name of the wallet to unload. If provided both here and in the RPC endpoint, the two must be identical."},
 414 |                      {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED_NAMED_ARG, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},
 415 | +                    {"force", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED_NAMED_ARG, "Force unload of wallet. This will abort any rescan in progress."},

luke-jr commented at 9:14 PM on December 7, 2022:

This is a positional parameter. See simulaterawtransaction for how to do named-only parameters in an options Object.

(Maybe move "load_on_startup" there too)

aureleoules commented at 11:05 AM on December 8, 2022:

Moving load_on_startup to the options object would be a compatibility-breaking change no?

luke-jr changes_requested

ziggie1984 commented at 12:01 AM on December 8, 2022: none

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).

in src/wallet/rpc/wallet.cpp:446 in 2567ffc5f1 outdated

 437 | @@ -437,6 +438,14 @@ static RPCHelpMan unloadwallet()
 438 |          throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
 439 |      }
 440 |  
 441 | +    if (wallet->IsScanning()) {
 442 | +        if (request.params[2].isNull() || !request.params[2].get_bool()) {
 443 | +            throw JSONRPCError(RPC_WALLET_ERROR, "Cannot unload a wallet while it is rescanning. Retry with the force option or run abortrescan first.");
 444 | +        }
 445 | +
 446 | +        wallet->AbortRescan();

furszy commented at 12:47 PM on December 8, 2022:

As AbortRescan() does not abort the scanning process right away (only sets a flag that is later read by the wallet's scanning thread), might be good to add a "wait until scan aborts" here.

So there is no race between the wallet removal and the scanning process.

aureleoules force-pushed on Dec 8, 2022

aureleoules commented at 3:16 PM on December 8, 2022: member

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.

in src/wallet/rpc/wallet.cpp:440 in 3f31949132 outdated

 436 | @@ -437,6 +437,11 @@ static RPCHelpMan unloadwallet()
 437 |          throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
 438 |      }
 439 |  
 440 | +    WalletRescanReserver reserver(*wallet);

promag commented at 3:29 PM on December 8, 2022:

reserver destructor needs to be called before UnloadWallet, something like:

{
    WalletRescanReserver reserver(*wallet);
    ....
    
    RemoveWallet()
}
UnloadWallet();

promag changes_requested

promag commented at 3:30 PM on December 8, 2022: member

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

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

aureleoules force-pushed on Dec 8, 2022

rpc: Prevent unloading a wallet when rescanning b13902d2e4

doc: Add release notes for #26618 109cbb819d

aureleoules force-pushed on Dec 8, 2022

aureleoules commented at 3:47 PM on December 8, 2022: member

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 commented at 4:08 PM on December 8, 2022: member

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 commented at 4:34 PM on December 8, 2022: member

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 commented at 4:36 PM on December 8, 2022: member

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

ziggie1984 commented at 11:02 PM on December 8, 2022: none

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 commented at 11:07 PM on December 8, 2022: member

@ziggie1984 user-misbehaving

luke-jr commented at 3:53 AM on December 9, 2022: member

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

aureleoules commented at 7:43 PM on December 14, 2022: member

I added a test in #26700 @promag.

w0xlt approved

w0xlt commented at 2:38 PM on December 16, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26618/commits/109cbb819ddd20220a255791c40261f746260f42

kouloumos commented at 3:44 PM on January 9, 2023: contributor

ACK 109cbb819ddd20220a255791c40261f746260f42 The implementation is consistent with how the WalletRescanReserver is used in the rest of the related RPCs as pointed out in #26618#pullrequestreview-1209074941 . (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 commented at 4:22 PM on January 9, 2023: member

ACK 109cbb819ddd20220a255791c40261f746260f42

achow101 commented at 9:18 PM on January 9, 2023: member

ACK 109cbb819ddd20220a255791c40261f746260f42

achow101 referenced this in commit 1aedc3b6c8 on Jan 9, 2023

achow101 commented at 10:02 PM on January 9, 2023: member

This was merged

achow101 closed this on Jan 9, 2023

sidhujag referenced this in commit 1ab8c80040 on Jan 10, 2023

aureleoules deleted the branch on Jan 12, 2023

bitcoin locked this on Jan 12, 2024