f: Alternate approach. Introduce padding as a field instead of WithPadding

- WithPadding approach has a crucial flaw: it doesn't enforce that
   the tlv of the tlvs struct is not already utilizing the `(1,...)`
   position, which should be used for padding.
- Introduce padding as a field in the `ForwardTlvs` and `ReceiveTlvs`
  structs to ensure the above condition is met.
- Calculate the padding length prior to padding the TLVs and use that
  data to update the padding field later.

Co-authored-by: Jeffrey Czyz <[email protected]>

Author: shaavan

lightning/src/blinded_path/message.rs

@@ -17,7 +17,7 @@ use bitcoin::secp256k1::{self, PublicKey, Secp256k1, SecretKey};
 use crate::prelude::*;
 
 use crate::blinded_path::{BlindedHop, BlindedPath, IntroductionNode, NextMessageHop, NodeIdLookUp};
-use crate::blinded_path::utils::{self, WithPadding};
+use crate::blinded_path::utils;
 use crate::io;
 use crate::io::Cursor;
 use crate::ln::channelmanager::PaymentId;
@@ -31,6 +31,8 @@ use crate::util::ser::{FixedLengthReader, LengthReadableArgs, Writeable, Writer}
 use core::mem;
 use core::ops::Deref;
 
+use super::utils::Padding;
+
 /// An intermediate node, and possibly a short channel id leading to the next node.
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub struct ForwardNode {
@@ -46,6 +48,8 @@ pub struct ForwardNode {
 /// route, they are encoded into [`BlindedHop::encrypted_payload`].
 #[derive(Clone)]
 pub(crate) struct ForwardTlvs {
+	/// The padding data used to make all packets of a Blinded Path of same size
+	pub(crate) padding: Option<Padding>,
 	/// The next hop in the onion message's path.
 	pub(crate) next_hop: NextMessageHop,
 	/// Senders to a blinded path use this value to concatenate the route they find to the
@@ -56,6 +60,8 @@ pub(crate) struct ForwardTlvs {
 /// Similar to [`ForwardTlvs`], but these TLVs are for the final node.
 #[derive(Clone)]
 pub(crate) struct ReceiveTlvs {
+	/// The padding data used to make all packets of a Blinded Path of same size
+	pub(crate) padding: Option<Padding>,
 	/// If `context` is `Some`, it is used to identify the blinded path that this onion message is
 	/// sending to. This is useful for receivers to check that said blinded path is being used in
 	/// the right context.
@@ -69,6 +75,7 @@ impl Writeable for ForwardTlvs {
 			NextMessageHop::ShortChannelId(scid) => (None, Some(scid)),
 		};
 		encode_tlv_stream!(writer, {
+			(1, self.padding, option),
 			(2, short_channel_id, option),
 			(4, next_node_id, option),
 			(8, self.next_blinding_override, option)
@@ -80,6 +87,7 @@ impl Writeable for ForwardTlvs {
 impl Writeable for ReceiveTlvs {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
 		encode_tlv_stream!(writer, {
+			(1, self.padding, option),
 			(65537, self.context, option),
 		});
 		Ok(())
@@ -184,15 +192,15 @@ pub(super) fn blinded_hops<T: secp256k1::Signing + secp256k1::Verification>(
 			Some(scid) => NextMessageHop::ShortChannelId(scid),
 			None => NextMessageHop::NodeId(*pubkey),
 		})
-		.map(|next_hop| ControlTlvs::Forward(ForwardTlvs { next_hop, next_blinding_override: None }))
-		.chain(core::iter::once(ControlTlvs::Receive(ReceiveTlvs{ context: Some(context) })));
+		.map(|next_hop| ControlTlvs::Forward(ForwardTlvs { padding: None, next_hop, next_blinding_override: None }))
+		.chain(core::iter::once(ControlTlvs::Receive(ReceiveTlvs{ padding: None, context: Some(context) })));
 
 	let max_length = tlvs.clone()
 		.map(|tlv| tlv.serialized_length())
 		.max()
 		.unwrap_or(0);
 
-	let length_tlvs = tlvs.map(|tlvs| WithPadding { max_length, tlvs });
+	let length_tlvs = tlvs.map(|tlv| tlv.pad_tlvs(max_length));
 
 	utils::construct_blinded_hops(secp_ctx, pks, length_tlvs, session_priv)
 }
@@ -216,7 +224,7 @@ where
 	let mut reader = FixedLengthReader::new(&mut s, encrypted_control_tlvs.len() as u64);
 	match ChaChaPolyReadAdapter::read(&mut reader, rho) {
 		Ok(ChaChaPolyReadAdapter {
-			readable: ControlTlvs::Forward(ForwardTlvs { next_hop, next_blinding_override })
+			readable: ControlTlvs::Forward(ForwardTlvs {padding: _, next_hop, next_blinding_override })
 		}) => {
 			let next_node_id = match next_hop {
 				NextMessageHop::NodeId(pubkey) => pubkey,

lightning/src/blinded_path/payment.rs

@@ -14,7 +14,7 @@
 use bitcoin::secp256k1::{self, PublicKey, Secp256k1, SecretKey};
 
 use crate::blinded_path::{BlindedHop, BlindedPath, IntroductionNode, NodeIdLookUp};
-use crate::blinded_path::utils::{self, WithPadding};
+use crate::blinded_path::utils;
 use crate::crypto::streams::ChaChaPolyReadAdapter;
 use crate::io;
 use crate::io::Cursor;
@@ -35,6 +35,8 @@ use core::ops::Deref;
 #[allow(unused_imports)]
 use crate::prelude::*;
 
+use super::utils::Padding;
+
 /// An intermediate node, its outbound channel, and relay parameters.
 #[derive(Clone, Debug)]
 pub struct ForwardNode {
@@ -50,6 +52,8 @@ pub struct ForwardNode {
 /// Data to construct a [`BlindedHop`] for forwarding a payment.
 #[derive(Clone, Debug)]
 pub struct ForwardTlvs {
+	/// The padding data used to make all packets of a Blinded Path of same size
+	pub padding: Option<Padding>,
 	/// The short channel id this payment should be forwarded out over.
 	pub short_channel_id: u64,
 	/// Payment parameters for relaying over [`Self::short_channel_id`].
@@ -67,6 +71,8 @@ pub struct ForwardTlvs {
 /// may not be valid if received by another lightning implementation.
 #[derive(Clone, Debug)]
 pub struct ReceiveTlvs {
+	/// The padding data used to make all packets of a Blinded Path of same size
+	pub padding: Option<Padding>,
 	/// Used to authenticate the sender of a payment to the receiver and tie MPP HTLCs together.
 	pub payment_secret: PaymentSecret,
 	/// Constraints for the receiver of this payment.
@@ -78,6 +84,7 @@ pub struct ReceiveTlvs {
 /// Data to construct a [`BlindedHop`] for sending a payment over.
 ///
 /// [`BlindedHop`]: crate::blinded_path::BlindedHop
+#[derive(Clone)]
 pub(crate) enum BlindedPaymentTlvs {
 	/// This blinded payment data is for a forwarding node.
 	Forward(ForwardTlvs),
@@ -92,6 +99,27 @@ enum BlindedPaymentTlvsRef<'a> {
 	Receive(&'a ReceiveTlvs),
 }
 
+impl<'a> BlindedPaymentTlvsRef<'a> {
+	pub(crate) fn pad_tlvs(self, max_length: usize) -> BlindedPaymentTlvs {
+		let length = max_length.checked_sub(self.serialized_length());
+		debug_assert!(length.is_some(), "Size of this packet should not be larger than the size of the largest packet.");
+		let padding = Some(Padding::new(length.unwrap()));
+
+		match self {
+			BlindedPaymentTlvsRef::Forward(tlvs) => {
+				let mut new_tlvs = tlvs.clone();
+				new_tlvs.padding = padding;
+				BlindedPaymentTlvs::Forward(new_tlvs)
+			}
+			BlindedPaymentTlvsRef::Receive(tlvs) => {
+				let mut new_tlvs = tlvs.clone();
+				new_tlvs.padding = padding;
+				BlindedPaymentTlvs::Receive(new_tlvs)
+			}
+		}
+	}
+}
+
 /// Parameters for relaying over a given [`BlindedHop`].
 ///
 /// [`BlindedHop`]: crate::blinded_path::BlindedHop
@@ -205,6 +233,7 @@ impl Writeable for ForwardTlvs {
 			if self.features == BlindedHopFeatures::empty() { None }
 			else { Some(&self.features) };
 		encode_tlv_stream!(w, {
+			(1, self.padding, option),
 			(2, self.short_channel_id, required),
 			(10, self.payment_relay, required),
 			(12, self.payment_constraints, required),
@@ -217,6 +246,7 @@ impl Writeable for ForwardTlvs {
 impl Writeable for ReceiveTlvs {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
 		encode_tlv_stream!(w, {
+			(1, self.padding, option),
 			(12, self.payment_constraints, required),
 			(65536, self.payment_secret, required),
 			(65537, self.payment_context, required)
@@ -225,6 +255,16 @@ impl Writeable for ReceiveTlvs {
 	}
 }
 
+impl Writeable for BlindedPaymentTlvs {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		match self {
+			Self::Forward(tlvs) => tlvs.write(w)?,
+			Self::Receive(tlvs) => tlvs.write(w)?,
+		}
+		Ok(())
+	}
+}
+
 impl<'a> Writeable for BlindedPaymentTlvsRef<'a> {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
 		match self {
@@ -253,6 +293,7 @@ impl Readable for BlindedPaymentTlvs {
 				return Err(DecodeError::InvalidValue)
 			}
 			Ok(BlindedPaymentTlvs::Forward(ForwardTlvs {
+				padding: None,
 				short_channel_id,
 				payment_relay: payment_relay.ok_or(DecodeError::InvalidValue)?,
 				payment_constraints: payment_constraints.0.unwrap(),
@@ -261,6 +302,7 @@ impl Readable for BlindedPaymentTlvs {
 		} else {
 			if payment_relay.is_some() || features.is_some() { return Err(DecodeError::InvalidValue) }
 			Ok(BlindedPaymentTlvs::Receive(ReceiveTlvs {
+				padding: None,
 				payment_secret: payment_secret.ok_or(DecodeError::InvalidValue)?,
 				payment_constraints: payment_constraints.0.unwrap(),
 				payment_context: payment_context.0.unwrap(),
@@ -284,7 +326,7 @@ pub(super) fn blinded_hops<T: secp256k1::Signing + secp256k1::Verification>(
 		.max()
 		.unwrap_or(0);
 
-	let length_tlvs = tlvs.map(|tlvs| WithPadding { max_length, tlvs });
+	let length_tlvs = tlvs.map(|tlv| tlv.pad_tlvs(max_length));
 
 	utils::construct_blinded_hops(secp_ctx, pks, length_tlvs, session_priv)
 }
@@ -498,6 +540,7 @@ mod tests {
 		let intermediate_nodes = vec![ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 144,
@@ -514,6 +557,7 @@ mod tests {
 		}, ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 144,
@@ -529,6 +573,7 @@ mod tests {
 			htlc_maximum_msat: u64::max_value(),
 		}];
 		let recv_tlvs = ReceiveTlvs {
+			padding: None,
 			payment_secret: PaymentSecret([0; 32]),
 			payment_constraints: PaymentConstraints {
 				max_cltv_expiry: 0,
@@ -548,6 +593,7 @@ mod tests {
 	#[test]
 	fn compute_payinfo_1_hop() {
 		let recv_tlvs = ReceiveTlvs {
+			padding: None,
 			payment_secret: PaymentSecret([0; 32]),
 			payment_constraints: PaymentConstraints {
 				max_cltv_expiry: 0,
@@ -571,6 +617,7 @@ mod tests {
 		let intermediate_nodes = vec![ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 0,
@@ -587,6 +634,7 @@ mod tests {
 		}, ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 0,
@@ -602,6 +650,7 @@ mod tests {
 			htlc_maximum_msat: u64::max_value()
 		}];
 		let recv_tlvs = ReceiveTlvs {
+			padding: None,
 			payment_secret: PaymentSecret([0; 32]),
 			payment_constraints: PaymentConstraints {
 				max_cltv_expiry: 0,
@@ -622,6 +671,7 @@ mod tests {
 		let intermediate_nodes = vec![ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 0,
@@ -638,6 +688,7 @@ mod tests {
 		}, ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 0,
@@ -653,6 +704,7 @@ mod tests {
 			htlc_maximum_msat: u64::max_value()
 		}];
 		let recv_tlvs = ReceiveTlvs {
+			padding: None,
 			payment_secret: PaymentSecret([0; 32]),
 			payment_constraints: PaymentConstraints {
 				max_cltv_expiry: 0,
@@ -677,6 +729,7 @@ mod tests {
 		let intermediate_nodes = vec![ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 0,
@@ -693,6 +746,7 @@ mod tests {
 		}, ForwardNode {
 			node_id: dummy_pk,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: 0,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: 0,
@@ -708,6 +762,7 @@ mod tests {
 			htlc_maximum_msat: 10_000
 		}];
 		let recv_tlvs = ReceiveTlvs {
+			padding: None,
 			payment_secret: PaymentSecret([0; 32]),
 			payment_constraints: PaymentConstraints {
 				max_cltv_expiry: 0,

lightning/src/blinded_path/utils.rs

@@ -137,7 +137,8 @@ fn encrypt_payload<P: Writeable>(payload: P, encrypted_tlvs_rho: [u8; 32]) -> Ve
 
 /// Represents optional padding for encrypted payloads.
 /// Padding is used to ensure payloads have a consistent length.
-pub(crate) struct Padding {
+#[derive(Clone, Debug)]
+pub struct Padding {
 	length: usize,
 }
 
@@ -177,27 +178,3 @@ impl Writeable for Padding {
 		Ok(())
 	}
 }
-
-
-/// A wrapper struct that stores the largest packet size for a [`BlindedPath`].
-/// This helps us calculate the appropriate padding size for the tlvs when writing them.
-pub(super) struct WithPadding<T: Writeable> {
-	/// Length of the packet with the largest size in the [`BlindedPath`].
-	pub(super) max_length: usize,
-	/// The current packet's TLVs.
-	pub(super) tlvs: T,
-}
-
-impl<T: Writeable> Writeable for WithPadding<T> {
-	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
-		let length = self.max_length.checked_sub(self.tlvs.serialized_length());
-		debug_assert!(length.is_some(), "Size of this packet should not be larger than the size of largest packet.");
-		let padding = Some(Padding::new(length.unwrap()));
-
-		encode_tlv_stream!(writer, {
-			(1, padding, option)
-		});
-
-		self.tlvs.write(writer)
-	}
-}

lightning/src/ln/blinded_payment_tests.rs

@@ -39,6 +39,7 @@ fn blinded_payment_path(
 		intermediate_nodes.push(ForwardNode {
 			node_id: *node_id,
 			tlvs: ForwardTlvs {
+				padding: None,
 				short_channel_id: chan_upd.short_channel_id,
 				payment_relay: PaymentRelay {
 					cltv_expiry_delta: chan_upd.cltv_expiry_delta,
@@ -57,6 +58,7 @@ fn blinded_payment_path(
 		});
 	}
 	let payee_tlvs = ReceiveTlvs {
+		padding: None,
 		payment_secret,
 		payment_constraints: PaymentConstraints {
 			max_cltv_expiry: u32::max_value(),
@@ -104,6 +106,7 @@ fn do_one_hop_blinded_path(success: bool) {
 	let amt_msat = 5000;
 	let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None);
 	let payee_tlvs = ReceiveTlvs {
+		padding: None,
 		payment_secret,
 		payment_constraints: PaymentConstraints {
 			max_cltv_expiry: u32::max_value(),
@@ -148,6 +151,7 @@ fn mpp_to_one_hop_blinded_path() {
 	let amt_msat = 15_000_000;
 	let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[3], Some(amt_msat), None);
 	let payee_tlvs = ReceiveTlvs {
+		padding: None,
 		payment_secret,
 		payment_constraints: PaymentConstraints {
 			max_cltv_expiry: u32::max_value(),
@@ -1293,6 +1297,7 @@ fn custom_tlvs_to_blinded_path() {
 	let amt_msat = 5000;
 	let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None);
 	let payee_tlvs = ReceiveTlvs {
+		padding: None,
 		payment_secret,
 		payment_constraints: PaymentConstraints {
 			max_cltv_expiry: u32::max_value(),

lightning/src/ln/channelmanager.rs

@@ -9345,6 +9345,7 @@ where
 		let max_cltv_expiry = self.best_block.read().unwrap().height + CLTV_FAR_FAR_AWAY
 			+ LATENCY_GRACE_PERIOD_BLOCKS;
 		let payee_tlvs = ReceiveTlvs {
+			padding: None,
 			payment_secret,
 			payment_constraints: PaymentConstraints {
 				max_cltv_expiry,