Merge pull request #2924 from tnull/2024-03-add-user-channel-id-to-payment-forwarded

Expose `{prev,next}_user_channel_id` fields in `PaymentForwarded`

Author: valentinewallace

lightning/src/events/mod.rs

@@ -797,12 +797,24 @@ pub enum Event {
 	/// This event is generated when a payment has been successfully forwarded through us and a
 	/// forwarding fee earned.
 	PaymentForwarded {
-		/// 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.
+		/// 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 outgoing channel between the next node and us. This is only `None` for events
-		/// generated or serialized by versions prior to 0.0.107.
+		/// 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 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
@@ -1121,8 +1133,9 @@ impl Writeable for Event {
 				});
 			}
 			&Event::PaymentForwarded {
-				total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx,
-				next_channel_id, outbound_amount_forwarded_msat, skimmed_fee_msat,
+				prev_channel_id, next_channel_id, prev_user_channel_id, next_user_channel_id,
+				total_fee_earned_msat, skimmed_fee_msat, claim_from_onchain_tx,
+				outbound_amount_forwarded_msat,
 			} => {
 				7u8.write(writer)?;
 				write_tlv_fields!(writer, {
@@ -1132,6 +1145,8 @@ impl Writeable for Event {
 					(3, 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),
 				});
 			},
 			&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason,
@@ -1427,23 +1442,28 @@ impl MaybeReadable for Event {
 			},
 			7u8 => {
 				let f = || {
-					let mut total_fee_earned_msat = None;
 					let mut prev_channel_id = None;
-					let mut claim_from_onchain_tx = false;
 					let mut next_channel_id = None;
-					let mut outbound_amount_forwarded_msat = None;
+					let mut prev_user_channel_id = None;
+					let mut next_user_channel_id = 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;
 					read_tlv_fields!(reader, {
 						(0, total_fee_earned_msat, option),
 						(1, prev_channel_id, option),
 						(2, claim_from_onchain_tx, required),
 						(3, 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),
 					});
 					Ok(Some(Event::PaymentForwarded {
-						total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id,
-						outbound_amount_forwarded_msat, skimmed_fee_msat,
+						prev_channel_id, next_channel_id, prev_user_channel_id,
+						next_user_channel_id, total_fee_earned_msat, skimmed_fee_msat,
+						claim_from_onchain_tx, outbound_amount_forwarded_msat,
 					}))
 				};
 				f()

lightning/src/ln/channelmanager.rs

@@ -5740,7 +5740,7 @@ where
 	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 		forwarded_htlc_value_msat: Option<u64>, skimmed_fee_msat: Option<u64>, from_onchain: bool,
 		startup_replay: bool, next_channel_counterparty_node_id: Option<PublicKey>,
-		next_channel_outpoint: OutPoint, next_channel_id: ChannelId,
+		next_channel_outpoint: OutPoint, next_channel_id: ChannelId, next_user_channel_id: Option<u128>,
 	) {
 		match source {
 			HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
@@ -5759,11 +5759,10 @@ where
 			},
 			HTLCSource::PreviousHopData(hop_data) => {
 				let prev_channel_id = hop_data.channel_id;
+				let prev_user_channel_id = hop_data.user_channel_id;
 				let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 				#[cfg(debug_assertions)]
 				let claiming_chan_funding_outpoint = hop_data.outpoint;
-				#[cfg(debug_assertions)]
-				let claiming_channel_id = hop_data.channel_id;
 				let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 					|htlc_claim_value_msat, definitely_duplicate| {
 						let chan_to_release =
@@ -5821,7 +5820,7 @@ where
 										BackgroundEvent::MonitorUpdatesComplete {
 											channel_id, ..
 										} =>
-											*channel_id == claiming_channel_id,
+											*channel_id == prev_channel_id,
 									}
 								}), "{:?}", *background_events);
 							}
@@ -5845,12 +5844,14 @@ where
 								"skimmed_fee_msat must always be included in total_fee_earned_msat");
 							Some(MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel {
 								event: events::Event::PaymentForwarded {
-									total_fee_earned_msat,
-									claim_from_onchain_tx: from_onchain,
 									prev_channel_id: Some(prev_channel_id),
 									next_channel_id: Some(next_channel_id),
-									outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
+									prev_user_channel_id,
+									next_user_channel_id,
+									total_fee_earned_msat,
 									skimmed_fee_msat,
+									claim_from_onchain_tx: from_onchain,
+									outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 								},
 								downstream_counterparty_and_funding_outpoint: chan_to_release,
 							})
@@ -6824,6 +6825,7 @@ where
 
 	fn internal_update_fulfill_htlc(&self, counterparty_node_id: &PublicKey, msg: &msgs::UpdateFulfillHTLC) -> Result<(), MsgHandleErrInternal> {
 		let funding_txo;
+		let next_user_channel_id;
 		let (htlc_source, forwarded_htlc_value, skimmed_fee_msat) = {
 			let per_peer_state = self.per_peer_state.read().unwrap();
 			let peer_state_mutex = per_peer_state.get(counterparty_node_id)
@@ -6853,6 +6855,7 @@ where
 						// outbound HTLC is claimed. This is guaranteed to all complete before we
 						// process the RAA as messages are processed from single peers serially.
 						funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
+						next_user_channel_id = chan.context.get_user_id();
 						res
 					} else {
 						return try_chan_phase_entry!(self, Err(ChannelError::Close(
@@ -6864,7 +6867,7 @@ where
 		};
 		self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(),
 			Some(forwarded_htlc_value), skimmed_fee_msat, false, false, Some(*counterparty_node_id),
-			funding_txo, msg.channel_id
+			funding_txo, msg.channel_id, Some(next_user_channel_id),
 		);
 
 		Ok(())
@@ -7366,7 +7369,7 @@ where
 							log_trace!(logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 							self.claim_funds_internal(htlc_update.source, preimage,
 								htlc_update.htlc_value_satoshis.map(|v| v * 1000), None, true,
-								false, counterparty_node_id, funding_outpoint, channel_id);
+								false, counterparty_node_id, funding_outpoint, channel_id, None);
 						} else {
 							log_trace!(logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 							let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id };
@@ -11386,7 +11389,9 @@ where
 			// don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 			// channel is closed we just assume that it probably came from an on-chain claim.
 			channel_manager.claim_funds_internal(source, preimage, Some(downstream_value), None,
-				downstream_closed, true, downstream_node_id, downstream_funding, downstream_channel_id);
+				downstream_closed, true, downstream_node_id, downstream_funding,
+				downstream_channel_id, None
+			);
 		}
 
 		//TODO: Broadcast channel update for closed channels, but only after we've made a

lightning/src/ln/functional_test_utils.rs

@@ -2231,8 +2231,8 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
 ) -> Option<u64> {
 	match event {
 		Event::PaymentForwarded {
-			total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id,
-			outbound_amount_forwarded_msat: _, skimmed_fee_msat
+			prev_channel_id, next_channel_id, prev_user_channel_id, next_user_channel_id,
+			total_fee_earned_msat, skimmed_fee_msat, claim_from_onchain_tx, ..
 		} => {
 			if allow_1_msat_fee_overpay {
 				// Aggregating fees for blinded paths may result in a rounding error, causing slight
@@ -2249,12 +2249,31 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
 			assert!(skimmed_fee_msat == expected_extra_fees_msat);
 			if !upstream_force_closed {
 				// Is the event prev_channel_id in one of the channels between the two nodes?
-				assert!(node.node().list_channels().iter().any(|x| x.counterparty.node_id == prev_node.node().get_our_node_id() && x.channel_id == prev_channel_id.unwrap()));
+				assert!(node.node().list_channels().iter().any(|x|
+					x.counterparty.node_id == prev_node.node().get_our_node_id() &&
+					x.channel_id == prev_channel_id.unwrap() &&
+					x.user_channel_id == prev_user_channel_id.unwrap()
+				));
 			}
 			// We check for force closures since a force closed channel is removed from the
 			// node's channel list
 			if !downstream_force_closed {
-				assert!(node.node().list_channels().iter().any(|x| x.counterparty.node_id == next_node.node().get_our_node_id() && x.channel_id == next_channel_id.unwrap()));
+				// As documented, `next_user_channel_id` will only be `Some` if we didn't settle via an
+				// onchain transaction, just as the `total_fee_earned_msat` field. Rather than
+				// introducing yet another variable, we use the latter's state as a flag to detect
+				// this and only check if it's `Some`.
+				if total_fee_earned_msat.is_none() {
+					assert!(node.node().list_channels().iter().any(|x|
+						x.counterparty.node_id == next_node.node().get_our_node_id() &&
+						x.channel_id == next_channel_id.unwrap()
+					));
+				} else {
+					assert!(node.node().list_channels().iter().any(|x|
+						x.counterparty.node_id == next_node.node().get_our_node_id() &&
+						x.channel_id == next_channel_id.unwrap() &&
+						x.user_channel_id == next_user_channel_id.unwrap()
+					));
+				}
 			}
 			assert_eq!(claim_from_onchain_tx, downstream_force_closed);
 			total_fee_earned_msat