Time out incoming HTLCs when we reach cltv_expiry (+ test)

We only do this for incoming HTLCs directly as we rely on channel
closure and HTLC-Timeout broadcast to fail any HTLCs which we
relayed onwards where our next-hop doesn't update_fail in time.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -76,6 +76,7 @@ enum PendingHTLCRouting {
 	},
 	Receive {
 		payment_data: Option<msgs::FinalOnionHopData>,
+		incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
 	},
 }
 
@@ -129,6 +130,7 @@ struct ClaimableHTLC {
 	/// payment_secret which prevents path-probing attacks and can associate different HTLCs which
 	/// are part of the same payment.
 	payment_data: Option<msgs::FinalOnionHopData>,
+	cltv_expiry: u32,
 }
 
 /// Tracks the inbound corresponding to an outbound HTLC
@@ -1063,7 +1065,10 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 				// delay) once they've send us a commitment_signed!
 
 				PendingHTLCStatus::Forward(PendingHTLCInfo {
-					routing: PendingHTLCRouting::Receive { payment_data },
+					routing: PendingHTLCRouting::Receive {
+						payment_data,
+						incoming_cltv_expiry: msg.cltv_expiry,
+					},
 					payment_hash: msg.payment_hash.clone(),
 					incoming_shared_secret: shared_secret,
 					amt_to_forward: next_hop_data.amt_to_forward,
@@ -1705,7 +1710,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 					for forward_info in pending_forwards.drain(..) {
 						match forward_info {
 							HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
-									routing: PendingHTLCRouting::Receive { payment_data },
+									routing: PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry },
 									incoming_shared_secret, payment_hash, amt_to_forward, .. }, } => {
 								let prev_hop = HTLCPreviousHopData {
 									short_channel_id: prev_short_channel_id,
@@ -1722,6 +1727,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 									prev_hop,
 									value: amt_to_forward,
 									payment_data: payment_data.clone(),
+									cltv_expiry: incoming_cltv_expiry,
 								});
 								if let &Some(ref data) = &payment_data {
 									for htlc in htlcs.iter() {
@@ -2983,6 +2989,7 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
 		log_trace!(self, "Block {} at height {} connected with {} txn matched", header_hash, height, txn_matched.len());
 		let _ = self.total_consistency_lock.read().unwrap();
 		let mut failed_channels = Vec::new();
+		let mut timed_out_htlcs = Vec::new();
 		{
 			let mut channel_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_lock;
@@ -3051,10 +3058,29 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
 				}
 				true
 			});
+
+			channel_state.claimable_htlcs.retain(|&(ref payment_hash, _), htlcs| {
+				htlcs.retain(|htlc| {
+					if height >= htlc.cltv_expiry - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS {
+						timed_out_htlcs.push((HTLCSource::PreviousHopData(htlc.prev_hop.clone()), payment_hash.clone(), htlc.value));
+						false
+					} else { true }
+				});
+				!htlcs.is_empty()
+			});
 		}
 		for failure in failed_channels.drain(..) {
 			self.finish_force_close_channel(failure);
 		}
+
+		for (source, payment_hash, value) in timed_out_htlcs.drain(..) {
+			// Call it preimage_unknown as the issue, ultimately, is that the user failed to
+			// provide us a preimage within the cltv_expiry time window.
+			self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), source, &payment_hash, HTLCFailReason::Reason {
+				failure_code: 0x4000 | 15,
+				data: byte_utils::be64_to_array(value).to_vec()
+			});
+		}
 		self.latest_block_height.store(height as usize, Ordering::Release);
 		*self.last_block_hash.try_lock().expect("block_(dis)connected must not be called in parallel") = header_hash;
 		loop {
@@ -3345,9 +3371,10 @@ impl Writeable for PendingHTLCInfo {
 				onion_packet.write(writer)?;
 				short_channel_id.write(writer)?;
 			},
-			&PendingHTLCRouting::Receive { ref payment_data } => {
+			&PendingHTLCRouting::Receive { ref payment_data, ref incoming_cltv_expiry } => {
 				1u8.write(writer)?;
 				payment_data.write(writer)?;
+				incoming_cltv_expiry.write(writer)?;
 			},
 		}
 		self.incoming_shared_secret.write(writer)?;
@@ -3368,6 +3395,7 @@ impl Readable for PendingHTLCInfo {
 				},
 				1u8 => PendingHTLCRouting::Receive {
 					payment_data: Readable::read(reader)?,
+					incoming_cltv_expiry: Readable::read(reader)?,
 				},
 				_ => return Err(DecodeError::InvalidValue),
 			},
@@ -3440,7 +3468,8 @@ impl_writeable!(HTLCPreviousHopData, 0, {
 impl_writeable!(ClaimableHTLC, 0, {
 	prev_hop,
 	value,
-	payment_data
+	payment_data,
+	cltv_expiry
 });
 
 impl Writeable for HTLCSource {

lightning/src/ln/functional_tests.rs

@@ -2325,6 +2325,15 @@ fn claim_htlc_outputs_single_tx() {
 		check_added_monitors!(nodes[0], 1);
 		nodes[1].block_notifier.block_connected(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 200);
 		check_added_monitors!(nodes[1], 1);
+
+		// Expect pending failures, but we don't bother trying to update the channel state with them.
+		let events = nodes[0].node.get_and_clear_pending_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
+			Event::PendingHTLCsForwardable { .. } => { },
+			_ => panic!("Unexpected event"),
+		};
+
 		connect_blocks(&nodes[1].block_notifier, ANTI_REORG_DELAY - 1, 200, true, header.bitcoin_hash());
 
 		let events = nodes[1].node.get_and_clear_pending_events();
@@ -3658,6 +3667,49 @@ fn test_drop_messages_peer_disconnect_dual_htlc() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
+#[test]
+fn test_htlc_timeout() {
+	// If the user fails to claim/fail an HTLC within the HTLC CLTV timeout we fail it for them
+	// to avoid our counterparty failing the channel.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::supported(), InitFeatures::supported());
+	let (_, our_payment_hash) = route_payment(&nodes[0], &[&nodes[1]], 100000);
+
+	let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+	nodes[0].block_notifier.block_connected_checked(&header, 101, &[], &[]);
+	nodes[1].block_notifier.block_connected_checked(&header, 101, &[], &[]);
+	for i in 102..TEST_FINAL_CLTV + 100 + 1 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS {
+		header.prev_blockhash = header.bitcoin_hash();
+		nodes[0].block_notifier.block_connected_checked(&header, i, &[], &[]);
+		nodes[1].block_notifier.block_connected_checked(&header, i, &[], &[]);
+	}
+
+	expect_pending_htlcs_forwardable!(nodes[1]);
+
+	check_added_monitors!(nodes[1], 1);
+	let htlc_timeout_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+	assert!(htlc_timeout_updates.update_add_htlcs.is_empty());
+	assert_eq!(htlc_timeout_updates.update_fail_htlcs.len(), 1);
+	assert!(htlc_timeout_updates.update_fail_malformed_htlcs.is_empty());
+	assert!(htlc_timeout_updates.update_fee.is_none());
+
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_timeout_updates.update_fail_htlcs[0]);
+	commitment_signed_dance!(nodes[0], nodes[1], htlc_timeout_updates.commitment_signed, false);
+	let events = nodes[0].node.get_and_clear_pending_events();
+	match events[0] {
+		Event::PaymentFailed { payment_hash, rejected_by_dest, error_code } => {
+			assert_eq!(payment_hash, our_payment_hash);
+			assert!(rejected_by_dest);
+			assert_eq!(error_code.unwrap(), 0x4000 | 15);
+		},
+		_ => panic!("Unexpected event"),
+	}
+}
+
 #[test]
 fn test_invalid_channel_announcement() {
 	//Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs
@@ -7145,6 +7197,15 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
 
 	// Broadcast set of revoked txn on A
 	let header_128 = connect_blocks(&nodes[0].block_notifier, 128, 0, true, header.bitcoin_hash());
+
+	// Expect pending failures, but we don't bother trying to update the channel state with them.
+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		Event::PendingHTLCsForwardable { .. } => { },
+		_ => panic!("Unexpected event"),
+	};
+
 	let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 	nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone(), revoked_htlc_txn[1].clone()] }, 129);
 	let first;
@@ -7477,6 +7538,15 @@ fn test_bump_txn_sanitize_tracking_maps() {
 
 	// Broadcast set of revoked txn on A
 	let header_128 = connect_blocks(&nodes[0].block_notifier, 128, 0,  false, Default::default());
+
+	// Expect pending failures, but we don't bother trying to update the channel state with them.
+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		Event::PendingHTLCsForwardable { .. } => { },
+		_ => panic!("Unexpected event"),
+	};
+
 	let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 	nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_local_txn[0].clone()] }, 129);
 	check_closed_broadcast!(nodes[0], false);

lightning/src/util/events.rs

@@ -55,6 +55,9 @@ pub enum Event {
 	/// ChannelManager::fail_htlc_backwards to free up resources for this HTLC.
 	/// The amount paid should be considered 'incorrect' when it is less than or more than twice
 	/// the amount expected.
+	/// If you fail to call either ChannelManager::claim_funds of
+	/// ChannelManager::fail_htlc_backwards within the HTLC's timeout, the HTLC will be
+	/// automatically failed with PaymentFailReason::PreimageUnknown.
 	PaymentReceived {
 		/// The hash for which the preimage should be handed to the ChannelManager.
 		payment_hash: PaymentHash,