From 28eea12bbe0d78d256f79ec725cf02366dce4e36 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 23 Sep 2021 16:06:12 -0400 Subject: [PATCH 1/8] Rename MppId to PaymentId Leftover from previous PR Jeff feedback. Useful in upcoming commits as we'll expose this to users for payment retries --- lightning/src/ln/channel.rs | 4 +- lightning/src/ln/channelmanager.rs | 68 ++++++++++++++-------------- lightning/src/ln/functional_tests.rs | 6 +-- 3 files changed, 39 insertions(+), 39 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 858e307b77e..4bb549d8f6c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5509,7 +5509,7 @@ mod tests { use bitcoin::hashes::hex::FromHex; use hex; use ln::{PaymentPreimage, PaymentHash}; - use ln::channelmanager::{HTLCSource, MppId}; + use ln::channelmanager::{HTLCSource, PaymentId}; use ln::channel::{Channel,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys}; use ln::channel::MAX_FUNDING_SATOSHIS; use ln::features::InitFeatures; @@ -5683,7 +5683,7 @@ mod tests { path: Vec::new(), session_priv: SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(), first_hop_htlc_msat: 548, - mpp_id: MppId([42; 32]), + payment_id: PaymentId([42; 32]), } }); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a4684bfe6cc..eb5cf6edd6b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -172,20 +172,20 @@ struct ClaimableHTLC { onion_payload: OnionPayload, } -/// A payment identifier used to correlate an MPP payment's per-path HTLC sources internally. +/// A payment identifier used to uniquely identify a payment to LDK. #[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)] -pub(crate) struct MppId(pub [u8; 32]); +pub(crate) struct PaymentId(pub [u8; 32]); -impl Writeable for MppId { +impl Writeable for PaymentId { fn write(&self, w: &mut W) -> Result<(), io::Error> { self.0.write(w) } } -impl Readable for MppId { +impl Readable for PaymentId { fn read(r: &mut R) -> Result { let buf: [u8; 32] = Readable::read(r)?; - Ok(MppId(buf)) + Ok(PaymentId(buf)) } } /// Tracks the inbound corresponding to an outbound HTLC @@ -198,7 +198,7 @@ pub(crate) enum HTLCSource { /// Technically we can recalculate this from the route, but we cache it here to avoid /// doing a double-pass on route when we get a failure back first_hop_htlc_msat: u64, - mpp_id: MppId, + payment_id: PaymentId, }, } #[cfg(test)] @@ -208,7 +208,7 @@ impl HTLCSource { path: Vec::new(), session_priv: SecretKey::from_slice(&[1; 32]).unwrap(), first_hop_htlc_msat: 0, - mpp_id: MppId([2; 32]), + payment_id: PaymentId([2; 32]), } } } @@ -499,7 +499,7 @@ pub struct ChannelManager>>, + pending_outbound_payments: Mutex>>, our_network_key: SecretKey, our_network_pubkey: PublicKey, @@ -1878,7 +1878,7 @@ impl ChannelMana } // Only public for testing, this should otherwise never be called direcly - pub(crate) fn send_payment_along_path(&self, path: &Vec, payment_hash: &PaymentHash, payment_secret: &Option, total_value: u64, cur_height: u32, mpp_id: MppId, keysend_preimage: &Option) -> Result<(), APIError> { + pub(crate) fn send_payment_along_path(&self, path: &Vec, payment_hash: &PaymentHash, payment_secret: &Option, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option) -> Result<(), APIError> { log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id); let prng_seed = self.keys_manager.get_secure_random_bytes(); let session_priv_bytes = self.keys_manager.get_secure_random_bytes(); @@ -1894,7 +1894,7 @@ impl ChannelMana let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap(); - let sessions = pending_outbounds.entry(mpp_id).or_insert(HashSet::new()); + let sessions = pending_outbounds.entry(payment_id).or_insert(HashSet::new()); assert!(sessions.insert(session_priv_bytes)); let err: Result<(), _> = loop { @@ -1917,7 +1917,7 @@ impl ChannelMana path: path.clone(), session_priv: session_priv.clone(), first_hop_htlc_msat: htlc_msat, - mpp_id, + payment_id, }, onion_packet, &self.logger), channel_state, chan) } { Some((update_add, commitment_signed, monitor_update)) => { @@ -2017,7 +2017,7 @@ impl ChannelMana let mut total_value = 0; let our_node_id = self.get_our_node_id(); let mut path_errs = Vec::with_capacity(route.paths.len()); - let mpp_id = MppId(self.keys_manager.get_secure_random_bytes()); + let payment_id = PaymentId(self.keys_manager.get_secure_random_bytes()); 'path_check: for path in route.paths.iter() { if path.len() < 1 || path.len() > 20 { path_errs.push(Err(APIError::RouteError{err: "Path didn't go anywhere/had bogus size"})); @@ -2039,7 +2039,7 @@ impl ChannelMana let cur_height = self.best_block.read().unwrap().height() + 1; let mut results = Vec::new(); for path in route.paths.iter() { - results.push(self.send_payment_along_path(&path, &payment_hash, payment_secret, total_value, cur_height, mpp_id, &keysend_preimage)); + results.push(self.send_payment_along_path(&path, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage)); } let mut has_ok = false; let mut has_err = false; @@ -2875,11 +2875,11 @@ impl ChannelMana self.fail_htlc_backwards_internal(channel_state, htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data}); }, - HTLCSource::OutboundRoute { session_priv, mpp_id, path, .. } => { + HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => { let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(mpp_id) { + if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(payment_id) { if sessions.get_mut().remove(&session_priv_bytes) { self.pending_events.lock().unwrap().push( events::Event::PaymentPathFailed { @@ -2922,12 +2922,12 @@ impl ChannelMana // from block_connected which may run during initialization prior to the chain_monitor // being fully configured. See the docs for `ChannelManagerReadArgs` for more. match source { - HTLCSource::OutboundRoute { ref path, session_priv, mpp_id, .. } => { + HTLCSource::OutboundRoute { ref path, session_priv, payment_id, .. } => { let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let mut all_paths_failed = false; - if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(mpp_id) { + if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(payment_id) { if !sessions.get_mut().remove(&session_priv_bytes) { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); return; @@ -3181,12 +3181,12 @@ impl ChannelMana fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option, from_onchain: bool) { match source { - HTLCSource::OutboundRoute { session_priv, mpp_id, .. } => { + HTLCSource::OutboundRoute { session_priv, payment_id, .. } => { mem::drop(channel_state_lock); let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - let found_payment = if let Some(mut sessions) = outbounds.remove(&mpp_id) { + let found_payment = if let Some(mut sessions) = outbounds.remove(&payment_id) { sessions.remove(&session_priv_bytes) } else { false }; if found_payment { @@ -5079,23 +5079,23 @@ impl Readable for HTLCSource { let mut session_priv: ::util::ser::OptionDeserWrapper = ::util::ser::OptionDeserWrapper(None); let mut first_hop_htlc_msat: u64 = 0; let mut path = Some(Vec::new()); - let mut mpp_id = None; + let mut payment_id = None; read_tlv_fields!(reader, { (0, session_priv, required), - (1, mpp_id, option), + (1, payment_id, option), (2, first_hop_htlc_msat, required), (4, path, vec_type), }); - if mpp_id.is_none() { - // For backwards compat, if there was no mpp_id written, use the session_priv bytes + if payment_id.is_none() { + // For backwards compat, if there was no payment_id written, use the session_priv bytes // instead. - mpp_id = Some(MppId(*session_priv.0.unwrap().as_ref())); + payment_id = Some(PaymentId(*session_priv.0.unwrap().as_ref())); } Ok(HTLCSource::OutboundRoute { session_priv: session_priv.0.unwrap(), first_hop_htlc_msat: first_hop_htlc_msat, path: path.unwrap(), - mpp_id: mpp_id.unwrap(), + payment_id: payment_id.unwrap(), }) } 1 => Ok(HTLCSource::PreviousHopData(Readable::read(reader)?)), @@ -5107,12 +5107,12 @@ impl Readable for HTLCSource { impl Writeable for HTLCSource { fn write(&self, writer: &mut W) -> Result<(), ::io::Error> { match self { - HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, mpp_id } => { + HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, payment_id } => { 0u8.write(writer)?; - let mpp_id_opt = Some(mpp_id); + let payment_id_opt = Some(payment_id); write_tlv_fields!(writer, { (0, session_priv, required), - (1, mpp_id_opt, option), + (1, payment_id_opt, option), (2, first_hop_htlc_msat, required), (4, path, vec_type), }); @@ -5518,11 +5518,11 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> } let pending_outbound_payments_count_compat: u64 = Readable::read(reader)?; - let mut pending_outbound_payments_compat: HashMap> = + let mut pending_outbound_payments_compat: HashMap> = HashMap::with_capacity(cmp::min(pending_outbound_payments_count_compat as usize, MAX_ALLOC_SIZE/32)); for _ in 0..pending_outbound_payments_count_compat { let session_priv = Readable::read(reader)?; - if pending_outbound_payments_compat.insert(MppId(session_priv), [session_priv].iter().cloned().collect()).is_some() { + if pending_outbound_payments_compat.insert(PaymentId(session_priv), [session_priv].iter().cloned().collect()).is_some() { return Err(DecodeError::InvalidValue) }; } @@ -5596,7 +5596,7 @@ mod tests { use bitcoin::hashes::sha256::Hash as Sha256; use core::time::Duration; use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; - use ln::channelmanager::{MppId, PaymentSendFailure}; + use ln::channelmanager::{PaymentId, PaymentSendFailure}; use ln::features::{InitFeatures, InvoiceFeatures}; use ln::functional_test_utils::*; use ln::msgs; @@ -5747,11 +5747,11 @@ mod tests { let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); let (payment_preimage, our_payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[1]); - let mpp_id = MppId([42; 32]); + let payment_id = PaymentId([42; 32]); // Use the utility function send_payment_along_path to send the payment with MPP data which // indicates there are more HTLCs coming. let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match. - nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, mpp_id, &None).unwrap(); + nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -5781,7 +5781,7 @@ mod tests { expect_payment_failed!(nodes[0], our_payment_hash, true); // Send the second half of the original MPP payment. - nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, mpp_id, &None).unwrap(); + nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 03b62ac185f..5cda9869810 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -19,7 +19,7 @@ use chain::transaction::OutPoint; use chain::keysinterface::BaseSign; use ln::{PaymentPreimage, PaymentSecret, PaymentHash}; use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC}; -use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, MppId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA}; +use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA}; use ln::channel::{Channel, ChannelError}; use ln::{chan_utils, onion_utils}; use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT; @@ -3957,8 +3957,8 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) { // Use the utility function send_payment_along_path to send the payment with MPP data which // indicates there are more HTLCs coming. let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match. - let mpp_id = MppId([42; 32]); - nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, mpp_id, &None).unwrap(); + let payment_id = PaymentId([42; 32]); + nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, payment_id, &None).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); From a1fc379151063aec45a01a7b22441ba3c9593d5e Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 23 Sep 2021 16:13:30 -0400 Subject: [PATCH 2/8] Return PaymentId from send_*payment functions Used in upcoming commits for retries --- lightning/src/ln/channelmanager.rs | 14 +++++++------- lightning/src/ln/functional_tests.rs | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index eb5cf6edd6b..c231258eaa0 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -174,7 +174,7 @@ struct ClaimableHTLC { /// A payment identifier used to uniquely identify a payment to LDK. #[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)] -pub(crate) struct PaymentId(pub [u8; 32]); +pub struct PaymentId(pub [u8; 32]); impl Writeable for PaymentId { fn write(&self, w: &mut W) -> Result<(), io::Error> { @@ -1997,11 +1997,11 @@ impl ChannelMana /// If a payment_secret *is* provided, we assume that the invoice had the payment_secret feature /// bit set (either as required or as available). If multiple paths are present in the Route, /// we assume the invoice had the basic_mpp feature set. - pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option) -> Result<(), PaymentSendFailure> { + pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option) -> Result { self.send_payment_internal(route, payment_hash, payment_secret, None) } - fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option, keysend_preimage: Option) -> Result<(), PaymentSendFailure> { + fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option, keysend_preimage: Option) -> Result { if route.paths.len() < 1 { return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"})); } @@ -2059,7 +2059,7 @@ impl ChannelMana } else if has_err { Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect())) } else { - Ok(()) + Ok(payment_id) } } @@ -2077,14 +2077,14 @@ impl ChannelMana /// Note that `route` must have exactly one path. /// /// [`send_payment`]: Self::send_payment - pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option) -> Result { + pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> { let preimage = match payment_preimage { Some(p) => p, None => PaymentPreimage(self.keys_manager.get_secure_random_bytes()), }; let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner()); match self.send_payment_internal(route, payment_hash, &None, Some(preimage)) { - Ok(()) => Ok(payment_hash), + Ok(payment_id) => Ok((payment_hash, payment_id)), Err(e) => Err(e) } } @@ -5877,7 +5877,7 @@ mod tests { // To start (2), send a keysend payment but don't claim it. let payment_preimage = PaymentPreimage([42; 32]); let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph, &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); - let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap(); + let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 5cda9869810..ca45a9a608b 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -9224,7 +9224,7 @@ fn test_keysend_payments_to_public_node() { nodes[0].logger).unwrap(); let test_preimage = PaymentPreimage([42; 32]); - let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap(); + let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -9254,7 +9254,7 @@ fn test_keysend_payments_to_private_node() { nodes[0].logger).unwrap(); let test_preimage = PaymentPreimage([42; 32]); - let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap(); + let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); From 5d316302dfb394742a9044651e3f59f334d27660 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 23 Sep 2021 16:30:15 -0400 Subject: [PATCH 3/8] Refactor send_payment internals for retries We want to reuse send_payment internal functions for retries, so some need to now be parameterized by PaymentId to avoid generating a new PaymentId on retry --- lightning/src/ln/channelmanager.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c231258eaa0..32bb0a8e855 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1998,10 +1998,10 @@ impl ChannelMana /// bit set (either as required or as available). If multiple paths are present in the Route, /// we assume the invoice had the basic_mpp feature set. pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option) -> Result { - self.send_payment_internal(route, payment_hash, payment_secret, None) + self.send_payment_internal(route, payment_hash, payment_secret, None, None) } - fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option, keysend_preimage: Option) -> Result { + fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option, keysend_preimage: Option, payment_id: Option) -> Result { if route.paths.len() < 1 { return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"})); } @@ -2017,7 +2017,7 @@ impl ChannelMana let mut total_value = 0; let our_node_id = self.get_our_node_id(); let mut path_errs = Vec::with_capacity(route.paths.len()); - let payment_id = PaymentId(self.keys_manager.get_secure_random_bytes()); + let payment_id = if let Some(id) = payment_id { id } else { PaymentId(self.keys_manager.get_secure_random_bytes()) }; 'path_check: for path in route.paths.iter() { if path.len() < 1 || path.len() > 20 { path_errs.push(Err(APIError::RouteError{err: "Path didn't go anywhere/had bogus size"})); @@ -2083,7 +2083,7 @@ impl ChannelMana None => PaymentPreimage(self.keys_manager.get_secure_random_bytes()), }; let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner()); - match self.send_payment_internal(route, payment_hash, &None, Some(preimage)) { + match self.send_payment_internal(route, payment_hash, &None, Some(preimage), None) { Ok(payment_id) => Ok((payment_hash, payment_id)), Err(e) => Err(e) } @@ -5936,7 +5936,7 @@ mod tests { let test_preimage = PaymentPreimage([42; 32]); let mismatch_payment_hash = PaymentHash([43; 32]); - let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage)).unwrap(); + let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), None).unwrap(); check_added_monitors!(nodes[0], 1); let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); @@ -5973,7 +5973,7 @@ mod tests { let test_preimage = PaymentPreimage([42; 32]); let test_secret = PaymentSecret([43; 32]); let payment_hash = PaymentHash(Sha256::hash(&test_preimage.0).into_inner()); - let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage)).unwrap(); + let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), None).unwrap(); check_added_monitors!(nodes[0], 1); let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); From 72e1c91d46c63f657b5cd50ae918abafaf3a4861 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 24 Sep 2021 12:12:16 -0400 Subject: [PATCH 4/8] Refactor send_payment internals for retries 2 Retrying a partial payment means send_payment_internal needs to be parameterized by a total payment amount, else 'HTLC values do not match' errors --- lightning/src/ln/channelmanager.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 32bb0a8e855..ebfe0d2bc80 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1998,10 +1998,10 @@ impl ChannelMana /// bit set (either as required or as available). If multiple paths are present in the Route, /// we assume the invoice had the basic_mpp feature set. pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option) -> Result { - self.send_payment_internal(route, payment_hash, payment_secret, None, None) + self.send_payment_internal(route, payment_hash, payment_secret, None, None, None) } - fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option, keysend_preimage: Option, payment_id: Option) -> Result { + fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option, keysend_preimage: Option, payment_id: Option, recv_value_msat: Option) -> Result { if route.paths.len() < 1 { return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"})); } @@ -2035,6 +2035,10 @@ impl ChannelMana if path_errs.iter().any(|e| e.is_err()) { return Err(PaymentSendFailure::PathParameterError(path_errs)); } + if let Some(amt_msat) = recv_value_msat { + debug_assert!(amt_msat >= total_value); + total_value = amt_msat; + } let cur_height = self.best_block.read().unwrap().height() + 1; let mut results = Vec::new(); @@ -2083,7 +2087,7 @@ impl ChannelMana None => PaymentPreimage(self.keys_manager.get_secure_random_bytes()), }; let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner()); - match self.send_payment_internal(route, payment_hash, &None, Some(preimage), None) { + match self.send_payment_internal(route, payment_hash, &None, Some(preimage), None, None) { Ok(payment_id) => Ok((payment_hash, payment_id)), Err(e) => Err(e) } @@ -5936,7 +5940,7 @@ mod tests { let test_preimage = PaymentPreimage([42; 32]); let mismatch_payment_hash = PaymentHash([43; 32]); - let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), None).unwrap(); + let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), None, None).unwrap(); check_added_monitors!(nodes[0], 1); let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); @@ -5973,7 +5977,7 @@ mod tests { let test_preimage = PaymentPreimage([42; 32]); let test_secret = PaymentSecret([43; 32]); let payment_hash = PaymentHash(Sha256::hash(&test_preimage.0).into_inner()); - let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), None).unwrap(); + let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), None, None).unwrap(); check_added_monitors!(nodes[0], 1); let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); From bf527b0ddb501b04773caeb452cfe58757bb6e9f Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 24 Sep 2021 16:02:11 -0400 Subject: [PATCH 5/8] channelmanager: Add retry data to pending_outbound_payments --- lightning/src/ln/channelmanager.rs | 153 ++++++++++++++++++++++++----- 1 file changed, 128 insertions(+), 25 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ebfe0d2bc80..e04217a9f29 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -400,6 +400,63 @@ struct PendingInboundPayment { min_value_msat: Option, } +/// Stores the session_priv for each part of a payment that is still pending. For versions 0.0.102 +/// and later, also stores information for retrying the payment. +enum PendingOutboundPayment { + Legacy { + session_privs: HashSet<[u8; 32]>, + }, + Retryable { + session_privs: HashSet<[u8; 32]>, + payment_hash: PaymentHash, + payment_secret: Option, + pending_amt_msat: u64, + /// The total payment amount across all paths, used to verify that a retry is not overpaying. + total_msat: u64, + }, +} + +impl PendingOutboundPayment { + fn remove(&mut self, session_priv: &[u8; 32], part_amt_msat: u64) -> bool { + let remove_res = match self { + PendingOutboundPayment::Legacy { session_privs } | + PendingOutboundPayment::Retryable { session_privs, .. } => { + session_privs.remove(session_priv) + } + }; + if remove_res { + if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self { + *pending_amt_msat -= part_amt_msat; + } + } + remove_res + } + + fn insert(&mut self, session_priv: [u8; 32], part_amt_msat: u64) -> bool { + let insert_res = match self { + PendingOutboundPayment::Legacy { session_privs } | + PendingOutboundPayment::Retryable { session_privs, .. } => { + session_privs.insert(session_priv) + } + }; + if insert_res { + if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self { + *pending_amt_msat += part_amt_msat; + } + } + insert_res + } + + fn remaining_parts(&self) -> usize { + match self { + PendingOutboundPayment::Legacy { session_privs } | + PendingOutboundPayment::Retryable { session_privs, .. } => { + session_privs.len() + } + } + } +} + /// SimpleArcChannelManager is useful when you need a ChannelManager with a static lifetime, e.g. /// when you're using lightning-net-tokio (since tokio::spawn requires parameters with static /// lifetimes). Other times you can afford a reference, which is more efficient, in which case @@ -486,7 +543,7 @@ pub struct ChannelManager>, - /// The session_priv bytes of outbound payments which are pending resolution. + /// The session_priv bytes and retry metadata of outbound payments which are pending resolution. /// The authoritative state of these HTLCs resides either within Channels or ChannelMonitors /// (if the channel has been force-closed), however we track them here to prevent duplicative /// PaymentSent/PaymentPathFailed events. Specifically, in the case of a duplicative @@ -495,11 +552,10 @@ pub struct ChannelManager>>, + pending_outbound_payments: Mutex>, our_network_key: SecretKey, our_network_pubkey: PublicKey, @@ -1894,8 +1950,14 @@ impl ChannelMana let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap(); - let sessions = pending_outbounds.entry(payment_id).or_insert(HashSet::new()); - assert!(sessions.insert(session_priv_bytes)); + let payment = pending_outbounds.entry(payment_id).or_insert_with(|| PendingOutboundPayment::Retryable { + session_privs: HashSet::new(), + pending_amt_msat: 0, + payment_hash: *payment_hash, + payment_secret: *payment_secret, + total_msat: total_value, + }); + assert!(payment.insert(session_priv_bytes, path.last().unwrap().fee_msat)); let err: Result<(), _> = loop { let mut channel_lock = self.channel_state.lock().unwrap(); @@ -2883,14 +2945,14 @@ impl ChannelMana let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(payment_id) { - if sessions.get_mut().remove(&session_priv_bytes) { + if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { + if payment.get_mut().remove(&session_priv_bytes, path.last().unwrap().fee_msat) { self.pending_events.lock().unwrap().push( events::Event::PaymentPathFailed { payment_hash, rejected_by_dest: false, network_update: None, - all_paths_failed: sessions.get().len() == 0, + all_paths_failed: payment.get().remaining_parts() == 0, path: path.clone(), #[cfg(test)] error_code: None, @@ -2898,8 +2960,8 @@ impl ChannelMana error_data: None, } ); - if sessions.get().len() == 0 { - sessions.remove(); + if payment.get().remaining_parts() == 0 { + payment.remove(); } } } else { @@ -2932,11 +2994,11 @@ impl ChannelMana let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let mut all_paths_failed = false; if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(payment_id) { - if !sessions.get_mut().remove(&session_priv_bytes) { + if !sessions.get_mut().remove(&session_priv_bytes, path.last().unwrap().fee_msat) { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); return; } - if sessions.get().len() == 0 { + if sessions.get().remaining_parts() == 0 { all_paths_failed = true; sessions.remove(); } @@ -3185,13 +3247,13 @@ impl ChannelMana fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option, from_onchain: bool) { match source { - HTLCSource::OutboundRoute { session_priv, payment_id, .. } => { + HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => { mem::drop(channel_state_lock); let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let found_payment = if let Some(mut sessions) = outbounds.remove(&payment_id) { - sessions.remove(&session_priv_bytes) + sessions.remove(&session_priv_bytes, path.last().unwrap().fee_msat) } else { false }; if found_payment { self.pending_events.lock().unwrap().push( @@ -5161,6 +5223,19 @@ impl_writeable_tlv_based!(PendingInboundPayment, { (8, min_value_msat, required), }); +impl_writeable_tlv_based_enum!(PendingOutboundPayment, + (0, Legacy) => { + (0, session_privs, required), + }, + (2, Retryable) => { + (0, session_privs, required), + (2, payment_hash, required), + (4, payment_secret, option), + (6, total_msat, required), + (8, pending_amt_msat, required), + }, +;); + impl Writeable for ChannelManager where M::Target: chain::Watch, T::Target: BroadcasterInterface, @@ -5251,18 +5326,34 @@ impl Writeable f let pending_outbound_payments = self.pending_outbound_payments.lock().unwrap(); // For backwards compat, write the session privs and their total length. let mut num_pending_outbounds_compat: u64 = 0; - for (_, outbounds) in pending_outbound_payments.iter() { - num_pending_outbounds_compat += outbounds.len() as u64; + for (_, outbound) in pending_outbound_payments.iter() { + num_pending_outbounds_compat += outbound.remaining_parts() as u64; } num_pending_outbounds_compat.write(writer)?; - for (_, outbounds) in pending_outbound_payments.iter() { - for outbound in outbounds.iter() { - outbound.write(writer)?; + for (_, outbound) in pending_outbound_payments.iter() { + match outbound { + PendingOutboundPayment::Legacy { session_privs } | + PendingOutboundPayment::Retryable { session_privs, .. } => { + for session_priv in session_privs.iter() { + session_priv.write(writer)?; + } + } } } + // Encode without retry info for 0.0.101 compatibility. + let mut pending_outbound_payments_no_retry: HashMap> = HashMap::new(); + for (id, outbound) in pending_outbound_payments.iter() { + match outbound { + PendingOutboundPayment::Legacy { session_privs } | + PendingOutboundPayment::Retryable { session_privs, .. } => { + pending_outbound_payments_no_retry.insert(*id, session_privs.clone()); + } + } + } write_tlv_fields!(writer, { - (1, pending_outbound_payments, required), + (1, pending_outbound_payments_no_retry, required), + (3, pending_outbound_payments, required), }); Ok(()) @@ -5522,21 +5613,33 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> } let pending_outbound_payments_count_compat: u64 = Readable::read(reader)?; - let mut pending_outbound_payments_compat: HashMap> = + let mut pending_outbound_payments_compat: HashMap = HashMap::with_capacity(cmp::min(pending_outbound_payments_count_compat as usize, MAX_ALLOC_SIZE/32)); for _ in 0..pending_outbound_payments_count_compat { let session_priv = Readable::read(reader)?; - if pending_outbound_payments_compat.insert(PaymentId(session_priv), [session_priv].iter().cloned().collect()).is_some() { + let payment = PendingOutboundPayment::Legacy { + session_privs: [session_priv].iter().cloned().collect() + }; + if pending_outbound_payments_compat.insert(PaymentId(session_priv), payment).is_some() { return Err(DecodeError::InvalidValue) }; } + // pending_outbound_payments_no_retry is for compatibility with 0.0.101 clients. + let mut pending_outbound_payments_no_retry: Option>> = None; let mut pending_outbound_payments = None; read_tlv_fields!(reader, { - (1, pending_outbound_payments, option), + (1, pending_outbound_payments_no_retry, option), + (3, pending_outbound_payments, option), }); - if pending_outbound_payments.is_none() { + if pending_outbound_payments.is_none() && pending_outbound_payments_no_retry.is_none() { pending_outbound_payments = Some(pending_outbound_payments_compat); + } else if pending_outbound_payments.is_none() { + let mut outbounds = HashMap::new(); + for (id, session_privs) in pending_outbound_payments_no_retry.unwrap().drain() { + outbounds.insert(id, PendingOutboundPayment::Legacy { session_privs }); + } + pending_outbound_payments = Some(outbounds); } let mut secp_ctx = Secp256k1::new(); From 8db9c50b8b58d01099cef83167594cdc5f4a7842 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 24 Sep 2021 16:04:47 -0400 Subject: [PATCH 6/8] Add method to retry payments --- lightning/src/ln/channelmanager.rs | 48 +++++++++++++++ lightning/src/ln/functional_tests.rs | 90 ++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index e04217a9f29..66c785654ab 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2129,6 +2129,54 @@ impl ChannelMana } } + /// Retries a payment along the given [`Route`]. + /// + /// Errors returned are a superset of those returned from [`send_payment`], so see + /// [`send_payment`] documentation for more details on errors. This method will also error if the + /// retry amount puts the payment more than 10% over the payment's total amount, or if the payment + /// for the given `payment_id` cannot be found (likely due to timeout or success). + /// + /// [`send_payment`]: [`ChannelManager::send_payment`] + pub fn retry_payment(&self, route: &Route, payment_id: PaymentId) -> Result<(), PaymentSendFailure> { + const RETRY_OVERFLOW_PERCENTAGE: u64 = 10; + for path in route.paths.iter() { + if path.len() == 0 { + return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + err: "length-0 path in route".to_string() + })) + } + } + + let (total_msat, payment_hash, payment_secret) = { + let outbounds = self.pending_outbound_payments.lock().unwrap(); + if let Some(payment) = outbounds.get(&payment_id) { + match payment { + PendingOutboundPayment::Retryable { + total_msat, payment_hash, payment_secret, pending_amt_msat, .. + } => { + let retry_amt_msat: u64 = route.paths.iter().map(|path| path.last().unwrap().fee_msat).sum(); + if retry_amt_msat + *pending_amt_msat > *total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 { + return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + err: format!("retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat).to_string() + })) + } + (*total_msat, *payment_hash, *payment_secret) + }, + PendingOutboundPayment::Legacy { .. } => { + return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + err: "Unable to retry payments that were initially sent on LDK versions prior to 0.0.102".to_string() + })) + } + } + } else { + return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + err: "Payment with ID {} not found".to_string() + })) + } + }; + return self.send_payment_internal(route, payment_hash, &payment_secret, None, Some(payment_id), Some(total_msat)).map(|_| ()) + } + /// Send a spontaneous payment, which is a payment that does not require the recipient to have /// generated an invoice. Optionally, you may specify the preimage. If you do choose to specify /// the preimage, it must be a cryptographically secure random value that no intermediate node diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index ca45a9a608b..4ff5f1ea154 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -4183,6 +4183,96 @@ fn mpp_failure() { fail_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_hash); } +#[test] +fn mpp_retry() { + let chanmon_cfgs = create_chanmon_cfgs(4); + let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); + let nodes = create_network(4, &node_cfgs, &node_chanmgrs); + + let chan_1_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let chan_3_id = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let chan_4_id = create_announced_chan_between_nodes(&nodes, 3, 2, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let logger = test_utils::TestLogger::new(); + // Rebalance + send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); + + let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[3]); + let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; + let mut route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &[], 1_000_000, TEST_FINAL_CLTV, &logger).unwrap(); + let path = route.paths[0].clone(); + route.paths.push(path); + route.paths[0][0].pubkey = nodes[1].node.get_our_node_id(); + route.paths[0][0].short_channel_id = chan_1_id; + route.paths[0][1].short_channel_id = chan_3_id; + route.paths[1][0].pubkey = nodes[2].node.get_our_node_id(); + route.paths[1][0].short_channel_id = chan_2_id; + route.paths[1][1].short_channel_id = chan_4_id; + + // Initiate the MPP payment. + let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 2); // one monitor per path + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 2); + + // Pass half of the payment along the success path. + let success_path_msgs = events.remove(0); + pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), success_path_msgs, false, None); + + // Add the HTLC along the first hop. + let fail_path_msgs_1 = events.remove(0); + let (update_add, commitment_signed) = match fail_path_msgs_1 { + MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => { + assert_eq!(update_add_htlcs.len(), 1); + assert!(update_fail_htlcs.is_empty()); + assert!(update_fulfill_htlcs.is_empty()); + assert!(update_fail_malformed_htlcs.is_empty()); + assert!(update_fee.is_none()); + (update_add_htlcs[0].clone(), commitment_signed.clone()) + }, + _ => panic!("Unexpected event"), + }; + nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false); + + // Attempt to forward the payment and complete the 2nd path's failure. + expect_pending_htlcs_forwardable!(&nodes[2]); + expect_pending_htlcs_forwardable!(&nodes[2]); + let htlc_updates = get_htlc_update_msgs!(nodes[2], nodes[0].node.get_our_node_id()); + assert!(htlc_updates.update_add_htlcs.is_empty()); + assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); + assert!(htlc_updates.update_fulfill_htlcs.is_empty()); + assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); + check_added_monitors!(nodes[2], 1); + nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[2], htlc_updates.commitment_signed, false); + expect_payment_failed!(nodes[0], payment_hash, false); + + // Rebalance the channel so the second half of the payment can succeed. + send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); + + // Make sure it errors as expected given a too-large amount. + if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, payment_id) { + assert!(err.contains("over total_payment_amt_msat")); + } else { panic!("Unexpected error"); } + + // Make sure it errors as expected given the wrong payment_id. + if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, PaymentId([0; 32])) { + assert!(err.contains("not found")); + } else { panic!("Unexpected error"); } + + // Retry the second half of the payment and make sure it succeeds. + let mut path = route.clone(); + path.paths.remove(0); + nodes[0].node.retry_payment(&path, payment_id).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None); + claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage); +} + #[test] fn test_dup_htlc_onchain_fails_on_reload() { // When a Channel is closed, any outbound HTLCs which were relayed through it are simply From 207479f32f158fd5f4382b40bfe24554bb42ec2c Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 27 Sep 2021 20:47:32 -0400 Subject: [PATCH 7/8] Don't remove failed payments when all paths fail This is because we want the ability to retry completely failed payments. Upcoming commits will remove these payments on timeout to prevent DoS issues Also test that this removal allows retrying single-path payments --- lightning/src/ln/channelmanager.rs | 4 --- lightning/src/ln/functional_tests.rs | 53 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 66c785654ab..f8fd4d92ece 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3008,9 +3008,6 @@ impl ChannelMana error_data: None, } ); - if payment.get().remaining_parts() == 0 { - payment.remove(); - } } } else { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); @@ -3048,7 +3045,6 @@ impl ChannelMana } if sessions.get().remaining_parts() == 0 { all_paths_failed = true; - sessions.remove(); } } else { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 4ff5f1ea154..a2a105758c5 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -4273,6 +4273,59 @@ fn mpp_retry() { claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage); } +#[test] +fn retry_single_path_payment() { + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + let _chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1, InitFeatures::known(), InitFeatures::known()); + // Rebalance to find a route + send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); + + let logger = test_utils::TestLogger::new(); + let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); + let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; + let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); + + // Rebalance so that the first hop fails. + send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); + + // Make sure the payment fails on the first hop. + let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + let mut payment_event = SendEvent::from_event(events.pop().unwrap()); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); + check_added_monitors!(nodes[1], 0); + commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); + expect_pending_htlcs_forwardable!(nodes[1]); + expect_pending_htlcs_forwardable!(&nodes[1]); + let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(htlc_updates.update_add_htlcs.is_empty()); + assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); + assert!(htlc_updates.update_fulfill_htlcs.is_empty()); + assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); + check_added_monitors!(nodes[1], 1); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false); + expect_payment_failed!(nodes[0], payment_hash, false); + + // Rebalance the channel so the retry succeeds. + send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); + + // Retry the payment and make sure it succeeds. + nodes[0].node.retry_payment(&route, payment_id).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + pass_along_path(&nodes[0], &[&nodes[1], &nodes[2]], 100_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None); + claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage); +} + #[test] fn test_dup_htlc_onchain_fails_on_reload() { // When a Channel is closed, any outbound HTLCs which were relayed through it are simply From 3e6297a664ba54164e47aa6be32b33227a912f2e Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 28 Sep 2021 18:31:39 -0400 Subject: [PATCH 8/8] Expire outbound payments after 3 blocks if no parts are pending --- lightning/src/ln/channelmanager.rs | 14 +++++++ lightning/src/ln/functional_tests.rs | 55 ++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f8fd4d92ece..e165134e8e3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -413,6 +413,8 @@ enum PendingOutboundPayment { pending_amt_msat: u64, /// The total payment amount across all paths, used to verify that a retry is not overpaying. total_msat: u64, + /// Our best known block height at the time this payment was initiated. + starting_block_height: u32, }, } @@ -1955,6 +1957,7 @@ impl ChannelMana pending_amt_msat: 0, payment_hash: *payment_hash, payment_secret: *payment_secret, + starting_block_height: self.best_block.read().unwrap().height(), total_msat: total_value, }); assert!(payment.insert(session_priv_bytes, path.last().unwrap().fee_msat)); @@ -4546,6 +4549,16 @@ where payment_secrets.retain(|_, inbound_payment| { inbound_payment.expiry_time > header.time as u64 }); + + let mut outbounds = self.pending_outbound_payments.lock().unwrap(); + outbounds.retain(|_, payment| { + const PAYMENT_EXPIRY_BLOCKS: u32 = 3; + if payment.remaining_parts() != 0 { return true } + if let PendingOutboundPayment::Retryable { starting_block_height, .. } = payment { + return *starting_block_height + PAYMENT_EXPIRY_BLOCKS > height + } + true + }); } fn get_relevant_txids(&self) -> Vec { @@ -5277,6 +5290,7 @@ impl_writeable_tlv_based_enum!(PendingOutboundPayment, (4, payment_secret, option), (6, total_msat, required), (8, pending_amt_msat, required), + (10, starting_block_height, required), }, ;); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index a2a105758c5..11154405a5f 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -4317,6 +4317,9 @@ fn retry_single_path_payment() { // Rebalance the channel so the retry succeeds. send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); + // Mine two blocks (we expire retries after 3, so this will check that we don't expire early) + connect_blocks(&nodes[0], 2); + // Retry the payment and make sure it succeeds. nodes[0].node.retry_payment(&route, payment_id).unwrap(); check_added_monitors!(nodes[0], 1); @@ -4326,6 +4329,58 @@ fn retry_single_path_payment() { claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage); } +#[test] +fn retry_expired_payment() { + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + let _chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1, InitFeatures::known(), InitFeatures::known()); + // Rebalance to find a route + send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); + + let logger = test_utils::TestLogger::new(); + let (_payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); + let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; + let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); + + // Rebalance so that the first hop fails. + send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); + + // Make sure the payment fails on the first hop. + let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + let mut payment_event = SendEvent::from_event(events.pop().unwrap()); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); + check_added_monitors!(nodes[1], 0); + commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); + expect_pending_htlcs_forwardable!(nodes[1]); + expect_pending_htlcs_forwardable!(&nodes[1]); + let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(htlc_updates.update_add_htlcs.is_empty()); + assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); + assert!(htlc_updates.update_fulfill_htlcs.is_empty()); + assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); + check_added_monitors!(nodes[1], 1); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false); + expect_payment_failed!(nodes[0], payment_hash, false); + + // Mine blocks so the payment will have expired. + connect_blocks(&nodes[0], 3); + + // Retry the payment and make sure it errors as expected. + if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, payment_id) { + assert!(err.contains("not found")); + } else { + panic!("Unexpected error"); + } +} + #[test] fn test_dup_htlc_onchain_fails_on_reload() { // When a Channel is closed, any outbound HTLCs which were relayed through it are simply