feedback from val

Author: Evanfeenstra

lightning/src/ln/channelmanager.rs

@@ -136,7 +136,7 @@ pub enum PendingHTLCRouting {
 		payment_data: Option<msgs::FinalOnionHopData>,
 		/// Preimage for this onion payment.
 		payment_preimage: PaymentPreimage,
-		/// Metadata associated with this payment.
+		/// See [`RecipientOnionFields::payment_metadata`] for more info.
 		payment_metadata: Option<Vec<u8>>,
 		/// CLTV expiry of the incoming HTLC.
 		incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
@@ -3030,10 +3030,16 @@ where
 
 			let cur_height = self.best_block.read().unwrap().height() + 1;
 
-			if let Err((err_msg, code, include_chan_update_opt)) = check_incoming_htlc_cltv(
+			if let Err((err_msg, code)) = check_incoming_htlc_cltv(
 				cur_height, outgoing_cltv_value, msg.cltv_expiry
 			) {
-				let chan_update_opt = if include_chan_update_opt { chan_update_opt } else { None };
+				if code & 0x1000 != 0 && chan_update_opt.is_none() {
+					// We really should set `incorrect_cltv_expiry` here but as we're not
+					// forwarding over a real channel we can't generate a channel_update
+					// for it. Instead we just return a generic temporary_node_failure.
+					break Some((err_msg, 0x2000 | 2, None))
+				}
+				let chan_update_opt = if code & 0x1000 != 0 { chan_update_opt } else { None };
 				break Some((err_msg, code, chan_update_opt));
 			}
 
@@ -7892,7 +7898,7 @@ where
 				}),
 			};
 
-			if let Err((err_msg, code, _)) = check_incoming_htlc_cltv(
+			if let Err((err_msg, code)) = check_incoming_htlc_cltv(
 				cur_height, outgoing_cltv_value, msg.cltv_expiry
 			) {
 				return Err(InboundOnionErr {
@@ -7924,9 +7930,7 @@ struct NextPacketDetails {
 
 fn decode_incoming_update_add_htlc_onion<NS: Deref, L: Deref, T: secp256k1::Verification>(
 	msg: &msgs::UpdateAddHTLC, node_signer: &NS, logger: &L, secp_ctx: &Secp256k1<T>,
-) -> Result<(
-	onion_utils::Hop, [u8; 32], Option<NextPacketDetails>
-), HTLCFailureMsg>
+) -> Result<(onion_utils::Hop, [u8; 32], Option<NextPacketDetails>), HTLCFailureMsg>
 where
 	NS::Target: NodeSigner,
 	L::Target: Logger,
@@ -8002,6 +8006,8 @@ where
 				outgoing_amt_msat: amt_to_forward, outgoing_cltv_value
 			}
 		},
+		// 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::InboundOnionPayload::Receive { .. }, .. } |
 			onion_utils::Hop::Forward { next_hop_data: msgs::InboundOnionPayload::BlindedReceive { .. }, .. } =>
@@ -8015,24 +8021,21 @@ where
 
 fn check_incoming_htlc_cltv(
 	cur_height: u32, outgoing_cltv_value: u32, cltv_expiry: u32
-) -> Result<(), (&'static str, u16, bool)> {
+) -> Result<(), (&'static str, u16)> {
 	if (cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
-		// We really should set `incorrect_cltv_expiry` here but as we're not
-		// forwarding over a real channel we can't generate a channel_update
-		// for it. Instead we just return a generic temporary_node_failure.
 		return Err((
-				"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
-				0x2000 | 2, false,
+			"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
+			0x1000 | 13, // incorrect_cltv_expiry
 		));
 	}
 	// Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
 	// but we want to be robust wrt to counterparty packet sanitization (see
 	// HTLC_FAIL_BACK_BUFFER rationale).
 	if cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
-		return Err(("CLTV expiry is too close", 0x1000 | 14, true));
+		return Err(("CLTV expiry is too close", 0x1000 | 14));
 	}
 	if cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
-		return Err(("CLTV expiry is too far in the future", 21, false));
+		return Err(("CLTV expiry is too far in the future", 21));
 	}
 	// If the HTLC expires ~now, don't bother trying to forward it to our
 	// counterparty. They should fail it anyway, but we don't want to bother with
@@ -8043,7 +8046,7 @@ fn check_incoming_htlc_cltv(
 	// but there is no need to do that, and since we're a bit conservative with our
 	// risk threshold it just results in failing to forward payments.
 	if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
-		return Err(("Outgoing CLTV value is too soon", 0x1000 | 14, true));
+		return Err(("Outgoing CLTV value is too soon", 0x1000 | 14));
 	}
 
 	Ok(())
@@ -12205,7 +12208,7 @@ mod tests {
 		let charlie = crate::sign::KeysManager::new(&[3; 32], 42, 42);
 		let charlie_pk = PublicKey::from_secret_key(&secp_ctx, &charlie.get_node_secret_key());
 
-		let (session_priv, total_amt_msat, cur_height, recipient_onion, preimage, payment_hash, 
+		let (session_priv, total_amt_msat, cur_height, recipient_onion, preimage, payment_hash,
 			prng_seed, hops, recipient_amount, pay_secret) = payment_onion_args(bob_pk, charlie_pk);
 
 		let path = Path {
@@ -12220,9 +12223,9 @@ 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)
-			.map_err(|e| e.msg).unwrap();		
+			.map_err(|e| e.msg).unwrap();
 
 		let next_onion = match peeled.routing {
 			PendingHTLCRouting::Forward { onion_packet, short_channel_id: _ } => {
@@ -12233,8 +12236,8 @@ 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)
-			.map_err(|e| e.msg).unwrap();	
-			
+			.map_err(|e| e.msg).unwrap();
+
 		match peeled2.routing {
 			PendingHTLCRouting::ReceiveKeysend { payment_preimage, payment_data, incoming_cltv_expiry, .. } => {
 				assert_eq!(payment_preimage, preimage);
@@ -12249,7 +12252,7 @@ mod tests {
 	}
 
 	fn make_update_add_msg(
-		amount_msat: u64, cltv_expiry: u32, payment_hash: PaymentHash, 
+		amount_msat: u64, cltv_expiry: u32, payment_hash: PaymentHash,
 		onion_routing_packet: msgs::OnionPacket
 	) -> msgs::UpdateAddHTLC {
 		msgs::UpdateAddHTLC {
@@ -12308,7 +12311,7 @@ mod tests {
 	}
 
 	pub fn create_payment_onion<T: bitcoin::secp256k1::Signing>(
-		secp_ctx: &Secp256k1<T>, path: &Path, session_priv: &SecretKey, total_msat: u64, 
+		secp_ctx: &Secp256k1<T>, path: &Path, session_priv: &SecretKey, total_msat: u64,
 		recipient_onion: RecipientOnionFields, best_block_height: u32, payment_hash: PaymentHash,
 		keysend_preimage: Option<PaymentPreimage>, prng_seed: [u8; 32]
 	) -> Result<(u64, u32, msgs::OnionPacket), ()> {