Allow overshooting total_msat for an MPP

While retrying a failed path of an MPP, a node may want to overshoot
the `total_msat` in order to use a path with an `htlc_minimum_msat`
greater than the remaining value being sent. This commit no longer
fails MPPs that overshoot the `total_msat`, however it does fail
HTLCs with the same payment hash that are received *after* a
payment has become claimable.

Author: alecchendev

lightning/src/events/mod.rs

@@ -230,7 +230,7 @@ pub enum HTLCDestination {
 	///
 	/// Some of the reasons may include:
 	/// * HTLC Timeouts
-	/// * Expected MPP amount to claim does not equal HTLC total
+	/// * Expected MPP amount has already been reached
 	/// * Claimable amount does not match expected amount
 	FailedPayment {
 		/// The payment hash of the payment we attempted to process.
@@ -712,7 +712,7 @@ pub enum Event {
 	/// * Insufficient capacity in the outbound channel
 	/// * While waiting to forward the HTLC, the channel it is meant to be forwarded through closes
 	/// * When an unknown SCID is requested for forwarding a payment.
-	/// * Claiming an amount for an MPP payment that exceeds the HTLC total
+	/// * Expected MPP amount has already been reached
 	/// * The HTLC has timed out
 	///
 	/// This event, however, does not get generated if an HTLC fails to meet the forwarding

lightning/src/ln/channelmanager.rs

@@ -2663,7 +2663,7 @@ where
 	}
 
 	#[cfg(test)]
-	fn test_send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
+	pub(super) fn test_send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
 		let best_block_height = self.best_block.read().unwrap().height();
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 		self.pending_outbound_payments.test_send_payment_internal(route, payment_hash, payment_secret, keysend_preimage, payment_id, recv_value_msat, onion_session_privs, &self.node_signer, best_block_height,
@@ -3354,11 +3354,13 @@ where
 												_ => unreachable!(),
 											}
 										}
-										if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data.total_msat {
-											log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
-												log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
+										if total_value >= msgs::MAX_VALUE_MSAT {
 											fail_htlc!(claimable_htlc, payment_hash);
-										} else if total_value == $payment_data.total_msat {
+										} else if total_value - claimable_htlc.value >= $payment_data.total_msat {
+											log_trace!(self.logger, "Failing HTLC with payment_hash {} as payment is already claimable",
+												log_bytes!(payment_hash.0));
+											fail_htlc!(claimable_htlc, payment_hash);
+										} else if total_value >= $payment_data.total_msat {
 											let prev_channel_id = prev_funding_outpoint.to_channel_id();
 											htlcs.push(claimable_htlc);
 											let amount_msat = htlcs.iter().map(|htlc| htlc.value).sum();
@@ -3689,7 +3691,7 @@ where
 				if let OnionPayload::Invoice { .. } = htlcs[0].onion_payload {
 					// Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat).
 					// In this case we're not going to handle any timeouts of the parts here.
-					if htlcs[0].total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
+					if htlcs[0].total_msat <= htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
 						return true;
 					} else if htlcs.into_iter().any(|htlc| {
 						htlc.timer_ticks += 1;

lightning/src/ln/functional_tests.rs

@@ -7926,6 +7926,76 @@ fn test_can_not_accept_unknown_inbound_channel() {
 	}
 }
 
+fn do_test_overshoot_mpp(msat_amounts: &[u64], total_msat: u64) {
+
+	let routing_node_count = msat_amounts.len();
+	let node_count = routing_node_count + 2;
+
+	let chanmon_cfgs = create_chanmon_cfgs(node_count);
+	let node_cfgs = create_node_cfgs(node_count, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(node_count, &node_cfgs, &vec![None; node_count]);
+	let nodes = create_network(node_count, &node_cfgs, &node_chanmgrs);
+
+	let src_idx = 0;
+	let dst_idx = 1;
+
+	// Create channels for each amount
+	let mut expected_paths = Vec::with_capacity(routing_node_count);
+	let mut src_chan_ids = Vec::with_capacity(routing_node_count);
+	let mut dst_chan_ids = Vec::with_capacity(routing_node_count);
+	for i in 0..routing_node_count {
+		let routing_node = 2 + i;
+		let src_chan_id = create_announced_chan_between_nodes(&nodes, src_idx, routing_node).0.contents.short_channel_id;
+		src_chan_ids.push(src_chan_id);
+		let dst_chan_id = create_announced_chan_between_nodes(&nodes, routing_node, dst_idx).0.contents.short_channel_id;
+		dst_chan_ids.push(dst_chan_id);
+		let path = vec![&nodes[routing_node], &nodes[dst_idx]];
+		expected_paths.push(path);
+	}
+	let expected_paths: Vec<&[&Node]> = expected_paths.iter().map(|route| route.as_slice()).collect();
+
+	// Create a route for each amount
+	let example_amount = 100000;
+	let (mut route, our_payment_hash, our_payment_preimage, our_payment_secret) = get_route_and_payment_hash!(&nodes[src_idx], nodes[dst_idx], example_amount);
+	let sample_path = route.paths.pop().unwrap();
+	for i in 0..routing_node_count {
+		let routing_node = 2 + i;
+		let mut path = sample_path.clone();
+		path[0].pubkey = nodes[routing_node].node.get_our_node_id();
+		path[0].short_channel_id = src_chan_ids[i];
+		path[1].pubkey = nodes[dst_idx].node.get_our_node_id();
+		path[1].short_channel_id = dst_chan_ids[i];
+		path[1].fee_msat = msat_amounts[i];
+		route.paths.push(path);
+	}
+
+	// Send payment with manually set total_msat
+	let payment_id = PaymentId(nodes[src_idx].keys_manager.backing.get_secure_random_bytes());
+	let onion_session_privs = nodes[src_idx].node.test_add_new_pending_payment(our_payment_hash, Some(our_payment_secret), payment_id, &route).unwrap();
+	nodes[src_idx].node.test_send_payment_internal(&route, our_payment_hash, &Some(our_payment_secret), None, payment_id, Some(total_msat), onion_session_privs).unwrap();
+	check_added_monitors!(nodes[src_idx], expected_paths.len());
+
+	let mut events = nodes[src_idx].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), expected_paths.len());
+	let mut amount_received = 0;
+	for (path_idx, expected_path) in expected_paths.iter().enumerate() {
+		let ev = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &mut events);
+
+		let current_path_amount = msat_amounts[path_idx];
+		amount_received += current_path_amount;
+		let became_claimable_now = amount_received >= total_msat && amount_received - current_path_amount < total_msat;
+		pass_along_path(&nodes[src_idx], expected_path, amount_received, our_payment_hash.clone(), Some(our_payment_secret), ev, became_claimable_now, None);
+	}
+
+	claim_payment_along_route(&nodes[src_idx], &expected_paths, false, our_payment_preimage);
+}
+
+#[test]
+fn test_overshoot_mpp() {
+	do_test_overshoot_mpp(&[100_000, 101_000], 200_000);
+	do_test_overshoot_mpp(&[100_000, 10_000, 100_000], 200_000);
+}
+
 #[test]
 fn test_simple_mpp() {
 	// Simple test of sending a multi-path payment.

lightning/src/ln/outbound_payment.rs

@@ -916,7 +916,6 @@ impl OutboundPayments {
 			return Err(PaymentSendFailure::PathParameterError(path_errs));
 		}
 		if let Some(amt_msat) = recv_value_msat {
-			debug_assert!(amt_msat >= total_value);
 			total_value = amt_msat;
 		}