feedback from val

Author: Evanfeenstra

lightning/src/ln/channelmanager.rs

@@ -106,31 +106,31 @@ 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 a received HTLC onion.
+/// Routing info for an inbound HTLC onion.
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
 pub enum PendingHTLCRouting {
 	/// A forwarded HTLC.
 	Forward {
-		/// Full Bolt04 onion packet.
+		/// BOLT 4 onion packet.
 		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.
 		short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
-	/// A paid invoice.
+	/// An HTLC paid to an invoice we generated.
 	Receive {
 		/// Payment secret and total msat received.
 		payment_data: msgs::FinalOnionHopData,
-		/// Metadata for this payment.
+		/// See [`RecipientOnionFields::payment_metadata`] for more info.
 		payment_metadata: Option<Vec<u8>>,
-		/// Used to track when we should expire pending HTLCs that go unclaimed
+		/// Used to track when we should expire pending HTLCs that go unclaimed.
 		incoming_cltv_expiry: u32,
 		/// Optional shared secret for phantom node.
 		phantom_shared_secret: Option<[u8; 32]>,
 		/// See [`RecipientOnionFields::custom_tlvs`] for more info.
 		custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
-	/// Incoming keysend (sender included the preimage in a TLV).
+	/// Incoming keysend (sender provided the preimage in a TLV).
 	ReceiveKeysend {
 		/// This was added in 0.0.116 and will break deserialization on downgrades.
 		payment_data: Option<msgs::FinalOnionHopData>,
@@ -148,7 +148,7 @@ pub enum PendingHTLCRouting {
 /// Full details of an incoming HTLC, including routing info.
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
 pub struct PendingHTLCInfo {
-	/// Routing struct of either Forward, Receive, or ReceiveKeysend.
+	/// Further routing details based on whether the HTLC is being forwarded or received.
 	pub routing: PendingHTLCRouting,
 	/// Shared secret from the previous hop.
 	pub incoming_shared_secret: [u8; 32],
@@ -2920,19 +2920,6 @@ where
 		}
 	}
 
-	fn construct_recv_pending_htlc_info(
-		&self, hop_data: msgs::InboundOnionPayload, 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 current_height: u32 = self.best_block.read().unwrap().height();
-		create_recv_pending_htlc_info(
-			hop_data, shared_secret, payment_hash, amt_msat, cltv_expiry, phantom_shared_secret,
-			allow_underpay, counterparty_skimmed_fee_msat, current_height,
-			self.default_configuration.accept_mpp_keysend,
-		)
-	}
-
 	fn decode_update_add_htlc_onion(
 		&self, msg: &msgs::UpdateAddHTLC
 	) -> Result<
@@ -2957,12 +2944,11 @@ where
 		}
 
 		// it is a receive, so no need for outbound checks
-		if let None = next_packet_details_opt {
+		if next_packet_details_opt.is_none() {
 			return Ok((next_hop, shared_secret, None));
 		}
 		let (next_packet_pk, outgoing_scid, outgoing_amt_msat, outgoing_cltv_value) = 
 			next_packet_details_opt.unwrap();
-		let next_packet_pk_opt = Some(next_packet_pk);
 
 		// Perform outbound checks here instead of in [`Self::construct_pending_htlc_info`] because we
 		// can't hold the outbound peer state lock at the same time as the inbound peer state lock.
@@ -2983,7 +2969,7 @@ where
 				},
 				Some((cp_id, id)) => Some((cp_id.clone(), id.clone())),
 			};
-			let (chan_update_opt, check_min_cltv_expiry_delta) = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
+			let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
 				let per_peer_state = self.per_peer_state.read().unwrap();
 				let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id);
 				if peer_state_mutex_opt.is_none() {
@@ -3037,15 +3023,15 @@ where
 					break Some((err, code, chan_update_opt));
 				}
 				// min_cltv_expiry_delta is already checked above in htlc_satisfies_config
-				(chan_update_opt, false)
+				chan_update_opt
 			} else {
-				(None, true)
+				None
 			};
 
 			let cur_height = self.best_block.read().unwrap().height() + 1;
 
 			if let Err((err_msg, code, include_chan_update_opt)) = check_incoming_htlc_cltv(
-				cur_height, outgoing_cltv_value, msg.cltv_expiry, check_min_cltv_expiry_delta
+				cur_height, outgoing_cltv_value, msg.cltv_expiry
 			) {
 				let chan_update_opt = if include_chan_update_opt { chan_update_opt } else { None };
 				break Some((err_msg, code, chan_update_opt));
@@ -3078,7 +3064,7 @@ where
 			}
 			return_err!(err, code, &res.0[..]);
 		}
-		Ok((next_hop, shared_secret, next_packet_pk_opt))
+		Ok((next_hop, shared_secret, Some(next_packet_pk)))
 	}
 
 	fn construct_pending_htlc_status<'a>(
@@ -3101,8 +3087,10 @@ where
 		match decoded_hop {
 			onion_utils::Hop::Receive(next_hop_data) => {
 				// OUR PAYMENT!
-				match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash,
-					msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat)
+				let current_height: u32 = self.best_block.read().unwrap().height();
+				match create_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash,
+					msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat,
+					current_height, self.default_configuration.accept_mpp_keysend)
 				{
 					Ok(info) => {
 						// Note that we could obviously respond immediately with an update_fulfill_htlc
@@ -4141,9 +4129,11 @@ where
 												};
 												match next_hop {
 													onion_utils::Hop::Receive(hop_data) => {
-														match self.construct_recv_pending_htlc_info(hop_data,
+														let current_height: u32 = self.best_block.read().unwrap().height();
+														match create_recv_pending_htlc_info(hop_data,
 															incoming_shared_secret, payment_hash, outgoing_amt_msat,
-															outgoing_cltv_value, Some(phantom_shared_secret), false, None)
+															outgoing_cltv_value, Some(phantom_shared_secret), false, None,
+															current_height, self.default_configuration.accept_mpp_keysend)
 														{
 															Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, prev_user_channel_id, vec![(info, prev_htlc_id)])),
 															Err(InboundOnionErr { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret))
@@ -7891,7 +7881,7 @@ where
 	})?;
 	Ok(match hop {
 		onion_utils::Hop::Forward{ next_hop_data, next_hop_hmac, new_packet_bytes } => {
-			if let None = next_packet_details_opt {
+			if next_packet_details_opt.is_none() {
 				// Forward should always include the next hop details
 				return Err(InboundOnionErr {
 					msg: "Failed to decode update add htlc onion",
@@ -7902,7 +7892,7 @@ where
 			let (next_packet_pubkey, _, _, outgoing_cltv_value) = next_packet_details_opt.unwrap();
 
 			if let Err((err_msg, code, _)) = check_incoming_htlc_cltv(
-				cur_height, outgoing_cltv_value, msg.cltv_expiry, true
+				cur_height, outgoing_cltv_value, msg.cltv_expiry
 			) {
 				return Err(InboundOnionErr {
 					msg: err_msg,
@@ -8019,19 +8009,17 @@ where
 }
 
 fn check_incoming_htlc_cltv(
-	cur_height: u32, outgoing_cltv_value: u32, cltv_expiry: u32, check_min_cltv_expiry_delta: bool
+	cur_height: u32, outgoing_cltv_value: u32, cltv_expiry: u32
 ) -> Result<(), (&'static str, u16, bool)> {
-	if check_min_cltv_expiry_delta {
-		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,
-			));
-		}
-	};
+	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,
+		));
+	}
 	// 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).
@@ -10920,7 +10908,7 @@ mod tests {
 	use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
 	use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 	use crate::ln::ChannelId;
-	use crate::ln::channelmanager::{inbound_payment, PaymentId, PaymentSendFailure, RecipientOnionFields, InterceptId};
+	use crate::ln::channelmanager::{create_recv_pending_htlc_info, inbound_payment, PaymentId, PaymentSendFailure, RecipientOnionFields, InterceptId};
 	use crate::ln::functional_test_utils::*;
 	use crate::ln::msgs::{self, ErrorAction};
 	use crate::ln::msgs::ChannelMessageHandler;
@@ -11922,9 +11910,11 @@ mod tests {
 		};
 		// Check that if the amount we received + the penultimate hop extra fee is less than the sender
 		// intended amount, we fail the payment.
+		let current_height: u32 = node[0].node.best_block.read().unwrap().height();
 		if let Err(crate::ln::channelmanager::InboundOnionErr { err_code, .. }) =
-			node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
-				sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true, Some(extra_fee_msat))
+			create_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
+				sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true, Some(extra_fee_msat),
+				current_height, node[0].node.default_configuration.accept_mpp_keysend)
 		{
 			assert_eq!(err_code, 19);
 		} else { panic!(); }
@@ -11940,8 +11930,10 @@ mod tests {
 			}),
 			custom_tlvs: Vec::new(),
 		};
-		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());
+		let current_height: u32 = node[0].node.best_block.read().unwrap().height();
+		assert!(create_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),
+			current_height, node[0].node.default_configuration.accept_mpp_keysend).is_ok());
 	}
 
 	#[test]
@@ -11951,7 +11943,8 @@ mod tests {
 		let node_chanmgr = create_node_chanmgrs(1, &node_cfg, &[None]);
 		let node = create_network(1, &node_cfg, &node_chanmgr);
 
-		let result = node[0].node.construct_recv_pending_htlc_info(msgs::InboundOnionPayload::Receive {
+		let current_height: u32 = node[0].node.best_block.read().unwrap().height();
+		let result = create_recv_pending_htlc_info(msgs::InboundOnionPayload::Receive {
 			amt_msat: 100,
 			outgoing_cltv_value: 22,
 			payment_metadata: None,
@@ -11960,7 +11953,8 @@ mod tests {
 				payment_secret: PaymentSecret([0; 32]), total_msat: 100,
 			}),
 			custom_tlvs: Vec::new(),
-		}, [0; 32], PaymentHash([0; 32]), 100, 23, None, true, None);
+		}, [0; 32], PaymentHash([0; 32]), 100, 23, None, true, None, current_height,
+			node[0].node.default_configuration.accept_mpp_keysend);
 
 		// Should not return an error as this condition:
 		// https://github.com/lightning/bolts/blob/4dcc377209509b13cf89a4b91fde7d478f5b46d8/04-onion-routing.md?plain=1#L334
@@ -12219,7 +12213,7 @@ mod tests {
 			payment_hash, Some(preimage), prng_seed
 		).unwrap();
 
-		let msg = make_udpate_add_msg(amount_msat, cltv_expiry, payment_hash, onion);
+		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)
@@ -12232,7 +12226,7 @@ mod tests {
 			_ => panic!("expected a forwarded onion"),
 		};
 
-		let msg2 = make_udpate_add_msg(amount_msat, cltv_expiry, payment_hash, next_onion);
+		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();	
 
@@ -12249,7 +12243,7 @@ mod tests {
 		};
 	}
 
-	fn make_udpate_add_msg(
+	fn make_update_add_msg(
 		amount_msat: u64, cltv_expiry: u32, payment_hash: PaymentHash, 
 		onion_routing_packet: msgs::OnionPacket
 	) -> msgs::UpdateAddHTLC {