Uh oh!
There was an error while loading. Please reload this page.
Free holding cells immediately rather than in message sending - #4320
Conversation
👋 I see @tnull was un-assigned. |
I noted in review for an unrelated PR that adding more per-channel logic in `ChannelManager::get_and_clear_pending_msg_events` really sucks for our performance, especially if it ends up hitting a sync monitor persistence. This made me wonder how far we actually are from not needing the holding `check_free_holding_cells` call that's currently there. Turns out, at least according to our functional test coverage, the answer is "not very far". Thus, here we drop it in favor of consistently calling a new util method on channels that might have the ability to release holding cell updates in the same lock where they change state, rather than waiting until `get_and_clear_pending_msg_events`. We still process async monitor events in `get_and_clear_pending_msg_events`, which can lead to channel (and monitor) updates, but that should only be the case for async persist applications, which then are likely to have fast `ChannelMonitorUpdate` in-line handling logic (cause its async).
d6c4618 to
cad88afCompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4320 +/- ##
=======================================
Coverage 86.61% 86.62% =======================================
Files 158 158 Lines 102730 102752 +22 Branches 102730 102752 +22 =======================================
+ Hits 88977 89006 +29 + Misses 11335 11332 -3 + Partials 2418 2414 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ldk-reviews-bot
commented
Jan 19, 2026
🔔 1st Reminder Hey @tnull@valentinewallace! This PR has been waiting for your review. |
valentinewallace
left a comment
There was a problem hiding this comment.
Nice. I looked around to see if some of the calls could be consolidated, but it makes sense that the holding cell would have HTLCs to free on reestablish + monitor update + existing quiescence, basically. Everywhere seems to have test coverage.
| } | ||
| if !post_event_actions.is_empty() { | ||
| let _read_guard = $self.total_consistency_lock.read().unwrap(); |
There was a problem hiding this comment.
This could use a comment IMO
There was a problem hiding this comment.
Will do it in a followup.
valentinewallace
commented
Jan 20, 2026
Not sure if @tnull wanted to take a look for a specific reason so guess I'll hold off on landing for the moment. |
ldk-reviews-bot
commented
Jan 21, 2026
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
Uh oh!
There was an error while loading. Please reload this page.
I noted in review for an unrelated PR that adding more per-channel logic in
ChannelManager::get_and_clear_pending_msg_eventsreally sucks for our performance, especially if it ends up hitting a sync monitor persistence. This made me wonder how far we actually are from not needing the holdingcheck_free_holding_cellscall that's currently there.Turns out, at least according to our functional test coverage, the answer is "not very far".
Thus, here we drop it in favor of consistently calling a new util method on channels that might have the ability to release holding cell updates in the same lock where they change state, rather than waiting until
get_and_clear_pending_msg_events.We still process async monitor events in
get_and_clear_pending_msg_events, which can lead to channel (and monitor) updates, but that should only be the case for async persist applications, which then are likely to have fastChannelMonitorUpdatein-line handling logic (cause its async).