Avoid force-closing 0-conf channels when funding is reorg'd

When we see a funding transaction for one of our chanels reorg'd
out, we worry that its possible we've been double-spent and
immediately force-close the channel to avoid accepting any more
HTLCs on it. This isn't ideal, but is mostly fine as most nodes
require 6 confirmations and 6 block reorgs are exceedingly rare.

However, this isn't so okay for 0-conf channels - in that case we
elected to trust the funder anyway, so reorgs shouldn't worry us.
Still, to handle this correctly we needed to track the old SCID and
ensure our logic is safe across an SCID change. Luckily, we did
that work for splices, and can now take advantage of it here.

Fixes #3836.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -11344,9 +11344,13 @@ where
 		}
 
 		// Check if the funding transaction was unconfirmed
+		let original_scid = self.funding.short_channel_id;
+		let was_confirmed = self.funding.funding_tx_confirmed_in.is_some();
 		let funding_tx_confirmations = self.funding.get_funding_tx_confirmations(height);
 		if funding_tx_confirmations == 0 {
 			self.funding.funding_tx_confirmation_height = 0;
+			self.funding.short_channel_id = None;
+			self.funding.funding_tx_confirmed_in = None;
 		}
 
 		if let Some(channel_ready) = self.check_get_channel_ready(height, logger) {
@@ -11361,18 +11365,30 @@ where
 			self.context.channel_state.is_our_channel_ready() {
 
 			// If we've sent channel_ready (or have both sent and received channel_ready), and
-			// the funding transaction has become unconfirmed,
-			// close the channel and hope we can get the latest state on chain (because presumably
-			// the funding transaction is at least still in the mempool of most nodes).
+			// the funding transaction has become unconfirmed, we'll probably get a new SCID when
+			// it re-confirms.
 			//
-			// Note that ideally we wouldn't force-close if we see *any* reorg on a 1-conf or
-			// 0-conf channel, but not doing so may lead to the
-			// `ChannelManager::short_to_chan_info` map  being inconsistent, so we currently have
-			// to.
-			if funding_tx_confirmations == 0 && self.funding.funding_tx_confirmed_in.is_some() {
-				let err_reason = format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.",
-					self.context.minimum_depth.unwrap(), funding_tx_confirmations);
-				return Err(ClosureReason::ProcessingError { err: err_reason });
+			// Worse, if the funding has un-confirmed we could have accepted some HTLC(s) over it
+			// and are now at risk of double-spend. While its possible, even likely, that this is
+			// just a trivial reorg and we should wait to see the new block connected in the next
+			// call, its also possible we've been double-spent. To avoid further loss of funds, we
+			// need some kind of method to freeze the channel and avoid accepting further HTLCs,
+			// but absent such a method, we just force-close.
+			//
+			// The one exception we make is for 0-conf channels, which we decided to trust anyway,
+			// in which case we simply track the previous SCID as a `historical_scids` the same as
+			// after a channel is spliced.
+			if funding_tx_confirmations == 0 && was_confirmed {
+				if let Some(scid) = original_scid {
+					self.context.historical_scids.push(scid);
+				} else {
+					debug_assert!(false);
+				}
+				if self.context.minimum_depth(&self.funding).expect("set for a ready channel") > 1 {
+					let err_reason = format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.",
+						self.context.minimum_depth.unwrap(), funding_tx_confirmations);
+					return Err(ClosureReason::ProcessingError { err: err_reason });
+				}
 			}
 		} else if !self.funding.is_outbound() && self.funding.funding_tx_confirmed_in.is_none() &&
 				height >= self.context.channel_creation_height + FUNDING_CONF_DEADLINE_BLOCKS {

lightning/src/ln/functional_test_utils.rs

@@ -3224,12 +3224,13 @@ pub fn expect_probe_successful_events(
 }
 
 pub struct PaymentFailedConditions<'a> {
-	pub(crate) expected_htlc_error_data: Option<(LocalHTLCFailureReason, &'a [u8])>,
-	pub(crate) expected_blamed_scid: Option<u64>,
-	pub(crate) expected_blamed_chan_closed: Option<bool>,
-	pub(crate) expected_mpp_parts_remain: bool,
-	pub(crate) retry_expected: bool,
-	pub(crate) from_mon_update: bool,
+	pub expected_htlc_error_data: Option<(LocalHTLCFailureReason, &'a [u8])>,
+	pub expected_blamed_scid: Option<u64>,
+	pub expected_blamed_chan_closed: Option<bool>,
+	pub expected_mpp_parts_remain: bool,
+	pub retry_expected: bool,
+	pub from_mon_update: bool,
+	pub reason: Option<PaymentFailureReason>,
 }
 
 impl<'a> PaymentFailedConditions<'a> {
@@ -3241,6 +3242,7 @@ impl<'a> PaymentFailedConditions<'a> {
 			expected_mpp_parts_remain: false,
 			retry_expected: false,
 			from_mon_update: false,
+			reason: None,
 		}
 	}
 	pub fn mpp_parts_remain(mut self) -> Self {
@@ -3321,14 +3323,21 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 				*payment_failed_permanently, expected_payment_failed_permanently,
 				"unexpected payment_failed_permanently value"
 			);
-			{
-				assert!(error_code.is_some(), "expected error_code.is_some() = true");
-				assert!(error_data.is_some(), "expected error_data.is_some() = true");
-				let reason: LocalHTLCFailureReason = error_code.unwrap().into();
-				if let Some((code, data)) = conditions.expected_htlc_error_data {
-					assert_eq!(reason, code, "unexpected error code");
-					assert_eq!(&error_data.as_ref().unwrap()[..], data, "unexpected error data");
-				}
+			match failure {
+				PathFailure::OnPath { .. } => {
+					assert!(error_code.is_some(), "expected error_code.is_some() = true");
+					assert!(error_data.is_some(), "expected error_data.is_some() = true");
+					let reason: LocalHTLCFailureReason = error_code.unwrap().into();
+					if let Some((code, data)) = conditions.expected_htlc_error_data {
+						assert_eq!(reason, code, "unexpected error code");
+						assert_eq!(&error_data.as_ref().unwrap()[..], data);
+					}
+				},
+				PathFailure::InitialSend { .. } => {
+					assert!(error_code.is_none());
+					assert!(error_data.is_none());
+					assert!(conditions.expected_htlc_error_data.is_none());
+				},
 			}
 
 			if let Some(chan_closed) = conditions.expected_blamed_chan_closed {
@@ -3362,7 +3371,9 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 				assert_eq!(*payment_id, expected_payment_id);
 				assert_eq!(
 					reason.unwrap(),
-					if expected_payment_failed_permanently {
+					if let Some(expected_reason) = conditions.reason {
+						expected_reason
+					} else if expected_payment_failed_permanently {
 						PaymentFailureReason::RecipientRejected
 					} else {
 						PaymentFailureReason::RetriesExhausted
@@ -3414,7 +3425,7 @@ pub fn send_along_route_with_secret<'a, 'b, 'c>(
 	payment_id
 }
 
-fn fail_payment_along_path<'a, 'b, 'c>(expected_path: &[&Node<'a, 'b, 'c>]) {
+pub fn fail_payment_along_path<'a, 'b, 'c>(expected_path: &[&Node<'a, 'b, 'c>]) {
 	let origin_node_id = expected_path[0].node.get_our_node_id();
 
 	// iterate from the receiving node to the origin node and handle update fail htlc.