From a90e9fedf91bac484d22181653d74011338c1aeb Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 17 Mar 2026 12:12:32 -0400 Subject: [PATCH 01/33] Split up some tests that have many variants Helps when debugging to know which variants failed. --- lightning/src/ln/monitor_tests.rs | 60 +++++++++++++++++++++++++++++-- lightning/src/ln/payment_tests.rs | 26 +++++++++++++- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index f52f093917b..d156f874703 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -3803,27 +3803,83 @@ fn do_test_lost_timeout_monitor_events(confirm_tx: CommitmentType, dust_htlcs: b } #[test] -fn test_lost_timeout_monitor_events() { +fn test_lost_timeout_monitor_events_a() { do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, false, false); +} +#[test] +fn test_lost_timeout_monitor_events_b() { do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, true, false); +} +#[test] +fn test_lost_timeout_monitor_events_c() { do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, false, false); +} +#[test] +fn test_lost_timeout_monitor_events_d() { do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, true, false); +} +#[test] +fn test_lost_timeout_monitor_events_e() { do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, false, false); +} +#[test] +fn test_lost_timeout_monitor_events_f() { do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, true, false); +} +#[test] +fn test_lost_timeout_monitor_events_g() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, false, false); +} +#[test] +fn test_lost_timeout_monitor_events_h() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, true, false); +} +#[test] +fn test_lost_timeout_monitor_events_i() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false, false); +} +#[test] +fn test_lost_timeout_monitor_events_j() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true, false); - +} +#[test] +fn test_lost_timeout_monitor_events_k() { do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, false, true); +} +#[test] +fn test_lost_timeout_monitor_events_l() { do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, true, true); +} +#[test] +fn test_lost_timeout_monitor_events_m() { do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, false, true); +} +#[test] +fn test_lost_timeout_monitor_events_n() { do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, true, true); +} +#[test] +fn test_lost_timeout_monitor_events_o() { do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, false, true); +} +#[test] +fn test_lost_timeout_monitor_events_p() { do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, true, true); +} +#[test] +fn test_lost_timeout_monitor_events_q() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, false, true); +} +#[test] +fn test_lost_timeout_monitor_events_r() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, true, true); +} +#[test] +fn test_lost_timeout_monitor_events_s() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false, true); +} +#[test] +fn test_lost_timeout_monitor_events_t() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true, true); } diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 807d1a1af39..a196e5135c2 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -1430,15 +1430,39 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( } #[test] -fn test_dup_htlc_onchain_doesnt_fail_on_reload() { +fn test_dup_htlc_onchain_doesnt_fail_on_reload_a() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, true); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_b() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, false); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_c() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, false, false); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_d() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, true); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_e() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, false); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_f() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, false, false); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_g() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, true); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_h() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, false); +} +#[test] +fn test_dup_htlc_onchain_doesnt_fail_on_reload_i() { do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, false, false); } From 8f127036af7eb2ecfe94d94668abf7ad265e2cd7 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 19 Mar 2026 16:08:46 -0400 Subject: [PATCH 02/33] Remove unnecessary pending_monitor_events clone --- lightning/src/chain/channelmonitor.rs | 31 +++++++++++++-------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 810de80da95..7a3c3315775 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -68,8 +68,8 @@ use crate::util::byte_utils; use crate::util::logger::{Logger, WithContext}; use crate::util::persist::MonitorName; use crate::util::ser::{ - MaybeReadable, Readable, ReadableArgs, RequiredWrapper, UpgradableRequired, Writeable, Writer, - U48, + Iterable, MaybeReadable, Readable, ReadableArgs, RequiredWrapper, UpgradableRequired, + Writeable, Writer, U48, }; #[allow(unused_imports)] @@ -1719,24 +1719,23 @@ pub(crate) fn write_chanmon_internal( channel_monitor.lockdown_from_offchain.write(writer)?; channel_monitor.holder_tx_signed.write(writer)?; - // If we have a `HolderForceClosedWithInfo` event, we need to write the `HolderForceClosed` for backwards compatibility. - let pending_monitor_events = - match channel_monitor.pending_monitor_events.iter().find(|ev| match ev { - MonitorEvent::HolderForceClosedWithInfo { .. } => true, - _ => false, - }) { - Some(MonitorEvent::HolderForceClosedWithInfo { outpoint, .. }) => { - let mut pending_monitor_events = channel_monitor.pending_monitor_events.clone(); - pending_monitor_events.push(MonitorEvent::HolderForceClosed(*outpoint)); - pending_monitor_events - }, - _ => channel_monitor.pending_monitor_events.clone(), - }; + // If we have a `HolderForceClosedWithInfo` event, we need to write the `HolderForceClosed` + // for backwards compatibility. + let holder_force_closed_compat = channel_monitor.pending_monitor_events.iter().find_map(|ev| { + if let MonitorEvent::HolderForceClosedWithInfo { outpoint, .. } = ev { + Some(MonitorEvent::HolderForceClosed(*outpoint)) + } else { + None + } + }); + let pending_monitor_events = Iterable( + channel_monitor.pending_monitor_events.iter().chain(holder_force_closed_compat.as_ref()), + ); write_tlv_fields!(writer, { (1, channel_monitor.funding_spend_confirmed, option), (3, channel_monitor.htlcs_resolved_on_chain, required_vec), - (5, pending_monitor_events, required_vec), + (5, pending_monitor_events, required), // Equivalent to required_vec because Iterable also writes as WithoutLength (7, channel_monitor.funding_spend_seen, required), (9, channel_monitor.counterparty_node_id, required), (11, channel_monitor.confirmed_commitment_tx_counterparty_output, option), From f76f16627662a600641541e07d45fc87303ac9cd Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 17 Mar 2026 15:26:43 -0400 Subject: [PATCH 03/33] Add persistent_monitor_events flag to monitors/manager Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. As a first step towards this, here we persist a flag in the ChannelManager and ChannelMonitors indicating whether this new feature is enabled. It will be used in upcoming commits to maintain compatibility and create an upgrade/downgrade path between LDK versions. --- lightning/src/chain/channelmonitor.rs | 20 +++++++++++++++++++ lightning/src/ln/channelmanager.rs | 28 +++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 7a3c3315775..18351f9c97d 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1281,6 +1281,12 @@ pub(crate) struct ChannelMonitorImpl { // block/transaction-connected events and *not* during block/transaction-disconnected events, // we further MUST NOT generate events during block/transaction-disconnection. pending_monitor_events: Vec, + /// When set, monitor events are retained until explicitly acked rather than cleared on read. + /// + /// Allows the ChannelManager to reconstruct pending HTLC state by replaying monitor events on + /// startup, and make the monitor responsible for both off- and on-chain payment resolution. Will + /// be always set once support for this feature is complete. + persistent_events_enabled: bool, pub(super) pending_events: Vec, pub(super) is_processing_pending_events: bool, @@ -1732,8 +1738,12 @@ pub(crate) fn write_chanmon_internal( channel_monitor.pending_monitor_events.iter().chain(holder_force_closed_compat.as_ref()), ); + // Only write `persistent_events_enabled` if it's set to true, as it's an even TLV. + let persistent_events_enabled = channel_monitor.persistent_events_enabled.then_some(()); + write_tlv_fields!(writer, { (1, channel_monitor.funding_spend_confirmed, option), + (2, persistent_events_enabled, option), (3, channel_monitor.htlcs_resolved_on_chain, required_vec), (5, pending_monitor_events, required), // Equivalent to required_vec because Iterable also writes as WithoutLength (7, channel_monitor.funding_spend_seen, required), @@ -1938,6 +1948,7 @@ impl ChannelMonitor { payment_preimages: new_hash_map(), pending_monitor_events: Vec::new(), + persistent_events_enabled: false, pending_events: Vec::new(), is_processing_pending_events: false, @@ -6695,8 +6706,10 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut is_manual_broadcast = RequiredWrapper(None); let mut funding_seen_onchain = RequiredWrapper(None); let mut best_block_previous_blocks = None; + let mut persistent_events_enabled: Option<()> = None; read_tlv_fields!(reader, { (1, funding_spend_confirmed, option), + (2, persistent_events_enabled, option), (3, htlcs_resolved_on_chain, optional_vec), (5, pending_monitor_events, optional_vec), (7, funding_spend_seen, option), @@ -6723,6 +6736,12 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP best_block.previous_blocks = previous_blocks; } + #[cfg(not(any(feature = "_test_utils", test)))] + if persistent_events_enabled.is_some() { + // This feature isn't supported yet so error if the writer expected it to be. + return Err(DecodeError::InvalidValue) + } + // Note that `payment_preimages_with_info` was added (and is always written) in LDK 0.1, so // we can use it to determine if this monitor was last written by LDK 0.1 or later. let written_by_0_1_or_later = payment_preimages_with_info.is_some(); @@ -6871,6 +6890,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP payment_preimages, pending_monitor_events: pending_monitor_events.unwrap(), + persistent_events_enabled: persistent_events_enabled.is_some(), pending_events, is_processing_pending_events: false, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 2c97e4adaa1..6327b22ce2b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2928,6 +2928,15 @@ pub struct ChannelManager< /// [`ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee`] estimate. last_days_feerates: Mutex>, + /// When set, monitors will repeatedly provide an event back to the `ChannelManager` on restart + /// until the event is explicitly acknowledged as processed. + /// + /// Allows us to reconstruct pending HTLC state by replaying monitor events on startup, rather + /// than from complexly polling and reconciling Channel{Monitor} APIs, as well as move the + /// responsibility of off-chain payment resolution from the Channel to the monitor. Will be + /// always set once support is complete. + persistent_monitor_events: bool, + #[cfg(test)] pub(super) entropy_source: ES, #[cfg(not(test))] @@ -3658,6 +3667,8 @@ impl< signer_provider, logger, + + persistent_monitor_events: false, } } @@ -18102,6 +18113,9 @@ impl< } } + // Only write `persistent_events_enabled` if it's set to true, as it's an even TLV. + let persistent_monitor_events = self.persistent_monitor_events.then_some(()); + write_tlv_fields!(writer, { (1, pending_outbound_payments_no_retry, required), (2, pending_intercepted_htlcs, option), @@ -18114,6 +18128,7 @@ impl< (9, htlc_purposes, required_vec), (10, legacy_in_flight_monitor_updates, option), (11, self.probing_cookie_secret, required), + (12, persistent_monitor_events, option), (13, htlc_onion_fields, optional_vec), (14, decode_update_add_htlcs_opt, option), (15, self.inbound_payment_id_secret, required), @@ -18213,6 +18228,7 @@ pub(super) struct ChannelManagerData { forward_htlcs_legacy: HashMap>, pending_intercepted_htlcs_legacy: HashMap, decode_update_add_htlcs_legacy: HashMap>, + persistent_monitor_events: bool, // The `ChannelManager` version that was written. version: u8, } @@ -18399,6 +18415,7 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger> let mut peer_storage_dir: Option)>> = None; let mut async_receive_offer_cache: AsyncReceiveOfferCache = AsyncReceiveOfferCache::new(); let mut best_block_previous_blocks = None; + let mut persistent_monitor_events: Option<()> = None; read_tlv_fields!(reader, { (1, pending_outbound_payments_no_retry, option), (2, pending_intercepted_htlcs_legacy, option), @@ -18411,6 +18428,7 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger> (9, claimable_htlc_purposes, optional_vec), (10, legacy_in_flight_monitor_updates, option), (11, probing_cookie_secret, option), + (12, persistent_monitor_events, option), (13, amountless_claimable_htlc_onion_fields, optional_vec), (14, decode_update_add_htlcs_legacy, option), (15, inbound_payment_id_secret, option), @@ -18420,6 +18438,12 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger> (23, best_block_previous_blocks, option), }); + #[cfg(not(any(feature = "_test_utils", test)))] + if persistent_monitor_events.is_some() { + // This feature isn't supported yet so error if the writer expected it to be. + return Err(DecodeError::InvalidValue); + } + // Merge legacy pending_outbound_payments fields into a single HashMap. // Priority: pending_outbound_payments (TLV 3) > pending_outbound_payments_no_retry (TLV 1) // > pending_outbound_payments_compat (non-TLV legacy) @@ -18539,6 +18563,7 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger> peer_storage_dir: peer_storage_dir.unwrap_or_default(), async_receive_offer_cache, version, + persistent_monitor_events: persistent_monitor_events.is_some(), }) } } @@ -18839,6 +18864,7 @@ impl< mut in_flight_monitor_updates, peer_storage_dir, async_receive_offer_cache, + persistent_monitor_events, version: _version, } = data; @@ -20100,6 +20126,8 @@ impl< logger: args.logger, config: RwLock::new(args.config), + + persistent_monitor_events, }; let mut processed_claims: HashSet> = new_hash_set(); From da3632b29f17e409e7a2128ee90e80caf618bcbc Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 18 Mar 2026 15:08:54 -0400 Subject: [PATCH 04/33] Add helper to push monitor events Cleans up the next commit --- lightning/src/chain/chainmonitor.rs | 53 +++++++-------------------- lightning/src/chain/channelmonitor.rs | 26 +++++++++---- 2 files changed, 32 insertions(+), 47 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index ca01e95c054..cc56bfaebc8 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -366,9 +366,6 @@ pub struct ChainMonitor< fee_estimator: F, persister: P, _entropy_source: ES, - /// "User-provided" (ie persistence-completion/-failed) [`MonitorEvent`]s. These came directly - /// from the user and not from a [`ChannelMonitor`]. - pending_monitor_events: Mutex, PublicKey)>>, /// The best block height seen, used as a proxy for the passage of time. highest_chain_height: AtomicUsize, @@ -436,7 +433,6 @@ where logger, fee_estimator: feeest, _entropy_source, - pending_monitor_events: Mutex::new(Vec::new()), highest_chain_height: AtomicUsize::new(0), event_notifier: Arc::clone(&event_notifier), persister: AsyncPersister { persister, event_notifier }, @@ -657,7 +653,6 @@ where fee_estimator: feeest, persister, _entropy_source, - pending_monitor_events: Mutex::new(Vec::new()), highest_chain_height: AtomicUsize::new(0), event_notifier: Arc::new(Notifier::new()), pending_send_only_events: Mutex::new(Vec::new()), @@ -802,16 +797,11 @@ where return Ok(()); } let funding_txo = monitor_data.monitor.get_funding_txo(); - self.pending_monitor_events.lock().unwrap().push(( + monitor_data.monitor.push_monitor_event(MonitorEvent::Completed { funding_txo, channel_id, - vec![MonitorEvent::Completed { - funding_txo, - channel_id, - monitor_update_id: monitor_data.monitor.get_latest_update_id(), - }], - monitor_data.monitor.get_counterparty_node_id(), - )); + monitor_update_id: monitor_data.monitor.get_latest_update_id(), + }); self.event_notifier.notify(); Ok(()) @@ -824,14 +814,11 @@ where pub fn force_channel_monitor_updated(&self, channel_id: ChannelId, monitor_update_id: u64) { let monitors = self.monitors.read().unwrap(); let monitor = &monitors.get(&channel_id).unwrap().monitor; - let counterparty_node_id = monitor.get_counterparty_node_id(); - let funding_txo = monitor.get_funding_txo(); - self.pending_monitor_events.lock().unwrap().push(( - funding_txo, + monitor.push_monitor_event(MonitorEvent::Completed { + funding_txo: monitor.get_funding_txo(), channel_id, - vec![MonitorEvent::Completed { funding_txo, channel_id, monitor_update_id }], - counterparty_node_id, - )); + monitor_update_id, + }); self.event_notifier.notify(); } @@ -1266,21 +1253,13 @@ where // The channel is post-close (funding spend seen, lockdown, or // holder tx signed). Return InProgress so ChannelManager freezes // the channel until the force-close MonitorEvents are processed. - // Push a Completed event into pending_monitor_events so it gets - // picked up after the per-monitor events in the next - // release_pending_monitor_events call. - let funding_txo = monitor.get_funding_txo(); - let channel_id = monitor.channel_id(); - self.pending_monitor_events.lock().unwrap().push(( - funding_txo, - channel_id, - vec![MonitorEvent::Completed { - funding_txo, - channel_id, - monitor_update_id: monitor.get_latest_update_id(), - }], - monitor.get_counterparty_node_id(), - )); + // Push a Completed event into the monitor so it gets picked up + // in the next release_pending_monitor_events call. + monitor.push_monitor_event(MonitorEvent::Completed { + funding_txo: monitor.get_funding_txo(), + channel_id: monitor.channel_id(), + monitor_update_id: monitor.get_latest_update_id(), + }); log_debug!( logger, "Deferring completion of ChannelMonitorUpdate id {:?} (channel is post-close)", @@ -1665,10 +1644,6 @@ where )); } } - // Drain pending_monitor_events (which includes deferred post-close - // completions) after per-monitor events so that force-close - // MonitorEvents are processed by ChannelManager first. - pending_monitor_events.extend(self.pending_monitor_events.lock().unwrap().split_off(0)); pending_monitor_events } } diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 18351f9c97d..6e1a82ab8ba 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -183,6 +183,10 @@ impl Readable for ChannelMonitorUpdate { } } +fn push_monitor_event(pending_monitor_events: &mut Vec, event: MonitorEvent) { + pending_monitor_events.push(event); +} + /// An event to be processed by the ChannelManager. #[derive(Clone, PartialEq, Eq)] pub enum MonitorEvent { @@ -226,8 +230,6 @@ pub enum MonitorEvent { }, } impl_writeable_tlv_based_enum_upgradable_legacy!(MonitorEvent, - // Note that Completed is currently never serialized to disk as it is generated only in - // ChainMonitor. (0, Completed) => { (0, funding_txo, required), (2, monitor_update_id, required), @@ -2166,6 +2168,10 @@ impl ChannelMonitor { self.inner.lock().unwrap().get_and_clear_pending_monitor_events() } + pub(super) fn push_monitor_event(&self, event: MonitorEvent) { + self.inner.lock().unwrap().push_monitor_event(event); + } + /// Processes [`SpendableOutputs`] events produced from each [`ChannelMonitor`] upon maturity. /// /// For channels featuring anchor outputs, this method will also process [`BumpTransaction`] @@ -3891,7 +3897,7 @@ impl ChannelMonitorImpl { outpoint: funding_outpoint, channel_id: self.channel_id, }; - self.pending_monitor_events.push(event); + push_monitor_event(&mut self.pending_monitor_events, event); } // Although we aren't signing the transaction directly here, the transaction will be signed @@ -4552,6 +4558,10 @@ impl ChannelMonitorImpl { &self.outputs_to_watch } + fn push_monitor_event(&mut self, event: MonitorEvent) { + push_monitor_event(&mut self.pending_monitor_events, event); + } + fn get_and_clear_pending_monitor_events(&mut self) -> Vec { let mut ret = Vec::new(); mem::swap(&mut ret, &mut self.pending_monitor_events); @@ -5611,7 +5621,7 @@ impl ChannelMonitorImpl { ); log_info!(logger, "Channel closed by funding output spend in txid {txid}"); if !self.funding_spend_seen { - self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(())); + self.push_monitor_event(MonitorEvent::CommitmentTxConfirmed(())); } self.funding_spend_seen = true; @@ -5786,7 +5796,7 @@ impl ChannelMonitorImpl { log_debug!(logger, "HTLC {} failure update in {} has got enough confirmations to be passed upstream", &payment_hash, entry.txid); - self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { + self.push_monitor_event(MonitorEvent::HTLCEvent(HTLCUpdate { payment_hash, payment_preimage: None, source, @@ -5896,7 +5906,7 @@ impl ChannelMonitorImpl { log_error!(logger, "Failing back HTLC {} upstream to preserve the \ channel as the forward HTLC hasn't resolved and our backward HTLC \ expires soon at {}", log_bytes!(htlc.payment_hash.0), inbound_htlc_expiry); - self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { + push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate { source: source.clone(), payment_preimage: None, payment_hash: htlc.payment_hash, @@ -6313,7 +6323,7 @@ impl ChannelMonitorImpl { }, }); self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); - self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { + push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate { source, payment_preimage: Some(payment_preimage), payment_hash, @@ -6337,7 +6347,7 @@ impl ChannelMonitorImpl { }, }); self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); - self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { + push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate { source, payment_preimage: Some(payment_preimage), payment_hash, From 2e84b26867b42b33a8b9c3d578efde629aa3d09f Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 18 Mar 2026 17:18:26 -0400 Subject: [PATCH 05/33] Rename pending_monitor_events to _legacy This field will be deprecated in upcoming commits when we start persisting MonitorEvent ids alongside the MonitorEvents. --- lightning/src/chain/channelmonitor.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 6e1a82ab8ba..611f83e9dd7 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1736,7 +1736,7 @@ pub(crate) fn write_chanmon_internal( None } }); - let pending_monitor_events = Iterable( + let pending_monitor_events_legacy = Iterable( channel_monitor.pending_monitor_events.iter().chain(holder_force_closed_compat.as_ref()), ); @@ -1747,7 +1747,7 @@ pub(crate) fn write_chanmon_internal( (1, channel_monitor.funding_spend_confirmed, option), (2, persistent_events_enabled, option), (3, channel_monitor.htlcs_resolved_on_chain, required_vec), - (5, pending_monitor_events, required), // Equivalent to required_vec because Iterable also writes as WithoutLength + (5, pending_monitor_events_legacy, required), // Equivalent to required_vec because Iterable also writes as WithoutLength (7, channel_monitor.funding_spend_seen, required), (9, channel_monitor.counterparty_node_id, required), (11, channel_monitor.confirmed_commitment_tx_counterparty_output, option), @@ -6645,16 +6645,16 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP } } - let pending_monitor_events_len: u64 = Readable::read(reader)?; - let mut pending_monitor_events = Some( - Vec::with_capacity(cmp::min(pending_monitor_events_len as usize, MAX_ALLOC_SIZE / (32 + 8*3)))); - for _ in 0..pending_monitor_events_len { + let pending_monitor_events_legacy_len: u64 = Readable::read(reader)?; + let mut pending_monitor_events_legacy = Some( + Vec::with_capacity(cmp::min(pending_monitor_events_legacy_len as usize, MAX_ALLOC_SIZE / (32 + 8*3)))); + for _ in 0..pending_monitor_events_legacy_len { let ev = match ::read(reader)? { 0 => MonitorEvent::HTLCEvent(Readable::read(reader)?), 1 => MonitorEvent::HolderForceClosed(outpoint), _ => return Err(DecodeError::InvalidValue) }; - pending_monitor_events.as_mut().unwrap().push(ev); + pending_monitor_events_legacy.as_mut().unwrap().push(ev); } let pending_events_len: u64 = Readable::read(reader)?; @@ -6721,7 +6721,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (1, funding_spend_confirmed, option), (2, persistent_events_enabled, option), (3, htlcs_resolved_on_chain, optional_vec), - (5, pending_monitor_events, optional_vec), + (5, pending_monitor_events_legacy, optional_vec), (7, funding_spend_seen, option), (9, counterparty_node_id, option), (11, confirmed_commitment_tx_counterparty_output, option), @@ -6774,11 +6774,11 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP // `HolderForceClosedWithInfo` replaced `HolderForceClosed` in v0.0.122. If we have both // events, we can remove the `HolderForceClosed` event and just keep the `HolderForceClosedWithInfo`. - if let Some(ref mut pending_monitor_events) = pending_monitor_events { - if pending_monitor_events.iter().any(|e| matches!(e, MonitorEvent::HolderForceClosed(_))) && - pending_monitor_events.iter().any(|e| matches!(e, MonitorEvent::HolderForceClosedWithInfo { .. })) + if let Some(ref mut evs) = pending_monitor_events_legacy { + if evs.iter().any(|e| matches!(e, MonitorEvent::HolderForceClosed(_))) && + evs.iter().any(|e| matches!(e, MonitorEvent::HolderForceClosedWithInfo { .. })) { - pending_monitor_events.retain(|e| !matches!(e, MonitorEvent::HolderForceClosed(_))); + evs.retain(|e| !matches!(e, MonitorEvent::HolderForceClosed(_))); } } @@ -6899,7 +6899,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP current_holder_commitment_number, payment_preimages, - pending_monitor_events: pending_monitor_events.unwrap(), + pending_monitor_events: pending_monitor_events_legacy.unwrap(), persistent_events_enabled: persistent_events_enabled.is_some(), pending_events, is_processing_pending_events: false, From fd90e0c065108b81575ffa2d92804fd48643843d Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 16 Mar 2026 13:37:38 -0400 Subject: [PATCH 06/33] Add chain::Watch ack_monitor_event API Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. Here we add an as-yet-unused API to chain::Watch to allow the ChannelManager to tell the a ChannelMonitor that a MonitorEvent has been irrevocably processed and can be deleted. --- fuzz/src/chanmon_consistency.rs | 5 +++++ lightning/src/chain/chainmonitor.rs | 24 ++++++++++++++++++++++++ lightning/src/chain/channelmonitor.rs | 6 ++++++ lightning/src/chain/mod.rs | 14 ++++++++++++++ lightning/src/util/test_utils.rs | 6 +++++- 5 files changed, 54 insertions(+), 1 deletion(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f20f93c789c..97e1422b542 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -41,6 +41,7 @@ use lightning::chain; use lightning::chain::chaininterface::{ BroadcasterInterface, ConfirmationTarget, FeeEstimator, TransactionType, }; +use lightning::chain::chainmonitor::MonitorEventSource; use lightning::chain::channelmonitor::{ChannelMonitor, MonitorEvent}; use lightning::chain::transaction::OutPoint; use lightning::chain::{ @@ -367,6 +368,10 @@ impl chain::Watch for TestChainMonitor { ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)> { return self.chain_monitor.release_pending_monitor_events(); } + + fn ack_monitor_event(&self, source: MonitorEventSource) { + self.chain_monitor.ack_monitor_event(source); + } } struct KeyProvider { diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index cc56bfaebc8..7b965bea047 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -66,6 +66,21 @@ use core::iter::Cycle; use core::ops::Deref; use core::sync::atomic::{AtomicUsize, Ordering}; +/// Identifies the source of a [`MonitorEvent`] for acknowledgment via +/// [`chain::Watch::ack_monitor_event`] once the event has been processed. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct MonitorEventSource { + /// The event ID assigned by the [`ChannelMonitor`]. + pub event_id: u64, + /// The channel from which the [`MonitorEvent`] originated. + pub channel_id: ChannelId, +} + +impl_writeable_tlv_based!(MonitorEventSource, { + (1, event_id, required), + (3, channel_id, required), +}); + /// A pending operation queued for later execution when `ChainMonitor` is in deferred mode. enum PendingMonitorOp { /// A new monitor to insert and persist. @@ -1646,6 +1661,15 @@ where } pending_monitor_events } + + fn ack_monitor_event(&self, source: MonitorEventSource) { + let monitors = self.monitors.read().unwrap(); + if let Some(monitor_state) = monitors.get(&source.channel_id) { + monitor_state.monitor.ack_monitor_event(source.event_id); + } else { + debug_assert!(false, "Ack'd monitor events should always have a corresponding monitor"); + } + } } impl< diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 611f83e9dd7..eb2ea746642 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2172,6 +2172,12 @@ impl ChannelMonitor { self.inner.lock().unwrap().push_monitor_event(event); } + /// Removes a [`MonitorEvent`] by its event ID, acknowledging that it has been processed. + /// Generally called by [`chain::Watch::ack_monitor_event`]. + pub fn ack_monitor_event(&self, _event_id: u64) { + // TODO: once events have ids, remove the corresponding event here + } + /// Processes [`SpendableOutputs`] events produced from each [`ChannelMonitor`] upon maturity. /// /// For channels featuring anchor outputs, this method will also process [`BumpTransaction`] diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs index 9692558cf7c..59400cc8da9 100644 --- a/lightning/src/chain/mod.rs +++ b/lightning/src/chain/mod.rs @@ -18,6 +18,7 @@ use bitcoin::network::Network; use bitcoin::script::{Script, ScriptBuf}; use bitcoin::secp256k1::PublicKey; +use crate::chain::chainmonitor::MonitorEventSource; use crate::chain::channelmonitor::{ ChannelMonitor, ChannelMonitorUpdate, MonitorEvent, ANTI_REORG_DELAY, }; @@ -425,6 +426,15 @@ pub trait Watch { fn release_pending_monitor_events( &self, ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)>; + + /// Acknowledges and removes a [`MonitorEvent`] previously returned by + /// [`Watch::release_pending_monitor_events`] by its event ID. + /// + /// Once acknowledged, the event will no longer be returned by future calls to + /// [`Watch::release_pending_monitor_events`] and will not be replayed on restart. + /// + /// Events may be acknowledged in any order. + fn ack_monitor_event(&self, source: MonitorEventSource); } impl + ?Sized, W: Deref> @@ -447,6 +457,10 @@ impl + ?Sized, W: Der ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)> { self.deref().release_pending_monitor_events() } + + fn ack_monitor_event(&self, source: MonitorEventSource) { + self.deref().ack_monitor_event(source) + } } /// The `Filter` trait defines behavior for indicating chain activity of interest pertaining to diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 57f9ba6b22f..c31a378be1a 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -15,7 +15,7 @@ use crate::chain::chaininterface; #[cfg(any(test, feature = "_externalize_tests"))] use crate::chain::chaininterface::FEERATE_FLOOR_SATS_PER_KW; use crate::chain::chaininterface::{ConfirmationTarget, TransactionType}; -use crate::chain::chainmonitor::{ChainMonitor, Persist}; +use crate::chain::chainmonitor::{ChainMonitor, MonitorEventSource, Persist}; use crate::chain::channelmonitor::{ ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, MonitorEvent, }; @@ -734,6 +734,10 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { } return self.chain_monitor.release_pending_monitor_events(); } + + fn ack_monitor_event(&self, source: MonitorEventSource) { + self.chain_monitor.ack_monitor_event(source); + } } #[cfg(any(test, feature = "_externalize_tests"))] From cb4a804bb7b4defd419f909e9fcf6978968e24da Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 18 Mar 2026 17:24:04 -0400 Subject: [PATCH 07/33] Add monitor event ids Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. To allow the ChannelManager to ack specific monitor events once they are resolved in upcoming commits, here we give each MonitorEvent a corresponding unique id. It's implemented in such a way that we can delete legacy monitor event code in the future when the new persistent monitor events flag is enabled by default. --- fuzz/src/chanmon_consistency.rs | 2 +- lightning/src/chain/chainmonitor.rs | 2 +- lightning/src/chain/channelmonitor.rs | 200 +++++++++++++----- lightning/src/chain/mod.rs | 8 +- lightning/src/ln/chanmon_update_fail_tests.rs | 6 +- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/util/test_utils.rs | 6 +- 7 files changed, 161 insertions(+), 65 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 97e1422b542..04d92901f32 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -365,7 +365,7 @@ impl chain::Watch for TestChainMonitor { fn release_pending_monitor_events( &self, - ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)> { + ) -> Vec<(OutPoint, ChannelId, Vec<(u64, MonitorEvent)>, PublicKey)> { return self.chain_monitor.release_pending_monitor_events(); } diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 7b965bea047..79ada7c70e6 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -1639,7 +1639,7 @@ where fn release_pending_monitor_events( &self, - ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)> { + ) -> Vec<(OutPoint, ChannelId, Vec<(u64, MonitorEvent)>, PublicKey)> { for (channel_id, update_id) in self.persister.get_and_clear_completed_updates() { let _ = self.channel_monitor_updated(channel_id, update_id); } diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index eb2ea746642..e007260ed60 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -183,8 +183,13 @@ impl Readable for ChannelMonitorUpdate { } } -fn push_monitor_event(pending_monitor_events: &mut Vec, event: MonitorEvent) { - pending_monitor_events.push(event); +fn push_monitor_event( + pending_monitor_events: &mut Vec<(u64, MonitorEvent)>, event: MonitorEvent, + next_monitor_event_id: &mut u64, +) { + let id = *next_monitor_event_id; + *next_monitor_event_id += 1; + pending_monitor_events.push((id, event)); } /// An event to be processed by the ChannelManager. @@ -1282,13 +1287,14 @@ pub(crate) struct ChannelMonitorImpl { // Note that because the `event_lock` in `ChainMonitor` is only taken in // block/transaction-connected events and *not* during block/transaction-disconnected events, // we further MUST NOT generate events during block/transaction-disconnection. - pending_monitor_events: Vec, + pending_monitor_events: Vec<(u64, MonitorEvent)>, /// When set, monitor events are retained until explicitly acked rather than cleared on read. /// /// Allows the ChannelManager to reconstruct pending HTLC state by replaying monitor events on /// startup, and make the monitor responsible for both off- and on-chain payment resolution. Will /// be always set once support for this feature is complete. persistent_events_enabled: bool, + next_monitor_event_id: u64, pub(super) pending_events: Vec, pub(super) is_processing_pending_events: bool, @@ -1669,32 +1675,38 @@ pub(crate) fn write_chanmon_internal( writer.write_all(&payment_preimage.0[..])?; } - writer.write_all( - &(channel_monitor - .pending_monitor_events - .iter() - .filter(|ev| match ev { - MonitorEvent::HTLCEvent(_) => true, - MonitorEvent::HolderForceClosed(_) => true, - MonitorEvent::HolderForceClosedWithInfo { .. } => true, - _ => false, - }) - .count() as u64) - .to_be_bytes(), - )?; - for event in channel_monitor.pending_monitor_events.iter() { - match event { - MonitorEvent::HTLCEvent(upd) => { - 0u8.write(writer)?; - upd.write(writer)?; - }, - MonitorEvent::HolderForceClosed(_) => 1u8.write(writer)?, - // `HolderForceClosedWithInfo` replaced `HolderForceClosed` in v0.0.122. To keep - // backwards compatibility, we write a `HolderForceClosed` event along with the - // `HolderForceClosedWithInfo` event. This is deduplicated in the reader. - MonitorEvent::HolderForceClosedWithInfo { .. } => 1u8.write(writer)?, - _ => {}, // Covered in the TLV writes below + if !channel_monitor.persistent_events_enabled { + writer.write_all( + &(channel_monitor + .pending_monitor_events + .iter() + .filter(|(_, ev)| match ev { + MonitorEvent::HTLCEvent(_) => true, + MonitorEvent::HolderForceClosed(_) => true, + MonitorEvent::HolderForceClosedWithInfo { .. } => true, + _ => false, + }) + .count() as u64) + .to_be_bytes(), + )?; + for (_, event) in channel_monitor.pending_monitor_events.iter() { + match event { + MonitorEvent::HTLCEvent(upd) => { + 0u8.write(writer)?; + upd.write(writer)?; + }, + MonitorEvent::HolderForceClosed(_) => 1u8.write(writer)?, + // `HolderForceClosedWithInfo` replaced `HolderForceClosed` in v0.0.122. To keep + // backwards compatibility, we write a `HolderForceClosed` event along with the + // `HolderForceClosedWithInfo` event. This is deduplicated in the reader. + MonitorEvent::HolderForceClosedWithInfo { .. } => 1u8.write(writer)?, + _ => {}, // Covered in the TLV writes below + } } + } else { + // If `persistent_events_enabled` is set, we'll write the events with their event ids in the + // TLV section below. + writer.write_all(&(0u64).to_be_bytes())?; } writer.write_all(&(channel_monitor.pending_events.len() as u64).to_be_bytes())?; @@ -1729,25 +1741,40 @@ pub(crate) fn write_chanmon_internal( // If we have a `HolderForceClosedWithInfo` event, we need to write the `HolderForceClosed` // for backwards compatibility. - let holder_force_closed_compat = channel_monitor.pending_monitor_events.iter().find_map(|ev| { - if let MonitorEvent::HolderForceClosedWithInfo { outpoint, .. } = ev { - Some(MonitorEvent::HolderForceClosed(*outpoint)) - } else { - None - } - }); - let pending_monitor_events_legacy = Iterable( - channel_monitor.pending_monitor_events.iter().chain(holder_force_closed_compat.as_ref()), - ); + let holder_force_closed_compat = + channel_monitor.pending_monitor_events.iter().find_map(|(_, ev)| { + if let MonitorEvent::HolderForceClosedWithInfo { outpoint, .. } = ev { + Some(MonitorEvent::HolderForceClosed(*outpoint)) + } else { + None + } + }); + let pending_monitor_events_legacy = if !channel_monitor.persistent_events_enabled { + Some(Iterable( + channel_monitor + .pending_monitor_events + .iter() + .map(|(_, ev)| ev) + .chain(holder_force_closed_compat.as_ref()), + )) + } else { + None + }; // Only write `persistent_events_enabled` if it's set to true, as it's an even TLV. let persistent_events_enabled = channel_monitor.persistent_events_enabled.then_some(()); + let pending_mon_evs_with_ids = if persistent_events_enabled.is_some() { + Some(Iterable(channel_monitor.pending_monitor_events.iter())) + } else { + None + }; write_tlv_fields!(writer, { (1, channel_monitor.funding_spend_confirmed, option), (2, persistent_events_enabled, option), (3, channel_monitor.htlcs_resolved_on_chain, required_vec), - (5, pending_monitor_events_legacy, required), // Equivalent to required_vec because Iterable also writes as WithoutLength + (4, pending_mon_evs_with_ids, option), + (5, pending_monitor_events_legacy, option), // Equivalent to optional_vec because Iterable also writes as WithoutLength (7, channel_monitor.funding_spend_seen, required), (9, channel_monitor.counterparty_node_id, required), (11, channel_monitor.confirmed_commitment_tx_counterparty_output, option), @@ -1767,6 +1794,7 @@ pub(crate) fn write_chanmon_internal( (35, channel_monitor.is_manual_broadcast, required), (37, channel_monitor.funding_seen_onchain, required), (39, channel_monitor.best_block.previous_blocks, required), + (41, channel_monitor.next_monitor_event_id, required), }); Ok(()) @@ -1951,6 +1979,7 @@ impl ChannelMonitor { payment_preimages: new_hash_map(), pending_monitor_events: Vec::new(), persistent_events_enabled: false, + next_monitor_event_id: 0, pending_events: Vec::new(), is_processing_pending_events: false, @@ -2164,7 +2193,7 @@ impl ChannelMonitor { /// Get the list of HTLCs who's status has been updated on chain. This should be called by /// ChannelManager via [`chain::Watch::release_pending_monitor_events`]. - pub fn get_and_clear_pending_monitor_events(&self) -> Vec { + pub fn get_and_clear_pending_monitor_events(&self) -> Vec<(u64, MonitorEvent)> { self.inner.lock().unwrap().get_and_clear_pending_monitor_events() } @@ -2178,6 +2207,20 @@ impl ChannelMonitor { // TODO: once events have ids, remove the corresponding event here } + /// Copies [`MonitorEvent`] state from `other` into `self`. + /// Used in tests to align transient runtime state before equality comparison after a + /// serialization round-trip. + #[cfg(any(test, feature = "_test_utils"))] + pub fn copy_monitor_event_state(&self, other: &ChannelMonitor) { + let (pending, next_id) = { + let other_inner = other.inner.lock().unwrap(); + (other_inner.pending_monitor_events.clone(), other_inner.next_monitor_event_id) + }; + let mut self_inner = self.inner.lock().unwrap(); + self_inner.pending_monitor_events = pending; + self_inner.next_monitor_event_id = next_id; + } + /// Processes [`SpendableOutputs`] events produced from each [`ChannelMonitor`] upon maturity. /// /// For channels featuring anchor outputs, this method will also process [`BumpTransaction`] @@ -3903,7 +3946,7 @@ impl ChannelMonitorImpl { outpoint: funding_outpoint, channel_id: self.channel_id, }; - push_monitor_event(&mut self.pending_monitor_events, event); + push_monitor_event(&mut self.pending_monitor_events, event, &mut self.next_monitor_event_id); } // Although we aren't signing the transaction directly here, the transaction will be signed @@ -4494,12 +4537,16 @@ impl ChannelMonitorImpl { "Failing HTLC from late counterparty commitment update immediately \ (funding spend already confirmed)" ); - self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { - payment_hash, - payment_preimage: None, - source: source.clone(), - htlc_value_satoshis, - })); + push_monitor_event( + &mut self.pending_monitor_events, + MonitorEvent::HTLCEvent(HTLCUpdate { + payment_hash, + payment_preimage: None, + source: source.clone(), + htlc_value_satoshis, + }), + &mut self.next_monitor_event_id, + ); self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { commitment_tx_output_idx: None, resolving_txid: Some(confirmed_txid), @@ -4565,10 +4612,14 @@ impl ChannelMonitorImpl { } fn push_monitor_event(&mut self, event: MonitorEvent) { - push_monitor_event(&mut self.pending_monitor_events, event); + push_monitor_event( + &mut self.pending_monitor_events, + event, + &mut self.next_monitor_event_id, + ); } - fn get_and_clear_pending_monitor_events(&mut self) -> Vec { + fn get_and_clear_pending_monitor_events(&mut self) -> Vec<(u64, MonitorEvent)> { let mut ret = Vec::new(); mem::swap(&mut ret, &mut self.pending_monitor_events); ret @@ -5899,7 +5950,7 @@ impl ChannelMonitorImpl { continue; } let duplicate_event = self.pending_monitor_events.iter().any( - |update| if let &MonitorEvent::HTLCEvent(ref upd) = update { + |(_, update)| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == *source } else { false }); if duplicate_event { @@ -5917,7 +5968,7 @@ impl ChannelMonitorImpl { payment_preimage: None, payment_hash: htlc.payment_hash, htlc_value_satoshis: Some(htlc.amount_msat / 1000), - })); + }), &mut self.next_monitor_event_id); } } } @@ -6316,7 +6367,7 @@ impl ChannelMonitorImpl { if let Some((source, payment_hash, amount_msat)) = payment_data { if accepted_preimage_claim { if !self.pending_monitor_events.iter().any( - |update| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == source } else { false }) { + |(_, update)| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == source } else { false }) { self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry { txid: tx.compute_txid(), height, @@ -6334,11 +6385,11 @@ impl ChannelMonitorImpl { payment_preimage: Some(payment_preimage), payment_hash, htlc_value_satoshis: Some(amount_msat / 1000), - })); + }), &mut self.next_monitor_event_id); } } else if offered_preimage_claim { if !self.pending_monitor_events.iter().any( - |update| if let &MonitorEvent::HTLCEvent(ref upd) = update { + |(_, update)| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == source } else { false }) { self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry { @@ -6358,7 +6409,7 @@ impl ChannelMonitorImpl { payment_preimage: Some(payment_preimage), payment_hash, htlc_value_satoshis: Some(amount_msat / 1000), - })); + }), &mut self.next_monitor_event_id); } } else { self.onchain_events_awaiting_threshold_conf.retain(|ref entry| { @@ -6723,10 +6774,13 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut funding_seen_onchain = RequiredWrapper(None); let mut best_block_previous_blocks = None; let mut persistent_events_enabled: Option<()> = None; + let mut next_monitor_event_id: Option = None; + let mut pending_mon_evs_with_ids: Option> = None; read_tlv_fields!(reader, { (1, funding_spend_confirmed, option), (2, persistent_events_enabled, option), (3, htlcs_resolved_on_chain, optional_vec), + (4, pending_mon_evs_with_ids, optional_vec), (5, pending_monitor_events_legacy, optional_vec), (7, funding_spend_seen, option), (9, counterparty_node_id, option), @@ -6747,6 +6801,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (35, is_manual_broadcast, (default_value, false)), (37, funding_seen_onchain, (default_value, true)), (39, best_block_previous_blocks, option), // Added and always set in 0.3 + (41, next_monitor_event_id, option), }); if let Some(previous_blocks) = best_block_previous_blocks { best_block.previous_blocks = previous_blocks; @@ -6788,6 +6843,22 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP } } + // If persistent events are enabled, use the events with their persisted IDs from TLV 4. + // Otherwise, use the legacy events from TLV 5 and assign sequential IDs. + let (next_monitor_event_id, pending_monitor_events): (u64, Vec<(u64, MonitorEvent)>) = + if persistent_events_enabled.is_some() { + let evs = pending_mon_evs_with_ids.unwrap_or_default() + .into_iter().map(|ev| (ev.0, ev.1)).collect(); + (next_monitor_event_id.unwrap_or(0), evs) + } else if let Some(events) = pending_monitor_events_legacy { + let next_id = next_monitor_event_id.unwrap_or(events.len() as u64); + let evs = events.into_iter().enumerate() + .map(|(i, ev)| (i as u64, ev)).collect(); + (next_id, evs) + } else { + (next_monitor_event_id.unwrap_or(0), Vec::new()) + }; + let channel_parameters = channel_parameters.unwrap_or_else(|| { onchain_tx_handler.channel_parameters().clone() }); @@ -6905,8 +6976,9 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP current_holder_commitment_number, payment_preimages, - pending_monitor_events: pending_monitor_events_legacy.unwrap(), + pending_monitor_events, persistent_events_enabled: persistent_events_enabled.is_some(), + next_monitor_event_id, pending_events, is_processing_pending_events: false, @@ -6955,6 +7027,22 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP } } +/// Deserialization wrapper for reading a `(u64, MonitorEvent)`. +/// Necessary because we can't deserialize a (Readable, MaybeReadable) tuple due to trait +/// conflicts. +struct ReadableIdMonitorEvent(u64, MonitorEvent); + +impl MaybeReadable for ReadableIdMonitorEvent { + fn read(reader: &mut R) -> Result, DecodeError> { + let id: u64 = Readable::read(reader)?; + let event_opt: Option = MaybeReadable::read(reader)?; + match event_opt { + Some(ev) => Ok(Some(ReadableIdMonitorEvent(id, ev))), + None => Ok(None), + } + } +} + #[cfg(test)] pub(super) fn dummy_monitor( channel_id: ChannelId, wrap_signer: impl FnOnce(crate::sign::InMemorySigner) -> S, diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs index 59400cc8da9..b7ff2cb917c 100644 --- a/lightning/src/chain/mod.rs +++ b/lightning/src/chain/mod.rs @@ -417,6 +417,10 @@ pub trait Watch { /// Returns any monitor events since the last call. Subsequent calls must only return new /// events. /// + /// Each event comes with a corresponding id. Once the event is processed, call + /// [`Watch::ack_monitor_event`] with the corresponding id and channel id. Unacknowledged events + /// will be re-provided by this method after startup. + /// /// Note that after any block- or transaction-connection calls to a [`ChannelMonitor`], no /// further events may be returned here until the [`ChannelMonitor`] has been fully persisted /// to disk. @@ -425,7 +429,7 @@ pub trait Watch { /// [`MonitorEvent::Completed`] here, see [`ChannelMonitorUpdateStatus::InProgress`]. fn release_pending_monitor_events( &self, - ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)>; + ) -> Vec<(OutPoint, ChannelId, Vec<(u64, MonitorEvent)>, PublicKey)>; /// Acknowledges and removes a [`MonitorEvent`] previously returned by /// [`Watch::release_pending_monitor_events`] by its event ID. @@ -454,7 +458,7 @@ impl + ?Sized, W: Der fn release_pending_monitor_events( &self, - ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)> { + ) -> Vec<(OutPoint, ChannelId, Vec<(u64, MonitorEvent)>, PublicKey)> { self.deref().release_pending_monitor_events() } diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index af4d1569d0c..4d1451e3ee6 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -5021,7 +5021,7 @@ fn native_async_persist() { let completed_persist = async_chain_monitor.release_pending_monitor_events(); assert_eq!(completed_persist.len(), 1); assert_eq!(completed_persist[0].2.len(), 1); - assert!(matches!(completed_persist[0].2[0], MonitorEvent::Completed { .. })); + assert!(matches!(completed_persist[0].2[0].1, MonitorEvent::Completed { .. })); // Now test two async `ChannelMonitorUpdate`s in flight at once, completing them in-order but // separately. @@ -5069,7 +5069,7 @@ fn native_async_persist() { let completed_persist = async_chain_monitor.release_pending_monitor_events(); assert_eq!(completed_persist.len(), 1); assert_eq!(completed_persist[0].2.len(), 1); - assert!(matches!(completed_persist[0].2[0], MonitorEvent::Completed { .. })); + assert!(matches!(completed_persist[0].2[0].1, MonitorEvent::Completed { .. })); // Finally, test two async `ChanelMonitorUpdate`s in flight at once, completing them // out-of-order and ensuring that no `MonitorEvent::Completed` is generated until they are both @@ -5115,7 +5115,7 @@ fn native_async_persist() { let completed_persist = async_chain_monitor.release_pending_monitor_events(); assert_eq!(completed_persist.len(), 1); assert_eq!(completed_persist[0].2.len(), 1); - if let MonitorEvent::Completed { monitor_update_id, .. } = &completed_persist[0].2[0] { + if let (_, MonitorEvent::Completed { monitor_update_id, .. }) = &completed_persist[0].2[0] { assert_eq!(*monitor_update_id, 4); } else { panic!(); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6327b22ce2b..0384fed65b9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13522,7 +13522,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ for (funding_outpoint, channel_id, mut monitor_events, counterparty_node_id) in pending_monitor_events.drain(..) { - for monitor_event in monitor_events.drain(..) { + for (_event_id, monitor_event) in monitor_events.drain(..) { match monitor_event { MonitorEvent::HTLCEvent(htlc_update) => { let logger = WithContext::from( diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index c31a378be1a..0961f31cfe6 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -649,6 +649,7 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { ) .unwrap() .1; + new_monitor.copy_monitor_event_state(&monitor); assert!(new_monitor == monitor); self.latest_monitor_update_id .lock() @@ -710,6 +711,9 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { // it so it doesn't leak into the rest of the test. let failed_back = monitor.inner.lock().unwrap().failed_back_htlc_ids.clone(); new_monitor.inner.lock().unwrap().failed_back_htlc_ids = failed_back; + // The deserialized monitor will reset the monitor event state, so copy it from the live + // monitor before comparing. + new_monitor.copy_monitor_event_state(&monitor); if let Some(chan_id) = self.expect_monitor_round_trip_fail.lock().unwrap().take() { assert_eq!(chan_id, channel_id); assert!(new_monitor != *monitor); @@ -723,7 +727,7 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { fn release_pending_monitor_events( &self, - ) -> Vec<(OutPoint, ChannelId, Vec, PublicKey)> { + ) -> Vec<(OutPoint, ChannelId, Vec<(u64, MonitorEvent)>, PublicKey)> { // Auto-flush pending operations so that the ChannelManager can pick up monitor // completion events. When not in deferred mode the queue is empty so this only // costs a lock acquisition. It ensures standard test helpers (route_payment, etc.) From 491f582855f9059b802bc91d3814eb9da5e6d900 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 16 Mar 2026 13:45:21 -0400 Subject: [PATCH 08/33] Ack monitor events immediately Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This will simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. Here for the purposes of merging initial support for persistent monitor events, we ack each immediately after it is received/handled by the ChannelManager, which is equivalent to the behavior we had prior to monitor events becoming persistent. In upcoming work, we'll want to have much more special handling for HTLCUpdate monitor events in particular -- e.g. for outbound payment claim events, we should only ACK the monitor event when the PaymentSent event is processed, until that point we want it to keep being provided back to us on startup. All the other monitor events are trivial to ACK, since they don't need to be re-processed on startup. --- lightning/src/ln/channelmanager.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0384fed65b9..89ebb41ce71 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -42,6 +42,7 @@ use crate::chain::chaininterface::{ BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator, TransactionType, }; +use crate::chain::chainmonitor::MonitorEventSource; use crate::chain::channelmonitor::{ ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, MonitorEvent, WithChannelMonitor, ANTI_REORG_DELAY, CLTV_CLAIM_BUFFER, HTLC_FAIL_BACK_BUFFER, @@ -13522,7 +13523,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ for (funding_outpoint, channel_id, mut monitor_events, counterparty_node_id) in pending_monitor_events.drain(..) { - for (_event_id, monitor_event) in monitor_events.drain(..) { + for (event_id, monitor_event) in monitor_events.drain(..) { + let monitor_event_source = MonitorEventSource { event_id, channel_id }; match monitor_event { MonitorEvent::HTLCEvent(htlc_update) => { let logger = WithContext::from( @@ -13572,6 +13574,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ completion_update, ); } + self.chain_monitor.ack_monitor_event(monitor_event_source); }, MonitorEvent::HolderForceClosed(_) | MonitorEvent::HolderForceClosedWithInfo { .. } => { @@ -13605,6 +13608,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ failed_channels.push((Err(e), counterparty_node_id)); } } + // Channel close monitor events do not need to be replayed on startup because we + // already check the monitors to see if the channel is closed. + self.chain_monitor.ack_monitor_event(monitor_event_source); }, MonitorEvent::CommitmentTxConfirmed(_) => { let per_peer_state = self.per_peer_state.read().unwrap(); @@ -13626,6 +13632,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ failed_channels.push((Err(e), counterparty_node_id)); } } + // Channel close monitor events do not need to be replayed on startup because we + // already check the monitors to see if the channel is closed. + self.chain_monitor.ack_monitor_event(monitor_event_source); }, MonitorEvent::Completed { channel_id, monitor_update_id, .. } => { self.channel_monitor_updated( @@ -13633,6 +13642,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Some(monitor_update_id), &counterparty_node_id, ); + self.chain_monitor.ack_monitor_event(monitor_event_source); }, } } From 76c142d868cebd59b128c51256367441747153ca Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 17 Mar 2026 15:31:39 -0400 Subject: [PATCH 09/33] Support persistent monitor events Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This will simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. Here we complete work that was built on recent prior commits and actually start re-providing monitor events on startup if they went un-acked during runtime. This isn't actually supported in prod yet, so this new code will run randomly in tests, to ensure we still support the old paths. --- lightning/src/chain/channelmonitor.rs | 61 +++++++++++++++++++++------ lightning/src/ln/channelmanager.rs | 27 +++++++++++- lightning/src/ln/monitor_tests.rs | 3 ++ lightning/src/util/config.rs | 8 ++++ 4 files changed, 86 insertions(+), 13 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index e007260ed60..a85cae4b90c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1288,6 +1288,11 @@ pub(crate) struct ChannelMonitorImpl { // block/transaction-connected events and *not* during block/transaction-disconnected events, // we further MUST NOT generate events during block/transaction-disconnection. pending_monitor_events: Vec<(u64, MonitorEvent)>, + // `MonitorEvent`s that have been provided to the `ChannelManager` via + // [`ChannelMonitor::get_and_clear_pending_monitor_events`] and are awaiting + // [`ChannelMonitor::ack_monitor_event`] for removal. If an event in this queue is not ack'd, it + // will be re-provided to the `ChannelManager` on startup. + provided_monitor_events: Vec<(u64, MonitorEvent)>, /// When set, monitor events are retained until explicitly acked rather than cleared on read. /// /// Allows the ChannelManager to reconstruct pending HTLC state by replaying monitor events on @@ -1764,7 +1769,12 @@ pub(crate) fn write_chanmon_internal( // Only write `persistent_events_enabled` if it's set to true, as it's an even TLV. let persistent_events_enabled = channel_monitor.persistent_events_enabled.then_some(()); let pending_mon_evs_with_ids = if persistent_events_enabled.is_some() { - Some(Iterable(channel_monitor.pending_monitor_events.iter())) + Some(Iterable( + channel_monitor + .provided_monitor_events + .iter() + .chain(channel_monitor.pending_monitor_events.iter()), + )) } else { None }; @@ -1978,6 +1988,7 @@ impl ChannelMonitor { payment_preimages: new_hash_map(), pending_monitor_events: Vec::new(), + provided_monitor_events: Vec::new(), persistent_events_enabled: false, next_monitor_event_id: 0, pending_events: Vec::new(), @@ -2203,8 +2214,15 @@ impl ChannelMonitor { /// Removes a [`MonitorEvent`] by its event ID, acknowledging that it has been processed. /// Generally called by [`chain::Watch::ack_monitor_event`]. - pub fn ack_monitor_event(&self, _event_id: u64) { - // TODO: once events have ids, remove the corresponding event here + pub fn ack_monitor_event(&self, event_id: u64) { + let inner = &mut *self.inner.lock().unwrap(); + inner.ack_monitor_event(event_id); + } + + /// Enables persistent monitor events mode. When enabled, monitor events are retained until + /// explicitly acked rather than cleared on read. + pub(crate) fn set_persistent_events_enabled(&self, enabled: bool) { + self.inner.lock().unwrap().persistent_events_enabled = enabled; } /// Copies [`MonitorEvent`] state from `other` into `self`. @@ -2212,11 +2230,16 @@ impl ChannelMonitor { /// serialization round-trip. #[cfg(any(test, feature = "_test_utils"))] pub fn copy_monitor_event_state(&self, other: &ChannelMonitor) { - let (pending, next_id) = { + let (provided, pending, next_id) = { let other_inner = other.inner.lock().unwrap(); - (other_inner.pending_monitor_events.clone(), other_inner.next_monitor_event_id) + ( + other_inner.provided_monitor_events.clone(), + other_inner.pending_monitor_events.clone(), + other_inner.next_monitor_event_id, + ) }; let mut self_inner = self.inner.lock().unwrap(); + self_inner.provided_monitor_events = provided; self_inner.pending_monitor_events = pending; self_inner.next_monitor_event_id = next_id; } @@ -4619,10 +4642,23 @@ impl ChannelMonitorImpl { ); } + fn ack_monitor_event(&mut self, event_id: u64) { + self.provided_monitor_events.retain(|(id, _)| *id != event_id); + // If this event was generated prior to a restart, it may be in this queue instead + self.pending_monitor_events.retain(|(id, _)| *id != event_id); + } + fn get_and_clear_pending_monitor_events(&mut self) -> Vec<(u64, MonitorEvent)> { - let mut ret = Vec::new(); - mem::swap(&mut ret, &mut self.pending_monitor_events); - ret + if self.persistent_events_enabled { + let mut ret = Vec::new(); + mem::swap(&mut ret, &mut self.pending_monitor_events); + self.provided_monitor_events.extend(ret.iter().cloned()); + ret + } else { + let mut ret = Vec::new(); + mem::swap(&mut ret, &mut self.pending_monitor_events); + ret + } } /// Gets the set of events that are repeated regularly (e.g. those which RBF bump @@ -5949,8 +5985,8 @@ impl ChannelMonitorImpl { if inbound_htlc_expiry > max_expiry_height { continue; } - let duplicate_event = self.pending_monitor_events.iter().any( - |(_, update)| if let &MonitorEvent::HTLCEvent(ref upd) = update { + let duplicate_event = self.pending_monitor_events.iter().chain(self.provided_monitor_events.iter()) + .any(|(_, update)| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == *source } else { false }); if duplicate_event { @@ -6366,7 +6402,7 @@ impl ChannelMonitorImpl { // HTLC resolution backwards to and figure out whether we learned a preimage from it. if let Some((source, payment_hash, amount_msat)) = payment_data { if accepted_preimage_claim { - if !self.pending_monitor_events.iter().any( + if !self.pending_monitor_events.iter().chain(self.provided_monitor_events.iter()).any( |(_, update)| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == source } else { false }) { self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry { txid: tx.compute_txid(), @@ -6388,7 +6424,7 @@ impl ChannelMonitorImpl { }), &mut self.next_monitor_event_id); } } else if offered_preimage_claim { - if !self.pending_monitor_events.iter().any( + if !self.pending_monitor_events.iter().chain(self.provided_monitor_events.iter()).any( |(_, update)| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == source } else { false }) { @@ -6977,6 +7013,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP payment_preimages, pending_monitor_events, + provided_monitor_events: Vec::new(), persistent_events_enabled: persistent_events_enabled.is_some(), next_monitor_event_id, pending_events, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 89ebb41ce71..3d62573eec5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3613,6 +3613,8 @@ impl< our_network_pubkey, current_timestamp, expanded_inbound_key, node_signer.get_receive_auth_key(), secp_ctx.clone(), message_router, logger.clone(), ); + #[cfg(any(test, feature = "_test_utils"))] + let override_persistent_monitor_events = config.override_persistent_monitor_events; ChannelManager { config: RwLock::new(config), @@ -3669,7 +3671,27 @@ impl< logger, - persistent_monitor_events: false, + persistent_monitor_events: { + #[cfg(not(any(test, feature = "_test_utils")))] + { false } + #[cfg(any(test, feature = "_test_utils"))] + { + override_persistent_monitor_events.unwrap_or_else(|| { + use core::hash::{BuildHasher, Hasher}; + match std::env::var("LDK_TEST_PERSISTENT_MON_EVENTS") { + Ok(val) => match val.as_str() { + "1" => true, + "0" => false, + _ => panic!("LDK_TEST_PERSISTENT_MON_EVENTS must be 0 or 1, got: {}", val), + }, + Err(_) => { + let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish(); + rand_val % 2 == 0 + }, + } + }) + } + }, } } @@ -11614,6 +11636,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ fail_chan!("Already had channel with the new channel_id"); }, hash_map::Entry::Vacant(e) => { + monitor.set_persistent_events_enabled(self.persistent_monitor_events); let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor); if let Ok(persist_state) = monitor_res { // There's no problem signing a counterparty's funding transaction if our monitor @@ -11784,6 +11807,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ match chan .funding_signed(&msg, best_block, &self.signer_provider, &self.logger) .and_then(|(funded_chan, monitor)| { + monitor.set_persistent_events_enabled(self.persistent_monitor_events); self.chain_monitor .watch_channel(funded_chan.context.channel_id(), monitor) .map_err(|()| { @@ -12698,6 +12722,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ if let Some(chan) = chan.as_funded_mut() { if let Some(monitor) = monitor_opt { + monitor.set_persistent_events_enabled(self.persistent_monitor_events); let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor); if let Ok(persist_state) = monitor_res { diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index d156f874703..438691b71c7 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -3594,6 +3594,9 @@ fn do_test_lost_timeout_monitor_events(confirm_tx: CommitmentType, dust_htlcs: b let mut cfg = test_default_channel_config(); cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; cfg.channel_handshake_config.negotiate_anchor_zero_fee_commitments = p2a_anchor; + // This test specifically tests lost monitor events, which requires the legacy + // (non-persistent) monitor event behavior. + cfg.override_persistent_monitor_events = Some(false); let cfgs = [Some(cfg.clone()), Some(cfg.clone()), Some(cfg.clone())]; let chanmon_cfgs = create_chanmon_cfgs(3); diff --git a/lightning/src/util/config.rs b/lightning/src/util/config.rs index ebef8c27bca..b6b70554b3f 100644 --- a/lightning/src/util/config.rs +++ b/lightning/src/util/config.rs @@ -1132,6 +1132,10 @@ pub struct UserConfig { /// /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel pub reject_inbound_splices: bool, + /// If set to `Some`, overrides the random selection of whether to use persistent monitor + /// events. Only available in tests. + #[cfg(any(test, feature = "_test_utils"))] + pub override_persistent_monitor_events: Option, } impl Default for UserConfig { @@ -1148,6 +1152,8 @@ impl Default for UserConfig { enable_htlc_hold: false, hold_outbound_htlcs_at_next_hop: false, reject_inbound_splices: true, + #[cfg(any(test, feature = "_test_utils"))] + override_persistent_monitor_events: None, } } } @@ -1170,6 +1176,8 @@ impl Readable for UserConfig { hold_outbound_htlcs_at_next_hop: Readable::read(reader)?, enable_htlc_hold: Readable::read(reader)?, reject_inbound_splices: Readable::read(reader)?, + #[cfg(any(test, feature = "_test_utils"))] + override_persistent_monitor_events: None, }) } } From b1054ffb0e83a3ca8fa651cf4722e4a1d277dc55 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 30 Mar 2026 11:05:53 -0400 Subject: [PATCH 10/33] Track recent monitor updates in TestChainMonitor And log them in check_added_monitors if it fails. --- lightning/src/ln/functional_test_utils.rs | 15 ++++++++++++++- lightning/src/util/test_utils.rs | 9 +++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index d39cee78b0f..ceba0c9af3b 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1260,7 +1260,20 @@ pub fn check_added_monitors>(node: & if let Some(chain_monitor) = node.chain_monitor() { let mut added_monitors = chain_monitor.added_monitors.lock().unwrap(); let n = added_monitors.len(); - assert_eq!(n, count, "expected {} monitors to be added, not {}", count, n); + if n != count { + let recent = chain_monitor.recent_monitor_updates.lock().unwrap(); + let mut desc = String::new(); + for (i, (chan_id, update)) in recent.iter().take(n).enumerate() { + desc += &format!( + "\n [{}] chan={} update_id={} steps={:?}", + i, chan_id, update.update_id, update.updates + ); + } + panic!( + "expected {} monitors to be added, not {}. Last {} updates (most recent first):{}", + count, n, n, desc + ); + } added_monitors.clear(); } } diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 0961f31cfe6..f4eb6287921 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -521,6 +521,8 @@ pub struct TestChainMonitor<'a> { /// deferred operations. This allows tests to control exactly when queued monitor updates /// are applied to the in-memory monitor. pub pause_flush: AtomicBool, + /// Buffer of the last 20 monitor updates, most recent first. + pub recent_monitor_updates: Mutex>, } impl<'a> TestChainMonitor<'a> { pub fn new( @@ -581,6 +583,7 @@ impl<'a> TestChainMonitor<'a> { #[cfg(feature = "std")] write_blocker: Mutex::new(None), pause_flush: AtomicBool::new(false), + recent_monitor_updates: Mutex::new(Vec::new()), } } @@ -679,6 +682,12 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { .or_insert(Vec::new()) .push(update.clone()); + { + let mut recent = self.recent_monitor_updates.lock().unwrap(); + recent.insert(0, (channel_id, update.clone())); + recent.truncate(20); + } + if let Some(exp) = self.expect_channel_force_closed.lock().unwrap().take() { assert_eq!(channel_id, exp.0); assert_eq!(update.updates.len(), 1); From 2bfeea5c461445f87bc6fb44596454edce53fe7f Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 24 Mar 2026 13:05:25 -0400 Subject: [PATCH 11/33] Persist user channel id in monitors Will be used in upcoming commits when generating MonitorEvents. --- lightning/src/chain/channelmonitor.rs | 10 +++++++++- lightning/src/ln/channel.rs | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a85cae4b90c..213fa43212f 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1211,6 +1211,8 @@ pub(crate) struct ChannelMonitorImpl { /// True if this channel was configured for manual funding broadcasts. Monitors written by /// versions prior to LDK 0.2 load with `false` until a new update persists it. is_manual_broadcast: bool, + /// The user-provided channel ID, used to populate events generated by the monitor. + user_channel_id: Option, /// True once we've observed either funding transaction on-chain. Older monitors prior to LDK 0.2 /// assume this is `true` when absent during upgrade so holder broadcasts aren't gated unexpectedly. /// In manual-broadcast channels we also use this to trigger deferred holder @@ -1805,6 +1807,7 @@ pub(crate) fn write_chanmon_internal( (37, channel_monitor.funding_seen_onchain, required), (39, channel_monitor.best_block.previous_blocks, required), (41, channel_monitor.next_monitor_event_id, required), + (43, channel_monitor.user_channel_id, option), // Added in 0.4 }); Ok(()) @@ -1909,7 +1912,7 @@ impl ChannelMonitor { commitment_transaction_number_obscure_factor: u64, initial_holder_commitment_tx: HolderCommitmentTransaction, best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId, - is_manual_broadcast: bool, + is_manual_broadcast: bool, user_channel_id: u128, ) -> ChannelMonitor { assert!(commitment_transaction_number_obscure_factor <= (1 << 48)); @@ -1958,6 +1961,7 @@ impl ChannelMonitor { pending_funding: vec![], is_manual_broadcast, + user_channel_id: Some(user_channel_id), funding_seen_onchain: false, latest_update_id: 0, @@ -6809,6 +6813,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut is_manual_broadcast = RequiredWrapper(None); let mut funding_seen_onchain = RequiredWrapper(None); let mut best_block_previous_blocks = None; + let mut user_channel_id: Option = None; let mut persistent_events_enabled: Option<()> = None; let mut next_monitor_event_id: Option = None; let mut pending_mon_evs_with_ids: Option> = None; @@ -6838,6 +6843,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (37, funding_seen_onchain, (default_value, true)), (39, best_block_previous_blocks, option), // Added and always set in 0.3 (41, next_monitor_event_id, option), + (43, user_channel_id, option), // Added in 0.4 }); if let Some(previous_blocks) = best_block_previous_blocks { best_block.previous_blocks = previous_blocks; @@ -6981,6 +6987,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP }, pending_funding: pending_funding.unwrap_or(vec![]), is_manual_broadcast: is_manual_broadcast.0.unwrap(), + user_channel_id, // Older monitors prior to LDK 0.2 assume this is `true` when absent // during upgrade so holder broadcasts aren't gated unexpectedly. funding_seen_onchain: funding_seen_onchain.0.unwrap(), @@ -7142,6 +7149,7 @@ pub(super) fn dummy_monitor( dummy_key, channel_id, false, + 0, ) } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 32c0e94bdc8..485d3c320d5 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3617,7 +3617,7 @@ trait InitialRemoteCommitmentReceiver { funding.get_holder_selected_contest_delay(), &context.destination_script, &funding.channel_transaction_parameters, funding.is_outbound(), obscure_factor, holder_commitment_tx, best_block, context.counterparty_node_id, context.channel_id(), - context.is_manual_broadcast, + context.is_manual_broadcast, context.get_user_id(), ); channel_monitor.provide_initial_counterparty_commitment_tx( counterparty_initial_commitment_tx.clone(), From 8aa3b86fbeb4d110cbb49d0cdcc4332fdb7f46c9 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 24 Mar 2026 14:48:56 -0400 Subject: [PATCH 12/33] Include user channel id in monitor event Processing MonitorEvent::HTLCEvent causes the ChannelManager to call claim_funds_internal, but it will currently pass in None for the user_channel_id parameter. In upcoming commits when we begin generating monitor events for off-chain HTLC claims as well as onchain, we'll want to start using an accurate value instead. --- lightning/src/chain/channelmonitor.rs | 7 +++++++ lightning/src/ln/channelmanager.rs | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 213fa43212f..36953ea0147 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -261,12 +261,14 @@ pub struct HTLCUpdate { pub(crate) payment_preimage: Option, pub(crate) source: HTLCSource, pub(crate) htlc_value_satoshis: Option, + pub(crate) user_channel_id: Option, } impl_writeable_tlv_based!(HTLCUpdate, { (0, payment_hash, required), (1, htlc_value_satoshis, option), (2, source, required), (4, payment_preimage, option), + (5, user_channel_id, option), }); /// If an output goes from claimable only by us to claimable by us or our counterparty within this @@ -4571,6 +4573,7 @@ impl ChannelMonitorImpl { payment_preimage: None, source: source.clone(), htlc_value_satoshis, + user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id, ); @@ -5898,6 +5901,7 @@ impl ChannelMonitorImpl { payment_preimage: None, source, htlc_value_satoshis, + user_channel_id: self.user_channel_id, })); self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { commitment_tx_output_idx, @@ -6008,6 +6012,7 @@ impl ChannelMonitorImpl { payment_preimage: None, payment_hash: htlc.payment_hash, htlc_value_satoshis: Some(htlc.amount_msat / 1000), + user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id); } } @@ -6425,6 +6430,7 @@ impl ChannelMonitorImpl { payment_preimage: Some(payment_preimage), payment_hash, htlc_value_satoshis: Some(amount_msat / 1000), + user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id); } } else if offered_preimage_claim { @@ -6449,6 +6455,7 @@ impl ChannelMonitorImpl { payment_preimage: Some(payment_preimage), payment_hash, htlc_value_satoshis: Some(amount_msat / 1000), + user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id); } } else { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3d62573eec5..c844064733b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13575,7 +13575,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ counterparty_node_id, funding_outpoint, channel_id, - None, + htlc_update.user_channel_id, None, None, ); From 9432f2b7c93435127acc6d2f731ca5001f714254 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 13:51:17 -0400 Subject: [PATCH 13/33] Pass best block height to outbound_payments::claim_htlc Used in an upcoming commit to insert a pending payment if it's missing on startup and we need to re-claim. --- lightning/src/ln/channelmanager.rs | 3 +++ lightning/src/ln/outbound_payment.rs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c844064733b..8654d90e96b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -10026,6 +10026,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Some(payment_preimage.into()), payment_id, ); + let best_block_height = self.best_block.read().unwrap().height; self.pending_outbound_payments.claim_htlc( payment_id, payment_preimage, @@ -10033,6 +10034,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ session_priv, path, from_onchain, + best_block_height, &mut ev_completion_action, &self.pending_events, &logger, @@ -19672,6 +19674,7 @@ impl< session_priv, path, true, + best_block.height, &mut compl_action, &pending_events, &logger, diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 7259f60796f..f0a52d50dc5 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2248,7 +2248,7 @@ impl OutboundPayments { #[rustfmt::skip] pub(super) fn claim_htlc( &self, payment_id: PaymentId, payment_preimage: PaymentPreimage, bolt12_invoice: Option, - session_priv: SecretKey, path: Path, from_onchain: bool, ev_completion_action: &mut Option, + session_priv: SecretKey, path: Path, from_onchain: bool, best_block_height: u32, ev_completion_action: &mut Option, pending_events: &Mutex)>>, logger: &WithContext, ) From 66afc4181fd6c5b509237622795e5a85cb8a7e90 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 13:53:16 -0400 Subject: [PATCH 14/33] Pass monitor event id to claim_funds_internal Used in an upcoming commit to generate an EventCompletionAction::AckMonitorEvent. --- lightning/src/ln/channelmanager.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8654d90e96b..fe232fb57c8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -9992,6 +9992,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ next_channel_counterparty_node_id: PublicKey, next_channel_outpoint: OutPoint, next_channel_id: ChannelId, next_user_channel_id: Option, attribution_data: Option, send_timestamp: Option, + monitor_event_id: Option, ) { let startup_replay = !self.background_events_processed_since_startup.load(Ordering::Acquire); @@ -12640,6 +12641,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Some(next_user_channel_id), msg.attribution_data, send_timestamp, + None, ); Ok(()) @@ -13580,6 +13582,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ htlc_update.user_channel_id, None, None, + Some(event_id), ); } else { log_trace!(logger, "Failing HTLC from our monitor"); @@ -20496,6 +20499,7 @@ impl< downstream_user_channel_id, None, None, + None, ); } From e48e7b1673c64d63db5f78951fb92e7c4917bcf7 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 14:50:30 -0400 Subject: [PATCH 15/33] Stop hardcoding from_onchain in monitor ev claim_funds In upcoming commits, we'll be generating monitor events for off-chain claims as well as on-chain. As a small prefactor, calculate the from_onchain value rather than hardcoding it to true. --- lightning/src/ln/channelmanager.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fe232fb57c8..669130b9ade 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13568,6 +13568,18 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ "Claiming HTLC with preimage {} from our monitor", preimage ); + let from_onchain = self + .per_peer_state + .read() + .unwrap() + .get(&counterparty_node_id) + .map_or(true, |peer_state_mtx| { + !peer_state_mtx + .lock() + .unwrap() + .channel_by_id + .contains_key(&channel_id) + }); // Claim the funds from the previous hop, if there is one. Because this is in response to a // chain event, no attribution data is available. self.claim_funds_internal( @@ -13575,7 +13587,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), None, - true, + from_onchain, counterparty_node_id, funding_outpoint, channel_id, From e913eced6cf3102d07ee7217904c5f67033804c2 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 12:22:38 -0400 Subject: [PATCH 16/33] Add EventCompletionAction::AckMonitorEvent If ChannelManager::persistent_monitor_events is enabled, we may want to avoid acking a monitor event until after an Event is processed by the user. In upcoming commits, we'll use this to ensure a MonitorEvent::HTLCEvent will keep being re-provided back to us until after an Event::PaymentSent is processed. --- lightning/src/ln/channelmanager.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 669130b9ade..40eac403422 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1553,6 +1553,11 @@ pub(crate) enum EventCompletionAction { /// fully-resolved in the [`ChannelMonitor`], which we do via this action. /// Note that this action will be dropped on downgrade to LDK prior to 0.2! ReleasePaymentCompleteChannelMonitorUpdate(PaymentCompleteUpdate), + /// If [`ChannelManager::persistent_monitor_events`] is enabled, we may want to avoid acking a + /// monitor event via [`Watch::ack_monitor_event`] until after an [`Event`] is processed by the + /// user. For example, we may want a [`MonitorEvent::HTLCEvent`] to keep being re-provided to us + /// until after an [`Event::PaymentSent`] is processed. + AckMonitorEvent { event_id: MonitorEventSource }, } impl_writeable_tlv_based_enum!(EventCompletionAction, (0, ReleaseRAAChannelMonitorUpdate) => { @@ -1564,6 +1569,9 @@ impl_writeable_tlv_based_enum!(EventCompletionAction, } ChannelId::v1_from_funding_outpoint(channel_funding_outpoint.unwrap()) })), + }, + (3, AckMonitorEvent) => { + (1, event_id, required), } {1, ReleasePaymentCompleteChannelMonitorUpdate} => (), ); @@ -15375,6 +15383,9 @@ impl< } } }, + EventCompletionAction::AckMonitorEvent { event_id } => { + self.chain_monitor.ack_monitor_event(event_id); + }, } } } From a6f59a4de93c05e70a628d6266af90522c67716d Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 13:56:33 -0400 Subject: [PATCH 17/33] Persistent mon events for off-chain outbound claims Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This will simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. In recent work, we added support for keeping monitor events around until they are explicitly acked by the ChannelManager, but would always ack monitor events immediately, which preserved the previous behavior and didn't break any tests. Up until this point, we only generated HTLC monitor events when a payment was claimed/failed on-chain. In this commit, we start generating persistent monitor events whenever a payment is claimed *off*-chain, specifically when new latest holder commitment data is provided to the monitor. For the purpose of making incremental progress on this feature, these events will be a no-op and/or continue to be acked immediately except in the narrow case of an off-chain outbound payment claim. HTLC forward claim monitor events will be a no-op, and on-chain outbound payment claim events continue to be acked immediately. Off-chain outbound payment claims, however, now have monitor events generated for them that will not be acked by the ChannelManager until the PaymentSent event is processed by the user. This also allows us to stop blocking the RAA monitor update that removes the preimage, because the purpose of that behavior was to ensure the user got a PaymentSent event and the monitor event now serves that purpose instead. --- lightning/src/chain/channelmonitor.rs | 52 ++++++++--- lightning/src/ln/blinded_payment_tests.rs | 4 +- lightning/src/ln/chanmon_update_fail_tests.rs | 25 +++-- lightning/src/ln/channelmanager.rs | 91 ++++++++++++++----- lightning/src/ln/functional_test_utils.rs | 12 ++- lightning/src/ln/functional_tests.rs | 20 +++- lightning/src/ln/invoice_utils.rs | 2 +- lightning/src/ln/monitor_tests.rs | 7 +- lightning/src/ln/outbound_payment.rs | 10 ++ lightning/src/ln/payment_tests.rs | 30 ++++-- lightning/src/ln/quiescence_tests.rs | 33 ++++++- lightning/src/ln/reload_tests.rs | 14 +-- lightning/src/ln/shutdown_tests.rs | 5 +- 13 files changed, 235 insertions(+), 70 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 36953ea0147..a115b0b669f 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2231,6 +2231,10 @@ impl ChannelMonitor { self.inner.lock().unwrap().persistent_events_enabled = enabled; } + pub(crate) fn has_pending_event_for_htlc(&self, source: &HTLCSource) -> bool { + self.inner.lock().unwrap().has_pending_event_for_htlc(source) + } + /// Copies [`MonitorEvent`] state from `other` into `self`. /// Used in tests to align transient runtime state before equality comparison after a /// serialization round-trip. @@ -3824,20 +3828,34 @@ impl ChannelMonitorImpl { self.prev_holder_htlc_data = Some(htlc_data); for (claimed_htlc_id, claimed_preimage) in claimed_htlcs { - #[cfg(debug_assertions)] - { - let cur_counterparty_htlcs = self - .funding - .counterparty_claimable_outpoints - .get(&self.funding.current_counterparty_commitment_txid.unwrap()) - .unwrap(); - assert!(cur_counterparty_htlcs.iter().any(|(_, source_opt)| { + let htlc_opt = self + .funding + .counterparty_claimable_outpoints + .get(&self.funding.current_counterparty_commitment_txid.unwrap()) + .unwrap() + .iter() + .find_map(|(htlc, source_opt)| { if let Some(source) = source_opt { - SentHTLCId::from_source(source) == *claimed_htlc_id - } else { - false + if SentHTLCId::from_source(source) == *claimed_htlc_id { + return Some((htlc, source)); + } } - })); + None + }); + debug_assert!(htlc_opt.is_some()); + if self.persistent_events_enabled { + if let Some((htlc, source)) = htlc_opt { + // If persistent_events_enabled is set, the ChannelMonitor is responsible for providing + // off-chain resolutions of HTLCs to the ChannelManager, will re-provide this event on + // startup until it is explicitly acked. + self.push_monitor_event(MonitorEvent::HTLCEvent(HTLCUpdate { + payment_hash: htlc.payment_hash, + payment_preimage: Some(*claimed_preimage), + source: *source.clone(), + htlc_value_satoshis: Some(htlc.amount_msat), + user_channel_id: self.user_channel_id, + })); + } } self.counterparty_fulfilled_htlcs.insert(*claimed_htlc_id, *claimed_preimage); } @@ -4655,6 +4673,16 @@ impl ChannelMonitorImpl { self.pending_monitor_events.retain(|(id, _)| *id != event_id); } + fn has_pending_event_for_htlc(&self, htlc: &HTLCSource) -> bool { + let htlc_id = SentHTLCId::from_source(htlc); + self.pending_monitor_events.iter().any(|(_, ev)| { + if let MonitorEvent::HTLCEvent(upd) = ev { + return htlc_id == SentHTLCId::from_source(&upd.source); + } + false + }) + } + fn get_and_clear_pending_monitor_events(&mut self) -> Vec<(u64, MonitorEvent)> { if self.persistent_events_enabled { let mut ret = Vec::new(); diff --git a/lightning/src/ln/blinded_payment_tests.rs b/lightning/src/ln/blinded_payment_tests.rs index d62f79957eb..9fd97717bf7 100644 --- a/lightning/src/ln/blinded_payment_tests.rs +++ b/lightning/src/ln/blinded_payment_tests.rs @@ -854,7 +854,7 @@ fn do_blinded_intercept_payment(intercept_node_fails: bool) { do_claim_payment_along_route( ClaimAlongRouteArgs::new(&nodes[0], &[&[&nodes[1], &nodes[2]]], payment_preimage) ); - expect_payment_sent(&nodes[0], payment_preimage, Some(Some(1000)), true, true); + expect_payment_sent!(nodes[0], payment_preimage, Some(1000)); } #[test] @@ -1399,7 +1399,7 @@ fn conditionally_round_fwd_amt() { let mut args = ClaimAlongRouteArgs::new(&nodes[0], &expected_route[..], payment_preimage) .allow_1_msat_fee_overpay(); let expected_fee = pass_claimed_payment_along_route(args); - expect_payment_sent(&nodes[0], payment_preimage, Some(Some(expected_fee)), true, true); + expect_payment_sent!(nodes[0], payment_preimage, Some(expected_fee)); } #[test] diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 4d1451e3ee6..8a0eae3368b 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -2108,7 +2108,7 @@ fn monitor_update_claim_fail_no_response() { let mut bs_updates = get_htlc_update_msgs(&nodes[1], &node_a_id); nodes[0].node.handle_update_fulfill_htlc(node_b_id, bs_updates.update_fulfill_htlcs.remove(0)); do_commitment_signed_dance(&nodes[0], &nodes[1], &bs_updates.commitment_signed, false, false); - expect_payment_sent!(nodes[0], payment_preimage_1); + expect_payment_sent!(&nodes[0], payment_preimage_1); claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2); } @@ -3450,7 +3450,10 @@ fn do_test_blocked_chan_preimage_release(completion_mode: BlockedUpdateComplMode let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); let persister; let new_chain_mon; - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut cfg = test_default_channel_config(); + // If persistent_monitor_events is enabed, monitor updates will never be blocked. + cfg.override_persistent_monitor_events = Some(false); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(cfg.clone()), Some(cfg)]); let nodes_1_reload; let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); @@ -3763,7 +3766,7 @@ fn do_test_inverted_mon_completion_order( ); // Finally, check that the payment was, ultimately, seen as sent by node A. - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } #[test] @@ -4256,7 +4259,7 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) { nodes[0].node.handle_update_fulfill_htlc(node_b_id, update_fulfill); let commitment = &a_update[0].commitment_signed; do_commitment_signed_dance(&nodes[0], &nodes[1], commitment, false, false); - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(nodes[0], payment_preimage); expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false); pass_along_path( @@ -4936,6 +4939,7 @@ fn native_async_persist() { let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let persistent_monitor_events = nodes[0].node.test_persistent_monitor_events_enabled(); let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1); @@ -5081,7 +5085,16 @@ fn native_async_persist() { assert_eq!(update_status, ChannelMonitorUpdateStatus::InProgress); persist_futures.poll_futures(); - assert_eq!(async_chain_monitor.release_pending_monitor_events().len(), 0); + let events = async_chain_monitor.release_pending_monitor_events(); + if persistent_monitor_events { + // With persistent monitor events, the LatestHolderCommitmentTXInfo update containing + // claimed_htlcs generates an HTLCEvent with the preimage. + assert_eq!(events.len(), 1); + assert_eq!(events[0].2.len(), 1); + assert!(matches!(events[0].2[0].1, MonitorEvent::HTLCEvent(..))); + } else { + assert!(events.is_empty()); + } let pending_writes = kv_store.list_pending_async_writes( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, @@ -5223,7 +5236,7 @@ fn test_mpp_claim_to_holding_cell() { let claims = vec![(b_claim_msgs, node_b_id), (c_claim_msgs, node_c_id)]; pass_claimed_payment_along_route_from_ev(250_000, claims, args); - expect_payment_sent(&nodes[0], preimage_1, None, true, true); + expect_payment_sent!(nodes[0], preimage_1); expect_and_process_pending_htlcs(&nodes[3], false); expect_payment_claimable!(nodes[3], paymnt_hash_2, payment_secret_2, 400_000); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 40eac403422..7e04c5ad0f0 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -10022,11 +10022,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }; Some(EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(release)) } else { - Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint: Some(next_channel_outpoint), - channel_id: next_channel_id, - counterparty_node_id: path.hops[0].pubkey, - }) + if self.persistent_monitor_events { + monitor_event_id.map(|event_id| EventCompletionAction::AckMonitorEvent { + event_id: MonitorEventSource { channel_id: next_channel_id, event_id }, + }) + } else { + Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { + channel_funding_outpoint: Some(next_channel_outpoint), + channel_id: next_channel_id, + counterparty_node_id: path.hops[0].pubkey, + }) + } }; let logger = WithContext::for_payment( &self.logger, @@ -10048,6 +10054,25 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ &self.pending_events, &logger, ); + + if matches!( + ev_completion_action, + Some(EventCompletionAction::AckMonitorEvent { .. }) + ) { + // If the `PaymentSent` for this redundant claim is still pending, add the event + // completion action here to ensure the `PaymentSent` will always be regenerated until it + // is processed by the user -- as long as the monitor event corresponding to this + // completion action is not acked, it will continue to be re-provided on startup. + let mut pending_events = self.pending_events.lock().unwrap(); + for (ev, act_opt) in pending_events.iter_mut() { + let found_payment_sent = matches!(ev, Event::PaymentSent { payment_id: Some(id), .. } if *id == payment_id); + if found_payment_sent && act_opt.is_none() { + *act_opt = ev_completion_action.take(); + break; + } + } + } + // If an event was generated, `claim_htlc` set `ev_completion_action` to None, if // not, we should go ahead and run it now (as the claim was duplicative), at least // if a PaymentClaimed event with the same action isn't already pending. @@ -12882,6 +12907,13 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }) } + /// Returns `true` if `ChannelManager::persistent_monitor_events` is enabled. This flag will only + /// be set randomly in tests for now. + #[cfg(any(test, feature = "_test_utils"))] + pub fn test_persistent_monitor_events_enabled(&self) -> bool { + self.persistent_monitor_events + } + #[cfg(any(test, feature = "_test_utils"))] pub(crate) fn test_raa_monitor_updates_held( &self, counterparty_node_id: PublicKey, channel_id: ChannelId, @@ -13588,22 +13620,29 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ .channel_by_id .contains_key(&channel_id) }); - // Claim the funds from the previous hop, if there is one. Because this is in response to a - // chain event, no attribution data is available. - self.claim_funds_internal( - htlc_update.source, - preimage, - htlc_update.htlc_value_satoshis.map(|v| v * 1000), - None, - from_onchain, - counterparty_node_id, - funding_outpoint, - channel_id, - htlc_update.user_channel_id, - None, - None, - Some(event_id), - ); + let we_are_sender = + matches!(htlc_update.source, HTLCSource::OutboundRoute { .. }); + if from_onchain | we_are_sender { + // Claim the funds from the previous hop, if there is one. In the future we can + // store attribution data in the `ChannelMonitor` and provide it here. + self.claim_funds_internal( + htlc_update.source, + preimage, + htlc_update.htlc_value_satoshis.map(|v| v * 1000), + None, + from_onchain, + counterparty_node_id, + funding_outpoint, + channel_id, + htlc_update.user_channel_id, + None, + None, + Some(event_id), + ); + } + if from_onchain | !we_are_sender { + self.chain_monitor.ack_monitor_event(monitor_event_source); + } } else { log_trace!(logger, "Failing HTLC from our monitor"); let failure_reason = LocalHTLCFailureReason::OnChainTimeout; @@ -13623,8 +13662,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ failure_type, completion_update, ); + self.chain_monitor.ack_monitor_event(monitor_event_source); } - self.chain_monitor.ack_monitor_event(monitor_event_source); }, MonitorEvent::HolderForceClosed(_) | MonitorEvent::HolderForceClosedWithInfo { .. } => { @@ -19055,6 +19094,14 @@ impl< break; } } + if persistent_monitor_events { + // This will not be necessary once we have persistent events for HTLC failures, we + // can delete this whole loop and wait to re-process the pending monitor events + // rather than failing them proactively below. + if monitor.has_pending_event_for_htlc(&channel_htlc_source) { + found_htlc = true; + } + } if !found_htlc { // If we have some HTLCs in the channel which are not present in the newer // ChannelMonitor, they have been removed and should be failed back to diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index ceba0c9af3b..3ae7d893c27 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -3074,7 +3074,7 @@ macro_rules! expect_payment_sent { $expected_payment_preimage, $expected_fee_msat_opt.map(|o| Some(o)), $expect_paths, - true, + if $node.node.test_persistent_monitor_events_enabled() { false } else { true }, ) }; } @@ -4235,7 +4235,15 @@ pub fn claim_payment_along_route( do_claim_payment_along_route(args) + expected_extra_total_fees_msat; if !skip_last { - expect_payment_sent!(origin_node, payment_preimage, Some(expected_total_fee_msat)) + let expect_post_ev_mon_update = + if origin_node.node.test_persistent_monitor_events_enabled() { false } else { true }; + expect_payment_sent( + origin_node, + payment_preimage, + Some(Some(expected_total_fee_msat)), + true, + expect_post_ev_mon_update, + ) } else { (None, Vec::new()) } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 8d9df062868..e861ca7e31e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1358,6 +1358,10 @@ pub fn do_test_multiple_package_conflicts(p2a_anchor: bool) { }; assert_eq!(updates.update_fulfill_htlcs.len(), 1); nodes[0].node.handle_update_fulfill_htlc(node_b_id, updates.update_fulfill_htlcs.remove(0)); + if nodes[0].node.test_persistent_monitor_events_enabled() { + // If persistent_monitor_events is enabled, the RAA monitor update is not blocked. + check_added_monitors(&nodes[0], 1); + } do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false); expect_payment_sent!(nodes[0], preimage_2); @@ -2672,7 +2676,10 @@ pub fn test_simple_peer_disconnect() { _ => panic!("Unexpected event"), } } - check_added_monitors(&nodes[0], 1); + check_added_monitors( + &nodes[0], + if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 1 }, + ); claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_4); fail_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_hash_6); @@ -4295,7 +4302,7 @@ pub fn test_duplicate_payment_hash_one_failure_one_success() { nodes[0].node.handle_update_fulfill_htlc(node_b_id, updates.update_fulfill_htlcs.remove(0)); do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false); - expect_payment_sent(&nodes[0], our_payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], our_payment_preimage); } #[xtest(feature = "_externalize_tests")] @@ -8574,7 +8581,9 @@ pub fn test_inconsistent_mpp_params() { pass_along_path(&nodes[0], path_b, real_amt, hash, Some(payment_secret), event, true, None); do_claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path_a, path_b], preimage)); - expect_payment_sent(&nodes[0], preimage, Some(None), true, true); + let expect_post_ev_mon_update = + if nodes[0].node.test_persistent_monitor_events_enabled() { false } else { true }; + expect_payment_sent(&nodes[0], preimage, Some(None), true, expect_post_ev_mon_update); } #[xtest(feature = "_externalize_tests")] @@ -9932,7 +9941,10 @@ fn do_test_multi_post_event_actions(do_reload: bool) { let chanmon_cfgs = create_chanmon_cfgs(3); let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); let (persister, chain_monitor); - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut cfg = test_default_channel_config(); + // If persistent_monitor_events is enabled, RAAs will not be blocked on events. + cfg.override_persistent_monitor_events = Some(false); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(cfg), None, None]); let node_a_reload; let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); diff --git a/lightning/src/ln/invoice_utils.rs b/lightning/src/ln/invoice_utils.rs index 63ad110bba0..7a0c0b2b8db 100644 --- a/lightning/src/ln/invoice_utils.rs +++ b/lightning/src/ln/invoice_utils.rs @@ -1344,7 +1344,7 @@ mod test { &[&[&nodes[fwd_idx]]], payment_preimage, )); - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } #[test] diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 438691b71c7..043ecddf7b5 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -3250,14 +3250,15 @@ fn test_event_replay_causing_monitor_replay() { // Now process the `PaymentSent` to get the final RAA `ChannelMonitorUpdate`, checking that it // resulted in a `ChannelManager` persistence request. nodes[0].node.get_and_clear_needs_persistence(); - expect_payment_sent(&nodes[0], payment_preimage, None, true, true /* expected post-event monitor update*/); + expect_payment_sent!(nodes[0], payment_preimage); assert!(nodes[0].node.get_and_clear_needs_persistence()); let serialized_monitor = get_monitor!(nodes[0], chan.2).encode(); reload_node!(nodes[0], &serialized_channel_manager, &[&serialized_monitor], persister, new_chain_monitor, node_deserialized); // Expect the `PaymentSent` to get replayed, this time without the duplicate monitor update - expect_payment_sent(&nodes[0], payment_preimage, None, false, false /* expected post-event monitor update*/); + let per_path_evs = if nodes[0].node.test_persistent_monitor_events_enabled() { true } else { false }; + expect_payment_sent(&nodes[0], payment_preimage, None, per_path_evs, false /* expected post-event monitor update*/); } #[test] @@ -3555,7 +3556,7 @@ fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool, p2a_anchor: bo let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_args.pending_cell_htlc_claims = (1, 0); reconnect_nodes(reconnect_args); - expect_payment_sent(&nodes[0], preimage_a, None, true, true); + expect_payment_sent!(&nodes[0], preimage_a); } #[test] diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index f0a52d50dc5..f41a4f06459 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2256,6 +2256,16 @@ impl OutboundPayments { { let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); + + if from_onchain { + // If we are re-processing this claim from a `MonitorEvent` and the `ChannelManager` is + // outdated and has no idea about the payment, we may need to re-insert here to ensure a + // `PaymentSent` event gets generated. + self.insert_from_monitor_on_startup( + payment_id, payment_preimage.into(), session_priv_bytes, &path, best_block_height, logger + ); + } + let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let mut pending_events = pending_events.lock().unwrap(); if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index a196e5135c2..a4a68536073 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2405,7 +2405,11 @@ fn do_test_intercepted_payment(test: InterceptTest) { }, _ => panic!("Unexpected event"), } - check_added_monitors(&nodes[0], 1); + if nodes[0].node.test_persistent_monitor_events_enabled() { + check_added_monitors(&nodes[0], 0); + } else { + check_added_monitors(&nodes[0], 1); + } } else if test == InterceptTest::Timeout { let mut block = create_dummy_block(nodes[0].best_block_hash(), 42, Vec::new()); connect_block(&nodes[0], &block); @@ -2610,7 +2614,7 @@ fn do_accept_underpaying_htlcs_config(num_mpp_parts: usize) { let total_fee_msat = pass_claimed_payment_along_route(args); // The sender doesn't know that the penultimate hop took an extra fee. let amt = total_fee_msat - skimmed_fee_msat * num_mpp_parts as u64; - expect_payment_sent(&nodes[0], payment_preimage, Some(Some(amt)), true, true); + expect_payment_sent!(nodes[0], payment_preimage, Some(amt)); } #[derive(PartialEq)] @@ -4791,7 +4795,7 @@ fn do_test_custom_tlvs_consistency( ClaimAlongRouteArgs::new(&nodes[0], &[path_a, &[&nodes[2], &nodes[3]]], preimage) .with_custom_tlvs(expected_tlvs), ); - expect_payment_sent(&nodes[0], preimage, Some(Some(2000)), true, true); + expect_payment_sent!(nodes[0], preimage, Some(2000)); } else { // Expect fail back let expected_destinations = [HTLCHandlingFailureType::Receive { payment_hash: hash }]; @@ -5589,7 +5593,10 @@ fn do_bolt11_multi_node_mpp(use_bolt11_pay: bool) { } let payment_sent = nodes[0].node.get_and_clear_pending_events(); - check_added_monitors(&nodes[0], 1); + check_added_monitors( + &nodes[0], + if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 1 }, + ); assert_eq!(payment_sent.len(), 2, "{payment_sent:?}"); if let Event::PaymentSent { payment_id, payment_hash, amount_msat, fee_paid_msat, .. } = @@ -5618,7 +5625,10 @@ fn do_bolt11_multi_node_mpp(use_bolt11_pay: bool) { } let payment_sent = nodes[1].node.get_and_clear_pending_events(); - check_added_monitors(&nodes[1], 1); + check_added_monitors( + &nodes[1], + if nodes[1].node.test_persistent_monitor_events_enabled() { 0 } else { 1 }, + ); assert_eq!(payment_sent.len(), 2, "{payment_sent:?}"); if let Event::PaymentSent { payment_id, payment_hash, amount_msat, fee_paid_msat, .. } = @@ -5874,7 +5884,10 @@ fn bolt11_multi_node_mpp_with_retry() { } let payment_sent_a = nodes[0].node.get_and_clear_pending_events(); - check_added_monitors(&nodes[0], 1); + check_added_monitors( + &nodes[0], + if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 1 }, + ); assert_eq!(payment_sent_a.len(), 2, "{payment_sent_a:?}"); if let Event::PaymentSent { payment_id, payment_hash, amount_msat, .. } = &payment_sent_a[0] { @@ -5899,7 +5912,10 @@ fn bolt11_multi_node_mpp_with_retry() { } let payment_sent_b = nodes[1].node.get_and_clear_pending_events(); - check_added_monitors(&nodes[1], 1); + check_added_monitors( + &nodes[1], + if nodes[1].node.test_persistent_monitor_events_enabled() { 0 } else { 1 }, + ); assert_eq!(payment_sent_b.len(), 2, "{payment_sent_b:?}"); if let Event::PaymentSent { payment_id, payment_hash, amount_msat, .. } = &payment_sent_b[0] { diff --git a/lightning/src/ln/quiescence_tests.rs b/lightning/src/ln/quiescence_tests.rs index 495a1622522..aa587e4fa8c 100644 --- a/lightning/src/ln/quiescence_tests.rs +++ b/lightning/src/ln/quiescence_tests.rs @@ -259,7 +259,7 @@ fn test_quiescence_waits_for_async_signer_and_monitor_update() { assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); // We have two updates pending: - { + if !nodes[0].node.test_persistent_monitor_events_enabled() { let test_chain_mon = &nodes[0].chain_monitor; let (_, latest_update) = test_chain_mon.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone(); @@ -280,6 +280,27 @@ fn test_quiescence_waits_for_async_signer_and_monitor_update() { } else { panic!("{events:?}"); } + } else { + // In the persistent monitor events path, we don't block the RAA monitor update, so + // `expect_post_ev_mon_update` is false here + expect_payment_sent(&nodes[0], preimage, None, false, false); + // The latest commitment transaction update from the the `revoke_and_ack` was previously + // blocked via `InProgress`, so update that here, which will unblock the commitment secret + // update from the RAA + let test_chain_mon = &nodes[0].chain_monitor; + let (_, latest_update) = + test_chain_mon.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone(); + let chain_monitor = &nodes[0].chain_monitor.chain_monitor; + // Complete the held update, which releases the commitment secret update, which releases the + // `PaymentPathSuccessful` event + chain_monitor.channel_monitor_updated(chan_id, latest_update).unwrap(); + check_added_monitors(&nodes[0], 1); + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + if let Event::PaymentPathSuccessful { .. } = &events[0] { + } else { + panic!("{events:?}"); + } } // With the updates completed, we can now become quiescent. @@ -438,7 +459,10 @@ fn quiescence_updates_go_to_holding_cell(fail_htlc: bool) { } else { assert!(events.iter().find(|e| matches!(e, Event::PaymentSent { .. })).is_some()); assert!(events.iter().find(|e| matches!(e, Event::PaymentPathSuccessful { .. })).is_some()); - check_added_monitors(&nodes[0], 1); + check_added_monitors( + &nodes[0], + if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 1 }, + ); } nodes[0].node.process_pending_htlc_forwards(); expect_payment_claimable!(nodes[0], payment_hash1, payment_secret1, payment_amount); @@ -467,7 +491,7 @@ fn quiescence_updates_go_to_holding_cell(fail_htlc: bool) { let conditions = PaymentFailedConditions::new(); expect_payment_failed_conditions(&nodes[1], payment_hash1, true, conditions); } else { - expect_payment_sent(&nodes[1], payment_preimage1, None, true, true); + expect_payment_sent!(&nodes[1], payment_preimage1); } } @@ -673,6 +697,9 @@ fn do_test_quiescence_during_disconnection(with_pending_claim: bool, propose_dis assert_eq!(bs_raa_stfu.len(), 2); if let MessageSendEvent::SendRevokeAndACK { msg, .. } = &bs_raa_stfu[0] { nodes[0].node.handle_revoke_and_ack(node_b_id, &msg); + if nodes[0].node.test_persistent_monitor_events_enabled() { + check_added_monitors(&nodes[0], 1); + } expect_payment_sent!(&nodes[0], preimage); } else { panic!("Unexpected first message {bs_raa_stfu:?}"); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 9e992467ecd..16ae8380d26 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -1296,7 +1296,7 @@ fn do_manager_persisted_pre_outbound_edge_forward(intercept_htlc: bool) { expect_payment_claimable!(nodes[2], payment_hash, payment_secret, amt_msat, None, nodes[2].node.get_our_node_id()); let path: &[&[_]] = &[&[&nodes[1], &nodes[2]]]; do_claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], path, payment_preimage)); - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } #[test] @@ -1354,7 +1354,7 @@ fn test_manager_persisted_post_outbound_edge_forward() { expect_payment_claimable!(nodes[2], payment_hash, payment_secret, amt_msat, None, nodes[2].node.get_our_node_id()); let path: &[&[_]] = &[&[&nodes[1], &nodes[2]]]; do_claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], path, payment_preimage)); - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(nodes[0], payment_preimage); } #[test] @@ -1458,7 +1458,7 @@ fn test_manager_persisted_post_outbound_edge_holding_cell() { // Claim the a->b->c payment on node_c. let path: &[&[_]] = &[&[&nodes[1], &nodes[2]]]; do_claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], path, payment_preimage)); - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); // Claim the c->b payment on node_b. nodes[1].node.claim_funds(payment_preimage_2); @@ -1467,7 +1467,7 @@ fn test_manager_persisted_post_outbound_edge_holding_cell() { let mut update = get_htlc_update_msgs(&nodes[1], &nodes[2].node.get_our_node_id()); nodes[2].node.handle_update_fulfill_htlc(nodes[1].node.get_our_node_id(), update.update_fulfill_htlcs.remove(0)); do_commitment_signed_dance(&nodes[2], &nodes[1], &update.commitment_signed, false, false); - expect_payment_sent(&nodes[2], payment_preimage_2, None, true, true); + expect_payment_sent!(&nodes[2], payment_preimage_2); } #[test] @@ -1879,7 +1879,7 @@ fn outbound_removed_holding_cell_resolved_no_double_forward() { reconnect_nodes(reconnect_args); // nodes[0] should now have received the fulfill and generate PaymentSent. - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } #[test] @@ -1983,7 +1983,7 @@ fn test_reload_node_with_preimage_in_monitor_claims_htlc() { reconnect_nodes(reconnect_args); // nodes[0] should now have received the fulfill and generate PaymentSent. - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } #[test] @@ -2244,5 +2244,5 @@ fn test_reload_with_mpp_claims_on_same_channel() { reconnect_nodes(reconnect_args); // nodes[0] should now have received both fulfills and generate PaymentSent. - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index d70b240e4e4..5363b9b3b82 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -1864,7 +1864,10 @@ fn test_pending_htlcs_arent_lost_on_mon_delay() { let chanmon_cfgs = create_chanmon_cfgs(3); let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut cfg = test_default_channel_config(); + // When persistent_monitor_events is enabled, monitor updates will never be blocked. + cfg.override_persistent_monitor_events = Some(false); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(cfg), None]); let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); let node_a_id = nodes[0].node.get_our_node_id(); From 8c4df02b01877203b0c3b8543195b31fc28a2cb5 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 16:27:30 -0400 Subject: [PATCH 18/33] Filter claims from get_onchain_failed_htlcs return value This isn't a bug at the moment because a claim in this situation would already be filtered out due to its inclusion in htlcs_resolved_to_user. However, when we stop issuing ReleasePaymentComplete monitor updates for claims in upcoming commits, HTLC claims will no longer be in htlcs_resolved_to_user when get_onchain_failed_htlcs checks. --- lightning/src/chain/channelmonitor.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a115b0b669f..66681466069 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3300,7 +3300,11 @@ impl ChannelMonitor { // The HTLC was not included in the confirmed commitment transaction, // which has now reached ANTI_REORG_DELAY confirmations and thus the // HTLC has been failed. - res.insert(source.clone(), candidate_htlc.payment_hash); + // However, if we have the preimage, the HTLC was fulfilled off-chain + // and should not be reported as failed. + if !us.counterparty_fulfilled_htlcs.contains_key(&htlc_id) { + res.insert(source.clone(), candidate_htlc.payment_hash); + } } } }; From 591144b977ea6fd13a26427e37f4e1ffb6d69466 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 16:45:45 -0400 Subject: [PATCH 19/33] Persistent monitor events for onchain outbound claims Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This will simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. In recent work, we added support for keeping monitor events around until they are explicitly acked by the ChannelManager, but would always ack monitor events immediately, which preserved the previous behavior and didn't break any tests. Here we start acking monitor events for on-chain HTLC claims when the user processes the PaymentSent event, if the persistent_monitor_events feature is enabled. This allows us to stop issuing ReleasePaymentComplete monitor updates for onchain payment claims, because the purpose of that behavior is to ensure we won't keep repeatedly issuing PaymentSent events, and the monitor event and acking it on PaymentSent processing now serves that purpose instead. --- lightning/src/ln/chanmon_update_fail_tests.rs | 2 +- lightning/src/ln/channelmanager.rs | 37 +++++++++++------ lightning/src/ln/functional_tests.rs | 5 ++- lightning/src/ln/monitor_tests.rs | 26 ++++++------ lightning/src/ln/payment_tests.rs | 40 +++++++++++-------- lightning/src/ln/reorg_tests.rs | 7 +++- lightning/src/ln/splicing_tests.rs | 7 +++- 7 files changed, 78 insertions(+), 46 deletions(-) diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 8a0eae3368b..0b1a9d0ca2c 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -3954,7 +3954,7 @@ fn do_test_durable_preimages_on_closed_channel( mine_transactions(&nodes[0], &[&as_closing_tx[0], bs_preimage_tx]); check_closed_broadcast(&nodes[0], 1, false); - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); if close_chans_before_reload && !hold_post_reload_mon_update { // For close_chans_before_reload with hold=false, the deferred completions diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7e04c5ad0f0..c10a067b1ed 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -10013,7 +10013,18 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ "We don't support claim_htlc claims during startup - monitors may not be available yet"); debug_assert_eq!(next_channel_counterparty_node_id, path.hops[0].pubkey); - let mut ev_completion_action = if from_onchain { + let mut ev_completion_action = if self.persistent_monitor_events { + // If persistent monitor events is enabled: + // * If the channel is on-chain, we don't need to use ReleasePaymentComplete like we do + // below because we will stop hearing about this payment after the relevant monitor event + // is acked + // * If the channel is open, we don't need to block the preimage-removing monitor update + // like we do below because we will keep hearing about the preimage until we explicitly + // ack the monitor event for this payment + monitor_event_id.map(|event_id| EventCompletionAction::AckMonitorEvent { + event_id: MonitorEventSource { channel_id: next_channel_id, event_id }, + }) + } else if from_onchain { let release = PaymentCompleteUpdate { counterparty_node_id: next_channel_counterparty_node_id, channel_funding_outpoint: next_channel_outpoint, @@ -10022,17 +10033,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }; Some(EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(release)) } else { - if self.persistent_monitor_events { - monitor_event_id.map(|event_id| EventCompletionAction::AckMonitorEvent { - event_id: MonitorEventSource { channel_id: next_channel_id, event_id }, - }) - } else { - Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint: Some(next_channel_outpoint), - channel_id: next_channel_id, - counterparty_node_id: path.hops[0].pubkey, - }) - } + Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { + channel_funding_outpoint: Some(next_channel_outpoint), + channel_id: next_channel_id, + counterparty_node_id: path.hops[0].pubkey, + }) }; let logger = WithContext::for_payment( &self.logger, @@ -13640,7 +13645,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Some(event_id), ); } - if from_onchain | !we_are_sender { + if !we_are_sender { self.chain_monitor.ack_monitor_event(monitor_event_source); } } else { @@ -19730,6 +19735,12 @@ impl< .. } => { if let Some(preimage) = preimage_opt { + if persistent_monitor_events { + // If persistent_monitor_events is enabled, then the monitor will keep + // providing us with a monitor event for this claim until the ChannelManager + // explicitly marks it as resolved, no need to explicitly handle it here. + continue; + } let pending_events = Mutex::new(pending_events_read); let update = PaymentCompleteUpdate { counterparty_node_id: monitor.get_counterparty_node_id(), diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index e861ca7e31e..bf5c54c86b6 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1629,7 +1629,10 @@ pub fn test_htlc_on_chain_success() { check_closed_broadcast(&nodes[0], 1, true); check_added_monitors(&nodes[0], 1); let events = nodes[0].node.get_and_clear_pending_events(); - check_added_monitors(&nodes[0], 2); + check_added_monitors( + &nodes[0], + if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 2 }, + ); assert_eq!(events.len(), 5); let mut first_claimed = false; for event in events { diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 043ecddf7b5..5dca3c4a582 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -274,7 +274,7 @@ fn archive_fully_resolved_monitors() { // Finally, we process the pending `MonitorEvent` from nodes[0], allowing the `ChannelMonitor` // to be archived `ARCHIVAL_DELAY_BLOCKS` blocks later. - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors(); assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1); connect_blocks(&nodes[0], ARCHIVAL_DELAY_BLOCKS - 1); @@ -747,9 +747,9 @@ fn do_test_claim_value_force_close(keyed_anchors: bool, p2a_anchor: bool, prev_c mine_transaction(&nodes[0], &b_broadcast_txn[0]); if prev_commitment_tx { expect_payment_path_successful!(nodes[0]); - check_added_monitors(&nodes[0], 1); + check_added_monitors(&nodes[0], if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 1 }); } else { - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } assert_eq!(sorted_vec(vec![sent_htlc_balance.clone(), sent_htlc_timeout_balance.clone()]), sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(chan_id).unwrap().get_claimable_balances())); @@ -1015,7 +1015,7 @@ fn do_test_balances_on_local_commitment_htlcs(keyed_anchors: bool, p2a_anchor: b // Now confirm nodes[1]'s HTLC claim, giving nodes[0] the preimage. Note that the "maybe // claimable" balance remains until we see ANTI_REORG_DELAY blocks. mine_transaction(&nodes[0], &bs_htlc_claim_txn[0]); - expect_payment_sent(&nodes[0], payment_preimage_2, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage_2); assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { amount_satoshis: 1_000_000 - 10_000 - 20_000 - commitment_tx_fee - anchor_outputs_value, confirmation_height: node_a_commitment_claimable, @@ -2097,7 +2097,7 @@ fn do_test_revoked_counterparty_aggregated_claims(keyed_anchors: bool, p2a_ancho as_revoked_txn[1].clone() }; mine_transaction(&nodes[1], &htlc_success_claim); - expect_payment_sent(&nodes[1], claimed_payment_preimage, None, true, true); + expect_payment_sent!(&nodes[1], claimed_payment_preimage); let mut claim_txn_2 = nodes[1].tx_broadcaster.txn_broadcast(); // Once B sees the HTLC-Success transaction it splits its claim transaction into two, though in @@ -3547,9 +3547,13 @@ fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool, p2a_anchor: bo // After the background events are processed in `get_and_clear_pending_events`, above, node B // will create the requisite `ChannelMontiorUpdate` for claiming the forwarded payment back. // The HTLC, however, is added to the holding cell for replay after the peer connects, below. - // It will also apply a `ChannelMonitorUpdate` to let the `ChannelMonitor` know that the - // payment can now be forgotten as the `PaymentSent` event was handled. - check_added_monitors(&nodes[1], 2); + if nodes[1].node.test_persistent_monitor_events_enabled() { + check_added_monitors(&nodes[1], 1); + } else { + // It will also apply a `ChannelMonitorUpdate` to let the `ChannelMonitor` know that the + // payment can now be forgotten as the `PaymentSent` event was handled. + check_added_monitors(&nodes[1], 2); + } nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); @@ -3943,8 +3947,7 @@ fn test_ladder_preimage_htlc_claims() { connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_sent(&nodes[0], payment_preimage1, None, true, false); - check_added_monitors(&nodes[0], 1); + expect_payment_sent!(&nodes[0], payment_preimage1); nodes[1].node.claim_funds(payment_preimage2); expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000); @@ -3966,6 +3969,5 @@ fn test_ladder_preimage_htlc_claims() { connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_sent(&nodes[0], payment_preimage2, None, true, false); - check_added_monitors(&nodes[0], 1); + expect_payment_sent!(&nodes[0], payment_preimage2); } diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index a4a68536073..a9a9998b1b0 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -957,7 +957,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { assert_eq!(txn[0].compute_txid(), as_commitment_tx.compute_txid()); } mine_transaction(&nodes[0], &bs_htlc_claim_txn); - expect_payment_sent(&nodes[0], payment_preimage_1, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage_1); connect_blocks(&nodes[0], TEST_FINAL_CLTV * 4 + 20); let (first_htlc_timeout_tx, second_htlc_timeout_tx) = { let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); @@ -1368,7 +1368,7 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( let conditions = PaymentFailedConditions::new().from_mon_update(); expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); } else { - expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_sent!(&nodes[0], payment_preimage); } // Note that if we persist the monitor before processing the events, above, we'll always get // them replayed on restart no matter what @@ -1406,18 +1406,22 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( } else { expect_payment_sent(&nodes[0], payment_preimage, None, true, false); } - if persist_manager_post_event { - // After reload, the ChannelManager identified the failed payment and queued up the - // PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we - // already did that) and corresponding ChannelMonitorUpdate to mark the payment - // handled, but while processing the pending `MonitorEvent`s (which were not processed - // before the monitor was persisted) we will end up with a duplicate - // ChannelMonitorUpdate. - check_added_monitors(&nodes[0], 2); + if !nodes[0].node.test_persistent_monitor_events_enabled() { + if persist_manager_post_event { + // After reload, the ChannelManager identified the failed payment and queued up the + // PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we + // already did that) and corresponding ChannelMonitorUpdate to mark the payment + // handled, but while processing the pending `MonitorEvent`s (which were not processed + // before the monitor was persisted) we will end up with a duplicate + // ChannelMonitorUpdate. + check_added_monitors(&nodes[0], 2); + } else { + // ...unless we got the PaymentSent event, in which case we have de-duplication logic + // preventing a redundant monitor event. + check_added_monitors(&nodes[0], 1); + } } else { - // ...unless we got the PaymentSent event, in which case we have de-duplication logic - // preventing a redundant monitor event. - check_added_monitors(&nodes[0], 1); + check_added_monitors(&nodes[0], 0); } } @@ -4198,10 +4202,14 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: check_added_monitors(&nodes[0], 0); nodes[0].node.test_process_background_events(); check_added_monitors(&nodes[0], 1); - // Then once we process the PaymentSent event we'll apply a monitor update to remove the - // pending payment from being re-hydrated on the next startup. let events = nodes[0].node.get_and_clear_pending_events(); - check_added_monitors(&nodes[0], 1); + if nodes[0].node.test_persistent_monitor_events_enabled() { + check_added_monitors(&nodes[0], 0); + } else { + // Once we process the PaymentSent event we'll apply a monitor update to remove the + // pending payment from being re-hydrated on the next startup. + check_added_monitors(&nodes[0], 1); + } assert_eq!(events.len(), 3, "{events:?}"); if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[0] { } else { diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 5b5160148d7..b99c5aa936a 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -254,7 +254,7 @@ fn test_counterparty_revoked_reorg() { // Connect the HTLC claim transaction for HTLC 3 mine_transaction(&nodes[1], &unrevoked_local_txn[2]); - expect_payment_sent(&nodes[1], payment_preimage_3, None, true, true); + expect_payment_sent!(&nodes[1], payment_preimage_3); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); // Connect blocks to confirm the unrevoked commitment transaction @@ -1228,7 +1228,10 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool, p2a check_added_monitors(&nodes[0], 0); let sent_events = nodes[0].node.get_and_clear_pending_events(); - check_added_monitors(&nodes[0], 2); + check_added_monitors( + &nodes[0], + if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 2 }, + ); assert_eq!(sent_events.len(), 4, "{sent_events:?}"); let mut found_expected_events = [false, false, false, false]; for event in sent_events { diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index 9adccd17627..e92cd245045 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -1984,7 +1984,12 @@ fn do_test_splice_commitment_broadcast(splice_status: SpliceStatus, claim_htlcs: 2 ); } - check_added_monitors(&nodes[0], 2); // Two `ReleasePaymentComplete` monitor updates + let expected_mons = if nodes[0].node.test_persistent_monitor_events_enabled() && claim_htlcs { + 0 + } else { + 2 // Two `ReleasePaymentComplete` monitor updates + }; + check_added_monitors(&nodes[0], expected_mons); // When the splice never confirms and we see a commitment transaction broadcast and confirm for // the current funding instead, we should expect to see an `Event::DiscardFunding` for the From a4c34892b37a2683d08927574c5e642c5110fcf8 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 3 Apr 2026 23:41:39 -0400 Subject: [PATCH 20/33] claim_funds api: Event -> ForwardEventContents Use new ForwardEventContents struct as the return value of the closure passed to claim_funds_from_htlc_forward_hop. Useful as upcoming commits will add a struct that wants to contain a forward event specifically, not just any Event. --- lightning/src/ln/channelmanager.rs | 54 +++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c10a067b1ed..f082f76ebf9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -51,7 +51,7 @@ use crate::chain::channelmonitor::{ use crate::chain::transaction::{OutPoint, TransactionData}; use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Confirm, Watch}; use crate::events::{ - self, ClosureReason, Event, EventHandler, EventsProvider, HTLCHandlingFailureType, + self, ClosureReason, Event, EventHandler, EventsProvider, HTLCHandlingFailureType, HTLCLocator, InboundChannelFunds, PaymentFailureReason, ReplayEvent, }; use crate::events::{FundingInfo, PaidBolt12Invoice}; @@ -1502,6 +1502,40 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, }, ); +/// Contents of an [`Event::PaymentForwarded`], useful for parent structs to contain a forward +/// event specifically. +#[derive(Debug)] +pub(crate) struct ForwardEventContents { + prev_htlcs: Vec, + next_htlcs: Vec, + total_fee_earned_msat: Option, + skimmed_fee_msat: Option, + claim_from_onchain_tx: bool, + outbound_amount_forwarded_msat: Option, +} + +impl From for Event { + fn from(contents: ForwardEventContents) -> Self { + Event::PaymentForwarded { + prev_htlcs: contents.prev_htlcs, + next_htlcs: contents.next_htlcs, + total_fee_earned_msat: contents.total_fee_earned_msat, + skimmed_fee_msat: contents.skimmed_fee_msat, + claim_from_onchain_tx: contents.claim_from_onchain_tx, + outbound_amount_forwarded_msat: contents.outbound_amount_forwarded_msat, + } + } +} + +impl_writeable_tlv_based!(ForwardEventContents, { + (1, prev_htlcs, required_vec), + (3, next_htlcs, required_vec), + (5, total_fee_earned_msat, option), + (7, skimmed_fee_msat, option), + (9, claim_from_onchain_tx, required), + (11, outbound_amount_forwarded_msat, option), +}); + /// Result of attempting to resume a channel after a monitor update completes while locks are held. /// Contains remaining work to be processed after locks are released. #[must_use] @@ -9545,7 +9579,7 @@ impl< /// single [`Event::PaymentForwarded`] event that represents the forward. fn claim_funds_from_htlc_forward_hop( &self, payment_preimage: PaymentPreimage, - make_payment_forwarded_event: impl FnOnce(Option) -> Option, + make_payment_forwarded_event: impl FnOnce(Option) -> Option, startup_replay: bool, next_channel_counterparty_node_id: PublicKey, next_channel_outpoint: OutPoint, next_channel_id: ChannelId, hop_data: HTLCPreviousHopData, attribution_data: Option, send_timestamp: Option, @@ -9643,15 +9677,9 @@ impl< ) } else { let event = make_payment_forwarded_event(htlc_claim_value_msat); - if let Some(ref payment_forwarded) = event { - debug_assert!(matches!( - payment_forwarded, - &events::Event::PaymentForwarded { .. } - )); - } ( Some(MonitorUpdateCompletionAction::EmitEventOptionAndFreeOtherChannel { - event, + event: event.map(|ev| ev.into()), downstream_counterparty_and_funding_outpoint: chan_to_release, }), None, @@ -10095,7 +10123,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let prev_htlcs = vec![events::HTLCLocator::from(&hop_data)]; self.claim_funds_from_htlc_forward_hop( payment_preimage, - |htlc_claim_value_msat: Option| -> Option { + |htlc_claim_value_msat: Option| -> Option { let total_fee_earned_msat = if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat { if let Some(claimed_htlc_value) = htlc_claim_value_msat { @@ -10111,7 +10139,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ "skimmed_fee_msat must always be included in total_fee_earned_msat" ); - Some(events::Event::PaymentForwarded { + Some(ForwardEventContents { prev_htlcs, next_htlcs: vec![events::HTLCLocator { channel_id: next_channel_id, @@ -10140,9 +10168,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ for (i, current_previous_hop_data) in previous_hop_data.into_iter().enumerate() { self.claim_funds_from_htlc_forward_hop( payment_preimage, - |_: Option| -> Option { + |_: Option| -> Option { if i == 0 { - Some(events::Event::PaymentForwarded { + Some(ForwardEventContents { prev_htlcs: prev_htlcs.clone(), // TODO: When trampoline payments are tracked in our // pending_outbound_payments, we'll be able to provide all the From 09d5aead978e4c6986dc6883152e19dcf7789a3a Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 01:29:21 -0400 Subject: [PATCH 21/33] Thread monitor event id to claim_mpp_part claim_mpp_part will use this event id in future commits when generating monitor update completion actions to ack the event corresponding to the id. --- lightning/src/ln/channelmanager.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f082f76ebf9..0f66e9c5c7a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -9535,6 +9535,7 @@ impl< payment_preimage, payment_info.clone(), Some(attribution_data), + None, |_, definitely_duplicate| { debug_assert!( !definitely_duplicate, @@ -9583,6 +9584,7 @@ impl< startup_replay: bool, next_channel_counterparty_node_id: PublicKey, next_channel_outpoint: OutPoint, next_channel_id: ChannelId, hop_data: HTLCPreviousHopData, attribution_data: Option, send_timestamp: Option, + monitor_event_id: Option, ) { let _prev_channel_id = hop_data.channel_id; let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data); @@ -9606,6 +9608,8 @@ impl< payment_preimage, None, Some(attribution_data), + monitor_event_id + .map(|event_id| MonitorEventSource { event_id, channel_id: next_channel_id }), |htlc_claim_value_msat, definitely_duplicate| { let chan_to_release = EventUnblockedChannel { counterparty_node_id: next_channel_counterparty_node_id, @@ -9697,7 +9701,7 @@ impl< >( &self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage, payment_info: Option, attribution_data: Option, - completion_action: ComplFunc, + monitor_event_id: Option, completion_action: ComplFunc, ) { let counterparty_node_id = prev_hop.counterparty_node_id.or_else(|| { let short_to_chan_info = self.short_to_chan_info.read().unwrap(); @@ -9723,6 +9727,7 @@ impl< payment_preimage, payment_info, attribution_data, + monitor_event_id, completion_action, ) } @@ -9735,7 +9740,7 @@ impl< >( &self, prev_hop: HTLCClaimSource, payment_preimage: PaymentPreimage, payment_info: Option, attribution_data: Option, - completion_action: ComplFunc, + monitor_event_id: Option, completion_action: ComplFunc, ) { //TODO: Delay the claimed_funds relaying just like we do outbound relay! @@ -10159,6 +10164,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ hop_data, attribution_data, send_timestamp, + monitor_event_id, ); }, HTLCSource::TrampolineForward { previous_hop_data, .. } => { @@ -10202,6 +10208,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ current_previous_hop_data, attribution_data.clone(), send_timestamp, + monitor_event_id, ); } }, @@ -20429,6 +20436,7 @@ impl< payment_preimage, None, None, + None, |_, _| { ( Some(MonitorUpdateCompletionAction::PaymentClaimed { From 80ea61250b985f7280362df0a5e0eff2b0e9b558 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 01:33:31 -0400 Subject: [PATCH 22/33] Pass monitor event id when claiming in Channel Channel will store this event id in upcoming commits, to allow the monitor event corresponding to the id to be acked after the HTLC is removed via revoke_and_ack. --- lightning/src/ln/channel.rs | 17 +++++++++++++---- lightning/src/ln/channelmanager.rs | 1 + 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 485d3c320d5..4ecd21f2bcb 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -30,6 +30,7 @@ use crate::blinded_path::message::BlindedMessagePath; use crate::chain::chaininterface::{ ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator, TransactionType, }; +use crate::chain::chainmonitor::MonitorEventSource; use crate::chain::channelmonitor::{ ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, CommitmentHTLCData, LATENCY_GRACE_PERIOD_BLOCKS, @@ -7482,8 +7483,14 @@ where // (see equivalent if condition there). assert!(!self.context.channel_state.can_generate_new_commitment()); let mon_update_id = self.context.latest_monitor_update_id; // Forget the ChannelMonitor update - let fulfill_resp = - self.get_update_fulfill_htlc(htlc_id_arg, payment_preimage_arg, None, None, logger); + let fulfill_resp = self.get_update_fulfill_htlc( + htlc_id_arg, + payment_preimage_arg, + None, + None, + None, + logger, + ); self.context.latest_monitor_update_id = mon_update_id; if let UpdateFulfillFetch::NewClaim { update_blocked, .. } = fulfill_resp { assert!(update_blocked); // The HTLC must have ended up in the holding cell. @@ -7493,7 +7500,7 @@ where fn get_update_fulfill_htlc( &mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, payment_info: Option, attribution_data: Option, - logger: &L, + monitor_event_id: Option, logger: &L, ) -> UpdateFulfillFetch { // Either ChannelReady got set (which means it won't be unset) or there is no way any // caller thought we could have something claimed (cause we wouldn't have accepted in an @@ -7644,7 +7651,7 @@ where pub fn get_update_fulfill_htlc_and_commit( &mut self, htlc_id: u64, payment_preimage: PaymentPreimage, payment_info: Option, attribution_data: Option, - logger: &L, + monitor_event_id: Option, logger: &L, ) -> UpdateFulfillCommitFetch { let release_cs_monitor = self.context.blocked_monitor_updates.is_empty(); match self.get_update_fulfill_htlc( @@ -7652,6 +7659,7 @@ where payment_preimage, payment_info, attribution_data, + monitor_event_id, logger, ) { UpdateFulfillFetch::NewClaim { @@ -8800,6 +8808,7 @@ where *payment_preimage, None, attribution_data.clone(), + None, logger, ); let mut additional_monitor_update = diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0f66e9c5c7a..b4d429b10dd 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -9777,6 +9777,7 @@ impl< payment_preimage, payment_info, attribution_data, + monitor_event_id, &&logger, ); From e447acd433cb5ca3cd9f3dd970c4825d55031bc2 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 16:05:48 -0400 Subject: [PATCH 23/33] Cache monitor event id in holding cell claims We don't actually need to write this field to disk because monitor events will be re-provided on startup. Will be used in upcoming commits to ack the monitor events corresponding to these ids after the htlcs are removed via revoke_and_ack. --- lightning/src/ln/channel.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 4ecd21f2bcb..9334fff20a2 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -548,6 +548,7 @@ enum HTLCUpdateAwaitingACK { payment_preimage: PaymentPreimage, attribution_data: Option, htlc_id: u64, + monitor_event_id: Option, }, FailHTLC { htlc_id: u64, @@ -7612,6 +7613,7 @@ where payment_preimage: payment_preimage_arg, htlc_id: htlc_id_arg, attribution_data, + monitor_event_id, }); return UpdateFulfillFetch::NewClaim { monitor_update, @@ -8792,6 +8794,7 @@ where ref payment_preimage, htlc_id, ref attribution_data, + monitor_event_id, } => { // If an HTLC claim was previously added to the holding cell (via // `get_update_fulfill_htlc`, then generating the claim message itself must @@ -8808,7 +8811,7 @@ where *payment_preimage, None, attribution_data.clone(), - None, + monitor_event_id, logger, ); let mut additional_monitor_update = @@ -15576,6 +15579,7 @@ impl Writeable for FundedChannel { ref payment_preimage, ref htlc_id, ref attribution_data, + monitor_event_id: _, // Will be re-provided on startup } => { 1u8.write(writer)?; payment_preimage.write(writer)?; @@ -16030,6 +16034,7 @@ impl<'a, 'b, 'c, ES: EntropySource, SP: SignerProvider> payment_preimage: Readable::read(reader)?, htlc_id: Readable::read(reader)?, attribution_data: None, + monitor_event_id: None, }, 2 => HTLCUpdateAwaitingACK::FailHTLC { htlc_id: Readable::read(reader)?, @@ -17569,6 +17574,7 @@ mod tests { payment_preimage: PaymentPreimage([42; 32]), htlc_id: 0, attribution_data, + monitor_event_id: None, }; let dummy_holding_cell_failed_htlc = |htlc_id, attribution_data| HTLCUpdateAwaitingACK::FailHTLC { From 46275f74a556f039f1c8c7c8e942a87a9c34d081 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 20:34:37 -0400 Subject: [PATCH 24/33] Cache monitor event id with inbound removed htlcs We don't actually need to write this field to disk because monitor events will be re-provided on startup. Will be used in upcoming commits to ack the monitor events corresponding to these ids after the htlcs are removed via revoke_and_ack. --- lightning/src/ln/channel.rs | 112 ++++++++++++++++++++---------------- 1 file changed, 64 insertions(+), 48 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9334fff20a2..8bee607c20a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -231,7 +231,7 @@ enum InboundHTLCState { /// ChannelMonitor::should_broadcast_holder_commitment_txn) so we actually remove the HTLC from /// our own local state before then, once we're sure that the next commitment_signed and /// ChannelMonitor::provide_latest_local_commitment_tx will not include this HTLC. - LocalRemoved(InboundHTLCRemovalReason), + LocalRemoved { reason: InboundHTLCRemovalReason, monitor_event_id: Option }, } impl From<&InboundHTLCState> for Option { @@ -245,15 +245,18 @@ impl From<&InboundHTLCState> for Option { Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd) }, InboundHTLCState::Committed { .. } => Some(InboundHTLCStateDetails::Committed), - InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(_)) => { - Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail) - }, - InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed { .. }) => { - Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail) - }, - InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill { .. }) => { - Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill) - }, + InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::FailRelay(_), + .. + } => Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail), + InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::FailMalformed { .. }, + .. + } => Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail), + InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::Fulfill { .. }, + .. + } => Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill), } } } @@ -266,7 +269,7 @@ impl fmt::Display for InboundHTLCState { InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => write!(f, "AwaitingRemoteRevokeToAnnounce"), InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => write!(f, "AwaitingAnnouncedRemoteRevoke"), InboundHTLCState::Committed { .. } => write!(f, "Committed"), - InboundHTLCState::LocalRemoved(_) => write!(f, "LocalRemoved"), + InboundHTLCState::LocalRemoved { .. } => write!(f, "LocalRemoved"), } } } @@ -278,15 +281,16 @@ impl InboundHTLCState { InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local, InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true, InboundHTLCState::Committed { .. } => true, - InboundHTLCState::LocalRemoved(_) => !generated_by_local, + InboundHTLCState::LocalRemoved { .. } => !generated_by_local, } } fn preimage(&self) -> Option { match self { - InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill { - preimage, .. - }) => Some(*preimage), + InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::Fulfill { preimage, .. }, + .. + } => Some(*preimage), _ => None, } } @@ -305,7 +309,7 @@ impl InboundHTLCState { }, InboundHTLCResolution::Resolved { .. } => false, }, - InboundHTLCState::Committed { .. } | InboundHTLCState::LocalRemoved(_) => false, + InboundHTLCState::Committed { .. } | InboundHTLCState::LocalRemoved { .. } => false, } } } @@ -4636,7 +4640,7 @@ impl ChannelContext { .any(|htlc| match htlc.state { InboundHTLCState::Committed { .. } => false, // An HTLC removal from the local node is pending on the remote commitment. - InboundHTLCState::LocalRemoved(_) => true, + InboundHTLCState::LocalRemoved { .. } => true, // An HTLC add from the remote node is pending on the local commitment. InboundHTLCState::RemoteAnnounced(_) | InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) @@ -5147,8 +5151,8 @@ impl ChannelContext { (InboundHTLCState::AwaitingRemoteRevokeToAnnounce(..), _) => true, (InboundHTLCState::AwaitingAnnouncedRemoteRevoke(..), _) => true, (InboundHTLCState::Committed { .. }, _) => true, - (InboundHTLCState::LocalRemoved(..), true) => true, - (InboundHTLCState::LocalRemoved(..), false) => false, + (InboundHTLCState::LocalRemoved { .. }, true) => true, + (InboundHTLCState::LocalRemoved { .. }, false) => false, }) .map(|&InboundHTLCOutput { amount_msat, .. }| HTLCAmountDirection { outbound: false, @@ -5220,8 +5224,8 @@ impl ChannelContext { .pending_inbound_htlcs .iter() .filter(|InboundHTLCOutput { state, .. }| match (state, local) { - (InboundHTLCState::LocalRemoved(Fulfill { .. }), true) => false, - (InboundHTLCState::LocalRemoved(Fulfill { .. }), false) => true, + (InboundHTLCState::LocalRemoved { reason: Fulfill { .. }, .. }, true) => false, + (InboundHTLCState::LocalRemoved { reason: Fulfill { .. }, .. }, false) => true, _ => false, }) .map(|InboundHTLCOutput { amount_msat, .. }| amount_msat) @@ -6918,7 +6922,10 @@ impl FailHTLCContents for msgs::OnionErrorPacket { } } fn to_inbound_htlc_state(self) -> InboundHTLCState { - InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(self)) + InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::FailRelay(self), + monitor_event_id: None, + } } fn to_htlc_update_awaiting_ack(self, htlc_id: u64) -> HTLCUpdateAwaitingACK { HTLCUpdateAwaitingACK::FailHTLC { htlc_id, err_packet: self } @@ -6935,10 +6942,13 @@ impl FailHTLCContents for ([u8; 32], u16) { } } fn to_inbound_htlc_state(self) -> InboundHTLCState { - InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed { - sha256_of_onion: self.0, - failure_code: self.1, - }) + InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::FailMalformed { + sha256_of_onion: self.0, + failure_code: self.1, + }, + monitor_event_id: None, + } } fn to_htlc_update_awaiting_ack(self, htlc_id: u64) -> HTLCUpdateAwaitingACK { HTLCUpdateAwaitingACK::FailMalformedHTLC { @@ -7531,7 +7541,7 @@ where ); match htlc.state { InboundHTLCState::Committed { .. } => {}, - InboundHTLCState::LocalRemoved(ref reason) => { + InboundHTLCState::LocalRemoved { ref reason, .. } => { if let &InboundHTLCRemovalReason::Fulfill { .. } = reason { } else { log_warn!(logger, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", &htlc.payment_hash, &self.context.channel_id()); @@ -7641,10 +7651,13 @@ where "Upgrading HTLC {} to LocalRemoved with a Fulfill!", &htlc.payment_hash, ); - htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill { - preimage: payment_preimage_arg.clone(), - attribution_data, - }); + htlc.state = InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::Fulfill { + preimage: payment_preimage_arg.clone(), + attribution_data, + }, + monitor_event_id, + }; } UpdateFulfillFetch::NewClaim { monitor_update, htlc_value_msat, update_blocked: false } @@ -7753,7 +7766,7 @@ where if htlc.htlc_id == htlc_id_arg { match htlc.state { InboundHTLCState::Committed { .. } => {}, - InboundHTLCState::LocalRemoved(_) => { + InboundHTLCState::LocalRemoved { .. } => { return Err(ChannelError::Ignore(format!("HTLC {} was already resolved", htlc.htlc_id))); }, _ => { @@ -9030,7 +9043,7 @@ where // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug) pending_inbound_htlcs.retain(|htlc| { - if let &InboundHTLCState::LocalRemoved(ref reason) = &htlc.state { + if let &InboundHTLCState::LocalRemoved { ref reason, .. } = &htlc.state { log_trace!(logger, " ...removing inbound LocalRemoved {}", &htlc.payment_hash); if let &InboundHTLCRemovalReason::Fulfill { .. } = reason { value_to_self_msat_diff += htlc.amount_msat as i64; @@ -9099,20 +9112,23 @@ where require_commitment = true; match fail_msg { HTLCFailureMsg::Relay(msg) => { - htlc.state = InboundHTLCState::LocalRemoved( - InboundHTLCRemovalReason::FailRelay( + htlc.state = InboundHTLCState::LocalRemoved { + reason: InboundHTLCRemovalReason::FailRelay( msg.clone().into(), ), - ); + monitor_event_id: None, + }; update_fail_htlcs.push(msg) }, HTLCFailureMsg::Malformed(msg) => { - htlc.state = InboundHTLCState::LocalRemoved( - InboundHTLCRemovalReason::FailMalformed { - sha256_of_onion: msg.sha256_of_onion, - failure_code: msg.failure_code, - }, - ); + htlc.state = InboundHTLCState::LocalRemoved { + reason: + InboundHTLCRemovalReason::FailMalformed { + sha256_of_onion: msg.sha256_of_onion, + failure_code: msg.failure_code, + }, + monitor_event_id: None, + }; update_fail_malformed_htlcs.push(msg) }, } @@ -9597,7 +9613,7 @@ where true }, InboundHTLCState::Committed { .. } => true, - InboundHTLCState::LocalRemoved(_) => { + InboundHTLCState::LocalRemoved { .. } => { // We (hopefully) sent a commitment_signed updating this HTLC (which we can // re-transmit if needed) and they may have even sent a revoke_and_ack back // (that we missed). Keep this around for now and if they tell us they missed @@ -10104,7 +10120,7 @@ where } for htlc in self.context.pending_inbound_htlcs.iter() { - if let &InboundHTLCState::LocalRemoved(ref reason) = &htlc.state { + if let &InboundHTLCState::LocalRemoved { ref reason, .. } = &htlc.state { match reason { &InboundHTLCRemovalReason::FailRelay(ref err_packet) => { update_fail_htlcs.push(msgs::UpdateFailHTLC { @@ -15454,9 +15470,9 @@ impl Writeable for FundedChannel { 3u8.write(writer)?; inbound_committed_update_adds.push(update_add_htlc); }, - &InboundHTLCState::LocalRemoved(ref removal_reason) => { + &InboundHTLCState::LocalRemoved { ref reason, monitor_event_id: _ } => { 4u8.write(writer)?; - match removal_reason { + match reason { InboundHTLCRemovalReason::FailRelay(msgs::OnionErrorPacket { data, attribution_data, @@ -15944,7 +15960,7 @@ impl<'a, 'b, 'c, ES: EntropySource, SP: SignerProvider> }, _ => return Err(DecodeError::InvalidValue), }; - InboundHTLCState::LocalRemoved(reason) + InboundHTLCState::LocalRemoved { reason, monitor_event_id: None } }, _ => return Err(DecodeError::InvalidValue), }, @@ -16447,7 +16463,7 @@ impl<'a, 'b, 'c, ES: EntropySource, SP: SignerProvider> } if let Some(attribution_data_list) = removed_htlc_attribution_data { let mut removed_htlcs = pending_inbound_htlcs.iter_mut().filter_map(|status| { - if let InboundHTLCState::LocalRemoved(reason) = &mut status.state { + if let InboundHTLCState::LocalRemoved { ref mut reason, .. } = &mut status.state { match reason { InboundHTLCRemovalReason::FailRelay(ref mut packet) => { Some(&mut packet.attribution_data) From f73aa935fc07dd1e86acc7fdc9a40b57226494e7 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 18:56:07 -0400 Subject: [PATCH 25/33] Ack monitor events post-initial CommitmentSecret If we stored a monitor event id with a pending inbound HTLC and that HTLC is about to be fully removed from the channel via revoke_and_ack, we should ack the monitor event corresponding to that id when the monitor update associated with the RAA is complete. We do this so the monitor event will keep being re-provided to us on startup until the HTLC is removed, to ensure the HTLC gets resolved even if we lose the holding cell. --- lightning/src/ln/channel.rs | 17 ++++++++++++++--- lightning/src/ln/channelmanager.rs | 25 ++++++++++++++++++++++++- lightning/src/util/ser.rs | 1 + 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8bee607c20a..c15441fcac7 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -8926,7 +8926,7 @@ where ( Vec<(HTLCSource, PaymentHash)>, Vec<(StaticInvoice, BlindedMessagePath)>, - Option, + Option<(ChannelMonitorUpdate, Vec)>, ), ChannelError, > { @@ -9033,6 +9033,7 @@ where let mut static_invoices = Vec::new(); let mut require_commitment = false; let mut value_to_self_msat_diff: i64 = 0; + let mut monitor_events_to_ack = Vec::new(); { // Take references explicitly so that we can hold multiple references to self.context. @@ -9043,12 +9044,17 @@ where // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug) pending_inbound_htlcs.retain(|htlc| { - if let &InboundHTLCState::LocalRemoved { ref reason, .. } = &htlc.state { + if let &InboundHTLCState::LocalRemoved { ref reason, monitor_event_id, .. } = + &htlc.state + { log_trace!(logger, " ...removing inbound LocalRemoved {}", &htlc.payment_hash); if let &InboundHTLCRemovalReason::Fulfill { .. } = reason { value_to_self_msat_diff += htlc.amount_msat as i64; } *expecting_peer_commitment_signed = true; + if let Some(id) = monitor_event_id { + monitor_events_to_ack.push(id); + } false } else { true @@ -9242,13 +9248,18 @@ where }; macro_rules! return_with_htlcs_to_fail { ($htlcs_to_fail: expr) => { + let events_to_ack = core::mem::take(&mut monitor_events_to_ack); if !release_monitor { self.context .blocked_monitor_updates .push(PendingChannelMonitorUpdate { update: monitor_update }); return Ok(($htlcs_to_fail, static_invoices, None)); } else { - return Ok(($htlcs_to_fail, static_invoices, Some(monitor_update))); + return Ok(( + $htlcs_to_fail, + static_invoices, + Some((monitor_update, events_to_ack)), + )); } }; } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b4d429b10dd..8bf4e1d05da 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1479,6 +1479,12 @@ pub(crate) enum MonitorUpdateCompletionAction { blocking_action: RAAMonitorUpdateBlockingAction, downstream_channel_id: ChannelId, }, + /// Indicates we should ack the set of monitor event ids via [`Watch::ack_monitor_event`]. + /// + /// This is generated when [`ChannelManager::persistent_monitor_events`] is enabled and we want + /// to avoid acking a monitor event until after an HTLC is fully removed via revoke_and_ack, to + /// ensure that the HTLC gets resolved even if we lose the holding cell. + AckMonitorEvents { event_ids: Vec }, } impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, @@ -1500,6 +1506,9 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, (0, event, upgradable_option), (1, downstream_counterparty_and_funding_outpoint, upgradable_required), }, + (3, AckMonitorEvents) => { + (1, event_ids, required_vec), + } ); /// Contents of an [`Event::PaymentForwarded`], useful for parent structs to contain a forward @@ -10457,6 +10466,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Some(blocking_action), ); }, + MonitorUpdateCompletionAction::AckMonitorEvents { event_ids } => { + for id in event_ids { + self.chain_monitor.ack_monitor_event(id); + } + }, } } @@ -12992,7 +13006,16 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ *counterparty_node_id); let (htlcs_to_fail, static_invoices, monitor_update_opt) = try_channel_entry!(self, peer_state, chan.revoke_and_ack(&msg, &self.fee_estimator, &&logger, mon_update_blocked), chan_entry); - if let Some(monitor_update) = monitor_update_opt { + if let Some((monitor_update, monitor_events_to_ack)) = monitor_update_opt { + if !monitor_events_to_ack.is_empty() { + peer_state + .monitor_update_blocked_actions + .entry(msg.channel_id) + .or_default() + .push(MonitorUpdateCompletionAction::AckMonitorEvents { + event_ids: monitor_events_to_ack, + }); + } let funding_txo = funding_txo_opt .expect("Funding outpoint must have been set for RAA handling to succeed"); if let Some(data) = self.handle_new_monitor_update( diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 2b02629d3b0..5c5bb6ac7f2 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -1095,6 +1095,7 @@ impl Readable for Vec { impl_for_vec!(ecdsa::Signature); impl_for_vec!(crate::chain::channelmonitor::ChannelMonitorUpdate); +impl_for_vec!(crate::chain::chainmonitor::MonitorEventSource); impl_for_vec!(crate::ln::channelmanager::MonitorUpdateCompletionAction); impl_for_vec!(crate::ln::channelmanager::PaymentClaimDetails); impl_for_vec!(crate::ln::msgs::SocketAddress); From 0ece3d6935b569c87858bde27aefd862d4050698 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 20:52:17 -0400 Subject: [PATCH 26/33] Ack monitor events if CommitmentSecret was blocked If we stored a monitor event id with a pending inbound HTLC and that HTLC is about to be fully removed from the channel via revoke_and_ack, we should ack the monitor event corresponding to that id when the monitor update associated with the RAA is complete. We do this so the monitor event will keep being re-provided to us on startup until the HTLC is removed, to ensure the HTLC gets resolved even if we lose the holding cell. --- lightning/src/ln/channel.rs | 32 +++++++++++++++++++----------- lightning/src/ln/channelmanager.rs | 11 +++++++++- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c15441fcac7..a358a3fa8e5 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1480,10 +1480,18 @@ pub(crate) const CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY: u32 = 144; #[derive(Debug)] struct PendingChannelMonitorUpdate { update: ChannelMonitorUpdate, + monitor_events_to_ack: Vec, +} + +impl PendingChannelMonitorUpdate { + fn new(update: ChannelMonitorUpdate) -> Self { + Self { update, monitor_events_to_ack: Vec::new() } + } } impl_writeable_tlv_based!(PendingChannelMonitorUpdate, { (0, update, required), + (1, monitor_events_to_ack, optional_vec), }); /// A payment channel with a counterparty throughout its life-cycle, encapsulating negotiation and @@ -7707,7 +7715,7 @@ where let update = self.build_commitment_no_status_check(logger); self.context .blocked_monitor_updates - .push(PendingChannelMonitorUpdate { update }); + .push(PendingChannelMonitorUpdate::new(update)); } } @@ -9250,9 +9258,10 @@ where ($htlcs_to_fail: expr) => { let events_to_ack = core::mem::take(&mut monitor_events_to_ack); if !release_monitor { - self.context - .blocked_monitor_updates - .push(PendingChannelMonitorUpdate { update: monitor_update }); + self.context.blocked_monitor_updates.push(PendingChannelMonitorUpdate { + update: monitor_update, + monitor_events_to_ack: events_to_ack, + }); return Ok(($htlcs_to_fail, static_invoices, None)); } else { return Ok(( @@ -11347,14 +11356,15 @@ where /// Returns the next blocked monitor update, if one exists, and a bool which indicates a /// further blocked monitor update exists after the next. - pub fn unblock_next_blocked_monitor_update(&mut self) -> Option<(ChannelMonitorUpdate, bool)> { + pub fn unblock_next_blocked_monitor_update( + &mut self, + ) -> Option<(ChannelMonitorUpdate, Vec, bool)> { if self.context.blocked_monitor_updates.is_empty() { return None; } - Some(( - self.context.blocked_monitor_updates.remove(0).update, - !self.context.blocked_monitor_updates.is_empty(), - )) + let PendingChannelMonitorUpdate { update, monitor_events_to_ack } = + self.context.blocked_monitor_updates.remove(0); + Some((update, monitor_events_to_ack, !self.context.blocked_monitor_updates.is_empty())) } /// Pushes a new monitor update into our monitor update queue, returning it if it should be @@ -11364,9 +11374,7 @@ where -> Option { let release_monitor = self.context.blocked_monitor_updates.is_empty(); if !release_monitor { - self.context.blocked_monitor_updates.push(PendingChannelMonitorUpdate { - update, - }); + self.context.blocked_monitor_updates.push(PendingChannelMonitorUpdate::new(update)); None } else { Some(update) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8bf4e1d05da..50144183d3d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -15372,9 +15372,18 @@ impl< channel_id) { if let Some(chan) = chan_entry.get_mut().as_funded_mut() { let channel_funding_outpoint = chan.funding_outpoint(); - if let Some((monitor_update, further_update_exists)) = chan.unblock_next_blocked_monitor_update() { + if let Some((monitor_update, mon_events_to_ack, further_update_exists)) = chan.unblock_next_blocked_monitor_update() { log_debug!(logger, "Unlocking monitor updating and updating monitor", ); + if !mon_events_to_ack.is_empty() { + peer_state + .monitor_update_blocked_actions + .entry(channel_id) + .or_default() + .push(MonitorUpdateCompletionAction::AckMonitorEvents { + event_ids: mon_events_to_ack, + }); + } let post_update_data = self.handle_new_monitor_update( &mut peer_state.in_flight_monitor_updates, &mut peer_state.monitor_update_blocked_actions, From 8fdad850d6b00c2fa0c2041e81e912104644f0c2 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 20:48:27 -0400 Subject: [PATCH 27/33] Update monitor event id on duplicate inbound claim The first time we claim an HTLC in a channel, we may not have an associated monitor event id. Once we later process the monitor event for the claim, we need to update the channel's internal htlc state to include the monitor event id, so the event can be properly acked afer the htlc is removed. --- lightning/src/ln/channel.rs | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a358a3fa8e5..bfdf1ed8aae 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -7535,7 +7535,8 @@ where let mut pending_idx = core::usize::MAX; let mut htlc_value_msat = 0; - for (idx, htlc) in self.context.pending_inbound_htlcs.iter().enumerate() { + let channel_id = self.context.channel_id(); + for (idx, htlc) in self.context.pending_inbound_htlcs.iter_mut().enumerate() { if htlc.htlc_id == htlc_id_arg { let expected_hash = PaymentHash(Sha256::hash(&payment_preimage_arg.0[..]).to_byte_array()); @@ -7549,10 +7550,17 @@ where ); match htlc.state { InboundHTLCState::Committed { .. } => {}, - InboundHTLCState::LocalRemoved { ref reason, .. } => { + InboundHTLCState::LocalRemoved { + ref reason, + monitor_event_id: ref mut id, + .. + } => { + if monitor_event_id.is_some() { + *id = monitor_event_id; + } if let &InboundHTLCRemovalReason::Fulfill { .. } = reason { } else { - log_warn!(logger, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", &htlc.payment_hash, &self.context.channel_id()); + log_warn!(logger, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {channel_id}", &htlc.payment_hash); debug_assert!( false, "Tried to fulfill an HTLC that was already failed" @@ -7593,17 +7601,24 @@ where // `claim_htlc_while_disconnected_dropping_mon_update` and must match exactly - // `claim_htlc_while_disconnected_dropping_mon_update` would not work correctly if we // do not not get into this branch. - for pending_update in self.context.holding_cell_htlc_updates.iter() { + for pending_update in self.context.holding_cell_htlc_updates.iter_mut() { match pending_update { - &HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => { + &mut HTLCUpdateAwaitingACK::ClaimHTLC { + htlc_id, + monitor_event_id: ref mut id, + .. + } => { if htlc_id_arg == htlc_id { // Make sure we don't leave latest_monitor_update_id incremented here: self.context.latest_monitor_update_id -= 1; + if monitor_event_id.is_some() { + *id = monitor_event_id; + } return UpdateFulfillFetch::DuplicateClaim {}; } }, - &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } - | &HTLCUpdateAwaitingACK::FailMalformedHTLC { htlc_id, .. } => { + &mut HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } + | &mut HTLCUpdateAwaitingACK::FailMalformedHTLC { htlc_id, .. } => { if htlc_id_arg == htlc_id { log_warn!(logger, "Have preimage and want to fulfill HTLC with pending failure against channel {}", &self.context.channel_id()); // TODO: We may actually be able to switch to a fulfill here, though its From 86a23fee300b3a3200a4712a6f0c171bebb933f0 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 4 Apr 2026 23:05:49 -0400 Subject: [PATCH 28/33] Abstract params to make claim mon completion action We'll be adding another parameter to this closure soon. Also this makes it a little clearer what's going on at the callsites. --- lightning/src/ln/channelmanager.rs | 46 +++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 50144183d3d..1bac450c344 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3615,6 +3615,20 @@ impl TrustedChannelFeatures { } } +struct ClaimCompletionActionParams { + definitely_duplicate: bool, + inbound_htlc_value_msat: Option, +} + +impl ClaimCompletionActionParams { + fn new_claim(inbound_htlc_value_msat: u64) -> Self { + Self { definitely_duplicate: false, inbound_htlc_value_msat: Some(inbound_htlc_value_msat) } + } + fn duplicate_claim() -> Self { + Self { definitely_duplicate: true, inbound_htlc_value_msat: None } + } +} + impl< M: chain::Watch, T: BroadcasterInterface, @@ -9545,9 +9559,9 @@ impl< payment_info.clone(), Some(attribution_data), None, - |_, definitely_duplicate| { + |claim_action_params| { debug_assert!( - !definitely_duplicate, + !claim_action_params.definitely_duplicate, "We shouldn't claim duplicatively from a payment" ); ( @@ -9619,7 +9633,9 @@ impl< Some(attribution_data), monitor_event_id .map(|event_id| MonitorEventSource { event_id, channel_id: next_channel_id }), - |htlc_claim_value_msat, definitely_duplicate| { + |claim_completion_action_params| { + let ClaimCompletionActionParams { definitely_duplicate, inbound_htlc_value_msat } = + claim_completion_action_params; let chan_to_release = EventUnblockedChannel { counterparty_node_id: next_channel_counterparty_node_id, funding_txo: next_channel_outpoint, @@ -9689,7 +9705,7 @@ impl< None, ) } else { - let event = make_payment_forwarded_event(htlc_claim_value_msat); + let event = make_payment_forwarded_event(inbound_htlc_value_msat); ( Some(MonitorUpdateCompletionAction::EmitEventOptionAndFreeOtherChannel { event: event.map(|ev| ev.into()), @@ -9704,8 +9720,7 @@ impl< fn claim_funds_from_hop< ComplFunc: FnOnce( - Option, - bool, + ClaimCompletionActionParams, ) -> (Option, Option), >( &self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage, @@ -9743,8 +9758,7 @@ impl< fn claim_mpp_part< ComplFunc: FnOnce( - Option, - bool, + ClaimCompletionActionParams, ) -> (Option, Option), >( &self, prev_hop: HTLCClaimSource, payment_preimage: PaymentPreimage, @@ -9792,8 +9806,9 @@ impl< match fulfill_res { UpdateFulfillCommitFetch::NewClaim { htlc_value_msat, monitor_update } => { - let (action_opt, raa_blocker_opt) = - completion_action(Some(htlc_value_msat), false); + let (action_opt, raa_blocker_opt) = completion_action( + ClaimCompletionActionParams::new_claim(htlc_value_msat), + ); if let Some(action) = action_opt { log_trace!( logger, @@ -9828,7 +9843,9 @@ impl< } }, UpdateFulfillCommitFetch::DuplicateClaim {} => { - let (action_opt, raa_blocker_opt) = completion_action(None, true); + let (action_opt, raa_blocker_opt) = + completion_action(ClaimCompletionActionParams::duplicate_claim()); + if let Some(raa_blocker) = raa_blocker_opt { // If we're making a claim during startup, its a replay of a // payment claim from a `ChannelMonitor`. In some cases (MPP or @@ -9956,7 +9973,10 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ // `ChannelMonitorUpdate` we're about to generate. This may result in a duplicate `Event`, // but note that `Event`s are generally always allowed to be duplicative (and it's // specifically noted in `PaymentForwarded`). - let (action_opt, raa_blocker_opt) = completion_action(None, false); + let (action_opt, raa_blocker_opt) = completion_action(ClaimCompletionActionParams { + definitely_duplicate: false, + inbound_htlc_value_msat: None, + }); if let Some(raa_blocker) = raa_blocker_opt { peer_state @@ -20470,7 +20490,7 @@ impl< None, None, None, - |_, _| { + |_| { ( Some(MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, From 13c5908f96ed17a67c1d5417284ae84bda9f9aef Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sun, 5 Apr 2026 12:57:05 -0400 Subject: [PATCH 29/33] Add MonitorUpdateCompletionAction::EmitForwardEvent Indicates we should emit an Event::PaymentForwarded and possibly ack a monitor event via Watch::ack_monitor_event. This is generated when we've completed an inbound edge preimage update for an HTLC forward, at which point it's safe to generate the forward event. If the inbound edge is closed, a monitor event id may be included so we can tell the outbound edge to stop telling us about the claim once the forward event is processed by the user. --- lightning/src/ln/channelmanager.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1bac450c344..e5006e611a6 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1485,6 +1485,17 @@ pub(crate) enum MonitorUpdateCompletionAction { /// to avoid acking a monitor event until after an HTLC is fully removed via revoke_and_ack, to /// ensure that the HTLC gets resolved even if we lose the holding cell. AckMonitorEvents { event_ids: Vec }, + /// Indicates we should emit an [`Event::PaymentForwarded`] and possibly ack a monitor event via + /// [`Watch::ack_monitor_event`]. + /// + /// This is generated when we've completed an inbound edge preimage update for an HTLC forward, + /// at which point it's safe to generate the forward event. If the inbound edge is closed, a + /// monitor event id may be included so we can tell the outbound edge to stop telling us about + /// the claim once the forward event is processed by the user. + EmitForwardEvent { + event: ForwardEventContents, + post_event_ackable_monitor_event: Option, + }, } impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, @@ -1508,6 +1519,10 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, }, (3, AckMonitorEvents) => { (1, event_ids, required_vec), + }, + (5, EmitForwardEvent) => { + (1, event, required), + (3, post_event_ackable_monitor_event, option), } ); @@ -10491,6 +10506,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ self.chain_monitor.ack_monitor_event(id); } }, + MonitorUpdateCompletionAction::EmitForwardEvent { + event, + post_event_ackable_monitor_event, + } => { + let post_event_action = post_event_ackable_monitor_event + .map(|event_id| EventCompletionAction::AckMonitorEvent { event_id }); + self.pending_events + .lock() + .unwrap() + .push_back((event.into(), post_event_action)); + }, } } From c980152761e217f289a54b03603c7cb9df32143b Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 6 Apr 2026 15:50:59 -0400 Subject: [PATCH 30/33] HTLCUpdate::htlc_value_satoshis -> msats We need this level of precision when generating this event for off-chain claims. One test failed due to us previously overreporting a fee due to this lack of precision, fixed now. --- lightning/src/chain/channelmonitor.rs | 20 ++++++++++---------- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/functional_tests.rs | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 66681466069..46e01d04a62 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -260,15 +260,16 @@ pub struct HTLCUpdate { pub(crate) payment_hash: PaymentHash, pub(crate) payment_preimage: Option, pub(crate) source: HTLCSource, - pub(crate) htlc_value_satoshis: Option, + pub(crate) htlc_value_msat: Option, pub(crate) user_channel_id: Option, } impl_writeable_tlv_based!(HTLCUpdate, { (0, payment_hash, required), - (1, htlc_value_satoshis, option), + (1, htlc_value_satoshis, (legacy, u64, |_| Ok(()), |us: &HTLCUpdate| us.htlc_value_msat.map(|v| v / 1000))), (2, source, required), (4, payment_preimage, option), (5, user_channel_id, option), + (7, htlc_value_msat, (default_value, htlc_value_satoshis.map(|v: u64| v * 1000))), // Added in 0.4 }); /// If an output goes from claimable only by us to claimable by us or our counterparty within this @@ -3856,7 +3857,7 @@ impl ChannelMonitorImpl { payment_hash: htlc.payment_hash, payment_preimage: Some(*claimed_preimage), source: *source.clone(), - htlc_value_satoshis: Some(htlc.amount_msat), + htlc_value_msat: Some(htlc.amount_msat), user_channel_id: self.user_channel_id, })); } @@ -4575,7 +4576,6 @@ impl ChannelMonitorImpl { if self.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).is_some() { continue; } - let htlc_value_satoshis = Some(amount_msat / 1000); let logger = WithContext::from(logger, None, None, Some(payment_hash)); // Defensively mark the HTLC as failed back so the expiry-based failure // path in `block_connected` doesn't generate a duplicate `HTLCUpdate` @@ -4594,7 +4594,7 @@ impl ChannelMonitorImpl { payment_hash, payment_preimage: None, source: source.clone(), - htlc_value_satoshis, + htlc_value_msat: Some(amount_msat), user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id, @@ -4616,7 +4616,7 @@ impl ChannelMonitorImpl { event: OnchainEvent::HTLCUpdate { source: source.clone(), payment_hash, - htlc_value_satoshis, + htlc_value_satoshis: Some(amount_msat / 1000), commitment_tx_output_idx: None, }, }; @@ -5932,7 +5932,7 @@ impl ChannelMonitorImpl { payment_hash, payment_preimage: None, source, - htlc_value_satoshis, + htlc_value_msat: htlc_value_satoshis.map(|v| v * 1000), user_channel_id: self.user_channel_id, })); self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { @@ -6043,7 +6043,7 @@ impl ChannelMonitorImpl { source: source.clone(), payment_preimage: None, payment_hash: htlc.payment_hash, - htlc_value_satoshis: Some(htlc.amount_msat / 1000), + htlc_value_msat: Some(htlc.amount_msat), user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id); } @@ -6461,7 +6461,7 @@ impl ChannelMonitorImpl { source, payment_preimage: Some(payment_preimage), payment_hash, - htlc_value_satoshis: Some(amount_msat / 1000), + htlc_value_msat: Some(amount_msat), user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id); } @@ -6486,7 +6486,7 @@ impl ChannelMonitorImpl { source, payment_preimage: Some(payment_preimage), payment_hash, - htlc_value_satoshis: Some(amount_msat / 1000), + htlc_value_msat: Some(amount_msat), user_channel_id: self.user_channel_id, }), &mut self.next_monitor_event_id); } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index e5006e611a6..c18fe42b574 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13738,7 +13738,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ self.claim_funds_internal( htlc_update.source, preimage, - htlc_update.htlc_value_satoshis.map(|v| v * 1000), + htlc_update.htlc_value_msat, None, from_onchain, counterparty_node_id, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index bf5c54c86b6..01bbff575e9 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -4294,7 +4294,7 @@ pub fn test_duplicate_payment_hash_one_failure_one_success() { // Finally, give node B the HTLC success transaction and ensure it extracts the preimage to // provide to node A. mine_transaction(&nodes[1], htlc_success_tx_to_confirm); - expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(392), true, true); + expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(196), true, true); let mut updates = get_htlc_update_msgs(&nodes[1], &node_a_id); assert!(updates.update_add_htlcs.is_empty()); assert!(updates.update_fail_htlcs.is_empty()); From bd2d404e38e2b1bde5a2b076cd57ff62ca8eccf0 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 6 Apr 2026 20:14:53 -0400 Subject: [PATCH 31/33] Persistent monitor events for HTLC forward claims Currently, the resolution of HTLCs (and decisions on when HTLCs can be forwarded) is the responsibility of Channel objects (a part of ChannelManager) until the channel is closed, and then the ChannelMonitor thereafter. This leads to some complexity around race conditions for HTLCs right around channel closure. Additionally, there is lots of complexity reconstructing the state of all HTLCs in the ChannelManager deserialization/loading logic. Instead, we want to do all resolution in ChannelMonitors (in response to ChannelMonitorUpdates) and pass them back to ChannelManager in the form of MonitorEvents (similar to how HTLCs are resolved after channels are closed). In order to have reliable resolution, we'll need to keep MonitorEvents around in the ChannelMonitor until the ChannelManager has finished processing them. This will simplify things - on restart instead of examining the set of HTLCs in monitors we can simply replay all the pending MonitorEvents. Building on the work of prior commits, here we stop immediately acking monitor events for HTLC claims for forwarded HTLCs, and instead ack the monitor event when the forward is fully resolved and we don't want the monitor event to be re-provided back to us on startup. If the inbound edge channel is on-chain, we'll ack the event when the inbound edge preimage monitor update completes and the user processes a PaymentForwarded event. If the inbound edge is open, we'll ack the event when the inbound HTLC is fully removed via revoke_and_ack (this ensures we'll remember to resolve the htlc off-chain even if we lose the holding cell). --- lightning/src/ln/chanmon_update_fail_tests.rs | 460 +++++++++++++++++- lightning/src/ln/channelmanager.rs | 148 ++++-- lightning/src/ln/functional_test_utils.rs | 10 +- lightning/src/ln/functional_tests.rs | 17 +- lightning/src/ln/payment_tests.rs | 29 +- lightning/src/ln/reload_tests.rs | 7 + lightning/src/util/test_utils.rs | 10 +- 7 files changed, 614 insertions(+), 67 deletions(-) diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 0b1a9d0ca2c..2a3117dca7f 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -3604,7 +3604,10 @@ fn do_test_inverted_mon_completion_order( let chain_mon; let node_b_reload; - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + // We won't block monitor updates if persistent_monitor_events is enabled. + let mut cfg = test_default_channel_config(); + cfg.override_persistent_monitor_events = Some(false); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(cfg), None]); let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); let node_a_id = nodes[0].node.get_our_node_id(); @@ -3777,7 +3780,7 @@ fn test_inverted_mon_completion_order() { do_test_inverted_mon_completion_order(false, false); } -fn do_test_durable_preimages_on_closed_channel( +fn do_test_durable_preimages_on_closed_channel_legacy( close_chans_before_reload: bool, close_only_a: bool, hold_post_reload_mon_update: bool, ) { // Test that we can apply a `ChannelMonitorUpdate` with a payment preimage even if the channel @@ -3796,7 +3799,8 @@ fn do_test_durable_preimages_on_closed_channel( let chain_mon; let node_b_reload; - let legacy_cfg = test_legacy_channel_config(); + let mut legacy_cfg = test_legacy_channel_config(); + legacy_cfg.override_persistent_monitor_events = Some(false); let node_chanmgrs = create_node_chanmgrs( 3, &node_cfgs, @@ -4017,16 +4021,373 @@ fn do_test_durable_preimages_on_closed_channel( } #[test] -fn test_durable_preimages_on_closed_channel() { - do_test_durable_preimages_on_closed_channel(true, true, true); - do_test_durable_preimages_on_closed_channel(true, true, false); - do_test_durable_preimages_on_closed_channel(true, false, true); - do_test_durable_preimages_on_closed_channel(true, false, false); - do_test_durable_preimages_on_closed_channel(false, false, true); - do_test_durable_preimages_on_closed_channel(false, false, false); +fn test_durable_preimages_on_closed_channel_legacy() { + do_test_durable_preimages_on_closed_channel_legacy(true, true, true); + do_test_durable_preimages_on_closed_channel_legacy(true, true, false); + do_test_durable_preimages_on_closed_channel_legacy(true, false, true); + do_test_durable_preimages_on_closed_channel_legacy(true, false, false); + do_test_durable_preimages_on_closed_channel_legacy(false, false, true); + do_test_durable_preimages_on_closed_channel_legacy(false, false, false); +} + +fn do_test_durable_preimages_on_closed_channel( + close_chans_before_reload: bool, close_only_a: bool, +) { + // Test that we can apply a `ChannelMonitorUpdate` with a payment preimage even if the channel + // is force-closed between when we generate the update on reload and when we go to handle the + // update or prior to generating the update at all. + + if !close_chans_before_reload && close_only_a { + // If we're not closing, it makes no sense to "only close A" + panic!(); + } + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + + let persister; + let chain_mon; + let node_b_reload; + + let mut legacy_cfg = test_legacy_channel_config(); + legacy_cfg.override_persistent_monitor_events = Some(true); + let node_chanmgrs = create_node_chanmgrs( + 3, + &node_cfgs, + &[Some(legacy_cfg.clone()), Some(legacy_cfg.clone()), Some(legacy_cfg.clone())], + ); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let node_a_id = nodes[0].node.get_our_node_id(); + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + let chan_id_ab = create_announced_chan_between_nodes(&nodes, 0, 1).2; + let chan_id_bc = create_announced_chan_between_nodes(&nodes, 1, 2).2; + + // Route a payment from A, through B, to C, then claim it on C. Once we pass B the + // `update_fulfill_htlc` we have a monitor update for both of B's channels. We complete the one + // on the B<->C channel but leave the A<->B monitor update pending, then reload B. + let (payment_preimage, payment_hash, ..) = + route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000); + + let mon_ab = get_monitor!(nodes[1], chan_id_ab).encode(); + + nodes[2].node.claim_funds(payment_preimage); + check_added_monitors(&nodes[2], 1); + expect_payment_claimed!(nodes[2], payment_hash, 1_000_000); + + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + let mut cs_updates = get_htlc_update_msgs(&nodes[2], &node_b_id); + nodes[1].node.handle_update_fulfill_htlc(node_c_id, cs_updates.update_fulfill_htlcs.remove(0)); + + // B generates a new monitor update for the A <-> B channel, but doesn't send the new messages + // for it since the monitor update is marked in-progress. + check_added_monitors(&nodes[1], 1); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // Now step the Commitment Signed Dance between B and C forward a bit, ensuring we won't get + // the preimage when the nodes reconnect, at which point we have to ensure we get it from the + // ChannelMonitor. + nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &cs_updates.commitment_signed); + check_added_monitors(&nodes[1], 1); + let _ = get_revoke_commit_msgs(&nodes[1], &node_c_id); + + let mon_bc = get_monitor!(nodes[1], chan_id_bc).encode(); + + if close_chans_before_reload { + if !close_only_a { + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + let message = "Channel force-closed".to_owned(); + nodes[1] + .node + .force_close_broadcasting_latest_txn(&chan_id_bc, &node_c_id, message.clone()) + .unwrap(); + check_closed_broadcast(&nodes[1], 1, true); + let reason = + ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message }; + check_closed_event(&nodes[1], 1, reason, &[node_c_id], 100000); + } + + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + let message = "Channel force-closed".to_owned(); + nodes[1] + .node + .force_close_broadcasting_latest_txn(&chan_id_ab, &node_a_id, message.clone()) + .unwrap(); + check_closed_broadcast(&nodes[1], 1, true); + let reason = + ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message }; + check_closed_event(&nodes[1], 1, reason, &[node_a_id], 100000); + } + + // Now reload node B + let manager_b = nodes[1].node.encode(); + reload_node!(nodes[1], &manager_b, &[&mon_ab, &mon_bc], persister, chain_mon, node_b_reload); + nodes[1].disable_monitor_completeness_assertion(); + nodes[1] + .chain_monitor + .deterministic_mon_events_order + .store(true, core::sync::atomic::Ordering::Release); + + nodes[0].node.peer_disconnected(node_b_id); + nodes[2].node.peer_disconnected(node_b_id); + + if close_chans_before_reload { + // If the channels were already closed, B will rebroadcast its closing transactions here. + let bs_close_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + if close_only_a { + assert_eq!(bs_close_txn.len(), 2); + } else { + assert_eq!(bs_close_txn.len(), 3); + } + } + + let err_msg = "Channel force-closed".to_owned(); + let reason = ClosureReason::HolderForceClosed { + broadcasted_latest_txn: Some(true), + message: err_msg.clone(), + }; + nodes[0].node.force_close_broadcasting_latest_txn(&chan_id_ab, &node_b_id, err_msg).unwrap(); + check_closed_event(&nodes[0], 1, reason, &[node_b_id], 100000); + check_added_monitors(&nodes[0], 1); + let as_closing_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(as_closing_tx.len(), 1); + + // In order to give A's closing transaction to B without processing background events first, + // use the _without_consistency_checks utility method. This is similar to connecting blocks + // during startup prior to the node being full initialized. + mine_transaction_without_consistency_checks(&nodes[1], &as_closing_tx[0]); + + // After a timer tick a payment preimage ChannelMonitorUpdate is applied to the A<->B + // ChannelMonitor (possible twice), even though the channel has since been closed. + check_added_monitors(&nodes[1], 0); + let mons_added = if close_chans_before_reload && close_only_a { 2 } else { 3 }; + if !close_chans_before_reload { + check_closed_broadcast(&nodes[1], 1, false); + let evs = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(evs.len(), 3, "{:?}", evs); + assert!(evs.iter().all(|e| matches!( + e, + Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } + | Event::PaymentForwarded { .. } + ))); + check_added_monitors(&nodes[1], mons_added); + } + nodes[1].node.timer_tick_occurred(); + let expected_mons = if !close_chans_before_reload { 0 } else { mons_added }; + check_added_monitors(&nodes[1], expected_mons); + + // Finally, check that B created a payment preimage transaction and close out the payment. + let bs_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(bs_txn.len(), if close_chans_before_reload && !close_only_a { 2 } else { 1 }); + let bs_preimage_tx = bs_txn + .iter() + .find(|tx| tx.input[0].previous_output.txid == as_closing_tx[0].compute_txid()) + .unwrap(); + check_spends!(bs_preimage_tx, as_closing_tx[0]); + + mine_transactions(&nodes[0], &[&as_closing_tx[0], bs_preimage_tx]); + check_closed_broadcast(&nodes[0], 1, false); + expect_payment_sent!(&nodes[0], payment_preimage); + + if close_chans_before_reload { + // For close_chans_before_reload, the deferred completions haven't been processed + // yet. Trigger process_pending_monitor_events now. + let _ = nodes[1].node.get_and_clear_pending_msg_events(); + // One of the monitor events was an HTLC update from the outbound edge containing the payment + // preimage, resulting in an inbound edge preimage update here. + check_added_monitors(&nodes[1], 1); + } + + if !close_chans_before_reload || close_only_a { + // Make sure the B<->C channel is still alive and well by sending a payment over it. + let mut reconnect_args = ReconnectArgs::new(&nodes[1], &nodes[2]); + reconnect_args.pending_responding_commitment_signed.1 = true; + reconnect_args.pending_raa.1 = true; + + reconnect_nodes(reconnect_args); + } + + // If the A<->B channel was closed before we reload, we'll replay the claim against it on + // reload, causing the `PaymentForwarded` event to get replayed. + let evs = nodes[1].node.get_and_clear_pending_events(); + if !close_chans_before_reload { + // PaymentForwarded already fired during get_and_clear_pending_events above. + assert!(evs.is_empty(), "{:?}", evs); + } else { + assert_eq!(evs.len(), 2, "{:?}", evs); + for ev in evs { + if let Event::PaymentForwarded { claim_from_onchain_tx, next_htlcs, .. } = ev { + if !claim_from_onchain_tx { + assert!(next_htlcs[0].user_channel_id.is_some()) + } + } else { + panic!("Unexpected event: {:?}", ev); + } + } + } + + if !close_chans_before_reload || close_only_a { + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + send_payment(&nodes[1], &[&nodes[2]], 100_000); + } +} + +#[test] +fn test_durable_preimages_on_closed_channel_a() { + do_test_durable_preimages_on_closed_channel(true, true); +} +#[test] +fn test_durable_preimages_on_closed_channel_b() { + do_test_durable_preimages_on_closed_channel(true, false); +} +#[test] +fn test_durable_preimages_on_closed_channel_c() { + do_test_durable_preimages_on_closed_channel(false, false); +} + +#[test] +fn test_reload_mon_update_completion_actions() { + do_test_reload_mon_update_completion_actions(true); + do_test_reload_mon_update_completion_actions(false); } fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) { + // With persistent monitor events, the outbound monitor generates an HTLCEvent when the + // commitment dance includes claimed_htlcs. Test that when the inbound edge's preimage + // update is still in-flight (InProgress), the ChannelManager has the preimage update + // pending and the outbound monitor retains the preimage HTLCEvent. Then reload and verify + // the event is re-provided and properly acked after processing. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + + let persister; + let chain_mon; + let node_b_reload; + + let mut cfg = test_legacy_channel_config(); + cfg.override_persistent_monitor_events = Some(true); + let node_chanmgrs = + create_node_chanmgrs(3, &node_cfgs, &[Some(cfg.clone()), Some(cfg.clone()), Some(cfg)]); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + let chan_id_ab = create_announced_chan_between_nodes(&nodes, 0, 1).2; + let chan_id_bc = create_announced_chan_between_nodes(&nodes, 1, 2).2; + + // Route a payment A -> B -> C, then claim it on C. + let (payment_preimage, payment_hash, ..) = + route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000); + + nodes[2].node.claim_funds(payment_preimage); + check_added_monitors(&nodes[2], 1); + expect_payment_claimed!(nodes[2], payment_hash, 1_000_000); + + // Hold the A<->B preimage monitor update as InProgress. + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + let mut cs_updates = get_htlc_update_msgs(&nodes[2], &node_b_id); + nodes[1].node.handle_update_fulfill_htlc(node_c_id, cs_updates.update_fulfill_htlcs.remove(0)); + check_added_monitors(&nodes[1], 1); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // Complete the B<->C commitment signed dance. + nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &cs_updates.commitment_signed); + check_added_monitors(&nodes[1], 1); + let (bs_raa, bs_cs) = get_revoke_commit_msgs(&nodes[1], &node_c_id); + + nodes[2].node.handle_revoke_and_ack(node_b_id, &bs_raa); + check_added_monitors(&nodes[2], 1); + nodes[2].node.handle_commitment_signed_batch_test(node_b_id, &bs_cs); + check_added_monitors(&nodes[2], 1); + // After handle_commitment_signed, the B<->C monitor has a preimage HTLCEvent from the + // LatestHolderCommitment update with claimed_htlcs, and the ChannelManager should have an + // in-flight PaymentPreimage update on the A<->B channel. + + let cs_final_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, node_b_id); + nodes[1].node.handle_revoke_and_ack(node_c_id, &cs_final_raa); + check_added_monitors(&nodes[1], 1); + + // Reload node B. The A<->B preimage update is still in-progress, and the B<->C monitor + // event hasn't been acked. + let mon_ab = get_monitor!(nodes[1], chan_id_ab).encode(); + let mon_bc = get_monitor!(nodes[1], chan_id_bc).encode(); + let manager_b = nodes[1].node.encode(); + reload_node!(nodes[1], &manager_b, &[&mon_ab, &mon_bc], persister, chain_mon, node_b_reload); + // Without setting this, nodes[1] may or may not generate a redundant preimage monitor update + // depending on what order the monitor events are provided in after restart. + nodes[1] + .chain_monitor + .deterministic_mon_events_order + .store(true, core::sync::atomic::Ordering::Release); + + if close_during_reload { + // Test that we still free the B<->C channel if the A<->B channel closed while we + // reloaded (as learned about during the on-reload block connection). + let msg = "Channel force-closed".to_owned(); + let reason = ClosureReason::HolderForceClosed { + broadcasted_latest_txn: Some(true), + message: msg.clone(), + }; + nodes[0].node.force_close_broadcasting_latest_txn(&chan_id_ab, &node_b_id, msg).unwrap(); + check_added_monitors(&nodes[0], 1); + check_closed_broadcast(&nodes[0], 1, true); + check_closed_event(&nodes[0], 1, reason, &[node_b_id], 100_000); + let as_closing_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(as_closing_tx.len(), 1); + mine_transaction_without_consistency_checks(&nodes[1], &as_closing_tx[0]); + } + + // On reload, the A<->B preimage update should be detected as completed (the monitor has it + // applied). Processing events should re-provide the B<->C monitor's HTLCEvent, trigger the + // claim, and generate the PaymentForwarded event. + let mut events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), if close_during_reload { 2 } else { 1 }); + expect_payment_forwarded( + events.remove(0), + &nodes[1], + &nodes[0], + &nodes[2], + Some(1000), + None, + close_during_reload, + false, + false, + ); + if close_during_reload { + check_added_monitors(&nodes[1], 2); // force close and redundant preimage update + + match events[0] { + Event::ChannelClosed { .. } => {}, + _ => panic!("Expected ChannelClosed event"), + } + check_closed_broadcast(&nodes[1], 1, false); + } + + if !close_during_reload { + // Reconnect and verify channels still work. B has a pending fulfill for A since the + // A<->B preimage update completed during reload. + nodes[0].node.peer_disconnected(node_b_id); + let mut reconnect_ab = ReconnectArgs::new(&nodes[0], &nodes[1]); + reconnect_ab.pending_htlc_claims = (1, 0); + reconnect_nodes(reconnect_ab); + expect_payment_sent!(&nodes[0], payment_preimage); + + nodes[2].node.peer_disconnected(node_b_id); + reconnect_nodes(ReconnectArgs::new(&nodes[1], &nodes[2])); + send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100_000); + } else { + // Verify B<->C still operates fine after A<->B was closed. + nodes[2].node.peer_disconnected(node_b_id); + reconnect_nodes(ReconnectArgs::new(&nodes[1], &nodes[2])); + expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], None, true, false); + send_payment(&nodes[1], &[&nodes[2]], 100_000); + } +} + +fn do_test_reload_mon_update_completion_actions_legacy(close_during_reload: bool) { // Test that if a `ChannelMonitorUpdate` completes but a `ChannelManager` isn't serialized // before restart we run the monitor update completion action on startup. let chanmon_cfgs = create_chanmon_cfgs(3); @@ -4036,8 +4397,13 @@ fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) { let chain_mon; let node_b_reload; - let legacy_cfg = test_legacy_channel_config(); - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(legacy_cfg), None, None]); + // This test relies on RAA blocking behavior which is bypassed when persistent_monitor_events is + // enabled. + let mut legacy_cfg = test_legacy_channel_config(); + legacy_cfg.override_persistent_monitor_events = Some(false); + let mut cfg = test_default_channel_config(); + cfg.override_persistent_monitor_events = Some(false); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(legacy_cfg), Some(cfg), None]); let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); let node_b_id = nodes[1].node.get_our_node_id(); @@ -4142,9 +4508,9 @@ fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) { } #[test] -fn test_reload_mon_update_completion_actions() { - do_test_reload_mon_update_completion_actions(true); - do_test_reload_mon_update_completion_actions(false); +fn test_reload_mon_update_completion_actions_legacy() { + do_test_reload_mon_update_completion_actions_legacy(true); + do_test_reload_mon_update_completion_actions_legacy(false); } fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) { @@ -4202,6 +4568,66 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) { if !hold_chan_a { expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false); send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100_000); + } else if nodes[1].node.test_persistent_monitor_events_enabled() { + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + let (route, payment_hash_2, payment_preimage_2, payment_secret_2) = + get_route_and_payment_hash!(&nodes[1], nodes[2], 1_000_000); + + // With persistent monitor events, the duplicate claim from the HTLCEvent was already + // processed during reconnect, so the B<->C channel is no longer blocked and the + // second payment goes through immediately. + let onion_2 = RecipientOnionFields::secret_only(payment_secret_2, 1_000_000); + let id_2 = PaymentId(payment_hash_2.0); + nodes[1].node.send_payment_with_route(route, payment_hash_2, onion_2, id_2).unwrap(); + check_added_monitors(&nodes[1], 1); + + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + let msg_events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1); + assert!( + matches!(&msg_events[0], MessageSendEvent::UpdateHTLCs { node_id, .. } if *node_id == node_c_id) + ); + let c_update = msg_events.into_iter().next().unwrap(); + + // Complete the A<->B channel preimage persistence, which sends the fulfill to A. + let (ab_update_id, _) = nodes[1].chain_monitor.get_latest_mon_update_id(chan_id_ab); + assert!(nodes[1] + .chain_monitor + .chain_monitor + .channel_monitor_updated(chan_id_ab, ab_update_id) + .is_ok()); + + let msg_events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1); + check_added_monitors(&nodes[1], 0); + let a_update = match &msg_events[0] { + MessageSendEvent::UpdateHTLCs { node_id, updates, .. } => { + assert_eq!(*node_id, node_a_id); + updates.clone() + }, + _ => panic!("Unexpected event"), + }; + + let update_fulfill = a_update.update_fulfill_htlcs[0].clone(); + nodes[0].node.handle_update_fulfill_htlc(node_b_id, update_fulfill); + let commitment = &a_update.commitment_signed; + do_commitment_signed_dance(&nodes[0], &nodes[1], commitment, false, false); + expect_payment_sent!(&nodes[0], payment_preimage); + expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false); + + pass_along_path( + &nodes[1], + &[&nodes[2]], + 1_000_000, + payment_hash_2, + Some(payment_secret_2), + c_update, + true, + None, + ); + claim_payment(&nodes[1], &[&nodes[2]], payment_preimage_2); } else { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); @@ -4527,7 +4953,9 @@ fn test_claim_to_closed_channel_blocks_forwarded_preimage_removal() { // This tests that behavior. let chanmon_cfgs = create_chanmon_cfgs(3); let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); - let legacy_cfg = test_legacy_channel_config(); + let mut legacy_cfg = test_legacy_channel_config(); + // We won't block preimage removal if persistent_monitor_events is enabled. + legacy_cfg.override_persistent_monitor_events = Some(false); let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(legacy_cfg.clone()), Some(legacy_cfg), None]); let nodes = create_network(3, &node_cfgs, &node_chanmgrs); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c18fe42b574..8016a9e78d2 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3633,14 +3633,23 @@ impl TrustedChannelFeatures { struct ClaimCompletionActionParams { definitely_duplicate: bool, inbound_htlc_value_msat: Option, + inbound_edge_closed: bool, } impl ClaimCompletionActionParams { fn new_claim(inbound_htlc_value_msat: u64) -> Self { - Self { definitely_duplicate: false, inbound_htlc_value_msat: Some(inbound_htlc_value_msat) } + Self { + definitely_duplicate: false, + inbound_htlc_value_msat: Some(inbound_htlc_value_msat), + inbound_edge_closed: false, + } } fn duplicate_claim() -> Self { - Self { definitely_duplicate: true, inbound_htlc_value_msat: None } + Self { + definitely_duplicate: true, + inbound_htlc_value_msat: None, + inbound_edge_closed: false, + } } } @@ -9649,8 +9658,11 @@ impl< monitor_event_id .map(|event_id| MonitorEventSource { event_id, channel_id: next_channel_id }), |claim_completion_action_params| { - let ClaimCompletionActionParams { definitely_duplicate, inbound_htlc_value_msat } = - claim_completion_action_params; + let ClaimCompletionActionParams { + definitely_duplicate, + inbound_htlc_value_msat, + inbound_edge_closed, + } = claim_completion_action_params; let chan_to_release = EventUnblockedChannel { counterparty_node_id: next_channel_counterparty_node_id, funding_txo: next_channel_outpoint, @@ -9658,7 +9670,44 @@ impl< blocking_action: completed_blocker, }; - if definitely_duplicate && startup_replay { + if self.persistent_monitor_events { + let monitor_event_source = monitor_event_id.map(|event_id| { + MonitorEventSource { event_id, channel_id: next_channel_id } + }); + // If persistent_monitor_events is enabled, then we'll get a MonitorEvent for this HTLC + // claim re-provided to us until we explicitly ack it. + // * If the inbound edge is closed, then we can ack it when we know the preimage is + // durably persisted there + the user has processed a `PaymentForwarded` event + // * If the inbound edge is open, then we'll ack the monitor event when HTLC has been + // irrevocably removed via revoke_and_ack. This prevents forgetting to claim the HTLC + // backwards if we lose the off-chain HTLC from the holding cell after a restart. + if definitely_duplicate { + if inbound_edge_closed { + if let Some(id) = monitor_event_source { + self.chain_monitor.ack_monitor_event(id); + } + } + (None, None) + } else if let Some(event) = + make_payment_forwarded_event(inbound_htlc_value_msat) + { + let preimage_update_action = + MonitorUpdateCompletionAction::EmitForwardEvent { + event, + post_event_ackable_monitor_event: inbound_edge_closed + .then_some(monitor_event_source) + .flatten(), + }; + (Some(preimage_update_action), None) + } else if inbound_edge_closed { + let preimage_update_action = monitor_event_source.map(|src| { + MonitorUpdateCompletionAction::AckMonitorEvents { event_ids: vec![src] } + }); + (preimage_update_action, None) + } else { + (None, None) + } + } else if definitely_duplicate && startup_replay { // On startup we may get redundant claims which are related to // monitor updates still in flight. In that case, we shouldn't // immediately free, but instead let that monitor update complete @@ -9991,6 +10040,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let (action_opt, raa_blocker_opt) = completion_action(ClaimCompletionActionParams { definitely_duplicate: false, inbound_htlc_value_msat: None, + inbound_edge_closed: true, }); if let Some(raa_blocker) = raa_blocker_opt { @@ -12712,23 +12762,32 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ chan.update_fulfill_htlc(&msg), chan_entry ); - let prev_hops = match &res.0 { - HTLCSource::PreviousHopData(prev_hop) => vec![prev_hop], - HTLCSource::TrampolineForward { previous_hop_data, .. } => { - previous_hop_data.iter().collect() - }, - _ => vec![], - }; - let logger = WithChannelContext::from(&self.logger, &chan.context, None); - for prev_hop in prev_hops { - log_trace!(logger, - "Holding the next revoke_and_ack until the preimage is durably persisted in the inbound edge's ChannelMonitor", - ); - peer_state - .actions_blocking_raa_monitor_updates - .entry(msg.channel_id) - .or_insert_with(Vec::new) - .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(prev_hop)); + // If persistent_monitor_events is enabled, we don't need to block preimage-removing + // monitor updates because we'll get the preimage from monitor events (that are + // guaranteed to be re-provided until they are explicitly acked) rather than from + // polling the monitor's internal state. + if !self.persistent_monitor_events { + let prev_hops = match &res.0 { + HTLCSource::PreviousHopData(prev_hop) => vec![prev_hop], + HTLCSource::TrampolineForward { previous_hop_data, .. } => { + previous_hop_data.iter().collect() + }, + _ => vec![], + }; + let logger = + WithChannelContext::from(&self.logger, &chan.context, None); + for prev_hop in prev_hops { + log_trace!(logger, + "Holding the next revoke_and_ack until the preimage is durably persisted in the inbound edge's ChannelMonitor", + ); + peer_state + .actions_blocking_raa_monitor_updates + .entry(msg.channel_id) + .or_insert_with(Vec::new) + .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data( + prev_hop, + )); + } } // Note that we do not need to push an `actions_blocking_raa_monitor_updates` @@ -13730,29 +13789,22 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ .channel_by_id .contains_key(&channel_id) }); - let we_are_sender = - matches!(htlc_update.source, HTLCSource::OutboundRoute { .. }); - if from_onchain | we_are_sender { - // Claim the funds from the previous hop, if there is one. In the future we can - // store attribution data in the `ChannelMonitor` and provide it here. - self.claim_funds_internal( - htlc_update.source, - preimage, - htlc_update.htlc_value_msat, - None, - from_onchain, - counterparty_node_id, - funding_outpoint, - channel_id, - htlc_update.user_channel_id, - None, - None, - Some(event_id), - ); - } - if !we_are_sender { - self.chain_monitor.ack_monitor_event(monitor_event_source); - } + // Claim the funds from the previous hop, if there is one. In the future we can + // store attribution data in the `ChannelMonitor` and provide it here. + self.claim_funds_internal( + htlc_update.source, + preimage, + htlc_update.htlc_value_msat, + None, + from_onchain, + counterparty_node_id, + funding_outpoint, + channel_id, + htlc_update.user_channel_id, + None, + None, + Some(event_id), + ); } else { log_trace!(logger, "Failing HTLC from our monitor"); let failure_reason = LocalHTLCFailureReason::OnChainTimeout; @@ -20679,6 +20731,12 @@ impl< downstream_user_channel_id, ) in pending_claims_to_replay { + // If persistent_monitor_events is enabled, we don't need to explicitly reclaim HTLCs on + // startup because we can just wait for the relevant MonitorEvents to be re-provided to us + // during runtime. + if channel_manager.persistent_monitor_events { + continue; + } // We use `downstream_closed` in place of `from_onchain` here just as a guess - we // don't remember in the `ChannelMonitor` where we got a preimage from, but if the // channel is closed we just assume that it probably came from an on-chain claim. diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 3ae7d893c27..4388ace8992 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -3166,7 +3166,7 @@ pub fn expect_payment_forwarded>( macro_rules! expect_payment_forwarded { ($node: expr, $prev_node: expr, $next_node: expr, $expected_fee: expr, $upstream_force_closed: expr, $downstream_force_closed: expr) => { let mut events = $node.node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); + assert_eq!(events.len(), 1, "{events:?}"); $crate::ln::functional_test_utils::expect_payment_forwarded( events.pop().unwrap(), &$node, @@ -5673,7 +5673,13 @@ pub fn reconnect_nodes<'a, 'b, 'c, 'd>(args: ReconnectArgs<'a, 'b, 'c, 'd>) { node_a.node.handle_revoke_and_ack(node_b_id, &bs_revoke_and_ack); check_added_monitors( &node_a, - if pending_responding_commitment_signed_dup_monitor.1 { 0 } else { 1 }, + if pending_responding_commitment_signed_dup_monitor.1 + && !node_a.node.test_persistent_monitor_events_enabled() + { + 0 + } else { + 1 + }, ); if !allow_post_commitment_dance_msgs.1 { assert!(node_a.node.get_and_clear_pending_msg_events().is_empty()); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 01bbff575e9..6143f243be8 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7916,7 +7916,15 @@ fn do_test_onchain_htlc_settlement_after_close( _ => panic!("Unexpected event"), }; nodes[1].node.handle_revoke_and_ack(node_c_id, &carol_revocation); - check_added_monitors(&nodes[1], 1); + if nodes[1].node.test_persistent_monitor_events_enabled() { + if broadcast_alice && !go_onchain_before_fulfill { + check_added_monitors(&nodes[1], 1); + } else { + check_added_monitors(&nodes[1], 2); + } + } else { + check_added_monitors(&nodes[1], 1); + } // If this test requires the force-closed channel to not be on-chain until after the fulfill, // here's where we put said channel's commitment tx on-chain. @@ -7950,6 +7958,13 @@ fn do_test_onchain_htlc_settlement_after_close( check_spends!(bob_txn[0], chan_ab.3); } } + if nodes[1].node.test_persistent_monitor_events_enabled() { + if !broadcast_alice || go_onchain_before_fulfill { + // In some cases we'll replay the claim via a MonitorEvent and be unable to detect that it's + // a duplicate since the inbound edge is on-chain. + expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], fee, went_onchain, false); + } + } // Step (6): // Finally, check that Bob broadcasted a preimage-claiming transaction for the HTLC output on the diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index a9a9998b1b0..8065cffdac0 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -927,8 +927,33 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { let fulfill_msg = htlc_fulfill.update_fulfill_htlcs.remove(0); nodes[1].node.handle_update_fulfill_htlc(node_c_id, fulfill_msg); check_added_monitors(&nodes[1], 1); - do_commitment_signed_dance(&nodes[1], &nodes[2], &htlc_fulfill.commitment_signed, false, false); - expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], None, true, false); + { + // Drive the commitment signed dance manually so we can account for the extra monitor + // update when persistent monitor events are enabled. + let persistent = nodes[1].node.test_persistent_monitor_events_enabled(); + nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &htlc_fulfill.commitment_signed); + check_added_monitors(&nodes[1], 1); + let (extra_msg, cs_raa, htlcs) = + do_main_commitment_signed_dance(&nodes[1], &nodes[2], false); + assert!(htlcs.is_empty()); + assert!(extra_msg.is_none()); + // nodes[1] handles nodes[2]'s RAA. When persistent monitor events are enabled, this + // triggers the re-provided outbound monitor event, generating an extra preimage update + // on the (closed) inbound channel. + nodes[1].node.handle_revoke_and_ack(node_c_id, &cs_raa); + check_added_monitors(&nodes[1], if persistent { 2 } else { 1 }); + } + if nodes[1].node.test_persistent_monitor_events_enabled() { + // The re-provided monitor event generates a duplicate PaymentForwarded against the + // closed inbound channel. + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); + for event in events { + assert!(matches!(event, Event::PaymentForwarded { .. })); + } + } else { + expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], None, true, false); + } if confirm_before_reload { let best_block = nodes[0].blocks.lock().unwrap().last().unwrap().clone(); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 16ae8380d26..416914b3167 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -1972,6 +1972,13 @@ fn test_reload_node_with_preimage_in_monitor_claims_htlc() { Some(true) ); + if nodes[1].node.test_persistent_monitor_events_enabled() { + // Polling messages causes us to re-release the unacked HTLC claim monitor event, which + // regenerates a preimage monitor update and forward event below. + let msgs = nodes[1].node.get_and_clear_pending_msg_events(); + assert!(msgs.is_empty()); + } + // When the claim is reconstructed during reload, a PaymentForwarded event is generated. // Fetching events triggers the pending monitor update (adding preimage) to be applied. expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index f4eb6287921..033065a2b96 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -523,6 +523,9 @@ pub struct TestChainMonitor<'a> { pub pause_flush: AtomicBool, /// Buffer of the last 20 monitor updates, most recent first. pub recent_monitor_updates: Mutex>, + /// When set to `true`, `release_pending_monitor_events` sorts events by `ChannelId` to + /// ensure deterministic processing order regardless of HashMap iteration order. + pub deterministic_mon_events_order: AtomicBool, } impl<'a> TestChainMonitor<'a> { pub fn new( @@ -584,6 +587,7 @@ impl<'a> TestChainMonitor<'a> { write_blocker: Mutex::new(None), pause_flush: AtomicBool::new(false), recent_monitor_updates: Mutex::new(Vec::new()), + deterministic_mon_events_order: AtomicBool::new(false), } } @@ -745,7 +749,11 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { let count = self.chain_monitor.pending_operation_count(); self.chain_monitor.flush(count, &self.logger); } - return self.chain_monitor.release_pending_monitor_events(); + let mut events = self.chain_monitor.release_pending_monitor_events(); + if self.deterministic_mon_events_order.load(Ordering::Acquire) { + events.sort_by_key(|(_, channel_id, _, _)| *channel_id); + } + events } fn ack_monitor_event(&self, source: MonitorEventSource) { From 8a860dcd554b5c8809e41b389d986c92ccd88f32 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 24 Mar 2026 14:21:11 -0400 Subject: [PATCH 32/33] Add skimmed fee to monitor updates We're moving towards having the ChannelMonitor be responsible for resolving HTLCs, via MonitorEvents, so we need to start tracking more payment information in monitor updates so the monitor later has access to it. Here we add tracking for the skimmed_fee field for claims. --- lightning/src/chain/channelmonitor.rs | 75 +++++++++++++++++++++++---- lightning/src/ln/channel.rs | 8 ++- lightning/src/util/ser.rs | 1 + 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 46e01d04a62..403cac862a8 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -642,6 +642,21 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent, }, ); +/// Information about an HTLC forward that was claimed via preimage on the outbound edge, included +/// in [`ChannelMonitorUpdateStep`] so the monitor can generate events with the full claim context. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct OutboundHTLCClaim { + pub htlc_id: SentHTLCId, + pub preimage: PaymentPreimage, + pub skimmed_fee_msat: Option, +} + +impl_writeable_tlv_based!(OutboundHTLCClaim, { + (1, htlc_id, required), + (3, preimage, required), + (5, skimmed_fee_msat, option), +}); + #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum ChannelMonitorUpdateStep { LatestHolderCommitmentTXInfo { @@ -653,13 +668,13 @@ pub(crate) enum ChannelMonitorUpdateStep { /// `htlc_outputs` will only include dust HTLCs. We still have to track the /// `Option` for backwards compatibility. htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>, - claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>, + claimed_htlcs: Vec, nondust_htlc_sources: Vec, }, LatestHolderCommitment { commitment_txs: Vec, htlc_data: CommitmentHTLCData, - claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>, + claimed_htlcs: Vec, }, LatestCounterpartyCommitmentTXInfo { commitment_txid: Txid, @@ -740,9 +755,29 @@ impl ChannelMonitorUpdateStep { impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (0, LatestHolderCommitmentTXInfo) => { (0, commitment_tx, required), - (1, claimed_htlcs, optional_vec), + (1, _legacy_claimed_htlcs, (legacy, Vec<(SentHTLCId, PaymentPreimage)>, + |v: Option<&Vec<(SentHTLCId, PaymentPreimage)>>| { + // Only populate from legacy if the new-format tag 5 wasn't read. + if let Some(legacy) = v { + if claimed_htlcs.as_ref().map_or(true, |v| v.is_empty()) { + claimed_htlcs = Some(legacy.iter().map(|(htlc_id, preimage)| OutboundHTLCClaim { + htlc_id: *htlc_id, preimage: *preimage, skimmed_fee_msat: None, + }).collect()); + } + } + Ok(()) + }, + |us: &ChannelMonitorUpdateStep| match us { + ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { claimed_htlcs, .. } => { + let claims: Vec<(SentHTLCId, PaymentPreimage)> = + claimed_htlcs.iter().map(|claim| (claim.htlc_id, claim.preimage)).collect(); + Some(claims) + } + _ => None + })), (2, htlc_outputs, required_vec), (4, nondust_htlc_sources, optional_vec), + (5, claimed_htlcs, optional_vec), // Added in 0.4 }, (1, LatestCounterpartyCommitmentTXInfo) => { (0, commitment_txid, required), @@ -777,7 +812,27 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (8, LatestHolderCommitment) => { (1, commitment_txs, required_vec), (3, htlc_data, required), - (5, claimed_htlcs, required_vec), + (5, _legacy_claimed_htlcs, (legacy, Vec<(SentHTLCId, PaymentPreimage)>, + |v: Option<&Vec<(SentHTLCId, PaymentPreimage)>>| { + // Only populate from legacy if the new-format tag 7 wasn't read. + if let Some(legacy) = v { + if claimed_htlcs.as_ref().map_or(true, |v| v.is_empty()) { + claimed_htlcs = Some(legacy.iter().map(|(htlc_id, preimage)| OutboundHTLCClaim { + htlc_id: *htlc_id, preimage: *preimage, skimmed_fee_msat: None, + }).collect()); + } + } + Ok(()) + }, + |us: &ChannelMonitorUpdateStep| match us { + ChannelMonitorUpdateStep::LatestHolderCommitment { claimed_htlcs, .. } => { + let claims: Vec<(SentHTLCId, PaymentPreimage)> = + claimed_htlcs.iter().map(|claim| (claim.htlc_id, claim.preimage)).collect(); + Some(claims) + } + _ => None + })), + (7, claimed_htlcs, optional_vec), // Added in 0.4 }, (10, RenegotiatedFunding) => { (1, channel_parameters, (required: ReadableArgs, None)), @@ -3691,7 +3746,7 @@ impl ChannelMonitorImpl { fn provide_latest_holder_commitment_tx( &mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: &[(HTLCOutputInCommitment, Option, Option)], - claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], mut nondust_htlc_sources: Vec, + claimed_htlcs: &[OutboundHTLCClaim], mut nondust_htlc_sources: Vec, ) -> Result<(), &'static str> { let dust_htlcs = if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) { // If we have non-dust HTLCs in htlc_outputs, ensure they match the HTLCs in the @@ -3812,7 +3867,7 @@ impl ChannelMonitorImpl { fn update_holder_commitment_data( &mut self, commitment_txs: Vec, - mut htlc_data: CommitmentHTLCData, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], + mut htlc_data: CommitmentHTLCData, claimed_htlcs: &[OutboundHTLCClaim], ) -> Result<(), &'static str> { self.verify_matching_commitment_transactions( commitment_txs.iter().map(|holder_commitment_tx| holder_commitment_tx.deref()), @@ -3832,7 +3887,7 @@ impl ChannelMonitorImpl { mem::swap(&mut htlc_data, &mut self.current_holder_htlc_data); self.prev_holder_htlc_data = Some(htlc_data); - for (claimed_htlc_id, claimed_preimage) in claimed_htlcs { + for claim in claimed_htlcs { let htlc_opt = self .funding .counterparty_claimable_outpoints @@ -3841,7 +3896,7 @@ impl ChannelMonitorImpl { .iter() .find_map(|(htlc, source_opt)| { if let Some(source) = source_opt { - if SentHTLCId::from_source(source) == *claimed_htlc_id { + if SentHTLCId::from_source(source) == claim.htlc_id { return Some((htlc, source)); } } @@ -3855,14 +3910,14 @@ impl ChannelMonitorImpl { // startup until it is explicitly acked. self.push_monitor_event(MonitorEvent::HTLCEvent(HTLCUpdate { payment_hash: htlc.payment_hash, - payment_preimage: Some(*claimed_preimage), + payment_preimage: Some(claim.preimage), source: *source.clone(), htlc_value_msat: Some(htlc.amount_msat), user_channel_id: self.user_channel_id, })); } } - self.counterparty_fulfilled_htlcs.insert(*claimed_htlc_id, *claimed_preimage); + self.counterparty_fulfilled_htlcs.insert(claim.htlc_id, claim.preimage); } Ok(()) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index bfdf1ed8aae..818a5c46784 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -33,7 +33,7 @@ use crate::chain::chaininterface::{ use crate::chain::chainmonitor::MonitorEventSource; use crate::chain::channelmonitor::{ ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, CommitmentHTLCData, - LATENCY_GRACE_PERIOD_BLOCKS, + OutboundHTLCClaim, LATENCY_GRACE_PERIOD_BLOCKS, }; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::chain::BestBlock; @@ -8632,7 +8632,11 @@ where // claims, but such an upgrade is unlikely and including claimed HTLCs here // fixes a bug which the user was exposed to on 0.0.104 when they started the // claim anyway. - claimed_htlcs.push((SentHTLCId::from_source(&htlc.source), preimage)); + claimed_htlcs.push(OutboundHTLCClaim { + htlc_id: SentHTLCId::from_source(&htlc.source), + preimage, + skimmed_fee_msat: htlc.skimmed_fee_msat, + }); } htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(reason); need_commitment = true; diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 5c5bb6ac7f2..258db34f74f 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -1113,6 +1113,7 @@ impl_writeable_for_vec_with_element_length_prefix!(&crate::ln::msgs::UpdateAddHT impl_for_vec!(u32); impl_for_vec!(crate::events::HTLCLocator); impl_for_vec!(crate::ln::types::ChannelId); +impl_for_vec!(crate::chain::channelmonitor::OutboundHTLCClaim); impl Writeable for Vec { #[inline] From dd5600954b1cbc237da8c1bee5c3d111298b0312 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 6 Apr 2026 21:12:03 -0400 Subject: [PATCH 33/33] MonitorEvents: HTLC failure reason and skimmed fee We're moving towards having the ChannelMonitor be responsible for resolving HTLCs, via MonitorEvents, so we need to start surfacing more information in monitor events. Here we start persisting/surfacing a specific HTLC failure reason and the skimmed fee in MonitorEvent::HTLCEvents, which is useful when generating and handling monitor events for off-chain claims/fails. The skimmed fee for forwards is already put to use in this commit for generating correct PaymentForwarded events. The failure reason will be used in upcoming commits. We add the failure reason now to not have to change what we're serializing to disk later. --- lightning/src/chain/channelmonitor.rs | 72 ++++++++-- lightning/src/ln/channelmanager.rs | 112 +++++++-------- lightning/src/ln/onion_utils.rs | 8 +- lightning/src/ln/reload_tests.rs | 192 +++++++++++++++++++++++++- 4 files changed, 313 insertions(+), 71 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 403cac862a8..8b8676c88a9 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -56,6 +56,7 @@ use crate::ln::channel_keys::{ }; use crate::ln::channelmanager::{HTLCSource, PaymentClaimDetails, SentHTLCId}; use crate::ln::msgs::DecodeError; +use crate::ln::onion_utils::{HTLCFailReason, LocalHTLCFailureReason}; use crate::ln::types::ChannelId; use crate::sign::{ ecdsa::EcdsaChannelSigner, ChannelDerivationParameters, DelayedPaymentOutputDescriptor, @@ -252,24 +253,76 @@ impl_writeable_tlv_based_enum_upgradable_legacy!(MonitorEvent, // 6 was `UpdateFailed` until LDK 0.0.117 ); +/// The resolution of an outbound HTLC — either claimed with a preimage or failed back. Used in +/// [`MonitorEvent::HTLCEvent`]s to provide resolution data to the `ChannelManager`. +#[derive(Clone, PartialEq, Eq, Debug)] +pub(crate) enum OutboundHTLCResolution { + Claimed { + preimage: PaymentPreimage, + skimmed_fee_msat: Option, + }, + /// The failure resolution may come from on-chain in the [`ChannelMonitor`] or off-chain from the + /// `ChannelManager`, such as from an outbound edge to provide the manager with a resolution for + /// the inbound edge. + Failed { + reason: HTLCFailReason, + }, +} + +impl OutboundHTLCResolution { + /// Returns an on-chain timeout failure resolution. + pub fn on_chain_timeout() -> Self { + OutboundHTLCResolution::Failed { + reason: HTLCFailReason::from_failure_code(LocalHTLCFailureReason::OnChainTimeout), + } + } +} + +impl_writeable_tlv_based_enum!(OutboundHTLCResolution, + (1, Claimed) => { + (1, preimage, required), + (3, skimmed_fee_msat, option), + }, + (3, Failed) => { + (1, reason, required), + }, +); + /// Simple structure sent back by `chain::Watch` when an HTLC from a forward channel is detected on /// chain. Used to update the corresponding HTLC in the backward channel. Failing to pass the /// preimage claim backward will lead to loss of funds. #[derive(Clone, PartialEq, Eq)] pub struct HTLCUpdate { pub(crate) payment_hash: PaymentHash, - pub(crate) payment_preimage: Option, pub(crate) source: HTLCSource, pub(crate) htlc_value_msat: Option, pub(crate) user_channel_id: Option, + pub(crate) resolution: OutboundHTLCResolution, } impl_writeable_tlv_based!(HTLCUpdate, { (0, payment_hash, required), (1, htlc_value_satoshis, (legacy, u64, |_| Ok(()), |us: &HTLCUpdate| us.htlc_value_msat.map(|v| v / 1000))), (2, source, required), - (4, payment_preimage, option), + (4, _legacy_payment_preimage, (legacy, Option, + |v: Option<&Option>| { + // Only use the legacy preimage if the new `resolution` TLV (9) wasn't read, + // otherwise we'd clobber it (losing e.g. skimmed_fee_msat). + if resolution.0.is_none() { + if let Some(Some(preimage)) = v { + resolution = crate::util::ser::RequiredWrapper(Some( + OutboundHTLCResolution::Claimed { preimage: *preimage, skimmed_fee_msat: None } + )); + } + } + Ok(()) + }, + |us: &HTLCUpdate| match &us.resolution { + OutboundHTLCResolution::Claimed { preimage, .. } => Some(Some(*preimage)), + _ => None, + })), (5, user_channel_id, option), (7, htlc_value_msat, (default_value, htlc_value_satoshis.map(|v: u64| v * 1000))), // Added in 0.4 + (9, resolution, (default_value, OutboundHTLCResolution::on_chain_timeout())), }); /// If an output goes from claimable only by us to claimable by us or our counterparty within this @@ -3910,7 +3963,10 @@ impl ChannelMonitorImpl { // startup until it is explicitly acked. self.push_monitor_event(MonitorEvent::HTLCEvent(HTLCUpdate { payment_hash: htlc.payment_hash, - payment_preimage: Some(claim.preimage), + resolution: OutboundHTLCResolution::Claimed { + preimage: claim.preimage, + skimmed_fee_msat: claim.skimmed_fee_msat, + }, source: *source.clone(), htlc_value_msat: Some(htlc.amount_msat), user_channel_id: self.user_channel_id, @@ -4647,7 +4703,7 @@ impl ChannelMonitorImpl { &mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate { payment_hash, - payment_preimage: None, + resolution: OutboundHTLCResolution::on_chain_timeout(), source: source.clone(), htlc_value_msat: Some(amount_msat), user_channel_id: self.user_channel_id, @@ -5985,7 +6041,7 @@ impl ChannelMonitorImpl { &payment_hash, entry.txid); self.push_monitor_event(MonitorEvent::HTLCEvent(HTLCUpdate { payment_hash, - payment_preimage: None, + resolution: OutboundHTLCResolution::on_chain_timeout(), source, htlc_value_msat: htlc_value_satoshis.map(|v| v * 1000), user_channel_id: self.user_channel_id, @@ -6096,7 +6152,7 @@ impl ChannelMonitorImpl { expires soon at {}", log_bytes!(htlc.payment_hash.0), inbound_htlc_expiry); push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate { source: source.clone(), - payment_preimage: None, + resolution: OutboundHTLCResolution::on_chain_timeout(), payment_hash: htlc.payment_hash, htlc_value_msat: Some(htlc.amount_msat), user_channel_id: self.user_channel_id, @@ -6514,7 +6570,7 @@ impl ChannelMonitorImpl { self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate { source, - payment_preimage: Some(payment_preimage), + resolution: OutboundHTLCResolution::Claimed { preimage: payment_preimage, skimmed_fee_msat: None }, payment_hash, htlc_value_msat: Some(amount_msat), user_channel_id: self.user_channel_id, @@ -6539,7 +6595,7 @@ impl ChannelMonitorImpl { self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate { source, - payment_preimage: Some(payment_preimage), + resolution: OutboundHTLCResolution::Claimed { preimage: payment_preimage, skimmed_fee_msat: None }, payment_hash, htlc_value_msat: Some(amount_msat), user_channel_id: self.user_channel_id, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8016a9e78d2..8367ff14873 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -45,8 +45,8 @@ use crate::chain::chaininterface::{ use crate::chain::chainmonitor::MonitorEventSource; use crate::chain::channelmonitor::{ ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, MonitorEvent, - WithChannelMonitor, ANTI_REORG_DELAY, CLTV_CLAIM_BUFFER, HTLC_FAIL_BACK_BUFFER, - LATENCY_GRACE_PERIOD_BLOCKS, MAX_BLOCKS_FOR_CONF, + OutboundHTLCResolution, WithChannelMonitor, ANTI_REORG_DELAY, CLTV_CLAIM_BUFFER, + HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, MAX_BLOCKS_FOR_CONF, }; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Confirm, Watch}; @@ -13771,60 +13771,62 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Some(channel_id), Some(htlc_update.payment_hash), ); - if let Some(preimage) = htlc_update.payment_preimage { - log_trace!( - logger, - "Claiming HTLC with preimage {} from our monitor", - preimage - ); - let from_onchain = self - .per_peer_state - .read() - .unwrap() - .get(&counterparty_node_id) - .map_or(true, |peer_state_mtx| { - !peer_state_mtx - .lock() - .unwrap() - .channel_by_id - .contains_key(&channel_id) + match htlc_update.resolution { + OutboundHTLCResolution::Claimed { preimage, skimmed_fee_msat } => { + log_trace!( + logger, + "Claiming HTLC with preimage {} from our monitor", + preimage + ); + let from_onchain = self + .per_peer_state + .read() + .unwrap() + .get(&counterparty_node_id) + .map_or(true, |peer_state_mtx| { + !peer_state_mtx + .lock() + .unwrap() + .channel_by_id + .contains_key(&channel_id) + }); + // Claim the funds from the previous hop, if there is one. In the future we can + // store attribution data in the `ChannelMonitor` and provide it here. + self.claim_funds_internal( + htlc_update.source, + preimage, + htlc_update.htlc_value_msat, + skimmed_fee_msat, + from_onchain, + counterparty_node_id, + funding_outpoint, + channel_id, + htlc_update.user_channel_id, + None, + None, + Some(event_id), + ); + }, + OutboundHTLCResolution::Failed { reason } => { + log_trace!(logger, "Failing HTLC from our monitor"); + let failure_type = htlc_update + .source + .failure_type(counterparty_node_id, channel_id); + let completion_update = Some(PaymentCompleteUpdate { + counterparty_node_id, + channel_funding_outpoint: funding_outpoint, + channel_id, + htlc_id: SentHTLCId::from_source(&htlc_update.source), }); - // Claim the funds from the previous hop, if there is one. In the future we can - // store attribution data in the `ChannelMonitor` and provide it here. - self.claim_funds_internal( - htlc_update.source, - preimage, - htlc_update.htlc_value_msat, - None, - from_onchain, - counterparty_node_id, - funding_outpoint, - channel_id, - htlc_update.user_channel_id, - None, - None, - Some(event_id), - ); - } else { - log_trace!(logger, "Failing HTLC from our monitor"); - let failure_reason = LocalHTLCFailureReason::OnChainTimeout; - let failure_type = - htlc_update.source.failure_type(counterparty_node_id, channel_id); - let reason = HTLCFailReason::from_failure_code(failure_reason); - let completion_update = Some(PaymentCompleteUpdate { - counterparty_node_id, - channel_funding_outpoint: funding_outpoint, - channel_id, - htlc_id: SentHTLCId::from_source(&htlc_update.source), - }); - self.fail_htlc_backwards_internal( - &htlc_update.source, - &htlc_update.payment_hash, - &reason, - failure_type, - completion_update, - ); - self.chain_monitor.ack_monitor_event(monitor_event_source); + self.fail_htlc_backwards_internal( + &htlc_update.source, + &htlc_update.payment_hash, + &reason, + failure_type, + completion_update, + ); + self.chain_monitor.ack_monitor_event(monitor_event_source); + }, } }, MonitorEvent::HolderForceClosed(_) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 9b1b009e93a..1cf09d0e7e1 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -1908,11 +1908,11 @@ impl From<&HTLCFailReason> for HTLCHandlingFailureReason { } #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug -#[cfg_attr(test, derive(PartialEq))] -pub(super) struct HTLCFailReason(HTLCFailReasonRepr); +#[derive(PartialEq, Eq)] +pub(crate) struct HTLCFailReason(HTLCFailReasonRepr); #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug -#[cfg_attr(test, derive(PartialEq))] +#[derive(PartialEq, Eq)] enum HTLCFailReasonRepr { LightningError { err: msgs::OnionErrorPacket, hold_time: Option }, Reason { data: Vec, failure_reason: LocalHTLCFailureReason }, @@ -2071,7 +2071,7 @@ impl HTLCFailReason { Self(HTLCFailReasonRepr::Reason { data, failure_reason }) } - pub(super) fn from_failure_code(failure_reason: LocalHTLCFailureReason) -> Self { + pub(crate) fn from_failure_code(failure_reason: LocalHTLCFailureReason) -> Self { Self::reason(failure_reason, Vec::new()) } diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 416914b3167..62c897c9da7 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -14,12 +14,13 @@ use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Watch}; use crate::chain::chaininterface::LowerBoundedFeeEstimator; use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateStep}; -use crate::routing::router::{PaymentParameters, RouteParameters}; +use crate::routing::gossip::RoutingFees; +use crate::routing::router::{PaymentParameters, RouteHint, RouteHintHop, RouteParameters}; use crate::sign::EntropySource; use crate::chain::transaction::OutPoint; use crate::events::{ClosureReason, Event, HTLCHandlingFailureType}; -use crate::ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder}; -use crate::ln::outbound_payment::RecipientOnionFields; +use crate::ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, MIN_CLTV_EXPIRY_DELTA, PaymentId, RAACommitmentOrder}; +use crate::ln::outbound_payment::{RecipientOnionFields, Retry}; use crate::ln::msgs; use crate::ln::types::ChannelId; use crate::ln::msgs::{BaseMessageHandler, ChannelMessageHandler, RoutingMessageHandler, ErrorAction, MessageSendEvent}; @@ -511,7 +512,6 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { #[cfg(feature = "std")] fn do_test_data_loss_protect(reconnect_panicing: bool, substantially_old: bool, not_stale: bool) { - use crate::ln::outbound_payment::Retry; use crate::types::string::UntrustedString; // When we get a data_loss_protect proving we're behind, we immediately panic as the // chain::Watch API requirements have been violated (e.g. the user restored from a backup). The @@ -2253,3 +2253,187 @@ fn test_reload_with_mpp_claims_on_same_channel() { // nodes[0] should now have received both fulfills and generate PaymentSent. expect_payment_sent!(&nodes[0], payment_preimage); } + +#[test] +fn test_reload_mon_event_skimmed_fee() { + // Test that skimmed fees survive the serialize/deserialize round-trip through monitor events. + // When a forwarding node learns about a claim only through a re-provided persistent monitor + // event (not the off-chain path), the PaymentForwarded event must still include the correct + // skimmed_fee_msat. + // + // We set up a payment with a skimmed fee via HTLC interception, claim it on the recipient, + // process the fulfill on the forwarding node but disconnect the sender before the claim + // propagates back, clear the holding cell, reload, and verify the re-provided monitor event + // produces a PaymentForwarded with the correct skimmed fee. + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let persister; + let new_chain_monitor; + + let mut intercept_forwards_config = test_default_channel_config(); + intercept_forwards_config.htlc_interception_flags = + HTLCInterceptionFlags::ToInterceptSCIDs as u8; + intercept_forwards_config.override_persistent_monitor_events = Some(true); + let mut underpay_config = test_default_channel_config(); + underpay_config.channel_config.accept_underpaying_htlcs = true; + + let node_chanmgrs = create_node_chanmgrs( + 3, &node_cfgs, &[None, Some(intercept_forwards_config), Some(underpay_config)], + ); + let nodes_1_deserialized; + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let node_0_id = nodes[0].node.get_our_node_id(); + let node_1_id = nodes[1].node.get_our_node_id(); + let node_2_id = nodes[2].node.get_our_node_id(); + + let chan_0_1 = create_announced_chan_between_nodes(&nodes, 0, 1); + let chan_1_2 = create_unannounced_chan_between_nodes_with_value(&nodes, 1, 2, 100_000, 0); + + let chan_id_0_1 = chan_0_1.2; + let chan_id_1_2 = chan_1_2.0.channel_id; + + let amt_msat = 900_000; + let skimmed_fee_msat = 20; + + // Build a route via the intercept SCID so nodes[1] can skim a fee. + let route_hint = RouteHint(vec![RouteHintHop { + src_node_id: node_1_id, + short_channel_id: nodes[1].node.get_intercept_scid(), + fees: RoutingFees { base_msat: 1000, proportional_millionths: 0 }, + cltv_expiry_delta: MIN_CLTV_EXPIRY_DELTA, + htlc_minimum_msat: None, + htlc_maximum_msat: Some(amt_msat + 5), + }]); + let payment_params = PaymentParameters::from_node_id(node_2_id, TEST_FINAL_CLTV) + .with_route_hints(vec![route_hint]) + .unwrap() + .with_bolt11_features(nodes[2].node.bolt11_invoice_features()) + .unwrap(); + let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); + let (payment_hash, payment_secret) = + nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60, None).unwrap(); + let onion = RecipientOnionFields::secret_only(payment_secret, amt_msat); + let payment_id = PaymentId(payment_hash.0); + nodes[0] + .node + .send_payment(payment_hash, onion, payment_id, route_params, Retry::Attempts(0)) + .unwrap(); + check_added_monitors(&nodes[0], 1); + let events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + + // Forward the payment via intercept, skimming a fee. + let ev = SendEvent::from_event(events.into_iter().next().unwrap()); + nodes[1].node.handle_update_add_htlc(node_0_id, &ev.msgs[0]); + do_commitment_signed_dance(&nodes[1], &nodes[0], &ev.commitment_msg, false, true); + expect_and_process_pending_htlcs(&nodes[1], false); + + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + let (intercept_id, expected_outbound_amt_msat) = match events[0] { + Event::HTLCIntercepted { + intercept_id, expected_outbound_amount_msat, payment_hash: pmt_hash, .. + } => { + assert_eq!(pmt_hash, payment_hash); + (intercept_id, expected_outbound_amount_msat) + }, + _ => panic!("Unexpected event"), + }; + nodes[1] + .node + .forward_intercepted_htlc( + intercept_id, &chan_id_1_2, node_2_id, + expected_outbound_amt_msat - skimmed_fee_msat, + ) + .unwrap(); + expect_and_process_pending_htlcs(&nodes[1], false); + let pay_event = { + { + let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap(); + assert_eq!(added_monitors.len(), 1); + added_monitors.clear(); + } + let mut events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + SendEvent::from_event(events.remove(0)) + }; + nodes[2].node.handle_update_add_htlc(node_1_id, &pay_event.msgs[0]); + do_commitment_signed_dance(&nodes[2], &nodes[1], &pay_event.commitment_msg, false, true); + expect_and_process_pending_htlcs(&nodes[2], false); + + // Drain the PaymentClaimable event and claim the payment on nodes[2]. + let payment_preimage = + nodes[2].node.get_payment_preimage(payment_hash, payment_secret).unwrap(); + let events = nodes[2].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + assert!(matches!(events[0], Event::PaymentClaimable { .. })); + nodes[2].node.claim_funds(payment_preimage); + check_added_monitors(&nodes[2], 1); + expect_payment_claimed!(nodes[2], payment_hash, amt_msat - skimmed_fee_msat); + + // Disconnect nodes[0] from nodes[1] BEFORE processing the fulfill, so the claim goes into + // the holding cell on chan_0_1. + nodes[0].node.peer_disconnected(node_1_id); + nodes[1].node.peer_disconnected(node_0_id); + + // Process the fulfill from nodes[2] to nodes[1]. + let updates_2_1 = get_htlc_update_msgs(&nodes[2], &node_1_id); + nodes[1] + .node + .handle_update_fulfill_htlc(node_2_id, updates_2_1.update_fulfill_htlcs[0].clone()); + check_added_monitors(&nodes[1], 1); + do_commitment_signed_dance(&nodes[1], &nodes[2], &updates_2_1.commitment_signed, false, false); + + // Consume the original PaymentForwarded event (with skimmed fee) from the off-chain path. + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match &events[0] { + Event::PaymentForwarded { skimmed_fee_msat: skimmed, .. } => { + assert_eq!(*skimmed, Some(skimmed_fee_msat)); + }, + _ => panic!("Expected PaymentForwarded, got {:?}", events[0]), + } + + // Clear the holding cell to simulate a crash where the claim didn't make it to a commitment. + nodes[1].node.test_clear_channel_holding_cell(node_0_id, chan_id_0_1); + + // Serialize and reload nodes[1]. + let node_1_serialized = nodes[1].node.encode(); + let mon_0_1_serialized = get_monitor!(nodes[1], chan_id_0_1).encode(); + let mon_1_2_serialized = get_monitor!(nodes[1], chan_id_1_2).encode(); + + reload_node!( + nodes[1], + node_1_serialized, + &[&mon_0_1_serialized, &mon_1_2_serialized], + persister, + new_chain_monitor, + nodes_1_deserialized, + Some(true) + ); + + // The persistent monitor event is re-provided, triggering the claim and a preimage + // monitor update. + let msgs = nodes[1].node.get_and_clear_pending_msg_events(); + assert!(msgs.is_empty()); + + // The re-provided monitor event (or the startup replay) should produce a PaymentForwarded + // that includes the skimmed fee. + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1, "{events:?}"); + match &events[0] { + Event::PaymentForwarded { skimmed_fee_msat: skimmed, .. } => { + assert_eq!(*skimmed, Some(skimmed_fee_msat)); + }, + _ => panic!("Expected PaymentForwarded, got {:?}", events[0]), + } + check_added_monitors(&nodes[1], 1); + + // Reconnect and finish the payment. + let mut reconnect_args = ReconnectArgs::new(&nodes[1], &nodes[0]); + reconnect_args.pending_cell_htlc_claims = (0, 1); + reconnect_nodes(reconnect_args); + expect_payment_sent!(&nodes[0], payment_preimage); +}