Introduce g_wallet_manager, prepare for better dynamic wallet loading/unloading #12587
Conversation
ec44883 to
e510ec4
Compare
src/qt/rpcconsole.cpp
Outdated
| // in Qt, use always the wallet with index 0 when running with multiple wallets | ||
| QByteArray encodedName = QUrl::toPercentEncoding(QString::fromStdString(vpwallets[0]->GetName())); | ||
| QByteArray encodedName = QUrl::toPercentEncoding(QString::fromStdString(g_wallet_manager->GetWalletAtPos(0)->GetName())); |
There was a problem hiding this comment.
Between the above call HasWallets() and GetWalletAtPos() the wallet can be unloaded. Not possible at the moment, but maybe we should take the opportunity to add a lock to g_wallet_manager?
There was a problem hiding this comment.
Indeed. The locking would be the next step (not only for this case). But its out of scope for this PR.
src/wallet/wallet.h
Outdated
| @@ -1274,10 +1271,10 @@ std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key); | |||
| class WalletRescanReserver | |||
| { | |||
| private: | |||
| CWalletRef m_wallet; | |||
| CWallet *m_wallet; | |||
src/wallet/walletmanager.cpp
Outdated
| #include <wallet/walletmanager.h> | ||
| #include <wallet/wallet.h> | ||
|
|
||
| std::unique_ptr<CWalletManager> g_wallet_manager; |
There was a problem hiding this comment.
Indeed. Fixed.
| unsigned int CountWallets(); | ||
|
|
||
| template<typename Callable> | ||
| void ForEachWallet(Callable&& func) |
There was a problem hiding this comment.
Why not just return a copy of the array?
There was a problem hiding this comment.
I think a lambda is more flexible... same reason we use it in CNode/g_conman.
There was a problem hiding this comment.
In g_connman makes sense because cs_vNodes is held all the time. Here we don't have locks. Even when we add dynamic wallets, IMHO the same pattern doesn't apply here.
There was a problem hiding this comment.
IMO, once we add the locking (not the scope of this PR), then it makes more sense to keep the locking inside of the manager instead of returning a copy.
e510ec4 to
bfab56c
Compare
|
Rebased. Fixed points reported by @promag and the travis issue. |
|
Needs rebase. |
Prepare for a better pointer handling
bfab56c to
d7523a9
Compare
|
Rebased. |
|
I guess this makes no longer sense after #10762. Closing. |
This PR moves manipulation and knowledge of
vpwallet(the vector holding the CWallet* pointers) intoCWalletManager. All access will go through the global g_wallet_manager (similar to g_connman).This should it make easier to later add concurrency protection for wallet adding (loading) / unloading as well as it cleans up the code.
There is a lot of room for optimisation, but, to make faster progress, the scope of this PR should not be widened.