Merge pull request #1704 from TheBlueMatt/2022-09-always-probe-failed

Dont use PaymentPathFailed a probe fails without making it out

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -3812,62 +3812,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		counterparty_node_id: &PublicKey
 	) {
 		for (htlc_src, payment_hash) in htlcs_to_fail.drain(..) {
-			match htlc_src {
-				HTLCSource::PreviousHopData(HTLCPreviousHopData { .. }) => {
-					let (failure_code, onion_failure_data) =
-						match self.channel_state.lock().unwrap().by_id.entry(channel_id) {
-							hash_map::Entry::Occupied(chan_entry) => {
-								self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan_entry.get())
-							},
-							hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new())
-						};
-					let channel_state = self.channel_state.lock().unwrap();
+			let mut channel_state = self.channel_state.lock().unwrap();
+			let (failure_code, onion_failure_data) =
+				match channel_state.by_id.entry(channel_id) {
+					hash_map::Entry::Occupied(chan_entry) => {
+						self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan_entry.get())
+					},
+					hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new())
+				};
 
-					let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id };
-					self.fail_htlc_backwards_internal(channel_state, htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data }, receiver)
-				},
-				HTLCSource::OutboundRoute { session_priv, payment_id, path, payment_params, .. } => {
-					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)) && !payment.get().is_fulfilled() {
-							let retry = if let Some(payment_params_data) = payment_params {
-								let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
-								Some(RouteParameters {
-									payment_params: payment_params_data,
-									final_value_msat: path_last_hop.fee_msat,
-									final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta,
-								})
-							} else { None };
-							let mut pending_events = self.pending_events.lock().unwrap();
-							pending_events.push(events::Event::PaymentPathFailed {
-								payment_id: Some(payment_id),
-								payment_hash,
-								payment_failed_permanently: false,
-								network_update: None,
-								all_paths_failed: payment.get().remaining_parts() == 0,
-								path: path.clone(),
-								short_channel_id: None,
-								retry,
-								#[cfg(test)]
-								error_code: None,
-								#[cfg(test)]
-								error_data: None,
-							});
-							if payment.get().abandoned() && payment.get().remaining_parts() == 0 {
-								pending_events.push(events::Event::PaymentFailed {
-									payment_id,
-									payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"),
-								});
-								payment.remove();
-							}
-						}
-					} else {
-						log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
-					}
-				},
-			};
+			let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id };
+			self.fail_htlc_backwards_internal(channel_state, htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data }, receiver);
 		}
 	}
 
@@ -3987,19 +3942,29 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						// channel here as we apparently can't relay through them anyway.
 						let scid = path.first().unwrap().short_channel_id;
 						retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
-						events::Event::PaymentPathFailed {
-							payment_id: Some(payment_id),
-							payment_hash: payment_hash.clone(),
-							payment_failed_permanently: false,
-							network_update: None,
-							all_paths_failed,
-							path: path.clone(),
-							short_channel_id: Some(scid),
-							retry,
+
+						if self.payment_is_probe(payment_hash, &payment_id) {
+							events::Event::ProbeFailed {
+								payment_id: payment_id,
+								payment_hash: payment_hash.clone(),
+								path: path.clone(),
+								short_channel_id: Some(scid),
+							}
+						} else {
+							events::Event::PaymentPathFailed {
+								payment_id: Some(payment_id),
+								payment_hash: payment_hash.clone(),
+								payment_failed_permanently: false,
+								network_update: None,
+								all_paths_failed,
+								path: path.clone(),
+								short_channel_id: Some(scid),
+								retry,
 #[cfg(test)]
-							error_code: Some(*failure_code),
+								error_code: Some(*failure_code),
 #[cfg(test)]
-							error_data: Some(data.clone()),
+								error_data: Some(data.clone()),
+							}
 						}
 					}
 				};

lightning/src/ln/functional_test_utils.rs

@@ -304,9 +304,18 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 	fn drop(&mut self) {
 		if !panicking() {
 			// Check that we processed all pending events
-			assert!(self.node.get_and_clear_pending_msg_events().is_empty());
-			assert!(self.node.get_and_clear_pending_events().is_empty());
-			assert!(self.chain_monitor.added_monitors.lock().unwrap().is_empty());
+			let msg_events = self.node.get_and_clear_pending_msg_events();
+			if !msg_events.is_empty() {
+				panic!("Had excess message events on node {}: {:?}", self.logger.id, msg_events);
+			}
+			let events = self.node.get_and_clear_pending_events();
+			if !events.is_empty() {
+				panic!("Had excess events on node {}: {:?}", self.logger.id, events);
+			}
+			let added_monitors = self.chain_monitor.added_monitors.lock().unwrap().split_off(0);
+			if !added_monitors.is_empty() {
+				panic!("Had {} excess added monitors on node {}", added_monitors.len(), self.logger.id);
+			}
 
 			// Check that if we serialize the Router, we can deserialize it again.
 			{

lightning/src/ln/functional_tests.rs

@@ -6264,15 +6264,13 @@ 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::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => {
 			assert_eq!(our_payment_id, *payment_id.as_ref().unwrap());
 			assert_eq!(our_payment_hash.clone(), *payment_hash);
 			assert_eq!(*payment_failed_permanently, false);
 			assert_eq!(*all_paths_failed, true);
 			assert_eq!(*network_update, None);
-			assert_eq!(*short_channel_id, None);
-			assert_eq!(*error_code, None);
-			assert_eq!(*error_data, None);
+			assert_eq!(*short_channel_id, Some(route.paths[0][0].short_channel_id));
 		},
 		_ => panic!("Unexpected event"),
 	}
@@ -6350,15 +6348,13 @@ 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::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => {
 			assert_eq!(payment_id_2, *payment_id.as_ref().unwrap());
 			assert_eq!(payment_hash_2.clone(), *payment_hash);
 			assert_eq!(*payment_failed_permanently, false);
 			assert_eq!(*all_paths_failed, true);
 			assert_eq!(*network_update, None);
-			assert_eq!(*short_channel_id, None);
-			assert_eq!(*error_code, None);
-			assert_eq!(*error_data, None);
+			assert_eq!(*short_channel_id, Some(route_2.paths[0][0].short_channel_id));
 		},
 		_ => panic!("Unexpected event"),
 	}

lightning/src/ln/payment_tests.rs

@@ -1200,3 +1200,57 @@ fn failed_probe_yields_event() {
 		_ => panic!(),
 	};
 }
+
+#[test]
+fn onchain_failed_probe_yields_event() {
+	// Tests that an attempt to probe over a channel that is eventaully closed results in a failure
+	// event.
+	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 nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
+	create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
+
+	let payment_params = PaymentParameters::from_node_id(nodes[2].node.get_our_node_id());
+
+	// Send a dust HTLC, which will be treated as if it timed out once the channel hits the chain.
+	let (route, _, _, _) = get_route_and_payment_hash!(&nodes[0], nodes[2], &payment_params, 1_000, 42);
+	let (payment_hash, payment_id) = nodes[0].node.send_probe(route.paths[0].clone()).unwrap();
+
+	// node[0] -- update_add_htlcs -> node[1]
+	check_added_monitors!(nodes[0], 1);
+	let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+	let probe_event = SendEvent::from_commitment_update(nodes[1].node.get_our_node_id(), updates);
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &probe_event.msgs[0]);
+	check_added_monitors!(nodes[1], 0);
+	commitment_signed_dance!(nodes[1], nodes[0], probe_event.commitment_msg, false);
+	expect_pending_htlcs_forwardable!(nodes[1]);
+
+	check_added_monitors!(nodes[1], 1);
+	let _ = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
+
+	// Don't bother forwarding the HTLC onwards and just confirm the force-close transaction on
+	// Node A, which after 6 confirmations should result in a probe failure event.
+	let bs_txn = get_local_commitment_txn!(nodes[1], chan_id);
+	confirm_transaction(&nodes[0], &bs_txn[0]);
+	check_closed_broadcast!(&nodes[0], true);
+	check_added_monitors!(nodes[0], 1);
+
+	let mut events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 2);
+	let mut found_probe_failed = false;
+	for event in events.drain(..) {
+		match event {
+			Event::ProbeFailed { payment_id: ev_pid, payment_hash: ev_ph, .. } => {
+				assert_eq!(payment_id, ev_pid);
+				assert_eq!(payment_hash, ev_ph);
+				found_probe_failed = true;
+			},
+			Event::ChannelClosed { .. } => {},
+			_ => panic!(),
+		}
+	}
+	assert!(found_probe_failed);
+}

lightning/src/util/test_utils.rs

@@ -517,10 +517,7 @@ impl events::MessageSendEventsProvider for TestRoutingMessageHandler {
 
 pub struct TestLogger {
 	level: Level,
-	#[cfg(feature = "std")]
-	id: String,
-	#[cfg(not(feature = "std"))]
-	_id: String,
+	pub(crate) id: String,
 	pub lines: Mutex<HashMap<(String, String), usize>>,
 }
 
@@ -531,10 +528,7 @@ impl TestLogger {
 	pub fn with_id(id: String) -> TestLogger {
 		TestLogger {
 			level: Level::Trace,
-			#[cfg(feature = "std")]
 			id,
-			#[cfg(not(feature = "std"))]
-			_id: id,
 			lines: Mutex::new(HashMap::new())
 		}
 	}
@@ -558,10 +552,10 @@ impl TestLogger {
 		assert_eq!(l, count)
 	}
 
-    /// Search for the number of occurrences of logged lines which
-    /// 1. belong to the specified module and
-    /// 2. match the given regex pattern.
-    /// Assert that the number of occurrences equals the given `count`
+	/// Search for the number of occurrences of logged lines which
+	/// 1. belong to the specified module and
+	/// 2. match the given regex pattern.
+	/// Assert that the number of occurrences equals the given `count`
 	pub fn assert_log_regex(&self, module: String, pattern: regex::Regex, count: usize) {
 		let log_entries = self.lines.lock().unwrap();
 		let l: usize = log_entries.iter().filter(|&(&(ref m, ref l), _c)| {