Skip to content

Add and use ForEachWallet #24683

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add and use ForEachWallet #24683

wants to merge 2 commits into from

Conversation

promag
Copy link
Contributor

@promag promag commented Mar 26, 2022

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.

promag added 2 commits March 25, 2022 22:47
@promag
wallet: Prevent wallet unload on GetWalletForJSONRPCRequest
f59959e 
Don't extend shared ownership of all wallets to GetWalletForJSONRPCRequest scope.
@promag
wallet: Add and use ForEachWallet
3ba886d 
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.
@promag
Copy link
Contributor Author

promag commented Mar 26, 2022

Same goal as #24678, to avoid calling GetWallets where it is not desirable to extend shared ownership of all loaded wallets.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 27, 2022 •
edited
Loading

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
shaavan reviewed Mar 28, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DrahtBot DrahtBot mentioned this pull request Mar 29, 2022
Closed
@luke-jr
Copy link
Member

luke-jr commented Apr 11, 2022

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?

This was referenced Jul 26, 2022
Open
Draft
@DrahtBot DrahtBot mentioned this pull request Aug 26, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 5, 2022

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

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@promag promag closed this Sep 5, 2022
@promag promag deleted the 2022-03-foreachwallet branch September 5, 2022 13:51
@bitcoin bitcoin locked and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@shaavan shaavan shaavan left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase RPC/REST/ZMQ Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@promag @DrahtBot @luke-jr @shaavan