MonitorEvents: HTLC failure reason and skimmed fee

XXX

Author: valentinewallace

lightning/src/chain/channelmonitor.rs

@@ -56,6 +56,7 @@ use crate::ln::channel_keys::{
 };
 use crate::ln::channelmanager::{HTLCSource, PaymentClaimDetails, SentHTLCId};
 use crate::ln::msgs::DecodeError;
+use crate::ln::onion_utils::{HTLCFailReason, LocalHTLCFailureReason};
 use crate::ln::types::ChannelId;
 use crate::sign::{
 	ecdsa::EcdsaChannelSigner, ChannelDerivationParameters, DelayedPaymentOutputDescriptor,
@@ -252,24 +253,76 @@ impl_writeable_tlv_based_enum_upgradable_legacy!(MonitorEvent,
 	// 6 was `UpdateFailed` until LDK 0.0.117
 );
 
+/// The resolution of an outbound HTLC — either claimed with a preimage or failed back. Used in
+/// [`MonitorEvent::HTLCEvent`]s to provide resolution data to the `ChannelManager`.
+#[derive(Clone, PartialEq, Eq, Debug)]
+pub(crate) enum OutboundHTLCResolution {
+	Claimed {
+		preimage: PaymentPreimage,
+		skimmed_fee_msat: Option<u64>,
+	},
+	/// The failure resolution may come from on-chain in the [`ChannelMonitor`] or off-chain from the
+	/// `ChannelManager`, such as from an outbound edge to provide the manager with a resolution for
+	/// the inbound edge.
+	Failed {
+		reason: HTLCFailReason,
+	},
+}
+
+impl OutboundHTLCResolution {
+	/// Returns an on-chain timeout failure resolution.
+	pub fn on_chain_timeout() -> Self {
+		OutboundHTLCResolution::Failed {
+			reason: HTLCFailReason::from_failure_code(LocalHTLCFailureReason::OnChainTimeout),
+		}
+	}
+}
+
+impl_writeable_tlv_based_enum!(OutboundHTLCResolution,
+	(1, Claimed) => {
+		(1, preimage, required),
+		(3, skimmed_fee_msat, option),
+	},
+	(3, Failed) => {
+		(1, reason, required),
+	},
+);
+
 /// Simple structure sent back by `chain::Watch` when an HTLC from a forward channel is detected on
 /// chain. Used to update the corresponding HTLC in the backward channel. Failing to pass the
 /// preimage claim backward will lead to loss of funds.
 #[derive(Clone, PartialEq, Eq)]
 pub struct HTLCUpdate {
 	pub(crate) payment_hash: PaymentHash,
-	pub(crate) payment_preimage: Option<PaymentPreimage>,
 	pub(crate) source: HTLCSource,
 	pub(crate) htlc_value_msat: Option<u64>,
 	pub(crate) user_channel_id: Option<u128>,
+	pub(crate) resolution: OutboundHTLCResolution,
 }
 impl_writeable_tlv_based!(HTLCUpdate, {
 	(0, payment_hash, required),
 	(1, htlc_value_satoshis, (legacy, u64, |_| Ok(()), |us: &HTLCUpdate| us.htlc_value_msat.map(|v| v / 1000))),
 	(2, source, required),
-	(4, payment_preimage, option),
+	(4, _legacy_payment_preimage, (legacy, Option<PaymentPreimage>,
+		|v: Option<&Option<PaymentPreimage>>| {
+			// Only use the legacy preimage if the new `resolution` TLV (9) wasn't read,
+			// otherwise we'd clobber it (losing e.g. skimmed_fee_msat).
+			if resolution.0.is_none() {
+				if let Some(Some(preimage)) = v {
+					resolution = crate::util::ser::RequiredWrapper(Some(
+						OutboundHTLCResolution::Claimed { preimage: *preimage, skimmed_fee_msat: None }
+					));
+				}
+			}
+			Ok(())
+		},
+		|us: &HTLCUpdate| match &us.resolution {
+			OutboundHTLCResolution::Claimed { preimage, .. } => Some(Some(*preimage)),
+			_ => None,
+		})),
 	(5, user_channel_id, option),
 	(7, htlc_value_msat, (default_value, htlc_value_satoshis.map(|v: u64| v * 1000))),
+	(9, resolution, (default_value, OutboundHTLCResolution::on_chain_timeout())),
 });
 
 /// If an output goes from claimable only by us to claimable by us or our counterparty within this
@@ -3908,7 +3961,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				if let Some((htlc, source)) = htlc_opt {
 					self.push_monitor_event(MonitorEvent::HTLCEvent(HTLCUpdate {
 						payment_hash: htlc.payment_hash,
-						payment_preimage: Some(claim.preimage),
+						resolution: OutboundHTLCResolution::Claimed {
+							preimage: claim.preimage,
+							skimmed_fee_msat: claim.skimmed_fee_msat,
+						},
 						source: *source.clone(),
 						htlc_value_msat: Some(htlc.amount_msat),
 						user_channel_id: self.user_channel_id,
@@ -4645,7 +4701,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					&mut self.pending_monitor_events,
 					MonitorEvent::HTLCEvent(HTLCUpdate {
 						payment_hash,
-						payment_preimage: None,
+						resolution: OutboundHTLCResolution::on_chain_timeout(),
 						source: source.clone(),
 						htlc_value_msat: Some(amount_msat),
 						user_channel_id: self.user_channel_id,
@@ -5983,7 +6039,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						&payment_hash, entry.txid);
 					self.push_monitor_event(MonitorEvent::HTLCEvent(HTLCUpdate {
 						payment_hash,
-						payment_preimage: None,
+						resolution: OutboundHTLCResolution::on_chain_timeout(),
 						source,
 						htlc_value_msat: htlc_value_satoshis.map(|v| v * 1000),
 						user_channel_id: self.user_channel_id,
@@ -6094,7 +6150,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						expires soon at {}", log_bytes!(htlc.payment_hash.0), inbound_htlc_expiry);
 					push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate {
 						source: source.clone(),
-						payment_preimage: None,
+						resolution: OutboundHTLCResolution::on_chain_timeout(),
 						payment_hash: htlc.payment_hash,
 						htlc_value_msat: Some(htlc.amount_msat),
 						user_channel_id: self.user_channel_id,
@@ -6512,7 +6568,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage);
 						push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate {
 							source,
-							payment_preimage: Some(payment_preimage),
+							resolution: OutboundHTLCResolution::Claimed { preimage: payment_preimage, skimmed_fee_msat: None },
 							payment_hash,
 							htlc_value_msat: Some(amount_msat),
 							user_channel_id: self.user_channel_id,
@@ -6537,7 +6593,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage);
 						push_monitor_event(&mut self.pending_monitor_events, MonitorEvent::HTLCEvent(HTLCUpdate {
 							source,
-							payment_preimage: Some(payment_preimage),
+							resolution: OutboundHTLCResolution::Claimed { preimage: payment_preimage, skimmed_fee_msat: None },
 							payment_hash,
 							htlc_value_msat: Some(amount_msat),
 							user_channel_id: self.user_channel_id,

lightning/src/ln/channelmanager.rs

@@ -45,8 +45,8 @@ use crate::chain::chaininterface::{
 use crate::chain::chainmonitor::MonitorEventSource;
 use crate::chain::channelmonitor::{
 	ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, MonitorEvent,
-	WithChannelMonitor, ANTI_REORG_DELAY, CLTV_CLAIM_BUFFER, HTLC_FAIL_BACK_BUFFER,
-	LATENCY_GRACE_PERIOD_BLOCKS, MAX_BLOCKS_FOR_CONF,
+	OutboundHTLCResolution, WithChannelMonitor, ANTI_REORG_DELAY, CLTV_CLAIM_BUFFER,
+	HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, MAX_BLOCKS_FOR_CONF,
 };
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Confirm, Watch};
@@ -13746,60 +13746,62 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 							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
-							);
-							let from_onchain = self
-								.per_peer_state
-								.read()
-								.unwrap()
-								.get(&counterparty_node_id)
-								.map_or(true, |peer_state_mtx| {
-									!peer_state_mtx
-										.lock()
-										.unwrap()
-										.channel_by_id
-										.contains_key(&channel_id)
+						match htlc_update.resolution {
+							OutboundHTLCResolution::Claimed { preimage, skimmed_fee_msat } => {
+								log_trace!(
+									logger,
+									"Claiming HTLC with preimage {} from our monitor",
+									preimage
+								);
+								let from_onchain = self
+									.per_peer_state
+									.read()
+									.unwrap()
+									.get(&counterparty_node_id)
+									.map_or(true, |peer_state_mtx| {
+										!peer_state_mtx
+											.lock()
+											.unwrap()
+											.channel_by_id
+											.contains_key(&channel_id)
+									});
+								// 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_msat,
+									skimmed_fee_msat,
+									from_onchain,
+									counterparty_node_id,
+									funding_outpoint,
+									channel_id,
+									htlc_update.user_channel_id,
+									None,
+									None,
+									Some(event_id),
+								);
+							},
+							OutboundHTLCResolution::Failed { reason } => {
+								log_trace!(logger, "Failing HTLC from our monitor");
+								let failure_type = htlc_update
+									.source
+									.failure_type(counterparty_node_id, channel_id);
+								let completion_update = Some(PaymentCompleteUpdate {
+									counterparty_node_id,
+									channel_funding_outpoint: funding_outpoint,
+									channel_id,
+									htlc_id: SentHTLCId::from_source(&htlc_update.source),
 								});
-							// 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_msat,
-								None,
-								from_onchain,
-								counterparty_node_id,
-								funding_outpoint,
-								channel_id,
-								htlc_update.user_channel_id,
-								None,
-								None,
-								Some(event_id),
-							);
-						} else {
-							log_trace!(logger, "Failing HTLC from our monitor");
-							let failure_reason = LocalHTLCFailureReason::OnChainTimeout;
-							let failure_type =
-								htlc_update.source.failure_type(counterparty_node_id, channel_id);
-							let reason = HTLCFailReason::from_failure_code(failure_reason);
-							let completion_update = Some(PaymentCompleteUpdate {
-								counterparty_node_id,
-								channel_funding_outpoint: funding_outpoint,
-								channel_id,
-								htlc_id: SentHTLCId::from_source(&htlc_update.source),
-							});
-							self.fail_htlc_backwards_internal(
-								&htlc_update.source,
-								&htlc_update.payment_hash,
-								&reason,
-								failure_type,
-								completion_update,
-							);
-							self.chain_monitor.ack_monitor_event(monitor_event_source);
+								self.fail_htlc_backwards_internal(
+									&htlc_update.source,
+									&htlc_update.payment_hash,
+									&reason,
+									failure_type,
+									completion_update,
+								);
+								self.chain_monitor.ack_monitor_event(monitor_event_source);
+							},
 						}
 					},
 					MonitorEvent::HolderForceClosed(_)

lightning/src/ln/onion_utils.rs

@@ -1908,11 +1908,11 @@ impl From<&HTLCFailReason> for HTLCHandlingFailureReason {
 }
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
-#[cfg_attr(test, derive(PartialEq))]
-pub(super) struct HTLCFailReason(HTLCFailReasonRepr);
+#[derive(PartialEq, Eq)]
+pub(crate) struct HTLCFailReason(HTLCFailReasonRepr);
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
-#[cfg_attr(test, derive(PartialEq))]
+#[derive(PartialEq, Eq)]
 enum HTLCFailReasonRepr {
 	LightningError { err: msgs::OnionErrorPacket, hold_time: Option<u32> },
 	Reason { data: Vec<u8>, failure_reason: LocalHTLCFailureReason },
@@ -2071,7 +2071,7 @@ impl HTLCFailReason {
 		Self(HTLCFailReasonRepr::Reason { data, failure_reason })
 	}
 
-	pub(super) fn from_failure_code(failure_reason: LocalHTLCFailureReason) -> Self {
+	pub(crate) fn from_failure_code(failure_reason: LocalHTLCFailureReason) -> Self {
 		Self::reason(failure_reason, Vec::new())
 	}