Merge pull request #1053 from valentinewallace/2021-08-dedup-payment-sent

Deduplicate PaymentSent events for MPP payments

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -5509,7 +5509,7 @@ mod tests {
 	use bitcoin::hashes::hex::FromHex;
 	use hex;
 	use ln::{PaymentPreimage, PaymentHash};
-	use ln::channelmanager::HTLCSource;
+	use ln::channelmanager::{HTLCSource, MppId};
 	use ln::channel::{Channel,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys};
 	use ln::channel::MAX_FUNDING_SATOSHIS;
 	use ln::features::InitFeatures;
@@ -5683,6 +5683,7 @@ mod tests {
 				path: Vec::new(),
 				session_priv: SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
 				first_hop_htlc_msat: 548,
+				mpp_id: MppId([42; 32]),
 			}
 		});
 

lightning/src/ln/channelmanager.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, 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: _, 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");
@@ -1242,9 +1242,11 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
 
 		if !skip_last {
 			last_update_fulfill_dance!(origin_node, expected_route.first().unwrap());
-			expect_payment_sent!(origin_node, our_payment_preimage);
 		}
 	}
+	if !skip_last {
+		expect_payment_sent!(origin_node, our_payment_preimage);
+	}
 }
 
 pub fn claim_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], our_payment_preimage: PaymentPreimage) {
@@ -1287,77 +1289,97 @@ 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_route: &[&Node<'a, 'b, 'c>], skip_last: bool, our_payment_hash: PaymentHash)  {
-	assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash));
-	expect_pending_htlcs_forwardable!(expected_route.last().unwrap());
-	check_added_monitors!(expected_route.last().unwrap(), 1);
+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());
+	}
+	assert!(expected_paths[0].last().unwrap().node.fail_htlc_backwards(&our_payment_hash));
+	expect_pending_htlcs_forwardable!(expected_paths[0].last().unwrap());
+	check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());
 
-	let mut next_msgs: Option<(msgs::UpdateFailHTLC, msgs::CommitmentSigned)> = None;
-	macro_rules! update_fail_dance {
-		($node: expr, $prev_node: expr, $last_node: expr) => {
-			{
-				$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, !$last_node);
-				if skip_last && $last_node {
-					expect_pending_htlcs_forwardable!($node);
+	let mut per_path_msgs: Vec<((msgs::UpdateFailHTLC, msgs::CommitmentSigned), PublicKey)> = Vec::with_capacity(expected_paths.len());
+	let events = expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), expected_paths.len());
+	for ev in events.iter() {
+		let (update_fail, commitment_signed, node_id) = match ev {
+			&MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {
+				assert!(update_add_htlcs.is_empty());
+				assert!(update_fulfill_htlcs.is_empty());
+				assert_eq!(update_fail_htlcs.len(), 1);
+				assert!(update_fail_malformed_htlcs.is_empty());
+				assert!(update_fee.is_none());
+				(update_fail_htlcs[0].clone(), commitment_signed.clone(), node_id.clone())
+			},
+			_ => panic!("Unexpected event"),
+		};
+		per_path_msgs.push(((update_fail, commitment_signed), node_id));
+	}
+	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 (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();
+
+		for (idx, node) in expected_route.iter().rev().enumerate().skip(1) {
+			assert_eq!(expected_next_node, node.node.get_our_node_id());
+			let update_next_node = !skip_last || idx != expected_route.len() - 1;
+			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, update_next_node);
+				if !update_next_node {
+					expect_pending_htlcs_forwardable!(node);
 				}
 			}
-		}
-	}
+			let events = node.node.get_and_clear_pending_msg_events();
+			if update_next_node {
+				assert_eq!(events.len(), 1);
+				match events[0] {
+					MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {
+						assert!(update_add_htlcs.is_empty());
+						assert!(update_fulfill_htlcs.is_empty());
+						assert_eq!(update_fail_htlcs.len(), 1);
+						assert!(update_fail_malformed_htlcs.is_empty());
+						assert!(update_fee.is_none());
+						expected_next_node = node_id.clone();
+						next_msgs = Some((update_fail_htlcs[0].clone(), commitment_signed.clone()));
+					},
+					_ => panic!("Unexpected event"),
+				}
+			} else {
+				assert!(events.is_empty());
+			}
+			if !skip_last && idx == expected_route.len() - 1 {
+				assert_eq!(expected_next_node, origin_node.node.get_our_node_id());
+			}
 
-	let mut expected_next_node = expected_route.last().unwrap().node.get_our_node_id();
-	let mut prev_node = expected_route.last().unwrap();
-	for (idx, node) in expected_route.iter().rev().enumerate() {
-		assert_eq!(expected_next_node, node.node.get_our_node_id());
-		if next_msgs.is_some() {
-			// We may be the "last node" for the purpose of the commitment dance if we're
-			// skipping the last node (implying it is disconnected) and we're the
-			// second-to-last node!
-			update_fail_dance!(node, prev_node, skip_last && idx == expected_route.len() - 1);
+			prev_node = node;
 		}
 
-		let events = node.node.get_and_clear_pending_msg_events();
-		if !skip_last || idx != expected_route.len() - 1 {
+		if !skip_last {
+			let prev_node = expected_route.first().unwrap();
+			origin_node.node.handle_update_fail_htlc(&prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
+			check_added_monitors!(origin_node, 0);
+			assert!(origin_node.node.get_and_clear_pending_msg_events().is_empty());
+			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] {
-				MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {
-					assert!(update_add_htlcs.is_empty());
-					assert!(update_fulfill_htlcs.is_empty());
-					assert_eq!(update_fail_htlcs.len(), 1);
-					assert!(update_fail_malformed_htlcs.is_empty());
-					assert!(update_fee.is_none());
-					expected_next_node = node_id.clone();
-					next_msgs = Some((update_fail_htlcs[0].clone(), commitment_signed.clone()));
+				Event::PaymentFailed { payment_hash, rejected_by_dest, all_paths_failed, .. } => {
+					assert_eq!(payment_hash, our_payment_hash);
+					assert!(rejected_by_dest);
+					assert_eq!(all_paths_failed, i == expected_paths.len() - 1);
 				},
 				_ => panic!("Unexpected event"),
 			}
-		} else {
-			assert!(events.is_empty());
-		}
-		if !skip_last && idx == expected_route.len() - 1 {
-			assert_eq!(expected_next_node, origin_node.node.get_our_node_id());
-		}
-
-		prev_node = node;
-	}
-
-	if !skip_last {
-		update_fail_dance!(origin_node, expected_route.first().unwrap(), true);
-
-		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"),
 		}
 	}
 }
 
-pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], our_payment_hash: PaymentHash)  {
-	fail_payment_along_route(origin_node, expected_route, false, our_payment_hash);
+pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], our_payment_hash: PaymentHash)  {
+	fail_payment_along_route(origin_node, &[&expected_path[..]], false, our_payment_hash);
 }
 
 pub fn create_chanmon_cfgs(node_count: usize) -> Vec<TestChanMonCfg> {

lightning/src/ln/functional_test_utils.rs

@@ -19,7 +19,7 @@ use chain::transaction::OutPoint;
 use chain::keysinterface::BaseSign;
 use ln::{PaymentPreimage, PaymentSecret, PaymentHash};
 use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
-use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
+use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, MppId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
 use ln::channel::{Channel, ChannelError};
 use ln::{chan_utils, onion_utils};
 use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
@@ -3308,7 +3308,7 @@ fn test_simple_peer_disconnect() {
 	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
 
 	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], true, payment_preimage_3);
-	fail_payment_along_route(&nodes[0], &[&nodes[1], &nodes[2]], true, payment_hash_5);
+	fail_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], true, payment_hash_5);
 
 	reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (1, 0), (1, 0), (false, false));
 	{
@@ -3886,7 +3886,8 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
 		// Use the utility function send_payment_along_path to send the payment with MPP data which
 		// indicates there are more HTLCs coming.
 		let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
-		nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, &None).unwrap();
+		let mpp_id = MppId([42; 32]);
+		nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, mpp_id, &None).unwrap();
 		check_added_monitors!(nodes[0], 1);
 		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 1);
@@ -4083,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
@@ -5913,9 +5942,10 @@ 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 error_code, ref error_data, ref all_paths_failed } => {
 			assert_eq!(our_payment_hash.clone(), *payment_hash);
 			assert_eq!(*rejected_by_dest, false);
+			assert_eq!(*all_paths_failed, true);
 			assert_eq!(*network_update, None);
 			assert_eq!(*error_code, None);
 			assert_eq!(*error_data, None);
@@ -5999,9 +6029,10 @@ 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 error_code, ref error_data, ref all_paths_failed } => {
 			assert_eq!(payment_hash_2.clone(), *payment_hash);
 			assert_eq!(*rejected_by_dest, false);
+			assert_eq!(*all_paths_failed, true);
 			assert_eq!(*network_update, None);
 			assert_eq!(*error_code, None);
 			assert_eq!(*error_data, None);

lightning/src/ln/functional_tests.rs

@@ -163,8 +163,9 @@ 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, ref error_code, error_data: _, ref all_paths_failed } = &events[0] {
 		assert_eq!(*rejected_by_dest, !expected_retryable);
+		assert_eq!(*all_paths_failed, true);
 		assert_eq!(*error_code, expected_error_code);
 		if expected_channel_update.is_some() {
 			match network_update {

lightning/src/ln/onion_route_tests.rs

@@ -332,7 +332,7 @@ pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type:
 /// 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 {
-	if let &HTLCSource::OutboundRoute { ref path, ref session_priv, ref first_hop_htlc_msat } = htlc_source {
+	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;
 		let mut error_code_ret = None;

lightning/src/ln/onion_utils.rs

@@ -1728,6 +1728,7 @@ mod tests {
 			net_graph_msg_handler.handle_event(&Event::PaymentFailed {
 				payment_hash: PaymentHash([0; 32]),
 				rejected_by_dest: false,
+				all_paths_failed: true,
 				network_update: Some(NetworkUpdate::ChannelUpdateMessage {
 					msg: valid_channel_update,
 				}),
@@ -1750,6 +1751,7 @@ mod tests {
 			net_graph_msg_handler.handle_event(&Event::PaymentFailed {
 				payment_hash: PaymentHash([0; 32]),
 				rejected_by_dest: false,
+				all_paths_failed: true,
 				network_update: Some(NetworkUpdate::ChannelClosed {
 					short_channel_id,
 					is_permanent: false,
@@ -1771,6 +1773,7 @@ mod tests {
 			net_graph_msg_handler.handle_event(&Event::PaymentFailed {
 				payment_hash: PaymentHash([0; 32]),
 				rejected_by_dest: false,
+				all_paths_failed: true,
 				network_update: Some(NetworkUpdate::ChannelClosed {
 					short_channel_id,
 					is_permanent: true,