Merge pull request #3655 from shaavan/i2274

Expand `PaymentClaimable` to include all inbound channel IDs for a payment

Author: valentinewallace

lightning/src/events/mod.rs

@@ -805,10 +805,10 @@ pub enum Event {
 		/// Information for claiming this received payment, based on whether the purpose of the
 		/// payment is to pay an invoice or to send a spontaneous payment.
 		purpose: PaymentPurpose,
-		/// The `channel_id` indicating over which channel we received the payment.
-		via_channel_id: Option<ChannelId>,
-		/// The `user_channel_id` indicating over which channel we received the payment.
-		via_user_channel_id: Option<u128>,
+		/// The `(channel_id, user_channel_id)` pairs over which the payment was received.
+		///
+		/// This will be an incomplete vector for MPP payment events created/serialized using LDK version 0.1.0 and prior.
+		inbound_channel_ids: Vec<(ChannelId, Option<u128>)>,
 		/// The block height at which this payment will be failed back and will no longer be
 		/// eligible for claiming.
 		///
@@ -1553,7 +1553,7 @@ impl Writeable for Event {
 				// drop any channels which have not yet exchanged funding_signed.
 			},
 			&Event::PaymentClaimable { ref payment_hash, ref amount_msat, counterparty_skimmed_fee_msat,
-				ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id,
+				ref purpose, ref receiver_node_id, ref inbound_channel_ids,
 				ref claim_deadline, ref onion_fields, ref payment_id,
 			} => {
 				1u8.write(writer)?;
@@ -1587,20 +1587,30 @@ impl Writeable for Event {
 				}
 				let skimmed_fee_opt = if counterparty_skimmed_fee_msat == 0 { None }
 					else { Some(counterparty_skimmed_fee_msat) };
+
+				let (via_channel_id_legacy, via_user_channel_id_legacy) = match inbound_channel_ids.last() {
+					Some((chan_id, user_chan_id)) => (Some(*chan_id), *user_chan_id),
+					None => (None, None),
+				};
 				write_tlv_fields!(writer, {
 					(0, payment_hash, required),
 					(1, receiver_node_id, option),
 					(2, payment_secret, option),
-					(3, via_channel_id, option),
+					// Marked as legacy in version 0.2.0; superseded by `inbound_channel_ids`, which
+					// includes all channel IDs used in the payment instead of only the last one.
+					(3, via_channel_id_legacy, option),
 					(4, amount_msat, required),
-					(5, via_user_channel_id, option),
+					// Marked as legacy in version 0.2.0 for the same reason as `via_channel_id_legacy`;
+					// superseded by `via_user_channel_ids`.
+					(5, via_user_channel_id_legacy, option),
 					// Type 6 was `user_payment_id` on 0.0.103 and earlier
 					(7, claim_deadline, option),
 					(8, payment_preimage, option),
 					(9, onion_fields, option),
 					(10, skimmed_fee_opt, option),
 					(11, payment_context, option),
 					(13, payment_id, option),
+					(15, *inbound_channel_ids, optional_vec),
 				});
 			},
 			&Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref amount_msat, ref fee_paid_msat, ref bolt12_invoice } => {
@@ -1898,41 +1908,47 @@ impl MaybeReadable for Event {
 					let mut counterparty_skimmed_fee_msat_opt = None;
 					let mut receiver_node_id = None;
 					let mut _user_payment_id = None::<u64>; // Used in 0.0.103 and earlier, no longer written in 0.0.116+.
-					let mut via_channel_id = None;
+					let mut via_channel_id_legacy = None;
 					let mut claim_deadline = None;
-					let mut via_user_channel_id = None;
+					let mut via_user_channel_id_legacy = None;
 					let mut onion_fields = None;
 					let mut payment_context = None;
 					let mut payment_id = None;
+					let mut inbound_channel_ids_opt = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
 						(1, receiver_node_id, option),
 						(2, payment_secret, option),
-						(3, via_channel_id, option),
+						(3, via_channel_id_legacy, option),
 						(4, amount_msat, required),
-						(5, via_user_channel_id, option),
+						(5, via_user_channel_id_legacy, option),
 						(6, _user_payment_id, option),
 						(7, claim_deadline, option),
 						(8, payment_preimage, option),
 						(9, onion_fields, option),
 						(10, counterparty_skimmed_fee_msat_opt, option),
 						(11, payment_context, option),
 						(13, payment_id, option),
+						(15, inbound_channel_ids_opt, optional_vec),
 					});
 					let purpose = match payment_secret {
 						Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context)
 							.map_err(|()| msgs::DecodeError::InvalidValue)?,
 						None if payment_preimage.is_some() => PaymentPurpose::SpontaneousPayment(payment_preimage.unwrap()),
 						None => return Err(msgs::DecodeError::InvalidValue),
 					};
+
+					let inbound_channel_ids = inbound_channel_ids_opt
+						.or_else(|| via_channel_id_legacy.map(|chan_id| vec![(chan_id, via_user_channel_id_legacy)]))
+						.unwrap_or_default();
+
 					Ok(Some(Event::PaymentClaimable {
 						receiver_node_id,
 						payment_hash,
 						amount_msat,
 						counterparty_skimmed_fee_msat: counterparty_skimmed_fee_msat_opt.unwrap_or(0),
 						purpose,
-						via_channel_id,
-						via_user_channel_id,
+						inbound_channel_ids,
 						claim_deadline,
 						onion_fields,
 						payment_id,

lightning/src/ln/blinded_payment_tests.rs

@@ -247,8 +247,26 @@ fn mpp_to_one_hop_blinded_path() {
 		Some(payment_secret), ev.clone(), false, None);
 
 	let ev = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
-	pass_along_path(&nodes[0], expected_route[1], amt_msat, payment_hash.clone(),
+	let event = pass_along_path(&nodes[0], expected_route[1], amt_msat, payment_hash.clone(),
 		Some(payment_secret), ev.clone(), true, None);
+
+	match event.unwrap() {
+		Event::PaymentClaimable { mut inbound_channel_ids, .. } => {
+			let mut expected_inbound_channel_ids = nodes[3].node.list_channels()
+				.iter()
+				.map(|d| (d.channel_id, Some(d.user_channel_id)))
+				.collect::<Vec<(_, _)>>();
+
+			// `list_channels` returns channels in arbitrary order, so we sort both vectors
+			// to ensure the comparison is order-agnostic.
+			inbound_channel_ids.sort();
+			expected_inbound_channel_ids.sort();
+
+			assert_eq!(inbound_channel_ids, expected_inbound_channel_ids);
+		}
+		_ => panic!("Unexpected event"),
+	}
+
 	claim_payment_along_route(
 		ClaimAlongRouteArgs::new(&nodes[0], expected_route, payment_preimage)
 	);

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -125,6 +125,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 	let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
+	let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
 
 	let (route, payment_hash_1, payment_preimage_1, payment_secret_1) = get_route_and_payment_hash!(&nodes[0], nodes[1], 1000000);
 
@@ -166,11 +167,11 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
 	let events_3 = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events_3.len(), 1);
 	match events_3[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(payment_hash_1, *payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
-			assert_eq!(via_channel_id, Some(channel_id));
+			assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());
@@ -248,6 +249,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 	let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
+	let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
 
 	let (payment_preimage_1, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
 
@@ -548,11 +550,11 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 	let events_5 = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events_5.len(), 1);
 	match events_5[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(payment_hash_2, *payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
-			assert_eq!(via_channel_id, Some(channel_id));
+			assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());
@@ -602,6 +604,7 @@ fn test_monitor_update_fail_cs() {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 	let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
+	let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
 
 	let (route, our_payment_hash, payment_preimage, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000);
 	{
@@ -666,11 +669,11 @@ fn test_monitor_update_fail_cs() {
 	let events = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
 	match events[0] {
-		Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
+		Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(payment_hash, our_payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
-			assert_eq!(via_channel_id, Some(channel_id));
+			assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());
@@ -1679,12 +1682,11 @@ fn test_monitor_update_fail_claim() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match events[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id, .. } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(payment_hash_2, *payment_hash);
 			assert_eq!(1_000_000, amount_msat);
 			assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());
-			assert_eq!(via_channel_id, Some(channel_id));
-			assert_eq!(via_user_channel_id, Some(42));
+			assert_eq!(*inbound_channel_ids.last().unwrap(), (channel_id, Some(42)));
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());
@@ -1696,11 +1698,11 @@ fn test_monitor_update_fail_claim() {
 		_ => panic!("Unexpected event"),
 	}
 	match events[1] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(payment_hash_3, *payment_hash);
 			assert_eq!(1_000_000, amount_msat);
 			assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());
-			assert_eq!(via_channel_id, Some(channel_id));
+			assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(42))]);
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());

lightning/src/ln/channelmanager.rs

@@ -930,6 +930,15 @@ impl ClaimablePayment {
 			self.htlcs.iter().map(|htlc| (htlc.prev_hop.channel_id, htlc.prev_hop.htlc_id))
 		)
 	}
+
+	/// Returns the inbound `(channel_id, user_channel_id)` pairs for all HTLCs associated with the payment.
+	///
+	/// Note: The `user_channel_id` will be `None` for HTLCs created using LDK version 0.0.117 or prior.
+	fn inbound_channel_ids(&self) -> Vec<(ChannelId, Option<u128>)> {
+		self.htlcs.iter().map(|htlc| {
+			(htlc.prev_hop.channel_id, htlc.prev_hop.user_channel_id)
+		}).collect()
+	}
 }
 
 /// Represent the channel funding transaction type.
@@ -6353,8 +6362,7 @@ where
 												purpose: $purpose,
 												amount_msat,
 												counterparty_skimmed_fee_msat,
-												via_channel_id: Some(prev_channel_id),
-												via_user_channel_id: Some(prev_user_channel_id),
+												inbound_channel_ids: claimable_payment.inbound_channel_ids(),
 												claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER),
 												onion_fields: claimable_payment.onion_fields.clone(),
 												payment_id: Some(payment_id),
@@ -11032,7 +11040,18 @@ where
 		let events = core::cell::RefCell::new(Vec::new());
 		let event_handler = |event: events::Event| Ok(events.borrow_mut().push(event));
 		self.process_pending_events(&event_handler);
-		events.into_inner()
+		let collected_events = events.into_inner();
+
+		// To expand the coverage and make sure all events are properly serialised and deserialised,
+		// we test all generated events round-trip:
+		for event in &collected_events {
+			let ser = event.encode();
+			if let Some(deser) = events::Event::read(&mut &ser[..]).expect("event should deserialize") {
+				assert_eq!(&deser, event, "event should roundtrip correctly");
+			}
+		}
+
+		collected_events
 	}
 
 	#[cfg(feature = "_test_utils")]

lightning/src/ln/functional_test_utils.rs

@@ -2770,7 +2770,7 @@ pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event>
 				assert_eq!(events_2.len(), 1);
 				match &events_2[0] {
 					Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat,
-						receiver_node_id, ref via_channel_id, ref via_user_channel_id,
+						receiver_node_id, ref inbound_channel_ids,
 						claim_deadline, onion_fields, ..
 					} => {
 						assert_eq!(our_payment_hash, *payment_hash);
@@ -2804,8 +2804,11 @@ pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event>
 							},
 						}
 						assert_eq!(*amount_msat, recv_value);
-						assert!(node.node.list_channels().iter().any(|details| details.channel_id == via_channel_id.unwrap()));
-						assert!(node.node.list_channels().iter().any(|details| details.user_channel_id == via_user_channel_id.unwrap()));
+						let channels = node.node.list_channels();
+						for (chan_id, user_chan_id) in inbound_channel_ids {
+							let chan = channels.iter().find(|details| &details.channel_id == chan_id).unwrap();
+							assert_eq!(*user_chan_id, Some(chan.user_channel_id));
+						}
 						assert!(claim_deadline.unwrap() > node.best_block_info().1);
 					},
 					_ => panic!("Unexpected event"),

lightning/src/ln/functional_tests.rs

@@ -1932,6 +1932,7 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 	let chan_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 190000, 1001);
 	let chan_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 190000, 1001);
+	let chan_2_user_id = nodes[2].node.list_channels()[0].user_channel_id;
 
 	let mut stat01 = get_channel_value_stat!(nodes[0], nodes[1], chan_1.2);
 	let mut stat11 = get_channel_value_stat!(nodes[1], nodes[0], chan_1.2);
@@ -2126,11 +2127,11 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
 	let events = nodes[2].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match events[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(our_payment_hash_21, *payment_hash);
 			assert_eq!(recv_value_21, amount_msat);
 			assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
-			assert_eq!(via_channel_id, Some(chan_2.2));
+			assert_eq!(*inbound_channel_ids, vec![(chan_2.2, Some(chan_2_user_id))]);
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());
@@ -2142,11 +2143,11 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
 		_ => panic!("Unexpected event"),
 	}
 	match events[1] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(our_payment_hash_22, *payment_hash);
 			assert_eq!(recv_value_22, amount_msat);
 			assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
-			assert_eq!(via_channel_id, Some(chan_2.2));
+			assert_eq!(*inbound_channel_ids, vec![(chan_2.2, Some(chan_2_user_id))]);
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());
@@ -4259,6 +4260,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
 	} else {
 		create_announced_chan_between_nodes(&nodes, 0, 1).2
 	};
+	let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
 
 	let (route, payment_hash_1, payment_preimage_1, payment_secret_1) = get_route_and_payment_hash!(nodes[0], nodes[1], 1_000_000);
 
@@ -4372,11 +4374,11 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
 	let events_2 = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events_2.len(), 1);
 	match events_2[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
 			assert_eq!(payment_hash_1, *payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
-			assert_eq!(via_channel_id, Some(channel_id));
+			assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
 			match &purpose {
 				PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
 					assert!(payment_preimage.is_none());