Add and use ForEachWallet #24683

pull promag wants to merge 2 commits into bitcoin:master from promag:2022-03-foreachwallet changing 6 files +41 −24

promag commented at 11:08 AM on March 26, 2022: member

There can be implicit wallet unload on GetWallets call site. With this change concurrent wallet unload is not possible and shared ownership is not extended to GetWallets's caller scope.

wallet: Prevent wallet unload on GetWalletForJSONRPCRequest

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

f59959e381

wallet: Add and use ForEachWallet

There can be implicit wallet unload on GetWallets call site. With this 
change  concurrent wallet unload is not possible and shared ownership is 
not extended to GetWallets's caller scope.

3ba886d64f

promag commented at 11:13 AM on March 26, 2022: member

Same goal as #24678, to avoid calling GetWallets where it is not desirable to extend shared ownership of all loaded wallets.
DrahtBot added the label RPC/REST/ZMQ on Mar 26, 2022
DrahtBot added the label Wallet on Mar 26, 2022
DrahtBot commented at 4:19 PM on March 27, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25768 (wallet: Properly rebroadcast unconfirmed transaction chains by achow101)
- #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
- #18904 (Don't call lsn_reset in periodic flush by bvbfan)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
shaavan commented at 8:05 AM on March 28, 2022: contributor

Concept ACK

I agree with the reasoning behind this PR. (Here is my detailed review of f59959e) However, I would like to listen to other contributors' views on the approach before giving this PR an ACK.
luke-jr commented at 10:32 PM on April 11, 2022: member

I'm not sure this is a good approach. Now WalletContexts is locked during the loop, and ForEachWallet cannot run in parallel.

Perhaps we should consider read-only shared locks?
DrahtBot added the label Needs rebase on Sep 5, 2022
DrahtBot commented at 1:48 PM on September 5, 2022: contributor



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
promag closed this on Sep 5, 2022
promag deleted the branch on Sep 5, 2022
bitcoin locked this on Sep 5, 2023

Contributors

Labels

Wallet RPC/REST/ZMQ Needs rebase

Add and use ForEachWallet #24683

Conflicts