prefactor: Allow multiple htlcs in/out in forwarding events for trampoline

lightning-liquidity/tests/lsps2_integration_tests.rs

@@ -1331,14 +1331,14 @@ fn client_trusts_lsp_end_to_end_test() {
 
 	let total_fee_msat = match service_events[0].clone() {
 		Event::PaymentForwarded {
-			prev_node_id,
-			next_node_id,
+			ref prev_htlcs,
+			ref next_htlcs,
 			skimmed_fee_msat,
 			total_fee_earned_msat,
 			..
 		} => {
-			assert_eq!(prev_node_id, Some(payer_node_id));
-			assert_eq!(next_node_id, Some(client_node_id));
+			assert_eq!(prev_htlcs[0].node_id, Some(payer_node_id));
+			assert_eq!(next_htlcs[0].node_id, Some(client_node_id));
 			service_handler.payment_forwarded(channel_id, skimmed_fee_msat.unwrap_or(0)).unwrap();
 			Some(total_fee_earned_msat.unwrap() - skimmed_fee_msat.unwrap())
 		},

lightning/src/chain/channelmonitor.rs

@@ -2795,6 +2795,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				let outbound_payment = match source {
 					None => panic!("Outbound HTLCs should have a source"),
 					Some(&HTLCSource::PreviousHopData(_)) => false,
+					Some(&HTLCSource::TrampolineForward { .. }) => false,
 					Some(&HTLCSource::OutboundRoute { .. }) => true,
 				};
 				return Some(Balance::MaybeTimeoutClaimableHTLC {
@@ -3007,6 +3008,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 					let outbound_payment = match source {
 						None => panic!("Outbound HTLCs should have a source"),
 						Some(HTLCSource::PreviousHopData(_)) => false,
+						Some(HTLCSource::TrampolineForward { .. }) => false,
 						Some(HTLCSource::OutboundRoute { .. }) => true,
 					};
 					if outbound_payment {

lightning/src/events/mod.rs

@@ -584,6 +584,10 @@ pub enum HTLCHandlingFailureType {
 		/// The payment hash of the payment we attempted to process.
 		payment_hash: PaymentHash,
 	},
+	/// We were responsible for pathfinding and forwarding of a trampoline payment, but failed to
+	/// do so. An example of such an instance is when we can't find a route to the specified
+	/// trampoline destination.
+	TrampolineForward {},
 }
 
 impl_writeable_tlv_based_enum_upgradable!(HTLCHandlingFailureType,
@@ -601,6 +605,7 @@ impl_writeable_tlv_based_enum_upgradable!(HTLCHandlingFailureType,
 	(4, Receive) => {
 		(0, payment_hash, required),
 	},
+	(5, TrampolineForward) => {},
 );
 
 /// The reason for HTLC failures in [`Event::HTLCHandlingFailed`].
@@ -738,6 +743,31 @@ pub enum InboundChannelFunds {
 	DualFunded,
 }
 
+/// Identifies the channel and peer committed to a HTLC, used for both incoming and outgoing HTLCs.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct HTLCLocator {
+	/// The channel that the HTLC was sent or received on.
+	pub channel_id: ChannelId,
+
+	/// The `user_channel_id` for `channel_id`.
+	///
+	/// This will be `None` if the payment was settled via an on-chain transaction. It will also
+	/// be `None` for events serialized by versions prior to 0.0.122.
+	pub user_channel_id: Option<u128>,
+
+	/// The public key identity of the node that the HTLC was sent to or received from.
+	///
+	/// This is only `None` for HTLCs received prior to 0.1 or for events serialized by versions
+	/// prior to 0.1.
+	pub node_id: Option<PublicKey>,
+}
+
+impl_writeable_tlv_based!(HTLCLocator, {
+	(1, channel_id, required),
+	(3, user_channel_id, option),
+	(5, node_id, option),
+});
+
 /// An Event which you should probably take some action in response to.
 ///
 /// Note that while Writeable and Readable are implemented for Event, you probably shouldn't use
@@ -1331,38 +1361,22 @@ pub enum Event {
 	/// This event is generated when a payment has been successfully forwarded through us and a
 	/// forwarding fee earned.
 	///
+	/// Note that downgrading from 0.3 and above with pending trampoline forwards that use multipart
+	/// payments will produce an event that only provides information about the first htlc that was
+	/// received/dispatched.
+	///
 	/// # Failure Behavior and Persistence
 	/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
 	/// returning `Err(ReplayEvent ())`) and will be persisted across restarts.
 	PaymentForwarded {
-		/// The channel id of the incoming channel between the previous node and us.
-		///
-		/// This is only `None` for events generated or serialized by versions prior to 0.0.107.
-		prev_channel_id: Option<ChannelId>,
-		/// The channel id of the outgoing channel between the next node and us.
-		///
-		/// This is only `None` for events generated or serialized by versions prior to 0.0.107.
-		next_channel_id: Option<ChannelId>,
-		/// The `user_channel_id` of the incoming channel between the previous node and us.
-		///
-		/// This is only `None` for events generated or serialized by versions prior to 0.0.122.
-		prev_user_channel_id: Option<u128>,
-		/// The `user_channel_id` of the outgoing channel between the next node and us.
-		///
-		/// This will be `None` if the payment was settled via an on-chain transaction. See the
-		/// caveat described for the `total_fee_earned_msat` field. Moreover it will be `None` for
-		/// events generated or serialized by versions prior to 0.0.122.
-		next_user_channel_id: Option<u128>,
-		/// The node id of the previous node.
-		///
-		/// This is only `None` for HTLCs received prior to 0.1 or for events serialized by
-		/// versions prior to 0.1
-		prev_node_id: Option<PublicKey>,
-		/// The node id of the next node.
-		///
-		/// This is only `None` for HTLCs received prior to 0.1 or for events serialized by
-		/// versions prior to 0.1
-		next_node_id: Option<PublicKey>,
+		/// The set of HTLCs forwarded to our node that will be claimed by this forward. Contains a
+		/// single HTLC for source-routed payments, and may contain multiple HTLCs when we acted as
+		/// a trampoline router, responsible for pathfinding within the route.
+		prev_htlcs: Vec<HTLCLocator>,
+		/// The set of HTLCs forwarded by our node that have been claimed by this forward. Contains
+		/// a single HTLC for regular source-routed payments, and may contain multiple HTLCs when
+		/// we acted as a trampoline router, responsible for pathfinding within the route.
+		next_htlcs: Vec<HTLCLocator>,
 		/// The total fee, in milli-satoshis, which was earned as a result of the payment.
 		///
 		/// Note that if we force-closed the channel over which we forwarded an HTLC while the HTLC
@@ -1656,12 +1670,17 @@ pub enum Event {
 	/// Indicates that the HTLC was accepted, but could not be processed when or after attempting to
 	/// forward it.
 	///
+	/// Note that downgrading from 0.3 with pending trampoline forwards that have incoming multipart
+	/// payments will produce an event that only provides information about the first htlc that was
+	/// received/dispatched.
+	///
 	/// # Failure Behavior and Persistence
 	/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
 	/// returning `Err(ReplayEvent ())`) and will be persisted across restarts.
 	HTLCHandlingFailed {
-		/// The channel over which the HTLC was received.
-		prev_channel_id: ChannelId,
+		/// The channel(s) over which the HTLC(s) was received. May contain multiple entries for
+		/// trampoline forwards.
+		prev_channel_ids: Vec<ChannelId>,
 		/// The type of HTLC handling that failed.
 		failure_type: HTLCHandlingFailureType,
 		/// The reason that the HTLC failed.
@@ -2026,29 +2045,47 @@ impl Writeable for Event {
 				});
 			},
 			&Event::PaymentForwarded {
-				prev_channel_id,
-				next_channel_id,
-				prev_user_channel_id,
-				next_user_channel_id,
-				prev_node_id,
-				next_node_id,
+				ref prev_htlcs,
+				ref next_htlcs,
 				total_fee_earned_msat,
 				skimmed_fee_msat,
 				claim_from_onchain_tx,
 				outbound_amount_forwarded_msat,
 			} => {
 				7u8.write(writer)?;
+				// Fields 1, 3, 9, 11, 13 and 15 are written for backwards compatibility. We don't
+				// want to fail writes, so we write garbage data if we don't have at least on htlc.
+				debug_assert!(
+					!prev_htlcs.is_empty(),
+					"at least one prev_htlc required for PaymentForwarded",
+				);
+				debug_assert!(
+					!next_htlcs.is_empty(),
+					"at least one next_htlc required for PaymentForwarded",
+				);
+				let empty_locator = HTLCLocator {
+					channel_id: ChannelId::new_zero(),
+					user_channel_id: None,
+					node_id: None,
+				};
+				let legacy_prev = prev_htlcs.first().unwrap_or(&empty_locator);
+				let legacy_next = next_htlcs.first().unwrap_or(&empty_locator);
 				write_tlv_fields!(writer, {
 					(0, total_fee_earned_msat, option),
-					(1, prev_channel_id, option),
+					(1, Some(legacy_prev.channel_id), option),
 					(2, claim_from_onchain_tx, required),
-					(3, next_channel_id, option),
+					(3, Some(legacy_next.channel_id), option),
 					(5, outbound_amount_forwarded_msat, option),
 					(7, skimmed_fee_msat, option),
-					(9, prev_user_channel_id, option),
-					(11, next_user_channel_id, option),
-					(13, prev_node_id, option),
-					(15, next_node_id, option),
+					(9, legacy_prev.user_channel_id, option),
+					(11, legacy_next.user_channel_id, option),
+					(13, legacy_prev.node_id, option),
+					(15, legacy_next.node_id, option),
+					// HTLCs are written as required, rather than required_vec, so that they can be
+					// deserialized using default_value to fill in legacy fields which expects
+					// LengthReadable (required_vec is WithoutLength).
+					(17, *prev_htlcs, required),
+					(19, *next_htlcs, required),
 				});
 			},
 			&Event::ChannelClosed {
@@ -2196,15 +2233,24 @@ impl Writeable for Event {
 				})
 			},
 			&Event::HTLCHandlingFailed {
-				ref prev_channel_id,
+				ref prev_channel_ids,
 				ref failure_type,
 				ref failure_reason,
 			} => {
 				25u8.write(writer)?;
+				// Legacy field is written for backwards compatibility. We don't want to fail writes
+				// so we write garbage data if we don't have the data we expect.
+				debug_assert!(
+					!prev_channel_ids.is_empty(),
+					"at least one prev_channel_id required for HTLCHandlingFailed"
+				);
+				let zero_id = ChannelId::new_zero();
+				let legacy_chan_id = prev_channel_ids.first().unwrap_or(&zero_id);
 				write_tlv_fields!(writer, {
-					(0, prev_channel_id, required),
+					(0, legacy_chan_id, required),
 					(1, failure_reason, option),
 					(2, failure_type, required),
+					(3, *prev_channel_ids, required),
 				})
 			},
 			&Event::BumpTransaction(ref event) => {
@@ -2548,35 +2594,59 @@ impl MaybeReadable for Event {
 			},
 			7u8 => {
 				let mut f = || {
-					let mut prev_channel_id = None;
-					let mut next_channel_id = None;
-					let mut prev_user_channel_id = None;
-					let mut next_user_channel_id = None;
-					let mut prev_node_id = None;
-					let mut next_node_id = None;
+					// Legacy values that have been replaced by prev_htlcs and next_htlcs.
+					let mut prev_channel_id_legacy = None;
+					let mut next_channel_id_legacy = None;
+					let mut prev_user_channel_id_legacy = None;
+					let mut next_user_channel_id_legacy = None;
+					let mut prev_node_id_legacy = None;
+					let mut next_node_id_legacy = None;
+
 					let mut total_fee_earned_msat = None;
 					let mut skimmed_fee_msat = None;
 					let mut claim_from_onchain_tx = false;
 					let mut outbound_amount_forwarded_msat = None;
+					let mut prev_htlcs = vec![];
+					let mut next_htlcs = vec![];
 					read_tlv_fields!(reader, {
 						(0, total_fee_earned_msat, option),
-						(1, prev_channel_id, option),
+						(1, prev_channel_id_legacy, option),
 						(2, claim_from_onchain_tx, required),
-						(3, next_channel_id, option),
+						(3, next_channel_id_legacy, option),
 						(5, outbound_amount_forwarded_msat, option),
 						(7, skimmed_fee_msat, option),
-						(9, prev_user_channel_id, option),
-						(11, next_user_channel_id, option),
-						(13, prev_node_id, option),
-						(15, next_node_id, option),
+						(9, prev_user_channel_id_legacy, option),
+						(11, next_user_channel_id_legacy, option),
+						(13, prev_node_id_legacy, option),
+						(15, next_node_id_legacy, option),
+						// We never expect prev/next_channel_id_legacy to be None because this field
+						// was only None for versions before 0.0.107 and we do not allow upgrades
+						// with pending forwards to 0.1 for any version 0.0.123 or earlier. We
+						// currently write the legacy fields for backwards compatibility, but we
+						// use a zero channel ID in the eagerly evaluated default_value block to
+						// allow the legacy field to be deprecated in future.
+						(17, prev_htlcs, (default_value, vec![HTLCLocator{
+							channel_id: prev_channel_id_legacy.unwrap_or(ChannelId::new_zero()),
+							user_channel_id: prev_user_channel_id_legacy,
+							node_id: prev_node_id_legacy,
+						}])),
+						(19, next_htlcs, (default_value, vec![HTLCLocator{
+							channel_id: next_channel_id_legacy.unwrap_or(ChannelId::new_zero()),
+							user_channel_id: next_user_channel_id_legacy,
+							node_id: next_node_id_legacy,
+						}])),
 					});
+
+					// Legacy fields must be present if prev/next_htlcs are not.
+					if prev_htlcs.is_empty() && prev_node_id_legacy.is_none()
+						|| next_htlcs.is_empty() && next_node_id_legacy.is_none()
+					{
+						return Err(msgs::DecodeError::InvalidValue);
+					}
+
 					Ok(Some(Event::PaymentForwarded {
-						prev_channel_id,
-						next_channel_id,
-						prev_user_channel_id,
-						next_user_channel_id,
-						prev_node_id,
-						next_node_id,
+						prev_htlcs,
+						next_htlcs,
 						total_fee_earned_msat,
 						skimmed_fee_msat,
 						claim_from_onchain_tx,
@@ -2766,13 +2836,17 @@ impl MaybeReadable for Event {
 			},
 			25u8 => {
 				let mut f = || {
-					let mut prev_channel_id = ChannelId::new_zero();
+					let mut prev_channel_id_legacy = ChannelId::new_zero();
 					let mut failure_reason = None;
 					let mut failure_type_opt = UpgradableRequired(None);
+					let mut prev_channel_ids = vec![];
 					read_tlv_fields!(reader, {
-						(0, prev_channel_id, required),
+						(0, prev_channel_id_legacy, required),
 						(1, failure_reason, option),
 						(2, failure_type_opt, upgradable_required),
+						(3, prev_channel_ids, (default_value, vec![
+							prev_channel_id_legacy,
+						])),
 					});
 
 					// If a legacy HTLCHandlingFailureType::UnknownNextHop was written, upgrade
@@ -2787,7 +2861,7 @@ impl MaybeReadable for Event {
 						failure_reason = Some(LocalHTLCFailureReason::UnknownNextPeer.into());
 					}
 					Ok(Some(Event::HTLCHandlingFailed {
-						prev_channel_id,
+						prev_channel_ids,
 						failure_type: _init_tlv_based_struct_field!(
 							failure_type_opt,
 							upgradable_required

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -3940,11 +3940,11 @@ fn do_test_durable_preimages_on_closed_channel(
 	let evs = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(evs.len(), if close_chans_before_reload { 2 } else { 1 });
 	for ev in evs {
-		if let Event::PaymentForwarded { claim_from_onchain_tx, next_user_channel_id, .. } = ev {
+		if let Event::PaymentForwarded { claim_from_onchain_tx, next_htlcs, .. } = ev {
 			if !claim_from_onchain_tx {
 				// If the outbound channel is still open, the `next_user_channel_id` should be available.
 				// This was previously broken.
-				assert!(next_user_channel_id.is_some())
+				assert!(next_htlcs[0].user_channel_id.is_some())
 			}
 		} else {
 			panic!();