From df1f981627a9daa97b5e3b06bc90666206245309 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 24 Dec 2023 04:49:24 +0000 Subject: [PATCH 1/6] Fix `Feature` eq + hash to ignore excess zero bytes If we get a `Feature` object which has excess zero bytes, we shouldn't consider it a different `Feature` from another with the same bits set, but no excess zero bytes. Here we fix both the `Hash` and `PartialEq` implementation for `Features` to ignore excess zero bytes. --- lightning/src/ln/features.rs | 38 ++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index df5c0abf25f..2e732b17aa8 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -469,12 +469,24 @@ impl Clone for Features { } impl Hash for Features { fn hash(&self, hasher: &mut H) { - self.flags.hash(hasher); + let mut nonzero_flags = &self.flags[..]; + while nonzero_flags.last() == Some(&0) { + nonzero_flags = &nonzero_flags[..nonzero_flags.len() - 1]; + } + nonzero_flags.hash(hasher); } } impl PartialEq for Features { fn eq(&self, o: &Self) -> bool { - self.flags.eq(&o.flags) + let mut o_iter = o.flags.iter(); + let mut self_iter = self.flags.iter(); + loop { + match (o_iter.next(), self_iter.next()) { + (Some(o), Some(us)) => if o != us { return false }, + (Some(b), None) | (None, Some(b)) => if *b != 0 { return false }, + (None, None) => return true, + } + } } } impl PartialOrd for Features { @@ -1215,4 +1227,26 @@ mod tests { assert!(!converted_features.supports_any_optional_bits()); assert!(converted_features.requires_static_remote_key()); } + + #[test] + #[cfg(feature = "std")] + fn test_excess_zero_bytes_ignored() { + // Checks that `Hash` and `PartialEq` ignore excess zero bytes, which may appear due to + // feature conversion or because a peer serialized their feature poorly. + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; + + let mut zerod_features = InitFeatures::empty(); + zerod_features.flags = vec![0]; + let empty_features = InitFeatures::empty(); + assert!(empty_features.flags.is_empty()); + + assert_eq!(zerod_features, empty_features); + + let mut zerod_hash = DefaultHasher::new(); + zerod_features.hash(&mut zerod_hash); + let mut empty_hash = DefaultHasher::new(); + empty_features.hash(&mut empty_hash); + assert_eq!(zerod_hash.finish(), empty_hash.finish()); + } } From ddb54fc2d2f539502ab36357470f8a9a9d9555f1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 24 Dec 2023 05:17:29 +0000 Subject: [PATCH 2/6] Stop including dust values in feerate affordability checks When we or our counterparty are updating the fees on the channel, we currently check that the resulting balance is sufficient not only to meet the reserve threshold, but also not push it below dust. This isn't required in the BOLTs and may lead to spurious force-closures (which would be a bit safer, but reserve should always exceed the dust threshold). Worse, the current logic is broken - it compares the output value in *billionths of satoshis* to the dust limit in satoshis. Thus, the code is borderline dead anyway, but can overflow for channels with several million Bitcoin, causing the fuzzer to get mad (and lead to spurious force-closures for few-billion-dollar channels). --- lightning/src/ln/channel.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 588995656ec..6e283f08660 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -732,8 +732,8 @@ struct CommitmentStats<'a> { total_fee_sat: u64, // the total fee included in the transaction num_nondust_htlcs: usize, // the number of HTLC outputs (dust HTLCs *non*-included) htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction - local_balance_msat: u64, // local balance before fees but considering dust limits - remote_balance_msat: u64, // remote balance before fees but considering dust limits + local_balance_msat: u64, // local balance before fees *not* considering dust limits + remote_balance_msat: u64, // remote balance before fees *not* considering dust limits outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment } @@ -1728,13 +1728,13 @@ impl ChannelContext where SP::Target: SignerProvider { } } - let mut value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset; + let value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset; assert!(value_to_self_msat >= 0); // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to // "violate" their reserve value by couting those against it. Thus, we have to convert // everything to i64 before subtracting as otherwise we can overflow. - let mut value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; + let value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; assert!(value_to_remote_msat >= 0); #[cfg(debug_assertions)] @@ -1800,10 +1800,6 @@ impl ChannelContext where SP::Target: SignerProvider { htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap()); htlcs_included.append(&mut included_dust_htlcs); - // For the stats, trimmed-to-0 the value in msats accordingly - value_to_self_msat = if (value_to_self_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_self_msat }; - value_to_remote_msat = if (value_to_remote_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_remote_msat }; - CommitmentStats { tx, feerate_per_kw, From c946edb218761c8ae7456ca9d89776410801c83a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 24 Dec 2023 05:55:11 +0000 Subject: [PATCH 3/6] Fix `REVOKEABLE_REDEEMSCRIPT_MAX_LENGTH` for contest delays >0x7fff When contest delays are >= 0x8000, script pushes require an extra byte to avoid being interpreted as a negative int. Thus, for channels with CSV delays longer than ~7.5 months we may generate transactions with slightly too little fee. This isn't really a huge deal, but we should prefer to be conservative here, and slightly too high fee in the general case is better than slightly too little fee in other cases. --- lightning/src/ln/chan_utils.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 3552748b31c..672e3aa862e 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -485,9 +485,11 @@ impl TxCreationKeys { } /// The maximum length of a script returned by get_revokeable_redeemscript. -// Calculated as 6 bytes of opcodes, 1 byte push plus 2 bytes for contest_delay, and two public -// keys of 33 bytes (+ 1 push). -pub const REVOKEABLE_REDEEMSCRIPT_MAX_LENGTH: usize = 6 + 3 + 34*2; +// Calculated as 6 bytes of opcodes, 1 byte push plus 3 bytes for contest_delay, and two public +// keys of 33 bytes (+ 1 push). Generally, pushes are only 2 bytes (for values below 0x7fff, i.e. +// around 7 months), however, a 7 month contest delay shouldn't result in being unable to reclaim +// on-chain funds. +pub const REVOKEABLE_REDEEMSCRIPT_MAX_LENGTH: usize = 6 + 4 + 34*2; /// A script either spendable by the revocation /// key or the broadcaster_delayed_payment_key and satisfying the relative-locktime OP_CSV constrain. From 5d8cd5a0a2545ab2304addf4412256f4eac8aef1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 24 Dec 2023 06:10:38 +0000 Subject: [PATCH 4/6] Fix debug assertion on opening a channel with a disconnected peer If we try to open a channel with a peer that is disconnected (but with which we have some other channels), we'll end up with an unfunded channel which will lead to a panic when the peer reconnects. Here we drop this debug assertion without bother to add a new test, given this behavior will change in a PR very soon. --- lightning/src/ln/channelmanager.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b2b28cdf365..dae29dcef82 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -8979,13 +8979,7 @@ where let pending_msg_events = &mut peer_state.pending_msg_events; peer_state.channel_by_id.iter_mut().filter_map(|(_, phase)| - if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { - // Since unfunded channel maps are cleared upon disconnecting a peer, and they're not persisted - // (so won't be recovered after a crash), they shouldn't exist here and we would never need to - // worry about closing and removing them. - debug_assert!(false); - None - } + if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None } ).for_each(|chan| { let logger = WithChannelContext::from(&self.logger, &chan.context); pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish { From 3b6a361ae76a087ac10c2e59f9163188fd5c20e1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 24 Dec 2023 06:24:38 +0000 Subject: [PATCH 5/6] Fix dust buffer feerate calculation overflow If a peer provides a feerate which nears `u32::MAX`, we may overflow calculating the dust buffer feerate, leading to spuriously keeping non-anchor channels open when they should be force-closed. --- lightning/src/ln/channel.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 6e283f08660..721d653519f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1872,7 +1872,8 @@ impl ChannelContext where SP::Target: SignerProvider { if let Some(feerate) = outbound_feerate_update { feerate_per_kw = cmp::max(feerate_per_kw, feerate); } - cmp::max(2530, feerate_per_kw * 1250 / 1000) + let feerate_plus_quarter = feerate_per_kw.checked_mul(1250).map(|v| v / 1000); + cmp::max(2530, feerate_plus_quarter.unwrap_or(u32::max_value())) } /// Get forwarding information for the counterparty. From 7f24e833fb97155844be2866ba2229104c12e020 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 24 Dec 2023 17:12:10 +0000 Subject: [PATCH 6/6] Fix reachable unwrap on non-channel_type manual channel acceptance If we receive an `OpenChannel` message without a `channel_type` with `manually_accept_inbound_channels` set, we will `unwrap()` `None`. This is uncommon these days as most nodes support `channel_type`, but sadly is rather trivial for a peer to hit for those with manual channel acceptance enabled. Reported in and fixes #2804. Luckily, the updated `full_stack_target` has no issue reaching this issue quickly. --- lightning/src/ln/channel.rs | 62 +++++++++++++++++------------- lightning/src/ln/channelmanager.rs | 9 ++++- 2 files changed, 43 insertions(+), 28 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 721d653519f..1dfc6dc552f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6845,6 +6845,41 @@ pub(super) struct InboundV1Channel where SP::Target: SignerProvider { pub unfunded_context: UnfundedChannelContext, } +/// Fetches the [`ChannelTypeFeatures`] that will be used for a channel built from a given +/// [`msgs::OpenChannel`]. +pub(super) fn channel_type_from_open_channel( + msg: &msgs::OpenChannel, their_features: &InitFeatures, + our_supported_features: &ChannelTypeFeatures +) -> Result { + if let Some(channel_type) = &msg.channel_type { + if channel_type.supports_any_optional_bits() { + return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".to_owned())); + } + + // We only support the channel types defined by the `ChannelManager` in + // `provided_channel_type_features`. The channel type must always support + // `static_remote_key`. + if !channel_type.requires_static_remote_key() { + return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned())); + } + // Make sure we support all of the features behind the channel type. + if !channel_type.is_subset(our_supported_features) { + return Err(ChannelError::Close("Channel Type contains unsupported features".to_owned())); + } + let announced_channel = if (msg.channel_flags & 1) == 1 { true } else { false }; + if channel_type.requires_scid_privacy() && announced_channel { + return Err(ChannelError::Close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned())); + } + Ok(channel_type.clone()) + } else { + let channel_type = ChannelTypeFeatures::from_init(&their_features); + if channel_type != ChannelTypeFeatures::only_static_remote_key() { + return Err(ChannelError::Close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); + } + Ok(channel_type) + } +} + impl InboundV1Channel where SP::Target: SignerProvider { /// Creates a new channel from a remote sides' request for one. /// Assumes chain_hash has already been checked and corresponds with what we expect! @@ -6863,32 +6898,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { // First check the channel type is known, failing before we do anything else if we don't // support this channel type. - let channel_type = if let Some(channel_type) = &msg.channel_type { - if channel_type.supports_any_optional_bits() { - return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".to_owned())); - } - - // We only support the channel types defined by the `ChannelManager` in - // `provided_channel_type_features`. The channel type must always support - // `static_remote_key`. - if !channel_type.requires_static_remote_key() { - return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned())); - } - // Make sure we support all of the features behind the channel type. - if !channel_type.is_subset(our_supported_features) { - return Err(ChannelError::Close("Channel Type contains unsupported features".to_owned())); - } - if channel_type.requires_scid_privacy() && announced_channel { - return Err(ChannelError::Close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned())); - } - channel_type.clone() - } else { - let channel_type = ChannelTypeFeatures::from_init(&their_features); - if channel_type != ChannelTypeFeatures::only_static_remote_key() { - return Err(ChannelError::Close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); - } - channel_type - }; + let channel_type = channel_type_from_open_channel(msg, their_features, our_supported_features)?; let channel_keys_id = signer_provider.generate_channel_keys_id(true, msg.funding_satoshis, user_id); let holder_signer = signer_provider.derive_channel_signer(msg.funding_satoshis, channel_keys_id); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index dae29dcef82..8e0ac2fdf08 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -43,7 +43,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa // Since this struct is returned in `list_channels` methods, expose it here in case users want to // construct one themselves. use crate::ln::{inbound_payment, ChannelId, PaymentHash, PaymentPreimage, PaymentSecret}; -use crate::ln::channel::{Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext}; +use crate::ln::channel::{self, Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext}; use crate::ln::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; #[cfg(any(feature = "_test_utils", test))] use crate::ln::features::Bolt11InvoiceFeatures; @@ -6170,13 +6170,18 @@ where // If we're doing manual acceptance checks on the channel, then defer creation until we're sure we want to accept. if self.default_configuration.manually_accept_inbound_channels { + let channel_type = channel::channel_type_from_open_channel( + &msg, &peer_state.latest_features, &self.channel_type_features() + ).map_err(|e| + MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id) + )?; let mut pending_events = self.pending_events.lock().unwrap(); pending_events.push_back((events::Event::OpenChannelRequest { temporary_channel_id: msg.temporary_channel_id.clone(), counterparty_node_id: counterparty_node_id.clone(), funding_satoshis: msg.funding_satoshis, push_msat: msg.push_msat, - channel_type: msg.channel_type.clone().unwrap(), + channel_type, }, None)); peer_state.inbound_channel_request_by_id.insert(channel_id, InboundChannelRequest { open_channel_msg: msg.clone(),