Add hold times to event

Author: joostjager

lightning/src/events/mod.rs

@@ -1080,6 +1080,9 @@ 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 +1819,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 +2320,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/channel.rs

@@ -348,11 +348,11 @@ impl From<&OutboundHTLCState> for OutboundHTLCStateDetails {
 			// the state yet.
 			OutboundHTLCState::RemoteRemoved(_) =>
 				OutboundHTLCStateDetails::Committed,
-			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) =>
+			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _)) =>
 				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess,
 			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Failure(_)) =>
 				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFailure,
-			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) =>
+			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _)) =>
 				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess,
 			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Failure(_)) =>
 				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFailure,
@@ -387,9 +387,9 @@ impl OutboundHTLCState {
 	#[rustfmt::skip]
 	fn preimage(&self) -> Option<PaymentPreimage> {
 		match self {
-			OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage))
-			| OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage))
-			| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage)) => {
+			OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage, _))
+			| OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage, _))
+			| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage, _)) => {
 				Some(*preimage)
 			},
 			_ => None,
@@ -402,14 +402,14 @@ 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),
+	Success(PaymentPreimage, Option<AttributionData>),
 	Failure(HTLCFailReason),
 }
 
 impl<'a> Into<Option<&'a HTLCFailReason>> for &'a OutboundHTLCOutcome {
 	fn into(self) -> Option<&'a HTLCFailReason> {
 		match self {
-			OutboundHTLCOutcome::Success(_) => None,
+			OutboundHTLCOutcome::Success(__, _) => None,
 			OutboundHTLCOutcome::Failure(ref r) => Some(r),
 		}
 	}
@@ -1181,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>,
@@ -2313,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>,
 
@@ -4160,9 +4160,9 @@ where
 		// transaction).
 		let mut removed_outbound_total_msat = 0;
 		for ref htlc in self.pending_outbound_htlcs.iter() {
-			if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) = htlc.state {
+			if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _)) = htlc.state {
 				removed_outbound_total_msat += htlc.amount_msat;
-			} else if let OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) = htlc.state {
+			} else if let OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _)) = htlc.state {
 				removed_outbound_total_msat += htlc.amount_msat;
 			}
 		}
@@ -4425,9 +4425,9 @@ where
 
 		let mut removed_outbound_total_msat = 0;
 		for ref htlc in self.pending_outbound_htlcs.iter() {
-			if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) = htlc.state {
+			if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _)) = htlc.state {
 				removed_outbound_total_msat += htlc.amount_msat;
-			} else if let OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) = htlc.state {
+			} else if let OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _)) = htlc.state {
 				removed_outbound_total_msat += htlc.amount_msat;
 			}
 		}
@@ -6705,7 +6705,7 @@ where
 	fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, outcome: OutboundHTLCOutcome) -> Result<&OutboundHTLCOutput, ChannelError> {
 		for htlc in self.context.pending_outbound_htlcs.iter_mut() {
 			if htlc.htlc_id == htlc_id {
-				if let OutboundHTLCOutcome::Success(ref payment_preimage) = outcome {
+				if let OutboundHTLCOutcome::Success(ref payment_preimage, ..) = outcome {
 					let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array());
 					if payment_hash != htlc.payment_hash {
 						return Err(ChannelError::close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id)));
@@ -6749,7 +6749,7 @@ where
 
 		self.mark_outbound_htlc_removed(
 			msg.htlc_id,
-			OutboundHTLCOutcome::Success(msg.payment_preimage),
+			OutboundHTLCOutcome::Success(msg.payment_preimage, msg.attribution_data.clone()),
 		)
 		.map(|htlc| {
 			(htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat, htlc.send_timestamp)
@@ -7091,9 +7091,9 @@ where
 				log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
 					&htlc.payment_hash, &self.context.channel_id);
 				// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
-				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
+				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None);
 				mem::swap(outcome, &mut reason);
-				if let OutboundHTLCOutcome::Success(preimage) = reason {
+				if let OutboundHTLCOutcome::Success(preimage, _) = reason {
 					// If a user (a) receives an HTLC claim using LDK 0.0.104 or before, then (b)
 					// upgrades to LDK 0.0.114 or later before the HTLC is fully resolved, we could
 					// have a `Success(None)` reason. In this case we could forget some HTLC
@@ -7547,19 +7547,21 @@ where
 						&htlc.payment_hash
 					);
 					// We really want take() here, but, again, non-mut ref :(
-					if let OutboundHTLCOutcome::Failure(mut reason) = outcome.clone() {
-						if let (Some(timestamp), Some(now)) = (htlc.send_timestamp, now) {
-							let elapsed_millis = now.saturating_sub(timestamp).as_millis();
-							let elapsed_units = elapsed_millis / HOLD_TIME_UNIT_MILLIS;
-							let hold_time = u32::try_from(elapsed_units).unwrap_or(u32::MAX);
-							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) => {
+							if let (Some(timestamp), Some(now)) = (htlc.send_timestamp, now) {
+								let elapsed_millis = now.saturating_sub(timestamp).as_millis();
+								let elapsed_units = elapsed_millis / HOLD_TIME_UNIT_MILLIS;
+								let hold_time = u32::try_from(elapsed_units).unwrap_or(u32::MAX);
+								reason.set_hold_time(hold_time);
+							}
+							revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
+						},
+						OutboundHTLCOutcome::Success(_, attribution_data) => {
+							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 {
@@ -7643,7 +7645,7 @@ where
 				{
 					log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
 					// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
-					let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
+					let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None);
 					mem::swap(outcome, &mut reason);
 					htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
 					require_commitment = true;
@@ -8049,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;
@@ -10776,7 +10778,7 @@ where
 			if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
 				log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
 				// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
-				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
+				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None);
 				mem::swap(outcome, &mut reason);
 				htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
 			}
@@ -12520,10 +12522,10 @@ where
 							1u8.write(writer)?;
 							(hash, code).write(writer)?;
 						},
-						InboundHTLCRemovalReason::Fulfill(preimage, _) => {
-							// TODO: Persistence
+						InboundHTLCRemovalReason::Fulfill(preimage, attribution_data) => {
 							2u8.write(writer)?;
 							preimage.write(writer)?;
+							removed_htlc_attribution_data.push(&attribution_data);
 						},
 					}
 				},
@@ -12558,15 +12560,15 @@ where
 				},
 				&OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref outcome) => {
 					3u8.write(writer)?;
-					if let OutboundHTLCOutcome::Success(preimage) = outcome {
+					if let OutboundHTLCOutcome::Success(preimage, _) = outcome {
 						preimages.push(Some(preimage));
 					}
 					let reason: Option<&HTLCFailReason> = outcome.into();
 					reason.write(writer)?;
 				},
 				&OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) => {
 					4u8.write(writer)?;
-					if let OutboundHTLCOutcome::Success(preimage) = outcome {
+					if let OutboundHTLCOutcome::Success(preimage, _) = outcome {
 						preimages.push(Some(preimage));
 					}
 					let reason: Option<&HTLCFailReason> = outcome.into();
@@ -12972,7 +12974,7 @@ where
 						let outcome = match option {
 							Some(r) => OutboundHTLCOutcome::Failure(r),
 							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
-							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None),
 						};
 						OutboundHTLCState::RemoteRemoved(outcome)
 					},
@@ -12981,7 +12983,7 @@ where
 						let outcome = match option {
 							Some(r) => OutboundHTLCOutcome::Failure(r),
 							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
-							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None),
 						};
 						OutboundHTLCState::AwaitingRemoteRevokeToRemove(outcome)
 					},
@@ -12990,7 +12992,7 @@ where
 						let outcome = match option {
 							Some(r) => OutboundHTLCOutcome::Failure(r),
 							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
-							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None),
 						};
 						OutboundHTLCState::AwaitingRemovedRemoteRevoke(outcome)
 					},
@@ -13260,6 +13262,7 @@ where
 			match &mut htlc.state {
 				OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(
 					ref mut preimage,
+					..,
 				)) => {
 					// This variant was initialized like this further above
 					debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32]));
@@ -13268,6 +13271,7 @@ where
 				},
 				OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(
 					ref mut preimage,
+					..,
 				)) => {
 					// This variant was initialized like this further above
 					debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32]));

lightning/src/ln/channelmanager.rs

@@ -56,7 +56,6 @@ use crate::events::{
 	InboundChannelFunds, PaymentFailureReason, ReplayEvent,
 };
 use crate::events::{FundingInfo, PaidBolt12Invoice};
-use crate::ln::onion_utils::process_onion_success;
 // Since this struct is returned in `list_channels` methods, expose it here in case users want to
 // construct one themselves.
 use crate::ln::channel::{
@@ -79,7 +78,7 @@ use crate::ln::onion_payment::{
 };
 use crate::ln::onion_utils::AttributionData;
 use crate::ln::onion_utils::{self};
-use crate::ln::onion_utils::{HTLCFailReason, LocalHTLCFailureReason};
+use crate::ln::onion_utils::{process_onion_success, HTLCFailReason, LocalHTLCFailureReason};
 use crate::ln::our_peer_storage::EncryptedOurPeerStorage;
 #[cfg(test)]
 use crate::ln::outbound_payment;
@@ -7972,8 +7971,30 @@ 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| {
+						process_onion_success(
+							&self.secp_ctx,
+							&self.logger,
+							path,
+							session_priv,
+							attribution_data,
+						)
+					});
+
+					Some((source, hold_times))
+				} else {
+					None
+				}
+			})
+			.collect();
+
+		self.pending_outbound_payments.finalize_claims(hold_times, &self.pending_events);
 	}
 
 	#[rustfmt::skip]
@@ -7985,16 +8006,10 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 	) {
 		match source {
 			HTLCSource::OutboundRoute { session_priv, payment_id, path, bolt12_invoice, .. } => {
-				// Extract the hold times for this fulfilled HTLC, if available.
-				if let Some(attribution_data) = attribution_data {
-					let _ = process_onion_success(&self.secp_ctx, &self.logger, &path,
-						&session_priv, attribution_data.clone());
-				}
-
 				debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 					"We don't support claim_htlc claims during startup - monitors may not be available yet");
 				debug_assert_eq!(next_channel_counterparty_node_id, path.hops[0].pubkey);
-				let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
+	 			let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 					channel_funding_outpoint: next_channel_outpoint, channel_id: next_channel_id,
 					counterparty_node_id: path.hops[0].pubkey,
 				};
@@ -16570,15 +16585,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/onion_utils.rs

@@ -1494,8 +1494,6 @@ where
 		if let Some(hold_time) = hold_time {
 			hold_times.push(hold_time);
 
-			log_debug!(logger, "Htlc hold time at pos {}: {} ms", route_hop_idx, hold_time);
-
 			// Shift attribution data to prepare for processing the next hop.
 			attribution_data.shift_left();
 		} else {