Add hold times to update_fulfill_htlc

Author: joostjager

lightning/src/ln/channel.rs

@@ -138,7 +138,7 @@ enum FeeUpdateState {
 enum InboundHTLCRemovalReason {
 	FailRelay(msgs::OnionErrorPacket),
 	FailMalformed(([u8; 32], u16)),
-	Fulfill(PaymentPreimage),
+	Fulfill(PaymentPreimage, Option<AttributionData>),
 }
 
 /// Represents the resolution status of an inbound HTLC.
@@ -234,7 +234,7 @@ impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> {
 				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail),
 			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed(_)) =>
 				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail),
-			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) =>
+			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_, _)) =>
 				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill),
 		}
 	}
@@ -266,7 +266,7 @@ impl InboundHTLCState {
 
 	fn preimage(&self) -> Option<PaymentPreimage> {
 		match self {
-			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => {
+			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage, _)) => {
 				Some(*preimage)
 			},
 			_ => None,
@@ -466,6 +466,7 @@ enum HTLCUpdateAwaitingACK {
 	},
 	ClaimHTLC {
 		payment_preimage: PaymentPreimage,
+		attribution_data: AttributionData,
 		htlc_id: u64,
 	},
 	FailHTLC {
@@ -6212,7 +6213,7 @@ where
 		// (see equivalent if condition there).
 		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);
+		let fulfill_resp = self.get_update_fulfill_htlc(htlc_id_arg, payment_preimage_arg, None, AttributionData::new(), 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.
@@ -6222,7 +6223,7 @@ where
 	#[rustfmt::skip]
 	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: AttributionData, logger: &L,
 	) -> UpdateFulfillFetch where L::Target: Logger {
 		// Either ChannelReady got set (which means it won't be unset) or there is no way any
 		// caller thought we could have something claimed (cause we wouldn't have accepted in an
@@ -6246,7 +6247,7 @@ where
 				match htlc.state {
 					InboundHTLCState::Committed => {},
 					InboundHTLCState::LocalRemoved(ref reason) => {
-						if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
+						if let &InboundHTLCRemovalReason::Fulfill(_, _) = reason {
 						} else {
 							log_warn!(logger, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", &htlc.payment_hash, &self.context.channel_id());
 							debug_assert!(false, "Tried to fulfill an HTLC that was already failed");
@@ -6312,6 +6313,7 @@ where
 			log_trace!(logger, "Adding HTLC claim to holding_cell in channel {}! Current state: {}", &self.context.channel_id(), self.context.channel_state.to_u32());
 			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, htlc_value_msat, update_blocked: true };
 		}
@@ -6324,7 +6326,7 @@ where
 				return UpdateFulfillFetch::NewClaim { monitor_update, htlc_value_msat, update_blocked: true };
 			}
 			log_trace!(logger, "Upgrading HTLC {} to LocalRemoved with a Fulfill in channel {}!", &htlc.payment_hash, &self.context.channel_id);
-			htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(payment_preimage_arg.clone()));
+			htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(payment_preimage_arg.clone(), Some(attribution_data)));
 		}
 
 		UpdateFulfillFetch::NewClaim {
@@ -6337,10 +6339,10 @@ where
 	#[rustfmt::skip]
 	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: 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, update_blocked } => {
 				// Even if we aren't supposed to let new monitor updates with commitment state
 				// updates run, we still need to push the preimage ChannelMonitorUpdateStep no
@@ -6658,7 +6660,7 @@ where
 	}
 
 	#[rustfmt::skip]
-	pub fn update_fulfill_htlc(&mut self, msg: &msgs::UpdateFulfillHTLC) -> Result<(HTLCSource, u64, Option<u64>), ChannelError> {
+	pub fn update_fulfill_htlc(&mut self, msg: &msgs::UpdateFulfillHTLC) -> Result<(HTLCSource, u64, Option<u64>, Option<Duration>), ChannelError> {
 		if self.context.channel_state.is_remote_stfu_sent() || self.context.channel_state.is_quiescent() {
 			return Err(ChannelError::WarnAndDisconnect("Got fulfill HTLC message while quiescent".to_owned()));
 		}
@@ -6669,7 +6671,7 @@ where
 			return Err(ChannelError::close("Peer sent update_fulfill_htlc when we needed a channel_reestablish".to_owned()));
 		}
 
-		self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Success(msg.payment_preimage)).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat))
+		self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Success(msg.payment_preimage)).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat, htlc.send_timestamp))
 	}
 
 	#[rustfmt::skip]
@@ -7160,7 +7162,7 @@ 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
@@ -7173,7 +7175,7 @@ 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)
+								self.get_update_fulfill_htlc(htlc_id, *payment_preimage, None, attribution_data.clone(), logger)
 							{ monitor_update } else { unreachable!() };
 						update_fulfill_count += 1;
 						monitor_update.updates.append(&mut additional_monitor_update.updates);
@@ -7376,7 +7378,7 @@ where
 			pending_inbound_htlcs.retain(|htlc| {
 				if let &InboundHTLCState::LocalRemoved(ref reason) = &htlc.state {
 					log_trace!(logger, " ...removing inbound LocalRemoved {}", &htlc.payment_hash);
-					if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
+					if let &InboundHTLCRemovalReason::Fulfill(_, _) = reason {
 						value_to_self_msat_diff += htlc.amount_msat as i64;
 					}
 					*expecting_peer_commitment_signed = true;
@@ -8242,12 +8244,12 @@ where
 							failure_code: failure_code.clone(),
 						});
 					},
-					&InboundHTLCRemovalReason::Fulfill(ref payment_preimage) => {
+					&InboundHTLCRemovalReason::Fulfill(ref payment_preimage, ref attribution_data) => {
 						update_fulfill_htlcs.push(msgs::UpdateFulfillHTLC {
 							channel_id: self.context.channel_id(),
 							htlc_id: htlc.htlc_id,
 							payment_preimage: payment_preimage.clone(),
-							attribution_data: None,
+							attribution_data: attribution_data.clone(),
 						});
 					},
 				}
@@ -12339,7 +12341,7 @@ where
 							1u8.write(writer)?;
 							(hash, code).write(writer)?;
 						},
-						InboundHTLCRemovalReason::Fulfill(preimage) => {
+						InboundHTLCRemovalReason::Fulfill(preimage, _) => { // TODO: Persistence
 							2u8.write(writer)?;
 							preimage.write(writer)?;
 						},
@@ -12418,7 +12420,7 @@ 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, .. } => {
 					1u8.write(writer)?;
 					payment_preimage.write(writer)?;
 					htlc_id.write(writer)?;
@@ -12705,7 +12707,7 @@ where
 								attribution_data: None,
 							}),
 							1 => InboundHTLCRemovalReason::FailMalformed(Readable::read(reader)?),
-							2 => InboundHTLCRemovalReason::Fulfill(Readable::read(reader)?),
+							2 => InboundHTLCRemovalReason::Fulfill(Readable::read(reader)?, None), // TODO: Persistence
 							_ => return Err(DecodeError::InvalidValue),
 						};
 						InboundHTLCState::LocalRemoved(reason)
@@ -12778,6 +12780,7 @@ where
 				1 => HTLCUpdateAwaitingACK::ClaimHTLC {
 					payment_preimage: Readable::read(reader)?,
 					htlc_id: Readable::read(reader)?,
+					attribution_data: AttributionData::new(),	// TODO: Persistence
 				},
 				2 => HTLCUpdateAwaitingACK::FailHTLC {
 					htlc_id: Readable::read(reader)?,
@@ -13283,7 +13286,7 @@ where
 	}
 }
 
-fn duration_since_epoch() -> Option<Duration> {
+pub(crate) fn duration_since_epoch() -> Option<Duration> {
 	#[cfg(not(feature = "std"))]
 	let now = None;
 
@@ -14004,6 +14007,7 @@ mod tests {
 		let dummy_holding_cell_claim_htlc = HTLCUpdateAwaitingACK::ClaimHTLC {
 			payment_preimage: PaymentPreimage([42; 32]),
 			htlc_id: 0,
+			attribution_data: AttributionData::new(),
 		};
 		let dummy_holding_cell_failed_htlc = |htlc_id| HTLCUpdateAwaitingACK::FailHTLC {
 			htlc_id, err_packet: msgs::OnionErrorPacket { data: vec![42], attribution_data: Some(AttributionData::new()) }

lightning/src/ln/channelmanager.rs

@@ -56,14 +56,15 @@ 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::PendingV2Channel;
 use crate::ln::channel::{
 	self, Channel, ChannelError, ChannelUpdateStatus, FundedChannel, InboundV1Channel,
 	OutboundV1Channel, ReconnectionMsg, ShutdownResult, UpdateFulfillCommitFetch,
 	WithChannelContext,
 };
+use crate::ln::channel::{duration_since_epoch, PendingV2Channel};
 use crate::ln::channel_state::ChannelDetails;
 use crate::ln::inbound_payment;
 use crate::ln::msgs;
@@ -76,6 +77,7 @@ use crate::ln::onion_payment::{
 	decode_incoming_update_add_htlc_onion, invalid_payment_err_data, HopConnector, InboundHTLCErr,
 	NextPacketDetails,
 };
+use crate::ln::onion_utils::AttributionData;
 use crate::ln::onion_utils::{self};
 use crate::ln::onion_utils::{HTLCFailReason, LocalHTLCFailureReason};
 use crate::ln::our_peer_storage::EncryptedOurPeerStorage;
@@ -7625,8 +7627,16 @@ where
 						pending_claim: PendingMPPClaimPointer(Arc::clone(pending_claim)),
 					}
 				});
+
+				// Create new attribution data as the final hop. Always report a zero hold time, because reporting a
+				// non-zero value will not make a difference in the penalty that may be applied by the sender.
+				let mut attribution_data = AttributionData::new();
+				attribution_data.update(&[], &htlc.prev_hop.incoming_packet_shared_secret, 0);
+				attribution_data.crypt(&htlc.prev_hop.incoming_packet_shared_secret);
+
 				self.claim_funds_from_hop(
 					htlc.prev_hop, payment_preimage, payment_info.clone(),
+					attribution_data,
 					|_, definitely_duplicate| {
 						debug_assert!(!definitely_duplicate, "We shouldn't claim duplicatively from a payment");
 						(Some(MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, pending_mpp_claim: this_mpp_claim }), raa_blocker)
@@ -7658,7 +7668,8 @@ where
 		) -> (Option<MonitorUpdateCompletionAction>, Option<RAAMonitorUpdateBlockingAction>),
 	>(
 		&self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage,
-		payment_info: Option<PaymentClaimDetails>, completion_action: ComplFunc,
+		payment_info: Option<PaymentClaimDetails>, attribution_data: AttributionData,
+		completion_action: ComplFunc,
 	) {
 		let counterparty_node_id = prev_hop.counterparty_node_id.or_else(|| {
 			let short_to_chan_info = self.short_to_chan_info.read().unwrap();
@@ -7671,15 +7682,21 @@ where
 			channel_id: prev_hop.channel_id,
 			htlc_id: prev_hop.htlc_id,
 		};
-		self.claim_mpp_part(htlc_source, payment_preimage, payment_info, completion_action)
+		self.claim_mpp_part(
+			htlc_source,
+			payment_preimage,
+			payment_info,
+			attribution_data,
+			completion_action,
+		)
 	}
 
 	#[rustfmt::skip]
 	fn claim_mpp_part<
 		ComplFunc: FnOnce(Option<u64>, bool) -> (Option<MonitorUpdateCompletionAction>, Option<RAAMonitorUpdateBlockingAction>)
 	>(
 		&self, prev_hop: HTLCClaimSource, payment_preimage: PaymentPreimage,
-		payment_info: Option<PaymentClaimDetails>, completion_action: ComplFunc,
+		payment_info: Option<PaymentClaimDetails>, attribution_data: AttributionData, completion_action: ComplFunc,
 	) {
 		//TODO: Delay the claimed_funds relaying just like we do outbound relay!
 
@@ -7712,7 +7729,7 @@ where
 			if let hash_map::Entry::Occupied(mut chan_entry) = peer_state.channel_by_id.entry(chan_id) {
 				if let Some(chan) = chan_entry.get_mut().as_funded_mut() {
 					let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-					let fulfill_res = chan.get_update_fulfill_htlc_and_commit(prev_hop.htlc_id, payment_preimage, payment_info, &&logger);
+					let fulfill_res = chan.get_update_fulfill_htlc_and_commit(prev_hop.htlc_id, payment_preimage, payment_info, attribution_data, &&logger);
 
 					match fulfill_res {
 						UpdateFulfillCommitFetch::NewClaim { htlc_value_msat, monitor_update } => {
@@ -7865,9 +7882,16 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		forwarded_htlc_value_msat: Option<u64>, skimmed_fee_msat: Option<u64>, from_onchain: bool,
 		startup_replay: bool, next_channel_counterparty_node_id: PublicKey,
 		next_channel_outpoint: OutPoint, next_channel_id: ChannelId, next_user_channel_id: Option<u128>,
+		attribution_data: Option<&AttributionData>, send_timestamp: Option<Duration>,
 	) {
 		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);
@@ -7884,7 +7908,31 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				let prev_user_channel_id = hop_data.user_channel_id;
 				let prev_node_id = hop_data.counterparty_node_id;
 				let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
-				self.claim_funds_from_hop(hop_data, payment_preimage, None,
+
+				// If attribution data was received from downstream, we shift it and get it ready for adding our hold
+				// time.
+				let mut attribution_data = attribution_data
+					.map_or(AttributionData::new(), |attribution_data| {
+						let mut attribution_data = attribution_data.clone();
+
+						attribution_data.shift_right();
+
+						attribution_data
+					});
+
+				// Obtain hold time, if available.
+				let now = duration_since_epoch();
+				let hold_time = if let (Some(timestamp), Some(now)) = (send_timestamp, now) {
+					u32::try_from(now.saturating_sub(timestamp).as_millis()).unwrap_or(u32::MAX)
+				} else {
+					0
+				};
+
+				// Finish attribution data by adding our hold time and crypting it.
+				attribution_data.update(&[], &hop_data.incoming_packet_shared_secret, hold_time);
+				attribution_data.crypt(&hop_data.incoming_packet_shared_secret);
+
+				self.claim_funds_from_hop(hop_data, payment_preimage, None, attribution_data,
 					|htlc_claim_value_msat, definitely_duplicate| {
 						let chan_to_release = Some(EventUnblockedChannel {
 							counterparty_node_id: next_channel_counterparty_node_id,
@@ -9428,7 +9476,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 	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 (htlc_source, forwarded_htlc_value, skimmed_fee_msat, send_timestamp) = {
 			let per_peer_state = self.per_peer_state.read().unwrap();
 			let peer_state_mutex = per_peer_state.get(counterparty_node_id)
 				.ok_or_else(|| {
@@ -9469,7 +9517,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		};
 		self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(),
 			Some(forwarded_htlc_value), skimmed_fee_msat, false, false, *counterparty_node_id,
-			funding_txo, msg.channel_id, Some(next_user_channel_id),
+			funding_txo, msg.channel_id, Some(next_user_channel_id), msg.attribution_data.as_ref(),
+			send_timestamp,
 		);
 
 		Ok(())
@@ -10283,10 +10332,13 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						let logger = WithContext::from(&self.logger, Some(counterparty_node_id), Some(channel_id), Some(htlc_update.payment_hash));
 						if let Some(preimage) = htlc_update.payment_preimage {
 							log_trace!(logger, "Claiming HTLC with preimage {} from our monitor", preimage);
+							// Claim the funds from the previous hop, if there is one. Because this is in response to a
+							// chain event, no attribution data is available.
 							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, None,
+								None, None,
 							);
 						} else {
 							log_trace!(logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
@@ -15668,8 +15720,11 @@ where
 							// Note that we don't need to pass the `payment_info` here - its
 							// already (clearly) durably on disk in the `ChannelMonitor` so there's
 							// no need to worry about getting it into others.
+							//
+							// We don't encode any attribution data, because the required onion shared secret isn't
+							// available here.
 							channel_manager.claim_mpp_part(
-								part.into(), payment_preimage, None,
+								part.into(), payment_preimage, None, AttributionData::new(),
 								|_, _|
 									(Some(MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, pending_mpp_claim }), pending_claim_ptr)
 							);
@@ -15773,9 +15828,10 @@ where
 			// We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 			// 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.
+			// The same holds for attribution data. We don't have any, so we pass an empty one.
 			channel_manager.claim_funds_internal(source, preimage, Some(downstream_value), None,
 				downstream_closed, true, downstream_node_id, downstream_funding,
-				downstream_channel_id, None
+				downstream_channel_id, None, None, None,
 			);
 		}