Add a PaymentId for inbound payments

We expect our users to have fully idempotent `Event` handling as we
may replay events on restart for one of a number of reasons. This
isn't a big deal as long as all our events have some kind of
identifier users can use to check if the `Event` has already been
handled.

For outbound payments, this is the `PaymentId` they provide in the
send methods, however for inbound payments we don't have a great
option.

`PaymentHash` largely suffices - users can simply always claim in
response to a `PaymentClaimable` of sufficient value and treat a
`PaymentClaimed` event as duplicate any time they see a second one
for the same `PaymentHash`. This mostly works, but may result in
accepting duplicative payments if someone (incorrectly) pays twice
for the same `PaymentHash`.

Users could also fail for duplicative `PaymentClaimable` events of
the same `PaymentHash`, but doing so may result in spuriously
failing a payment if the `PaymentClaimable` event is a replay and
they never saw a corresponding `PaymentClaimed` event.

While none of this will result in spuriously thinking they've been
paid when they have not, it does result in some pretty awkward
semantics which we'd rather avoid our users having to deal with.

Instead, here, we add a new `PaymentId` which is simply an HMAC of
the HTLCs (as Channel ID, HTLC ID pairs) which were included in the
payment.

Author: TheBlueMatt

lightning/src/events/mod.rs

@@ -748,6 +748,14 @@ pub enum Event {
 		///
 		/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds
 		claim_deadline: Option<u32>,
+		/// A unique ID describing this payment (derived from the list of HTLCs in the payment).
+		///
+		/// Payers may pay for the same [`PaymentHash`] multiple times (though this is unsafe and
+		/// an intermediary node may steal the funds). Thus, in order to accurately track when
+		/// payments are received and claimed, you should use this identifier.
+		///
+		/// Only filled in for payments received on LDK versions 0.1 and higher.
+		payment_id: Option<PaymentId>,
 	},
 	/// Indicates a payment has been claimed and we've received money!
 	///
@@ -795,6 +803,14 @@ pub enum Event {
 		///
 		/// Payments received on LDK versions prior to 0.0.124 will have this field unset.
 		onion_fields: Option<RecipientOnionFields>,
+		/// A unique ID describing this payment (derived from the list of HTLCs in the payment).
+		///
+		/// Payers may pay for the same [`PaymentHash`] multiple times (though this is unsafe and
+		/// an intermediary node may steal the funds). Thus, in order to accurately track when
+		/// payments are received and claimed, you should use this identifier.
+		///
+		/// Only filled in for payments received on LDK versions 0.1 and higher.
+		payment_id: Option<PaymentId>,
 	},
 	/// Indicates that a peer connection with a node is needed in order to send an [`OnionMessage`].
 	///
@@ -1431,7 +1447,7 @@ impl Writeable for Event {
 			},
 			&Event::PaymentClaimable { ref payment_hash, ref amount_msat, counterparty_skimmed_fee_msat,
 				ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id,
-				ref claim_deadline, ref onion_fields
+				ref claim_deadline, ref onion_fields, ref payment_id,
 			} => {
 				1u8.write(writer)?;
 				let mut payment_secret = None;
@@ -1477,6 +1493,7 @@ impl Writeable for Event {
 					(9, onion_fields, option),
 					(10, skimmed_fee_opt, option),
 					(11, payment_context, option),
+					(13, payment_id, option),
 				});
 			},
 			&Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => {
@@ -1633,7 +1650,10 @@ impl Writeable for Event {
 				// We never write the OpenChannelRequest events as, upon disconnection, peers
 				// drop any channels which have not yet exchanged funding_signed.
 			},
-			&Event::PaymentClaimed { ref payment_hash, ref amount_msat, ref purpose, ref receiver_node_id, ref htlcs, ref sender_intended_total_msat, ref onion_fields } => {
+			&Event::PaymentClaimed { ref payment_hash, ref amount_msat, ref purpose,
+				ref receiver_node_id, ref htlcs, ref sender_intended_total_msat, ref onion_fields,
+				ref payment_id,
+			} => {
 				19u8.write(writer)?;
 				write_tlv_fields!(writer, {
 					(0, payment_hash, required),
@@ -1643,6 +1663,7 @@ impl Writeable for Event {
 					(5, *htlcs, optional_vec),
 					(7, sender_intended_total_msat, option),
 					(9, onion_fields, option),
+					(11, payment_id, option),
 				});
 			},
 			&Event::ProbeSuccessful { ref payment_id, ref payment_hash, ref path } => {
@@ -1766,6 +1787,7 @@ impl MaybeReadable for Event {
 					let mut via_user_channel_id = None;
 					let mut onion_fields = None;
 					let mut payment_context = None;
+					let mut payment_id = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
 						(1, receiver_node_id, option),
@@ -1779,6 +1801,7 @@ impl MaybeReadable for Event {
 						(9, onion_fields, option),
 						(10, counterparty_skimmed_fee_msat_opt, option),
 						(11, payment_context, option),
+						(13, payment_id, option),
 					});
 					let purpose = match payment_secret {
 						Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context),
@@ -1795,6 +1818,7 @@ impl MaybeReadable for Event {
 						via_user_channel_id,
 						claim_deadline,
 						onion_fields,
+						payment_id,
 					}))
 				};
 				f()
@@ -2036,6 +2060,7 @@ impl MaybeReadable for Event {
 					let mut htlcs: Option<Vec<ClaimedHTLC>> = Some(vec![]);
 					let mut sender_intended_total_msat: Option<u64> = None;
 					let mut onion_fields = None;
+					let mut payment_id = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
 						(1, receiver_node_id, option),
@@ -2044,6 +2069,7 @@ impl MaybeReadable for Event {
 						(5, htlcs, optional_vec),
 						(7, sender_intended_total_msat, option),
 						(9, onion_fields, option),
+						(11, payment_id, option),
 					});
 					Ok(Some(Event::PaymentClaimed {
 						receiver_node_id,
@@ -2053,6 +2079,7 @@ impl MaybeReadable for Event {
 						htlcs: htlcs.unwrap_or_default(),
 						sender_intended_total_msat,
 						onion_fields,
+						payment_id,
 					}))
 				};
 				f()

lightning/src/ln/channelmanager.rs

@@ -23,7 +23,7 @@ use bitcoin::constants::ChainHash;
 use bitcoin::key::constants::SECRET_KEY_SIZE;
 use bitcoin::network::Network;
 
-use bitcoin::hashes::Hash;
+use bitcoin::hashes::{Hash, HashEngine, HmacEngine};
 use bitcoin::hashes::hmac::Hmac;
 use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hash_types::{BlockHash, Txid};
@@ -366,6 +366,7 @@ pub(crate) struct HTLCPreviousHopData {
 	counterparty_node_id: Option<PublicKey>,
 }
 
+#[derive(PartialEq, Eq)]
 enum OnionPayload {
 	/// Indicates this incoming onion payload is for the purpose of paying an invoice.
 	Invoice {
@@ -378,6 +379,7 @@ enum OnionPayload {
 }
 
 /// HTLCs that are to us and can be failed/claimed by the user
+#[derive(PartialEq, Eq)]
 struct ClaimableHTLC {
 	prev_hop: HTLCPreviousHopData,
 	cltv_expiry: u32,
@@ -409,6 +411,23 @@ impl From<&ClaimableHTLC> for events::ClaimedHTLC {
 	}
 }
 
+impl PartialOrd for ClaimableHTLC {
+	fn partial_cmp(&self, other: &ClaimableHTLC) -> Option<cmp::Ordering> {
+		Some(self.cmp(other))
+	}
+}
+impl Ord for ClaimableHTLC {
+	fn cmp(&self, other: &ClaimableHTLC) -> cmp::Ordering {
+		let res = (self.prev_hop.channel_id, self.prev_hop.htlc_id).cmp(
+			&(other.prev_hop.channel_id, other.prev_hop.htlc_id)
+		);
+		if res.is_eq() {
+			debug_assert!(self == other, "ClaimableHTLCs from the same source should be identical");
+		}
+		res
+	}
+}
+
 /// A trait defining behavior for creating and verifing the HMAC for authenticating a given data.
 pub trait Verification {
 	/// Constructs an HMAC to include in [`OffersContext`] for the data along with the given
@@ -471,6 +490,22 @@ impl Verification for PaymentId {
 	}
 }
 
+impl PaymentId {
+	fn for_inbound_from_htlcs<I: Iterator<Item=(ChannelId, u64)>>(key: &[u8; 32], htlcs: I) -> PaymentId {
+		let mut prev_pair = None;
+		let mut hasher = HmacEngine::new(key);
+		for (channel_id, htlc_id) in htlcs {
+			hasher.input(&channel_id.0);
+			hasher.input(&htlc_id.to_le_bytes());
+			if let Some(prev) = prev_pair {
+				debug_assert!(prev < (channel_id, htlc_id), "HTLCs should be sorted");
+			}
+			prev_pair = Some((channel_id, htlc_id));
+		}
+		PaymentId(Hmac::<Sha256>::from_engine(hasher).to_byte_array())
+	}
+}
+
 impl Writeable for PaymentId {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
 		self.0.write(w)
@@ -744,6 +779,7 @@ struct ClaimingPayment {
 	htlcs: Vec<events::ClaimedHTLC>,
 	sender_intended_value: Option<u64>,
 	onion_fields: Option<RecipientOnionFields>,
+	payment_id: Option<PaymentId>,
 }
 impl_writeable_tlv_based!(ClaimingPayment, {
 	(0, amount_msat, required),
@@ -752,6 +788,7 @@ impl_writeable_tlv_based!(ClaimingPayment, {
 	(5, htlcs, optional_vec),
 	(7, sender_intended_value, option),
 	(9, onion_fields, option),
+	(11, payment_id, option),
 });
 
 struct ClaimablePayment {
@@ -760,6 +797,15 @@ struct ClaimablePayment {
 	htlcs: Vec<ClaimableHTLC>,
 }
 
+impl ClaimablePayment {
+	fn inbound_payment_id(&self, secret: &[u8; 32]) -> PaymentId {
+		PaymentId::for_inbound_from_htlcs(
+			secret,
+			self.htlcs.iter().map(|htlc| (htlc.prev_hop.channel_id, htlc.prev_hop.htlc_id))
+		)
+	}
+}
+
 /// Represent the channel funding transaction type.
 enum FundingType {
 	/// This variant is useful when we want LDK to validate the funding transaction and
@@ -5627,10 +5673,9 @@ where
 										} else {
 											claimable_payment.onion_fields = Some(onion_fields);
 										}
-										let ref mut htlcs = &mut claimable_payment.htlcs;
 										let mut total_value = claimable_htlc.sender_intended_value;
 										let mut earliest_expiry = claimable_htlc.cltv_expiry;
-										for htlc in htlcs.iter() {
+										for htlc in claimable_payment.htlcs.iter() {
 											total_value += htlc.sender_intended_value;
 											earliest_expiry = cmp::min(earliest_expiry, htlc.cltv_expiry);
 											if htlc.total_msat != claimable_htlc.total_msat {
@@ -5652,13 +5697,18 @@ where
 											#[allow(unused_assignments)] {
 												committed_to_claimable = true;
 											}
-											htlcs.push(claimable_htlc);
-											let amount_msat = htlcs.iter().map(|htlc| htlc.value).sum();
-											htlcs.iter_mut().for_each(|htlc| htlc.total_value_received = Some(amount_msat));
-											let counterparty_skimmed_fee_msat = htlcs.iter()
+											claimable_payment.htlcs.push(claimable_htlc);
+											let amount_msat =
+												claimable_payment.htlcs.iter().map(|htlc| htlc.value).sum();
+											claimable_payment.htlcs.iter_mut()
+												.for_each(|htlc| htlc.total_value_received = Some(amount_msat));
+											let counterparty_skimmed_fee_msat = claimable_payment.htlcs.iter()
 												.map(|htlc| htlc.counterparty_skimmed_fee_msat.unwrap_or(0)).sum();
 											debug_assert!(total_value.saturating_sub(amount_msat) <=
 												counterparty_skimmed_fee_msat);
+											claimable_payment.htlcs.sort();
+											let payment_id =
+												claimable_payment.inbound_payment_id(&self.inbound_payment_id_secret);
 											new_events.push_back((events::Event::PaymentClaimable {
 												receiver_node_id: Some(receiver_node_id),
 												payment_hash,
@@ -5669,13 +5719,14 @@ where
 												via_user_channel_id: Some(prev_user_channel_id),
 												claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER),
 												onion_fields: claimable_payment.onion_fields.clone(),
+												payment_id: Some(payment_id),
 											}, None));
 											payment_claimable_generated = true;
 										} else {
 											// Nothing to do - we haven't reached the total
 											// payment value yet, wait until we receive more
 											// MPP parts.
-											htlcs.push(claimable_htlc);
+											claimable_payment.htlcs.push(claimable_htlc);
 											#[allow(unused_assignments)] {
 												committed_to_claimable = true;
 											}
@@ -6472,6 +6523,7 @@ where
 					}
 				}
 
+				let payment_id = payment.inbound_payment_id(&self.inbound_payment_id_secret);
 				let claiming_payment = claimable_payments.pending_claiming_payments
 					.entry(payment_hash)
 					.and_modify(|_| {
@@ -6489,6 +6541,7 @@ where
 							htlcs,
 							sender_intended_value,
 							onion_fields: payment.onion_fields,
+							payment_id: Some(payment_id),
 						}
 					});
 
@@ -7006,6 +7059,7 @@ where
 						htlcs,
 						sender_intended_value: sender_intended_total_msat,
 						onion_fields,
+						payment_id,
 					}) = payment {
 						self.pending_events.lock().unwrap().push_back((events::Event::PaymentClaimed {
 							payment_hash,
@@ -7015,6 +7069,7 @@ where
 							htlcs,
 							sender_intended_total_msat,
 							onion_fields,
+							payment_id,
 						}, None));
 					}
 				},
@@ -12740,6 +12795,7 @@ where
 							previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &args.tx_broadcaster, &bounded_fee_estimator, &args.logger);
 						}
 					}
+					let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap());
 					pending_events_read.push_back((events::Event::PaymentClaimed {
 						receiver_node_id,
 						payment_hash,
@@ -12748,6 +12804,7 @@ where
 						htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(),
 						sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat),
 						onion_fields: payment.onion_fields,
+						payment_id: Some(payment_id),
 					}, None));
 				}
 			}

lightning/src/ln/msgs.rs

@@ -1729,8 +1729,7 @@ pub trait OnionMessageHandler {
 	fn provided_init_features(&self, their_node_id: PublicKey) -> InitFeatures;
 }
 
-#[derive(Clone)]
-#[cfg_attr(test, derive(Debug, PartialEq))]
+#[derive(Clone, Debug, PartialEq, Eq)]
 /// Information communicated in the onion to the recipient for multi-part tracking and proof that
 /// the payment is associated with an invoice.
 pub struct FinalOnionHopData {