Fail HTLC backwards before upstream claims on-chain

Fail inbound HTLCs if they expire within a certain number of blocks from
the current height. If we haven't seen the preimage for an HTLC by the
time the previous hop's timeout expires, we've lost that HTLC, so we
might as well fail it back instead of having our counterparty
force-close the channel.

Author: alecchendev

lightning/src/chain/channelmonitor.rs

@@ -37,7 +37,7 @@ use crate::ln::{PaymentHash, PaymentPreimage};
 use crate::ln::msgs::DecodeError;
 use crate::ln::chan_utils;
 use crate::ln::chan_utils::{CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction};
-use crate::ln::channelmanager::{HTLCSource, SentHTLCId};
+use crate::ln::channelmanager::{HTLCSource, SentHTLCId, HTLCPreviousHopData};
 use crate::chain;
 use crate::chain::{BestBlock, WatchedOutput};
 use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
@@ -236,6 +236,15 @@ pub const ANTI_REORG_DELAY: u32 = 6;
 /// in a race condition between the user connecting a block (which would fail it) and the user
 /// providing us the preimage (which would claim it).
 pub(crate) const HTLC_FAIL_BACK_BUFFER: u32 = CLTV_CLAIM_BUFFER + LATENCY_GRACE_PERIOD_BLOCKS;
+/// Number of blocks before an inbound HTLC expires at which we fail it backwards.
+///
+/// If we have forwarded an HTLC to a peer and they have not claimed it on or off-chain by the
+/// time the previous hop's HTLC timeout expires, we're probably going to lose the inbound HTLC.
+/// Instead of also losing the channel, we fail the HTLC backwards.
+///
+/// To give us some buffer in case we're slow to process blocks, we fail a few blocks before the
+/// timeout officially expires to ensure we fail back before our counterparty force closes.
+pub(crate) const TIMEOUT_FAIL_BACK_BUFFER: u32 = 3;
 
 // TODO(devrandom) replace this with HolderCommitmentTransaction
 #[derive(Clone, PartialEq, Eq)]
@@ -3309,6 +3318,60 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			}
 		}
 
+		// Fail back HTLCs on backwards channels if they expire within `TIMEOUT_FAIL_BACK_BUFFER`
+		// blocks. If we haven't seen the preimage for an HTLC by the time the previous hop's
+		// timeout expires, we've lost that HTLC, so we might as well fail it back instead of having our
+		// counterparty force-close the channel.
+		let height = self.best_block.height();
+		macro_rules! fail_soon_to_expire_htlcs {
+			($htlcs: expr) => {{
+				for (htlc, source_opt) in $htlcs {
+					// Only check forwarded HTLCs' previous hops
+					let source = match source_opt {
+						Some(source) => source,
+						None => continue,
+					};
+					let cltv_expiry = match source {
+						HTLCSource::PreviousHopData(HTLCPreviousHopData { cltv_expiry: Some(cltv_expiry), .. }) => *cltv_expiry,
+						_ => continue,
+					};
+					if cltv_expiry <= height + TIMEOUT_FAIL_BACK_BUFFER {
+						let duplicate_event = self.pending_monitor_events.iter().any(
+							|update| if let &MonitorEvent::HTLCEvent(ref upd) = update {
+								upd.source == *source
+							} else { false });
+						if !duplicate_event {
+							log_debug!(logger, "Failing back HTLC {} upstream to preserve the \
+								channel as the forward HTLC hasn't resolved and our backward HTLC \
+								expires soon at {}", log_bytes!(htlc.payment_hash.0), cltv_expiry);
+							self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
+								source: source.clone(),
+								payment_preimage: None,
+								payment_hash: htlc.payment_hash,
+								htlc_value_satoshis: Some(htlc.amount_msat / 1000),
+							}));
+						}
+					}
+				}
+			}}
+		}
+
+		let current_holder_htlcs = self.current_holder_commitment_tx.htlc_outputs.iter()
+			.map(|&(ref a, _, ref b)| (a, b.as_ref()));
+		fail_soon_to_expire_htlcs!(current_holder_htlcs);
+
+		if let Some(ref txid) = self.current_counterparty_commitment_txid {
+			if let Some(ref htlc_outputs) = self.counterparty_claimable_outpoints.get(txid) {
+				fail_soon_to_expire_htlcs!(htlc_outputs.iter().map(|&(ref a, ref b)| (a, (b.as_ref().clone()).map(|boxed| &**boxed))));
+			}
+		};
+
+		if let Some(ref txid) = self.prev_counterparty_commitment_txid {
+			if let Some(ref htlc_outputs) = self.counterparty_claimable_outpoints.get(txid) {
+				fail_soon_to_expire_htlcs!(htlc_outputs.iter().map(|&(ref a, ref b)| (a, (b.as_ref().clone()).map(|boxed| &**boxed))));
+			}
+		};
+
 		// Find which on-chain events have reached their confirmation threshold.
 		let onchain_events_awaiting_threshold_conf =
 			self.onchain_events_awaiting_threshold_conf.drain(..).collect::<Vec<_>>();
@@ -3353,20 +3416,24 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						matured_htlcs.push(source.clone());
 					}
 
-					log_debug!(logger, "HTLC {} failure update in {} has got enough confirmations to be passed upstream",
-						log_bytes!(payment_hash.0), entry.txid);
-					self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
-						payment_hash,
-						payment_preimage: None,
-						source: source.clone(),
-						htlc_value_satoshis,
-					}));
-					self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC {
-						commitment_tx_output_idx,
-						resolving_txid: Some(entry.txid),
-						resolving_tx: entry.transaction,
-						payment_preimage: None,
-					});
+					let duplicate_event = self.pending_monitor_events.iter().any(
+						|update| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == *source } else { false });
+					if !duplicate_event {
+						log_debug!(logger, "HTLC {} failure update in {} has got enough confirmations to be passed upstream",
+							log_bytes!(payment_hash.0), entry.txid);
+						self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
+							payment_hash,
+							payment_preimage: None,
+							source: source.clone(),
+							htlc_value_satoshis,
+						}));
+						self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC {
+							commitment_tx_output_idx,
+							resolving_txid: Some(entry.txid),
+							resolving_tx: entry.transaction,
+							payment_preimage: None,
+						});
+					}
 				},
 				OnchainEvent::MaturingOutput { descriptor } => {
 					log_debug!(logger, "Descriptor {} has got enough confirmations to be passed upstream", log_spendable!(descriptor));

lightning/src/ln/functional_tests.rs

@@ -14,7 +14,7 @@
 use crate::chain;
 use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
 use crate::chain::chaininterface::LowerBoundedFeeEstimator;
-use crate::chain::channelmonitor;
+use crate::chain::channelmonitor::{self, TIMEOUT_FAIL_BACK_BUFFER};
 use crate::chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
 use crate::chain::transaction::OutPoint;
 use crate::sign::{ChannelSigner, EcdsaChannelSigner, EntropySource};
@@ -2211,6 +2211,58 @@ fn channel_reserve_in_flight_removes() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_3);
 }
 
+#[test]
+fn test_fail_back_before_backwards_timeout() {
+	// Test that we fail an HTLC upstream if we are still waiting for confirmation downstream
+	// by the time the upstream timeout expires
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	create_announced_chan_between_nodes(&nodes, 0, 1);
+	let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2);
+
+	connect_blocks(&nodes[0], 2*CHAN_CONFIRM_DEPTH + 1 - nodes[0].best_block_info().1);
+	connect_blocks(&nodes[1], 2*CHAN_CONFIRM_DEPTH + 1 - nodes[1].best_block_info().1);
+	connect_blocks(&nodes[2], 2*CHAN_CONFIRM_DEPTH + 1 - nodes[2].best_block_info().1);
+
+	let (_, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3_000_000);
+
+	// Force close downstream with timeout
+	nodes[1].node.force_close_broadcasting_latest_txn(&chan_2.2, &nodes[2].node.get_our_node_id()).unwrap();
+	check_added_monitors!(nodes[1], 1);
+	check_closed_broadcast!(nodes[1], true);
+
+	connect_blocks(&nodes[1], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1);
+	test_txn_broadcast(&nodes[1], &chan_2, None, HTLCType::TIMEOUT);
+	check_closed_event!(nodes[1], 1, ClosureReason::HolderForceClosed);
+
+	// Nothing is confirmed for a while
+	connect_blocks(&nodes[1], MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - TIMEOUT_FAIL_BACK_BUFFER);
+
+	// Check that node 2 fails the HTLC upstream
+	expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_2.2 }]);
+	check_added_monitors!(nodes[1], 1);
+	let events = nodes[1].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	let (update_fail, commitment_signed) = match events[0] {
+		MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {
+			assert!(update_add_htlcs.is_empty());
+			assert!(update_fulfill_htlcs.is_empty());
+			assert_eq!(update_fail_htlcs.len(), 1);
+			assert!(update_fail_malformed_htlcs.is_empty());
+			assert!(update_fee.is_none());
+			(update_fail_htlcs[0].clone(), commitment_signed.clone())
+		},
+		_ => panic!("Unexpected event"),
+	};
+
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &update_fail);
+	commitment_signed_dance!(nodes[0], nodes[1], commitment_signed, false);
+	expect_payment_failed_conditions(&nodes[0], payment_hash_1, false, PaymentFailedConditions::new().blamed_chan_closed(true));
+}
+
 #[test]
 fn channel_monitor_network_test() {
 	// Simple test which builds a network of ChannelManagers, connects them to each other, and
@@ -2307,7 +2359,7 @@ fn channel_monitor_network_test() {
 	let node2_commitment_txid;
 	{
 		let node_txn = test_txn_broadcast(&nodes[2], &chan_3, None, HTLCType::NONE);
-		connect_blocks(&nodes[2], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + MIN_CLTV_EXPIRY_DELTA as u32 + 1);
+		connect_blocks(&nodes[2], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
 		test_txn_broadcast(&nodes[2], &chan_3, None, HTLCType::TIMEOUT);
 		node2_commitment_txid = node_txn[0].txid();
 
@@ -2992,7 +3044,7 @@ fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
 	// Verify that B's ChannelManager is able to detect that HTLC is timeout by its own tx and react backward in consequence
 	mine_transaction(&nodes[1], &commitment_tx[0]);
 	check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false);
-	connect_blocks(&nodes[1], 200 - nodes[2].best_block_info().1);
+	connect_blocks(&nodes[1], 100 - nodes[2].best_block_info().1);
 	let timeout_tx = {
 		let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
 		if nodes[1].connect_style.borrow().skips_blocks() {