Prevent wallet unload on GetWalletForJSONRPCRequest#24678
Merged
fanquake merged 1 commit intobitcoin:masterfrom Aug 17, 2022
Merged
Prevent wallet unload on GetWalletForJSONRPCRequest#24678fanquake merged 1 commit intobitcoin:masterfrom
fanquake merged 1 commit intobitcoin:masterfrom
Conversation
Don't extend shared ownership of all wallets to GetWalletForJSONRPCRequest scope.
shaavan
approved these changes
Mar 26, 2022
Contributor
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
Code Review ACK f59959e
The reasoning for this change, as far as I understood this PR, is this:
- In the
GetWalletForJSONRPCRequestfunction, we only needed the wallet shared pointer if the number of context wallets equals one. In other cases, we would like to return an error. - So for this, we would not like this function to have information about all the wallets corresponding to context.
- That is why this PR suggests moving this process of unloading all wallets and returning a single one to a function in the wallet.cpp file.
I have checked that the changed code is clean and clear to reason with. And if my original understanding of this PR is correct, I think this PR can be merged. And in case my analysis was erroneous, please do correct me.
Member
|
What do you mean by "unload" in this context? |
Contributor
Author
|
The actual wallet unload, which occurs on the last shared pointer. |
Member
|
Oh, I see. Is there a scenario where this could actually happen, and if so, can we test for it? |
Contributor
Author
|
Could happen when an explicit unload races with other call of any wallet. I don't think we can test without code changes. |
Member
|
ACK f59959e |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 17, 2022
f59959e wallet: Prevent wallet unload on GetWalletForJSONRPCRequest (João Barbosa) Pull request description: Don't extend shared ownership of all wallets to `GetWalletForJSONRPCRequest` scope. ACKs for top commit: achow101: ACK f59959e shaavan: Code Review ACK f59959e theStack: Concept and code-review ACK f59959e Tree-SHA512: 7c0294098b5c32acaab8cc6fcf17a581d580ad1a557ba0602a9506074ac035815739afb4a25b3e61be9132535c7fc3ec7ef5137c1dfc9d4078f13663d508ef55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Don't extend shared ownership of all wallets to
GetWalletForJSONRPCRequestscope.