Decode hold times as a sender

Hold times are surfaced via the PaymentPathSuccessful event.

Author: joostjager

fuzz/src/process_onion_failure.rs

@@ -115,7 +115,7 @@ fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 	let path = Path { hops, blinded_tail };
 
 	let htlc_source = HTLCSource::OutboundRoute {
-		path,
+		path: path.clone(),
 		session_priv,
 		first_hop_htlc_msat: 0,
 		payment_id,
@@ -133,8 +133,19 @@ fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 	} else {
 		None
 	};
-	let encrypted_packet = OnionErrorPacket { data: failure_data.into(), attribution_data };
+	let encrypted_packet =
+		OnionErrorPacket { data: failure_data.into(), attribution_data: attribution_data.clone() };
 	lightning::ln::process_onion_failure(&secp_ctx, &logger, &htlc_source, encrypted_packet);
+
+	if let Some(attribution_data) = attribution_data {
+		lightning::ln::process_onion_success(
+			&secp_ctx,
+			&logger,
+			&path,
+			&session_priv,
+			attribution_data,
+		);
+	}
 }
 
 /// Method that needs to be added manually, {name}_test

lightning-background-processor/src/lib.rs

@@ -2732,6 +2732,7 @@ mod tests {
 				payment_id: PaymentId([42; 32]),
 				payment_hash: None,
 				path: path.clone(),
+				hold_times: None,
 			});
 			let event = $receive.expect("PaymentPathSuccessful not handled within deadline");
 			match event {

lightning/src/events/mod.rs

@@ -1080,6 +1080,8 @@ pub enum Event {
 		///
 		/// May contain a closed channel if the HTLC sent along the path was fulfilled on chain.
 		path: Path,
+		/// The hold times as reported by each hop.
+		hold_times: Option<Vec<u32>>,
 	},
 	/// Indicates an outbound HTLC we sent failed, likely due to an intermediary node being unable to
 	/// handle the HTLC.
@@ -1816,10 +1818,16 @@ impl Writeable for Event {
 					(4, funding_info, required),
 				})
 			},
-			&Event::PaymentPathSuccessful { ref payment_id, ref payment_hash, ref path } => {
+			&Event::PaymentPathSuccessful {
+				ref payment_id,
+				ref payment_hash,
+				ref path,
+				ref hold_times,
+			} => {
 				13u8.write(writer)?;
 				write_tlv_fields!(writer, {
 					(0, payment_id, required),
+					(1, hold_times, option),
 					(2, payment_hash, option),
 					(4, path.hops, required_vec),
 					(6, path.blinded_tail, option),
@@ -2308,6 +2316,7 @@ impl MaybeReadable for Event {
 				let mut f = || {
 					_init_and_read_len_prefixed_tlv_fields!(reader, {
 						(0, payment_id, required),
+						(1, hold_times, option),
 						(2, payment_hash, option),
 						(4, path, required_vec),
 						(6, blinded_tail, option),
@@ -2316,6 +2325,7 @@ impl MaybeReadable for Event {
 						payment_id: payment_id.0.unwrap(),
 						payment_hash,
 						path: Path { hops: path, blinded_tail },
+						hold_times,
 					}))
 				};
 				f()

lightning/src/ln/channel.rs

@@ -403,7 +403,7 @@ impl OutboundHTLCState {
 enum OutboundHTLCOutcome {
 	/// We started always filling in the preimages here in 0.0.105, and the requirement
 	/// that the preimages always be filled in was added in 0.2.
-	Success(PaymentPreimage, #[allow(dead_code)] Option<AttributionData>),
+	Success(PaymentPreimage, Option<AttributionData>),
 	Failure(HTLCFailReason),
 }
 
@@ -1182,7 +1182,7 @@ pub(super) struct MonitorRestoreUpdates {
 	pub order: RAACommitmentOrder,
 	pub accepted_htlcs: Vec<(PendingHTLCInfo, u64)>,
 	pub failed_htlcs: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
-	pub finalized_claimed_htlcs: Vec<HTLCSource>,
+	pub finalized_claimed_htlcs: Vec<(HTLCSource, Option<AttributionData>)>,
 	pub pending_update_adds: Vec<msgs::UpdateAddHTLC>,
 	pub funding_broadcastable: Option<Transaction>,
 	pub channel_ready: Option<msgs::ChannelReady>,
@@ -2314,7 +2314,7 @@ where
 	// but need to handle this somehow or we run the risk of losing HTLCs!
 	monitor_pending_forwards: Vec<(PendingHTLCInfo, u64)>,
 	monitor_pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
-	monitor_pending_finalized_fulfills: Vec<HTLCSource>,
+	monitor_pending_finalized_fulfills: Vec<(HTLCSource, Option<AttributionData>)>,
 	monitor_pending_update_adds: Vec<msgs::UpdateAddHTLC>,
 	monitor_pending_tx_signatures: Option<msgs::TxSignatures>,
 
@@ -6720,7 +6720,8 @@ where
 			));
 		}
 
-		let outcome = OutboundHTLCOutcome::Success(msg.payment_preimage, None);
+		let outcome =
+			OutboundHTLCOutcome::Success(msg.payment_preimage, msg.attribution_data.clone());
 		self.mark_outbound_htlc_removed(msg.htlc_id, outcome).map(|htlc| {
 			(htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat, htlc.send_timestamp)
 		})
@@ -7543,16 +7544,21 @@ where
 						&htlc.payment_hash
 					);
 					// We really want take() here, but, again, non-mut ref :(
-					if let OutboundHTLCOutcome::Failure(mut reason) = outcome.clone() {
-						hold_time(htlc.send_timestamp, now).map(|hold_time| {
-							reason.set_hold_time(hold_time);
-						});
-
-						revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
-					} else {
-						finalized_claimed_htlcs.push(htlc.source.clone());
-						// They fulfilled, so we sent them money
-						value_to_self_msat_diff -= htlc.amount_msat as i64;
+					match outcome.clone() {
+						OutboundHTLCOutcome::Failure(mut reason) => {
+							hold_time(htlc.send_timestamp, now).map(|hold_time| {
+								reason.set_hold_time(hold_time);
+							});
+							revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
+						},
+						OutboundHTLCOutcome::Success(_, attribution_data) => {
+							// Even though a fast track was taken for fulfilled HTLCs to the incoming side, we still
+							// pass along attribution data here so that we can include hold time information in the
+							// final PaymentPathSuccessful events.
+							finalized_claimed_htlcs.push((htlc.source.clone(), attribution_data));
+							// They fulfilled, so we sent them money
+							value_to_self_msat_diff -= htlc.amount_msat as i64;
+						},
 					}
 					false
 				} else {
@@ -8042,7 +8048,7 @@ where
 		&mut self, resend_raa: bool, resend_commitment: bool, resend_channel_ready: bool,
 		mut pending_forwards: Vec<(PendingHTLCInfo, u64)>,
 		mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
-		mut pending_finalized_claimed_htlcs: Vec<HTLCSource>,
+		mut pending_finalized_claimed_htlcs: Vec<(HTLCSource, Option<AttributionData>)>,
 	) {
 		self.context.monitor_pending_revoke_and_ack |= resend_raa;
 		self.context.monitor_pending_commitment_signed |= resend_commitment;

lightning/src/ln/channelmanager.rs

@@ -77,8 +77,10 @@ use crate::ln::onion_payment::{
 	NextPacketDetails,
 };
 use crate::ln::onion_utils::{self};
+use crate::ln::onion_utils::{
+	decode_fulfill_attribution_data, HTLCFailReason, LocalHTLCFailureReason,
+};
 use crate::ln::onion_utils::{process_fulfill_attribution_data, AttributionData};
-use crate::ln::onion_utils::{HTLCFailReason, LocalHTLCFailureReason};
 use crate::ln::our_peer_storage::EncryptedOurPeerStorage;
 #[cfg(test)]
 use crate::ln::outbound_payment;
@@ -8002,8 +8004,27 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		);
 	}
 
-	fn finalize_claims(&self, sources: Vec<HTLCSource>) {
-		self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
+	fn finalize_claims(&self, sources: Vec<(HTLCSource, Option<AttributionData>)>) {
+		// Decode attribution data to hold times.
+		let hold_times = sources.into_iter().filter_map(|(source, attribution_data)| {
+			if let HTLCSource::OutboundRoute { ref session_priv, ref path, .. } = source {
+				let hold_times = attribution_data.map(|attribution_data| {
+					decode_fulfill_attribution_data(
+						&self.secp_ctx,
+						&self.logger,
+						path,
+						session_priv,
+						attribution_data,
+					)
+				});
+
+				Some((source, hold_times))
+			} else {
+				None
+			}
+		});
+
+		self.pending_outbound_payments.finalize_claims(hold_times, &self.pending_events);
 	}
 
 	fn claim_funds_internal(
@@ -16708,15 +16729,15 @@ mod tests {
 		let events = nodes[0].node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 2);
 		match events[0] {
-			Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path } => {
+			Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path, .. } => {
 				assert_eq!(payment_id, *actual_payment_id);
 				assert_eq!(our_payment_hash, *payment_hash.as_ref().unwrap());
 				assert_eq!(route.paths[0], *path);
 			},
 			_ => panic!("Unexpected event"),
 		}
 		match events[1] {
-			Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path } => {
+			Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path, ..} => {
 				assert_eq!(payment_id, *actual_payment_id);
 				assert_eq!(our_payment_hash, *payment_hash.as_ref().unwrap());
 				assert_eq!(route.paths[0], *path);

lightning/src/ln/mod.rs

@@ -52,6 +52,8 @@ pub use onion_utils::{create_payment_onion, LocalHTLCFailureReason};
 // without the node parameter being mut. This is incorrect, and thus newer rustcs will complain
 // about an unnecessary mut. Thus, we silence the unused_mut warning in two test modules below.
 
+#[cfg(fuzzing)]
+pub use onion_utils::decode_fulfill_attribution_data;
 #[cfg(fuzzing)]
 pub use onion_utils::process_onion_failure;