Use shared pointer to retain wallet instance #13063

promag · 2018-04-23T23:11:45Z

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.

jnewbery · 2018-04-24T15:59:43Z

needs rebase

promag · 2018-04-24T16:30:38Z

Rebased.

Empact · 2018-04-25T21:19:09Z

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

jonasschnelli · 2018-04-26T07:56:17Z

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 · 2018-04-26T09:04:25Z

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 · 2018-04-26T09:05:37Z

@promag: Yes. Your right.

promag · 2018-05-07T20:39:58Z

Rebased on master.

jnewbery

utACK 1279e2bdc1493e1d7b53ab43d09d16ac0926eb65

jnewbery · 2018-05-08T14:08:45Z

src/interfaces/wallet.cpp

Is it possible to remove m_wallet and just keep m_shared_wallet?

Yes, sure, but the diff will grow.

Can be done in a follow-up PR.

jnewbery · 2018-05-08T14:09:52Z

src/wallet/rpcwallet.cpp

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?

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?

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

jnewbery · 2018-05-08T14:21:35Z

src/wallet/init.cpp

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 · 2018-05-08T14:23:33Z

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

jonasschnelli

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.

jimpo

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

std::vector<std::unique_ptr<CWallet>> vpwallets;  // Has owning references to all the wallets

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

std::vector<CWallet&> GetWallets();
CWallet& GetWallet(const std::string& name);

Any particular reason they need to be shared?

jimpo · 2018-05-19T00:01:39Z

src/wallet/rpcwallet.h

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

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

danielsam

Looks good

maflcko · 2018-05-21T19:26:27Z

Needs rebase

promag · 2018-05-22T14:14:49Z

std::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 · 2018-05-22T14:17:01Z

@jnewbery improved PR description.

jimpo

Ah, yes, I see the issue with unique_ptr.

jimpo · 2018-05-22T16:49:58Z

src/wallet/wallet.cpp


    // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.
-    RegisterValidationInterface(temp_wallet.release());
+    RegisterValidationInterface(walletInstance.get());


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

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

jnewbery · 2018-05-22T18:14:52Z

utACK 80b4910

jimpo · 2018-05-22T18:56:51Z

utACK 80b4910

80b4910 wallet: Use shared pointer to retain wallet instance (João Barbosa) Pull request description: 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. Tree-SHA512: b7e37c7e1ab56626085afe2d40b1628e8d4f0dbda08df01b7e618ecd2d894ce9b83d4219443f444ba889096286eff002f163cb0a48f37063b62e9ba4ccfa6cce

@ken2812221

3a03d2a Qt: load wallet in UI after possible init aborts (Jonas Schnelli) Pull request description: Bug was introduced in #13063 (80b4910) where #13097 made possible to get "hit" by that bug. Reported by @ken2812221 (#13097 (comment)). Dynamically loading a wallet informs the UI (and therefore makes the instance accessible) about the new wallet before all possible error cases where handled. Easy to reproduce by starting `bitcoin-qt --regtest --nowallet -usehd=0` then in the console enter `loadwallet wallet.dat`. This PR will make sure only correctly initialised (loaded) wallets will appear in the UI. Tree-SHA512: 3139545e852d53b117182b579f45259c198d1c25c1a6fa4e0108f942d45f6fe2691e6bfcbbae2e18c33ad0174a520f379c17867b1eb87f950d830a5f519fec4f

@ken2812221

3a03d2a Qt: load wallet in UI after possible init aborts (Jonas Schnelli) Pull request description: Bug was introduced in bitcoin#13063 (80b4910) where bitcoin#13097 made possible to get "hit" by that bug. Reported by @ken2812221 (bitcoin#13097 (comment)). Dynamically loading a wallet informs the UI (and therefore makes the instance accessible) about the new wallet before all possible error cases where handled. Easy to reproduce by starting `bitcoin-qt --regtest --nowallet -usehd=0` then in the console enter `loadwallet wallet.dat`. This PR will make sure only correctly initialised (loaded) wallets will appear in the UI. Tree-SHA512: 3139545e852d53b117182b579f45259c198d1c25c1a6fa4e0108f942d45f6fe2691e6bfcbbae2e18c33ad0174a520f379c17867b1eb87f950d830a5f519fec4f

…2 rebased) 7c90c67 rpc: refactor rpc wallet functions to take references instead of pointers (fanquake) 4866934 rpc: remove calls to CWallet.get() (fanquake) Pull request description: This is a rebased #18592. > This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here bitcoin/bitcoin#18590 > It seems that this PR is indirectly related to this issue: bitcoin/bitcoin#13063 (comment) > Notice: I have deliberately **not** changed the class `WalletRescanReserver ` whose constructor expects a raw pointer, because it's external and affects other areas, which I didn't touch to avoid making this PR "viral". > Fixes bitcoin/bitcoin#18590 ACKs for top commit: MarcoFalke: ACK 7c90c67 🐧 ryanofsky: Code review ACK 7c90c67. Changes easy to review with `--word-diff-regex=. -U0` Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba

80b4910 wallet: Use shared pointer to retain wallet instance (João Barbosa) Pull request description: 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 bitcoin#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 bitcoin#11402. Tree-SHA512: b7e37c7e1ab56626085afe2d40b1628e8d4f0dbda08df01b7e618ecd2d894ce9b83d4219443f444ba889096286eff002f163cb0a48f37063b62e9ba4ccfa6cce

) 80b4910 wallet: Use shared pointer to retain wallet instance (João Barbosa) Pull request description: 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 bitcoin#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 bitcoin#11402. Tree-SHA512: b7e37c7e1ab56626085afe2d40b1628e8d4f0dbda08df01b7e618ecd2d894ce9b83d4219443f444ba889096286eff002f163cb0a48f37063b62e9ba4ccfa6cce Co-authored-by: Wladimir J. van der Laan <[email protected]>

@ken2812221

3a03d2a Qt: load wallet in UI after possible init aborts (Jonas Schnelli) Pull request description: Bug was introduced in bitcoin#13063 (80b4910) where bitcoin#13097 made possible to get "hit" by that bug. Reported by @ken2812221 (bitcoin#13097 (comment)). Dynamically loading a wallet informs the UI (and therefore makes the instance accessible) about the new wallet before all possible error cases where handled. Easy to reproduce by starting `bitcoin-qt --regtest --nowallet -usehd=0` then in the console enter `loadwallet wallet.dat`. This PR will make sure only correctly initialised (loaded) wallets will appear in the UI. Tree-SHA512: 3139545e852d53b117182b579f45259c198d1c25c1a6fa4e0108f942d45f6fe2691e6bfcbbae2e18c33ad0174a520f379c17867b1eb87f950d830a5f519fec4f

promag force-pushed the 2018-04-wallet-sharedptr branch 2 times, most recently from 647a963 to 8bb5467 Compare April 23, 2018 23:29

fanquake added the Wallet label Apr 24, 2018

This was referenced Apr 24, 2018

wallet: Fix possible memory leak in CreateWalletFromFile. #12647

Closed

Introduce WalletManager #13034

Closed

promag force-pushed the 2018-04-wallet-sharedptr branch from 8bb5467 to 58962fd Compare April 24, 2018 16:30

promag mentioned this pull request Apr 26, 2018

Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort #13076

Merged

promag force-pushed the 2018-04-wallet-sharedptr branch from 58962fd to 0a08100 Compare April 29, 2018 09:34

jnewbery mentioned this pull request Apr 30, 2018

Dynamic wallet load / create / unload #13059

Closed

10 tasks

promag mentioned this pull request May 7, 2018

[wallet] loadwallet RPC - load wallet at runtime #10740

Merged

promag force-pushed the 2018-04-wallet-sharedptr branch 2 times, most recently from 4993bf9 to 25c5c3f Compare May 7, 2018 20:39

promag force-pushed the 2018-04-wallet-sharedptr branch from 25c5c3f to 1279e2b Compare May 8, 2018 06:39

jnewbery reviewed May 8, 2018

View reviewed changes

jonasschnelli approved these changes May 9, 2018

View reviewed changes

jimpo reviewed May 19, 2018

View reviewed changes

danielsam approved these changes May 21, 2018

View reviewed changes

promag force-pushed the 2018-04-wallet-sharedptr branch from 1279e2b to ab74564 Compare May 22, 2018 14:03

promag force-pushed the 2018-04-wallet-sharedptr branch from ab74564 to 6661b1f Compare May 22, 2018 14:26

jimpo reviewed May 22, 2018

View reviewed changes

laanwj merged commit 80b4910 into bitcoin:master May 24, 2018

promag mentioned this pull request May 25, 2018

[wallet] createwallet RPC - create new wallet at runtime #13058

Merged

Bushstar mentioned this pull request May 26, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#336

Merged

jonasschnelli mentioned this pull request Jun 19, 2018

Qt: load wallet in UI after possible init aborts #13506

Merged

ryanofsky mentioned this pull request Jun 26, 2018

Refactor: separate wallet from node #10973

Merged

promag deleted the 2018-04-wallet-sharedptr branch April 11, 2020 00:51

promag mentioned this pull request Apr 11, 2020

rpc: raw pointers should be replaced with shared_ptr #18590

Closed

brakmic mentioned this pull request Apr 11, 2020

rpc: replace raw pointers with shared_ptrs #18592

Closed

UdjinM6 mentioned this pull request Jul 26, 2020

Fix a crash on splash screen when wallet fails to load dashpay/dash#3629

Merged

fanquake mentioned this pull request Mar 2, 2021

rpc: replace wallet raw pointers with references (#18592 rebased) #21331

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Use shared pointer to retain wallet instance #13063

Use shared pointer to retain wallet instance #13063

Uh oh!

Conversation

promag commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 24, 2018

Uh oh!

promag commented Apr 24, 2018

Uh oh!

Empact commented Apr 25, 2018

Uh oh!

jonasschnelli commented Apr 26, 2018

Uh oh!

promag commented Apr 26, 2018

Uh oh!

jonasschnelli commented Apr 26, 2018

Uh oh!

promag commented May 7, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 8, 2018

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

jimpo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo May 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielsam left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 21, 2018

Uh oh!

promag commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 22, 2018

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 22, 2018

Uh oh!

jimpo commented May 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

promag commented Apr 23, 2018 •

edited

Loading

promag May 22, 2018 •

edited

Loading

jimpo left a comment •

edited

Loading

jimpo May 19, 2018 •

edited

Loading

promag commented May 22, 2018 •

edited

Loading