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
Add wallets management functions #13017
Conversation
cb09fd3
to
8df99f9
Compare
b1fdc87
to
4187365
Compare
Make sure to align this with the discussion in #10740 (comment) |
src/wallet/wallet.cpp
Outdated
std::vector<CWalletRef> vpwallets; | ||
static std::vector<CWallet*> vpwallets; | ||
|
||
void AddWallet(CWallet* 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.
Do we want to provide guarantees that GetWallets
returns no nullptrs
? Could check here for that.
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.
Added assertions in AddWallet
and RemoveWallet
. Also added a check to prevent adding the same wallet (RemoveWallet
already checks if wallet exists).
If we're doing shared ptrs alal #11402, |
@MarcoFalke afaiu yes. Having @Empact thanks for the review. I'm not saying we should use |
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.
Tested ACK 689de7c08605643ebe48096c59e6810fbe3dc605.
Nice cleanup, one comment inline. Please squash final commit.
src/wallet/wallet.h
Outdated
extern std::vector<CWalletRef> vpwallets; | ||
void AddWallet(CWallet* wallet); | ||
void RemoveWallet(CWallet* wallet); | ||
bool HasWallets(); |
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.
This might be more correctly named HasWallet()
, since it returns true if there's a single 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.
Maybe unsigned int CountWallets()
is preferable?
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.
no need to overcomplicate it. HasWallets()
is fine. I was just pointing out that the name could be a little bit more precise 🙂
689de7c
to
ac0de44
Compare
@jnewbery squashed. |
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; |
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.
Note to reviewers, I've switched these instructions. Inverted condition to throw the error.
Could you clang-format? Some whitespace inconsistencies. |
ac0de44
to
a2ea288
Compare
@Empact done, sorry. |
With these new functions all vpwallets usage are removed and vpwallets is now a static variable (no external linkage).
a2ea288
to
3c058fd
Compare
Latests changes are:
|
ACK 3c058fd. |
utACK 3c058fd |
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 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
).
{ | ||
assert(wallet); | ||
std::vector<CWallet*>::const_iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet); | ||
if (i != vpwallets.end()) return false; |
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: 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).
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.
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.
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.
That's a pity, but no big deal.
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. |
@@ -268,7 +271,7 @@ class CMerkleTx | |||
//Get the marginal bytes of spending the specified output | |||
int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet); | |||
|
|||
/** |
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 can remove these whitespace change if that is controversial.
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.
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
Signed-off-by: Pasta <pasta@dashboost.org>
…et and GetWallets With these new functions all vpwallets usage are removed and vpwallets is now a static variable (no external linkage). Signed-off-by: Pasta <pasta@dashboost.org>
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
Signed-off-by: Pasta <pasta@dashboost.org>
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
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
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
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
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
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
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
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
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
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
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
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
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
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
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
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
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
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
This is a small step towards dynamic wallet load/unload. The wallets registry
vpwallets
is used in several places. With these new functions allvpwallets
usage are removed andvpwallets
is now a static variable (no external linkage).The typedef
CWalletRef
is also removed as it is narrowly used.