Uh oh!
There was an error while loading. Please reload this page.
Handle mon update completion actions even with update(s) is blocked - #4236
Conversation
👋 Thanks for assigning @valentinewallace as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4236 +/- ##
==========================================
- Coverage 89.32% 89.30% -0.03%
==========================================
Files 180 180 Lines 138641 138803 +162 Branches 138641 138803 +162 ==========================================
+ Hits 123844 123955 +111 - Misses 12174 12223 +49 - Partials 2623 2625 +2
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:
|
6700e7c to
eb5492fCompareUh oh!
There was an error while loading. Please reload this page.
ldk-reviews-bot
commented
Nov 21, 2025
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
| let (chan_4_update, _, chan_4_id, ..) = create_announced_chan_between_nodes(&nodes, 2, 3); | ||
| let chan_4_scid = chan_4_update.contents.short_channel_id; | ||
| let (mut route, paymnt_hash_1, preimage_1, payment_secret) = |
There was a problem hiding this comment.
nit: s/paymnt_hash_1/payment_hash_1
There was a problem hiding this comment.
Does it really impact readability? Was trying to avoid a rustfmt.
There was a problem hiding this comment.
Oh, it looks like a typo. Would prefer pmt_hash_1. Non-blocking though.
If we complete a `ChannelMonitorUpdate` persistence but there are blocked `ChannelMonitorUpdate`s in the channel, we'll skip all the post-monitor-update logic entirely. While its correct that we can't resume the channel (as it expected the monitor updates it generated to complete, even if they ended up blocked), the post-update actions are a `channelmanager.rs` concept - they cannot be tied to blocked updates because `channelmanager.rs` doesn't even see blocked updates. This can lead to a channel getting stuck waiting on itself. In a production environment, an LDK user saw a case where: (a) an MPP payment was received over several channels, let's call them A + B. (b) channel B got into `AwaitingRAA` due to unrelated operations, (c) the MPP payment was claimed, with async monitor updating, (d) the `revoke_and_ack` we were waiting on was delivered, but the resulting `ChannelMonitorUpdate` was blocked due to the pending claim having inserted an RAA-blocking action, (e) the preimage `ChannelMonitorUpdate` generated for channel B completed persistence, which did nothing due to the blocked `ChannelMonitorUpdate`. (f) the `Event::PaymentClaimed` event was handled but it, too, failed to unblock the channel. Instead, here, we simply process post-update actions when an update completes, even if there are pending blocked updates. We do not fully unblock the channel, of course.
eb5492f to
8f4a4d2CompareTheBlueMatt
commented
Dec 1, 2025
Corrected the now-spurious documentation on the post-update macro - $ git diff-tree -U2 eb5492f5cf 8f4a4d239d
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cd063bbe33..11923a20b7 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3356,6 +3356,5 @@ macro_rules! emit_initial_channel_ready_event {
/// `handle_new_monitor_update` or [`ChannelManager::channel_monitor_updated`] to call it for you.
///
-/// Requires that `$chan.blocked_monitor_updates_pending() == 0` and the in-flight monitor update-/// set for this channel is empty!+/// Requires that the in-flight monitor update set for this channel is empty!
macro_rules! handle_monitor_update_completion {
($self: ident, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan: expr) => {{ |
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMatt
commented
Dec 2, 2025
Backported in #4254 |
v0.1.8 - Dec 2, 2025 - "Async Update Completion" Bug Fixes ========= * In cases where an MPP payment is claimed while one channel is waiting on a counterparty's `revoke_and_ack` message and the `revoke_and_ack` message is received prior to the asynchronous completion of the MPP-claim `ChannelMonitorUpdate`, the channel will no longer hang (lightningdevkit#4236). * Deserializing invalid `Duration`s can no longer panic (lightningdevkit#4172).
If we complete a
ChannelMonitorUpdatepersistence but there are blockedChannelMonitorUpdates in the channel, we'll skip all the post-monitor-update logic entirely. While its correct that we can't resume the channel (as it expected the monitor updates it generated to complete, even if they ended up blocked), the post-update actions are achannelmanager.rsconcept - they cannot be tied to blocked updates becausechannelmanager.rsdoesn't even see blocked updates.This can lead to a channel getting stuck waiting on itself. In a production environment, an LDK user saw a case where:
(a) an MPP payment was received over several channels, let's call
them A + B.
(b) channel B got into
AwaitingRAAdue to unrelated operations,(c) the MPP payment was claimed, with async monitor updating,
(d) the
revoke_and_ackwe were waiting on was delivered, but theresulting
ChannelMonitorUpdatewas blocked due to thepending claim having inserted an RAA-blocking action,
(e) the preimage
ChannelMonitorUpdategenerated for channel Bcompleted persistence, which did nothing due to the blocked
ChannelMonitorUpdate.(f) the
Event::PaymentClaimedevent was handled but it, too,failed to unblock the channel.
Instead, here, we simply process post-update actions when an update completes, even if there are pending blocked updates. We do not fully unblock the channel, of course.