From b4f7fdfd1c471ee29051b8a24feeb10586d3ba8e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 14 Nov 2025 23:20:41 +0000 Subject: [PATCH] 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. --- lightning/src/ln/channel.rs | 41 +++- lightning/src/ln/functional_test_utils.rs | 43 ++-- lightning/src/ln/priv_short_conf_tests.rs | 236 ++++++++++++++++++---- 3 files changed, 259 insertions(+), 61 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e55e4144ef2..bdbc0586676 100644 --- a/lightning/src/ln/channel.rs +++ b/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,33 @@ 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 { + // Reset the original short_channel_id so that we'll generate a closure + // `channel_update` broadcast event. + self.funding.short_channel_id = original_scid; + 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 { diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index e31630a4926..be32e1fd23a 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/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, - pub(crate) expected_blamed_chan_closed: Option, - 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, + pub expected_blamed_chan_closed: Option, + pub expected_mpp_parts_remain: bool, + pub retry_expected: bool, + pub from_mon_update: bool, + pub reason: Option, } 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. diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs index ea34e88f619..ab7cad9be44 100644 --- a/lightning/src/ln/priv_short_conf_tests.rs +++ b/lightning/src/ln/priv_short_conf_tests.rs @@ -12,7 +12,8 @@ //! LSP). use crate::chain::ChannelMonitorUpdateStatus; -use crate::events::{ClosureReason, Event, HTLCHandlingFailureType}; +use crate::events::{ClosureReason, Event, HTLCHandlingFailureType, PaymentFailureReason}; +use crate::ln::channel::CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY; use crate::ln::channelmanager::{PaymentId, RecipientOnionFields, MIN_CLTV_EXPIRY_DELTA}; use crate::ln::msgs; use crate::ln::msgs::{ @@ -1078,66 +1079,233 @@ fn test_public_0conf_channel() { #[test] fn test_0conf_channel_reorg() { // If we accept a 0conf channel, which is then confirmed, but then changes SCID in a reorg, we - // have to make sure we handle this correctly (or, currently, just force-close the channel). + // have to ensure we still accept relays to the previous SCID, at least for some time, as well + // as send a fresh channel announcement. - let chanmon_cfgs = create_chanmon_cfgs(2); - let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); let mut chan_config = test_default_channel_config(); chan_config.manually_accept_inbound_channels = true; - let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(chan_config.clone())]); - let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let node_a_id = nodes[0].node.get_our_node_id(); + let node_chanmgrs = + create_node_chanmgrs(3, &node_cfgs, &[None, None, Some(chan_config.clone())]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + create_chan_between_nodes(&nodes[0], &nodes[1]); + + // Make sure all nodes are at the same starting height + connect_blocks(&nodes[0], CHAN_CONFIRM_DEPTH + 1 - nodes[0].best_block_info().1); + connect_blocks(&nodes[1], CHAN_CONFIRM_DEPTH + 1 - nodes[1].best_block_info().1); + connect_blocks(&nodes[2], CHAN_CONFIRM_DEPTH + 1 - nodes[2].best_block_info().1); // This is the default but we force it on anyway chan_config.channel_handshake_config.announce_for_forwarding = true; - let (tx, ..) = open_zero_conf_channel(&nodes[0], &nodes[1], Some(chan_config)); + let (tx, ..) = open_zero_conf_channel(&nodes[1], &nodes[2], Some(chan_config)); // We can use the channel immediately, but we can't announce it until we get 6+ confirmations - send_payment(&nodes[0], &[&nodes[1]], 100_000); + send_payment(&nodes[1], &[&nodes[2]], 100_000); - mine_transaction(&nodes[0], &tx); mine_transaction(&nodes[1], &tx); + mine_transaction(&nodes[2], &tx); // Send a payment using the channel's real SCID, which will be public in a few blocks once we // can generate a channel_announcement. - let real_scid = nodes[0].node.list_usable_channels()[0].short_channel_id.unwrap(); - assert_eq!(nodes[1].node.list_usable_channels()[0].short_channel_id.unwrap(), real_scid); + let bs_chans = nodes[1].node.list_usable_channels(); + let bs_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap(); + let original_scid = bs_chan.short_channel_id.unwrap(); + assert_eq!(nodes[2].node.list_usable_channels()[0].short_channel_id.unwrap(), original_scid); let (mut route, payment_hash, payment_preimage, payment_secret) = - get_route_and_payment_hash!(nodes[0], nodes[1], 10_000); - assert_eq!(route.paths[0].hops[0].short_channel_id, real_scid); + get_route_and_payment_hash!(nodes[1], nodes[2], 10_000); + assert_eq!(route.paths[0].hops[0].short_channel_id, original_scid); + send_along_route_with_secret( + &nodes[1], + route.clone(), + &[&[&nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[1], &[&nodes[2]], payment_preimage); + + // Check that we can forward a payment over the channel's SCID as well (i.e. as if node C + // generated an invoice with a route hint through the 0-conf channel). + let mut forwarded_route = route.clone(); + let (ab_route, ..) = get_route_and_payment_hash!(nodes[0], nodes[1], 10_000); + forwarded_route.paths[0].hops.insert(0, ab_route.paths[0].hops[0].clone()); + forwarded_route.paths[0].hops[0].fee_msat = 1000; + forwarded_route.paths[0].hops[0].cltv_expiry_delta = MIN_CLTV_EXPIRY_DELTA.into(); send_along_route_with_secret( &nodes[0], - route, - &[&[&nodes[1]]], + forwarded_route.clone(), + &[&[&nodes[1], &nodes[2]]], 10_000, payment_hash, payment_secret, ); - claim_payment(&nodes[0], &[&nodes[1]], payment_preimage); + claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage); - disconnect_blocks(&nodes[0], 1); + // Now disconnect blocks, checking that the SCID was wiped but that it still works both for a + // forwarded HTLC and a directly-sent one. disconnect_blocks(&nodes[1], 1); + disconnect_blocks(&nodes[2], 1); - // At this point the channel no longer has an SCID again. In the future we should likely - // support simply un-setting the SCID and waiting until the channel gets re-confirmed, but for - // now we force-close the channel here. - let reason = ClosureReason::ProcessingError { - err: "Funding transaction was un-confirmed. Locked at 0 confs, now have 0 confs." - .to_owned(), - }; - check_closed_event(&nodes[0], 1, reason, &[node_b_id], 100000); - check_closed_broadcast!(nodes[0], true); + let bs_chans = nodes[1].node.list_usable_channels(); + let bs_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap(); + assert!(bs_chan.short_channel_id.is_none()); + assert!(nodes[2].node.list_usable_channels()[0].short_channel_id.is_none()); + + send_along_route_with_secret( + &nodes[1], + route.clone(), + &[&[&nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[1], &[&nodes[2]], payment_preimage); + + send_along_route_with_secret( + &nodes[0], + forwarded_route.clone(), + &[&[&nodes[1], &nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage); + + // Finally, connect an extra block then re-mine the funding tx, giving the channel a new SCID. + connect_blocks(&nodes[1], 1); + connect_blocks(&nodes[2], 1); + + mine_transaction(&nodes[1], &tx); + mine_transaction(&nodes[2], &tx); + + let bs_chans = nodes[1].node.list_usable_channels(); + let bs_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap(); + let new_scid = bs_chan.short_channel_id.unwrap(); + assert_ne!(original_scid, new_scid); + assert_eq!(nodes[2].node.list_usable_channels()[0].short_channel_id.unwrap(), new_scid); + + // At this point, the channel should happily forward or send payments with either the old SCID + // or the new SCID... + send_along_route_with_secret( + &nodes[1], + route.clone(), + &[&[&nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[1], &[&nodes[2]], payment_preimage); + + send_along_route_with_secret( + &nodes[0], + forwarded_route.clone(), + &[&[&nodes[1], &nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage); + + let mut new_scid_route = route.clone(); + new_scid_route.paths[0].hops[0].short_channel_id = new_scid; + send_along_route_with_secret( + &nodes[1], + new_scid_route.clone(), + &[&[&nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[1], &[&nodes[2]], payment_preimage); + + let mut new_scid_forwarded_route = forwarded_route.clone(); + new_scid_forwarded_route.paths[0].hops[1].short_channel_id = new_scid; + send_along_route_with_secret( + &nodes[0], + new_scid_forwarded_route.clone(), + &[&[&nodes[1], &nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage); + + // However after CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY blocks, the old SCID should be removed + // and will no longer work for sent or forwarded payments (but the new one still will). + connect_blocks(&nodes[1], 5); + let bs_announcement_sigs = + get_event_msg!(nodes[1], MessageSendEvent::SendAnnouncementSignatures, node_c_id); + + connect_blocks(&nodes[2], 5); + let cs_announcement_sigs = + get_event_msg!(nodes[2], MessageSendEvent::SendAnnouncementSignatures, node_b_id); + + nodes[2].node.handle_announcement_signatures(node_b_id, &bs_announcement_sigs); + let cs_broadcast = nodes[2].node.get_and_clear_pending_msg_events(); + assert_eq!(cs_broadcast.len(), 1); + if let MessageSendEvent::BroadcastChannelAnnouncement { .. } = cs_broadcast[0] { + } else { + panic!("Expected broadcast"); + } + + nodes[1].node.handle_announcement_signatures(node_c_id, &cs_announcement_sigs); + let bs_broadcast = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(bs_broadcast.len(), 1); + if let MessageSendEvent::BroadcastChannelAnnouncement { .. } = bs_broadcast[0] { + } else { + panic!("Expected broadcast"); + } + + connect_blocks(&nodes[0], CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY); + connect_blocks(&nodes[1], CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY - 5); + connect_blocks(&nodes[2], CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY - 5); + + send_along_route_with_secret( + &nodes[1], + new_scid_route, + &[&[&nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[1], &[&nodes[2]], payment_preimage); + + send_along_route_with_secret( + &nodes[0], + new_scid_forwarded_route, + &[&[&nodes[1], &nodes[2]]], + 10_000, + payment_hash, + payment_secret, + ); + claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage); + + let onion = RecipientOnionFields::secret_only(payment_secret); + let id = PaymentId([0; 32]); + nodes[1].node.send_payment_with_route(route, payment_hash, onion.clone(), id).unwrap(); + let mut conditions = PaymentFailedConditions::new(); + conditions.reason = Some(PaymentFailureReason::RouteNotFound); + expect_payment_failed_conditions(&nodes[1], payment_hash, false, conditions); + + nodes[0].node.send_payment_with_route(forwarded_route, payment_hash, onion, id).unwrap(); check_added_monitors(&nodes[0], 1); - let reason = ClosureReason::ProcessingError { - err: "Funding transaction was un-confirmed. Locked at 0 confs, now have 0 confs." - .to_owned(), - }; - check_closed_event(&nodes[1], 1, reason, &[node_a_id], 100000); - check_closed_broadcast!(nodes[1], true); - check_added_monitors(&nodes[1], 1); + let mut ev = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(ev.len(), 1); + let ev = ev.pop().unwrap(); + let path = &[&nodes[1]]; + let failure = HTLCHandlingFailureType::InvalidForward { requested_forward_scid: original_scid }; + let args = + PassAlongPathArgs::new(&nodes[0], path, 10_000, payment_hash, ev).expect_failure(failure); + do_pass_along_path(args); + fail_payment_along_path(&[&nodes[0], &nodes[1]]); + expect_payment_failed!(nodes[0], payment_hash, false); } #[test]