Skip to content
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

Introduce WalletManager #13034

Closed
wants to merge 1 commit into from
Closed

Conversation

promag
Copy link
Member

@promag promag commented Apr 20, 2018

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.

@promag
Copy link
Member Author

promag commented Apr 20, 2018

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).

@@ -756,7 +757,7 @@ void MaybeCompactWalletDB()
return;
}

for (CWallet* pwallet : GetWallets()) {
for (CWallet* pwallet : g_wallet_manager.GetWallets()) {
Copy link
Member Author

@promag promag Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/Makefile.am Outdated
@@ -258,6 +259,7 @@ libbitcoin_wallet_a_SOURCES = \
wallet/rpcwallet.cpp \
wallet/wallet.cpp \
wallet/walletdb.cpp \
wallet/walletmanager.cpp \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use spaces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, ty.

@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 20, 2018

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 0b39b17 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
Copy link
Member Author

promag commented Apr 20, 2018

@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
Copy link
Member Author

promag commented Apr 20, 2018

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
Copy link
Contributor

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.

{
LOCK(m_cs);
for (CWallet* wallet : m_wallets) {
if (wallet->GetName() == name) return wallet;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@promag promag Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
return ::vpwallets.size() == 1 || (request.fHelp && ::vpwallets.size() > 0) ? ::vpwallets[0] : nullptr;

std::vector<CWallet*> wallets = g_wallet_manager.GetWallets();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@promag promag Apr 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jnewbery
Copy link
Contributor

needs rebase

@promag
Copy link
Member Author

promag commented Apr 24, 2018

Rebased.

@jnewbery
Copy link
Contributor

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

Tested ACK d04cd68d5d1a6b1ddaa36b9eb6884014f62da6ea

@promag
Copy link
Member Author

promag commented Apr 24, 2018

@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
Copy link
Member Author

promag commented Apr 29, 2018

Rebased due #12830 which added a new test file.

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

@DrahtBot DrahtBot closed this Sep 21, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants