diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index ddb1e31f645..9f03e5575ee 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -77,7 +77,7 @@ use crate::util::ser::{ use crate::prelude::*; use crate::io::{self, Error}; -use crate::sync::{LockTestExt, Mutex}; +use crate::sync::Mutex; use core::ops::Deref; use core::{cmp, mem}; @@ -681,6 +681,20 @@ pub(crate) enum ChannelMonitorUpdateStep { RenegotiatedFundingLocked { funding_txid: Txid, }, + /// When a payment is finally resolved by the user handling an [`Event::PaymentSent`] or + /// [`Event::PaymentFailed`] event, the `ChannelManager` no longer needs to hear about it on + /// startup (which would cause it to re-hydrate the payment information even though the user + /// already learned about the payment's result). + /// + /// This will remove the HTLC from [`ChannelMonitor::get_all_current_outbound_htlcs`] and + /// [`ChannelMonitor::get_onchain_failed_outbound_htlcs`]. + /// + /// Note that this is only generated for closed channels as this is implicit in the + /// [`Self::CommitmentSecret`] update which clears the payment information from all un-revoked + /// counterparty commitment transactions. + ReleasePaymentComplete { + htlc: SentHTLCId, + }, } impl ChannelMonitorUpdateStep { @@ -697,6 +711,7 @@ impl ChannelMonitorUpdateStep { ChannelMonitorUpdateStep::ShutdownScript { .. } => "ShutdownScript", ChannelMonitorUpdateStep::RenegotiatedFunding { .. } => "RenegotiatedFunding", ChannelMonitorUpdateStep::RenegotiatedFundingLocked { .. } => "RenegotiatedFundingLocked", + ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => "ReleasePaymentComplete", } } } @@ -735,6 +750,9 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (1, commitment_txs, required_vec), (3, htlc_data, required), }, + (7, ReleasePaymentComplete) => { + (1, htlc, required), + }, (8, LatestHolderCommitment) => { (1, commitment_txs, required_vec), (3, htlc_data, required), @@ -1234,6 +1252,12 @@ pub(crate) struct ChannelMonitorImpl { /// spending CSV for revocable outputs). htlcs_resolved_on_chain: Vec, + /// When a payment is fully resolved by the user processing a PaymentSent or PaymentFailed + /// event, we are informed by the ChannelManager (if the payment was resolved by an on-chain + /// transaction) of this so that we can stop telling the ChannelManager about the payment in + /// the future. We store the set of fully resolved payments here. + htlcs_resolved_to_user: HashSet, + /// The set of `SpendableOutput` events which we have already passed upstream to be claimed. /// These are tracked explicitly to ensure that we don't generate the same events redundantly /// if users duplicatively confirm old transactions. Specifically for transactions claiming a @@ -1330,18 +1354,30 @@ macro_rules! holder_commitment_htlcs { /// Transaction outputs to watch for on-chain spends. pub type TransactionOutputs = (Txid, Vec<(u32, TxOut)>); +// Because we have weird workarounds for `ChannelMonitor` equality checks in `OnchainTxHandler` and +// `PackageTemplate` the equality implementation isn't really fit for public consumption. Instead, +// we only expose it during tests. +#[cfg(any(feature = "_test_utils", test))] impl PartialEq for ChannelMonitor where Signer: PartialEq, { - #[rustfmt::skip] fn eq(&self, other: &Self) -> bool { + use crate::sync::LockTestExt; // We need some kind of total lockorder. Absent a better idea, we sort by position in // memory and take locks in that order (assuming that we can't move within memory while a // lock is held). let ord = ((self as *const _) as usize) < ((other as *const _) as usize); - let a = if ord { self.inner.unsafe_well_ordered_double_lock_self() } else { other.inner.unsafe_well_ordered_double_lock_self() }; - let b = if ord { other.inner.unsafe_well_ordered_double_lock_self() } else { self.inner.unsafe_well_ordered_double_lock_self() }; + let a = if ord { + self.inner.unsafe_well_ordered_double_lock_self() + } else { + other.inner.unsafe_well_ordered_double_lock_self() + }; + let b = if ord { + other.inner.unsafe_well_ordered_double_lock_self() + } else { + self.inner.unsafe_well_ordered_double_lock_self() + }; a.eq(&b) } } @@ -1555,6 +1591,7 @@ impl Writeable for ChannelMonitorImpl { (29, self.initial_counterparty_commitment_tx, option), (31, self.funding.channel_parameters, required), (32, self.pending_funding, optional_vec), + (33, self.htlcs_resolved_to_user, required), }); Ok(()) @@ -1767,6 +1804,7 @@ impl ChannelMonitor { funding_spend_confirmed: None, confirmed_commitment_tx_counterparty_output: None, htlcs_resolved_on_chain: Vec::new(), + htlcs_resolved_to_user: new_hash_set(), spendable_txids_confirmed: Vec::new(), best_block, @@ -2891,46 +2929,42 @@ impl ChannelMonitor { /// Gets the set of outbound HTLCs which can be (or have been) resolved by this /// `ChannelMonitor`. This is used to determine if an HTLC was removed from the channel prior /// to the `ChannelManager` having been persisted. - /// - /// This is similar to [`Self::get_pending_or_resolved_outbound_htlcs`] except it includes - /// HTLCs which were resolved on-chain (i.e. where the final HTLC resolution was done by an - /// event from this `ChannelMonitor`). - #[rustfmt::skip] - pub(crate) fn get_all_current_outbound_htlcs(&self) -> HashMap)> { + pub(crate) fn get_all_current_outbound_htlcs( + &self, + ) -> HashMap)> { let mut res = new_hash_map(); // Just examine the available counterparty commitment transactions. See docs on // `fail_unbroadcast_htlcs`, below, for justification. let us = self.inner.lock().unwrap(); - macro_rules! walk_counterparty_commitment { - ($txid: expr) => { - if let Some(ref latest_outpoints) = us.funding.counterparty_claimable_outpoints.get($txid) { - for &(ref htlc, ref source_option) in latest_outpoints.iter() { - if let &Some(ref source) = source_option { - res.insert((**source).clone(), (htlc.clone(), - us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned())); + let mut walk_counterparty_commitment = |txid| { + if let Some(latest_outpoints) = us.funding.counterparty_claimable_outpoints.get(txid) { + for &(ref htlc, ref source_option) in latest_outpoints.iter() { + if let &Some(ref source) = source_option { + let htlc_id = SentHTLCId::from_source(source); + if !us.htlcs_resolved_to_user.contains(&htlc_id) { + let preimage_opt = + us.counterparty_fulfilled_htlcs.get(&htlc_id).cloned(); + res.insert((**source).clone(), (htlc.clone(), preimage_opt)); } } } } - } + }; if let Some(ref txid) = us.funding.current_counterparty_commitment_txid { - walk_counterparty_commitment!(txid); + walk_counterparty_commitment(txid); } if let Some(ref txid) = us.funding.prev_counterparty_commitment_txid { - walk_counterparty_commitment!(txid); + walk_counterparty_commitment(txid); } res } - /// Gets the set of outbound HTLCs which are pending resolution in this channel or which were - /// resolved with a preimage from our counterparty. - /// - /// This is used to reconstruct pending outbound payments on restart in the ChannelManager. - /// - /// Currently, the preimage is unused, however if it is present in the relevant internal state - /// an HTLC is always included even if it has been resolved. - #[rustfmt::skip] - pub(crate) fn get_pending_or_resolved_outbound_htlcs(&self) -> HashMap)> { + /// Gets the set of outbound HTLCs which hit the chain and ultimately were claimed by us via + /// the timeout path and reached [`ANTI_REORG_DELAY`] confirmations. This is used to determine + /// if an HTLC has failed without the `ChannelManager` having seen it prior to being persisted. + pub(crate) fn get_onchain_failed_outbound_htlcs( + &self, + ) -> HashMap { let us = self.inner.lock().unwrap(); // We're only concerned with the confirmation count of HTLC transactions, and don't // actually care how many confirmations a commitment transaction may or may not have. Thus, @@ -2939,66 +2973,53 @@ impl ChannelMonitor { us.onchain_events_awaiting_threshold_conf.iter().find_map(|event| { if let OnchainEvent::FundingSpendConfirmation { .. } = event.event { Some(event.txid) - } else { None } + } else { + None + } }) }); if confirmed_txid.is_none() { - // If we have not seen a commitment transaction on-chain (ie the channel is not yet - // closed), just get the full set. - mem::drop(us); - return self.get_all_current_outbound_htlcs(); + return new_hash_map(); } let mut res = new_hash_map(); macro_rules! walk_htlcs { ($holder_commitment: expr, $htlc_iter: expr) => { for (htlc, source) in $htlc_iter { - if us.htlcs_resolved_on_chain.iter().any(|v| v.commitment_tx_output_idx == htlc.transaction_output_index) { - // We should assert that funding_spend_confirmed is_some() here, but we - // have some unit tests which violate HTLC transaction CSVs entirely and - // would fail. - // TODO: Once tests all connect transactions at consensus-valid times, we - // should assert here like we do in `get_claimable_balances`. - } else if htlc.offered == $holder_commitment { - // If the payment was outbound, check if there's an HTLCUpdate - // indicating we have spent this HTLC with a timeout, claiming it back - // and awaiting confirmations on it. - let htlc_update_confd = us.onchain_events_awaiting_threshold_conf.iter().any(|event| { - if let OnchainEvent::HTLCUpdate { commitment_tx_output_idx: Some(commitment_tx_output_idx), .. } = event.event { - // If the HTLC was timed out, we wait for ANTI_REORG_DELAY blocks - // before considering it "no longer pending" - this matches when we - // provide the ChannelManager an HTLC failure event. - Some(commitment_tx_output_idx) == htlc.transaction_output_index && - us.best_block.height >= event.height + ANTI_REORG_DELAY - 1 - } else if let OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, .. } = event.event { - // If the HTLC was fulfilled with a preimage, we consider the HTLC - // immediately non-pending, matching when we provide ChannelManager - // the preimage. - Some(commitment_tx_output_idx) == htlc.transaction_output_index - } else { false } - }); + let filter = |v: &&IrrevocablyResolvedHTLC| { + v.commitment_tx_output_idx == htlc.transaction_output_index + }; + if let Some(state) = us.htlcs_resolved_on_chain.iter().filter(filter).next() { if let Some(source) = source { - let counterparty_resolved_preimage_opt = - us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned(); - if !htlc_update_confd || counterparty_resolved_preimage_opt.is_some() { - res.insert(source.clone(), (htlc.clone(), counterparty_resolved_preimage_opt)); + if state.payment_preimage.is_none() { + let htlc_id = SentHTLCId::from_source(source); + if !us.htlcs_resolved_to_user.contains(&htlc_id) { + res.insert(source.clone(), htlc.clone()); + } } - } else { - panic!("Outbound HTLCs should have a source"); } } } - } + }; } let txid = confirmed_txid.unwrap(); - if Some(txid) == us.funding.current_counterparty_commitment_txid || Some(txid) == us.funding.prev_counterparty_commitment_txid { - walk_htlcs!(false, us.funding.counterparty_claimable_outpoints.get(&txid).unwrap().iter().filter_map(|(a, b)| { - if let &Some(ref source) = b { - Some((a, Some(&**source))) - } else { None } - })); + if Some(txid) == us.funding.current_counterparty_commitment_txid + || Some(txid) == us.funding.prev_counterparty_commitment_txid + { + walk_htlcs!( + false, + us.funding.counterparty_claimable_outpoints.get(&txid).unwrap().iter().filter_map( + |(a, b)| { + if let &Some(ref source) = b { + Some((a, Some(&**source))) + } else { + None + } + } + ) + ); } else if txid == us.funding.current_holder_commitment_tx.trust().txid() { walk_htlcs!(true, holder_commitment_htlcs!(us, CURRENT_WITH_SOURCES)); } else if let Some(prev_commitment_tx) = &us.funding.prev_holder_commitment_tx { @@ -3849,6 +3870,7 @@ impl ChannelMonitorImpl { if updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID || self.lockdown_from_offchain { assert_eq!(updates.updates.len(), 1); match updates.updates[0] { + ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {}, ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {}, // We should have already seen a `ChannelForceClosed` update if we're trying to // provide a preimage at this point. @@ -3976,6 +3998,10 @@ impl ChannelMonitorImpl { panic!("Attempted to replace shutdown script {} with {}", shutdown_script, scriptpubkey); } }, + ChannelMonitorUpdateStep::ReleasePaymentComplete { htlc } => { + log_trace!(logger, "HTLC {htlc:?} permanently and fully resolved"); + self.htlcs_resolved_to_user.insert(*htlc); + }, } } @@ -4006,6 +4032,7 @@ impl ChannelMonitorImpl { // talking to our peer. ChannelMonitorUpdateStep::PaymentPreimage { .. } => {}, ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {}, + ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {}, } } @@ -5490,6 +5517,7 @@ impl ChannelMonitorImpl { on_to_local_output_csv: None, }, }); + self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { source, payment_preimage: Some(payment_preimage), @@ -5513,6 +5541,7 @@ impl ChannelMonitorImpl { on_to_local_output_csv: None, }, }); + self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { source, payment_preimage: Some(payment_preimage), @@ -5856,6 +5885,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut funding_spend_confirmed = None; let mut htlcs_resolved_on_chain = Some(Vec::new()); + let mut htlcs_resolved_to_user = Some(new_hash_set()); let mut funding_spend_seen = Some(false); let mut counterparty_node_id = None; let mut confirmed_commitment_tx_counterparty_output = None; @@ -5888,6 +5918,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (29, initial_counterparty_commitment_tx, option), (31, channel_parameters, (option: ReadableArgs, None)), (32, pending_funding, optional_vec), + (33, htlcs_resolved_to_user, option), }); if let Some(payment_preimages_with_info) = payment_preimages_with_info { if payment_preimages_with_info.len() != payment_preimages.len() { @@ -6046,6 +6077,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP funding_spend_confirmed, confirmed_commitment_tx_counterparty_output, htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(), + htlcs_resolved_to_user: htlcs_resolved_to_user.unwrap(), spendable_txids_confirmed: spendable_txids_confirmed.unwrap(), best_block, diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 3de690e492f..bdc5774e598 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -1093,7 +1093,7 @@ enum PackageMalleability { /// /// As packages are time-sensitive, we fee-bump and rebroadcast them at scheduled intervals. /// Failing to confirm a package translate as a loss of funds for the user. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Eq)] pub struct PackageTemplate { // List of onchain outputs and solving data to generate satisfying witnesses. inputs: Vec<(BitcoinOutPoint, PackageSolvingData)>, @@ -1122,6 +1122,50 @@ pub struct PackageTemplate { height_timer: u32, } +impl PartialEq for PackageTemplate { + fn eq(&self, o: &Self) -> bool { + if self.inputs != o.inputs + || self.malleability != o.malleability + || self.feerate_previous != o.feerate_previous + || self.height_timer != o.height_timer + { + return false; + } + #[cfg(test)] + { + // In some cases we may reset `counterparty_spendable_height` to zero on reload, which + // can cause our test assertions that ChannelMonitors round-trip exactly to trip. Here + // we allow exactly the same case as we tweak in the `PackageTemplate` `Readable` + // implementation. + if self.counterparty_spendable_height == 0 { + for (_, input) in self.inputs.iter() { + if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { + htlc, .. + }) = input + { + if !htlc.offered && htlc.cltv_expiry != 0 { + return true; + } + } + } + } + if o.counterparty_spendable_height == 0 { + for (_, input) in o.inputs.iter() { + if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { + htlc, .. + }) = input + { + if !htlc.offered && htlc.cltv_expiry != 0 { + return true; + } + } + } + } + } + self.counterparty_spendable_height == o.counterparty_spendable_height + } +} + impl PackageTemplate { #[rustfmt::skip] pub(crate) fn can_merge_with(&self, other: &PackageTemplate, cur_height: u32) -> bool { diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 878608e14f6..5fc0f6c53bc 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -3870,6 +3870,7 @@ fn do_test_durable_preimages_on_closed_channel( }; nodes[0].node.force_close_broadcasting_latest_txn(&chan_id_ab, &node_b_id, err_msg).unwrap(); check_closed_event(&nodes[0], 1, reason, false, &[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); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6f411273aab..261bd205c2e 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1233,6 +1233,21 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, }, ); +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct PaymentCompleteUpdate { + counterparty_node_id: PublicKey, + channel_funding_outpoint: OutPoint, + channel_id: ChannelId, + htlc_id: SentHTLCId, +} + +impl_writeable_tlv_based!(PaymentCompleteUpdate, { + (1, channel_funding_outpoint, required), + (3, counterparty_node_id, required), + (5, channel_id, required), + (7, htlc_id, required), +}); + #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum EventCompletionAction { ReleaseRAAChannelMonitorUpdate { @@ -1240,6 +1255,9 @@ pub(crate) enum EventCompletionAction { channel_funding_outpoint: OutPoint, channel_id: ChannelId, }, + + /// Note that this action will be dropped on downgrade to LDK prior to 0.2! + ReleasePaymentCompleteChannelMonitorUpdate(PaymentCompleteUpdate), } impl_writeable_tlv_based_enum!(EventCompletionAction, (0, ReleaseRAAChannelMonitorUpdate) => { @@ -1248,7 +1266,8 @@ impl_writeable_tlv_based_enum!(EventCompletionAction, // Note that by the time we get past the required read above, channel_funding_outpoint will be // filled in, so we can safely unwrap it here. (3, channel_id, (default_value, ChannelId::v1_from_funding_outpoint(channel_funding_outpoint.0.unwrap()))), - } + }, + {1, ReleasePaymentCompleteChannelMonitorUpdate} => (), ); /// The source argument which is passed to [`ChannelManager::claim_mpp_part`]. @@ -3448,7 +3467,7 @@ macro_rules! handle_monitor_update_completion { $self.finalize_claims(updates.finalized_claimed_htlcs); for failure in updates.failed_htlcs.drain(..) { let receiver = HTLCHandlingFailureType::Forward { node_id: Some(counterparty_node_id), channel_id }; - $self.fail_htlc_backwards_internal(&failure.0, &failure.1, &failure.2, receiver); + $self.fail_htlc_backwards_internal(&failure.0, &failure.1, &failure.2, receiver, None); } } } } @@ -4121,7 +4140,8 @@ where let failure_reason = LocalHTLCFailureReason::ChannelClosed; let reason = HTLCFailReason::from_failure_code(failure_reason); let receiver = HTLCHandlingFailureType::Forward { node_id: Some(*counterparty_node_id), channel_id: *chan_id }; - self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver); + let (source, hash) = htlc_source; + self.fail_htlc_backwards_internal(&source, &hash, &reason, receiver, None); } let _ = handle_error!(self, shutdown_result, *counterparty_node_id); @@ -4255,7 +4275,7 @@ where let failure_reason = LocalHTLCFailureReason::ChannelClosed; let reason = HTLCFailReason::from_failure_code(failure_reason); let receiver = HTLCHandlingFailureType::Forward { node_id: Some(counterparty_node_id), channel_id }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver, None); } if let Some((_, funding_txo, _channel_id, monitor_update)) = shutdown_res.monitor_update { debug_assert!(false, "This should have been handled in `locked_close_channel`"); @@ -6159,7 +6179,8 @@ where let reason = HTLCFailReason::from_failure_code(LocalHTLCFailureReason::UnknownNextPeer); let destination = HTLCHandlingFailureType::InvalidForward { requested_forward_scid: short_channel_id }; - self.fail_htlc_backwards_internal(&htlc_source, &payment.forward_info.payment_hash, &reason, destination); + let hash = payment.forward_info.payment_hash; + self.fail_htlc_backwards_internal(&htlc_source, &hash, &reason, destination, None); } else { unreachable!() } // Only `PendingHTLCRouting::Forward`s are intercepted Ok(()) @@ -6448,6 +6469,7 @@ where &payment_hash, &failure_reason, destination, + None, ); } self.forward_htlcs(&mut phantom_receives); @@ -7657,7 +7679,7 @@ where let failure_reason = LocalHTLCFailureReason::MPPTimeout; let reason = HTLCFailReason::from_failure_code(failure_reason); let receiver = HTLCHandlingFailureType::Receive { payment_hash: htlc_source.1 }; - self.fail_htlc_backwards_internal(&source, &htlc_source.1, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &htlc_source.1, &reason, receiver, None); } for (err, counterparty_node_id) in handle_errors { @@ -7724,7 +7746,7 @@ where let reason = self.get_htlc_fail_reason_from_failure_code(failure_code, &htlc); let source = HTLCSource::PreviousHopData(htlc.prev_hop); let receiver = HTLCHandlingFailureType::Receive { payment_hash: *payment_hash }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver, None); } } } @@ -7817,7 +7839,7 @@ where node_id: Some(counterparty_node_id.clone()), channel_id, }; - self.fail_htlc_backwards_internal(&htlc_src, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&htlc_src, &payment_hash, &reason, receiver, None); } } @@ -7826,6 +7848,7 @@ where fn fail_htlc_backwards_internal( &self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, failure_type: HTLCHandlingFailureType, + mut from_monitor_update_completion: Option, ) { // Ensure that no peer state channel storage lock is held when calling this function. // This ensures that future code doesn't introduce a lock-order requirement for @@ -7857,7 +7880,27 @@ where &self.secp_ctx, &self.pending_events, &self.logger, + &mut from_monitor_update_completion, ); + if let Some(update) = from_monitor_update_completion { + // If `fail_htlc` didn't `take` the post-event action, we should go ahead and + // complete it here as the failure was duplicative - we've already handled it. + // This should mostly only happen on startup, but it is possible to hit it in + // rare cases where a MonitorUpdate is replayed after restart because a + // ChannelMonitor wasn't persisted after it was applied (even though the + // ChannelManager was). + // For such cases, we also check that there's no existing pending event to + // complete this action already, which we let finish instead. + let action = + EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(update); + let have_action = { + let pending_events = self.pending_events.lock().unwrap(); + pending_events.iter().any(|(_, act)| act.as_ref() == Some(&action)) + }; + if !have_action { + self.handle_post_event_actions([action]); + } + } }, HTLCSource::PreviousHopData(HTLCPreviousHopData { ref short_channel_id, @@ -7995,6 +8038,7 @@ where &payment_hash, &reason, receiver, + None, ); } return; @@ -8142,7 +8186,7 @@ where err_data, ); let receiver = HTLCHandlingFailureType::Receive { payment_hash }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver, None); } self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash); } @@ -8475,17 +8519,33 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ next_user_channel_id: Option, attribution_data: Option, send_timestamp: Option, ) { + debug_assert_eq!( + startup_replay, + !self.background_events_processed_since_startup.load(Ordering::Acquire) + ); + let htlc_id = SentHTLCId::from_source(&source); match source { HTLCSource::OutboundRoute { session_priv, payment_id, path, bolt12_invoice, .. } => { - debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire), + debug_assert!(!startup_replay, "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 ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint: next_channel_outpoint, - channel_id: next_channel_id, - counterparty_node_id: path.hops[0].pubkey, + + let mut ev_completion_action = if from_onchain { + let release = PaymentCompleteUpdate { + counterparty_node_id: next_channel_counterparty_node_id, + channel_funding_outpoint: next_channel_outpoint, + channel_id: next_channel_id, + htlc_id, + }; + Some(EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(release)) + } else { + Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { + channel_funding_outpoint: next_channel_outpoint, + channel_id: next_channel_id, + counterparty_node_id: path.hops[0].pubkey, + }) }; self.pending_outbound_payments.claim_htlc( payment_id, @@ -8494,10 +8554,22 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ session_priv, path, from_onchain, - ev_completion_action, + &mut ev_completion_action, &self.pending_events, &self.logger, ); + // 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. + let have_action = if ev_completion_action.is_some() { + let pending_events = self.pending_events.lock().unwrap(); + pending_events.iter().any(|(_, act)| *act == ev_completion_action) + } else { + false + }; + if !have_action { + self.handle_post_event_actions(ev_completion_action); + } }, HTLCSource::PreviousHopData(hop_data) => { let prev_channel_id = hop_data.channel_id; @@ -10030,7 +10102,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ channel_id: msg.channel_id, }; let reason = HTLCFailReason::from_failure_code(LocalHTLCFailureReason::ChannelClosed); - self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver); + let (source, hash) = htlc_source; + self.fail_htlc_backwards_internal(&source, &hash, &reason, receiver, None); } Ok(()) @@ -10508,6 +10581,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ &payment_hash, &failure_reason, destination, + None, ); } @@ -11096,11 +11170,18 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ 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, receiver, + completion_update, ); } }, @@ -12623,14 +12704,17 @@ where } } - fn handle_post_event_actions(&self, actions: Vec) { - for action in actions { + fn handle_post_event_actions>(&self, actions: I) { + for action in actions.into_iter() { match action { EventCompletionAction::ReleaseRAAChannelMonitorUpdate { channel_funding_outpoint, channel_id, counterparty_node_id, } => { + let startup_complete = + self.background_events_processed_since_startup.load(Ordering::Acquire); + debug_assert!(startup_complete); self.handle_monitor_update_release( counterparty_node_id, channel_funding_outpoint, @@ -12638,6 +12722,57 @@ where None, ); }, + EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate( + PaymentCompleteUpdate { + counterparty_node_id, + channel_funding_outpoint, + channel_id, + htlc_id, + }, + ) => { + let per_peer_state = self.per_peer_state.read().unwrap(); + let mut peer_state = per_peer_state + .get(&counterparty_node_id) + .map(|state| state.lock().unwrap()) + .expect("Channels originating a preimage must have peer state"); + let update_id = peer_state + .closed_channel_monitor_update_ids + .get_mut(&channel_id) + .expect("Channels originating a preimage must have a monitor"); + *update_id += 1; + + let update = ChannelMonitorUpdate { + update_id: *update_id, + channel_id: Some(channel_id), + updates: vec![ChannelMonitorUpdateStep::ReleasePaymentComplete { + htlc: htlc_id, + }], + }; + + let during_startup = + !self.background_events_processed_since_startup.load(Ordering::Acquire); + if during_startup { + let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id, + funding_txo: channel_funding_outpoint, + channel_id, + update, + }; + self.pending_background_events.lock().unwrap().push(event); + } else { + handle_new_monitor_update!( + self, + channel_funding_outpoint, + update, + peer_state, + peer_state, + per_peer_state, + counterparty_node_id, + channel_id, + POST_CHANNEL_CLOSE + ); + } + }, } } } @@ -13479,7 +13614,7 @@ where } for (source, payment_hash, reason, destination) in timed_out_htlcs.drain(..) { - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, destination); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, destination, None); } } @@ -15650,7 +15785,7 @@ where log_error!(logger, " The ChannelMonitor for channel {} is at counterparty commitment transaction number {} but the ChannelManager is at counterparty commitment transaction number {}.", &channel.context.channel_id(), monitor.get_cur_counterparty_commitment_number(), channel.get_cur_counterparty_commitment_transaction_number()); } - let mut shutdown_result = + let shutdown_result = channel.force_shutdown(ClosureReason::OutdatedChannelManager); if shutdown_result.unbroadcasted_batch_funding_txid.is_some() { return Err(DecodeError::InvalidValue); @@ -15682,7 +15817,10 @@ where }, ); } - failed_htlcs.append(&mut shutdown_result.dropped_outbound_htlcs); + for (source, hash, cp_id, chan_id) in shutdown_result.dropped_outbound_htlcs { + let reason = LocalHTLCFailureReason::ChannelClosed; + failed_htlcs.push((source, hash, cp_id, chan_id, reason, None)); + } channel_closures.push_back(( events::Event::ChannelClosed { channel_id: channel.context.channel_id(), @@ -15724,6 +15862,8 @@ where *payment_hash, channel.context.get_counterparty_node_id(), channel.context.channel_id(), + LocalHTLCFailureReason::ChannelClosed, + None, )); } } @@ -16297,6 +16437,10 @@ where // payments which are still in-flight via their on-chain state. // We only rebuild the pending payments map if we were most recently serialized by // 0.0.102+ + // + // First we rebuild the pending payments, and only once we do so we go through and + // re-claim and re-fail pending payments. This avoids edge-cases around MPP payments + // resulting in redundant actions. for (channel_id, monitor) in args.channel_monitors.iter() { let mut is_channel_closed = false; let counterparty_node_id = monitor.get_counterparty_node_id(); @@ -16307,8 +16451,7 @@ where } if is_channel_closed { - for (htlc_source, (htlc, _)) in monitor.get_pending_or_resolved_outbound_htlcs() - { + for (htlc_source, (htlc, _)) in monitor.get_all_current_outbound_htlcs() { let logger = WithChannelMonitor::from( &args.logger, monitor, @@ -16335,6 +16478,18 @@ where ); } } + } + } + for (channel_id, monitor) in args.channel_monitors.iter() { + let mut is_channel_closed = false; + let counterparty_node_id = monitor.get_counterparty_node_id(); + if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) { + let mut peer_state_lock = peer_state_mtx.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + is_channel_closed = !peer_state.channel_by_id.contains_key(channel_id); + } + + if is_channel_closed { for (htlc_source, (htlc, preimage_opt)) in monitor.get_all_current_outbound_htlcs() { @@ -16343,6 +16498,7 @@ where monitor, Some(htlc.payment_hash), ); + let htlc_id = SentHTLCId::from_source(&htlc_source); match htlc_source { HTLCSource::PreviousHopData(prev_hop_data) => { let pending_forward_matches_htlc = |info: &PendingAddHTLCInfo| { @@ -16400,36 +16556,88 @@ where } => { if let Some(preimage) = preimage_opt { let pending_events = Mutex::new(pending_events_read); - // Note that we set `from_onchain` to "false" here, - // deliberately keeping the pending payment around forever. - // Given it should only occur when we have a channel we're - // force-closing for being stale that's okay. - // The alternative would be to wipe the state when claiming, - // generating a `PaymentPathSuccessful` event but regenerating - // it and the `PaymentSent` on every restart until the - // `ChannelMonitor` is removed. - let compl_action = - EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint: monitor.get_funding_txo(), - channel_id: monitor.channel_id(), - counterparty_node_id: path.hops[0].pubkey, - }; + let update = PaymentCompleteUpdate { + counterparty_node_id: monitor.get_counterparty_node_id(), + channel_funding_outpoint: monitor.get_funding_txo(), + channel_id: monitor.channel_id(), + htlc_id, + }; + let mut compl_action = Some( + EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(update) + ); pending_outbounds.claim_htlc( payment_id, preimage, bolt12_invoice, session_priv, path, - false, - compl_action, + true, + &mut compl_action, &pending_events, &&logger, ); + // If the completion action was not consumed, then there was no + // payment to claim, and we need to tell the `ChannelMonitor` + // we don't need to hear about the HTLC again, at least as long + // as the PaymentSent event isn't still sitting around in our + // event queue. + let have_action = if compl_action.is_some() { + let pending_events = pending_events.lock().unwrap(); + pending_events.iter().any(|(_, act)| *act == compl_action) + } else { + false + }; + if !have_action && compl_action.is_some() { + let mut peer_state = per_peer_state + .get(&counterparty_node_id) + .map(|state| state.lock().unwrap()) + .expect("Channels originating a preimage must have peer state"); + let update_id = peer_state + .closed_channel_monitor_update_ids + .get_mut(channel_id) + .expect("Channels originating a preimage must have a monitor"); + *update_id += 1; + + pending_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id: monitor.get_counterparty_node_id(), + funding_txo: monitor.get_funding_txo(), + channel_id: monitor.channel_id(), + update: ChannelMonitorUpdate { + update_id: *update_id, + channel_id: Some(monitor.channel_id()), + updates: vec![ChannelMonitorUpdateStep::ReleasePaymentComplete { + htlc: htlc_id, + }], + }, + }); + } pending_events_read = pending_events.into_inner().unwrap(); } }, } } + for (htlc_source, htlc) in monitor.get_onchain_failed_outbound_htlcs() { + log_info!( + args.logger, + "Failing HTLC with payment hash {} as it was resolved on-chain.", + htlc.payment_hash + ); + let completion_action = Some(PaymentCompleteUpdate { + counterparty_node_id: monitor.get_counterparty_node_id(), + channel_funding_outpoint: monitor.get_funding_txo(), + channel_id: monitor.channel_id(), + htlc_id: SentHTLCId::from_source(&htlc_source), + }); + + failed_htlcs.push(( + htlc_source, + htlc.payment_hash, + monitor.get_counterparty_node_id(), + monitor.channel_id(), + LocalHTLCFailureReason::OnChainTimeout, + completion_action, + )); + } } // Whether the downstream channel was closed or not, try to re-apply any payment @@ -17112,15 +17320,14 @@ where } } - for htlc_source in failed_htlcs.drain(..) { - let (source, payment_hash, counterparty_node_id, channel_id) = htlc_source; - let failure_reason = LocalHTLCFailureReason::ChannelClosed; - let receiver = HTLCHandlingFailureType::Forward { - node_id: Some(counterparty_node_id), - channel_id, - }; + for htlc_source in failed_htlcs { + let (source, hash, counterparty_id, channel_id, failure_reason, ev_action) = + htlc_source; + let receiver = + HTLCHandlingFailureType::Forward { node_id: Some(counterparty_id), channel_id }; let reason = HTLCFailReason::from_failure_code(failure_reason); - channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + channel_manager + .fail_htlc_backwards_internal(&source, &hash, &reason, receiver, ev_action); } for ( diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 00e883e8561..099ad6e4b5b 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2798,6 +2798,9 @@ pub fn expect_payment_sent>( expected_fee_msat_opt: Option>, expect_per_path_claims: bool, expect_post_ev_mon_update: bool, ) -> (Option, Vec) { + if expect_post_ev_mon_update { + check_added_monitors(node, 0); + } let events = node.node().get_and_clear_pending_events(); let expected_payment_hash = PaymentHash( bitcoin::hashes::sha256::Hash::hash(&expected_payment_preimage.0).to_byte_array(), @@ -3047,6 +3050,7 @@ pub struct PaymentFailedConditions<'a> { pub(crate) expected_blamed_chan_closed: Option, pub(crate) expected_mpp_parts_remain: bool, pub(crate) retry_expected: bool, + pub(crate) from_mon_update: bool, } impl<'a> PaymentFailedConditions<'a> { @@ -3057,6 +3061,7 @@ impl<'a> PaymentFailedConditions<'a> { expected_blamed_chan_closed: None, expected_mpp_parts_remain: false, retry_expected: false, + from_mon_update: false, } } pub fn mpp_parts_remain(mut self) -> Self { @@ -3081,6 +3086,10 @@ impl<'a> PaymentFailedConditions<'a> { self.retry_expected = true; self } + pub fn from_mon_update(mut self) -> Self { + self.from_mon_update = true; + self + } } #[cfg(any(test, feature = "_externalize_tests"))] @@ -3190,7 +3199,13 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>( node: &'a Node<'b, 'c, 'd>, expected_payment_hash: PaymentHash, expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e>, ) { + if conditions.from_mon_update { + check_added_monitors(node, 0); + } let events = node.node.get_and_clear_pending_events(); + if conditions.from_mon_update { + check_added_monitors(node, 1); + } expect_payment_failed_conditions_event( events, expected_payment_hash, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 388db6d174f..7771db1ee30 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1528,7 +1528,8 @@ pub fn claim_htlc_outputs() { // ANTI_REORG_DELAY confirmations. mine_transaction(&nodes[1], accepted_claim); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[1], payment_hash_2, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], payment_hash_2, false, conditions); } get_announce_close_broadcast_events(&nodes, 0, 1); assert_eq!(nodes[0].node.list_channels().len(), 0); @@ -2015,6 +2016,7 @@ pub fn test_htlc_on_chain_success() { check_closed_broadcast!(nodes[0], true); check_added_monitors(&nodes[0], 1); let events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 2); assert_eq!(events.len(), 5); let mut first_claimed = false; for event in events { @@ -2433,7 +2435,11 @@ fn do_test_commitment_revoked_fail_backward_exhaustive( check_added_monitors(&nodes[1], 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + check_added_monitors(&nodes[1], 0); let events = nodes[1].node.get_and_clear_pending_events(); + if deliver_bs_raa { + check_added_monitors(&nodes[1], 1); + } assert_eq!(events.len(), if deliver_bs_raa { 3 + nodes.len() - 1 } else { 3 + nodes.len() }); assert!(events.iter().any(|ev| matches!( ev, @@ -4036,7 +4042,8 @@ pub fn test_static_spendable_outputs_timeout_tx() { mine_transaction(&nodes[1], &node_txn[0]); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [node_a_id], 100000); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[1], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], our_payment_hash, false, conditions); let spend_txn = check_spendable_outputs!(nodes[1], node_cfgs[1].keys_manager); assert_eq!(spend_txn.len(), 3); // SpendableOutput: remote_commitment_tx.to_remote, timeout_tx.output @@ -5109,7 +5116,8 @@ pub fn test_dynamic_spendable_outputs_local_htlc_timeout_tx() { mine_transaction(&nodes[0], &htlc_timeout); connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1); - expect_payment_failed!(nodes[0], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], our_payment_hash, false, conditions); // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor let spend_txn = check_spendable_outputs!(nodes[0], node_cfgs[0].keys_manager); @@ -5229,7 +5237,8 @@ pub fn test_key_derivation_params() { mine_transaction(&nodes[0], &htlc_timeout); connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1); - expect_payment_failed!(nodes[0], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], our_payment_hash, false, conditions); // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor let new_keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet); @@ -6233,7 +6242,9 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + check_added_monitors(&nodes[0], 0); let events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 2); // Only 2 PaymentPathFailed events should show up, over-dust HTLC has to be failed by timeout tx assert_eq!(events.len(), 4); let mut first_failed = false; @@ -6302,12 +6313,14 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { // We fail dust-HTLC 1 by broadcast of local commitment tx mine_transaction(&nodes[0], &as_commitment_tx[0]); check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed, [node_b_id], 100000); + check_closed_broadcast!(nodes[0], true); + check_added_monitors(&nodes[0], 1); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], dust_hash, false, conditions); connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS - ANTI_REORG_DELAY); - check_closed_broadcast!(nodes[0], true); - check_added_monitors(&nodes[0], 1); + check_added_monitors(&nodes[0], 0); assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); timeout_tx.push(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0].clone()); assert_eq!( @@ -6318,7 +6331,8 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); mine_transaction(&nodes[0], &timeout_tx[0]); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], non_dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], non_dust_hash, false, conditions); } else { // We fail dust-HTLC 1 by broadcast of remote commitment tx. If revoked, fail also non-dust HTLC mine_transaction(&nodes[0], &bs_commitment_tx[0]); @@ -6339,7 +6353,8 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { check_spends!(timeout_tx[0], bs_commitment_tx[0]); // For both a revoked or non-revoked commitment transaction, after ANTI_REORG_DELAY the // dust HTLC should have been failed. - expect_payment_failed!(nodes[0], dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], dust_hash, false, conditions); if !revoked { assert_eq!( @@ -6353,7 +6368,8 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { mine_transaction(&nodes[0], &timeout_tx[0]); assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], non_dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], non_dust_hash, false, conditions); } } @@ -8429,7 +8445,8 @@ pub fn test_htlc_no_detection() { &create_dummy_block(nodes[0].best_block_hash(), 42, vec![htlc_timeout.clone()]), ); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], our_payment_hash, false, conditions); } fn do_test_onchain_htlc_settlement_after_close( diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 13b9301084b..c1d5da5f45b 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -12,6 +12,7 @@ //! Further functional tests which test blockchain reorganizations. use crate::sign::{ecdsa::EcdsaChannelSigner, OutputSpender, SignerProvider, SpendableOutputDescriptor}; +use crate::chain::Watch; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE, Balance, BalanceSource, ChannelMonitorUpdateStep}; use crate::chain::transaction::OutPoint; use crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator, compute_feerate_sat_per_1000_weight}; @@ -164,7 +165,8 @@ fn revoked_output_htlc_resolution_timing() { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[1], payment_hash_1, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], payment_hash_1, false, conditions); } #[test] @@ -271,7 +273,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, false); + expect_payment_sent(&nodes[0], payment_preimage, None, true, true); 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); @@ -703,7 +705,8 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(chan_id).unwrap().get_claimable_balances())); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], dust_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], dust_payment_hash, false, conditions); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); // After ANTI_REORG_DELAY, A will consider its balance fully spendable and generate a @@ -726,8 +729,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { mine_transaction(&nodes[0], &b_broadcast_txn[0]); if prev_commitment_tx { expect_payment_path_successful!(nodes[0]); + check_added_monitors(&nodes[0], 1); } else { - expect_payment_sent(&nodes[0], payment_preimage, None, true, false); + expect_payment_sent(&nodes[0], payment_preimage, None, true, true); } 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())); @@ -759,7 +763,8 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); assert_eq!(Vec::::new(), nodes[0].chain_monitor.chain_monitor.get_monitor(chan_id).unwrap().get_claimable_balances()); - expect_payment_failed!(nodes[0], timeout_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], timeout_payment_hash, false, conditions); test_spendable_output(&nodes[0], &a_htlc_timeout_tx, false); @@ -974,7 +979,7 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { // 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, false); + expect_payment_sent(&nodes[0], payment_preimage_2, None, true, true); 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, @@ -1016,7 +1021,8 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { // panicked as described in the test introduction. This will remove the "maybe claimable" // spendable output as nodes[1] has fully claimed the second HTLC. connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { amount_satoshis: 1_000_000 - 10_000 - 20_000 - commitment_tx_fee - anchor_outputs_value, @@ -1246,7 +1252,8 @@ fn test_no_preimage_inbound_htlc_balances() { // Once as_htlc_timeout_claim[0] reaches ANTI_REORG_DELAY confirmations, we should get a // payment failure event. connect_blocks(&nodes[0], ANTI_REORG_DELAY - 2); - expect_payment_failed!(nodes[0], to_b_failed_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], to_b_failed_payment_hash, false, conditions); connect_blocks(&nodes[0], 1); assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { @@ -1294,7 +1301,8 @@ fn test_no_preimage_inbound_htlc_balances() { sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(chan_id).unwrap().get_claimable_balances())); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2); - expect_payment_failed!(nodes[1], to_a_failed_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], to_a_failed_payment_hash, false, conditions); assert_eq!(vec![b_received_htlc_balance.clone()], nodes[1].chain_monitor.chain_monitor.get_monitor(chan_id).unwrap().get_claimable_balances()); @@ -1554,7 +1562,9 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ connect_blocks(&nodes[1], 3); test_spendable_output(&nodes[1], &as_revoked_txn[0], false); + check_added_monitors(&nodes[1], 0); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[1], 2); expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), missing_htlc_payment_hash, false, PaymentFailedConditions::new()); expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), @@ -1563,7 +1573,9 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ connect_blocks(&nodes[1], 1); if confirm_htlc_spend_first { test_spendable_output(&nodes[1], &claim_txn[0], false); + check_added_monitors(&nodes[1], 0); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[1], 2); expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), live_payment_hash, false, PaymentFailedConditions::new()); expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), @@ -1576,7 +1588,9 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ test_spendable_output(&nodes[1], &claim_txn[1], false); } else { test_spendable_output(&nodes[1], &claim_txn[0], false); + check_added_monitors(&nodes[1], 0); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[1], 2); expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), live_payment_hash, false, PaymentFailedConditions::new()); expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), @@ -2024,7 +2038,7 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { as_revoked_txn[1].clone() }; mine_transaction(&nodes[1], &htlc_success_claim); - expect_payment_sent(&nodes[1], claimed_payment_preimage, None, true, false); + expect_payment_sent(&nodes[1], claimed_payment_preimage, None, true, true); 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 @@ -2125,7 +2139,8 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); // We shouldn't fail the payment until we spend the output connect_blocks(&nodes[1], 5); - expect_payment_failed!(nodes[1], revoked_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], revoked_payment_hash, false, conditions); let spendable_output_events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); assert_eq!(spendable_output_events.len(), 2); for event in spendable_output_events { @@ -2597,7 +2612,8 @@ fn do_test_yield_anchors_events(have_htlcs: bool) { connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); - expect_payment_failed!(nodes[0], payment_hash_1.unwrap(), false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], payment_hash_1.unwrap(), false, conditions); connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32); @@ -3211,3 +3227,350 @@ fn test_update_replay_panics() { monitor.update_monitor(&updates[2], &nodes[1].tx_broadcaster, &nodes[1].fee_estimator, &nodes[1].logger).unwrap(); monitor.update_monitor(&updates[3], &nodes[1].tx_broadcaster, &nodes[1].fee_estimator, &nodes[1].logger).unwrap(); } + +fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool) { + // `MonitorEvent`s aren't delivered to the `ChannelManager` in a durable fasion - if the + // `ChannelManager` fetches the pending `MonitorEvent`s, then the `ChannelMonitor` gets + // persisted (i.e. due to a block update) then the node crashes, prior to persisting the + // `ChannelManager` again, the `MonitorEvent` and its effects on the `ChannelManger` will be + // lost. This isn't likely in a sync persist environment, but in an async one this could be an + // issue. + // + // Note that this is only an issue for closed channels - `MonitorEvent`s only inform the + // `ChannelManager` that a channel is closed (which the `ChannelManager` will learn on startup + // or when it next tries to advance the channel state), that `ChannelMonitorUpdate` writes + // completed (which the `ChannelManager` will detect on startup), or that HTLCs resolved + // on-chain post closure. Of the three, only the last is problematic to lose prior to a reload. + // + // Here we test that losing `MonitorEvent`s that contain HTLC resolution preimages does not + // cause us to lose funds or miss a `PaymentSent` event. + let mut cfg = test_default_channel_config(); + cfg.manually_accept_inbound_channels = true; + cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; + let cfgs = [Some(cfg.clone()), Some(cfg.clone()), Some(cfg.clone())]; + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let persister; + let new_chain_mon; + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &cfgs); + let node_b_reload; + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let coinbase_tx = provide_anchor_reserves(&nodes); + + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0).2; + let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).2; + + let (preimage_a, hash_a, ..) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000); + let (preimage_b, hash_b, ..) = route_payment(&nodes[1], &[&nodes[2]], 1_000_000); + + nodes[1].node.peer_disconnected(nodes[2].node.get_our_node_id()); + nodes[2].node.peer_disconnected(nodes[1].node.get_our_node_id()); + + nodes[2].node.claim_funds(preimage_a); + check_added_monitors(&nodes[2], 1); + expect_payment_claimed!(nodes[2], hash_a, 1_000_000); + nodes[2].node.claim_funds(preimage_b); + check_added_monitors(&nodes[2], 1); + expect_payment_claimed!(nodes[2], hash_b, 1_000_000); + + // Force-close the channel, confirming a commitment transaction then letting C claim the HTLCs. + let message = "Closed".to_owned(); + nodes[2] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_b_id, message.clone()) + .unwrap(); + check_added_monitors(&nodes[2], 1); + let c_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message }; + check_closed_event!(nodes[2], 1, c_reason, [node_b_id], 1_000_000); + check_closed_broadcast!(nodes[2], true); + + let cs_commit_tx = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(cs_commit_tx.len(), 1); + + let message = "Closed".to_owned(); + nodes[1] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_c_id, message.clone()) + .unwrap(); + check_added_monitors(&nodes[1], 1); + let b_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message }; + check_closed_event!(nodes[1], 1, b_reason, [node_c_id], 1_000_000); + check_closed_broadcast!(nodes[1], true); + + let bs_commit_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(bs_commit_tx.len(), 1); + + let selected_commit_tx = if on_counterparty_tx { + &cs_commit_tx[0] + } else { + &bs_commit_tx[0] + }; + + mine_transaction(&nodes[2], selected_commit_tx); + let mut cs_transactions = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + let c_replays_commitment = nodes[2].connect_style.borrow().updates_best_block_first(); + let cs_htlc_claims = if on_counterparty_tx { + assert_eq!(cs_transactions.len(), if c_replays_commitment { 1 } else { 0 }); + + let mut events = nodes[2].chain_monitor.chain_monitor.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events.pop().unwrap() { + Event::BumpTransaction(bump_event) => { + nodes[2].bump_tx_handler.handle_event(&bump_event); + }, + _ => panic!("Unexpected event"), + } + + nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0) + } else { + assert_eq!(cs_transactions.len(), if c_replays_commitment { 2 } else { 1 }); + vec![cs_transactions.pop().unwrap()] + }; + + assert_eq!(cs_htlc_claims.len(), 1); + check_spends!(cs_htlc_claims[0], selected_commit_tx, coinbase_tx); + + // Now replay the claims on node B, which should generate preimage-containing `MonitorUpdate`s + mine_transaction(&nodes[1], selected_commit_tx); + mine_transaction(&nodes[1], &cs_htlc_claims[0]); + + // Now simulate a restart where the B<->C ChannelMonitor has been persisted (i.e. because we + // just processed a new block) but the ChannelManager was not. This should be exceedingly rare + // given we have to be connecting a block at the right moment and not manage to get a + // ChannelManager persisted after it does a thing that should immediately precede persistence, + // but with async persist it is more common. + // + // We do this by wiping the `MonitorEvent`s from the monitors and then reloading with the + // latest state. + let mon_events = nodes[1].chain_monitor.chain_monitor.release_pending_monitor_events(); + assert_eq!(mon_events.len(), 1); + assert_eq!(mon_events[0].2.len(), 2); + + let node_ser = nodes[1].node.encode(); + let mon_a_ser = get_monitor!(nodes[1], chan_a).encode(); + let mon_b_ser = get_monitor!(nodes[1], chan_b).encode(); + let mons = &[&mon_a_ser[..], &mon_b_ser[..]]; + reload_node!(nodes[1], cfg, &node_ser, mons, persister, new_chain_mon, node_b_reload); + + check_added_monitors(&nodes[1], 0); + let preimage_events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(preimage_events.len(), 3, "{preimage_events:?}"); + for ev in preimage_events { + match ev { + Event::PaymentSent { payment_hash, .. } => { + assert_eq!(payment_hash, hash_b); + }, + Event::PaymentPathSuccessful { payment_hash, .. } => { + assert_eq!(payment_hash, Some(hash_b)); + }, + Event::PaymentForwarded { claim_from_onchain_tx, .. } => { + assert!(claim_from_onchain_tx); + }, + _ => panic!("Wrong event {ev:?}"), + } + } + + // 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); + + 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); +} + +#[test] +fn test_lost_preimage_monitor_events() { + do_test_lost_preimage_monitor_events(true); + do_test_lost_preimage_monitor_events(false); +} + +fn do_test_lost_timeout_monitor_events(on_counterparty_tx: bool) { + // `MonitorEvent`s aren't delivered to the `ChannelManager` in a durable fasion - if the + // `ChannelManager` fetches the pending `MonitorEvent`s, then the `ChannelMonitor` gets + // persisted (i.e. due to a block update) then the node crashes, prior to persisting the + // `ChannelManager` again, the `MonitorEvent` and its effects on the `ChannelManger` will be + // lost. This isn't likely in a sync persist environment, but in an async one this could be an + // issue. + // + // Note that this is only an issue for closed channels - `MonitorEvent`s only inform the + // `ChannelManager` that a channel is closed (which the `ChannelManager` will learn on startup + // or when it next tries to advance the channel state), that `ChannelMonitorUpdate` writes + // completed (which the `ChannelManager` will detect on startup), or that HTLCs resolved + // on-chain post closure. Of the three, only the last is problematic to lose prior to a reload. + // + // Here we test that losing `MonitorEvent`s that contain HTLC resolution via timeouts does not + // cause us to lose a `PaymentFailed` event. + let mut cfg = test_default_channel_config(); + cfg.manually_accept_inbound_channels = true; + cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; + let cfgs = [Some(cfg.clone()), Some(cfg.clone()), Some(cfg.clone())]; + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let persister; + let new_chain_mon; + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &cfgs); + let node_b_reload; + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + provide_anchor_reserves(&nodes); + + 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_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0).2; + let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).2; + + // Ensure all nodes are at the same height + let node_max_height = + nodes.iter().map(|node| node.blocks.lock().unwrap().len()).max().unwrap() as u32; + connect_blocks(&nodes[0], node_max_height - nodes[0].best_block_info().1); + connect_blocks(&nodes[1], node_max_height - nodes[1].best_block_info().1); + connect_blocks(&nodes[2], node_max_height - nodes[2].best_block_info().1); + + let (_, hash_a, ..) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5_000_000); + let (_, hash_b, ..) = route_payment(&nodes[1], &[&nodes[2]], 5_000_000); + + nodes[1].node.peer_disconnected(nodes[2].node.get_our_node_id()); + nodes[2].node.peer_disconnected(nodes[1].node.get_our_node_id()); + + // Force-close the channel, confirming a commitment transaction then letting C claim the HTLCs. + let message = "Closed".to_owned(); + nodes[2] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_b_id, message.clone()) + .unwrap(); + check_added_monitors(&nodes[2], 1); + let c_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message }; + check_closed_event!(nodes[2], 1, c_reason, [node_b_id], 1_000_000); + check_closed_broadcast!(nodes[2], true); + + let cs_commit_tx = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(cs_commit_tx.len(), 1); + + let message = "Closed".to_owned(); + nodes[1] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_c_id, message.clone()) + .unwrap(); + check_added_monitors(&nodes[1], 1); + let b_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message }; + check_closed_event!(nodes[1], 1, b_reason, [node_c_id], 1_000_000); + check_closed_broadcast!(nodes[1], true); + + let bs_commit_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(bs_commit_tx.len(), 1); + + let selected_commit_tx = if on_counterparty_tx { + &cs_commit_tx[0] + } else { + &bs_commit_tx[0] + }; + + mine_transaction(&nodes[1], selected_commit_tx); + // If the block gets connected first we may re-broadcast B's commitment transaction before + // seeing the C's confirm. + nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + let mut events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); + if on_counterparty_tx { + assert_eq!(events.len(), 1, "{events:?}"); + match events[0] { + Event::SpendableOutputs { .. } => {}, + _ => panic!("Unexpected event {events:?}"), + } + } else { + assert_eq!(events.len(), 0); + } + + connect_blocks(&nodes[1], TEST_FINAL_CLTV - ANTI_REORG_DELAY + 1); + if !on_counterparty_tx { + let mut events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); + assert_eq!(events.len(), 1, "{events:?}"); + match events.pop().unwrap() { + Event::BumpTransaction(bump_event) => { + nodes[1].bump_tx_handler.handle_event(&bump_event); + }, + _ => panic!("Unexpected event"), + } + } + let bs_htlc_timeouts = + nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(bs_htlc_timeouts.len(), 1); + + // Now replay the timeouts on node B, which after 6 confirmations should fail the HTLCs via + // `MonitorUpdate`s + mine_transactions(&nodes[1], &bs_htlc_timeouts.iter().collect::>()); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + + // Now simulate a restart where the B<->C ChannelMonitor has been persisted (i.e. because we + // just processed a new block) but the ChannelManager was not. This should be exceedingly rare + // given we have to be connecting a block at the right moment and not manage to get a + // ChannelManager persisted after it does a thing that should immediately precede persistence, + // but with async persist it is more common. + // + // We do this by wiping the `MonitorEvent`s from the monitors and then reloading with the + // latest state. + let mon_events = nodes[1].chain_monitor.chain_monitor.release_pending_monitor_events(); + assert_eq!(mon_events.len(), 1); + assert_eq!(mon_events[0].2.len(), 2); + + let node_ser = nodes[1].node.encode(); + let mon_a_ser = get_monitor!(nodes[1], chan_a).encode(); + let mon_b_ser = get_monitor!(nodes[1], chan_b).encode(); + let mons = &[&mon_a_ser[..], &mon_b_ser[..]]; + reload_node!(nodes[1], cfg, &node_ser, mons, persister, new_chain_mon, node_b_reload); + + // After reload, once we process the `PaymentFailed` event, the sent HTLC will be marked + // handled so that we won't ever see the event again. + check_added_monitors(&nodes[1], 0); + let timeout_events = nodes[1].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[1], 1); + assert_eq!(timeout_events.len(), 3, "{timeout_events:?}"); + for ev in timeout_events { + match ev { + Event::PaymentPathFailed { payment_hash, .. } => { + assert_eq!(payment_hash, hash_b); + }, + Event::PaymentFailed { payment_hash, .. } => { + assert_eq!(payment_hash, Some(hash_b)); + }, + Event::HTLCHandlingFailed { prev_channel_id, .. } => { + assert_eq!(prev_channel_id, chan_a); + }, + _ => panic!("Wrong event {ev:?}"), + } + } + + 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_fails = (0, 0); + reconnect_nodes(reconnect_args); + + nodes[1].node.process_pending_htlc_forwards(); + check_added_monitors(&nodes[1], 1); + let bs_fail = get_htlc_update_msgs(&nodes[1], &node_a_id); + nodes[0].node.handle_update_fail_htlc(node_b_id, &bs_fail.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], bs_fail.commitment_signed, true, true); + expect_payment_failed!(nodes[0], hash_a, false); +} + +#[test] +fn test_lost_timeout_monitor_events() { + do_test_lost_timeout_monitor_events(true); + do_test_lost_timeout_monitor_events(false); +} diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index ffc3ee4ae19..de2154a8b4b 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -17,7 +17,9 @@ use lightning_invoice::Bolt11Invoice; use crate::blinded_path::{IntroductionNode, NodeIdLookUp}; use crate::events::{self, PaidBolt12Invoice, PaymentFailureReason}; use crate::ln::channel_state::ChannelDetails; -use crate::ln::channelmanager::{EventCompletionAction, HTLCSource, PaymentId}; +use crate::ln::channelmanager::{ + EventCompletionAction, HTLCSource, PaymentCompleteUpdate, PaymentId, +}; use crate::ln::onion_utils; use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason}; use crate::offers::invoice::Bolt12Invoice; @@ -2160,7 +2162,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: EventCompletionAction, + session_priv: SecretKey, path: Path, from_onchain: bool, ev_completion_action: &mut Option, pending_events: &Mutex)>>, logger: &L, ) where L::Target: Logger { @@ -2181,7 +2183,7 @@ impl OutboundPayments { amount_msat, fee_paid_msat, bolt12_invoice: bolt12_invoice, - }, Some(ev_completion_action.clone()))); + }, ev_completion_action.take())); payment.get_mut().mark_fulfilled(); } @@ -2199,7 +2201,7 @@ impl OutboundPayments { payment_hash, path, hold_times: Vec::new(), - }, Some(ev_completion_action))); + }, ev_completion_action.take())); } } } else { @@ -2328,7 +2330,7 @@ impl OutboundPayments { path: &Path, session_priv: &SecretKey, payment_id: &PaymentId, probing_cookie_secret: [u8; 32], secp_ctx: &Secp256k1, pending_events: &Mutex)>>, - logger: &L, + logger: &L, completion_action: &mut Option, ) where L::Target: Logger, { @@ -2479,9 +2481,14 @@ impl OutboundPayments { } }; let mut pending_events = pending_events.lock().unwrap(); - pending_events.push_back((path_failure, None)); + let completion_action = completion_action + .take() + .map(|act| EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(act)); if let Some(ev) = full_failure_ev { - pending_events.push_back((ev, None)); + pending_events.push_back((path_failure, None)); + pending_events.push_back((ev, completion_action)); + } else { + pending_events.push_back((path_failure, completion_action)); } } diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index b11294f1158..e3954fd804a 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -14,7 +14,7 @@ use crate::chain::channelmonitor::{ ANTI_REORG_DELAY, HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, }; -use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen}; +use crate::chain::{Confirm, Listen}; use crate::events::{ ClosureReason, Event, HTLCHandlingFailureType, PathFailure, PaymentFailureReason, PaymentPurpose, @@ -933,7 +933,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, false); + expect_payment_sent(&nodes[0], payment_preimage_1, None, true, true); 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(); @@ -949,7 +949,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { confirm_transaction(&nodes[0], &first_htlc_timeout_tx); } nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); - let conditions = PaymentFailedConditions::new(); + let conditions = PaymentFailedConditions::new().from_mon_update(); expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); // Finally, retry the payment (which was reloaded from the ChannelMonitor when nodes[0] was @@ -1164,7 +1164,8 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { // (which should also still work). connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed_conditions(&nodes[0], hash, false, PaymentFailedConditions::new()); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], hash, false, conditions); let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); let chan_1_monitor_serialized = get_monitor!(nodes[0], chan_id_3).encode(); @@ -1231,7 +1232,8 @@ fn test_completed_payment_not_retryable_on_reload() { } fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( - persist_manager_post_event: bool, confirm_commitment_tx: bool, payment_timeout: bool, + persist_manager_post_event: bool, persist_monitor_after_events: bool, + confirm_commitment_tx: bool, payment_timeout: bool, ) { // When a Channel is closed, any outbound HTLCs which were relayed through it are simply // dropped. From there, the ChannelManager relies on the ChannelMonitor having a copy of the @@ -1310,16 +1312,15 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( connect_blocks(&nodes[0], ANTI_REORG_DELAY - 2); } - // Now connect the HTLC claim transaction with the ChainMonitor-generated ChannelMonitor update - // returning InProgress. This should cause the claim event to never make its way to the - // ChannelManager. - chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); - + // Now connect the HTLC claim transaction. Note that ChannelMonitors aren't re-persisted on + // each block connection (as the block being reconnected on startup should get us the same + // result). if payment_timeout { connect_blocks(&nodes[0], 1); } else { connect_block(&nodes[0], &claim_block); } + check_added_monitors(&nodes[0], 0); // Note that we skip persisting ChannelMonitors. We should still be generating the payment sent // event without ChannelMonitor persistence. If we reset to a previous state on reload, the block @@ -1332,28 +1333,65 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( node_a_ser = nodes[0].node.encode(); } - let mon_ser = get_monitor!(nodes[0], chan_id).encode(); + let mut mon_ser = Vec::new(); + if !persist_monitor_after_events { + mon_ser = get_monitor!(nodes[0], chan_id).encode(); + } if payment_timeout { - expect_payment_failed!(nodes[0], payment_hash, false); + 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, false); + expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + } + // Note that if we persist the monitor before processing the events, above, we'll always get + // them replayed on restart no matter what + if persist_monitor_after_events { + mon_ser = get_monitor!(nodes[0], chan_id).encode(); } // If we persist the ChannelManager after we get the PaymentSent event, we shouldn't get it // twice. if persist_manager_post_event { node_a_ser = nodes[0].node.encode(); + } else if persist_monitor_after_events { + // Persisting the monitor after the events (resulting in a new monitor being persisted) but + // didn't persist the manager will result in an FC, which we don't test here. + panic!(); } // Now reload nodes[0]... reload_node!(nodes[0], &node_a_ser, &[&mon_ser], persister, chain_monitor, node_a_reload); - if persist_manager_post_event { + check_added_monitors(&nodes[0], 0); + if persist_manager_post_event && persist_monitor_after_events { assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + check_added_monitors(&nodes[0], 0); } else if payment_timeout { - expect_payment_failed!(nodes[0], payment_hash, false); + let mut conditions = PaymentFailedConditions::new(); + if !persist_monitor_after_events { + conditions = conditions.from_mon_update(); + } + expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); + check_added_monitors(&nodes[0], 0); } else { - expect_payment_sent(&nodes[0], payment_preimage, None, true, false); + if persist_manager_post_event { + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + } 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); + } 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); + } } // Note that if we re-connect the block which exposed nodes[0] to the payment preimage (but @@ -1366,12 +1404,15 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( #[test] fn test_dup_htlc_onchain_doesnt_fail_on_reload() { - do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, false); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, false); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, true, true); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, true, false); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, true); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, false, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, true); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, false, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, true); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, false, false); } #[test] @@ -1626,7 +1667,9 @@ fn onchain_failed_probe_yields_event() { check_closed_broadcast!(&nodes[0], true); check_added_monitors!(nodes[0], 1); + check_added_monitors(&nodes[0], 0); let mut events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 1); assert_eq!(events.len(), 2); let mut found_probe_failed = false; for event in events.drain(..) { @@ -4085,8 +4128,15 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: let config = test_default_channel_config(); reload_node!(nodes[0], config, &node_a_ser, &[&mon_ser], persist_a, chain_monitor_a, node_a_1); + // When we first process background events, we'll apply a channel-closed monitor update... + 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(); - assert_eq!(events.len(), 2); + check_added_monitors(&nodes[0], 1); + assert_eq!(events.len(), 3, "{events:?}"); if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[0] { } else { panic!(); @@ -4096,6 +4146,10 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: } else { panic!(); } + if let Event::PaymentPathSuccessful { .. } = events[2] { + } else { + panic!(); + } // Note that we don't get a PaymentPathSuccessful here as we leave the HTLC pending to avoid // the double-claim that would otherwise appear at the end of this test. nodes[0].node.timer_tick_occurred(); @@ -4115,6 +4169,8 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: let node_ser = nodes[0].node.encode(); let config = test_default_channel_config(); reload_node!(nodes[0], config, &node_ser, &[&mon_ser], persist_b, chain_monitor_b, node_a_2); + + nodes[0].node.test_process_background_events(); let events = nodes[0].node.get_and_clear_pending_events(); assert!(events.is_empty()); @@ -4127,6 +4183,7 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: let events = nodes[0].node.get_and_clear_pending_events(); assert!(events.is_empty()); + check_added_monitors(&nodes[0], 0); let mon_ser = get_monitor!(nodes[0], chan_id).encode(); let config = test_default_channel_config(); diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index cbe0d8250ed..cfd6f102d5a 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -237,12 +237,13 @@ 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, false); + expect_payment_sent(&nodes[1], payment_preimage_3, None, true, true); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); // Connect blocks to confirm the unrevoked commitment transaction connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2); - expect_payment_failed!(nodes[1], payment_hash_4, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], payment_hash_4, false, conditions) } fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_unconfirmed: bool, connect_style: ConnectStyle) { @@ -1023,7 +1024,9 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) { let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); assert_eq!(txn.len(), 0); + check_added_monitors(&nodes[0], 0); let sent_events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 2); assert_eq!(sent_events.len(), 4, "{sent_events:?}"); let mut found_expected_events = [false, false, false, false]; for event in sent_events { @@ -1112,7 +1115,9 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) { // Connect two more blocks to get `as_third_htlc_spend_tx` to `ANTI_REORG_DELAY` confs. connect_blocks(&nodes[0], 2); if use_third_htlc { + check_added_monitors(&nodes[0], 0); let failed_events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 1); assert_eq!(failed_events.len(), 2); let mut found_expected_events = [false, false]; for event in failed_events {