From a150a5e45ad3435a9ace9ca601e79874704ad142 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 | 54 ++++++++++++++++++++++------ lightning/src/ln/channelmanager.rs | 6 ++-- lightning/src/ln/functional_tests.rs | 39 ++++++++++++++++++-- 3 files changed, 83 insertions(+), 16 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 361179caf08..35c2a917a0e 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -356,6 +356,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. +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 @@ -452,6 +457,9 @@ pub(super) struct Channel { funding_tx_confirmed_in: Option, funding_tx_confirmation_height: u32, short_channel_id: Option, + /// If this channel was created in recent software, this will be set to the height at which + /// channel creation was intiated. This is used to close if funding is never broadcasted. + channel_creation_height: Option, counterparty_dust_limit_satoshis: u64, #[cfg(test)] @@ -620,7 +628,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_heigt: u32 + ) -> Result, APIError> where K::Target: KeysInterface, F::Target: FeeEstimator, { @@ -707,6 +718,7 @@ impl Channel { funding_tx_confirmed_in: None, funding_tx_confirmation_height: 0, short_channel_id: None, + channel_creation_height: Some(current_chain_heigt), feerate_per_kw: feerate, counterparty_dust_limit_satoshis: 0, @@ -775,7 +787,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_heigt: u32 + ) -> Result, ChannelError> where K::Target: KeysInterface, F::Target: FeeEstimator { @@ -970,6 +985,7 @@ impl Channel { funding_tx_confirmed_in: None, funding_tx_confirmation_height: 0, short_channel_id: None, + channel_creation_height: Some(current_chain_heigt), feerate_per_kw: msg.feerate_per_kw, channel_value_satoshis: msg.funding_satoshis, @@ -4145,6 +4161,18 @@ impl Channel { data: err_reason.clone(), }, ClosureReason::ProcessingError { err: err_reason })); } + } else if let Some(creation_height) = self.channel_creation_height { + if !self.is_outbound() && self.funding_tx_confirmed_in.is_none() && + creation_height + FUNDING_CONF_DEADLINE_BLOCKS <= height { + 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(msgs::ErrorMessage { + channel_id: self.channel_id(), + data: format!("Funding transaction failed to confirm in {} blocks", FUNDING_CONF_DEADLINE_BLOCKS), + }); + } } Ok((None, timed_out_htlcs)) @@ -5174,6 +5202,7 @@ impl Writeable for Channel { (5, self.config, required), (7, self.shutdown_scriptpubkey, option), (9, self.target_closing_feerate_sats_per_kw, option), + (11, self.channel_creation_height, option), }); Ok(()) @@ -5407,6 +5436,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel let mut announcement_sigs = None; let mut target_closing_feerate_sats_per_kw = None; + let mut channel_creation_height = None; read_tlv_fields!(reader, { (0, announcement_sigs, option), (1, minimum_depth, option), @@ -5414,6 +5444,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel (5, config, option), // Note that if none is provided we will *not* overwrite the existing one. (7, shutdown_scriptpubkey, option), (9, target_closing_feerate_sats_per_kw, option), + (11, channel_creation_height, option), }); let mut secp_ctx = Secp256k1::new(); @@ -5470,6 +5501,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, counterparty_dust_limit_satoshis, holder_dust_limit_satoshis, @@ -5616,7 +5648,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()); }, @@ -5638,7 +5670,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. @@ -5663,13 +5695,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(); @@ -5731,7 +5763,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); @@ -5780,12 +5812,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(); @@ -5842,7 +5874,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()); @@ -5906,7 +5938,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 69107393814..a0f19059f0a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1236,7 +1236,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_id, config)? + Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features, + channel_value_satoshis, push_msat, user_id, config, self.best_block.read().unwrap().height())? }, None => return Err(APIError::ChannelUnavailable { err: format!("Not connected to node: {}", their_network_key) }), } @@ -3317,7 +3318,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; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index f55e1e1840c..20bfed4e6ba 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7241,7 +7241,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"), @@ -7252,7 +7252,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"), @@ -7281,7 +7281,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"), @@ -8171,6 +8171,39 @@ 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: + 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); + 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, "Funding transaction failed to confirm in 2016 blocks"); + }, + _ => panic!("Unexpected event"), + } +} + #[test] fn test_override_channel_config() { let chanmon_cfgs = create_chanmon_cfgs(2);