From 87da91042ed8e6c51ae61173ba375f29af8804f5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 25 Oct 2021 17:52:30 +0000 Subject: [PATCH 1/3] Make `Payee::pubkey` pub. `Payee` is expected to be used by users to get routes for payment retries, potentially with their own router. Thus, its helpful if it is pub, even if it is redundant with the last hop in the `path` field in `Events::PaymentPathFailed`. --- lightning/src/routing/router.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 4d1deb61d25..099661bb7f7 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -156,7 +156,7 @@ impl_writeable_tlv_based!(PaymentPathRetry, { #[derive(Clone, Debug)] pub struct Payee { /// The node id of the payee. - pubkey: PublicKey, + pub pubkey: PublicKey, /// Features supported by the payee. /// From fe237f9280ae78883104be64c28d6cfc6b61a4c7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 25 Oct 2021 04:42:29 +0000 Subject: [PATCH 2/3] Copy `Payee` into `Route`s to provide them to `ChannelManager` --- fuzz/src/chanmon_consistency.rs | 2 ++ lightning/src/ln/functional_tests.rs | 4 ++-- lightning/src/ln/onion_utils.rs | 1 + lightning/src/routing/router.rs | 28 ++++++++++++++++++++++------ 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 28bab5a28f8..464e784d514 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -305,6 +305,7 @@ fn send_payment(source: &ChanMan, dest: &ChanMan, dest_chan_id: u64, amt: u64, p fee_msat: amt, cltv_expiry_delta: 200, }]], + payee: None, }, payment_hash, &Some(payment_secret)) { check_payment_err(err); false @@ -330,6 +331,7 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des fee_msat: amt, cltv_expiry_delta: 200, }]], + payee: None, }, payment_hash, &Some(payment_secret)) { check_payment_err(err); false diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 61e2afef34c..fc35a030d8d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -919,7 +919,7 @@ fn fake_network_test() { }); hops[1].fee_msat = chan_4.1.contents.fee_base_msat as u64 + chan_4.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000; hops[0].fee_msat = chan_3.0.contents.fee_base_msat as u64 + chan_3.0.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000; - let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops] }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0; + let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops], payee: None }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0; let mut hops = Vec::with_capacity(3); hops.push(RouteHop { @@ -948,7 +948,7 @@ fn fake_network_test() { }); hops[1].fee_msat = chan_2.1.contents.fee_base_msat as u64 + chan_2.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000; hops[0].fee_msat = chan_3.1.contents.fee_base_msat as u64 + chan_3.1.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000; - let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops] }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1; + let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops], payee: None }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1; // Claim the rebalances... fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2); diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 20ff0c8344d..c1767c025a3 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -555,6 +555,7 @@ mod tests { short_channel_id: 0, fee_msat: 0, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually }, ]], + payee: None, }; let session_priv = SecretKey::from_slice(&hex::decode("4141414141414141414141414141414141414141414141414141414141414141").unwrap()[..]).unwrap(); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 099661bb7f7..5de71fb6cf1 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -70,6 +70,12 @@ pub struct Route { /// given path is variable, keeping the length of any path to less than 20 should currently /// ensure it is viable. pub paths: Vec>, + /// The `payee` parameter passed to [`get_route`]. + /// This is used by `ChannelManager` to track information which may be required for retries, + /// provided back to you via [`Event::PaymentPathFailed`]. + /// + /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed + pub payee: Option, } impl Route { @@ -106,7 +112,9 @@ impl Writeable for Route { hop.write(writer)?; } } - write_tlv_fields!(writer, {}); + write_tlv_fields!(writer, { + (1, self.payee, option), + }); Ok(()) } } @@ -124,8 +132,11 @@ impl Readable for Route { } paths.push(hops); } - read_tlv_fields!(reader, {}); - Ok(Route { paths }) + let mut payee = None; + read_tlv_fields!(reader, { + (1, payee, option), + }); + Ok(Route { paths, payee }) } } @@ -153,7 +164,7 @@ impl_writeable_tlv_based!(PaymentPathRetry, { }); /// The recipient of a payment. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct Payee { /// The node id of the payee. pub pubkey: PublicKey, @@ -1442,7 +1453,10 @@ where L::Target: Logger { } } - let route = Route { paths: selected_paths.into_iter().map(|path| path.into_iter().collect()).collect::, _>>()? }; + let route = Route { + paths: selected_paths.into_iter().map(|path| path.into_iter().collect()).collect::, _>>()?, + payee: Some(payee.clone()), + }; log_info!(logger, "Got route to {}: {}", payee.pubkey, log_route!(route)); Ok(route) } @@ -4600,6 +4614,7 @@ mod tests { short_channel_id: 0, fee_msat: 225, cltv_expiry_delta: 0 }, ]], + payee: None, }; assert_eq!(route.get_total_fees(), 250); @@ -4632,6 +4647,7 @@ mod tests { short_channel_id: 0, fee_msat: 150, cltv_expiry_delta: 0 }, ]], + payee: None, }; assert_eq!(route.get_total_fees(), 200); @@ -4643,7 +4659,7 @@ mod tests { // In an earlier version of `Route::get_total_fees` and `Route::get_total_amount`, they // would both panic if the route was completely empty. We test to ensure they return 0 // here, even though its somewhat nonsensical as a route. - let route = Route { paths: Vec::new() }; + let route = Route { paths: Vec::new(), payee: None }; assert_eq!(route.get_total_fees(), 0); assert_eq!(route.get_total_amount(), 0); From 1b99c93334c092f6fb7224729649316a23b46373 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 25 Oct 2021 04:46:26 +0000 Subject: [PATCH 3/3] Store `Payee` information in `HTLCSource::OutboundRoute`. This stores and tracks HTLC payee information with HTLCSource info, allowing us to provide it back to the user if the HTLC fails and ensuring persistence by keeping it with the HTLC itself as it passes between Channel and ChannelMonitor. --- lightning/src/ln/channel.rs | 1 + lightning/src/ln/channelmanager.rs | 54 ++++++++++++++++------- lightning/src/ln/functional_test_utils.rs | 10 ++++- lightning/src/ln/functional_tests.rs | 2 +- 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c2e158627ad..dffde0be791 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5768,6 +5768,7 @@ mod tests { first_hop_htlc_msat: 548, payment_id: PaymentId([42; 32]), payment_secret: None, + payee: None, } }); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 63c4bcc6b13..4eb67af2e51 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -45,7 +45,7 @@ use chain::transaction::{OutPoint, TransactionData}; use ln::{PaymentHash, PaymentPreimage, PaymentSecret}; use ln::channel::{Channel, ChannelError, ChannelUpdateStatus, UpdateFulfillCommitFetch}; use ln::features::{InitFeatures, NodeFeatures}; -use routing::router::{Route, RouteHop}; +use routing::router::{Payee, PaymentPathRetry, Route, RouteHop}; use ln::msgs; use ln::msgs::NetAddress; use ln::onion_utils; @@ -201,6 +201,7 @@ pub(crate) enum HTLCSource { first_hop_htlc_msat: u64, payment_id: PaymentId, payment_secret: Option, + payee: Option, }, } #[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash @@ -211,13 +212,14 @@ impl core::hash::Hash for HTLCSource { 0u8.hash(hasher); prev_hop_data.hash(hasher); }, - HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, first_hop_htlc_msat } => { + HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, first_hop_htlc_msat, payee } => { 1u8.hash(hasher); path.hash(hasher); session_priv[..].hash(hasher); payment_id.hash(hasher); payment_secret.hash(hasher); first_hop_htlc_msat.hash(hasher); + payee.hash(hasher); }, } } @@ -231,6 +233,7 @@ impl HTLCSource { first_hop_htlc_msat: 0, payment_id: PaymentId([2; 32]), payment_secret: None, + payee: None, } } } @@ -2021,7 +2024,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, payment_id: PaymentId, keysend_preimage: &Option) -> Result<(), APIError> { + pub(crate) fn send_payment_along_path(&self, path: &Vec, payee: &Option, 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(); @@ -2071,6 +2074,7 @@ impl ChannelMana first_hop_htlc_msat: htlc_msat, payment_id, payment_secret: payment_secret.clone(), + payee: payee.clone(), }, onion_packet, &self.logger), channel_state, chan); @@ -2209,7 +2213,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, payment_id, &keysend_preimage)); + results.push(self.send_payment_along_path(&path, &route.payee, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage)); } let mut has_ok = false; let mut has_err = false; @@ -3098,14 +3102,22 @@ 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, payment_id, path, .. } => { + HTLCSource::OutboundRoute { session_priv, payment_id, path, payee, .. } => { 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 payment) = outbounds.entry(payment_id) { - if payment.get_mut().remove(&session_priv_bytes, Some(path.last().unwrap().fee_msat)) && + let path_last_hop = path.last().expect("Outbound payments must have had a valid path"); + if payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) && !payment.get().is_fulfilled() { + let retry = if let Some(payee_data) = payee { + Some(PaymentPathRetry { + payee: payee_data, + final_value_msat: path_last_hop.fee_msat, + final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta, + }) + } else { None }; self.pending_events.lock().unwrap().push( events::Event::PaymentPathFailed { payment_hash, @@ -3114,7 +3126,7 @@ impl ChannelMana all_paths_failed: payment.get().remaining_parts() == 0, path: path.clone(), short_channel_id: None, - retry: None, + retry, #[cfg(test)] error_code: None, #[cfg(test)] @@ -3146,13 +3158,14 @@ 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, payment_id, .. } => { + HTLCSource::OutboundRoute { ref path, session_priv, payment_id, ref payee, .. } => { 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; + let path_last_hop = path.last().expect("Outbound payments must have had a valid path"); if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { - if !payment.get_mut().remove(&session_priv_bytes, Some(path.last().unwrap().fee_msat)) { + if !payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); return; } @@ -3167,8 +3180,15 @@ impl ChannelMana log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); return; } - log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0)); mem::drop(channel_state_lock); + let retry = if let Some(payee_data) = payee { + Some(PaymentPathRetry { + payee: payee_data.clone(), + final_value_msat: path_last_hop.fee_msat, + final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta, + }) + } else { None }; + log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0)); match &onion_error { &HTLCFailReason::LightningError { ref err } => { #[cfg(test)] @@ -3186,7 +3206,7 @@ impl ChannelMana all_paths_failed, path: path.clone(), short_channel_id, - retry: None, + retry, #[cfg(test)] error_code: onion_error_code, #[cfg(test)] @@ -3215,7 +3235,7 @@ impl ChannelMana all_paths_failed, path: path.clone(), short_channel_id: Some(path.first().unwrap().short_channel_id), - retry: None, + retry, #[cfg(test)] error_code: Some(*failure_code), #[cfg(test)] @@ -5378,12 +5398,14 @@ impl Readable for HTLCSource { let mut path = Some(Vec::new()); let mut payment_id = None; let mut payment_secret = None; + let mut payee = None; read_tlv_fields!(reader, { (0, session_priv, required), (1, payment_id, option), (2, first_hop_htlc_msat, required), (3, payment_secret, option), (4, path, vec_type), + (5, payee, option), }); if payment_id.is_none() { // For backwards compat, if there was no payment_id written, use the session_priv bytes @@ -5396,6 +5418,7 @@ impl Readable for HTLCSource { path: path.unwrap(), payment_id: payment_id.unwrap(), payment_secret, + payee, }) } 1 => Ok(HTLCSource::PreviousHopData(Readable::read(reader)?)), @@ -5407,7 +5430,7 @@ 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, payment_id, payment_secret } => { + HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, payment_id, payment_secret, payee } => { 0u8.write(writer)?; let payment_id_opt = Some(payment_id); write_tlv_fields!(writer, { @@ -5416,6 +5439,7 @@ impl Writeable for HTLCSource { (2, first_hop_htlc_msat, required), (3, payment_secret, option), (4, path, vec_type), + (5, payee, option), }); } HTLCSource::PreviousHopData(ref field) => { @@ -6160,7 +6184,7 @@ mod tests { // 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, payment_id, &None).unwrap(); + nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &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); @@ -6190,7 +6214,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, payment_id, &None).unwrap(); + nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &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_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index fb6c1e489f4..13fe7f73c31 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1114,9 +1114,12 @@ macro_rules! expect_payment_failed_with_update { let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data, .. } => { + Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data, ref path, ref retry, .. } => { assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash"); assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value"); + assert!(retry.is_some(), "expected retry.is_some()"); + assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path"); + assert_eq!(retry.as_ref().unwrap().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path"); assert!(error_code.is_some(), "expected error_code.is_some() = true"); assert!(error_data.is_some(), "expected error_data.is_some() = true"); match network_update { @@ -1143,9 +1146,12 @@ macro_rules! expect_payment_failed { let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data, .. } => { + Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data, ref path, ref retry, .. } => { assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash"); assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value"); + assert!(retry.is_some(), "expected retry.is_some()"); + assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path"); + assert_eq!(retry.as_ref().unwrap().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path"); assert!(error_code.is_some(), "expected error_code.is_some() = true"); assert!(error_data.is_some(), "expected error_data.is_some() = true"); $( diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index fc35a030d8d..7c8bd075fa3 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3912,7 +3912,7 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) { // 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 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(); + nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &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);