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

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

2. jonasschnelli added the label Wallet on Mar 3, 2018
3. jonasschnelli added the label Refactoring on Mar 3, 2018
4. jonasschnelli force-pushed on Mar 3, 2018
5. jonasschnelli force-pushed on Mar 3, 2018
6. jonasschnelli force-pushed on Mar 6, 2018
7. 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.
8. 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;
9. 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.
10. 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.
11. 
    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?
12. jonasschnelli force-pushed on Mar 18, 2018
13. 
    jonasschnelli commented at 3:06 am on March 19, 2018: contributor

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

    Needs rebase.
15. Hide vpwallets behind wallet/init.cpp 373b681056
16. Move multiwallet handling to walletmanager.cpp/.h d5b9d9a964
17. Classify the wallet manager ed16e969ce
18. Revert CWalletRef

    Prepare for a better pointer handling

    d7523a9c18
19. jonasschnelli force-pushed on Mar 28, 2018
20. 
    jonasschnelli commented at 5:28 am on March 28, 2018: contributor

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

    I guess this makes no longer sense after #10762. Closing.
22. jonasschnelli closed this on Apr 8, 2018

    ---

23. DrahtBot locked this on Sep 8, 2021

Contributors
jonasschnelli promag

Labels
Refactoring Wallet