From 8c8bb6d246a53f1a0e4fc70788eb59947352fcb9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Dec 2022 18:51:49 +0000 Subject: [PATCH 1/9] Move `claimable_htlcs` to a struct for more fields in the same mutex --- lightning/src/ln/channelmanager.rs | 45 +++++++++++++++++------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 410386aa076..3007f736c0b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -424,6 +424,16 @@ pub(super) enum RAACommitmentOrder { RevokeAndACKFirst, } +/// Information about claimable or being-claimed payments +struct ClaimablePayments { + /// Map from payment hash to the payment data and any HTLCs which are to us and can be + /// failed/claimed by the user. + /// + /// Note that, no consistency guarantees are made about the channels given here actually + /// existing anymore by the time you go to read them! + claimable_htlcs: HashMap)>, +} + // Note this is only exposed in cfg(test): pub(super) struct ChannelHolder { pub(super) by_id: HashMap<[u8; 32], Channel>, @@ -699,7 +709,7 @@ pub type SimpleRefChannelManager<'a, 'b, 'c, 'd, 'e, M, T, F, L> = ChannelManage // | // |__`pending_inbound_payments` // | | -// | |__`claimable_htlcs` +// | |__`claimable_payments` // | | // | |__`pending_outbound_payments` // | | @@ -787,14 +797,11 @@ pub struct ChannelManager /// See `ChannelManager` struct-level documentation for lock order requirements. pending_intercepted_htlcs: Mutex>, - /// Map from payment hash to the payment data and any HTLCs which are to us and can be - /// failed/claimed by the user. - /// - /// Note that, no consistency guarantees are made about the channels given here actually - /// existing anymore by the time you go to read them! + /// The sets of payments which are claimable or currently being claimed. See + /// [`ClaimablePayments`]' individual field docs for more info. /// /// See `ChannelManager` struct-level documentation for lock order requirements. - claimable_htlcs: Mutex)>>, + claimable_payments: Mutex, /// The set of outbound SCID aliases across all our channels, including unconfirmed channels /// and some closed channels which reached a usable state prior to being closed. This is used @@ -1600,7 +1607,7 @@ impl ChannelManager ChannelManager ChannelManager { - match self.claimable_htlcs.lock().unwrap().entry(payment_hash) { + match self.claimable_payments.lock().unwrap().claimable_htlcs.entry(payment_hash) { hash_map::Entry::Vacant(e) => { let purpose = events::PaymentPurpose::SpontaneousPayment(preimage); e.insert((purpose.clone(), vec![claimable_htlc])); @@ -3851,7 +3858,7 @@ impl ChannelManager ChannelManager ChannelManager Writeable for ChannelMana } let pending_inbound_payments = self.pending_inbound_payments.lock().unwrap(); - let claimable_htlcs = self.claimable_htlcs.lock().unwrap(); + let claimable_payments = self.claimable_payments.lock().unwrap(); let pending_outbound_payments = self.pending_outbound_payments.lock().unwrap(); let mut htlc_purposes: Vec<&events::PaymentPurpose> = Vec::new(); - (claimable_htlcs.len() as u64).write(writer)?; - for (payment_hash, (purpose, previous_hops)) in claimable_htlcs.iter() { + (claimable_payments.claimable_htlcs.len() as u64).write(writer)?; + for (payment_hash, (purpose, previous_hops)) in claimable_payments.claimable_htlcs.iter() { payment_hash.write(writer)?; (previous_hops.len() as u64).write(writer)?; for htlc in previous_hops.iter() { @@ -7827,7 +7834,7 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> pending_intercepted_htlcs: Mutex::new(pending_intercepted_htlcs.unwrap()), forward_htlcs: Mutex::new(forward_htlcs), - claimable_htlcs: Mutex::new(claimable_htlcs), + claimable_payments: Mutex::new(ClaimablePayments { claimable_htlcs }), outbound_scid_aliases: Mutex::new(outbound_scid_aliases), id_to_peer: Mutex::new(id_to_peer), short_to_chan_info: FairRwLock::new(short_to_chan_info), From 27e59ef01a2e68499d3cbfee135a49f0579f64dc Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Dec 2022 18:33:52 +0000 Subject: [PATCH 2/9] Store pending claims awaiting monitor update in a separate map In the next commits we'll move to generating `PaymentClaimed` events while handling `ChannelMonitorUpdate`s rather than directly in line. Thus, as a prerequisite, here we move to storing the info required to generate the `PaymentClaimed` event in a separate map. Note that while this does introduce a new map which is written as an even value which users cannot opt out of, the map is only filled in when users use the asynchronous `ChannelMonitor` updates and after a future PR. As these are still considered beta, breaking downgrades for such users is considered acceptable in the future PR (which will likely be one LDK version later). --- lightning/src/ln/channelmanager.rs | 272 ++++++++++++++++++----------- 1 file changed, 166 insertions(+), 106 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3007f736c0b..7e6d72a8f51 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -424,6 +424,18 @@ pub(super) enum RAACommitmentOrder { RevokeAndACKFirst, } +/// Information about a payment which is currently being claimed. +struct ClaimingPayment { + amount_msat: u64, + payment_purpose: events::PaymentPurpose, + receiver_node_id: PublicKey, +} +impl_writeable_tlv_based!(ClaimingPayment, { + (0, amount_msat, required), + (2, payment_purpose, required), + (4, receiver_node_id, required), +}); + /// Information about claimable or being-claimed payments struct ClaimablePayments { /// Map from payment hash to the payment data and any HTLCs which are to us and can be @@ -431,7 +443,15 @@ struct ClaimablePayments { /// /// Note that, no consistency guarantees are made about the channels given here actually /// existing anymore by the time you go to read them! + /// + /// When adding to the map, [`Self::pending_claiming_payments`] must also be checked to ensure + /// we don't get a duplicate payment. claimable_htlcs: HashMap)>, + + /// Map from payment hash to the payment data for HTLCs which we have begun claiming, but which + /// are waiting on a [`ChannelMonitorUpdate`] to complete in order to be surfaced to the user + /// as an [`events::Event::PaymentClaimed`]. + pending_claiming_payments: HashMap, } // Note this is only exposed in cfg(test): @@ -1607,7 +1627,7 @@ impl ChannelManager ChannelManager ChannelManager { - match self.claimable_payments.lock().unwrap().claimable_htlcs.entry(payment_hash) { + let mut claimable_payments = self.claimable_payments.lock().unwrap(); + if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) { + fail_htlc!(claimable_htlc, payment_hash); + continue + } + match claimable_payments.claimable_htlcs.entry(payment_hash) { hash_map::Entry::Vacant(e) => { let purpose = events::PaymentPurpose::SpontaneousPayment(preimage); e.insert((purpose.clone(), vec![claimable_htlc])); @@ -4215,126 +4244,144 @@ impl ChannelManager chan_id.clone(), - None => { - valid_mpp = false; + let mut sources = { + let mut claimable_payments = self.claimable_payments.lock().unwrap(); + if let Some((payment_purpose, sources)) = claimable_payments.claimable_htlcs.remove(&payment_hash) { + let mut receiver_node_id = self.our_network_pubkey; + for htlc in sources.iter() { + if htlc.prev_hop.phantom_shared_secret.is_some() { + let phantom_pubkey = self.keys_manager.get_node_id(Recipient::PhantomNode) + .expect("Failed to get node_id for phantom node recipient"); + receiver_node_id = phantom_pubkey; break; } - }; + } - if let None = channel_state.by_id.get(&chan_id) { - valid_mpp = false; - break; + let dup_purpose = claimable_payments.pending_claiming_payments.insert(payment_hash, + ClaimingPayment { amount_msat: sources.iter().map(|source| source.value).sum(), + payment_purpose, receiver_node_id, + }); + if dup_purpose.is_some() { + debug_assert!(false, "Shouldn't get a duplicate pending claim event ever"); + log_error!(self.logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug", + log_bytes!(payment_hash.0)); } + sources + } else { return; } + }; + debug_assert!(!sources.is_empty()); - if expected_amt_msat.is_some() && expected_amt_msat != Some(htlc.total_msat) { - log_error!(self.logger, "Somehow ended up with an MPP payment with different total amounts - this should not be reachable!"); - debug_assert!(false); + // If we are claiming an MPP payment, we have to take special care to ensure that each + // channel exists before claiming all of the payments (inside one lock). + // Note that channel existance is sufficient as we should always get a monitor update + // which will take care of the real HTLC claim enforcement. + // + // If we find an HTLC which we would need to claim but for which we do not have a + // channel, we will fail all parts of the MPP payment. While we could wait and see if + // the sender retries the already-failed path(s), it should be a pretty rare case where + // we got all the HTLCs and then a channel closed while we were waiting for the user to + // provide the preimage, so worrying too much about the optimal handling isn't worth + // it. + let mut claimable_amt_msat = 0; + let mut expected_amt_msat = None; + let mut valid_mpp = true; + let mut errs = Vec::new(); + let mut claimed_any_htlcs = false; + let mut channel_state_lock = self.channel_state.lock().unwrap(); + let channel_state = &mut *channel_state_lock; + for htlc in sources.iter() { + let chan_id = match self.short_to_chan_info.read().unwrap().get(&htlc.prev_hop.short_channel_id) { + Some((_cp_id, chan_id)) => chan_id.clone(), + None => { valid_mpp = false; break; } - expected_amt_msat = Some(htlc.total_msat); - if let OnionPayload::Spontaneous(_) = &htlc.onion_payload { - // We don't currently support MPP for spontaneous payments, so just check - // that there's one payment here and move on. - if sources.len() != 1 { - log_error!(self.logger, "Somehow ended up with an MPP spontaneous payment - this should not be reachable!"); - debug_assert!(false); - valid_mpp = false; - break; - } - } - let phantom_shared_secret = htlc.prev_hop.phantom_shared_secret; - if phantom_shared_secret.is_some() { - let phantom_pubkey = self.keys_manager.get_node_id(Recipient::PhantomNode) - .expect("Failed to get node_id for phantom node recipient"); - receiver_node_id = Some(phantom_pubkey) - } + }; - claimable_amt_msat += htlc.value; - } - if sources.is_empty() || expected_amt_msat.is_none() { - log_info!(self.logger, "Attempted to claim an incomplete payment which no longer had any available HTLCs!"); - return; + if let None = channel_state.by_id.get(&chan_id) { + valid_mpp = false; + break; } - if claimable_amt_msat != expected_amt_msat.unwrap() { - log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.", - expected_amt_msat.unwrap(), claimable_amt_msat); - return; + + if expected_amt_msat.is_some() && expected_amt_msat != Some(htlc.total_msat) { + log_error!(self.logger, "Somehow ended up with an MPP payment with different total amounts - this should not be reachable!"); + debug_assert!(false); + valid_mpp = false; + break; } - if valid_mpp { - for htlc in sources.drain(..) { - match self.claim_funds_from_hop(&mut channel_state_lock, htlc.prev_hop, payment_preimage) { - ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => { - if let msgs::ErrorAction::IgnoreError = err.err.action { - // We got a temporary failure updating monitor, but will claim the - // HTLC when the monitor updating is restored (or on chain). - log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err); - claimed_any_htlcs = true; - } else { errs.push((pk, err)); } - }, - ClaimFundsFromHop::PrevHopForceClosed => unreachable!("We already checked for channel existence, we can't fail here!"), - ClaimFundsFromHop::DuplicateClaim => { - // While we should never get here in most cases, if we do, it likely - // indicates that the HTLC was timed out some time ago and is no longer - // available to be claimed. Thus, it does not make sense to set - // `claimed_any_htlcs`. - }, - ClaimFundsFromHop::Success(_) => claimed_any_htlcs = true, - } + expected_amt_msat = Some(htlc.total_msat); + if let OnionPayload::Spontaneous(_) = &htlc.onion_payload { + // We don't currently support MPP for spontaneous payments, so just check + // that there's one payment here and move on. + if sources.len() != 1 { + log_error!(self.logger, "Somehow ended up with an MPP spontaneous payment - this should not be reachable!"); + debug_assert!(false); + valid_mpp = false; + break; } } - mem::drop(channel_state_lock); - if !valid_mpp { - for htlc in sources.drain(..) { - let mut htlc_msat_height_data = htlc.value.to_be_bytes().to_vec(); - htlc_msat_height_data.extend_from_slice(&self.best_block.read().unwrap().height().to_be_bytes()); - let source = HTLCSource::PreviousHopData(htlc.prev_hop); - let reason = HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data); - let receiver = HTLCDestination::FailedPayment { payment_hash }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + + claimable_amt_msat += htlc.value; + } + if sources.is_empty() || expected_amt_msat.is_none() { + mem::drop(channel_state); + self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash); + log_info!(self.logger, "Attempted to claim an incomplete payment which no longer had any available HTLCs!"); + return; + } + if claimable_amt_msat != expected_amt_msat.unwrap() { + mem::drop(channel_state); + self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash); + log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.", + expected_amt_msat.unwrap(), claimable_amt_msat); + return; + } + if valid_mpp { + for htlc in sources.drain(..) { + match self.claim_funds_from_hop(&mut channel_state_lock, htlc.prev_hop, payment_preimage) { + ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => { + if let msgs::ErrorAction::IgnoreError = err.err.action { + // We got a temporary failure updating monitor, but will claim the + // HTLC when the monitor updating is restored (or on chain). + log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err); + claimed_any_htlcs = true; + } else { errs.push((pk, err)); } + }, + ClaimFundsFromHop::PrevHopForceClosed => unreachable!("We already checked for channel existence, we can't fail here!"), + ClaimFundsFromHop::DuplicateClaim => { + // While we should never get here in most cases, if we do, it likely + // indicates that the HTLC was timed out some time ago and is no longer + // available to be claimed. Thus, it does not make sense to set + // `claimed_any_htlcs`. + }, + ClaimFundsFromHop::Success(_) => claimed_any_htlcs = true, } } - - if claimed_any_htlcs { - self.pending_events.lock().unwrap().push(events::Event::PaymentClaimed { - receiver_node_id, - payment_hash, - purpose: payment_purpose, - amount_msat: claimable_amt_msat, - }); + } + mem::drop(channel_state_lock); + if !valid_mpp { + for htlc in sources.drain(..) { + let mut htlc_msat_height_data = htlc.value.to_be_bytes().to_vec(); + htlc_msat_height_data.extend_from_slice(&self.best_block.read().unwrap().height().to_be_bytes()); + let source = HTLCSource::PreviousHopData(htlc.prev_hop); + let reason = HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data); + let receiver = HTLCDestination::FailedPayment { payment_hash }; + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); } + } - // Now we can handle any errors which were generated. - for (counterparty_node_id, err) in errs.drain(..) { - let res: Result<(), _> = Err(err); - let _ = handle_error!(self, res, counterparty_node_id); - } + let ClaimingPayment { amount_msat, payment_purpose: purpose, receiver_node_id } = + self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash).unwrap(); + if claimed_any_htlcs { + self.pending_events.lock().unwrap().push(events::Event::PaymentClaimed { + payment_hash, purpose, amount_msat, receiver_node_id: Some(receiver_node_id), + }); + } + + // Now we can handle any errors which were generated. + for (counterparty_node_id, err) in errs.drain(..) { + let res: Result<(), _> = Err(err); + let _ = handle_error!(self, res, counterparty_node_id); } } @@ -7242,10 +7289,21 @@ impl Writeable for ChannelMana if our_pending_intercepts.len() != 0 { pending_intercepted_htlcs = Some(our_pending_intercepts); } + + let mut pending_claiming_payments = Some(&claimable_payments.pending_claiming_payments); + if pending_claiming_payments.as_ref().unwrap().is_empty() { + // LDK versions prior to 0.0.113 do not know how to read the pending claimed payments + // map. Thus, if there are no entries we skip writing a TLV for it. + pending_claiming_payments = None; + } else { + debug_assert!(false, "While we have code to serialize pending_claiming_payments, the map should always be empty until a later PR"); + } + write_tlv_fields!(writer, { (1, pending_outbound_payments_no_retry, required), (2, pending_intercepted_htlcs, option), (3, pending_outbound_payments, required), + (4, pending_claiming_payments, option), (5, self.our_network_pubkey, required), (7, self.fake_scid_rand_bytes, required), (9, htlc_purposes, vec_type), @@ -7572,10 +7630,12 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> let mut fake_scid_rand_bytes: Option<[u8; 32]> = None; let mut probing_cookie_secret: Option<[u8; 32]> = None; let mut claimable_htlc_purposes = None; + let mut pending_claiming_payments = Some(HashMap::new()); read_tlv_fields!(reader, { (1, pending_outbound_payments_no_retry, option), (2, pending_intercepted_htlcs, option), (3, pending_outbound_payments, option), + (4, pending_claiming_payments, option), (5, received_network_pubkey, option), (7, fake_scid_rand_bytes, option), (9, claimable_htlc_purposes, vec_type), @@ -7834,7 +7894,7 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> pending_intercepted_htlcs: Mutex::new(pending_intercepted_htlcs.unwrap()), forward_htlcs: Mutex::new(forward_htlcs), - claimable_payments: Mutex::new(ClaimablePayments { claimable_htlcs }), + claimable_payments: Mutex::new(ClaimablePayments { claimable_htlcs, pending_claiming_payments: pending_claiming_payments.unwrap() }), outbound_scid_aliases: Mutex::new(outbound_scid_aliases), id_to_peer: Mutex::new(id_to_peer), short_to_chan_info: FairRwLock::new(short_to_chan_info), From bffbd3784d5832df45f079f0058cb1e4fa2cd91d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Nov 2022 18:37:12 +0000 Subject: [PATCH 3/9] Add support for handling "actions" after a monitor update completes This adds a new enum, `MonitorUpdateCompletionAction` and a method to execute the "actions". They are intended to be done once a (potentially-async) `ChannelMonitorUpdate` persistence completes, however this behavior will be implemented in a future PR. For now, this adds the relevant infrastructure which will allow us to prepare `claim_funds` for better monitor async handling. --- lightning/src/ln/channelmanager.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7e6d72a8f51..c4a4d8b34c8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -471,6 +471,16 @@ enum BackgroundEvent { ClosingMonitorUpdate((OutPoint, ChannelMonitorUpdate)), } +pub(crate) enum MonitorUpdateCompletionAction { + /// Indicates that a payment ultimately destined for us was claimed and we should emit an + /// [`events::Event::PaymentClaimed`] to the user if we haven't yet generated such an event for + /// this payment. Note that this is only best-effort. On restart it's possible such a duplicate + /// event can be generated. + PaymentClaimed { payment_hash: PaymentHash }, + /// Indicates an [`events::Event`] should be surfaced to the user. + EmitEvent { event: events::Event }, +} + /// State we hold per-peer. In the future we should put channels in here, but for now we only hold /// the latest Init features we heard from the peer. struct PeerState { @@ -4582,6 +4592,24 @@ impl ChannelManager>(&self, actions: I) { + for action in actions.into_iter() { + match action { + MonitorUpdateCompletionAction::PaymentClaimed { payment_hash } => { + let payment = self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash); + if let Some(ClaimingPayment { amount_msat, payment_purpose: purpose, receiver_node_id }) = payment { + self.pending_events.lock().unwrap().push(events::Event::PaymentClaimed { + payment_hash, purpose, amount_msat, receiver_node_id: Some(receiver_node_id), + }); + } + }, + MonitorUpdateCompletionAction::EmitEvent { event } => { + self.pending_events.lock().unwrap().push(event); + }, + } + } + } + /// Handles a channel reentering a functional state, either due to reconnect or a monitor /// update completion. fn handle_channel_resumption(&self, pending_msg_events: &mut Vec, From 7b77a016b58face4a2d8a97edae84f2777094ee3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Dec 2022 20:46:02 +0000 Subject: [PATCH 4/9] Handle closed-chan HTLC claims in `claim_funds_from_hop` Currently `claim_funds` does all HTLC claims in one `channel_state` lock, ensuring that we always make claims from channels which are open. It can thus avoid ever having to generate a `ChannelMonitorUpdate` containing a preimage for a closed channel, which we only do in `claim_funds_internal` (for forwarded payments). In the next commit we'll change the locking of `claim_funds_from_hop` so that `claim_funds` is no longer under a single lock but takes a lock for each claim. This allows us to be more flexible with locks going forward, and ultimately isn't a huge change - if our counterparty intends to force-close a channel, us choosing to ignore it by holding the `channel_state` lock for the duration of the claim isn't going to result in a commitment update, it will just result in the preimage already being in the `ChannelMonitor`. --- lightning/src/ln/channelmanager.rs | 39 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c4a4d8b34c8..95d11ebc366 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4454,7 +4454,26 @@ impl ChannelManager) { @@ -4535,24 +4554,6 @@ impl ChannelManager None, }; if let ClaimFundsFromHop::PrevHopForceClosed = res { - let preimage_update = ChannelMonitorUpdate { - update_id: CLOSED_CHANNEL_UPDATE_ID, - updates: vec![ChannelMonitorUpdateStep::PaymentPreimage { - payment_preimage: payment_preimage.clone(), - }], - }; - // We update the ChannelMonitor on the backward link, after - // receiving an offchain preimage event from the forward link (the - // event being update_fulfill_htlc). - let update_res = self.chain_monitor.update_channel(prev_outpoint, preimage_update); - if update_res != ChannelMonitorUpdateStatus::Completed { - // TODO: This needs to be handled somehow - if we receive a monitor update - // with a preimage we *must* somehow manage to propagate it to the upstream - // channel, or we must have an ability to receive the same event and try - // again on restart. - log_error!(self.logger, "Critical error: failed to update channel monitor with preimage {:?}: {:?}", - payment_preimage, update_res); - } // Note that we do *not* set `claimed_htlc` to false here. In fact, this // totally could be a duplicate claim, but we have no way of knowing // without interrogating the `ChannelMonitor` we've provided the above From def193d6bda4c46efddd13847612ceb58d040dc1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Nov 2022 05:47:16 +0000 Subject: [PATCH 5/9] Don't hold `channel_state` lock for entire duration of claim_funds When `claim_funds` has to claim multiple HTLCs as a part of a single MPP payment, it currently does so holding the `channel_state` lock for the entire duration of the claim loop. Here we swap that for taking the lock once for each HTLC. This allows us to be more flexible with locks going forward, and ultimately isn't a huge change - if our counterparty intends to force-close a channel, us choosing to ignore it by holding the `channel_state` lock for the duration of the claim isn't going to result in a commitment update, it will just result in the preimage already being in the `ChannelMonitor`. --- lightning/src/ln/channelmanager.rs | 38 ++++++++++++++++++------------ 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 95d11ebc366..b0742ab051b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4281,10 +4281,14 @@ impl ChannelManager ChannelManager chan_id.clone(), @@ -4308,7 +4311,7 @@ impl ChannelManager ChannelManager { if let msgs::ErrorAction::IgnoreError = err.err.action { // We got a temporary failure updating monitor, but will claim the @@ -4357,7 +4361,12 @@ impl ChannelManager unreachable!("We already checked for channel existence, we can't fail here!"), + ClaimFundsFromHop::PrevHopForceClosed => { + // This should be incredibly rare - we checked that all the channels were + // open above, though as we release the lock at each loop iteration it's + // still possible. We should still claim the HTLC on-chain through the + // closed-channel-update generated in claim_funds_from_hop. + }, ClaimFundsFromHop::DuplicateClaim => { // While we should never get here in most cases, if we do, it likely // indicates that the HTLC was timed out some time ago and is no longer @@ -4368,7 +4377,7 @@ impl ChannelManager ChannelManager::Signer>>, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage) -> ClaimFundsFromHop { + fn claim_funds_from_hop(&self, mut channel_state_lock: MutexGuard::Signer>>, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage) -> ClaimFundsFromHop { //TODO: Delay the claimed_funds relaying just like we do outbound relay! let chan_id = prev_hop.outpoint.to_channel_id(); - let channel_state = &mut **channel_state_lock; + let channel_state = &mut *channel_state_lock; if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(chan_id) { match chan.get_mut().get_update_fulfill_htlc_and_commit(prev_hop.htlc_id, payment_preimage, &self.logger) { Ok(msgs_monitor_option) => { @@ -4499,7 +4508,7 @@ impl ChannelManager::Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option, from_onchain: bool, next_channel_id: [u8; 32]) { + fn claim_funds_internal(&self, channel_state_lock: MutexGuard::Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option, from_onchain: bool, next_channel_id: [u8; 32]) { match source { HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => { mem::drop(channel_state_lock); @@ -4546,7 +4555,7 @@ impl ChannelManager { let prev_outpoint = hop_data.outpoint; - let res = self.claim_funds_from_hop(&mut channel_state_lock, hop_data, payment_preimage); + let res = self.claim_funds_from_hop(channel_state_lock, hop_data, payment_preimage); let claimed_htlc = if let ClaimFundsFromHop::DuplicateClaim = res { false } else { true }; let htlc_claim_value_msat = match res { ClaimFundsFromHop::MonitorUpdateFail(_, _, amt_opt) => amt_opt, @@ -4560,7 +4569,6 @@ impl ChannelManager = Err(err); let _ = handle_error!(self, result, pk); From 1feb459811e283ab8fa1aa9c67e04a1e419cf710 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Dec 2022 21:01:50 +0000 Subject: [PATCH 6/9] Handle claim result event generation in claim_funds_from_hop Currently `claim_funds` and `claim_funds_internal` call `claim_funds_from_hop` and then surface and `Event` to the user informing them of the forwarded/claimed payment based on it's result. In both places we assume that a claim "completed" even if a monitor update is being done async. Instead, here we push that event generation through a `MonitorUpdateCompletionAction` and a call to `handle_monitor_update_completion_action`. This will allow us to hold the event(s) until async monitor updates complete in the future. --- lightning/src/ln/channelmanager.rs | 93 ++++++++++++++---------------- 1 file changed, 43 insertions(+), 50 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b0742ab051b..ecc30fd12b5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4300,7 +4300,6 @@ impl ChannelManager ChannelManager { if let msgs::ErrorAction::IgnoreError = err.err.action { // We got a temporary failure updating monitor, but will claim the // HTLC when the monitor updating is restored (or on chain). log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err); - claimed_any_htlcs = true; } else { errs.push((pk, err)); } }, ClaimFundsFromHop::PrevHopForceClosed => { @@ -4373,7 +4373,7 @@ impl ChannelManager claimed_any_htlcs = true, + ClaimFundsFromHop::Success(_) => {}, } } } @@ -4387,14 +4387,7 @@ impl ChannelManager ChannelManager::Signer>>, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage) -> ClaimFundsFromHop { + fn claim_funds_from_hop) -> Option>(&self, + mut channel_state_lock: MutexGuard::Signer>>, + prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage, completion_action: ComplFunc) + -> ClaimFundsFromHop { //TODO: Delay the claimed_funds relaying just like we do outbound relay! let chan_id = prev_hop.outpoint.to_channel_id(); let channel_state = &mut *channel_state_lock; if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(chan_id) { + let counterparty_node_id = chan.get().get_counterparty_node_id(); match chan.get_mut().get_update_fulfill_htlc_and_commit(prev_hop.htlc_id, payment_preimage, &self.logger) { Ok(msgs_monitor_option) => { if let UpdateFulfillCommitFetch::NewClaim { msgs, htlc_value_msat, monitor_update } = msgs_monitor_option { @@ -4419,10 +4416,11 @@ impl ChannelManager ChannelManager ChannelManager ChannelManager ChannelManager { let prev_outpoint = hop_data.outpoint; - let res = self.claim_funds_from_hop(channel_state_lock, hop_data, payment_preimage); - let claimed_htlc = if let ClaimFundsFromHop::DuplicateClaim = res { false } else { true }; - let htlc_claim_value_msat = match res { - ClaimFundsFromHop::MonitorUpdateFail(_, _, amt_opt) => amt_opt, - ClaimFundsFromHop::Success(amt) => Some(amt), - _ => None, - }; - if let ClaimFundsFromHop::PrevHopForceClosed = res { - // Note that we do *not* set `claimed_htlc` to false here. In fact, this - // totally could be a duplicate claim, but we have no way of knowing - // without interrogating the `ChannelMonitor` we've provided the above - // update to. Instead, we simply document in `PaymentForwarded` that this - // can happen. - } + let res = self.claim_funds_from_hop(channel_state_lock, hop_data, payment_preimage, + |htlc_claim_value_msat| { + if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat { + let fee_earned_msat = if let Some(claimed_htlc_value) = htlc_claim_value_msat { + Some(claimed_htlc_value - forwarded_htlc_value) + } else { None }; + + let prev_channel_id = Some(prev_outpoint.to_channel_id()); + let next_channel_id = Some(next_channel_id); + + Some(MonitorUpdateCompletionAction::EmitEvent { event: events::Event::PaymentForwarded { + fee_earned_msat, + claim_from_onchain_tx: from_onchain, + prev_channel_id, + next_channel_id, + }}) + } else { None } + }); if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res { let result: Result<(), _> = Err(err); let _ = handle_error!(self, result, pk); } - - if claimed_htlc { - if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat { - let fee_earned_msat = if let Some(claimed_htlc_value) = htlc_claim_value_msat { - Some(claimed_htlc_value - forwarded_htlc_value) - } else { None }; - - let mut pending_events = self.pending_events.lock().unwrap(); - let prev_channel_id = Some(prev_outpoint.to_channel_id()); - let next_channel_id = Some(next_channel_id); - - pending_events.push(events::Event::PaymentForwarded { - fee_earned_msat, - claim_from_onchain_tx: from_onchain, - prev_channel_id, - next_channel_id, - }); - } - } }, } } From b331778e34c289cb986d2426df2e12cd2ecea568 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Nov 2022 21:48:46 +0000 Subject: [PATCH 7/9] Drop now-unused `ClaimFundsFromHop` enum and replace with an `Err` --- lightning/src/ln/channelmanager.rs | 52 ++++++++---------------------- 1 file changed, 14 insertions(+), 38 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ecc30fd12b5..fa38e54ed76 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -303,14 +303,6 @@ struct ReceiveError { msg: &'static str, } -/// Return value for claim_funds_from_hop -enum ClaimFundsFromHop { - PrevHopForceClosed, - MonitorUpdateFail(PublicKey, MsgHandleErrInternal, Option), - Success(u64), - DuplicateClaim, -} - type ShutdownResult = (Option<(OutPoint, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash, PublicKey, [u8; 32])>); /// Error type returned across the channel_state mutex boundary. When an Err is generated for a @@ -4351,29 +4343,15 @@ impl ChannelManager { - if let msgs::ErrorAction::IgnoreError = err.err.action { - // We got a temporary failure updating monitor, but will claim the - // HTLC when the monitor updating is restored (or on chain). - log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err); - } else { errs.push((pk, err)); } - }, - ClaimFundsFromHop::PrevHopForceClosed => { - // This should be incredibly rare - we checked that all the channels were - // open above, though as we release the lock at each loop iteration it's - // still possible. We should still claim the HTLC on-chain through the - // closed-channel-update generated in claim_funds_from_hop. - }, - ClaimFundsFromHop::DuplicateClaim => { - // While we should never get here in most cases, if we do, it likely - // indicates that the HTLC was timed out some time ago and is no longer - // available to be claimed. Thus, it does not make sense to set - // `claimed_any_htlcs`. - }, - ClaimFundsFromHop::Success(_) => {}, + if let msgs::ErrorAction::IgnoreError = err.err.action { + // We got a temporary failure updating monitor, but will claim the + // HTLC when the monitor updating is restored (or on chain). + log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err); + } else { errs.push((pk, err)); } } } } @@ -4400,7 +4378,7 @@ impl ChannelManager) -> Option>(&self, mut channel_state_lock: MutexGuard::Signer>>, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage, completion_action: ComplFunc) - -> ClaimFundsFromHop { + -> Result<(), (PublicKey, MsgHandleErrInternal)> { //TODO: Delay the claimed_funds relaying just like we do outbound relay! let chan_id = prev_hop.outpoint.to_channel_id(); @@ -4419,9 +4397,7 @@ impl ChannelManager ChannelManager { @@ -4461,7 +4437,7 @@ impl ChannelManager ChannelManager ChannelManager = Err(err); let _ = handle_error!(self, result, pk); } From 7c48151c227819abc33a7bf77a89af5002516b39 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Dec 2022 21:13:35 +0000 Subject: [PATCH 8/9] Drop unused link in `claim_funds` --- lightning/src/ln/channelmanager.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fa38e54ed76..d20569df046 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4240,7 +4240,6 @@ impl ChannelManager Date: Tue, 6 Dec 2022 21:19:29 +0000 Subject: [PATCH 9/9] Add second TODO when claiming to mirror the existing TODO on claim fail --- lightning/src/ln/channelmanager.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d20569df046..0d09ead3aca 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4425,6 +4425,10 @@ impl ChannelManager {}, e => { + // TODO: This needs to be handled somehow - if we receive a monitor update + // with a preimage we *must* somehow manage to propagate it to the upstream + // channel, or we must have an ability to receive the same update and try + // again on restart. log_given_level!(self.logger, if e == ChannelMonitorUpdateStatus::PermanentFailure { Level::Error } else { Level::Info }, "Failed to update channel monitor with preimage {:?} immediately prior to force-close: {:?}", payment_preimage, e);