Add wallets management functions #13017

promag · 2018-04-18T12:38:32Z

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.

maflcko · 2018-04-18T12:49:27Z

Make sure to align this with the discussion in #10740 (comment)

Empact · 2018-04-18T12:53:21Z

src/wallet/wallet.cpp

-std::vector<CWalletRef> vpwallets;
+static std::vector<CWallet*> vpwallets;
+
+void AddWallet(CWallet* wallet)


Do we want to provide guarantees that GetWallets returns no nullptrs? Could check here for that.

Added assertions in AddWallet and RemoveWallet. Also added a check to prevent adding the same wallet (RemoveWallet already checks if wallet exists).

Empact · 2018-04-18T12:59:03Z

If we're doing shared ptrs alal #11402, CWalletRef would be preferable to auto IMO. #11402 (comment)

promag · 2018-04-18T13:10:11Z

@MarcoFalke afaiu yes. Having 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.

jnewbery

Tested ACK 689de7c08605643ebe48096c59e6810fbe3dc605.

Nice cleanup, one comment inline. Please squash final commit.

jnewbery · 2018-04-18T14:55:53Z

src/wallet/wallet.h

-extern std::vector<CWalletRef> vpwallets;
+void AddWallet(CWallet* wallet);
+void RemoveWallet(CWallet* wallet);
+bool HasWallets();


This might be more correctly named HasWallet(), since it returns true if there's a single wallet

Maybe unsigned int CountWallets() is preferable?

no need to overcomplicate it. HasWallets() is fine. I was just pointing out that the name could be a little bit more precise 🙂

promag · 2018-04-18T20:24:56Z

@jnewbery squashed.

promag · 2018-04-18T20:27:32Z

src/wallet/rpcwallet.cpp

-        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
+        CWallet* pwallet = GetWallet(requestedWallet);
+        if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
+        return pwallet;


Note to reviewers, I've switched these instructions. Inverted condition to throw the error.

Empact · 2018-04-18T20:39:17Z

Could you clang-format? Some whitespace inconsistencies.

promag · 2018-04-18T21:03:00Z

@Empact done, sorry.

With these new functions all vpwallets usage are removed and vpwallets is now a static variable (no external linkage).

promag · 2018-04-18T21:13:29Z

Latests changes are:

fix include order and whitespace
removed auto usage
added return bool to AddWallet and RemoveWallet.

jnewbery · 2018-04-18T21:22:04Z

ACK 3c058fd.

Empact · 2018-04-18T22:06:12Z

utACK 3c058fd

kallewoof

utACK 3c058fd

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).

kallewoof · 2018-04-19T01:06:04Z

src/wallet/wallet.cpp

+{
+    assert(wallet);
+    std::vector<CWallet*>::const_iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet);
+    if (i != vpwallets.end()) return false;


μNit: Maybe you could add an

inline bool HasWallet(CWallet* wallet) { assert(wallet); return std::find(vpwallets.begin(), vpwallets.end(), wallet) != vpwallets.end(); }

and use that here and below (also removing the assert in both places since it's done inline here).

Your suggestion "conflicts" with adding a critical section here (which will come next). If I do as you say then the lock will be recursive, something we want to avoid AFAICK. So I'd avoid calls between these functions.

That's a pity, but no big deal.

promag · 2018-04-19T01:15:01Z

Thanks for the review @kallewoof. Like @MarcoFalke said, this is a small piece of a large feature and I'd like to keep the PR scope on focus.

promag · 2018-04-19T13:51:38Z

src/wallet/wallet.h

@@ -268,7 +271,7 @@ class CMerkleTx
 //Get the marginal bytes of spending the specified output
 int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet);

-/** 


I can remove these whitespace change if that is controversial.

no - please don't invalidate reviewer ACKs unnecessarily

e2f58f4 wallet: Make vpwallets usage thread safe (João Barbosa) Pull request description: This PR turns the functions introduced in #13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in #10740. Tree-SHA512: efaa09e501636cf957aa33de83719ce09dc0c2a19daff741a94ef10d6b7ba5dee538355b80c96ead995140f99f5df0c92fb0e22ae1adb8f397eb478280c8d8c7

e2f58f4 wallet: Make vpwallets usage thread safe (João Barbosa) Pull request description: This PR turns the functions introduced in bitcoin#13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in bitcoin#10740. Tree-SHA512: efaa09e501636cf957aa33de83719ce09dc0c2a19daff741a94ef10d6b7ba5dee538355b80c96ead995140f99f5df0c92fb0e22ae1adb8f397eb478280c8d8c7