Introduce g_wallet_manager, prepare for better dynamic wallet loading/unloading #12587

pull jonasschnelli wants to merge 4 commits into bitcoin:master from jonasschnelli:2018/03/dyn_wallet changing 14 files +174 −97

jonasschnelli commented at 5:16 AM on March 3, 2018: contributor

This PR moves manipulation and knowledge of vpwallet (the vector holding the CWallet* pointers) into CWalletManager. 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.
jonasschnelli added the label Wallet on Mar 3, 2018
jonasschnelli added the label Refactoring on Mar 3, 2018
jonasschnelli force-pushed on Mar 3, 2018
jonasschnelli force-pushed on Mar 3, 2018
jonasschnelli force-pushed on Mar 6, 2018

in src/qt/rpcconsole.cpp:309 in e510ec4a87 outdated

 303 | @@ -304,9 +304,9 @@ bool RPCConsole::RPCParseCommandLine(std::string &strResult, const std::string &
 304 |                              req.strMethod = stack.back()[0];
 305 |  #ifdef ENABLE_WALLET
 306 |                              // TODO: Move this logic to WalletModel
 307 | -                            if (!vpwallets.empty()) {
 308 | +                            if (g_wallet_manager->HasWallets()) {
 309 |                                  // in Qt, use always the wallet with index 0 when running with multiple wallets
 310 | -                                QByteArray encodedName = QUrl::toPercentEncoding(QString::fromStdString(vpwallets[0]->GetName()));
 311 | +                                QByteArray encodedName = QUrl::toPercentEncoding(QString::fromStdString(g_wallet_manager->GetWalletAtPos(0)->GetName()));

promag commented at 2:10 PM on March 6, 2018:

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?

jonasschnelli commented at 9:16 AM on March 18, 2018:

Indeed. The locking would be the next step (not only for this case). But its out of scope for this PR.

in src/wallet/wallet.h:1274 in e510ec4a87 outdated

1270 | @@ -1274,10 +1271,10 @@ std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key);
1271 |  class WalletRescanReserver
1272 |  {
1273 |  private:
1274 | -    CWalletRef m_wallet;
1275 | +    CWallet *m_wallet;

promag commented at 2:11 PM on March 6, 2018:

Nit, CWallet* m_wallet;

in src/wallet/walletmanager.cpp:8 in e510ec4a87 outdated

   0 | @@ -0,0 +1,84 @@
   1 | +// Copyright (c) 2018 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include <wallet/walletmanager.h>
   6 | +#include <wallet/wallet.h>
   7 | +
   8 | +std::unique_ptr<CWalletManager> g_wallet_manager;

promag commented at 2:13 PM on March 6, 2018:

Could be initialized here?

jonasschnelli commented at 9:20 AM on March 18, 2018:

Indeed. Fixed.

in src/wallet/walletmanager.h:41 in e510ec4a87 outdated
```
  36 | +    bool HasWallets();
  37 | +
  38 | +    unsigned int CountWallets();
  39 | +
  40 | +    template<typename Callable>
  41 | +    void ForEachWallet(Callable&& func)
```
promag commented at 2:27 PM on March 6, 2018:

Why not just return a copy of the array?

jonasschnelli commented at 8:39 AM on March 18, 2018:

I think a lambda is more flexible... same reason we use it in CNode/g_conman.

promag commented at 10:15 PM on March 26, 2018:

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.

jonasschnelli commented at 12:08 PM on March 27, 2018:

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.
promag commented at 2:28 PM on March 6, 2018: member

In #11402 I've changed to shared pointer to manage the lifecycle of wallets. Maybe that's out of scope?
jonasschnelli force-pushed on Mar 18, 2018
jonasschnelli commented at 3:06 AM on March 19, 2018: contributor

Rebased. Fixed points reported by @promag and the travis issue.
promag commented at 2:25 PM on March 26, 2018: member

Needs rebase.
Hide vpwallets behind wallet/init.cpp 373b681056
Move multiwallet handling to walletmanager.cpp/.h d5b9d9a964
Classify the wallet manager ed16e969ce
Revert CWalletRef
Prepare for a better pointer handling
d7523a9c18
jonasschnelli force-pushed on Mar 28, 2018
jonasschnelli commented at 5:28 AM on March 28, 2018: contributor

Rebased.
jonasschnelli commented at 2:39 PM on April 8, 2018: contributor

I guess this makes no longer sense after #10762. Closing.
jonasschnelli closed this on Apr 8, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

jonasschnelli

promag

Labels

Refactoring Wallet

Linked (view graph)

#13034 Introduce WalletManager #14870 Bitcoind crashes with Too Many Files error