Skip to content

Avoid taking and immediately releasing and re-taking channel_state in claim_funds #1732

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

Closed
TheBlueMatt opened this issue Sep 20, 2022 · 3 comments · Fixed by #1772
Closed
Labels
good first issue Good for newcomers

Comments

@TheBlueMatt
Copy link
Collaborator

See #1656 (comment)

@TheBlueMatt TheBlueMatt added the good first issue Good for newcomers label Sep 20, 2022
@ViktorTigerstrom
Copy link
Contributor

ViktorTigerstrom commented Sep 24, 2022

I've created a branch that would address this:
https://github.com/ViktorTigerstrom/rust-lightning/tree/2022-09-avoid-retaking-channel-state

But I'm not 100% if we'd actually like to push this change, as it would only be temporary:
Sadly, the acquiring and retaking of the channel_state lock will have to be reintroduced with #1507 unless we also move out the pending_msg_events lock already from the channel_state as well.
The pending_msg_events lock is acquired in claim_funds_from_hop, and as we aim to move the lock order of the channel_state lock with #1507, we can only temporarily lock the channel_state in claims_funds to not violate the lock orders.

I'd suggest that instead of pushing this fix, let's move claimable_htlcs to a standalone lock instead? claimable_htlcs has no consistency guarantees, so it should be pretty simple.

Perhaps this also show's that we'd like to move the pending_msg_events lock either before or with #1507. Am I correct to assume that we'd like to have it per peer in the per_peer_state lock?

@TheBlueMatt
Copy link
Collaborator Author

Maybe push the messages into the per-peer map as well? That would make sense, I think, without having dug into everything - the messages have to be sent in the order they're created (by the channel), so it makes sense for them to be under the same lock (cause the locks has to be held at the same time either way).

@TheBlueMatt
Copy link
Collaborator Author

That said, yes, claimable_htlcs being a separate lock makes sense as well, I believe, so maybe addresses this better, but both need to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants