Use MPPFragmentFailed event if other MPP fragments are pending upon one path's failure

valentinewallace · valentinewallace · commit 7c92daa08535 · 2021-09-16T16:38:56.000-04:00
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -485,7 +485,7 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
 	/// The session_priv bytes 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/PaymentFailed events. Specifically, in the case of a duplicative
+	/// PaymentSent/MPPFragmentFailed/PaymentFailed events. Specifically, in the case of a duplicative
 	/// update_fulfill_htlc message after a reconnect, we may "claim" a payment twice.
 	/// Additionally, because ChannelMonitors are often not re-serialized after connecting block(s)
 	/// which may generate a claim event, we may receive similar duplicate claim/fail MonitorEvents
@@ -2837,25 +2837,35 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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, .. } => {
+				HTLCSource::OutboundRoute { session_priv, mpp_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 sessions.get_mut().remove(&session_priv_bytes) {
-							self.pending_events.lock().unwrap().push(
-								events::Event::PaymentFailed {
-									payment_hash,
-									rejected_by_dest: false,
-									network_update: None,
-									#[cfg(test)]
-									error_code: None,
-									#[cfg(test)]
-									error_data: None,
+							match sessions.get().len() {
+								0 => {
+									self.pending_events.lock().unwrap().push(
+										events::Event::PaymentFailed {
+											payment_hash,
+											rejected_by_dest: false,
+											network_update: None,
+											#[cfg(test)]
+											error_code: None,
+											#[cfg(test)]
+											error_data: None,
+										}
+									);
+									sessions.remove();
+								},
+								_ => {
+									self.pending_events.lock().unwrap().push(
+										events::Event::MPPFragmentFailed {
+											payment_hash,
+											payment_path: path.clone()
+										}
+									);
 								}
-							);
-							if sessions.get().len() == 0 {
-								sessions.remove();
 							}
 						}
 					} else {
@@ -2886,12 +2896,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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();
+				let mut partial_failure = true;
 				if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(mpp_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;
 					}
 					if sessions.get().len() == 0 {
+						partial_failure = false;
 						sessions.remove();
 					}
 				} else {
@@ -2909,42 +2921,60 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						// 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!
-						self.pending_events.lock().unwrap().push(
-							events::Event::PaymentFailed {
-								payment_hash: payment_hash.clone(),
-								rejected_by_dest: !payment_retryable,
-								network_update,
-#[cfg(test)]
-								error_code: onion_error_code,
-#[cfg(test)]
-								error_data: onion_error_data
-							}
-						);
+						if partial_failure {
+							self.pending_events.lock().unwrap().push(
+								events::Event::MPPFragmentFailed {
+									payment_hash: payment_hash.clone(),
+									payment_path: path.clone()
+								}
+							);
+						} else {
+							self.pending_events.lock().unwrap().push(
+								events::Event::PaymentFailed {
+									payment_hash: payment_hash.clone(),
+									rejected_by_dest: !payment_retryable,
+									network_update,
+									#[cfg(test)]
+									error_code: onion_error_code,
+									#[cfg(test)]
+									error_data: onion_error_data
+								}
+							);
+						}
 					},
 					&HTLCFailReason::Reason {
-#[cfg(test)]
-							ref failure_code,
-#[cfg(test)]
-							ref data,
-							.. } => {
-						// we get a fail_malformed_htlc from the first hop
-						// TODO: We'd like to generate a NetworkUpdate for temporary
-						// failures here, but that would be insufficient as get_route
-						// generally ignores its view of our own channels as we provide them via
-						// ChannelDetails.
-						// TODO: For non-temporary failures, we really should be closing the
-						// channel here as we apparently can't relay through them anyway.
-						self.pending_events.lock().unwrap().push(
-							events::Event::PaymentFailed {
-								payment_hash: payment_hash.clone(),
-								rejected_by_dest: path.len() == 1,
-								network_update: None,
-#[cfg(test)]
-								error_code: Some(*failure_code),
-#[cfg(test)]
-								error_data: Some(data.clone()),
-							}
-						);
+						#[cfg(test)]
+						ref failure_code,
+						#[cfg(test)]
+						ref data,
+						.. } => {
+						if partial_failure {
+							self.pending_events.lock().unwrap().push(
+								events::Event::MPPFragmentFailed {
+									payment_hash: payment_hash.clone(),
+									payment_path: path.clone()
+								}
+							);
+						} else {
+							// we get a fail_malformed_htlc from the first hop
+							// TODO: We'd like to generate a PaymentFailureNetworkUpdate for temporary
+							// failures here, but that would be insufficient as get_route
+							// generally ignores its view of our own channels as we provide them via
+							// ChannelDetails.
+							// TODO: For non-temporary failures, we really should be closing the
+							// channel here as we apparently can't relay through them anyway.
+							self.pending_events.lock().unwrap().push(
+								events::Event::PaymentFailed {
+									payment_hash: payment_hash.clone(),
+									rejected_by_dest: path.len() == 1,
+									network_update: None,
+									#[cfg(test)]
+									error_code: Some(*failure_code),
+									#[cfg(test)]
+									error_data: Some(data.clone()),
+								}
+							);
+						}
 					}
 				}
 			},
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
@@ -1289,7 +1289,8 @@ pub fn send_payment<'a, 'b, 'c>(origin: &Node<'a, 'b, 'c>, expected_route: &[&No
 	claim_payment(&origin, expected_route, our_payment_preimage);
 }
 
-pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash)  {
+pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash)  {
+	let mut expected_paths: Vec<_> = expected_paths_slice.iter().collect();
 	for path in expected_paths.iter() {
 		assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id());
 	}
@@ -1319,8 +1320,10 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
 	for ev in events.iter() {
 		per_path_msgs.push(msgs_from_ev!(ev));
 	}
+	per_path_msgs.sort_unstable_by(|(_, node_id_a), (_, node_id_b)| node_id_a.cmp(node_id_b));
+	expected_paths.sort_unstable_by(|path_a, path_b| path_a[path_a.len() - 2].node.get_our_node_id().cmp(&path_b[path_b.len() - 2].node.get_our_node_id()));
 
-	for (expected_route, (path_msgs, next_hop)) in expected_paths.iter().zip(per_path_msgs.drain(..)) {
+	for (i, (expected_route, (path_msgs, next_hop))) in expected_paths.iter().zip(per_path_msgs.drain(..)).enumerate() {
 		let mut next_msgs = Some(path_msgs);
 		let mut expected_next_node = next_hop;
 		let mut prev_node = expected_route.last().unwrap();
@@ -1329,8 +1332,9 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
 			assert_eq!(expected_next_node, node.node.get_our_node_id());
 			if next_msgs.is_some() {
 				node.node.handle_update_fail_htlc(&prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
-				commitment_signed_dance!(node, prev_node, next_msgs.as_ref().unwrap().1, !(skip_last && idx == expected_route.len() - 1));
-				if skip_last && idx == expected_route.len() - 1 {
+				let last_node = !(skip_last && idx == expected_route.len() - 1);
+				commitment_signed_dance!(node, prev_node, next_msgs.as_ref().unwrap().1, last_node);
+				if !last_node {
 					expect_pending_htlcs_forwardable!(node);
 				}
 			}
@@ -1367,12 +1371,21 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
 			commitment_signed_dance!(origin_node, prev_node, next_msgs.as_ref().unwrap().1, false);
 			let events = origin_node.node.get_and_clear_pending_events();
 			assert_eq!(events.len(), 1);
-			match events[0] {
-				Event::PaymentFailed { payment_hash, rejected_by_dest, .. } => {
-					assert_eq!(payment_hash, our_payment_hash);
-					assert!(rejected_by_dest);
-				},
-				_ => panic!("Unexpected event"),
+			if i != expected_paths.len() - 1 {
+				match events[0] {
+					Event::MPPFragmentFailed { payment_hash, .. } => {
+						assert_eq!(payment_hash, our_payment_hash);
+					},
+					_ => panic!("Unexpected event"),
+				}
+			} else {
+				match events[0] {
+					Event::PaymentFailed { payment_hash, rejected_by_dest, .. } => {
+						assert_eq!(payment_hash, our_payment_hash);
+						assert!(rejected_by_dest);
+					},
+					_ => panic!("Unexpected event"),
+				}
 			}
 		}
 	}
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -4084,6 +4084,34 @@ fn test_no_txn_manager_serialize_deserialize() {
 	send_payment(&nodes[0], &[&nodes[1]], 1000000);
 }
 
+#[test]
+fn mpp_failure() {
+	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, 2, 3, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
+	let logger = test_utils::TestLogger::new();
+
+	let (_, 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, &[], 100000, 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;
+	send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], 200_000, payment_hash, payment_secret);
+	fail_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_hash);
+}
+
 #[test]
 fn test_dup_htlc_onchain_fails_on_reload() {
 	// When a Channel is closed, any outbound HTLCs which were relayed through it are simply
diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs
@@ -126,7 +126,8 @@ pub enum Event {
 		payment_preimage: PaymentPreimage,
 	},
 	/// Indicates an outbound payment we made failed. Probably some intermediary node dropped
-	/// something. You may wish to retry with a different route.
+	/// something. You may wish to retry with a different route. In the case of MPP payments, this
+	/// event indicates that all payment paths failed.
 	PaymentFailed {
 		/// The hash which was given to ChannelManager::send_payment.
 		payment_hash: PaymentHash,
