rpc: Fix wallet unload during walletpassphrase timeout #14453

pull promag wants to merge 2 commits into bitcoin:master from promag:2018-10-unload-unlocked-wallet changing 2 files +17 −8

promag commented at 11:43 PM on October 9, 2018: member
Replaces the raw wallet pointer in the RPCRunLater callback with a std::weak_ptr to check if the wallet is not expired.

To test:
```
bitcoind -regtest
bitcoin-cli -regtest encryptwallet foobar
bitcoin-cli -regtest walletpassphrase foobar 5 && bitcoin-cli -regtest unloadwallet ""
```
Fixes #14452.
promag commented at 11:45 PM on October 9, 2018: member

@furqansiddiqui please confirm if this change fixes your issue.
fanquake added the label Wallet on Oct 9, 2018
fanquake added the label RPC/REST/ZMQ on Oct 9, 2018
DrahtBot commented at 12:16 AM on October 10, 2018: member
Reviewers, this pull request conflicts with the following ones:
- #14350 (Add WalletLocation class by promag)
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.
MarcoFalke added this to the milestone 0.17.1 on Oct 10, 2018
MarcoFalke added the label Needs backport on Oct 10, 2018
DocOBITCOIN approved
in src/wallet/rpcwallet.cpp:2002 in ada40e9a6c outdated
```
1996 | @@ -2004,7 +1997,14 @@ static UniValue walletpassphrase(const JSONRPCRequest& request)
1997 |      pwallet->TopUpKeyPool();
1998 |  
1999 |      pwallet->nRelockTime = GetTime() + nSleepTime;
2000 | -    RPCRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), std::bind(LockWallet, pwallet), nSleepTime);
2001 | +    std::weak_ptr<CWallet> weak_wallet = wallet;
2002 | +    RPCRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), [weak_wallet] {
2003 | +        if (auto wallet = weak_wallet.lock()) {
```
practicalswift commented at 1:49 PM on October 13, 2018:

Use another name here to avoid shadowing existing local variable wallet :-)

promag commented at 1:58 PM on October 13, 2018:

wallet is not captured.

practicalswift commented at 2:15 PM on October 13, 2018:

Sorry, sloppy wording from me - it is not a local variable. Still better to minimize surprises by not re-using the name, no?

promag commented at 9:07 PM on October 15, 2018:

What surprise?

ryanofsky commented at 9:21 PM on October 18, 2018:

What surprise?

Surprise of having two different variables inside the walletpassphrase function with same name. I agree it would be a little clearer to rename this latter wallet variable to something like shared_wallet to differentiate it.

promag commented at 10:28 PM on October 18, 2018:

@practicalswift renamed.
sipa commented at 7:29 AM on October 17, 2018: member

utACK ada40e9a6cfb3d5d738658c739e80b03a86a2384
ryanofsky approved
ryanofsky commented at 9:23 PM on October 18, 2018: member

utACK ada40e9a6cfb3d5d738658c739e80b03a86a2384
ryanofsky commented at 9:29 PM on October 18, 2018: member

From the description in #14452, it seems like it wouldn't be difficult to add an automated test (maybe in wallet_encryption.py) that triggers the current crash. But I think this PR looks good even without the test.
promag commented at 9:45 PM on October 18, 2018: member

Renamed variable and added a comment with rationale.
promag force-pushed on Oct 18, 2018
ryanofsky commented at 9:50 PM on October 18, 2018: member

utACK e24902bd4b2ad79e929cc36a67bc7d1edcec5d16
rpc: Fix wallet unload during walletpassphrase timeout 321decffa1
promag force-pushed on Oct 18, 2018
promag commented at 10:39 PM on October 18, 2018: member

@ryanofsky ops e24902b has a typo. Fixed and squashed.
ryanofsky commented at 4:49 PM on October 19, 2018: member

utACK 321decffa1fbf213462d97e5372bd0c4eeb99635. Only changes are shadow variable rename and new comment.
promag commented at 4:55 PM on October 19, 2018: member

Will add a test.
promag commented at 10:12 PM on October 19, 2018: member

@ryanofsky @jonasschnelli the test change fails in master.
sipa commented at 1:54 AM on October 20, 2018: member

reutACK 07a3942c51e0b55952bebe03f4c70a2d8f967c9c
MarcoFalke commented at 6:50 AM on October 20, 2018: member

utACK 07a3942c51e0b55952bebe03f4c70a2d8f967c9c
qa: Ensure wallet unload during walletpassphrase timeout 8907df9e02
promag force-pushed on Oct 20, 2018
promag commented at 10:06 AM on October 20, 2018: member

@sipa @MarcoFalke sorry, just added a comment to the test.
MarcoFalke commented at 2:17 AM on October 21, 2018: member

re-utACK 8907df9e02ec47ef249a7422faa766f06aa01e94. Only change is a new comment in the test commit
in test/functional/wallet_multiwallet.py:275 in 8907df9e02
```
 267 | @@ -267,7 +268,11 @@ def wallet_file(name):
 268 |          assert 'w1' not in self.nodes[0].listwallets()
 269 |  
 270 |          # Successfully unload the wallet referenced by the request endpoint
 271 | +        # Also ensure unload works during walletpassphrase timeout
 272 | +        w2.encryptwallet('test')
 273 | +        w2.walletpassphrase('test', 1)
 274 |          w2.unloadwallet()
 275 | +        time.sleep(1.1)
```
ryanofsky commented at 4:13 PM on October 22, 2018:

Why is a sleep needed between unloadwallet and listwallets? If it's not needed to make listwallets work, it might make the test clearer to move the sleep immediately before the code that actually does depends on it (loadwallet?) and add a comment describing what the sleep is for.

promag commented at 5:04 PM on October 23, 2018:

Yeah, the sleep is not actually needed (there). However if the test terminates (and the node quits) then it's not guaranteed the timeout callback was executed? I can add a comment like "wait to ensure walletpassphrase callback is executed".
ryanofsky approved
ryanofsky commented at 6:18 PM on October 22, 2018: member

utACK 8907df9e02ec47ef249a7422faa766f06aa01e94. Only change is new test.
laanwj merged this on Oct 24, 2018
laanwj closed this on Oct 24, 2018
laanwj referenced this in commit a74ed3a05b on Oct 24, 2018
promag deleted the branch on Oct 24, 2018
MarcoFalke referenced this in commit bd7cb7f84b on Oct 24, 2018
MarcoFalke referenced this in commit 44450e972a on Oct 24, 2018
MarcoFalke referenced this in commit c5d53b67af on Oct 25, 2018
MarcoFalke referenced this in commit 8fed8078da on Oct 25, 2018
MarcoFalke referenced this in commit df222b2d6f on Oct 28, 2018
MarcoFalke referenced this in commit 9b7e5191da on Oct 28, 2018
MarcoFalke referenced this in commit 8dfae4cdcd on Dec 5, 2018
promag referenced this in commit 75b5d8c4ea on Dec 5, 2018
promag referenced this in commit 98fd889999 on Dec 5, 2018
promag referenced this in commit dcb032dcdf on Dec 6, 2018
MarcoFalke referenced this in commit abae8aeff1 on Dec 6, 2018
MarcoFalke removed the label Needs backport on Dec 6, 2018
deadalnix referenced this in commit f4912034ad on May 25, 2020
ftrader referenced this in commit d1de921473 on Aug 17, 2020
PastaPastaPasta referenced this in commit 3466bf7071 on Jun 27, 2021
PastaPastaPasta referenced this in commit 1e38d4c633 on Jun 28, 2021
PastaPastaPasta referenced this in commit aeae2b5caa on Jun 29, 2021
PastaPastaPasta referenced this in commit ea211c80a3 on Jul 1, 2021
PastaPastaPasta referenced this in commit 1576f3b470 on Jul 1, 2021
PastaPastaPasta referenced this in commit 52eb9183b1 on Jul 1, 2021
PastaPastaPasta referenced this in commit 631020a6e8 on Jul 3, 2021
MarcoFalke locked this on Sep 8, 2021