Prevent wallet unload on GetWalletForJSONRPCRequest #24678

pull promag wants to merge 1 commits into bitcoin:master from promag:2022-03-getwallets changing 3 files +12 −5

promag commented at 10:49 PM on March 25, 2022: member

Don't extend shared ownership of all wallets to GetWalletForJSONRPCRequest scope.

wallet: Prevent wallet unload on GetWalletForJSONRPCRequest

Don't extend shared ownership of all wallets to GetWalletForJSONRPCRequest scope.

f59959e381

DrahtBot added the label RPC/REST/ZMQ on Mar 25, 2022
DrahtBot added the label Wallet on Mar 25, 2022
shaavan approved
shaavan commented at 8:04 AM on March 26, 2022: contributor
Code Review ACK f59959e3818692c5b3c2dfa51c14e515085e940f

The reasoning for this change, as far as I understood this PR, is this:
- In the GetWalletForJSONRPCRequest function, 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.
promag cross-referenced this on Mar 26, 2022 from issue Add and use ForEachWallet by promag
achow101 commented at 5:41 PM on April 19, 2022: member

What do you mean by "unload" in this context?
promag commented at 5:43 PM on April 19, 2022: member

The actual wallet unload, which occurs on the last shared pointer.
achow101 commented at 5:54 PM on April 19, 2022: member

Oh, I see. Is there a scenario where this could actually happen, and if so, can we test for it?
promag commented at 6:37 PM on April 19, 2022: member

Could happen when an explicit unload races with other call of any wallet. I don't think we can test without code changes.
achow101 commented at 6:56 PM on April 19, 2022: member

ACK f59959e3818692c5b3c2dfa51c14e515085e940f
theStack approved
theStack commented at 12:06 PM on August 17, 2022: contributor

Concept and code-review ACK f59959e3818692c5b3c2dfa51c14e515085e940f
fanquake merged this on Aug 17, 2022
fanquake closed this on Aug 17, 2022
sidhujag referenced this in commit 016e6cc96f on Aug 17, 2022
promag deleted the branch on Aug 17, 2022
bitcoin locked this on Aug 17, 2023

Contributors

Labels

Linked (view graph)

#24683 Add and use ForEachWallet