diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f20f93c789c..04d92901f32 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::{ @@ -364,9 +365,13 @@ 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(); } + + 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 ca01e95c054..79ada7c70e6 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. @@ -366,9 +381,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 +448,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 +668,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 +812,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 +829,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 +1268,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)", @@ -1645,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); } @@ -1665,12 +1659,17 @@ 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 } + + 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 810de80da95..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, @@ -68,8 +69,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)] @@ -183,6 +184,15 @@ impl Readable for ChannelMonitorUpdate { } } +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. #[derive(Clone, PartialEq, Eq)] pub enum MonitorEvent { @@ -226,8 +236,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), @@ -245,21 +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_satoshis: Option, + 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, 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), + (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 @@ -632,6 +695,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 { @@ -643,13 +721,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, @@ -730,9 +808,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), @@ -767,7 +865,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)), @@ -1204,6 +1322,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 @@ -1280,7 +1400,19 @@ 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)>, + // `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 + /// 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, @@ -1661,32 +1793,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())?; @@ -1719,24 +1857,47 @@ 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_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 + .provided_monitor_events + .iter() + .chain(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, required_vec), + (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), @@ -1756,6 +1917,8 @@ 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), + (43, channel_monitor.user_channel_id, option), // Added in 0.4 }); Ok(()) @@ -1860,7 +2023,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)); @@ -1909,6 +2072,7 @@ impl ChannelMonitor { pending_funding: vec![], is_manual_broadcast, + user_channel_id: Some(user_channel_id), funding_seen_onchain: false, latest_update_id: 0, @@ -1939,6 +2103,9 @@ 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(), is_processing_pending_events: false, @@ -2152,10 +2319,50 @@ 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() } + pub(super) fn push_monitor_event(&self, event: MonitorEvent) { + 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) { + 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; + } + + 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. + #[cfg(any(test, feature = "_test_utils"))] + pub fn copy_monitor_event_state(&self, other: &ChannelMonitor) { + let (provided, pending, next_id) = { + let other_inner = other.inner.lock().unwrap(); + ( + 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; + } + /// Processes [`SpendableOutputs`] events produced from each [`ChannelMonitor`] upon maturity. /// /// For channels featuring anchor outputs, this method will also process [`BumpTransaction`] @@ -3202,7 +3409,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); + } } } }; @@ -3588,7 +3799,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 @@ -3709,7 +3920,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()), @@ -3729,23 +3940,40 @@ 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 { - #[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)| { + for claim in claimed_htlcs { + 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) == claim.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, + 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, + })); + } } - self.counterparty_fulfilled_htlcs.insert(*claimed_htlc_id, *claimed_preimage); + self.counterparty_fulfilled_htlcs.insert(claim.htlc_id, claim.preimage); } Ok(()) @@ -3881,7 +4109,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, &mut self.next_monitor_event_id); } // Although we aren't signing the transaction directly here, the transaction will be signed @@ -4459,7 +4687,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` @@ -4472,12 +4699,17 @@ 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, + resolution: OutboundHTLCResolution::on_chain_timeout(), + source: source.clone(), + htlc_value_msat: Some(amount_msat), + user_channel_id: self.user_channel_id, + }), + &mut self.next_monitor_event_id, + ); self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { commitment_tx_output_idx: None, resolving_txid: Some(confirmed_txid), @@ -4495,7 +4727,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, }, }; @@ -4542,10 +4774,41 @@ impl ChannelMonitorImpl { &self.outputs_to_watch } - fn get_and_clear_pending_monitor_events(&mut self) -> Vec { - let mut ret = Vec::new(); - mem::swap(&mut ret, &mut self.pending_monitor_events); - ret + fn push_monitor_event(&mut self, event: MonitorEvent) { + push_monitor_event( + &mut self.pending_monitor_events, + event, + &mut self.next_monitor_event_id, + ); + } + + 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 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(); + 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 @@ -5601,7 +5864,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; @@ -5776,11 +6039,12 @@ 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, + resolution: OutboundHTLCResolution::on_chain_timeout(), 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 { commitment_tx_output_idx, @@ -5872,8 +6136,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 { @@ -5886,12 +6150,13 @@ 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, + resolution: OutboundHTLCResolution::on_chain_timeout(), 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); } } } @@ -6289,8 +6554,8 @@ 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( - |update| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == source } else { false }) { + 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(), height, @@ -6303,16 +6568,17 @@ 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), + resolution: OutboundHTLCResolution::Claimed { preimage: payment_preimage, skimmed_fee_msat: None }, 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); } } else if offered_preimage_claim { - if !self.pending_monitor_events.iter().any( - |update| if let &MonitorEvent::HTLCEvent(ref upd) = update { + 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 { @@ -6327,12 +6593,13 @@ 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), + resolution: OutboundHTLCResolution::Claimed { preimage: payment_preimage, skimmed_fee_msat: None }, 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); } } else { self.onchain_events_awaiting_threshold_conf.retain(|ref entry| { @@ -6625,16 +6892,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)?; @@ -6696,10 +6963,16 @@ 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; 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), + (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), (11, confirmed_commitment_tx_counterparty_output, option), @@ -6719,11 +6992,19 @@ 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), + (43, user_channel_id, option), // Added in 0.4 }); if let Some(previous_blocks) = best_block_previous_blocks { 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(); @@ -6746,13 +7027,29 @@ 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(_))); + } + } + + // 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() @@ -6840,6 +7137,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(), @@ -6871,7 +7169,10 @@ 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, + provided_monitor_events: Vec::new(), + persistent_events_enabled: persistent_events_enabled.is_some(), + next_monitor_event_id, pending_events, is_processing_pending_events: false, @@ -6920,6 +7221,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, @@ -6982,6 +7299,7 @@ pub(super) fn dummy_monitor( dummy_key, channel_id, false, + 0, ) } diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs index 9692558cf7c..b7ff2cb917c 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, }; @@ -416,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. @@ -424,7 +429,16 @@ 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. + /// + /// 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> @@ -444,9 +458,13 @@ 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() } + + 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/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 af4d1569d0c..2a3117dca7f 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); @@ -3601,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(); @@ -3763,7 +3769,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] @@ -3774,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 @@ -3793,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, @@ -3951,7 +3958,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 @@ -4014,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); @@ -4033,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(); @@ -4139,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) { @@ -4199,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()); @@ -4256,7 +4685,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( @@ -4524,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); @@ -4936,6 +5367,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); @@ -5021,7 +5453,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 +5501,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 @@ -5081,7 +5513,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, @@ -5115,7 +5556,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!(); @@ -5223,7 +5664,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/channel.rs b/lightning/src/ln/channel.rs index 32c0e94bdc8..818a5c46784 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -30,9 +30,10 @@ 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, + OutboundHTLCClaim, LATENCY_GRACE_PERIOD_BLOCKS, }; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::chain::BestBlock; @@ -230,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 { @@ -244,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), } } } @@ -265,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"), } } } @@ -277,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, } } @@ -304,7 +309,7 @@ impl InboundHTLCState { }, InboundHTLCResolution::Resolved { .. } => false, }, - InboundHTLCState::Committed { .. } | InboundHTLCState::LocalRemoved(_) => false, + InboundHTLCState::Committed { .. } | InboundHTLCState::LocalRemoved { .. } => false, } } } @@ -547,6 +552,7 @@ enum HTLCUpdateAwaitingACK { payment_preimage: PaymentPreimage, attribution_data: Option, htlc_id: u64, + monitor_event_id: Option, }, FailHTLC { htlc_id: u64, @@ -1474,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 @@ -3617,7 +3631,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(), @@ -4634,7 +4648,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(_) @@ -5145,8 +5159,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, @@ -5218,8 +5232,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) @@ -6916,7 +6930,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 } @@ -6933,10 +6950,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 { @@ -7482,8 +7502,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 +7519,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 @@ -7509,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()); @@ -7523,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" @@ -7567,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 @@ -7605,6 +7646,7 @@ where payment_preimage: payment_preimage_arg, htlc_id: htlc_id_arg, attribution_data, + monitor_event_id, }); return UpdateFulfillFetch::NewClaim { monitor_update, @@ -7632,10 +7674,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 } @@ -7644,7 +7689,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 +7697,7 @@ where payment_preimage, payment_info, attribution_data, + monitor_event_id, logger, ) { UpdateFulfillFetch::NewClaim { @@ -7684,7 +7730,7 @@ where let update = self.build_commitment_no_status_check(logger); self.context .blocked_monitor_updates - .push(PendingChannelMonitorUpdate { update }); + .push(PendingChannelMonitorUpdate::new(update)); } } @@ -7743,7 +7789,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))); }, _ => { @@ -8586,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; @@ -8784,6 +8834,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 @@ -8800,6 +8851,7 @@ where *payment_preimage, None, attribution_data.clone(), + monitor_event_id, logger, ); let mut additional_monitor_update = @@ -8901,7 +8953,7 @@ where ( Vec<(HTLCSource, PaymentHash)>, Vec<(StaticInvoice, BlindedMessagePath)>, - Option, + Option<(ChannelMonitorUpdate, Vec)>, ), ChannelError, > { @@ -9008,6 +9060,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. @@ -9018,12 +9071,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 @@ -9087,20 +9145,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) }, } @@ -9214,13 +9275,19 @@ 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 }); + 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(($htlcs_to_fail, static_invoices, Some(monitor_update))); + return Ok(( + $htlcs_to_fail, + static_invoices, + Some((monitor_update, events_to_ack)), + )); } }; } @@ -9585,7 +9652,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 @@ -10092,7 +10159,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 { @@ -11308,14 +11375,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 @@ -11325,9 +11393,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) @@ -15442,9 +15508,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, @@ -15567,6 +15633,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)?; @@ -15931,7 +15998,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), }, @@ -16021,6 +16088,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)?, @@ -16433,7 +16501,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) @@ -17560,6 +17628,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 { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 2c97e4adaa1..8367ff14873 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -42,15 +42,16 @@ 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, - 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}; use crate::events::{ - self, ClosureReason, Event, EventHandler, EventsProvider, HTLCHandlingFailureType, + self, ClosureReason, Event, EventHandler, EventsProvider, HTLCHandlingFailureType, HTLCLocator, InboundChannelFunds, PaymentFailureReason, ReplayEvent, }; use crate::events::{FundingInfo, PaidBolt12Invoice}; @@ -1478,6 +1479,23 @@ 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 }, + /// 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, @@ -1499,8 +1517,49 @@ 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), + }, + (5, EmitForwardEvent) => { + (1, event, required), + (3, post_event_ackable_monitor_event, option), + } ); +/// 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] @@ -1552,6 +1611,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) => { @@ -1563,6 +1627,9 @@ impl_writeable_tlv_based_enum!(EventCompletionAction, } ChannelId::v1_from_funding_outpoint(channel_funding_outpoint.unwrap()) })), + }, + (3, AckMonitorEvent) => { + (1, event_id, required), } {1, ReleasePaymentCompleteChannelMonitorUpdate} => (), ); @@ -2928,6 +2995,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))] @@ -3554,6 +3630,29 @@ 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), + inbound_edge_closed: false, + } + } + fn duplicate_claim() -> Self { + Self { + definitely_duplicate: true, + inbound_htlc_value_msat: None, + inbound_edge_closed: false, + } + } +} + impl< M: chain::Watch, T: BroadcasterInterface, @@ -3603,6 +3702,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), @@ -3658,6 +3759,28 @@ impl< signer_provider, logger, + + 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 + }, + } + }) + } + }, } } @@ -9459,9 +9582,10 @@ impl< payment_preimage, payment_info.clone(), Some(attribution_data), - |_, definitely_duplicate| { + None, + |claim_action_params| { debug_assert!( - !definitely_duplicate, + !claim_action_params.definitely_duplicate, "We shouldn't claim duplicatively from a payment" ); ( @@ -9503,10 +9627,11 @@ 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, + monitor_event_id: Option, ) { let _prev_channel_id = hop_data.channel_id; let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data); @@ -9530,7 +9655,14 @@ impl< payment_preimage, None, Some(attribution_data), - |htlc_claim_value_msat, definitely_duplicate| { + 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, + 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, @@ -9538,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 @@ -9600,16 +9769,10 @@ impl< None, ) } 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 { .. } - )); - } + let event = make_payment_forwarded_event(inbound_htlc_value_msat); ( Some(MonitorUpdateCompletionAction::EmitEventOptionAndFreeOtherChannel { - event, + event: event.map(|ev| ev.into()), downstream_counterparty_and_funding_outpoint: chan_to_release, }), None, @@ -9621,13 +9784,12 @@ impl< fn claim_funds_from_hop< ComplFunc: FnOnce( - Option, - bool, + ClaimCompletionActionParams, ) -> (Option, Option), >( &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(); @@ -9653,19 +9815,19 @@ impl< payment_preimage, payment_info, attribution_data, + monitor_event_id, completion_action, ) } fn claim_mpp_part< ComplFunc: FnOnce( - Option, - bool, + ClaimCompletionActionParams, ) -> (Option, Option), >( &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! @@ -9702,13 +9864,15 @@ impl< payment_preimage, payment_info, attribution_data, + monitor_event_id, &&logger, ); 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, @@ -9743,7 +9907,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 @@ -9871,7 +10037,11 @@ 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, + inbound_edge_closed: true, + }); if let Some(raa_blocker) = raa_blocker_opt { peer_state @@ -9958,6 +10128,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); @@ -9970,7 +10141,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, @@ -9992,6 +10174,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, @@ -9999,10 +10182,30 @@ 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, ); + + 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. @@ -10020,7 +10223,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 { @@ -10036,7 +10239,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, @@ -10056,6 +10259,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, .. } => { @@ -10065,9 +10269,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 @@ -10099,6 +10303,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, ); } }, @@ -10346,6 +10551,22 @@ 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); + } + }, + 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)); + }, } } @@ -11602,6 +11823,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 @@ -11772,6 +11994,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(|()| { @@ -12539,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` @@ -12602,6 +12834,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(()) @@ -12686,6 +12919,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 { @@ -12833,6 +13067,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, @@ -12870,7 +13111,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( @@ -13511,7 +13761,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 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( @@ -13520,46 +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 - ); - // 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, - true, - counterparty_node_id, - funding_outpoint, - channel_id, - None, - None, - None, - ); - } 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, - ); + 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), + }); + 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(_) @@ -13594,6 +13861,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(); @@ -13615,6 +13885,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( @@ -13622,6 +13895,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); }, } } @@ -15198,9 +15472,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, @@ -15312,6 +15595,9 @@ impl< } } }, + EventCompletionAction::AckMonitorEvent { event_id } => { + self.chain_monitor.ack_monitor_event(event_id); + }, } } } @@ -18102,6 +18388,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 +18403,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 +18503,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 +18690,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 +18703,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 +18713,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 +18838,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 +19139,7 @@ impl< mut in_flight_monitor_updates, peer_storage_dir, async_receive_offer_cache, + persistent_monitor_events, version: _version, } = data; @@ -18966,6 +19267,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 @@ -19594,6 +19903,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(), @@ -19611,6 +19926,7 @@ impl< session_priv, path, true, + best_block.height, &mut compl_action, &pending_events, &logger, @@ -20100,6 +20416,8 @@ impl< logger: args.logger, config: RwLock::new(args.config), + + persistent_monitor_events, }; let mut processed_claims: HashSet> = new_hash_set(); @@ -20251,7 +20569,8 @@ impl< payment_preimage, None, None, - |_, _| { + None, + |_| { ( Some(MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, @@ -20414,6 +20733,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. @@ -20430,6 +20755,7 @@ impl< downstream_user_channel_id, None, None, + None, ); } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index d39cee78b0f..4388ace8992 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(); } } @@ -3061,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 }, ) }; } @@ -3153,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, @@ -4222,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()) } @@ -5652,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 8d9df062868..6143f243be8 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); @@ -1625,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 { @@ -2672,7 +2679,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); @@ -4284,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()); @@ -4295,7 +4305,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")] @@ -7906,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. @@ -7940,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 @@ -8574,7 +8599,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 +9959,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 f52f093917b..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 @@ -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] @@ -3546,16 +3547,20 @@ 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()); 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] @@ -3594,6 +3599,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); @@ -3803,27 +3811,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); } @@ -3883,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); @@ -3906,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/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/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 7259f60796f..f41a4f06459 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, ) @@ -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 807d1a1af39..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(); @@ -957,7 +982,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 +1393,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 +1431,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); } } @@ -1430,15 +1459,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); } @@ -2381,7 +2434,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); @@ -2586,7 +2643,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)] @@ -4170,10 +4227,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 { @@ -4767,7 +4828,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 }]; @@ -5565,7 +5626,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, .. } = @@ -5594,7 +5658,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, .. } = @@ -5850,7 +5917,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] { @@ -5875,7 +5945,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..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 @@ -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] @@ -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); @@ -1983,7 +1990,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 +2251,189 @@ 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); +} + +#[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); } 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/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(); 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 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, }) } } diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 2b02629d3b0..258db34f74f 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); @@ -1112,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] diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 57f9ba6b22f..033065a2b96 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, }; @@ -521,6 +521,11 @@ 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>, + /// 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( @@ -581,6 +586,8 @@ impl<'a> TestChainMonitor<'a> { #[cfg(feature = "std")] write_blocker: Mutex::new(None), pause_flush: AtomicBool::new(false), + recent_monitor_updates: Mutex::new(Vec::new()), + deterministic_mon_events_order: AtomicBool::new(false), } } @@ -649,6 +656,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() @@ -678,6 +686,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); @@ -710,6 +724,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 +740,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.) @@ -732,7 +749,15 @@ 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) { + self.chain_monitor.ack_monitor_event(source); } }