Add hold times to update_fulfill_htlc

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,13 +1818,19 @@ 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),
 					(2, payment_hash, option),
 					(4, path.hops, required_vec),
 					(6, path.blinded_tail, option),
+					(8, hold_times, option),
 				})
 			},
 			&Event::PaymentFailed { ref payment_id, ref payment_hash, ref reason } => {
@@ -2311,11 +2319,13 @@ impl MaybeReadable for Event {
 						(2, payment_hash, option),
 						(4, path, required_vec),
 						(6, blinded_tail, option),
+						(8, hold_times, option),
 					});
 					Ok(Some(Event::PaymentPathSuccessful {
 						payment_id: payment_id.0.unwrap(),
 						payment_hash,
 						path: Path { hops: path, blinded_tail },
+						hold_times,
 					}))
 				};
 				f()

lightning/src/ln/async_payments_tests.rs

@@ -445,7 +445,7 @@ fn async_receive_flow_success() {
 	let args = PassAlongPathArgs::new(&nodes[0], route[0], amt_msat, payment_hash, ev)
 		.with_payment_preimage(keysend_preimage);
 	do_pass_along_path(args);
-	let res =
+	let (res, _) =
 		claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], route, keysend_preimage));
 	assert!(res.is_some());
 	assert_eq!(res, Some(PaidBolt12Invoice::StaticInvoice(static_invoice)));

lightning/src/ln/channel.rs

@@ -402,7 +402,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),
 }
 
@@ -466,6 +466,7 @@ enum HTLCUpdateAwaitingACK {
 	},
 	ClaimHTLC {
 		payment_preimage: PaymentPreimage,
+		attribution_data: Option<AttributionData>,
 		htlc_id: u64,
 	},
 	FailHTLC {
@@ -1180,7 +1181,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>,
@@ -2312,7 +2313,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>,
 
@@ -6214,7 +6215,7 @@ where
 		assert!(!self.context.channel_state.can_generate_new_commitment());
 		let mon_update_id = self.context.latest_monitor_update_id; // Forget the ChannelMonitor update
 		let fulfill_resp =
-			self.get_update_fulfill_htlc(htlc_id_arg, payment_preimage_arg, None, logger);
+			self.get_update_fulfill_htlc(htlc_id_arg, payment_preimage_arg, None, None, logger);
 		self.context.latest_monitor_update_id = mon_update_id;
 		if let UpdateFulfillFetch::NewClaim { update_blocked, .. } = fulfill_resp {
 			assert!(update_blocked); // The HTLC must have ended up in the holding cell.
@@ -6223,7 +6224,8 @@ where
 
 	fn get_update_fulfill_htlc<L: Deref>(
 		&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage,
-		payment_info: Option<PaymentClaimDetails>, logger: &L,
+		payment_info: Option<PaymentClaimDetails>, attribution_data: Option<AttributionData>,
+		logger: &L,
 	) -> UpdateFulfillFetch
 	where
 		L::Target: Logger,
@@ -6339,6 +6341,7 @@ where
 			self.context.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::ClaimHTLC {
 				payment_preimage: payment_preimage_arg,
 				htlc_id: htlc_id_arg,
+				attribution_data,
 			});
 			return UpdateFulfillFetch::NewClaim {
 				monitor_update,
@@ -6369,7 +6372,7 @@ where
 			);
 			htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(
 				payment_preimage_arg.clone(),
-				None,
+				attribution_data,
 			));
 		}
 
@@ -6378,13 +6381,20 @@ where
 
 	pub fn get_update_fulfill_htlc_and_commit<L: Deref>(
 		&mut self, htlc_id: u64, payment_preimage: PaymentPreimage,
-		payment_info: Option<PaymentClaimDetails>, logger: &L,
+		payment_info: Option<PaymentClaimDetails>, attribution_data: Option<AttributionData>,
+		logger: &L,
 	) -> UpdateFulfillCommitFetch
 	where
 		L::Target: Logger,
 	{
 		let release_cs_monitor = self.context.blocked_monitor_updates.is_empty();
-		match self.get_update_fulfill_htlc(htlc_id, payment_preimage, payment_info, logger) {
+		match self.get_update_fulfill_htlc(
+			htlc_id,
+			payment_preimage,
+			payment_info,
+			attribution_data,
+			logger,
+		) {
 			UpdateFulfillFetch::NewClaim {
 				mut monitor_update,
 				htlc_value_msat,
@@ -6718,7 +6728,7 @@ where
 
 	pub fn update_fulfill_htlc(
 		&mut self, msg: &msgs::UpdateFulfillHTLC,
-	) -> Result<(HTLCSource, u64, Option<u64>), ChannelError> {
+	) -> Result<(HTLCSource, u64, Option<u64>, Option<Duration>), ChannelError> {
 		if self.context.channel_state.is_remote_stfu_sent()
 			|| self.context.channel_state.is_quiescent()
 		{
@@ -6739,9 +6749,11 @@ where
 
 		self.mark_outbound_htlc_removed(
 			msg.htlc_id,
-			OutboundHTLCOutcome::Success(msg.payment_preimage, None),
+			OutboundHTLCOutcome::Success(msg.payment_preimage, msg.attribution_data.clone()),
 		)
-		.map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat))
+		.map(|htlc| {
+			(htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat, htlc.send_timestamp)
+		})
 	}
 
 	#[rustfmt::skip]
@@ -7277,7 +7289,11 @@ where
 						}
 						None
 					},
-					&HTLCUpdateAwaitingACK::ClaimHTLC { ref payment_preimage, htlc_id, .. } => {
+					&HTLCUpdateAwaitingACK::ClaimHTLC {
+						ref payment_preimage,
+						htlc_id,
+						ref attribution_data,
+					} => {
 						// If an HTLC claim was previously added to the holding cell (via
 						// `get_update_fulfill_htlc`, then generating the claim message itself must
 						// not fail - any in between attempts to claim the HTLC will have resulted
@@ -7290,8 +7306,13 @@ where
 						// We do not bother to track and include `payment_info` here, however.
 						let mut additional_monitor_update =
 							if let UpdateFulfillFetch::NewClaim { monitor_update, .. } = self
-								.get_update_fulfill_htlc(htlc_id, *payment_preimage, None, logger)
-							{
+								.get_update_fulfill_htlc(
+									htlc_id,
+									*payment_preimage,
+									None,
+									attribution_data.clone(),
+									logger,
+								) {
 								monitor_update
 							} else {
 								unreachable!()
@@ -7533,8 +7554,11 @@ where
 							});
 							revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
 						},
-						OutboundHTLCOutcome::Success(..) => {
-							finalized_claimed_htlcs.push(htlc.source.clone());
+						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;
 						},
@@ -8027,7 +8051,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;
@@ -12560,7 +12584,7 @@ where
 			Vec::with_capacity(holding_cell_htlc_update_count);
 		let mut holding_cell_blinding_points: Vec<Option<PublicKey>> =
 			Vec::with_capacity(holding_cell_htlc_update_count);
-		let mut holding_cell_failure_attribution_data: Vec<Option<&AttributionData>> =
+		let mut holding_cell_attribution_data: Vec<Option<&AttributionData>> =
 			Vec::with_capacity(holding_cell_htlc_update_count);
 		// Vec of (htlc_id, failure_code, sha256_of_onion)
 		let mut malformed_htlcs: Vec<(u64, u16, [u8; 32])> = Vec::new();
@@ -12586,19 +12610,25 @@ where
 					holding_cell_skimmed_fees.push(skimmed_fee_msat);
 					holding_cell_blinding_points.push(blinding_point);
 				},
-				&HTLCUpdateAwaitingACK::ClaimHTLC { ref payment_preimage, ref htlc_id } => {
+				&HTLCUpdateAwaitingACK::ClaimHTLC {
+					ref payment_preimage,
+					ref htlc_id,
+					ref attribution_data,
+				} => {
 					1u8.write(writer)?;
 					payment_preimage.write(writer)?;
 					htlc_id.write(writer)?;
+
+					// Store the attribution data for later writing.
+					holding_cell_attribution_data.push(attribution_data.as_ref());
 				},
 				&HTLCUpdateAwaitingACK::FailHTLC { ref htlc_id, ref err_packet } => {
 					2u8.write(writer)?;
 					htlc_id.write(writer)?;
 					err_packet.data.write(writer)?;
 
 					// Store the attribution data for later writing.
-					holding_cell_failure_attribution_data
-						.push(err_packet.attribution_data.as_ref());
+					holding_cell_attribution_data.push(err_packet.attribution_data.as_ref());
 				},
 				&HTLCUpdateAwaitingACK::FailMalformedHTLC {
 					htlc_id,
@@ -12615,7 +12645,7 @@ where
 
 					// Push 'None' attribution data for FailMalformedHTLC, because FailMalformedHTLC uses the same
 					// type 2 and is deserialized as a FailHTLC.
-					holding_cell_failure_attribution_data.push(None);
+					holding_cell_attribution_data.push(None);
 				},
 			}
 		}
@@ -12819,7 +12849,7 @@ where
 			(53, funding_tx_broadcast_safe_event_emitted, option), // Added in 0.0.124
 			(54, self.pending_funding, optional_vec), // Added in 0.2
 			(55, removed_htlc_attribution_data, optional_vec), // Added in 0.2
-			(57, holding_cell_failure_attribution_data, optional_vec), // Added in 0.2
+			(57, holding_cell_attribution_data, optional_vec), // Added in 0.2
 			(58, self.interactive_tx_signing_session, option), // Added in 0.2
 			(59, self.funding.minimum_depth_override, option), // Added in 0.2
 			(60, self.context.historical_scids, optional_vec), // Added in 0.2
@@ -12993,6 +13023,7 @@ where
 				1 => HTLCUpdateAwaitingACK::ClaimHTLC {
 					payment_preimage: Readable::read(reader)?,
 					htlc_id: Readable::read(reader)?,
+					attribution_data: None,
 				},
 				2 => HTLCUpdateAwaitingACK::FailHTLC {
 					htlc_id: Readable::read(reader)?,
@@ -13165,7 +13196,7 @@ where
 		let mut holding_cell_blinding_points_opt: Option<Vec<Option<PublicKey>>> = None;
 
 		let mut removed_htlc_attribution_data: Option<Vec<Option<AttributionData>>> = None;
-		let mut holding_cell_failure_attribution_data: Option<Vec<Option<AttributionData>>> = None;
+		let mut holding_cell_attribution_data: Option<Vec<Option<AttributionData>>> = None;
 
 		let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
 		let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
@@ -13218,7 +13249,7 @@ where
 			(53, funding_tx_broadcast_safe_event_emitted, option),
 			(54, pending_funding, optional_vec), // Added in 0.2
 			(55, removed_htlc_attribution_data, optional_vec),
-			(57, holding_cell_failure_attribution_data, optional_vec),
+			(57, holding_cell_attribution_data, optional_vec),
 			(58, interactive_tx_signing_session, option), // Added in 0.2
 			(59, minimum_depth_override, option), // Added in 0.2
 			(60, historical_scids, optional_vec), // Added in 0.2
@@ -13350,18 +13381,17 @@ where
 			}
 		}
 
-		if let Some(attribution_data_list) = holding_cell_failure_attribution_data {
+		if let Some(attribution_data_list) = holding_cell_attribution_data {
 			let mut holding_cell_failures =
-				holding_cell_htlc_updates.iter_mut().filter_map(|upd| {
-					if let HTLCUpdateAwaitingACK::FailHTLC {
+				holding_cell_htlc_updates.iter_mut().filter_map(|upd| match upd {
+					HTLCUpdateAwaitingACK::FailHTLC {
 						err_packet: OnionErrorPacket { ref mut attribution_data, .. },
 						..
-					} = upd
-					{
+					} => Some(attribution_data),
+					HTLCUpdateAwaitingACK::ClaimHTLC { attribution_data, .. } => {
 						Some(attribution_data)
-					} else {
-						None
-					}
+					},
+					_ => None,
 				});
 
 			for attribution_data in attribution_data_list {
@@ -13578,7 +13608,7 @@ where
 	}
 }
 
-fn duration_since_epoch() -> Option<Duration> {
+pub(crate) fn duration_since_epoch() -> Option<Duration> {
 	#[cfg(not(feature = "std"))]
 	let now = None;
 
@@ -13592,7 +13622,9 @@ fn duration_since_epoch() -> Option<Duration> {
 	now
 }
 
-fn get_hold_time(send_timestamp: Option<Duration>, now: Option<Duration>) -> Option<u32> {
+pub(crate) fn get_hold_time(
+	send_timestamp: Option<Duration>, now: Option<Duration>,
+) -> Option<u32> {
 	send_timestamp.and_then(|t| {
 		now.map(|now| {
 			let elapsed = now.saturating_sub(t).as_millis() / HOLD_TIME_UNIT_MILLIS;
@@ -14359,6 +14391,7 @@ mod tests {
 		let dummy_holding_cell_claim_htlc = HTLCUpdateAwaitingACK::ClaimHTLC {
 			payment_preimage: PaymentPreimage([42; 32]),
 			htlc_id: 0,
+			attribution_data: None,
 		};
 		let dummy_holding_cell_failed_htlc = |htlc_id| HTLCUpdateAwaitingACK::FailHTLC {
 			htlc_id,