MonitorEvents: HTLC failure reason and skimmed fee

We're moving towards having the ChannelMonitor be responsible for resolving
HTLCs, via MonitorEvents, so we need to start surfacing more information in
monitor events.

Here we start persisting/surfacing a specific HTLC failure reason and the
skimmed fee in MonitorEvent::HTLCEvents, which is useful when generating
and handling monitor events for off-chain claims/fails.

The skimmed fee for forwards is already put to use in this commit for
generating correct PaymentForwarded events. The failure reason will be used in
upcoming commits. We add the failure reason now to not have to change what
we're serializing to disk later.

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))), // Added in 0.4
+	(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
@@ -3910,7 +3963,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					// startup until it is explicitly acked.
 					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,
@@ -4647,7 +4703,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,
@@ -5985,7 +6041,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,
@@ -6096,7 +6152,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,
@@ -6514,7 +6570,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,
@@ -6539,7 +6595,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};
@@ -13771,60 +13771,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. In the future we can
+								// store attribution data in the `ChannelMonitor` and provide it here.
+								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. In the future we can
-							// store attribution data in the `ChannelMonitor` and provide it here.
-							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())
 	}