This is a small step towards dynamic wallet load/unload. The wallets registry vpwallets is used in several places. With these new functions all vpwallets usage are removed and vpwallets is now a static variable (no external linkage).
The typedef CWalletRef is also removed as it is narrowly used.
35 #include <boost/algorithm/string/replace.hpp>
36
37-std::vector<CWalletRef> vpwallets;
38+static std::vector<CWallet*> vpwallets;
39+
40+void AddWallet(CWallet* wallet)
GetWallets returns no nullptrs? Could check here for that.
AddWallet and RemoveWallet. Also added a check to prevent adding the same wallet (RemoveWallet already checks if wallet exists).
vpwallets centralised helps in adding concurrency, signals/notifications and eventually switching to shared pointers. I’ve also kept these as global functions as I’m not sure if these can belong to WalletInitInterface (imo no).
@Empact thanks for the review. I’m not saying we should use auto (it’s not used at the moment). I’m not sure if consensus is to use CWalletRef instead of std::shared_ptr<CWallet> (when that happens). I can remove that commit if it’s controversial.
31@@ -32,8 +32,11 @@
32 #include <utility>
33 #include <vector>
34
35-typedef CWallet* CWalletRef;
36-extern std::vector<CWalletRef> vpwallets;
37+void AddWallet(CWallet* wallet);
38+void RemoveWallet(CWallet* wallet);
39+bool HasWallets();
HasWallet(), since it returns true if there’s a single wallet
unsigned int CountWallets() is preferable?
HasWallets() is fine. I was just pointing out that the name could be a little bit more precise :slightly_smiling_face:
Tested ACK 689de7c08605643ebe48096c59e6810fbe3dc605.
Nice cleanup, one comment inline. Please squash final commit.
52- }
53- }
54- throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
55+ CWallet* pwallet = GetWallet(requestedWallet);
56+ if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
57+ return pwallet;
With these new functions all vpwallets usage are removed
and vpwallets is now a static variable (no external linkage).
Latests changes are:
auto usagebool to AddWallet and RemoveWallet.
39+
40+bool AddWallet(CWallet* wallet)
41+{
42+ assert(wallet);
43+ std::vector<CWallet*>::const_iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet);
44+ if (i != vpwallets.end()) return false;
μNit: Maybe you could add an
0inline bool HasWallet(CWallet* wallet)
1{
2 assert(wallet);
3 return std::find(vpwallets.begin(), vpwallets.end(), wallet) != vpwallets.end();
4}
and use that here and below (also removing the assert in both places since it’s done inline here).
utACK 3c058fdcc8a71d17296973cb7f09e44a310df22e
Nice clean-up. Agree about removing CWalletRef as it is mostly unused (and ambiguous – I assumed it was a std::shared_ptr<CWallet> like CTransactionRef).
270@@ -268,7 +271,7 @@ class CMerkleTx
271 //Get the marginal bytes of spending the specified output
272 int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet);
273
274-/**
ACK on adding wallet management functions.
The typedef CWalletRef is also removed as it is narrowly used.
If we’re doing shared ptrs alal #11402, CWalletRef would be preferable to auto IMO. #11402 (comment)
I think this part still needs discussion. CWalletRef was introduced very recently as a generalized handle for wallets, to be able to turn it into some kind of smart pointer later. Hardcoding CWallet* complicates that.
I think this part still needs discussion. CWalletRef was introduced very recently
Introduced in #8694, ping @luke-jr.
Hardcoding CWallet* complicates that.
Switching to shared pointers is not a matter of just changing the typedef.
Anyway, if the commit is controversial I prefer to remove it, request re-ACK and defer this discussion, it’s really out of scope here.
CWalletRef shouldn’t be removed. It was specifically added to eventually become a shared_ptr when we have dynamic wallet loading/unloading.
This PR doesn’t make changing to shared pointers any more or less difficult.
#10740 builds on this. Like @MarcoFalke says, this is only used in a very few places, so let’s not hold up this PR for this.
it’s really out of scope here.
Yes, that’s how I felt about that commit in the first place. Whether to have CWalletRef or not is not part of “Add wallets management functions”. Anyhow, I’ll merge this is if there is broad agreement.