Add a test for timeout'ing HTLCs which claim to be a part of an MPP

This is a key test for our automatic HTLC time-out logic, as it
ensures we don't allow an HTLC which indicates we should wait for
additional HTLCs before responding to cause us to force-close a
channel due to HTLC near-timeout.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -1225,6 +1225,80 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 		})
 	}
 
+	// Only public for testing, this should otherwise never be called direcly
+	pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32) -> Result<(), APIError> {
+		log_trace!(self, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
+		let (session_priv, prng_seed) = self.keys_manager.get_onion_rand();
+
+		let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
+			.map_err(|_| APIError::RouteError{err: "Pubkey along hop was maliciously selected"})?;
+		let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(path, total_value, payment_secret, cur_height)?;
+		if onion_utils::route_size_insane(&onion_payloads) {
+			return Err(APIError::RouteError{err: "Route size too large considering onion data"});
+		}
+		let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash);
+
+		let _ = self.total_consistency_lock.read().unwrap();
+
+		let err: Result<(), _> = loop {
+			let mut channel_lock = self.channel_state.lock().unwrap();
+			let id = match channel_lock.short_to_id.get(&path.first().unwrap().short_channel_id) {
+				None => return Err(APIError::ChannelUnavailable{err: "No channel available with first hop!"}),
+				Some(id) => id.clone(),
+			};
+
+			let channel_state = &mut *channel_lock;
+			if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
+				match {
+					if chan.get().get_their_node_id() != path.first().unwrap().pubkey {
+						return Err(APIError::RouteError{err: "Node ID mismatch on first hop!"});
+					}
+					if !chan.get().is_live() {
+						return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!"});
+					}
+					break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
+						path: path.clone(),
+						session_priv: session_priv.clone(),
+						first_hop_htlc_msat: htlc_msat,
+					}, onion_packet), channel_state, chan)
+				} {
+					Some((update_add, commitment_signed, monitor_update)) => {
+						if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) {
+							maybe_break_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst, false, true);
+							// Note that MonitorUpdateFailed here indicates (per function docs)
+							// that we will resend the commitment update once monitor updating
+							// is restored. Therefore, we must return an error indicating that
+							// it is unsafe to retry the payment wholesale, which we do in the
+							// send_payment check for MonitorUpdateFailed, below.
+							return Err(APIError::MonitorUpdateFailed);
+						}
+
+						channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
+							node_id: path.first().unwrap().pubkey,
+							updates: msgs::CommitmentUpdate {
+								update_add_htlcs: vec![update_add],
+								update_fulfill_htlcs: Vec::new(),
+								update_fail_htlcs: Vec::new(),
+								update_fail_malformed_htlcs: Vec::new(),
+								update_fee: None,
+								commitment_signed,
+							},
+						});
+					},
+					None => {},
+				}
+			} else { unreachable!(); }
+			return Ok(());
+		};
+
+		match handle_error!(self, err, path.first().unwrap().pubkey) {
+			Ok(_) => unreachable!(),
+			Err(e) => {
+				Err(APIError::ChannelUnavailable { err: e.err })
+			},
+		}
+	}
+
 	/// Sends a payment along a given route.
 	///
 	/// Value parameters are provided via the last hop in route, see documentation for RouteHop
@@ -1297,89 +1371,8 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 
 		let cur_height = self.latest_block_height.load(Ordering::Acquire) as u32 + 1;
 		let mut results = Vec::new();
-		'path_loop: for path in route.paths.iter() {
-			macro_rules! check_res_push {
-				($res: expr) => { match $res {
-						Ok(r) => r,
-						Err(e) => {
-							results.push(Err(e));
-							continue 'path_loop;
-						},
-					}
-				}
-			}
-
-			log_trace!(self, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
-			let (session_priv, prng_seed) = self.keys_manager.get_onion_rand();
-
-			let onion_keys = check_res_push!(onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
-				.map_err(|_| APIError::RouteError{err: "Pubkey along hop was maliciously selected"}));
-			let (onion_payloads, htlc_msat, htlc_cltv) = check_res_push!(onion_utils::build_onion_payloads(&path, total_value, payment_secret, cur_height));
-			if onion_utils::route_size_insane(&onion_payloads) {
-				check_res_push!(Err(APIError::RouteError{err: "Route size too large considering onion data"}));
-			}
-			let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, &payment_hash);
-
-			let _ = self.total_consistency_lock.read().unwrap();
-
-			let err: Result<(), _> = loop {
-				let mut channel_lock = self.channel_state.lock().unwrap();
-				let id = match channel_lock.short_to_id.get(&path.first().unwrap().short_channel_id) {
-					None => check_res_push!(Err(APIError::ChannelUnavailable{err: "No channel available with first hop!"})),
-					Some(id) => id.clone(),
-				};
-
-				let channel_state = &mut *channel_lock;
-				if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
-					match {
-						if chan.get().get_their_node_id() != path.first().unwrap().pubkey {
-							check_res_push!(Err(APIError::RouteError{err: "Node ID mismatch on first hop!"}));
-						}
-						if !chan.get().is_live() {
-							check_res_push!(Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!"}));
-						}
-						break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
-							path: path.clone(),
-							session_priv: session_priv.clone(),
-							first_hop_htlc_msat: htlc_msat,
-						}, onion_packet), channel_state, chan)
-					} {
-						Some((update_add, commitment_signed, monitor_update)) => {
-							if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) {
-								maybe_break_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst, false, true);
-								// Note that MonitorUpdateFailed here indicates (per function docs)
-								// that we will resend the commitment update once monitor updating
-								// is restored. Therefore, we must return an error indicating that
-								// it is unsafe to retry the payment wholesale, which we do in the
-								// next check for MonitorUpdateFailed, below.
-								check_res_push!(Err(APIError::MonitorUpdateFailed));
-							}
-
-							channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
-								node_id: path.first().unwrap().pubkey,
-								updates: msgs::CommitmentUpdate {
-									update_add_htlcs: vec![update_add],
-									update_fulfill_htlcs: Vec::new(),
-									update_fail_htlcs: Vec::new(),
-									update_fail_malformed_htlcs: Vec::new(),
-									update_fee: None,
-									commitment_signed,
-								},
-							});
-						},
-						None => {},
-					}
-				} else { unreachable!(); }
-				results.push(Ok(()));
-				continue 'path_loop;
-			};
-
-			match handle_error!(self, err, path.first().unwrap().pubkey) {
-				Ok(_) => unreachable!(),
-				Err(e) => {
-					check_res_push!(Err(APIError::ChannelUnavailable { err: e.err }));
-				},
-			}
+		for path in route.paths.iter() {
+			results.push(self.send_payment_along_path(&path, &payment_hash, payment_secret, total_value, cur_height));
 		}
 		let mut has_ok = false;
 		let mut has_err = false;

lightning/src/ln/functional_test_utils.rs

@@ -786,49 +786,57 @@ macro_rules! expect_payment_failed {
 pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>) {
 	origin_node.node.send_payment(&route, our_payment_hash, &our_payment_secret).unwrap();
 	check_added_monitors!(origin_node, expected_paths.len());
+	pass_along_route(origin_node, expected_paths, recv_value, our_payment_hash, our_payment_secret);
+}
 
-	let mut events = origin_node.node.get_and_clear_pending_msg_events();
-	assert_eq!(events.len(), expected_paths.len());
-	for (path_idx, (ev, expected_route)) in events.drain(..).zip(expected_paths.iter()).enumerate() {
-		let mut payment_event = SendEvent::from_event(ev);
-		let mut prev_node = origin_node;
-
-		for (idx, &node) in expected_route.iter().enumerate() {
-			assert_eq!(node.node.get_our_node_id(), payment_event.node_id);
-
-			node.node.handle_update_add_htlc(&prev_node.node.get_our_node_id(), &payment_event.msgs[0]);
-			check_added_monitors!(node, 0);
-			commitment_signed_dance!(node, prev_node, payment_event.commitment_msg, false);
-
-			expect_pending_htlcs_forwardable!(node);
-
-			if idx == expected_route.len() - 1 {
-				let events_2 = node.node.get_and_clear_pending_events();
-				// Once we've gotten through all the HTLCs, the last one should result in a
-				// PaymentReceived (but each previous one should not!).
-				if path_idx == expected_paths.len() - 1 {
-					assert_eq!(events_2.len(), 1);
-					match events_2[0] {
-						Event::PaymentReceived { ref payment_hash, ref payment_secret, amt } => {
-							assert_eq!(our_payment_hash, *payment_hash);
-							assert_eq!(our_payment_secret, *payment_secret);
-							assert_eq!(amt, recv_value);
-						},
-						_ => panic!("Unexpected event"),
-					}
-				} else {
-					assert!(events_2.is_empty());
+pub fn pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_received_expected: bool) {
+	let mut payment_event = SendEvent::from_event(ev);
+	let mut prev_node = origin_node;
+
+	for (idx, &node) in expected_path.iter().enumerate() {
+		assert_eq!(node.node.get_our_node_id(), payment_event.node_id);
+
+		node.node.handle_update_add_htlc(&prev_node.node.get_our_node_id(), &payment_event.msgs[0]);
+		check_added_monitors!(node, 0);
+		commitment_signed_dance!(node, prev_node, payment_event.commitment_msg, false);
+
+		expect_pending_htlcs_forwardable!(node);
+
+		if idx == expected_path.len() - 1 {
+			let events_2 = node.node.get_and_clear_pending_events();
+			if payment_received_expected {
+				assert_eq!(events_2.len(), 1);
+				match events_2[0] {
+					Event::PaymentReceived { ref payment_hash, ref payment_secret, amt } => {
+						assert_eq!(our_payment_hash, *payment_hash);
+						assert_eq!(our_payment_secret, *payment_secret);
+						assert_eq!(amt, recv_value);
+					},
+					_ => panic!("Unexpected event"),
 				}
 			} else {
-				let mut events_2 = node.node.get_and_clear_pending_msg_events();
-				assert_eq!(events_2.len(), 1);
-				check_added_monitors!(node, 1);
-				payment_event = SendEvent::from_event(events_2.remove(0));
-				assert_eq!(payment_event.msgs.len(), 1);
+				assert!(events_2.is_empty());
 			}
-
-			prev_node = node;
+		} else {
+			let mut events_2 = node.node.get_and_clear_pending_msg_events();
+			assert_eq!(events_2.len(), 1);
+			check_added_monitors!(node, 1);
+			payment_event = SendEvent::from_event(events_2.remove(0));
+			assert_eq!(payment_event.msgs.len(), 1);
 		}
+
+		prev_node = node;
+	}
+}
+
+pub fn pass_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>) {
+	let mut events = origin_node.node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), expected_route.len());
+	for (path_idx, (ev, expected_path)) in events.drain(..).zip(expected_route.iter()).enumerate() {
+		// Once we've gotten through all the HTLCs, the last one should result in a
+		// PaymentReceived (but each previous one should not!), .
+		let expect_payment = path_idx == expected_route.len() - 1;
+		pass_along_path(origin_node, expected_path, recv_value, our_payment_hash.clone(), our_payment_secret, ev, expect_payment);
 	}
 }
 

lightning/src/ln/functional_tests.rs

@@ -3618,8 +3618,7 @@ fn test_drop_messages_peer_disconnect_dual_htlc() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-#[test]
-fn test_htlc_timeout() {
+fn do_test_htlc_timeout(send_partial_mpp: bool) {
 	// If the user fails to claim/fail an HTLC within the HTLC CLTV timeout we fail it for them
 	// to avoid our counterparty failing the channel.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -3628,7 +3627,24 @@ fn test_htlc_timeout() {
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::supported(), InitFeatures::supported());
-	let (_, our_payment_hash) = route_payment(&nodes[0], &[&nodes[1]], 100000);
+
+	let our_payment_hash = if send_partial_mpp {
+		let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 100000, TEST_FINAL_CLTV).unwrap();
+		let (_, our_payment_hash) = get_payment_preimage_hash!(&nodes[0]);
+		let payment_secret = PaymentSecret([0xdb; 32]);
+		// Use the utility function send_payment_along_path to send the payment with MPP data which
+		// indicates there are more HTLCs coming.
+		nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, CHAN_CONFIRM_DEPTH).unwrap();
+		check_added_monitors!(nodes[0], 1);
+		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		// Now do the relevant commitment_signed/RAA dances along the path, noting that the final
+		// hop should *not* yet generate any PaymentReceived event(s).
+		pass_along_path(&nodes[0], &[&nodes[1]], 100000, our_payment_hash, Some(payment_secret), events.drain(..).next().unwrap(), false);
+		our_payment_hash
+	} else {
+		route_payment(&nodes[0], &[&nodes[1]], 100000).1
+	};
 
 	let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 	nodes[0].block_notifier.block_connected_checked(&header, 101, &[], &[]);
@@ -3656,6 +3672,12 @@ fn test_htlc_timeout() {
 	expect_payment_failed!(nodes[0], our_payment_hash, true, 0x4000 | 15, &expected_failure_data[..]);
 }
 
+#[test]
+fn test_htlc_timeout() {
+	do_test_htlc_timeout(true);
+	do_test_htlc_timeout(false);
+}
+
 #[test]
 fn test_invalid_channel_announcement() {
 	//Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs