Add test coverage for serialization of malformed HTLCs.

in Channel and ChannelManager.

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -8264,6 +8264,7 @@ mod tests {
 	use bitcoin::blockdata::transaction::{Transaction, TxOut};
 	use bitcoin::blockdata::opcodes;
 	use bitcoin::network::constants::Network;
+	use crate::ln::onion_utils::INVALID_ONION_BLINDING;
 	use crate::ln::{PaymentHash, PaymentPreimage};
 	use crate::ln::channel_keys::{RevocationKey, RevocationBasepoint};
 	use crate::ln::channelmanager::{self, HTLCSource, PaymentId};
@@ -8800,8 +8801,9 @@ mod tests {
 	}
 
 	#[test]
-	fn blinding_point_skimmed_fee_ser() {
-		// Ensure that channel blinding points and skimmed fees are (de)serialized properly.
+	fn blinding_point_skimmed_fee_malformed_ser() {
+		// Ensure that channel blinding points, skimmed fees, and malformed HTLCs are (de)serialized
+		// properly.
 		let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
 		let secp_ctx = Secp256k1::new();
 		let seed = [42; 32];
@@ -8866,13 +8868,18 @@ mod tests {
 			payment_preimage: PaymentPreimage([42; 32]),
 			htlc_id: 0,
 		};
-		let mut holding_cell_htlc_updates = Vec::with_capacity(10);
-		for i in 0..10 {
-			if i % 3 == 0 {
+		let dummy_holding_cell_malformed_htlc = HTLCUpdateAwaitingACK::FailMalformedHTLC {
+			htlc_id: 0,
+			failure_code: INVALID_ONION_BLINDING,
+			sha256_of_onion: [0; 32],
+		};
+		let mut holding_cell_htlc_updates = Vec::with_capacity(12);
+		for i in 0..12 {
+			if i % 4 == 0 {
 				holding_cell_htlc_updates.push(dummy_holding_cell_add_htlc.clone());
-			} else if i % 3 == 1 {
+			} else if i % 4 == 1 {
 				holding_cell_htlc_updates.push(dummy_holding_cell_claim_htlc.clone());
-			} else {
+			} else if i % 4 == 2 {
 				let mut dummy_add = dummy_holding_cell_add_htlc.clone();
 				if let HTLCUpdateAwaitingACK::AddHTLC {
 					ref mut blinding_point, ref mut skimmed_fee_msat, ..
@@ -8881,6 +8888,8 @@ mod tests {
 					*skimmed_fee_msat = Some(42);
 				} else { panic!() }
 				holding_cell_htlc_updates.push(dummy_add);
+			} else {
+				holding_cell_htlc_updates.push(dummy_holding_cell_malformed_htlc.clone());
 			}
 		}
 		chan.context.holding_cell_htlc_updates = holding_cell_htlc_updates.clone();
@@ -8892,6 +8901,17 @@ mod tests {
 		let features = channelmanager::provided_channel_type_features(&config);
 		let decoded_chan = Channel::read(&mut reader, (&&keys_provider, &&keys_provider, 0, &features)).unwrap();
 		assert_eq!(decoded_chan.context.pending_outbound_htlcs, pending_outbound_htlcs);
+
+		// As part of maintaining support for downgrades, malformed holding cell HTLCs are serialized in
+		// `Channel` TLVs and thus will be shuffled to the end of `holding_cell_htlc_updates` on read.
+		holding_cell_htlc_updates.sort_by(|a, b| {
+			let a_is_malformed = if let HTLCUpdateAwaitingACK::FailMalformedHTLC { .. } = a { true } else { false };
+			let b_is_malformed = if let HTLCUpdateAwaitingACK::FailMalformedHTLC { .. } = b { true } else { false };
+			if a_is_malformed && !b_is_malformed { return core::cmp::Ordering::Greater }
+			else if !a_is_malformed && b_is_malformed { return core::cmp::Ordering::Less }
+			core::cmp::Ordering::Equal
+		});
+
 		assert_eq!(decoded_chan.context.holding_cell_htlc_updates, holding_cell_htlc_updates);
 	}
 

lightning/src/ln/channelmanager.rs

@@ -111,6 +111,7 @@ use crate::ln::script::ShutdownScript;
 
 /// Information about where a received HTLC('s onion) has indicated the HTLC should go.
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
+#[cfg_attr(test, derive(Debug, PartialEq))]
 pub enum PendingHTLCRouting {
 	/// An HTLC which should be forwarded on to another node.
 	Forward {
@@ -189,7 +190,7 @@ pub enum PendingHTLCRouting {
 }
 
 /// Information used to forward or fail this HTLC that is being forwarded within a blinded path.
-#[derive(Clone, Copy, Hash, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub struct BlindedForward {
 	/// The `blinding_point` that was set in the inbound [`msgs::UpdateAddHTLC`], or in the inbound
 	/// onion payload if we're the introduction node. Useful for calculating the next hop's
@@ -213,6 +214,7 @@ impl PendingHTLCRouting {
 /// Information about an incoming HTLC, including the [`PendingHTLCRouting`] describing where it
 /// should go next.
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
+#[cfg_attr(test, derive(Debug, PartialEq))]
 pub struct PendingHTLCInfo {
 	/// Further routing details based on whether the HTLC is being forwarded or received.
 	pub routing: PendingHTLCRouting,
@@ -267,6 +269,7 @@ pub(super) enum PendingHTLCStatus {
 	Fail(HTLCFailureMsg),
 }
 
+#[cfg_attr(test, derive(Clone, Debug, PartialEq))]
 pub(super) struct PendingAddHTLCInfo {
 	pub(super) forward_info: PendingHTLCInfo,
 
@@ -282,6 +285,7 @@ pub(super) struct PendingAddHTLCInfo {
 	prev_user_channel_id: u128,
 }
 
+#[cfg_attr(test, derive(Clone, Debug, PartialEq))]
 pub(super) enum HTLCForwardInfo {
 	AddHTLC(PendingAddHTLCInfo),
 	FailHTLC {
@@ -11064,12 +11068,14 @@ mod tests {
 	use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
 	use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 	use crate::ln::ChannelId;
-	use crate::ln::channelmanager::{create_recv_pending_htlc_info, inbound_payment, PaymentId, PaymentSendFailure, RecipientOnionFields, InterceptId};
+	use crate::ln::channelmanager::{create_recv_pending_htlc_info, HTLCForwardInfo, inbound_payment, PaymentId, PaymentSendFailure, RecipientOnionFields, InterceptId};
 	use crate::ln::functional_test_utils::*;
 	use crate::ln::msgs::{self, ErrorAction};
 	use crate::ln::msgs::ChannelMessageHandler;
+	use crate::prelude::*;
 	use crate::routing::router::{PaymentParameters, RouteParameters, find_route};
 	use crate::util::errors::APIError;
+	use crate::util::ser::Writeable;
 	use crate::util::test_utils;
 	use crate::util::config::{ChannelConfig, ChannelConfigUpdate};
 	use crate::sign::EntropySource;
@@ -12344,6 +12350,69 @@ mod tests {
 			check_spends!(txn[0], funding_tx);
 		}
 	}
+
+	#[test]
+	fn test_malformed_forward_htlcs_ser() {
+		// Ensure that `HTLCForwardInfo::FailMalformedHTLC`s are (de)serialized properly.
+		let chanmon_cfg = create_chanmon_cfgs(1);
+		let node_cfg = create_node_cfgs(1, &chanmon_cfg);
+		let persister;
+		let chain_monitor;
+		let chanmgrs = create_node_chanmgrs(1, &node_cfg, &[None]);
+		let deserialized_chanmgr;
+		let mut nodes = create_network(1, &node_cfg, &chanmgrs);
+
+		let dummy_failed_htlc = HTLCForwardInfo::FailHTLC {
+			htlc_id: 0, err_packet: msgs::OnionErrorPacket { data: vec![0] },
+		};
+
+		let dummy_malformed_htlc = HTLCForwardInfo::FailMalformedHTLC {
+			htlc_id: 0, failure_code: 0x4000, sha256_of_onion: [0; 32]
+		};
+
+		let dummy_htlcs_1 = vec![
+			dummy_failed_htlc.clone(), dummy_failed_htlc.clone(), dummy_malformed_htlc.clone(),
+			dummy_malformed_htlc.clone(), dummy_failed_htlc.clone(), dummy_malformed_htlc.clone()
+		];
+		let dummy_htlcs_2 = vec![
+			dummy_malformed_htlc.clone(), dummy_failed_htlc.clone(), dummy_malformed_htlc.clone(),
+			dummy_failed_htlc.clone()
+		];
+
+		let (scid_1, scid_2) = (42, 43);
+		let mut forward_htlcs = HashMap::new();
+		forward_htlcs.insert(scid_1, dummy_htlcs_1.clone());
+		forward_htlcs.insert(scid_2, dummy_htlcs_2.clone());
+
+		let mut chanmgr_fwd_htlcs = nodes[0].node.forward_htlcs.lock().unwrap();
+		*chanmgr_fwd_htlcs = forward_htlcs.clone();
+		core::mem::drop(chanmgr_fwd_htlcs);
+
+		reload_node!(nodes[0], nodes[0].node.encode(), &[], persister, chain_monitor, deserialized_chanmgr);
+
+		// As part of maintaining support for downgrades, malformed HTLCs are serialized in TLVs and
+		// thus will be shuffled to the end in `forward_htlcs` on read.
+		let sort_htlcs: fn(&HTLCForwardInfo, &HTLCForwardInfo) -> _ = |a, b| {
+			let a_is_malformed = if let HTLCForwardInfo::FailMalformedHTLC { .. } = a { true } else { false };
+			let b_is_malformed = if let HTLCForwardInfo::FailMalformedHTLC { .. } = b { true } else { false };
+			if a_is_malformed && !b_is_malformed { return core::cmp::Ordering::Greater }
+			else if !a_is_malformed && b_is_malformed { return core::cmp::Ordering::Less }
+			core::cmp::Ordering::Equal
+		};
+		for htlcs in forward_htlcs.values_mut() {
+			htlcs.sort_by(sort_htlcs);
+		}
+
+		let mut deserialized_fwd_htlcs = nodes[0].node.forward_htlcs.lock().unwrap();
+		for scid in [scid_1, scid_2].iter() {
+			let deserialized_htlcs = deserialized_fwd_htlcs.remove(scid).unwrap();
+			assert_eq!(forward_htlcs.remove(scid).unwrap(), deserialized_htlcs);
+		}
+		assert!(deserialized_fwd_htlcs.is_empty());
+		core::mem::drop(deserialized_fwd_htlcs);
+
+		expect_pending_htlcs_forwardable!(nodes[0]);
+	}
 }
 
 #[cfg(ldk_bench)]

lightning/src/ln/msgs.rs

@@ -1678,6 +1678,7 @@ 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)]
+	#[cfg_attr(test, derive(Debug, PartialEq))]
 	pub struct FinalOnionHopData {
 		pub payment_secret: PaymentSecret,
 		/// The total value, in msat, of the payment as received by the ultimate recipient.