Merge pull request lightningdevkit#2762 from TheBlueMatt/2023-11-htlc…

…-docs

Improve docs on newly-public structs after  lightningdevkit#2700

lightning/src/ln/channelmanager.rs

@@ -109,50 +109,79 @@ use crate::ln::script::ShutdownScript;
 // Alternatively, we can fill an outbound HTLC with a HTLCSource::OutboundRoute indicating this is
 // our payment, which we can use to decode errors or inform the user that the payment was sent.
 
-/// Routing info for an inbound HTLC onion.
+/// Information about where a received HTLC('s onion) has indicated the HTLC should go.
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
 pub enum PendingHTLCRouting {
-	/// A forwarded HTLC.
+	/// An HTLC which should be forwarded on to another node.
 	Forward {
-		/// BOLT 4 onion packet.
+		/// The onion which should be included in the forwarded HTLC, telling the next hop what to
+		/// do with the HTLC.
 		onion_packet: msgs::OnionPacket,
-		/// The SCID from the onion that we should forward to. This could be a real SCID or a fake one
-		/// generated using `get_fake_scid` from the scid_utils::fake_scid module.
+		/// The short channel ID of the channel which we were instructed to forward this HTLC to.
+		///
+		/// This could be a real on-chain SCID, an SCID alias, or some other SCID which has meaning
+		/// to the receiving node, such as one returned from
+		/// [`ChannelManager::get_intercept_scid`] or [`ChannelManager::get_phantom_scid`].
 		short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 		/// Set if this HTLC is being forwarded within a blinded path.
 		blinded: Option<BlindedForward>,
 	},
-	/// An HTLC paid to an invoice (supposedly) generated by us.
-	/// At this point, we have not checked that the invoice being paid was actually generated by us,
-	/// but rather it's claiming to pay an invoice of ours.
+	/// The onion indicates that this is a payment for an invoice (supposedly) generated by us.
+	///
+	/// Note that at this point, we have not checked that the invoice being paid was actually
+	/// generated by us, but rather it's claiming to pay an invoice of ours.
 	Receive {
-		/// Payment secret and total msat received.
+		/// Information about the amount the sender intended to pay and (potential) proof that this
+		/// is a payment for an invoice we generated. This proof of payment is is also used for
+		/// linking MPP parts of a larger payment.
 		payment_data: msgs::FinalOnionHopData,
-		/// See [`RecipientOnionFields::payment_metadata`] for more info.
+		/// Additional data which we (allegedly) instructed the sender to include in the onion.
+		///
+		/// For HTLCs received by LDK, this will ultimately be exposed in
+		/// [`Event::PaymentClaimable::onion_fields`] as
+		/// [`RecipientOnionFields::payment_metadata`].
 		payment_metadata: Option<Vec<u8>>,
 		/// CLTV expiry of the received HTLC.
+		///
 		/// Used to track when we should expire pending HTLCs that go unclaimed.
 		incoming_cltv_expiry: u32,
-		/// Shared secret derived using a phantom node secret key. If this field is Some, the
-		/// payment was sent to a phantom node (one hop beyond the current node), but can be
-		/// settled by this node.
+		/// If the onion had forwarding instructions to one of our phantom node SCIDs, this will
+		/// provide the onion shared secret used to decrypt the next level of forwarding
+		/// instructions.
 		phantom_shared_secret: Option<[u8; 32]>,
-		/// See [`RecipientOnionFields::custom_tlvs`] for more info.
+		/// Custom TLVs which were set by the sender.
+		///
+		/// For HTLCs received by LDK, this will ultimately be exposed in
+		/// [`Event::PaymentClaimable::onion_fields`] as
+		/// [`RecipientOnionFields::custom_tlvs`].
 		custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
-	/// Incoming keysend (sender provided the preimage in a TLV).
+	/// The onion indicates that this is for payment to us but which contains the preimage for
+	/// claiming included, and is unrelated to any invoice we'd previously generated (aka a
+	/// "keysend" or "spontaneous" payment).
 	ReceiveKeysend {
-		/// This was added in 0.0.116 and will break deserialization on downgrades.
+		/// Information about the amount the sender intended to pay and possibly a token to
+		/// associate MPP parts of a larger payment.
+		///
+		/// This will only be filled in if receiving MPP keysend payments is enabled, and it being
+		/// present will cause deserialization to fail on versions of LDK prior to 0.0.116.
 		payment_data: Option<msgs::FinalOnionHopData>,
 		/// Preimage for this onion payment. This preimage is provided by the sender and will be
 		/// used to settle the spontaneous payment.
 		payment_preimage: PaymentPreimage,
-		/// See [`RecipientOnionFields::payment_metadata`] for more info.
+		/// Additional data which we (allegedly) instructed the sender to include in the onion.
+		///
+		/// For HTLCs received by LDK, this will ultimately bubble back up as
+		/// [`RecipientOnionFields::payment_metadata`].
 		payment_metadata: Option<Vec<u8>>,
 		/// CLTV expiry of the received HTLC.
+		///
 		/// Used to track when we should expire pending HTLCs that go unclaimed.
 		incoming_cltv_expiry: u32,
-		/// See [`RecipientOnionFields::custom_tlvs`] for more info.
+		/// Custom TLVs which were set by the sender.
+		///
+		/// For HTLCs received by LDK, these will ultimately bubble back up as
+		/// [`RecipientOnionFields::custom_tlvs`].
 		custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
 }
@@ -179,25 +208,47 @@ impl PendingHTLCRouting {
 	}
 }
 
-/// Full details of an incoming HTLC, including routing info.
+/// Information about an incoming HTLC, including the [`PendingHTLCRouting`] describing where it
+/// should go next.
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
 pub struct PendingHTLCInfo {
 	/// Further routing details based on whether the HTLC is being forwarded or received.
 	pub routing: PendingHTLCRouting,
-	/// Shared secret from the previous hop.
-	/// Used encrypt failure packets in the event that the HTLC needs to be failed backwards.
+	/// The onion shared secret we build with the sender used to decrypt the onion.
+	///
+	/// This is later used to encrypt failure packets in the event that the HTLC is failed.
 	pub incoming_shared_secret: [u8; 32],
 	/// Hash of the payment preimage, to lock the payment until the receiver releases the preimage.
 	pub payment_hash: PaymentHash,
-	/// Amount offered by this HTLC.
-	pub incoming_amt_msat: Option<u64>, // Added in 0.0.113
-	/// Sender intended amount to forward or receive (actual amount received
-	/// may overshoot this in either case)
+	/// Amount received in the incoming HTLC.
+	///
+	/// This field was added in LDK 0.0.113 and will be `None` for objects written by prior
+	/// versions.
+	pub incoming_amt_msat: Option<u64>,
+	/// The amount the sender indicated should be forwarded on to the next hop or amount the sender
+	/// intended for us to receive for received payments.
+	///
+	/// If the received amount is less than this for received payments, an intermediary hop has
+	/// attempted to steal some of our funds and we should fail the HTLC (the sender should retry
+	/// it along another path).
+	///
+	/// Because nodes can take less than their required fees, and because senders may wish to
+	/// improve their own privacy, this amount may be less than [`Self::incoming_amt_msat`] for
+	/// received payments. In such cases, recipients must handle this HTLC as if it had received
+	/// [`Self::outgoing_amt_msat`].
 	pub outgoing_amt_msat: u64,
-	/// Outgoing timelock expiration blockheight.
+	/// The CLTV the sender has indicated we should set on the forwarded HTLC (or has indicated
+	/// should have been set on the received HTLC for received payments).
 	pub outgoing_cltv_value: u32,
-	/// The fee being skimmed off the top of this HTLC. If this is a forward, it'll be the fee we are
-	/// skimming. If we're receiving this HTLC, it's the fee that our counterparty skimmed.
+	/// The fee taken for this HTLC in addition to the standard protocol HTLC fees.
+	///
+	/// If this is a payment for forwarding, this is the fee we are taking before forwarding the
+	/// HTLC.
+	///
+	/// If this is a received payment, this is the fee that our counterparty took.
+	///
+	/// This is used to allow LSPs to take fees as a part of payments, without the sender having to
+	/// shoulder them.
 	pub skimmed_fee_msat: Option<u64>,
 }
 

lightning/src/ln/onion_payment.rs

@@ -1,6 +1,7 @@
-//! Utilities for channelmanager.rs
+//! Utilities to decode payment onions and do contextless validation of incoming payments.
 //!
-//! Includes a public [`peel_payment_onion`] function for use by external projects or libraries.
+//! Primarily features [`peel_payment_onion`], which allows the decoding of an onion statelessly
+//! and can be used to predict whether we'd accept a payment.
 
 use bitcoin::hashes::Hash;
 use bitcoin::hashes::sha256::Hash as Sha256;
@@ -225,7 +226,9 @@ pub(super) fn create_recv_pending_htlc_info(
 	})
 }
 
-/// Peel one layer off an incoming onion, returning [`PendingHTLCInfo`] (either Forward or Receive).
+/// Peel one layer off an incoming onion, returning a [`PendingHTLCInfo`] that contains information
+/// about the intended next-hop for the HTLC.
+///
 /// This does all the relevant context-free checks that LDK requires for payment relay or
 /// acceptance. If the payment is to be received, and the amount matches the expected amount for
 /// a given invoice, this indicates the [`msgs::UpdateAddHTLC`], once fully committed in the
@@ -234,7 +237,7 @@ pub(super) fn create_recv_pending_htlc_info(
 /// [`Event::PaymentClaimable`]: crate::events::Event::PaymentClaimable
 pub fn peel_payment_onion<NS: Deref, L: Deref, T: secp256k1::Verification>(
 	msg: &msgs::UpdateAddHTLC, node_signer: &NS, logger: &L, secp_ctx: &Secp256k1<T>,
-	cur_height: u32, accept_mpp_keysend: bool,
+	cur_height: u32, accept_mpp_keysend: bool, allow_skimmed_fees: bool,
 ) -> Result<PendingHTLCInfo, InboundOnionErr>
 where
 	NS::Target: NodeSigner,
@@ -273,6 +276,10 @@ where
 					err_data: Vec::new(),
 				});
 			}
+
+			// TODO: If this is potentially a phantom payment we should decode the phantom payment
+			// onion here and check it.
+
 			create_fwd_pending_htlc_info(
 				msg, next_hop_data, next_hop_hmac, new_packet_bytes, shared_secret,
 				Some(next_packet_pubkey)
@@ -281,7 +288,7 @@ where
 		onion_utils::Hop::Receive(received_data) => {
 			create_recv_pending_htlc_info(
 				received_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry,
-				None, false, msg.skimmed_fee_msat, cur_height, accept_mpp_keysend,
+				None, allow_skimmed_fees, msg.skimmed_fee_msat, cur_height, accept_mpp_keysend,
 			)?
 		}
 	})
@@ -477,7 +484,7 @@ mod tests {
 		let msg = make_update_add_msg(amount_msat, cltv_expiry, payment_hash, onion);
 		let logger = test_utils::TestLogger::with_id("bob".to_string());
 
-		let peeled = peel_payment_onion(&msg, &&bob, &&logger, &secp_ctx, cur_height, true)
+		let peeled = peel_payment_onion(&msg, &&bob, &&logger, &secp_ctx, cur_height, true, false)
 			.map_err(|e| e.msg).unwrap();
 
 		let next_onion = match peeled.routing {
@@ -488,7 +495,7 @@ mod tests {
 		};
 
 		let msg2 = make_update_add_msg(amount_msat, cltv_expiry, payment_hash, next_onion);
-		let peeled2 = peel_payment_onion(&msg2, &&charlie, &&logger, &secp_ctx, cur_height, true)
+		let peeled2 = peel_payment_onion(&msg2, &&charlie, &&logger, &secp_ctx, cur_height, true, false)
 			.map_err(|e| e.msg).unwrap();
 
 		match peeled2.routing {