Centralize custom TLV validation behind CustomTlvs

Introduce a `CustomTlvs` wrapper to move sorting and validation of custom
TLVs out of `RecipientOnionFields` and into a dedicated type.

This makes TLV validity an explicit construction-time concern, allowing
`RecipientOnionFields` to assume correctness and remain a simple data
carrier. In turn, custom TLV usage becomes easier to extend without
duplicating protocol checks.

Author: shaavan

lightning/src/ln/blinded_payment_tests.rs

@@ -22,7 +22,7 @@ use crate::ln::msgs::{
 };
 use crate::ln::onion_payment;
 use crate::ln::onion_utils::{self, LocalHTLCFailureReason};
-use crate::ln::outbound_payment::{Retry, IDEMPOTENCY_TIMEOUT_TICKS};
+use crate::ln::outbound_payment::{RecipientCustomTlvs, Retry, IDEMPOTENCY_TIMEOUT_TICKS};
 use crate::ln::types::ChannelId;
 use crate::offers::invoice::UnsignedBolt12Invoice;
 use crate::prelude::*;
@@ -1431,8 +1431,7 @@ fn custom_tlvs_to_blinded_path() {
 	);
 
 	let recipient_onion_fields = RecipientOnionFields::spontaneous_empty()
-		.with_custom_tlvs(vec![((1 << 16) + 1, vec![42, 42])])
-		.unwrap();
+		.with_custom_tlvs(RecipientCustomTlvs::new(vec![((1 << 16) + 1, vec![42, 42])]).unwrap());
 	nodes[0].node.send_payment(payment_hash, recipient_onion_fields.clone(),
 		PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap();
 	check_added_monitors(&nodes[0], 1);

lightning/src/ln/max_payment_path_len_tests.rs

@@ -23,7 +23,9 @@ use crate::ln::msgs;
 use crate::ln::msgs::{BaseMessageHandler, OnionMessageHandler};
 use crate::ln::onion_utils;
 use crate::ln::onion_utils::MIN_FINAL_VALUE_ESTIMATE_WITH_OVERPAY;
-use crate::ln::outbound_payment::{RecipientOnionFields, Retry, RetryableSendFailure};
+use crate::ln::outbound_payment::{
+	RecipientCustomTlvs, RecipientOnionFields, Retry, RetryableSendFailure,
+};
 use crate::prelude::*;
 use crate::routing::router::{
 	PaymentParameters, RouteParameters, DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA,
@@ -259,9 +261,9 @@ fn one_hop_blinded_path_with_custom_tlv() {
 		- final_payload_len_without_custom_tlv;
 
 	// Check that we can send the maximum custom TLV with 1 blinded hop.
-	let max_sized_onion = RecipientOnionFields::spontaneous_empty()
-		.with_custom_tlvs(vec![(CUSTOM_TLV_TYPE, vec![42; max_custom_tlv_len])])
-		.unwrap();
+	let max_sized_onion = RecipientOnionFields::spontaneous_empty().with_custom_tlvs(
+		RecipientCustomTlvs::new(vec![(CUSTOM_TLV_TYPE, vec![42; max_custom_tlv_len])]).unwrap(),
+	);
 	let id = PaymentId(payment_hash.0);
 	let no_retry = Retry::Attempts(0);
 	nodes[1]
@@ -385,9 +387,9 @@ fn blinded_path_with_custom_tlv() {
 		- reserved_packet_bytes_without_custom_tlv;
 
 	// Check that we can send the maximum custom TLV size with 0 intermediate unblinded hops.
-	let max_sized_onion = RecipientOnionFields::spontaneous_empty()
-		.with_custom_tlvs(vec![(CUSTOM_TLV_TYPE, vec![42; max_custom_tlv_len])])
-		.unwrap();
+	let max_sized_onion = RecipientOnionFields::spontaneous_empty().with_custom_tlvs(
+		RecipientCustomTlvs::new(vec![(CUSTOM_TLV_TYPE, vec![42; max_custom_tlv_len])]).unwrap(),
+	);
 	let no_retry = Retry::Attempts(0);
 	let id = PaymentId(payment_hash.0);
 	nodes[1]

lightning/src/ln/msgs.rs

@@ -3537,7 +3537,7 @@ impl<'a> Writeable for OutboundOnionPayload<'a> {
 				ref invoice_request,
 				ref custom_tlvs,
 			} => {
-				// We need to update [`ln::outbound_payment::RecipientOnionFields::with_custom_tlvs`]
+				// We need to update [`ln::outbound_payments::RecipientCustomTlvs::new`]
 				// to reject any reserved types in the experimental range if new ones are ever
 				// standardized.
 				let invoice_request_tlv = invoice_request.map(|invreq| (77_777, invreq.encode())); // TODO: update TLV type once the async payments spec is merged

lightning/src/ln/outbound_payment.rs

@@ -677,6 +677,54 @@ pub enum ProbeSendFailure {
 	DuplicateProbe,
 }
 
+/// A validated, sorted set of custom TLVs for payment recipient onion fields.
+#[derive(Clone)]
+pub struct RecipientCustomTlvs(Vec<(u64, Vec<u8>)>);
+
+impl RecipientCustomTlvs {
+	/// Each TLV is provided as a `(u64, Vec<u8>)` for the type number and
+	/// serialized value respectively. TLV type numbers must be unique and
+	/// within the range reserved for custom types, i.e. >= 2^16, otherwise
+	/// this method will return `Err(())`.
+	///
+	/// This method will also error for TLV types in the experimental range
+	/// which have since been standardized within the protocol. This currently
+	/// includes 5482373484 (keysend) and 77_777 (invoice requests for async
+	/// payments).
+	pub fn new(mut tlvs: Vec<(u64, Vec<u8>)>) -> Result<Self, ()> {
+		tlvs.sort_unstable_by_key(|(typ, _)| *typ);
+		let mut prev_type = None;
+		for (typ, _) in tlvs.iter() {
+			if *typ < 1 << 16 {
+				return Err(());
+			}
+			if *typ == 5482373484 {
+				return Err(());
+			} // keysend
+			if *typ == 77_777 {
+				return Err(());
+			} // invoice requests for async payments
+			match prev_type {
+				Some(prev) if prev >= *typ => return Err(()),
+				_ => {},
+			}
+			prev_type = Some(*typ);
+		}
+
+		Ok(Self(tlvs))
+	}
+
+	/// Returns the inner TLV list.
+	pub(super) fn into_inner(self) -> Vec<(u64, Vec<u8>)> {
+		self.0
+	}
+
+	/// Borrow the inner TLV list.
+	pub fn as_slice(&self) -> &[(u64, Vec<u8>)] {
+		&self.0
+	}
+}
+
 /// Information which is provided, encrypted, to the payment recipient when sending HTLCs.
 ///
 /// This should generally be constructed with data communicated to us from the recipient (via a
@@ -739,31 +787,13 @@ impl RecipientOnionFields {
 		Self { payment_secret: None, payment_metadata: None, custom_tlvs: Vec::new() }
 	}
 
-	/// Creates a new [`RecipientOnionFields`] from an existing one, adding custom TLVs. Each
-	/// TLV is provided as a `(u64, Vec<u8>)` for the type number and serialized value
-	/// respectively. TLV type numbers must be unique and within the range
-	/// reserved for custom types, i.e. >= 2^16, otherwise this method will return `Err(())`.
-	///
-	/// This method will also error for types in the experimental range which have been
-	/// standardized within the protocol, which only includes 5482373484 (keysend) for now.
+	/// Creates a new [`RecipientOnionFields`] from an existing one, adding validated custom TLVs.
 	///
 	/// See [`Self::custom_tlvs`] for more info.
 	#[rustfmt::skip]
-	pub fn with_custom_tlvs(mut self, mut custom_tlvs: Vec<(u64, Vec<u8>)>) -> Result<Self, ()> {
-		custom_tlvs.sort_unstable_by_key(|(typ, _)| *typ);
-		let mut prev_type = None;
-		for (typ, _) in custom_tlvs.iter() {
-			if *typ < 1 << 16 { return Err(()); }
-			if *typ == 5482373484 { return Err(()); } // keysend
-			if *typ == 77_777 { return Err(()); } // invoice requests for async payments
-			match prev_type {
-				Some(prev) if prev >= *typ => return Err(()),
-				_ => {},
-			}
-			prev_type = Some(*typ);
-		}
-		self.custom_tlvs = custom_tlvs;
-		Ok(self)
+	pub fn with_custom_tlvs(mut self, custom_tlvs: RecipientCustomTlvs) -> Self {
+		self.custom_tlvs = custom_tlvs.into_inner();
+		self
 	}
 
 	/// Gets the custom TLVs that will be sent or have been received.
@@ -2815,8 +2845,8 @@ mod tests {
 	use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
 	use crate::ln::inbound_payment::ExpandedKey;
 	use crate::ln::outbound_payment::{
-		Bolt12PaymentError, OutboundPayments, PendingOutboundPayment, Retry, RetryableSendFailure,
-		StaleExpiration,
+		Bolt12PaymentError, OutboundPayments, PendingOutboundPayment, RecipientCustomTlvs, Retry,
+		RetryableSendFailure, StaleExpiration,
 	};
 	#[cfg(feature = "std")]
 	use crate::offers::invoice::DEFAULT_RELATIVE_EXPIRY;
@@ -2843,22 +2873,23 @@ mod tests {
 	fn test_recipient_onion_fields_with_custom_tlvs() {
 		let onion_fields = RecipientOnionFields::spontaneous_empty();
 
-		let bad_type_range_tlvs = vec![
+		let bad_type_range_tlvs = RecipientCustomTlvs::new(vec![
 			(0, vec![42]),
 			(1, vec![42; 32]),
-		];
-		assert!(onion_fields.clone().with_custom_tlvs(bad_type_range_tlvs).is_err());
+		]);
+		assert!(bad_type_range_tlvs.is_err());
 
-		let keysend_tlv = vec![
+		let keysend_tlv = RecipientCustomTlvs::new(vec![
 			(5482373484, vec![42; 32]),
-		];
-		assert!(onion_fields.clone().with_custom_tlvs(keysend_tlv).is_err());
+		]);
+		assert!(keysend_tlv.is_err());
 
-		let good_tlvs = vec![
+		let good_tlvs = RecipientCustomTlvs::new(vec![
 			((1 << 16) + 1, vec![42]),
 			((1 << 16) + 3, vec![42; 32]),
-		];
-		assert!(onion_fields.with_custom_tlvs(good_tlvs).is_ok());
+		]);
+		assert!(good_tlvs.is_ok());
+		onion_fields.with_custom_tlvs(good_tlvs.unwrap());
 	}
 
 	#[test]

lightning/src/ln/payment_tests.rs

@@ -32,7 +32,7 @@ use crate::ln::msgs;
 use crate::ln::msgs::{BaseMessageHandler, ChannelMessageHandler, MessageSendEvent};
 use crate::ln::onion_utils::{self, LocalHTLCFailureReason};
 use crate::ln::outbound_payment::{
-	ProbeSendFailure, Retry, RetryableSendFailure, IDEMPOTENCY_TIMEOUT_TICKS,
+	ProbeSendFailure, RecipientCustomTlvs, Retry, RetryableSendFailure, IDEMPOTENCY_TIMEOUT_TICKS,
 };
 use crate::ln::types::ChannelId;
 use crate::routing::gossip::{EffectiveCapacity, RoutingFees};
@@ -4539,7 +4539,7 @@ fn test_retry_custom_tlvs() {
 
 	let custom_tlvs = vec![((1 << 16) + 1, vec![0x42u8; 16])];
 	let onion = RecipientOnionFields::secret_only(payment_secret);
-	let onion = onion.with_custom_tlvs(custom_tlvs.clone()).unwrap();
+	let onion = onion.with_custom_tlvs(RecipientCustomTlvs::new(custom_tlvs.clone()).unwrap());
 
 	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
 	nodes[0].node.send_payment(hash, onion, id, route_params.clone(), Retry::Attempts(1)).unwrap();
@@ -5079,8 +5079,7 @@ fn peel_payment_onion_custom_tlvs() {
 	let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
 	let route = functional_test_utils::get_route(&nodes[0], &route_params).unwrap();
 	let mut recipient_onion = RecipientOnionFields::spontaneous_empty()
-		.with_custom_tlvs(vec![(414141, vec![42; 1200])])
-		.unwrap();
+		.with_custom_tlvs(RecipientCustomTlvs::new(vec![(414141, vec![42; 1200])]).unwrap());
 	let prng_seed = chanmon_cfgs[0].keys_manager.get_secure_random_bytes();
 	let session_priv = SecretKey::from_slice(&prng_seed[..]).expect("RNG is busted");
 	let keysend_preimage = PaymentPreimage([42; 32]);