Add failing RouteHop and path to PaymentFailed

This will be useful for scoring channels and retrying multi-path
payments.

Author: jkczyz

lightning/src/ln/channelmanager.rs

@@ -2823,6 +2823,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								payment_hash,
 								rejected_by_dest: false,
 								network_update: None,
+								failing_route_hop: None,
 #[cfg(test)]
 								error_code: None,
 #[cfg(test)]
@@ -2867,9 +2868,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				match &onion_error {
 					&HTLCFailReason::LightningError { ref err } => {
 #[cfg(test)]
-						let (network_update, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+						let (network_update, failing_route_hop, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
 #[cfg(not(test))]
-						let (network_update, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+						let (network_update, failing_route_hop, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
 						// TODO: If we decided to blame ourselves (or one of our channels) in
 						// process_onion_failure we should close that channel as it implies our
 						// next-hop is needlessly blaming us!
@@ -2878,6 +2879,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								payment_hash: payment_hash.clone(),
 								rejected_by_dest: !payment_retryable,
 								network_update,
+								failing_route_hop,
 #[cfg(test)]
 								error_code: onion_error_code,
 #[cfg(test)]
@@ -2903,6 +2905,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								payment_hash: payment_hash.clone(),
 								rejected_by_dest: path.len() == 1,
 								network_update: None,
+								failing_route_hop: None,
 #[cfg(test)]
 								error_code: Some(*failure_code),
 #[cfg(test)]

lightning/src/ln/functional_test_utils.rs

@@ -1041,7 +1041,7 @@ 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::PaymentFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data } => {
+			Event::PaymentFailed { ref payment_hash, rejected_by_dest, ref network_update, failing_route_hop: _, ref error_code, ref error_data } => {
 				assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash");
 				assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value");
 				assert!(error_code.is_some(), "expected error_code.is_some() = true");
@@ -1070,7 +1070,7 @@ macro_rules! expect_payment_failed {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {
-			Event::PaymentFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data } => {
+			Event::PaymentFailed { ref payment_hash, rejected_by_dest, network_update: _, failing_route_hop: _, ref error_code, ref error_data } => {
 				assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash");
 				assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value");
 				assert!(error_code.is_some(), "expected error_code.is_some() = true");

lightning/src/ln/functional_tests.rs

@@ -5913,10 +5913,11 @@ fn test_fail_holding_cell_htlc_upon_free() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
 	match &events[0] {
-		&Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref error_code, ref error_data } => {
+		&Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref failing_route_hop, ref error_code, ref error_data } => {
 			assert_eq!(our_payment_hash.clone(), *payment_hash);
 			assert_eq!(*rejected_by_dest, false);
 			assert_eq!(*network_update, None);
+			assert_eq!(*failing_route_hop, None);
 			assert_eq!(*error_code, None);
 			assert_eq!(*error_data, None);
 		},
@@ -5999,10 +6000,11 @@ fn test_free_and_fail_holding_cell_htlcs() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
 	match &events[0] {
-		&Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref error_code, ref error_data } => {
+		&Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref failing_route_hop, ref error_code, ref error_data } => {
 			assert_eq!(payment_hash_2.clone(), *payment_hash);
 			assert_eq!(*rejected_by_dest, false);
 			assert_eq!(*network_update, None);
+			assert_eq!(*failing_route_hop, None);
 			assert_eq!(*error_code, None);
 			assert_eq!(*error_data, None);
 		},

lightning/src/ln/onion_route_tests.rs

@@ -163,7 +163,7 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
-	if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref network_update, ref error_code, error_data: _ } = &events[0] {
+	if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref network_update, failing_route_hop: _, ref error_code, error_data: _ } = &events[0] {
 		assert_eq!(*rejected_by_dest, !expected_retryable);
 		assert_eq!(*error_code, expected_error_code);
 		if expected_channel_update.is_some() {

lightning/src/ln/onion_utils.rs

@@ -11,7 +11,7 @@ use ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use ln::channelmanager::HTLCSource;
 use ln::msgs;
 use routing::network_graph::NetworkUpdate;
-use routing::router::RouteHop;
+use routing::router::{FailingRouteHop, RouteHop};
 use util::chacha20::ChaCha20;
 use util::errors::{self, APIError};
 use util::ser::{Readable, Writeable, LengthCalculatingWriter};
@@ -331,7 +331,7 @@ pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type:
 /// OutboundRoute).
 /// Returns update, a boolean indicating that the payment itself failed, and the error code.
 #[inline]
-pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec<u8>) -> (Option<NetworkUpdate>, bool, Option<u16>, Option<Vec<u8>>) where L::Target: Logger {
+pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec<u8>) -> (Option<NetworkUpdate>, Option<FailingRouteHop>, bool, Option<u16>, Option<Vec<u8>>) where L::Target: Logger {
 	if let &HTLCSource::OutboundRoute { ref path, ref session_priv, ref first_hop_htlc_msat } = htlc_source {
 		let mut res = None;
 		let mut htlc_msat = *first_hop_htlc_msat;
@@ -382,6 +382,7 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
 						|| error_code == 21; // Special case error 21 as the Route object is bogus, TODO: Maybe fail the node if the CLTV was reasonable?
 
 						let mut network_update = None;
+						let mut failing_route_hop = None;
 
 						if error_code & NODE == NODE {
 							network_update = Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: error_code & PERM == PERM });
@@ -394,6 +395,12 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
 									is_permanent: true,
 								})
 							};
+							failing_route_hop = if payment_failed { None } else {
+								Some(FailingRouteHop {
+									path: path.clone(),
+									index: if is_from_final_node { route_hop_idx } else { route_hop_idx + 1 } as u16,
+								})
+							}
 						}
 						else if error_code & UPDATE == UPDATE {
 							if let Some(update_len_slice) = err_packet.failuremsg.get(debug_field_size+2..debug_field_size+4) {
@@ -447,10 +454,17 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
 							});
 						}
 
+						if failing_route_hop.is_none() {
+							failing_route_hop = Some(FailingRouteHop {
+								path: path.clone(),
+								index: route_hop_idx as u16,
+							});
+						}
+
 						// TODO: Here (and a few other places) we assume that BADONION errors
 						// are always "sourced" from the node previous to the one which failed
 						// to decode the onion.
-						res = Some((network_update, !(error_code & PERM == PERM && is_from_final_node)));
+						res = Some((network_update, failing_route_hop, !(error_code & PERM == PERM && is_from_final_node)));
 
 						let (description, title) = errors::get_onion_error_description(error_code);
 						if debug_field_size > 0 && err_packet.failuremsg.len() >= 4 + debug_field_size {
@@ -462,20 +476,25 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
 					} else {
 						// Useless packet that we can't use but it passed HMAC, so it
 						// definitely came from the peer in question
-						res = Some((Some(NetworkUpdate::NodeFailure {
+						let network_update = Some(NetworkUpdate::NodeFailure {
 							node_id: route_hop.pubkey,
 							is_permanent: true,
-						}), !is_from_final_node));
+						});
+						let failing_route_hop = Some(FailingRouteHop {
+							path: path.clone(),
+							index: route_hop_idx as u16,
+						});
+						res = Some((network_update, failing_route_hop, !is_from_final_node));
 					}
 				}
 			}
 		}).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?");
-		if let Some((channel_update, payment_retryable)) = res {
-			(channel_update, payment_retryable, error_code_ret, error_packet_ret)
+		if let Some((channel_update, failing_route_hop, payment_retryable)) = res {
+			(channel_update, failing_route_hop, payment_retryable, error_code_ret, error_packet_ret)
 		} else {
 			// only not set either packet unparseable or hmac does not match with any
 			// payment not retryable only when garbage is from the final node
-			(None, !is_from_final_node, None, None)
+			(None, None, !is_from_final_node, None, None)
 		}
 	} else { unreachable!(); }
 }

lightning/src/routing/network_graph.rs

@@ -1731,6 +1731,7 @@ mod tests {
 				network_update: Some(NetworkUpdate::ChannelUpdateMessage {
 					msg: valid_channel_update,
 				}),
+				failing_route_hop: None,
 				error_code: None,
 				error_data: None,
 			});
@@ -1754,6 +1755,7 @@ mod tests {
 					short_channel_id,
 					is_permanent: false,
 				}),
+				failing_route_hop: None,
 				error_code: None,
 				error_data: None,
 			});
@@ -1775,6 +1777,7 @@ mod tests {
 					short_channel_id,
 					is_permanent: true,
 				}),
+				failing_route_hop: None,
 				error_code: None,
 				error_data: None,
 			});

lightning/src/routing/router.rs

@@ -28,7 +28,7 @@ use core::cmp;
 use core::ops::Deref;
 
 /// A hop in a route
-#[derive(Clone, Hash, PartialEq, Eq)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct RouteHop {
 	/// The node_id of the node at this hop.
 	pub pubkey: PublicKey,
@@ -71,6 +71,21 @@ pub struct Route {
 	pub paths: Vec<Vec<RouteHop>>,
 }
 
+/// A [`RouteHop`] responsible for a failed payment in context of the path containing it.
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct FailingRouteHop {
+	/// A path that included the failing [`RouteHop`].
+	pub path: Vec<RouteHop>,
+
+	/// The index of the failing [`RouteHop`] within the path.
+	pub index: u16,
+}
+
+impl_writeable_tlv_based!(FailingRouteHop, {
+	(0, path, vec_type),
+	(2, index, required),
+});
+
 const SERIALIZATION_VERSION: u8 = 1;
 const MIN_SERIALIZATION_VERSION: u8 = 1;
 

lightning/src/util/events.rs

@@ -18,6 +18,7 @@ use chain::keysinterface::SpendableOutputDescriptor;
 use ln::msgs;
 use ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 use routing::network_graph::NetworkUpdate;
+use routing::router::FailingRouteHop;
 use util::ser::{Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper};
 
 use bitcoin::blockdata::script::Script;
@@ -138,6 +139,10 @@ pub enum Event {
 		/// [`NetworkGraph`]: crate::routing::network_graph::NetworkGraph
 		/// [`NetGraphMsgHandler`]: crate::routing::network_graph::NetGraphMsgHandler
 		network_update: Option<NetworkUpdate>,
+		/// The [`RouteHop`] responsible for the payment failure.
+		///
+		/// [[`RouteHop`]: crate::routing::router::FailingRouteHop
+		failing_route_hop: Option<FailingRouteHop>,
 #[cfg(test)]
 		error_code: Option<u16>,
 #[cfg(test)]
@@ -221,7 +226,7 @@ impl Writeable for Event {
 					(0, payment_preimage, required),
 				});
 			},
-			&Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update,
+			&Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref failing_route_hop,
 				#[cfg(test)]
 				ref error_code,
 				#[cfg(test)]
@@ -236,6 +241,7 @@ impl Writeable for Event {
 					(0, payment_hash, required),
 					(1, network_update, option),
 					(2, rejected_by_dest, required),
+					(3, failing_route_hop, option),
 				});
 			},
 			&Event::PendingHTLCsForwardable { time_forwardable: _ } => {
@@ -319,15 +325,18 @@ impl MaybeReadable for Event {
 					let mut payment_hash = PaymentHash([0; 32]);
 					let mut rejected_by_dest = false;
 					let mut network_update = None;
+					let mut failing_route_hop = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
 						(1, network_update, ignorable),
 						(2, rejected_by_dest, required),
+						(3, failing_route_hop, ignorable),
 					});
 					Ok(Some(Event::PaymentFailed {
 						payment_hash,
 						rejected_by_dest,
 						network_update,
+						failing_route_hop,
 						#[cfg(test)]
 						error_code,
 						#[cfg(test)]