Introduce WalletManager

promag commented at 1:53 am on April 20, 2018: member

This PR (re)introduces WalletManager but follows a different approach than #12587 by @jonasschnelli. It builds on top of #13028 and #13017.

A global g_wallet_manager instance is also added which is only available in builds with ENABLE_WALLET.

The goal here is to have a better place for all code that manages wallet instances, which will be useful for:

wallet lifecycle;
wallet background tasks coordination (flush for instance);
src/interface/walletmanager.h for UI and process separation (@ryanofsky sgty?);
RPC wallet load/unload calls.

fanquake added the label Refactoring on Apr 20, 2018

fanquake added the label Wallet on Apr 20, 2018

promag commented at 1:54 am on April 20, 2018: member

Note to reviewers, I would prefer to have base PR’s reviewed and merged first, but feel free to stay around and comment.

2nd commit is mostly moved code (67420d3).

in src/wallet/walletdb.cpp:760 in 67420d3191 outdated

756@@ -756,7 +757,7 @@ void MaybeCompactWalletDB()
757         return;
758     }
759 
760-    for (CWallet* pwallet : GetWallets()) {
761+    for (CWallet* pwallet : g_wallet_manager.GetWallets()) {

promag commented at 1:58 am on April 20, 2018:

BTW, I’m planning to move the iteration to WalletManager and make MaybeCompactWalletDB(WalletDatabase& dbh). This way WalletManager dependency is removed here. @jnewbery it would also move scheduler.scheduleEvery(MaybeCompactWalletDB, 500); to WalletManager.

promag force-pushed on Apr 20, 2018

in src/Makefile.am:262 in ac21dd2343 outdated

258@@ -258,6 +259,7 @@ libbitcoin_wallet_a_SOURCES = \
259   wallet/rpcwallet.cpp \
260   wallet/wallet.cpp \
261   wallet/walletdb.cpp \
262+	wallet/walletmanager.cpp \

ken2812221 commented at 8:03 am on April 20, 2018:

It would be better to use spaces.

promag commented at 8:37 am on April 20, 2018:

Fixed, ty.

promag force-pushed on Apr 20, 2018

ryanofsky commented at 10:48 am on April 20, 2018: member

which will be useful for […] src/interface/walletmanager.h for UI and process separation (@ryanofsky sgty?);

It does not seem correct to me to claim shallow organizational changes like this PR, #12587, or #13017 will help with process separation or with loading and unloading wallets after startup. I don’t think these changes will hurt anything either (except maybe giving me a few interesting merge conflicts to resolve in 0b39b173e32e1a6e54abbd8d583cd0321a8b26c2 from #10973). But at best they seem like flailing around with code organization and running in place.

Personally, I would prefer to just manage wallets with standard c++ data structures and synchronization primitives instead of having a management interface. But if you’d prefer to have a management interface, I don’t see a reason why you’d want it to use pimpl, or a reason why you would want to expose and duplicate it in src/interfaces/ outside the wallet, instead of just adding a simple handleUnloadWallet callback for the GUI to complement the existing handleLoadWallet and getWallets.

All of these are just surface disagreements about code organization, though, that really don’t have any effect on features. If I could encourage you to work on an actual feature, I think it would be either writing code to implement wallet unloading, or writing a design doc or sequence diagram showing what data structures in the GUI and wallet need to be updated in what order for wallet unloading to work safely and responsively.

promag force-pushed on Apr 20, 2018

promag commented at 12:29 pm on April 20, 2018: member

@ryanofsky thanks for the feedback.

I don’t see a reason why you’d want it to use pimpl

No reason at all, removed.

Personally, I would prefer to just manage wallets with standard c++ data structures and synchronization primitives instead of having a management interface

In the case of dynamic wallet load/unload that would lead to code duplication right? - UI and RPC layers want to load/unload wallets.

But if you’d prefer to have a management interface

It’s not a matter of my preference. It just makes sense to have such entity like CAddrMan, CConnman and BanMan. Also, see PR description for other things that can be handled by WalletManager.

All of these are just surface disagreements about code organization, though, that really don’t have any effect on features.

It does not seem correct to me to claim shallow organizational changes like this PR, #12587, or #13017 will help with process separation or with loading and unloading wallets after startup

I disagree if these changes make it easy to actually implement and review those features. Historically big PR’s lack reviews, have lots of discussions and later are usually split in multiple PR’s.

If I could encourage you to work on an actual feature, I think it would be either writing code to implement wallet unloading

I am, please read PR description and #11402 rationale.

promag force-pushed on Apr 20, 2018

promag commented at 12:38 pm on April 20, 2018: member

why you would want to expose and duplicate it in src/interfaces/ outside the wallet, instead of just adding a simple handleUnloadWallet callback for the GUI to complement the existing handleLoadWallet and getWallets. @ryanofsky I don’t, I was asking for your feedback. So you also suggest to add interfaces::Node::loadWallet and interfaces::Node::unloadWallet and or alike?

ryanofsky commented at 3:00 pm on April 20, 2018: member

So you also suggest to add interfaces::Node::loadWallet and interfaces::Node::unloadWallet and or alike?

loadWallet might make sense in short term. Longer term with #10102, though I think it would be replaced by a makeWallet method in interfaces::Init to allow the GUI to create wallets without going through the node at all.

unloadWallet should probably be avoided in favor of having more flexible shutdown methods on the Wallet interface itself and cleanup in the destructor.

More broadly, I think there will probably be multiple PRs updating the GUI to support wallet loading and unloading. The first PR might follow up #10740 and use existing handleLoadWallet callback to get newly loaded wallets to show up in the GUI. Independently there could be a PR that adds a handleUnloadWallet method and removes unloaded wallets in the GUI. And there could be one or two PRs adding GUI controls for loading and unloading wallets adding a new loadWallet method. And a cleanup PR could drop getWallets after dynamic loading works.

But the implementation of all these features will primarily consist of updates to gui and wallet code. Changes to interfaces/ code should only be one or two line diffs, and basically be an afterthought.

Personally, I would prefer to just manage wallets with standard c++ data structures and synchronization primitives instead of having a management interface

In the case of dynamic wallet load/unload that would lead to code duplication right? - UI and RPC layers want to load/unload wallets.

No, but this is a bikeshed debate, so please implement what you like! My preference would be to use std::vector<std::unique_ptr<interfaces::Wallet>> in the gui code and std::map<std::string, std::weak_ptr<CWallet>> in rpc and wallet code, along with plain mutexes and condition variables. But if you and others prefer more layered abstractions in the style of WalletManager, more power to you!

But if you’d prefer to have a management interface

It’s not a matter of my preference. It just makes sense to have such entity like CAddrMan, CConnman and BanMan.

Disagree, but this is the same bikeshed debate, so again you should feel free to proceed in whatever way makes sense to you. In my opinion, it makes sense to have classes like AddrMan and ConnMan that manage more complex state, but it doesn’t necessarily make sense to have a class that just manages a list of pointers.

Also, see PR description for other things that can be handled by WalletManager.

Yeah, I didn’t mention it, but this seemed pretty weird to me. This suggests having a bunch of methods that pertain to individual wallets as part of a class that’s supposed to be about managing a list of wallets. Again, this is another bikeshed debate: you might prefer a code organization with more nested layers, and I might prefer an code organization with fewer layers, but I don’t think the difference is very important, and I do think you should implement what you like.

I disagree if these changes make it easy to actually implement and review those features. Historically big PR’s lack reviews, have lots of discussions and later are usually split in multiple PR’s.

It’s doesn’t seem true to me that introducing WalletManager helps implement process separation or wallet unloading. You should feel free to add it anyway, but I think if you sat down and drew a sequence diagram of how wallet and gui state needs to be updated when an unload request comes in, the data structures and notification mechanisms that should be used, and tradeoffs in layering would become more clear, and you could start implementing features more simply without unnecessarily shuffling a bunch of code around first.

in src/wallet/walletmanager.cpp:50 in 8dd937087c outdated

45+
46+CWallet* WalletManager::GetWallet(const std::string& name) const
47+{
48+    LOCK(m_cs);
49+    for (CWallet* wallet : m_wallets) {
50+        if (wallet->GetName() == name) return wallet;

skeees commented at 4:53 pm on April 20, 2018:

if a wallet is canonically identified by its name - you should probably prevent adding a wallet with the same name in AddWallet - it looks like you’re doing find comparison by address there?

promag commented at 6:06 pm on April 24, 2018:

Currently those kinds of checks are in WalletInit::Verify(). Among other things, I would like to move that to WalletManager since those checks will be needed after for dynamic wallet loading (and so doesn’t make sense to be in WalletInit).

in src/wallet/rpcwallet.cpp:54 in 8dd937087c outdated

57+        if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
58+        return pwallet;
59     }
60-    return ::vpwallets.size() == 1 || (request.fHelp && ::vpwallets.size() > 0) ? ::vpwallets[0] : nullptr;
61+
62+    std::vector<CWallet*> wallets = g_wallet_manager.GetWallets();

TheSamHughes commented at 8:44 pm on April 22, 2018:

Doesn’t seem like a good idea to be needlessly copying by value, const reference should be used. However in this case I can see you are taking a copy in the scope of the critical section, however this doesnt make it thread safe, as the wallets them selves can still be deleted, I would recommend moving them to be refcounted.

promag commented at 8:51 pm on April 22, 2018:

I would recommend moving them to be refcounted

That’s the plan (for instance, ~~#11402~~ #13063). Currently wallets aren’t deleted so this is safe for now.

promag force-pushed on Apr 23, 2018

jnewbery commented at 3:59 pm on April 24, 2018: member

needs rebase

promag force-pushed on Apr 24, 2018

promag commented at 4:30 pm on April 24, 2018: member

Rebased.

jnewbery commented at 5:45 pm on April 24, 2018: member

Seems reasonable. Is the plan for WalletManager to eventually subsume WalletInit?

Tested ACK d04cd68d5d1a6b1ddaa36b9eb6884014f62da6ea

promag commented at 6:13 pm on April 24, 2018: member

@jnewbery thanks for the review.

Is the plan for WalletManager to eventually subsume WalletInit?

IMO some code in WalletInit could be moved to WalletManager, especially the one that can be used by dynamic wallet load/unloading. WalletInit would call those new methods.

Some other code of CWallet could be moved here too. I’ll follow up.

promag force-pushed on Apr 29, 2018

promag commented at 0:47 am on April 29, 2018: member

Rebased due #12830 which added a new test file.

promag force-pushed on Apr 29, 2018

wallet: Introduce WalletManager c0e00ee71c

promag force-pushed on May 2, 2018

MarcoFalke added the label Needs rebase on Jun 6, 2018

DrahtBot commented at 5:12 pm on September 21, 2018: member

DrahtBot added the label Up for grabs on Sep 21, 2018

DrahtBot closed this on Sep 21, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

Introduce WalletManager #13034