Correctly detect missing HTLCs when a local commitment tx was broadcast

If we forward an HTLC to our counterparty, but we force-closed the
channel before our counterparty provides us an updated commitment
transaction, we'll end up with a commitment transaction that does
not contain the HTLC which we attempted to forward. In this case,
we need to wait `ANTI_REORG_DELAY` blocks and then fail back the
HTLC as there is no way for us to learn the preimage and the
confirmed commitment transaction paid us the value of the HTLC.

However, check_spend_holder_transaction did not do this - it
instead only looked for dust HTLCs in the confirmed commitment
transaction, paying no attention to what other HTLCs may exist that
are missed.

This will eventually lead to channel force-closure as the channel
on which we received the inbound HTLC to forward will be closed in
time for the initial sender to claim the HTLC on-chain.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -1758,27 +1758,6 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		let mut claim_requests = Vec::new();
 		let mut watch_outputs = Vec::new();
 
-		macro_rules! wait_threshold_conf {
-			($source: expr, $commitment_tx: expr, $payment_hash: expr) => {
-				self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
-					if entry.height != height { return true; }
-					match entry.event {
-						OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
-							*update_source != $source
-						},
-						_ => true,
-					}
-				});
-				let entry = OnchainEventEntry {
-					txid: commitment_txid,
-					height,
-					event: OnchainEvent::HTLCUpdate { source: $source, payment_hash: $payment_hash },
-				};
-				log_trace!(logger, "Failing HTLC with payment_hash {} from {} holder commitment tx due to broadcast of transaction, waiting confirmation (at height{})", log_bytes!($payment_hash.0), $commitment_tx, entry.confirmation_threshold());
-				self.onchain_events_awaiting_threshold_conf.push(entry);
-			}
-		}
-
 		macro_rules! append_onchain_update {
 			($updates: expr, $to_watch: expr) => {
 				claim_requests = $updates.0;
@@ -1796,33 +1775,19 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 			let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
 			let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
 			append_onchain_update!(res, to_watch);
+			fail_unbroadcast_htlcs!(self, "latest holder", height, self.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
 		} else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
 			if holder_tx.txid == commitment_txid {
 				is_holder_tx = true;
 				log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
 				let res = self.get_broadcasted_holder_claims(holder_tx, height);
 				let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
 				append_onchain_update!(res, to_watch);
-			}
-		}
-
-		macro_rules! fail_dust_htlcs_after_threshold_conf {
-			($holder_tx: expr) => {
-				for &(ref htlc, _, ref source) in &$holder_tx.htlc_outputs {
-					if htlc.transaction_output_index.is_none() {
-						if let &Some(ref source) = source {
-							wait_threshold_conf!(source.clone(), "lastest", htlc.payment_hash.clone());
-						}
-					}
-				}
+				fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
 			}
 		}
 
 		if is_holder_tx {
-			fail_dust_htlcs_after_threshold_conf!(self.current_holder_commitment_tx);
-			if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
-				fail_dust_htlcs_after_threshold_conf!(holder_tx);
-			}
 		}
 
 		(claim_requests, (commitment_txid, watch_outputs))

lightning/src/ln/mod.rs

@@ -52,6 +52,8 @@ mod reorg_tests;
 #[cfg(test)]
 #[allow(unused_mut)]
 mod onion_route_tests;
+#[cfg(test)]
+mod monitor_tests;
 
 pub use self::peer_channel_encryptor::LN_MAX_MSG_LEN;
 

lightning/src/ln/monitor_tests.rs

@@ -0,0 +1,81 @@
+// This file is Copyright its original authors, visible in version control
+// history.
+//
+// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE
+// or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option.
+// You may not use this file except in accordance with one or both of these
+// licenses.
+
+//! Further functional tests which test blockchain reorganizations.
+
+use chain::channelmonitor::ANTI_REORG_DELAY;
+use ln::{PaymentPreimage, PaymentHash};
+use ln::features::InitFeatures;
+use ln::msgs::{ChannelMessageHandler, HTLCFailChannelUpdate, ErrorAction};
+use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
+use routing::router::get_route;
+
+use bitcoin::hashes::sha256::Hash as Sha256;
+use bitcoin::hashes::Hash;
+
+use prelude::*;
+
+use ln::functional_test_utils::*;
+
+#[test]
+fn chanmon_fail_from_stale_commitment() {
+	// If we forward an HTLC to our counterparty, but we force-closed the channel before our
+	// counterparty provides us an updated commitment transaction, we'll end up with a commitment
+	// transaction that does not contain the HTLC which we attempted to forward. In this case, we
+	// need to wait `ANTI_REORG_DELAY` blocks and then fail back the HTLC as there is no way for us
+	// to learn the preimage and the confirmed commitment transaction paid us the value of the
+	// HTLC.
+	//
+	// However, previously, we did not do this, ignoring the HTLC entirely.
+	//
+	// This could lead to channel closure if the sender we received the HTLC from decides to go on
+	// chain to get their HTLC back before it times out.
+	//
+	// Here, we check exactly this case, forwarding a payment from A, through B, to C, before B
+	// broadcasts its latest commitment transaction, which should result in it eventually failing
+	// the HTLC back off-chain to A.
+	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 mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+	let (update_a, _, chan_id_2, _) = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
+
+	let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
+	nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+	check_added_monitors!(nodes[0], 1);
+
+	let bs_txn = get_local_commitment_txn!(nodes[1], chan_id_2);
+
+	let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
+	commitment_signed_dance!(nodes[1], nodes[0], updates.commitment_signed, false);
+
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
+	check_added_monitors!(nodes[1], 1);
+
+	// Don't bother delivering the new HTLC add/commits, instead confirming the pre-HTLC commitment
+	// transaction for nodes[1].
+	mine_transaction(&nodes[1], &bs_txn[0]);
+	check_added_monitors!(nodes[1], 1);
+	check_closed_broadcast!(nodes[1], true);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	check_added_monitors!(nodes[1], 1);
+	let fail_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates.update_fail_htlcs[0]);
+	commitment_signed_dance!(nodes[0], nodes[1], fail_updates.commitment_signed, true, true);
+	expect_payment_failed!(nodes[0], payment_hash, false);
+	expect_payment_failure_chan_update!(nodes[0], update_a.contents.short_channel_id, true);
+}