New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Make unloadwallet wait for complete wallet unload #14941

Merged
merged 2 commits into from Jan 15, 2019

Conversation

Projects
None yet
7 participants
@promag
Copy link
Member

promag commented Dec 12, 2018

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

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Member Author

promag commented Dec 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Dec 13, 2018

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 promag force-pushed the promag:2018-12-sync-unloadwallet branch Dec 13, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Dec 13, 2018

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.

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch Dec 13, 2018

@promag 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 promag force-pushed the promag:2018-12-sync-unloadwallet branch Dec 13, 2018

Show resolved Hide resolved 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 src/wallet/wallet.cpp Outdated

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch Dec 14, 2018

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

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch 4 times, most recently Dec 18, 2018

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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."
Show resolved Hide resolved src/wallet/wallet.h Outdated
Show resolved Hide resolved src/init.cpp
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
@promag

This comment has been minimized.

Copy link
Member Author

promag commented Dec 18, 2018

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

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch Dec 18, 2018

@promag
Copy link
Member Author

promag left a comment

Self review, 2 comments must be fixed.

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

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch 4 times, most recently Dec 19, 2018

@laanwj laanwj added this to the 0.17.2 milestone Dec 19, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 4, 2019

Updated after @ryanofsky review, added release notes and updated description.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jan 4, 2019

Concept ACK.

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK c2ae9d9c295ca75f6653892b5f5e1ad6f5c1a9e7. Changes since last review were adding documentation and assert. Also fixing unrelated (I think) encrypted_batch potential memory leak in CWallet destructor.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 8, 2019

utACK c2ae9d9c295ca75f6653892b5f5e1ad6f5c1a9e7

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 10, 2019

Lightly tested ACK c2ae9d9c295ca75f6653892b5f5e1ad6f5c1a9e7, although I have a preference for @ryanofsky's proposed simplification here: #14941 (comment)

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 14, 2019

Pushed the simplification, also split code from assertions.

@jnewbery
Copy link
Member

jnewbery left a comment

Looks good. Comment needs updating, and I'd squash the new commit 'fixup: Russ simplification' into 'rpc: Make unloadwallet wait for complete wallet unload'

Show resolved Hide resolved src/wallet/wallet.cpp Outdated
@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK e6cfc2b566b5a036d884754363b95ac94808907d. Only change since last review is new commit.

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

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch 2 times, most recently Jan 14, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 14, 2019

Latest changes in 4a50dce. Squashed.

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch 2 times, most recently Jan 14, 2019

@promag promag force-pushed the promag:2018-12-sync-unloadwallet branch to 645e905 Jan 15, 2019

@laanwj laanwj merged commit 645e905 into bitcoin:master Jan 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 15, 2019

Merge #14941: rpc: Make unloadwallet wait for complete wallet 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 #14919, fixes #14917.

Tree-SHA512: ad88b980e2f3652809a58f904afbfe020299f3aa6a517f495ba943b8d54d4520f6e70074d6749be8f5967065c0f476e0faedcde64c8b4899e5f99c70f0fd6534
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 15, 2019

utACK 645e905

@jnewbery jnewbery removed this from Blockers in High-priority for review Jan 15, 2019

promag added a commit to promag/bitcoin that referenced this pull request Jan 16, 2019

@promag promag deleted the promag:2018-12-sync-unloadwallet branch Jan 20, 2019

laanwj added a commit that referenced this pull request Jan 31, 2019

Merge #15002: 0.17: Backport #14941
0cd9ad2 rpc: Make unloadwallet wait for complete wallet unload (João Barbosa)

Pull request description:

  #14941 makes `unloadwallet` a synchronous call meaning that it waits for the wallet to fully unload/delete.

Tree-SHA512: df7a490306ee2cca399129a4ebfba4b19b65fe67d1657ec3518352fe453327cb347010f94cf7fe4a60aeb51c928cb9ad6b24c40123fd0b9dc0aab5920a59f48d

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 14, 2019

bvbfan added a commit to bvbfan/bitcoin that referenced this pull request Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment