Receive payment onions as new InboundPayload instead of OnionHopData

To support route blinding, we want to split OnionHopData into two separate
structs, one for inbound onions and one for outbound onions. This is because
blinded payloads change the fields present in the onion hop data struct based
on whether we're sending vs receiving (outbound onions include encrypted blobs,
inbound onions can decrypt those blobs and contain the decrypted fields
themselves).

In upcoming commits, we'll add variants for blinded payloads to the new
InboundPayload enum.

Author: valentinewallace

fuzz/src/msg_targets/msg_onion_hop_data.rs

@@ -15,11 +15,11 @@ use crate::utils::test_logger;
 
 #[inline]
 pub fn msg_onion_hop_data_test<Out: test_logger::Output>(data: &[u8], _out: Out) {
-	test_msg_simple!(lightning::ln::msgs::OnionHopData, data);
+	// TODO
 }
 
 #[no_mangle]
 pub extern "C" fn msg_onion_hop_data_run(data: *const u8, datalen: usize) {
 	let data = unsafe { std::slice::from_raw_parts(data, datalen) };
-	test_msg_simple!(lightning::ln::msgs::OnionHopData, data);
+	// TODO
 }

lightning/src/ln/channelmanager.rs

@@ -2617,7 +2617,7 @@ where
 	}
 
 	fn construct_fwd_pending_htlc_info(
-		&self, msg: &msgs::UpdateAddHTLC, hop_data: msgs::OnionHopData, hop_hmac: [u8; 32],
+		&self, msg: &msgs::UpdateAddHTLC, hop_data: msgs::InboundPayload, hop_hmac: [u8; 32],
 		new_packet_bytes: [u8; onion_utils::ONION_DATA_LEN], shared_secret: [u8; 32],
 		next_packet_pubkey_opt: Option<Result<PublicKey, secp256k1::Error>>
 	) -> Result<PendingHTLCInfo, InboundOnionErr> {
@@ -2626,42 +2626,44 @@ where
 			version: 0,
 			public_key: next_packet_pubkey_opt.unwrap_or(Err(secp256k1::Error::InvalidPublicKey)),
 			hop_data: new_packet_bytes,
-			hmac: hop_hmac.clone(),
+			hmac: hop_hmac,
 		};
 
-		let short_channel_id = match hop_data.format {
-			msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id,
-			msgs::OnionHopDataFormat::FinalNode { .. } => {
+		let (short_channel_id, amt_to_forward, outgoing_cltv_value) = match hop_data {
+			msgs::InboundPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } =>
+				(short_channel_id, amt_to_forward, outgoing_cltv_value),
+			msgs::InboundPayload::Receive { .. } =>
 				return Err(InboundOnionErr {
 					msg: "Final Node OnionHopData provided for us as an intermediary node",
 					err_code: 0x4000 | 22,
 					err_data: Vec::new(),
-				})
-			},
+				}),
 		};
 
 		Ok(PendingHTLCInfo {
 			routing: PendingHTLCRouting::Forward {
 				onion_packet: outgoing_packet,
 				short_channel_id,
 			},
-			payment_hash: msg.payment_hash.clone(),
+			payment_hash: msg.payment_hash,
 			incoming_shared_secret: shared_secret,
 			incoming_amt_msat: Some(msg.amount_msat),
-			outgoing_amt_msat: hop_data.amt_to_forward,
-			outgoing_cltv_value: hop_data.outgoing_cltv_value,
+			outgoing_amt_msat: amt_to_forward,
+			outgoing_cltv_value,
 			skimmed_fee_msat: None,
 		})
 	}
 
 	fn construct_recv_pending_htlc_info(
-		&self, hop_data: msgs::OnionHopData, shared_secret: [u8; 32], payment_hash: PaymentHash,
+		&self, hop_data: msgs::InboundPayload, shared_secret: [u8; 32], payment_hash: PaymentHash,
 		amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool,
 		counterparty_skimmed_fee_msat: Option<u64>,
 	) -> Result<PendingHTLCInfo, InboundOnionErr> {
-		let (payment_data, keysend_preimage, payment_metadata) = match hop_data.format {
-			msgs::OnionHopDataFormat::FinalNode { payment_data, keysend_preimage, payment_metadata } =>
-				(payment_data, keysend_preimage, payment_metadata),
+		let (payment_data, keysend_preimage, onion_amt_msat, outgoing_cltv_value, payment_metadata) = match hop_data {
+			msgs::InboundPayload::Receive {
+				payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata, ..
+			} =>
+				(payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata),
 			_ =>
 				return Err(InboundOnionErr {
 					err_code: 0x4000|22,
@@ -2670,7 +2672,7 @@ where
 				}),
 		};
 		// final_incorrect_cltv_expiry
-		if hop_data.outgoing_cltv_value > cltv_expiry {
+		if outgoing_cltv_value > cltv_expiry {
 			return Err(InboundOnionErr {
 				msg: "Upstream node set CLTV to less than the CLTV set by the sender",
 				err_code: 18,
@@ -2685,7 +2687,7 @@ where
 		// payment logic has enough time to fail the HTLC backward before our onchain logic triggers a
 		// channel closure (see HTLC_FAIL_BACK_BUFFER rationale).
 		let current_height: u32 = self.best_block.read().unwrap().height();
-		if (hop_data.outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 {
+		if (outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 {
 			let mut err_data = Vec::with_capacity(12);
 			err_data.extend_from_slice(&amt_msat.to_be_bytes());
 			err_data.extend_from_slice(&current_height.to_be_bytes());
@@ -2694,8 +2696,8 @@ where
 				msg: "The final CLTV expiry is too soon to handle",
 			});
 		}
-		if (!allow_underpay && hop_data.amt_to_forward > amt_msat) ||
-			(allow_underpay && hop_data.amt_to_forward >
+		if (!allow_underpay && onion_amt_msat > amt_msat) ||
+			(allow_underpay && onion_amt_msat >
 			 amt_msat.saturating_add(counterparty_skimmed_fee_msat.unwrap_or(0)))
 		{
 			return Err(InboundOnionErr {
@@ -2731,13 +2733,13 @@ where
 				payment_data,
 				payment_preimage,
 				payment_metadata,
-				incoming_cltv_expiry: hop_data.outgoing_cltv_value,
+				incoming_cltv_expiry: outgoing_cltv_value,
 			}
 		} else if let Some(data) = payment_data {
 			routing = PendingHTLCRouting::Receive {
 				payment_data: data,
 				payment_metadata,
-				incoming_cltv_expiry: hop_data.outgoing_cltv_value,
+				incoming_cltv_expiry: outgoing_cltv_value,
 				phantom_shared_secret,
 			}
 		} else {
@@ -2752,8 +2754,8 @@ where
 			payment_hash,
 			incoming_shared_secret: shared_secret,
 			incoming_amt_msat: Some(amt_msat),
-			outgoing_amt_msat: hop_data.amt_to_forward,
-			outgoing_cltv_value: hop_data.outgoing_cltv_value,
+			outgoing_amt_msat: onion_amt_msat,
+			outgoing_cltv_value,
 			skimmed_fee_msat: counterparty_skimmed_fee_msat,
 		})
 	}
@@ -2817,9 +2819,8 @@ where
 		};
 		let (outgoing_scid, outgoing_amt_msat, outgoing_cltv_value, next_packet_pk_opt) = match next_hop {
 			onion_utils::Hop::Forward {
-				next_hop_data: msgs::OnionHopData {
-					format: msgs::OnionHopDataFormat::NonFinalNode { short_channel_id }, amt_to_forward,
-					outgoing_cltv_value,
+				next_hop_data: msgs::InboundPayload::Forward {
+					short_channel_id, amt_to_forward, outgoing_cltv_value
 				}, ..
 			} => {
 				let next_pk = onion_utils::next_hop_packet_pubkey(&self.secp_ctx,
@@ -2829,9 +2830,7 @@ where
 			// We'll do receive checks in [`Self::construct_pending_htlc_info`] so we have access to the
 			// inbound channel's state.
 			onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret, None)),
-			onion_utils::Hop::Forward {
-				next_hop_data: msgs::OnionHopData { format: msgs::OnionHopDataFormat::FinalNode { .. }, .. }, ..
-			} => {
+			onion_utils::Hop::Forward { next_hop_data: msgs::InboundPayload::Receive { .. }, .. } => {
 				return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]);
 			}
 		};
@@ -10021,16 +10020,14 @@ mod tests {
 		let node = create_network(1, &node_cfg, &node_chanmgr);
 		let sender_intended_amt_msat = 100;
 		let extra_fee_msat = 10;
-		let hop_data = msgs::OnionHopData {
-			amt_to_forward: 100,
+		let hop_data = msgs::InboundPayload::Receive {
+			amt_msat: 100,
 			outgoing_cltv_value: 42,
-			format: msgs::OnionHopDataFormat::FinalNode {
-				keysend_preimage: None,
-				payment_metadata: None,
-				payment_data: Some(msgs::FinalOnionHopData {
-					payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat,
-				}),
-			}
+			payment_metadata: None,
+			keysend_preimage: None,
+			payment_data: Some(msgs::FinalOnionHopData {
+				payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat,
+			}),
 		};
 		// Check that if the amount we received + the penultimate hop extra fee is less than the sender
 		// intended amount, we fail the payment.
@@ -10042,16 +10039,14 @@ mod tests {
 		} else { panic!(); }
 
 		// If amt_received + extra_fee is equal to the sender intended amount, we're fine.
-		let hop_data = msgs::OnionHopData { // This is the same hop_data as above, OnionHopData doesn't implement Clone
-			amt_to_forward: 100,
+		let hop_data = msgs::InboundPayload::Receive { // This is the same payload as above, InboundPayload doesn't implement Clone
+			amt_msat: 100,
 			outgoing_cltv_value: 42,
-			format: msgs::OnionHopDataFormat::FinalNode {
-				keysend_preimage: None,
-				payment_metadata: None,
-				payment_data: Some(msgs::FinalOnionHopData {
-					payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat,
-				}),
-			}
+			payment_metadata: None,
+			keysend_preimage: None,
+			payment_data: Some(msgs::FinalOnionHopData {
+				payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat,
+			}),
 		};
 		assert!(node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
 			sender_intended_amt_msat - extra_fee_msat, 42, None, true, Some(extra_fee_msat)).is_ok());

lightning/src/ln/msgs.rs

@@ -1419,11 +1419,26 @@ mod fuzzy_internal_msgs {
 	// These types aren't intended to be pub, but are exposed for direct fuzzing (as we deserialize
 	// them from untrusted input):
 	#[derive(Clone)]
-	pub(crate) struct FinalOnionHopData {
-		pub(crate) payment_secret: PaymentSecret,
+	pub struct FinalOnionHopData {
+		pub payment_secret: PaymentSecret,
 		/// The total value, in msat, of the payment as received by the ultimate recipient.
 		/// Message serialization may panic if this value is more than 21 million Bitcoin.
-		pub(crate) total_msat: u64,
+		pub total_msat: u64,
+	}
+
+	pub enum InboundPayload {
+		Forward {
+			short_channel_id: u64,
+			amt_to_forward: u64,
+			outgoing_cltv_value: u32,
+		},
+		Receive {
+			payment_data: Option<FinalOnionHopData>,
+			payment_metadata: Option<Vec<u8>>,
+			keysend_preimage: Option<PaymentPreimage>,
+			amt_msat: u64,
+			outgoing_cltv_value: u32,
+		},
 	}
 
 	pub(crate) enum OnionHopDataFormat {
@@ -1975,7 +1990,7 @@ impl Writeable for OnionHopData {
 	}
 }
 
-impl Readable for OnionHopData {
+impl Readable for InboundPayload {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
 		let mut amt = HighZeroBytesDroppedBigSize(0u64);
 		let mut cltv_value = HighZeroBytesDroppedBigSize(0u32);
@@ -1993,39 +2008,35 @@ impl Readable for OnionHopData {
 			(5482373484, keysend_preimage, option)
 		});
 
-		let format = if let Some(short_channel_id) = short_id {
-			if payment_data.is_some() { return Err(DecodeError::InvalidValue); }
+		if amt.0 > MAX_VALUE_MSAT { return Err(DecodeError::InvalidValue) }
+		if let Some(short_channel_id) = short_id {
+			if payment_data.is_some() { return Err(DecodeError::InvalidValue) }
 			if payment_metadata.is_some() { return Err(DecodeError::InvalidValue); }
-			OnionHopDataFormat::NonFinalNode {
+			Ok(Self::Forward {
 				short_channel_id,
-			}
+				amt_to_forward: amt.0,
+				outgoing_cltv_value: cltv_value.0,
+			})
 		} else {
 			if let Some(data) = &payment_data {
 				if data.total_msat > MAX_VALUE_MSAT {
 					return Err(DecodeError::InvalidValue);
 				}
 			}
-			OnionHopDataFormat::FinalNode {
+			Ok(Self::Receive {
 				payment_data,
 				payment_metadata: payment_metadata.map(|w| w.0),
 				keysend_preimage,
-			}
-		};
-
-		if amt.0 > MAX_VALUE_MSAT {
-			return Err(DecodeError::InvalidValue);
+				amt_msat: amt.0,
+				outgoing_cltv_value: cltv_value.0,
+			})
 		}
-		Ok(OnionHopData {
-			format,
-			amt_to_forward: amt.0,
-			outgoing_cltv_value: cltv_value.0,
-		})
 	}
 }
 
 // ReadableArgs because we need onion_utils::decode_next_hop to accommodate payment packets and
 // onion message packets.
-impl ReadableArgs<()> for OnionHopData {
+impl ReadableArgs<()> for InboundPayload {
 	fn read<R: Read>(r: &mut R, _arg: ()) -> Result<Self, DecodeError> {
 		<Self as Readable>::read(r)
 	}
@@ -3526,7 +3537,7 @@ mod tests {
 
 	#[test]
 	fn encoding_nonfinal_onion_hop_data() {
-		let mut msg = msgs::OnionHopData {
+		let msg = msgs::OnionHopData {
 			format: OnionHopDataFormat::NonFinalNode {
 				short_channel_id: 0xdeadbeef1bad1dea,
 			},
@@ -3536,17 +3547,18 @@ mod tests {
 		let encoded_value = msg.encode();
 		let target_value = hex::decode("1a02080badf00d010203040404ffffffff0608deadbeef1bad1dea").unwrap();
 		assert_eq!(encoded_value, target_value);
-		msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
-		if let OnionHopDataFormat::NonFinalNode { short_channel_id } = msg.format {
+
+		let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+		if let msgs::InboundPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } = inbound_msg {
 			assert_eq!(short_channel_id, 0xdeadbeef1bad1dea);
+			assert_eq!(amt_to_forward, 0x0badf00d01020304);
+			assert_eq!(outgoing_cltv_value, 0xffffffff);
 		} else { panic!(); }
-		assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
-		assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
 	}
 
 	#[test]
 	fn encoding_final_onion_hop_data() {
-		let mut msg = msgs::OnionHopData {
+		let msg = msgs::OnionHopData {
 			format: OnionHopDataFormat::FinalNode {
 				payment_data: None,
 				payment_metadata: None,
@@ -3558,16 +3570,18 @@ mod tests {
 		let encoded_value = msg.encode();
 		let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap();
 		assert_eq!(encoded_value, target_value);
-		msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
-		if let OnionHopDataFormat::FinalNode { payment_data: None, .. } = msg.format { } else { panic!(); }
-		assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
-		assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
+
+		let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+		if let msgs::InboundPayload::Receive { payment_data: None, amt_msat, outgoing_cltv_value, .. } = inbound_msg {
+			assert_eq!(amt_msat, 0x0badf00d01020304);
+			assert_eq!(outgoing_cltv_value, 0xffffffff);
+		} else { panic!(); }
 	}
 
 	#[test]
 	fn encoding_final_onion_hop_data_with_secret() {
 		let expected_payment_secret = PaymentSecret([0x42u8; 32]);
-		let mut msg = msgs::OnionHopData {
+		let msg = msgs::OnionHopData {
 			format: OnionHopDataFormat::FinalNode {
 				payment_data: Some(FinalOnionHopData {
 					payment_secret: expected_payment_secret,
@@ -3582,19 +3596,21 @@ mod tests {
 		let encoded_value = msg.encode();
 		let target_value = hex::decode("3602080badf00d010203040404ffffffff082442424242424242424242424242424242424242424242424242424242424242421badca1f").unwrap();
 		assert_eq!(encoded_value, target_value);
-		msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
-		if let OnionHopDataFormat::FinalNode {
+
+		let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+		if let msgs::InboundPayload::Receive {
 			payment_data: Some(FinalOnionHopData {
 				payment_secret,
 				total_msat: 0x1badca1f
 			}),
+			amt_msat, outgoing_cltv_value,
 			payment_metadata: None,
 			keysend_preimage: None,
-		} = msg.format {
+		} = inbound_msg  {
 			assert_eq!(payment_secret, expected_payment_secret);
+			assert_eq!(amt_msat, 0x0badf00d01020304);
+			assert_eq!(outgoing_cltv_value, 0xffffffff);
 		} else { panic!(); }
-		assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
-		assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
 	}
 
 	#[test]
@@ -3744,7 +3760,7 @@ mod tests {
 		// payload length to be encoded over multiple bytes rather than a single u8.
 		let big_payload = encode_big_payload().unwrap();
 		let mut rd = Cursor::new(&big_payload[..]);
-		<msgs::OnionHopData as Readable>::read(&mut rd).unwrap();
+		<msgs::InboundPayload as Readable>::read(&mut rd).unwrap();
 	}
 	// see above test, needs to be a separate method for use of the serialization macros.
 	fn encode_big_payload() -> Result<Vec<u8>, io::Error> {