From 358292141a02ae52bdd54f44c63404267b5150db Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 19 Sep 2021 23:49:57 +0000 Subject: [PATCH] Automatically close channels that go unfunded for 2016 blocks As recommended by BOLT 2 added in https://github.com/lightningnetwork/lightning-rfc/pull/839 --- lightning/src/ln/channel.rs | 55 +++++++++++++++++++++------- lightning/src/ln/channelmanager.rs | 8 ++-- lightning/src/ln/functional_tests.rs | 42 +++++++++++++++++++-- lightning/src/ln/mod.rs | 2 +- lightning/src/util/events.rs | 5 +++ 5 files changed, 92 insertions(+), 20 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8703f6c95..e0f7a1ae7 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -379,6 +379,11 @@ pub const FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE: u64 = 2; #[cfg(not(fuzzing))] const FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE: u64 = 2; +/// If we fail to see a funding transaction confirmed on-chain within this many blocks after the +/// channel creation on an inbound channel, we simply force-close and move on. +/// This constant is the one suggested in BOLT 2. +pub(crate) const FUNDING_CONF_DEADLINE_BLOCKS: u32 = 2016; + // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking // has been completed, and then turn into a Channel to get compiler-time enforcement of things like // calling channel_id() before we're set up or things like get_outbound_funding_signed on an @@ -476,6 +481,10 @@ pub(super) struct Channel { funding_tx_confirmed_in: Option, funding_tx_confirmation_height: u32, short_channel_id: Option, + /// Either the height at which this channel was created or the height at which it was last + /// serialized if it was serialized by versions prior to 0.0.103. + /// We use this to close if funding is never broadcasted. + channel_creation_height: u32, counterparty_dust_limit_satoshis: u64, #[cfg(test)] @@ -647,7 +656,10 @@ impl Channel { } // Constructors: - pub fn new_outbound(fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures, channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig) -> Result, APIError> + pub fn new_outbound( + fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures, + channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig, current_chain_height: u32 + ) -> Result, APIError> where K::Target: KeysInterface, F::Target: FeeEstimator, { @@ -735,6 +747,7 @@ impl Channel { funding_tx_confirmed_in: None, funding_tx_confirmation_height: 0, short_channel_id: None, + channel_creation_height: current_chain_height, feerate_per_kw: feerate, counterparty_dust_limit_satoshis: 0, @@ -808,7 +821,10 @@ impl Channel { /// Creates a new channel from a remote sides' request for one. /// Assumes chain_hash has already been checked and corresponds with what we expect! - pub fn new_from_req(fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures, msg: &msgs::OpenChannel, user_id: u64, config: &UserConfig) -> Result, ChannelError> + pub fn new_from_req( + fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures, + msg: &msgs::OpenChannel, user_id: u64, config: &UserConfig, current_chain_height: u32 + ) -> Result, ChannelError> where K::Target: KeysInterface, F::Target: FeeEstimator { @@ -1021,6 +1037,7 @@ impl Channel { funding_tx_confirmed_in: None, funding_tx_confirmation_height: 0, short_channel_id: None, + channel_creation_height: current_chain_height, feerate_per_kw: msg.feerate_per_kw, channel_value_satoshis: msg.funding_satoshis, @@ -4236,6 +4253,13 @@ impl Channel { self.minimum_depth.unwrap(), funding_tx_confirmations); return Err(ClosureReason::ProcessingError { err: err_reason }); } + } else if !self.is_outbound() && self.funding_tx_confirmed_in.is_none() && + height >= self.channel_creation_height + FUNDING_CONF_DEADLINE_BLOCKS { + log_info!(logger, "Closing channel {} due to funding timeout", log_bytes!(self.channel_id)); + // If funding_tx_confirmed_in is unset, the channel must not be active + assert!(non_shutdown_state <= ChannelState::ChannelFunded as u32); + assert_eq!(non_shutdown_state & ChannelState::OurFundingLocked as u32, 0); + return Err(ClosureReason::FundingTimedOut); } Ok((None, timed_out_htlcs)) @@ -5274,6 +5298,7 @@ impl Writeable for Channel { (7, self.shutdown_scriptpubkey, option), (9, self.target_closing_feerate_sats_per_kw, option), (11, self.monitor_pending_finalized_fulfills, vec_type), + (13, self.channel_creation_height, required), }); Ok(()) @@ -5281,9 +5306,10 @@ impl Writeable for Channel { } const MAX_ALLOC_SIZE: usize = 64*1024; -impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel +impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel where K::Target: KeysInterface { - fn read(reader: &mut R, keys_source: &'a K) -> Result { + fn read(reader: &mut R, args: (&'a K, u32)) -> Result { + let (keys_source, serialized_height) = args; let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION); let user_id = Readable::read(reader)?; @@ -5511,6 +5537,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel // Prior to supporting channel type negotiation, all of our channels were static_remotekey // only, so we default to that if none was written. let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key()); + let mut channel_creation_height = Some(serialized_height); read_tlv_fields!(reader, { (0, announcement_sigs, option), (1, minimum_depth, option), @@ -5520,6 +5547,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel (7, shutdown_scriptpubkey, option), (9, target_closing_feerate_sats_per_kw, option), (11, monitor_pending_finalized_fulfills, vec_type), + (13, channel_creation_height, option), }); let chan_features = channel_type.as_ref().unwrap(); @@ -5584,6 +5612,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel funding_tx_confirmed_in, funding_tx_confirmation_height, short_channel_id, + channel_creation_height: channel_creation_height.unwrap(), counterparty_dust_limit_satoshis, holder_dust_limit_satoshis, @@ -5732,7 +5761,7 @@ mod tests { let secp_ctx = Secp256k1::new(); let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - match Channel::::new_outbound(&&fee_estimator, &&keys_provider, node_id, &features, 10000000, 100000, 42, &config) { + match Channel::::new_outbound(&&fee_estimator, &&keys_provider, node_id, &features, 10000000, 100000, 42, &config, 0) { Err(APIError::IncompatibleShutdownScript { script }) => { assert_eq!(script.into_inner(), non_v0_segwit_shutdown_script.into_inner()); }, @@ -5754,7 +5783,7 @@ mod tests { let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let node_a_chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); + let node_a_chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); // Now change the fee so we can check that the fee in the open_channel message is the // same as the old fee. @@ -5779,13 +5808,13 @@ mod tests { // Create Node A's channel pointing to Node B's pubkey let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); // Create Node B's channel by receiving Node A's open_channel message // Make sure A's dust limit is as we expect. let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash()); let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap()); - let node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config).unwrap(); + let node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0).unwrap(); // Node B --> Node A: accept channel, explicitly setting B's dust limit. let mut accept_channel_msg = node_b_chan.get_accept_channel(); @@ -5849,7 +5878,7 @@ mod tests { let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); + let mut chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); let commitment_tx_fee_0_htlcs = chan.commit_tx_fee_msat(0); let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1); @@ -5898,12 +5927,12 @@ mod tests { // Create Node A's channel pointing to Node B's pubkey let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); // Create Node B's channel by receiving Node A's open_channel message let open_channel_msg = node_a_chan.get_open_channel(chain_hash); let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap()); - let mut node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config).unwrap(); + let mut node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0).unwrap(); // Node B --> Node A: accept channel let accept_channel_msg = node_b_chan.get_accept_channel(); @@ -5960,7 +5989,7 @@ mod tests { // Create a channel. let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); assert!(node_a_chan.counterparty_forwarding_info.is_none()); assert_eq!(node_a_chan.holder_htlc_minimum_msat, 1); // the default assert!(node_a_chan.counterparty_forwarding_info().is_none()); @@ -6024,7 +6053,7 @@ mod tests { let counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let mut config = UserConfig::default(); config.channel_options.announced_channel = false; - let mut chan = Channel::::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, &InitFeatures::known(), 10_000_000, 100000, 42, &config).unwrap(); // Nothing uses their network key in this test + let mut chan = Channel::::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, &InitFeatures::known(), 10_000_000, 100000, 42, &config, 0).unwrap(); // Nothing uses their network key in this test chan.holder_dust_limit_satoshis = 546; chan.counterparty_selected_channel_reserve_satoshis = Some(0); // Filled in in accept_channel diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a6c319b6e..b6d530ac5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1398,7 +1398,8 @@ impl ChannelMana let peer_state = peer_state.lock().unwrap(); let their_features = &peer_state.latest_features; let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration }; - Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features, channel_value_satoshis, push_msat, user_channel_id, config)? + Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features, + channel_value_satoshis, push_msat, user_channel_id, config, self.best_block.read().unwrap().height())? }, None => return Err(APIError::ChannelUnavailable { err: format!("Not connected to node: {}", their_network_key) }), } @@ -3645,7 +3646,8 @@ impl ChannelMana return Err(MsgHandleErrInternal::send_err_msg_no_close("Unknown genesis block hash".to_owned(), msg.temporary_channel_id.clone())); } - let channel = Channel::new_from_req(&self.fee_estimator, &self.keys_manager, counterparty_node_id.clone(), &their_features, msg, 0, &self.default_configuration) + let channel = Channel::new_from_req(&self.fee_estimator, &self.keys_manager, counterparty_node_id.clone(), + &their_features, msg, 0, &self.default_configuration, self.best_block.read().unwrap().height()) .map_err(|e| MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id))?; let mut channel_state_lock = self.channel_state.lock().unwrap(); let channel_state = &mut *channel_state_lock; @@ -5833,7 +5835,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> let mut short_to_id = HashMap::with_capacity(cmp::min(channel_count as usize, 128)); let mut channel_closures = Vec::new(); for _ in 0..channel_count { - let mut channel: Channel = Channel::read(reader, &args.keys_manager)?; + let mut channel: Channel = Channel::read(reader, (&args.keys_manager, best_block_height))?; let funding_txo = channel.get_funding_txo().ok_or(DecodeError::InvalidValue)?; funding_txo_set.insert(funding_txo.clone()); if let Some(ref mut monitor) = args.channel_monitors.get_mut(&funding_txo) { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index b6b3d345e..8e9893870 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -6982,7 +6982,7 @@ fn test_user_configurable_csv_delay() { let nodes = create_network(2, &node_cfgs, &node_chanmgrs); // We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound() - if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), 1000000, 1000000, 0, &low_our_to_self_config) { + if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), 1000000, 1000000, 0, &low_our_to_self_config, 0) { match error { APIError::APIMisuseError { err } => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -6993,7 +6993,7 @@ fn test_user_configurable_csv_delay() { nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap(); let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id()); open_channel.to_self_delay = 200; - if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, &low_our_to_self_config) { + if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, &low_our_to_self_config, 0) { match error { ChannelError::Close(err) => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -7022,7 +7022,7 @@ fn test_user_configurable_csv_delay() { nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap(); let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id()); open_channel.to_self_delay = 200; - if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, &high_their_to_self_config) { + if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, &high_their_to_self_config, 0) { match error { ChannelError::Close(err) => { assert!(regex::Regex::new(r"They wanted our payments to be delayed by a needlessly long period\. Upper limit: \d+\. Actual: \d+").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -7908,6 +7908,42 @@ fn test_bump_txn_sanitize_tracking_maps() { } } +#[test] +fn test_channel_conf_timeout() { + // Tests that, for inbound channels, we give up on them if the funding transaction does not + // confirm within 2016 blocks, as recommended by BOLT 2. + 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); + + let _funding_tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 1_000_000, 100_000, InitFeatures::known(), InitFeatures::known()); + + // The outbound node should wait forever for confirmation: + // This matches `channel::FUNDING_CONF_DEADLINE_BLOCKS` and BOLT 2's suggested timeout, thus is + // copied here instead of directly referencing the constant. + connect_blocks(&nodes[0], 2016); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + // The inbound node should fail the channel after exactly 2016 blocks + connect_blocks(&nodes[1], 2015); + check_added_monitors!(nodes[1], 0); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + connect_blocks(&nodes[1], 1); + check_added_monitors!(nodes[1], 1); + check_closed_event!(nodes[1], 1, ClosureReason::FundingTimedOut); + let close_ev = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(close_ev.len(), 1); + match close_ev[0] { + MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, ref node_id } => { + assert_eq!(*node_id, nodes[0].node.get_our_node_id()); + assert_eq!(msg.data, "Channel closed because funding transaction failed to confirm within 2016 blocks"); + }, + _ => panic!("Unexpected event"), + } +} + #[test] fn test_override_channel_config() { let chanmon_cfgs = create_chanmon_cfgs(2); diff --git a/lightning/src/ln/mod.rs b/lightning/src/ln/mod.rs index a2a0b4efe..41d978e8d 100644 --- a/lightning/src/ln/mod.rs +++ b/lightning/src/ln/mod.rs @@ -37,7 +37,7 @@ pub(crate) mod peer_channel_encryptor; #[cfg(feature = "fuzztarget")] pub mod channel; #[cfg(not(feature = "fuzztarget"))] -mod channel; +pub(crate) mod channel; mod onion_utils; pub mod wire; diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index c021fe3c1..6bbd6b994 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -16,6 +16,7 @@ use chain::keysinterface::SpendableOutputDescriptor; use ln::channelmanager::PaymentId; +use ln::channel::FUNDING_CONF_DEADLINE_BLOCKS; use ln::msgs; use ln::msgs::DecodeError; use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; @@ -101,6 +102,8 @@ pub enum ClosureReason { /// commitment transaction came from our counterparty, but it may also have come from /// a copy of our own `ChannelMonitor`. CommitmentTxConfirmed, + /// The funding transaction failed to confirm in a timely manner on an inbound channel. + FundingTimedOut, /// Closure generated from processing an event, likely a HTLC forward/relay/reception. ProcessingError { /// A developer-readable error message which we generated. @@ -129,6 +132,7 @@ impl core::fmt::Display for ClosureReason { ClosureReason::HolderForceClosed => f.write_str("user manually force-closed the channel"), ClosureReason::CooperativeClosure => f.write_str("the channel was cooperatively closed"), ClosureReason::CommitmentTxConfirmed => f.write_str("commitment or closing transaction was confirmed on chain."), + ClosureReason::FundingTimedOut => write!(f, "funding transaction failed to confirm within {} blocks", FUNDING_CONF_DEADLINE_BLOCKS), ClosureReason::ProcessingError { err } => { f.write_str("of an exception: ")?; f.write_str(&err) @@ -141,6 +145,7 @@ impl core::fmt::Display for ClosureReason { impl_writeable_tlv_based_enum_upgradable!(ClosureReason, (0, CounterpartyForceClosed) => { (1, peer_msg, required) }, + (1, FundingTimedOut) => {}, (2, HolderForceClosed) => {}, (6, CommitmentTxConfirmed) => {}, (4, CooperativeClosure) => {}, -- 2.39.5