rpc: Make unloadwallet wait for complete wallet unload #14941

promag · 2018-12-12T23:31:13Z

Currently the unloadwallet RPC is asynchronous, it only signals the intent to unload the wallet and then returns the response to the client. The actual unload can happen later and the client has no way to be notified of that.

This PR makes the unloadwallet RPC synchronous, meaning that it blocks until the wallet is fully unloaded.

Replaces #14919, fixes #14917.

DrahtBot · 2018-12-13T02:03:48Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)
#13926 ([Tools] bitcoin-wallet - a tool for creating and managing wallets offline by jnewbery)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

promag · 2018-12-13T09:07:13Z

@ryanofsky @MarcoFalke

laanwj · 2018-12-13T10:39:53Z

Concept ACK - I think this can be useful outside of the tests, as well.

Also I'd like to discuss the wait default value, currently it avoid changing behavior. If wait=true is considered the correct behavior then this could be backport to 0.17.2?

Strictly spoken this is a feature, not a bugfix, though if it's optional and improves testing I think a point can be made to backport it anyhow.

ryanofsky · 2018-12-13T14:11:46Z

Code change looks ok, but this shouldn't be an option. Making wait=false the default makes unloadwallet pointlessly difficult to use, and inconsistent with other methods, including closely related methods like createwallet and loadwallet.

If you think there is reason to support wait=false, I think you need to say what the use-case is, and why it wouldn't be better served by having the client make a fully asynchronous JSON-RPC request, instead of using this half-asynchronous/half-synchronous wait option that doesn't (I guess?) block on acquiring cs_wallet, but does block on the network connection and on acquiring cs_wallets, and on whatever the UI currently does in NotifyUnload (or will do in the future).

Also, if you want to keep the async=false option, it would be good to have a test to ensure that it doesn't acquire cs_wallet and cs_main. This would be easy to write as a c++ unit test which acquires the locks in one thread and calls unloadwallet in another.

promag · 2018-12-13T16:09:35Z

Changed to synchronous unload. Locally added some sleeps to see if there were any connection timeout, but didn't manage to cause one.

doesn't (I guess?) block on acquiring cs_wallet

no it doesn't, it waits until the weak pointer is expired.

src/wallet/wallet.cpp

ryanofsky

This change seems like it should work, but multiwallet test is failing. There's some problem with combine_logs on travis, but locally I see:

AssertionError: [node 0] Expected message "BerkeleyBatch: Can't open database w8_copy \(duplicates fileid \w+ from w8\)" does not partially match stderr:
"bitcoind: wallet/wallet.cpp:95: void ReleaseWallet(CWallet*): Assertion `g_unloading_wallet_set.erase(wallet) == 1' failed."

src/wallet/wallet.h

src/init.cpp

src/wallet/wallet.cpp

promag · 2018-12-18T21:01:43Z

@ryanofsky looks like you started reviewing before my latest push?

promag

Self review, 2 comments must be fixed.

src/wallet/wallet.cpp

Summary: This adds the `unloadwallet` RPC, release notes, and tests for it. Partial backport of Core PR13111 bitcoin/bitcoin@6608c36 bitcoin/bitcoin@9f9b50d bitcoin/bitcoin@4940a20 bitcoin/bitcoin@ccbf7ae bitcoin/bitcoin@0b82bac (only the additional test added to `wallet_multiwallet.py` is taken from here). Depends on D4252 Note to reviewers: This RPC has a bug discussed here bitcoin/bitcoin#14941 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4253

…unload 645e905 doc: Add release notes for unloadwallet change to synchronous call (João Barbosa) c37851d rpc: Make unloadwallet wait for complete wallet unload (João Barbosa) Pull request description: Currently the `unloadwallet` RPC is asynchronous, it only signals the intent to unload the wallet and then returns the response to the client. The actual unload can happen later and the client has no way to be notified of that. This PR makes the `unloadwallet` RPC synchronous, meaning that it blocks until the wallet is fully unloaded. Replaces bitcoin#14919, fixes bitcoin#14917. Tree-SHA512: ad88b980e2f3652809a58f904afbfe020299f3aa6a517f495ba943b8d54d4520f6e70074d6749be8f5967065c0f476e0faedcde64c8b4899e5f99c70f0fd6534

meshcollider added Wallet RPC/REST/ZMQ labels Dec 12, 2018

laanwj mentioned this pull request Dec 13, 2018

test: Prevent "Duplicate-wallet filename specified" #14919

Closed

promag force-pushed the 2018-12-sync-unloadwallet branch from efaa691 to 36d41ff Compare December 13, 2018 15:59

promag force-pushed the 2018-12-sync-unloadwallet branch from 36d41ff to c85d15d Compare December 13, 2018 18:01

promag changed the title ~~rpc: Add option to unloadwallet to wait for complete wallet unload~~ rpc: Make unloadwallet wait for complete wallet unload Dec 13, 2018

promag force-pushed the 2018-12-sync-unloadwallet branch from c85d15d to ffa8799 Compare December 13, 2018 18:08

ryanofsky reviewed Dec 13, 2018

View reviewed changes

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

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

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

ryanofsky reviewed Dec 13, 2018

View reviewed changes

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

promag force-pushed the 2018-12-sync-unloadwallet branch from ffa8799 to 353659d Compare December 14, 2018 02:21

promag commented Dec 14, 2018

View reviewed changes

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

promag force-pushed the 2018-12-sync-unloadwallet branch 4 times, most recently from fec3c58 to 981138c Compare December 18, 2018 20:38

ryanofsky reviewed Dec 18, 2018

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

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

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

promag force-pushed the 2018-12-sync-unloadwallet branch from 981138c to c8d60d1 Compare December 18, 2018 21:22

promag commented Dec 18, 2018

View reviewed changes

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

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

promag force-pushed the 2018-12-sync-unloadwallet branch 4 times, most recently from a453db6 to 7aed2bb Compare December 19, 2018 00:12

laanwj added this to the 0.17.2 milestone Dec 19, 2018

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

rpc: Make unloadwallet wait for complete wallet unload #14941

rpc: Make unloadwallet wait for complete wallet unload #14941

Uh oh!

Conversation

promag commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

promag commented Dec 13, 2018

Uh oh!

laanwj commented Dec 13, 2018

Uh oh!

ryanofsky commented Dec 13, 2018

Uh oh!

promag commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag commented Dec 18, 2018 • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

promag commented Dec 12, 2018 •

edited

Loading

DrahtBot commented Dec 13, 2018 •

edited

Loading

promag commented Dec 13, 2018 •

edited

Loading

promag commented Dec 18, 2018 •

edited

Loading