New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use shared pointer to retain wallet instance #13063
Conversation
647a963
to
8bb5467
Compare
needs rebase |
8bb5467
to
58962fd
Compare
Rebased. |
I like the idea of |
Concept ACK. |
@jonasschnelli what you mean by outside? There is always a |
@promag: Yes. Your right. |
58962fd
to
0a08100
Compare
4993bf9
to
25c5c3f
Compare
Rebased on master. |
25c5c3f
to
1279e2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 1279e2bdc1493e1d7b53ab43d09d16ac0926eb65
@@ -453,11 +453,12 @@ class WalletImpl : public Wallet | |||
return MakeHandler(m_wallet.NotifyWatchonlyChanged.connect(fn)); | |||
} | |||
|
|||
std::shared_ptr<CWallet> m_shared_wallet; | |||
CWallet& m_wallet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to remove m_wallet
and just keep m_shared_wallet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure, but the diff will grow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be done in a follow-up PR.
@@ -134,7 +134,9 @@ static std::string LabelFromValue(const UniValue& value) | |||
|
|||
static UniValue getnewaddress(const JSONRPCRequest& request) | |||
{ | |||
CWallet * const pwallet = GetWalletForJSONRPCRequest(request); | |||
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request); | |||
CWallet* const pwallet = wallet.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. Can be done in a follow-up PR.
@@ -259,7 +259,7 @@ bool WalletInit::Open() const | |||
} | |||
|
|||
for (const std::string& walletFile : gArgs.GetArgs("-wallet")) { | |||
CWallet * const pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir())); | |||
std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Can you update the PR description with the motivation for this (possibly just copy from #11402)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -20,7 +20,7 @@ void RegisterWalletRPCCommands(CRPCTable &t); | |||
* @param[in] request JSONRPCRequest that wishes to access a wallet | |||
* @return nullptr if no wallet should be used, or a pointer to the CWallet | |||
*/ | |||
CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request); | |||
std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should just return a raw pointer or CWallet&.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RPC handler must hold the wallet to prevent concurrent unload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Needs rebase |
1279e2b
to
ab74564
Compare
@jimpo without shared_ptr the caller of With |
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
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
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
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
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
) 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 <laanwj@gmail.com>
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
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
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
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
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
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
Currently there are 3 places where it makes sense to retain a wallet shared pointer:
vpwallets
;interfaces::Wallet
interface instance - used by the UI;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.