Use shared pointer for wallet instances #11402

pull promag wants to merge 4 commits into bitcoin:master from promag:2017-09-wallet-shared-pointer changing 12 files +243 −201

promag commented at 11:05 PM on September 25, 2017: member
This is a small step towards better multi wallet management. Starts by removing the global vpwallets and adding an interface to add/remove/retrieve wallets.

The way it is now it is possible to have, for instance, listunspent RPC and in parallel unload the wallet (once #10740 is merged) without blocking. Once the RPC finishes, the shared pointer will release the wallet.

It is also possible to get all existing wallets without blocking because the caller keeps a local list of shared pointers.

I would like to include either here or in follow ups:
- kind of WalletManager class;
- keep a weak pointer so when the app terminates it is possible to gracefully unload the wallet;
- make the new interface thread safe.
Please consider this RFC.

This is related to #10615, #11383 and #10740.
fanquake added the label Wallet on Sep 25, 2017
[wallet] Use shared pointer for wallet instances 3191f84214
promag force-pushed on Sep 25, 2017

in src/wallet/wallet.cpp:40 in 3191f84214 outdated

  36 | @@ -37,7 +37,36 @@
  37 |  #include <boost/algorithm/string/replace.hpp>
  38 |  #include <boost/thread.hpp>
  39 |  
  40 | -std::vector<CWalletRef> vpwallets;
  41 | +static std::vector<std::shared_ptr<CWallet>> vpwallets;

luke-jr commented at 11:20 PM on September 25, 2017:

Should typedef std::shared_ptr<CWallet> CWalletRef

in src/wallet/wallet.cpp:42 in 3191f84214 outdated

  36 | @@ -37,7 +37,36 @@
  37 |  #include <boost/algorithm/string/replace.hpp>
  38 |  #include <boost/thread.hpp>
  39 |  
  40 | -std::vector<CWalletRef> vpwallets;
  41 | +static std::vector<std::shared_ptr<CWallet>> vpwallets;
  42 | +
  43 | +bool AddWallet(std::shared_ptr<CWallet> pwallet)

luke-jr commented at 11:21 PM on September 25, 2017:

These all need locking.

promag commented at 11:25 PM on September 25, 2017:

At the moment no because wallets are added at startup. This is mentioned in the PR description:

make the new interface thread safe.

luke-jr changes_requested
Squash me [wallet] Prevent adding the same wallet 7ff54b680e
[wallet] Make multi-wallet management thread safe 1a5bd9c762
Squash me [wallet] Use CWalletRef type 85b2b4f91d
promag force-pushed on Sep 26, 2017
fanquake added this to the "In progress" column in a project

TheBlueMatt commented at 5:18 PM on November 9, 2017: contributor

Needs rebase. Maybe @jnewbery wants to give this a concept review in how it interacts (or doesnt) with #10740?
promag commented at 3:25 AM on December 14, 2017: member

@jnewbery ping.
jnewbery commented at 2:09 PM on March 27, 2018: member

What's the status of this PR now that #12587 is open?
promag commented at 2:19 PM on March 27, 2018: member

I think both are valid. This PR changes the ownership of the wallets. For example this PR allows:

listunspent RPC and in parallel unload the wallet (once #10740 is merged) without blocking. Once the RPC finishes, the shared pointer will release the wallet.

I'm happy to update this once there is more positive feedback.
promag renamed this:
~~[wallet] Use shared pointer for wallet instances~~
Use shared pointer for wallet instances
on Apr 7, 2018
jnewbery cross-referenced this on Apr 10, 2018 from issue [wallet] `loadwallet` RPC - load wallet at runtime by jnewbery
Empact cross-referenced this on Apr 18, 2018 from issue Add wallets management functions by promag
promag commented at 1:17 PM on April 18, 2018: member

This PR includes some changes that are now in #13017. Please review that first.
promag cross-referenced this on Apr 20, 2018 from issue Introduce WalletManager by promag
promag cross-referenced this on Apr 23, 2018 from issue Use shared pointer to retain wallet instance by promag
promag closed this on Apr 23, 2018
promag deleted the branch on Apr 23, 2018
fanquake removed this from the "In progress" column in a project

laanwj referenced this in commit 6378eef18f on May 24, 2018
xdustinface referenced this in commit 61c36b5b60 on Mar 31, 2021
xdustinface referenced this in commit 72349ea80c on Mar 31, 2021
xdustinface referenced this in commit 5f44ba0c03 on Mar 31, 2021
xdustinface referenced this in commit c376b64a4f on Apr 1, 2021
PastaPastaPasta referenced this in commit a3cc51118e on Apr 1, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10615 RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet #10740 [wallet] `loadwallet` RPC - load wallet at runtime #11383 Basic Multiwallet GUI support #12587 Introduce g_wallet_manager, prepare for better dynamic wallet loading/unloading #13017 Add wallets management functions #13034 Introduce WalletManager #13063 Use shared pointer to retain wallet instance