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

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

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

 //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

3c058fd wallet: Add HasWallets (João Barbosa) 373aee2 wallet: Add AddWallet, RemoveWallet, GetWallet and GetWallets (João Barbosa) 6efd964 refactor: Drop CWalletRef typedef (João Barbosa) Pull request description: 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. Tree-SHA512: 2ea19da2e17b521ad678bfe10f3257e497ccaf7ab9fd0b6647f9d829f1d6131cfa68db8e8492421711c6da399859432b963a568bdd4ca40a77dd95b597839423

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

fanquake added Refactoring Wallet labels Apr 18, 2018

refactor: Drop CWalletRef typedef

6efd964

promag force-pushed the 2018-04-vpwallets branch from cb09fd3 to 8df99f9 Compare April 18, 2018 12:41

promag changed the title ~~Add AddWallet, RemoveWallet, GetWallet and GetWallets~~ Add wallet management functions Apr 18, 2018

promag force-pushed the 2018-04-vpwallets branch from b1fdc87 to 4187365 Compare April 18, 2018 12:48

promag changed the title ~~Add wallet management functions~~ Add wallets management functions Apr 18, 2018

Empact reviewed Apr 18, 2018

View reviewed changes

promag mentioned this pull request Apr 18, 2018

Use shared pointer for wallet instances #11402

Closed

jnewbery reviewed Apr 18, 2018

View reviewed changes

promag force-pushed the 2018-04-vpwallets branch from 689de7c to ac0de44 Compare April 18, 2018 17:41

promag commented Apr 18, 2018

View reviewed changes

promag force-pushed the 2018-04-vpwallets branch from ac0de44 to a2ea288 Compare April 18, 2018 21:02

jnewbery mentioned this pull request Apr 18, 2018

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

Merged

promag added 2 commits April 18, 2018 22:07

wallet: Add AddWallet, RemoveWallet, GetWallet and GetWallets

373aee2

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

wallet: Add HasWallets

3c058fd

promag force-pushed the 2018-04-vpwallets branch from a2ea288 to 3c058fd Compare April 18, 2018 21:08

kallewoof approved these changes Apr 19, 2018

View reviewed changes

promag commented Apr 19, 2018

View reviewed changes

promag mentioned this pull request Apr 19, 2018

Make vpwallets usage thread safe #13028

Merged

jnewbery mentioned this pull request Apr 23, 2018

Dynamic wallet load / create / unload #13059

Closed

10 tasks

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add wallets management functions #13017

Add wallets management functions #13017

Uh oh!

Conversation

promag commented Apr 18, 2018

Uh oh!

maflcko commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Apr 18, 2018

Uh oh!

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

Uh oh!

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 Apr 18, 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!

promag commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

promag commented Apr 18, 2018

Uh oh!

promag commented Apr 18, 2018

Uh oh!

jnewbery commented Apr 18, 2018

Uh oh!

Empact commented Apr 18, 2018

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Apr 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!

promag commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

promag commented Apr 18, 2018 •

edited

Loading

promag Apr 18, 2018 •

edited

Loading

Empact commented Apr 18, 2018 •

edited

Loading

promag Apr 19, 2018 •

edited

Loading