Use shared pointer to retain wallet instance

promag commented at 11:11 pm on April 23, 2018: member

Currently there are 3 places where it makes sense to retain a wallet shared pointer:

vpwallets;
interfaces::Wallet interface instance - used by the UI;
wallet RPC functions - given by GetWalletForJSONRPCRequest.

The way it is now it is possible to have, for instance, listunspent RPC and in parallel unload the wallet (once #13111 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.

This is mostly relevant for wallet unloading.

This PR replaces #11402.

promag force-pushed on Apr 23, 2018

fanquake added the label Wallet on Apr 24, 2018

jnewbery commented at 3:59 pm on April 24, 2018: member

needs rebase

promag force-pushed on Apr 24, 2018

promag commented at 4:30 pm on April 24, 2018: member

Rebased.

Empact commented at 9:19 pm on April 25, 2018: member

I like the idea of CWalletRef here. std::shared_ptr<CWallet> buries the lede a bit.

jonasschnelli commented at 7:56 am on April 26, 2018: contributor

Concept ACK. A bit unfortunate that we have a lot of CWallet* const pwallet = wallet.get(); calls outside of any lock which AFAIK partially defeats the improvements the shared pointer adds.

promag commented at 9:04 am on April 26, 2018: member

A bit unfortunate that we have a lot of CWallet* const pwallet = wallet.get(); calls outside of any lock @jonasschnelli what you mean by outside? There is always a std::shared_ptr<CWallet> in scope when .get() pointer is used. The only exception is RegisterValidationInterface(walletInstance.get());.

jonasschnelli commented at 9:05 am on April 26, 2018: contributor

@promag: Yes. Your right.

promag force-pushed on Apr 29, 2018

jnewbery added this to the "In progress" column in a project

promag force-pushed on May 7, 2018

promag commented at 8:39 pm on May 7, 2018: member

Rebased on master.

promag force-pushed on May 8, 2018

in src/interfaces/wallet.cpp:457 in 1279e2bdc1 outdated

452@@ -453,11 +453,12 @@ class WalletImpl : public Wallet
453         return MakeHandler(m_wallet.NotifyWatchonlyChanged.connect(fn));
454     }
455 
456+    std::shared_ptr<CWallet> m_shared_wallet;
457     CWallet& m_wallet;

jnewbery commented at 2:08 pm on May 8, 2018:

Is it possible to remove m_wallet and just keep m_shared_wallet?

promag commented at 2:23 pm on May 22, 2018:

Yes, sure, but the diff will grow.

jnewbery commented at 3:12 pm on May 22, 2018:

Can be done in a follow-up PR.

in src/wallet/rpcwallet.cpp:138 in 1279e2bdc1 outdated

133@@ -134,7 +134,9 @@ static std::string LabelFromValue(const UniValue& value)
134 
135 static UniValue getnewaddress(const JSONRPCRequest& request)
136 {
137-    CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
138+    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
139+    CWallet* const pwallet = wallet.get();

jnewbery commented at 2:09 pm on May 8, 2018:

Can we update EnsureWalletIsAvailable() to take a std::shared_ptr<CWallet>, and then remove the need to create a CWallet* const in all of these rpc methods?

promag commented at 2:22 pm on May 22, 2018:

The problem is that there are a lot of other calls that receive CWallet* and the diff started to be big. I can do that in a separate branch so you can see, WDYT?

jnewbery commented at 3:12 pm on May 22, 2018:

Definitely. Can be done in a follow-up PR.

in src/wallet/init.cpp:229 in 1279e2bdc1 outdated

258@@ -259,7 +259,7 @@ bool WalletInit::Open() const
259     }
260 
261     for (const std::string& walletFile : gArgs.GetArgs("-wallet")) {
262-        CWallet * const pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir()));
263+        std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir()));

jnewbery commented at 2:21 pm on May 8, 2018:

nit: it’d be nice to rename these variables to wallet (yes, I know these are (smart) pointers, but pwallet everywhere else means raw pointers)

jnewbery commented at 2:22 pm on May 8, 2018: member

utACK 1279e2bdc1493e1d7b53ab43d09d16ac0926eb65

jnewbery commented at 2:23 pm on May 8, 2018: member

Can you update the PR description with the motivation for this (possibly just copy from #11402)?

jonasschnelli approved

jonasschnelli commented at 6:54 am on May 9, 2018: contributor

utACK 1279e2bdc1493e1d7b53ab43d09d16ac0926eb65

I agree with @jnewbery that removing m_wallet in interface/wallet.cpp would be good, though I think this can also be done in another pull.

laanwj added this to the "Blockers" column in a project

in src/wallet/rpcwallet.h:23 in 1279e2bdc1 outdated

19@@ -20,7 +20,7 @@ void RegisterWalletRPCCommands(CRPCTable &t);
20  * @param[in] request JSONRPCRequest that wishes to access a wallet
21  * @return nullptr if no wallet should be used, or a pointer to the CWallet
22  */
23-CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
24+std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);

jimpo commented at 0:01 am on May 19, 2018:

I think this should just return a raw pointer or CWallet&.

promag commented at 2:18 pm on May 22, 2018:

The RPC handler must hold the wallet to prevent concurrent unload.

jimpo commented at 0:08 am on May 19, 2018: contributor

I feel like this can all be done with unique_ptrs and references, which is preferable. Basically,

0std::vector<std::unique_ptr<CWallet>> vpwallets;  // Has owning references to all the wallets
1
2CWallet& AddWallet(std::unique_ptr<CWallet> wallet);  // Adds and gives ownership to vpwallets
3std::unique_ptr<CWallet> RemoveWallet(const CWallet& wallet);  // Removes and releases ownership from vpwallets
4
5std::vector<CWallet&> GetWallets();
6CWallet& GetWallet(const std::string& name);

Any particular reason they need to be shared?

danielsam approved

danielsam commented at 6:48 pm on May 21, 2018: none

Looks good

MarcoFalke commented at 7:26 pm on May 21, 2018: member

Needs rebase

promag force-pushed on May 22, 2018

promag commented at 2:14 pm on May 22, 2018: member

0std::unique_ptr<CWallet> RemoveWallet(const CWallet& wallet);  // Removes and releases ownership from vpwallets

@jimpo without shared_ptr the caller of RemoveWallet must ensure no one is using the wallet before releasing it. This may require synchronization between multiple threads, including UI.

With shared_ptr, the last pointer can safely release the wallet.

promag commented at 2:17 pm on May 22, 2018: member

@jnewbery improved PR description.

promag force-pushed on May 22, 2018

promag commented at 2:28 pm on May 22, 2018: member

Rebased due to merge of #12560 and #13059.

promag force-pushed on May 22, 2018

jnewbery commented at 3:49 pm on May 22, 2018: member

Fails build:

0CXX      wallet/libbitcoin_wallet_a-wallet.o
1wallet/wallet.cpp: In member function ‘DBErrors CWallet::LoadWallet(bool&)’:
2wallet/wallet.cpp:3204:32: error: no match for call to ‘(boost::signals2::signal<void(std::shared_ptr<CWallet>)>) (CWallet* const)’
3     uiInterface.LoadWallet(this);
4                                ^

wallet: Use shared pointer to retain wallet instance 80b4910f7d

promag force-pushed on May 22, 2018

promag commented at 4:19 pm on May 22, 2018: member

Fixed @jnewbery.

in src/wallet/wallet.cpp:4342 in 80b4910f7d

4338@@ -4342,7 +4339,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
4339     }
4340 
4341     // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.
4342-    RegisterValidationInterface(temp_wallet.release());
4343+    RegisterValidationInterface(walletInstance.get());

jimpo commented at 4:49 pm on May 22, 2018:

When does this get unregistered? Seems like the validation interface queue may hold a pointer after the wallet is released.

promag commented at 5:41 pm on May 22, 2018:

When UnregisterAllValidationInterfaces is called. Obviously unloadwallet RPC or the Unload wallet action in the UI will have to unregister.

jimpo commented at 4:50 pm on May 22, 2018: contributor

Ah, yes, I see the issue with unique_ptr.

jnewbery commented at 6:14 pm on May 22, 2018: member

utACK 80b4910f7d87983f50047074c3c2397b0a5c4e92

jimpo commented at 6:56 pm on May 22, 2018: contributor

utACK 80b4910

laanwj merged this on May 24, 2018

laanwj closed this on May 24, 2018

laanwj referenced this in commit 6378eef18f on May 24, 2018

MarcoFalke removed this from the "Blockers" column in a project

fanquake moved this from the "In progress" to the "Done" column in a project

MarcoFalke referenced this in commit 6579d80572 on Jun 20, 2018

promag deleted the branch on Apr 11, 2020

PastaPastaPasta referenced this in commit 5d483106f8 on Dec 16, 2020

PastaPastaPasta referenced this in commit ce5b06ed66 on Dec 18, 2020

MarcoFalke referenced this in commit eea6196c3d on Mar 10, 2021

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

xdustinface referenced this in commit a34ac73dd2 on Apr 4, 2021

xdustinface referenced this in commit af40021c9f on Apr 4, 2021

xdustinface referenced this in commit 713e241b6f on Apr 4, 2021

xdustinface referenced this in commit d84ee328ff on Apr 4, 2021

xdustinface referenced this in commit 773e39e1e5 on Apr 5, 2021

xdustinface referenced this in commit a8454e22b0 on Apr 13, 2021

DrahtBot locked this on Feb 15, 2022

Use shared pointer to retain wallet instance #13063