[wallet] `loadwallet` RPC - load wallet at runtime by jnewbery · Pull Request #10740 · bitcoin/bitcoin

jnewbery · 2017-07-04T14:24:10Z

Adds a loadwallet RPCs. This allows wallets to be loaded dynamically during runtime without having to stop-start the node with new -wallet params.

Includes functional tests and release notes.

Limitations:

currently this functionality is only available through the RPC interface.
wallets loaded in this way will not be displayed in the GUI.

jonasschnelli · 2017-07-05T16:22:10Z

src/init.cpp

It'd probably call this deallocWallets()

Yes, DeleteWallets() is a bad name. DeallocWallets() is good, or perhaps DetachWallets()

jonasschnelli · 2017-07-05T16:24:20Z

src/wallet/db.cpp

What's the reasons for keeping this function (that now always returns true)?

You're right. This (and CWalletDB::VerifyEnvironment()) are now entirely vestigial and are not called. Both can be removed.

jonasschnelli · 2017-07-05T16:26:54Z

src/wallet/wallet.cpp

I guess this belongs to walletinit.h now?

Why? vpwallets is accessed by functions in several of the wallet source files.

I had the idea that walletinit.h/.c is kind of the multiwallet interface (wallet manager). The functions we have there (DeleteWallets(), VerifyWallets(), ShutdownWallets();, etc.) sounded for me after vpwallets belongs there...

IMO, the multiwallet map should ideally not sit within the objects (CWallet) implementation.

Tend to agree with @jonasschnelli here, though I agree the barrier isn't exactly clear (yet). Ideally we'd move more towards clarifying the interface here - making CWallet less and less aware of all the things around it (including other wallets). @sipa thoughts here?

jnewbery · 2017-07-07T10:30:35Z

This overlaps significantly with #10762. Not sure how I should ask for review. Perhaps find out which one people think is the priority? Or I could split the walletinit.cpp commits into their own PR.

jnewbery · 2017-07-07T10:33:57Z

src/wallet/rpcwallet.cpp

On further thought, I think I prefer the names attachwallet and detachwallet. That gives a bit more distinction from the dump/import RPCs and makes a stronger implication that the new .dat file is being loaded and attached as a separate wallet.

promag · 2017-07-07T10:43:28Z

@jnewbery IMO this one should rebase on the other.

jnewbery · 2017-07-07T10:46:52Z

IMO this one should rebase on the other.

Makes logical sense. Downside is that the other is a more significant change, so may take longer to get merged.

promag · 2017-07-07T10:54:19Z

If this really depends on that work then there's no other way than waiting. The other downside is that the other commits will show up here too.

jnewbery · 2017-07-07T15:09:16Z

I've changed the PR sequence, so this will now depend on #10767. If I get positive concept feedbac on #10767 I'll rebase this on top of it.

For now, I'm still just looking for concept feedback.

jnewbery · 2017-08-28T21:21:22Z

Rebased on #10767 . Reviewers, please review #10767 first.

…terface 5d2a399 [trivial] fixup comment for VerifyWallets() (John Newbery) 43b0e81 [wallet] Add StartWallets() function to wallet/init.cpp (John Newbery) 290f3c5 [wallet] Add RegisterWalletRPC() function to wallet/init.cpp (John Newbery) 062d631 [wallet] Add CloseWallets() function to wallet/init.cpp (John Newbery) 77fe07c [wallet] Add StopWallets() function to wallet/init.cpp (John Newbery) 2da5eaf [wallet] Add FlushWallets() function to wallet/init.cpp (John Newbery) 1b9cee6 [wallet] Rename WalletVerify() to VerifyWallets() (John Newbery) 9c76ba1 [wallet] Rename InitLoadWallet() to OpenWallets() (John Newbery) Pull request description: Apologies for the mostly code move only PR. This is a pre-req for both #10740 and #10762 All wallet component initialization/destruction functions are now in their own `wallet/init.cpp` translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet. There should be no changes in behavior from this PR. Tree-SHA512: 7c260eb094f2fa1a88d803769ba60935810968a7309f731135e4b17623b97f18c03bbcd293c942093d1efce62c6c978f9ff484d54dc9a60bc2fcb5af2d160fcd

jnewbery · 2017-09-11T16:31:21Z

Rebased now that #10767 is merged. Still a work in progress. PR text updated.

jnewbery · 2017-09-20T14:16:16Z

Bad automatic merge of the testcase. Re-merged manually.

jnewbery · 2017-09-20T16:51:34Z

Manual rebase fixed the test bug. Travis now passes.

This is ready for initial review.

promag

Quick review. All touched code could be updated as per dev notes.

Most importantly, With the LOCK(cs_wallets) it will be impossible to have concurrent RPC to different wallets, not to mention to the same wallet. Is this expected behavior? - I'm assuming there is a thread pool for RPC.

promag · 2017-09-21T09:07:28Z

src/qt/bitcoin.cpp

promag · 2017-09-21T09:08:01Z

src/qt/bitcoin.cpp

Missing spaces after ,?

promag · 2017-09-21T09:39:58Z

src/wallet/init.cpp

Throw an error if -salvagewallet and there are multiple wallets?

Sounds like a sensible change in behaviour. I think it's beyond the scope of this PR.

#10740 (comment)

In commit "Allow single wallet to be verified"

It looks like WalletParameterInteraction is already throwing an error in this case. I think it would be better to drop the comment and wallet_files check here. If you'd prefer to keep the check, I think the comment should be something clearer like "// parameter interaction code should have thrown an error if -salvagewallet was enabled with more than wallet file, so the wallet_files size check here should have no effect."

Updated comment as suggested by @ryanofsky .

Appears this comment update was lost somewhere?

promag · 2017-09-21T09:45:52Z

src/wallet/rpcwallet.cpp

Why not just LOCK(cs_wallets) too?

Because I'd prefer to avoid recursive locking if possible.

Just curious, why?

Yes, definitely. cs_wallets is locked before cs_main in global lock order, so putting it too many places is just asking for trouble, IMO.

jnewbery · 2017-09-21T13:43:28Z

Thanks for the review @promag!

Regarding the locking: Yes, I've implemented this as a mutex for now, which as you point out would limit wallet RPCs to be single-threaded. I'd like to update this to be something a bit better before v0.16. Suggestions are either a shared mutex or (suggested by @theuni) a version of the decay pointers in #10738. The problem with using a shared mutex is that C++ doesn't have a standard library shared mutex until C++14 (std::shared_timed_mutex) so I'd either have to use boost::shared_mutex or roll my own.

(apologies - I thought I'd already written a note in this PR explaining this, but I must have failed to hit send!)

For now, I think review of this PR with the mutex is helpful, and I'll change it around to use a shared_mutex later.

luke-jr · 2017-09-21T20:03:51Z

This seems to ignore the whole issue of long-term wallet references, and probably locks cs_wallets too long.

…tion interface 5d2a399 [trivial] fixup comment for VerifyWallets() (John Newbery) 43b0e81 [wallet] Add StartWallets() function to wallet/init.cpp (John Newbery) 290f3c5 [wallet] Add RegisterWalletRPC() function to wallet/init.cpp (John Newbery) 062d631 [wallet] Add CloseWallets() function to wallet/init.cpp (John Newbery) 77fe07c [wallet] Add StopWallets() function to wallet/init.cpp (John Newbery) 2da5eaf [wallet] Add FlushWallets() function to wallet/init.cpp (John Newbery) 1b9cee6 [wallet] Rename WalletVerify() to VerifyWallets() (John Newbery) 9c76ba1 [wallet] Rename InitLoadWallet() to OpenWallets() (John Newbery) Pull request description: Apologies for the mostly code move only PR. This is a pre-req for both bitcoin#10740 and bitcoin#10762 All wallet component initialization/destruction functions are now in their own `wallet/init.cpp` translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet. There should be no changes in behavior from this PR. Tree-SHA512: 7c260eb094f2fa1a88d803769ba60935810968a7309f731135e4b17623b97f18c03bbcd293c942093d1efce62c6c978f9ff484d54dc9a60bc2fcb5af2d160fcd

fanquake added the Wallet label Jul 5, 2017

jnewbery force-pushed the load_unload_wallet branch from f12b3f6 to 72c3745 Compare July 5, 2017 15:55

jonasschnelli reviewed Jul 5, 2017

View reviewed changes

jnewbery commented Jul 7, 2017

View reviewed changes

This was referenced Jul 7, 2017

[wallet] Remove Wallet dependencies from init.cpp #10762

Merged

[wallet] Clarify wallet initialization / destruction interface #10767

Merged

jnewbery force-pushed the load_unload_wallet branch from 72c3745 to 3f80aae Compare August 28, 2017 21:18

jnewbery force-pushed the load_unload_wallet branch from 3f80aae to f258768 Compare August 30, 2017 18:13

jnewbery force-pushed the load_unload_wallet branch from f258768 to 91aea7e Compare September 11, 2017 16:28

jnewbery mentioned this pull request Sep 12, 2017

[tests] Test listwallets RPC #11310

Merged

jnewbery force-pushed the load_unload_wallet branch 3 times, most recently from 9baf566 to 15705f4 Compare September 15, 2017 21:16

jnewbery changed the title ~~[WIP] [wallet] dynamic loading/unloading of wallets~~ [wallet] dynamic loading/unloading of wallets Sep 15, 2017

jnewbery force-pushed the load_unload_wallet branch from 15705f4 to 6c5b841 Compare September 20, 2017 14:15

promag reviewed Sep 21, 2017

View reviewed changes

PastaPastaPasta mentioned this pull request Apr 16, 2020

Merge bitcoin#10762: [wallet] Remove Wallet dependencies from init.cpp dashpay/dash#3412

Merged

PastaPastaPasta mentioned this pull request Apr 18, 2020

Backport Wallet refactoring and external wallet files 11687 and 13017 dashpay/dash#3426

Merged

Conversation

jnewbery commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Dec 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jul 7, 2017

Uh oh!

jnewbery commented Jul 7, 2017

Uh oh!

promag commented Jul 7, 2017

Uh oh!

jnewbery commented Jul 7, 2017

Uh oh!

jnewbery commented Aug 28, 2017

Uh oh!

jnewbery commented Sep 11, 2017

Uh oh!

jnewbery commented Sep 20, 2017

Uh oh!

jnewbery commented Sep 20, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 21, 2017

Uh oh!

luke-jr commented Sep 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jnewbery commented Jul 4, 2017 •

edited

Loading

TheBlueMatt Dec 3, 2017 •

edited

Loading

promag Sep 21, 2017 •

edited

Loading