From db3d58c586b9f2972414a07b09980db710428c7a Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 13 Jul 2023 16:02:17 -0700 Subject: [PATCH 1/3] Add new ConfirmationTarget variant for min mempool feerates Now that we support channels with anchor outputs, we add a new ConfirmationTarget variant that, for now, will only apply to such channels. This new variant should target estimating the minimum feerate required to be accepted into most node mempools across the network. --- fuzz/src/chanmon_consistency.rs | 2 +- lightning/src/chain/chaininterface.rs | 17 +++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 8f2c30ffa..cd9c484da 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -78,7 +78,7 @@ impl FeeEstimator for FuzzEstimator { // Background feerate which is <= the minimum Normal feerate. match conf_target { ConfirmationTarget::HighPriority => MAX_FEE, - ConfirmationTarget::Background => 253, + ConfirmationTarget::Background|ConfirmationTarget::MempoolMinimum => 253, ConfirmationTarget::Normal => cmp::min(self.ret_val.load(atomic::Ordering::Acquire), MAX_FEE), } } diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index d875dcce3..4204d714f 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -35,15 +35,24 @@ pub trait BroadcasterInterface { fn broadcast_transactions(&self, txs: &[&Transaction]); } -/// An enum that represents the speed at which we want a transaction to confirm used for feerate +/// An enum that represents the priority at which we want a transaction to confirm used for feerate /// estimation. #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub enum ConfirmationTarget { - /// We are happy with this transaction confirming slowly when feerate drops some. + /// We'd like a transaction to confirm in the future, but don't want to commit most of the fees + /// required to do so yet. The remaining fees will come via a Child-Pays-For-Parent (CPFP) fee + /// bump of the transaction. + /// + /// The feerate returned should be the absolute minimum feerate required to enter most node + /// mempools across the network. Note that if you are not able to obtain this feerate estimate, + /// you should likely use the furthest-out estimate allowed by your fee estimator. + MempoolMinimum, + /// We are happy with a transaction confirming slowly, at least within a day or so worth of + /// blocks. Background, - /// We'd like this transaction to confirm without major delay, but 12-18 blocks is fine. + /// We'd like a transaction to confirm without major delayed, i.e., within the next 12-24 blocks. Normal, - /// We'd like this transaction to confirm in the next few blocks. + /// We'd like a transaction to confirm in the next few blocks. HighPriority, } From 1349ac8e2d1c220a425469721394d2dc8dcd33e4 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 13 Jul 2023 16:03:32 -0700 Subject: [PATCH 2/3] Relax constraints for inbound feerate updates on anchor channels Channels supporting anchors outputs no longer require their feerate updates to target a prompt confirmation since commitment transactions can be bumped when broadcasting. Commitment transactions must now at least meet the minimum mempool feerate, until package relay is deployed, such that they can propagate across node mempools in the network by themselves. The existing higher bound no longer applies to channels supporting anchor outputs since their HTLC transactions don't have any fees committed, which directly impact the available balance users can send. --- lightning/src/ln/channel.rs | 40 ++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8232c5e1b..169c031fb 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2045,20 +2045,35 @@ struct CommitmentTxInfoCached { } impl Channel { - fn check_remote_fee(fee_estimator: &LowerBoundedFeeEstimator, - feerate_per_kw: u32, cur_feerate_per_kw: Option, logger: &L) - -> Result<(), ChannelError> where F::Target: FeeEstimator, L::Target: Logger, + fn check_remote_fee( + channel_type: &ChannelTypeFeatures, fee_estimator: &LowerBoundedFeeEstimator, + feerate_per_kw: u32, cur_feerate_per_kw: Option, logger: &L + ) -> Result<(), ChannelError> where F::Target: FeeEstimator, L::Target: Logger, { // We only bound the fee updates on the upper side to prevent completely absurd feerates, // always accepting up to 25 sat/vByte or 10x our fee estimator's "High Priority" fee. // We generally don't care too much if they set the feerate to something very high, but it - // could result in the channel being useless due to everything being dust. - let upper_limit = cmp::max(250 * 25, - fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10); - if feerate_per_kw as u64 > upper_limit { - return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit))); + // could result in the channel being useless due to everything being dust. This doesn't + // apply to channels supporting anchor outputs since HTLC transactions are pre-signed with a + // zero fee, so their fee is no longer considered to determine dust limits. + if !channel_type.supports_anchors_zero_fee_htlc_tx() { + let upper_limit = cmp::max(250 * 25, + fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10); + if feerate_per_kw as u64 > upper_limit { + return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit))); + } } - let lower_limit = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background); + + // We can afford to use a lower bound with anchors than previously since we can now bump + // fees when broadcasting our commitment. However, we must still make sure we meet the + // minimum mempool feerate, until package relay is deployed, such that we can ensure the + // commitment transaction propagates throughout node mempools on its own. + let lower_limit_conf_target = if channel_type.supports_anchors_zero_fee_htlc_tx() { + ConfirmationTarget::MempoolMinimum + } else { + ConfirmationTarget::Background + }; + let lower_limit = fee_estimator.bounded_sat_per_1000_weight(lower_limit_conf_target); // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing // occasional issues with feerate disagreements between an initiator that wants a feerate // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250 @@ -3688,7 +3703,7 @@ impl Channel { if self.context.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 { return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish".to_owned())); } - Channel::::check_remote_fee(fee_estimator, msg.feerate_per_kw, Some(self.context.feerate_per_kw), logger)?; + Channel::::check_remote_fee(&self.context.channel_type, fee_estimator, msg.feerate_per_kw, Some(self.context.feerate_per_kw), logger)?; let feerate_over_dust_buffer = msg.feerate_per_kw > self.context.get_dust_buffer_feerate(None); self.context.pending_update_fee = Some((msg.feerate_per_kw, FeeUpdateState::RemoteAnnounced)); @@ -6039,7 +6054,7 @@ impl InboundV1Channel { if msg.htlc_minimum_msat >= full_channel_value_msat { return Err(ChannelError::Close(format!("Minimum htlc value ({}) was larger than full channel value ({})", msg.htlc_minimum_msat, full_channel_value_msat))); } - Channel::::check_remote_fee(fee_estimator, msg.feerate_per_kw, None, logger)?; + Channel::::check_remote_fee(&channel_type, fee_estimator, msg.feerate_per_kw, None, logger)?; let max_counterparty_selected_contest_delay = u16::min(config.channel_handshake_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT); if msg.to_self_delay > max_counterparty_selected_contest_delay { @@ -7441,7 +7456,8 @@ mod tests { // arithmetic, causing a panic with debug assertions enabled. let fee_est = TestFeeEstimator { fee_est: 42 }; let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_est); - assert!(Channel::::check_remote_fee(&bounded_fee_estimator, + assert!(Channel::::check_remote_fee( + &ChannelTypeFeatures::only_static_remote_key(), &bounded_fee_estimator, u32::max_value(), None, &&test_utils::TestLogger::new()).is_err()); } From 7751cb90669c81c4ca843cba9a0f0001504a1285 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 13 Jul 2023 16:04:10 -0700 Subject: [PATCH 3/3] Use min mempool feerate for outbound updates on anchor channels As done with inbound feerate updates, we can afford to commit less in fees, as long as we still may the minimum mempool feerate. This enables users to spend a bit more of their balance, as less funds are being committed to transaction fees. --- lightning/src/ln/channel.rs | 19 +++++++++++++++---- lightning/src/ln/channelmanager.rs | 18 +++++++++++++++--- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 169c031fb..7379c7128 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5517,10 +5517,15 @@ impl OutboundV1Channel { let channel_type = Self::get_initial_channel_type(&config, their_features); debug_assert!(channel_type.is_subset(&channelmanager::provided_channel_type_features(&config))); - let feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); + let commitment_conf_target = if channel_type.supports_anchors_zero_fee_htlc_tx() { + ConfirmationTarget::MempoolMinimum + } else { + ConfirmationTarget::Normal + }; + let commitment_feerate = fee_estimator.bounded_sat_per_1000_weight(commitment_conf_target); let value_to_self_msat = channel_value_satoshis * 1000 - push_msat; - let commitment_tx_fee = commit_tx_fee_msat(feerate, MIN_AFFORDABLE_HTLC_COUNT, &channel_type); + let commitment_tx_fee = commit_tx_fee_msat(commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, &channel_type); if value_to_self_msat < commitment_tx_fee { return Err(APIError::APIMisuseError{ err: format!("Funding amount ({}) can't even pay fee for initial commitment transaction fee of {}.", value_to_self_msat / 1000, commitment_tx_fee / 1000) }); } @@ -5614,7 +5619,7 @@ impl OutboundV1Channel { short_channel_id: None, channel_creation_height: current_chain_height, - feerate_per_kw: feerate, + feerate_per_kw: commitment_feerate, counterparty_dust_limit_satoshis: 0, holder_dust_limit_satoshis: MIN_CHAN_DUST_LIMIT_SATOSHIS, counterparty_max_htlc_value_in_flight_msat: 0, @@ -5768,7 +5773,12 @@ impl OutboundV1Channel { /// If we receive an error message, it may only be a rejection of the channel type we tried, /// not of our ability to open any channel at all. Thus, on error, we should first call this /// and see if we get a new `OpenChannel` message, otherwise the channel is failed. - pub(crate) fn maybe_handle_error_without_close(&mut self, chain_hash: BlockHash) -> Result { + pub(crate) fn maybe_handle_error_without_close( + &mut self, chain_hash: BlockHash, fee_estimator: &LowerBoundedFeeEstimator + ) -> Result + where + F::Target: FeeEstimator + { if !self.context.is_outbound() || self.context.channel_state != ChannelState::OurInitSent as u32 { return Err(()); } if self.context.channel_type == ChannelTypeFeatures::only_static_remote_key() { // We've exhausted our options @@ -5785,6 +5795,7 @@ impl OutboundV1Channel { // whatever reason. if self.context.channel_type.supports_anchors_zero_fee_htlc_tx() { self.context.channel_type.clear_anchors_zero_fee_htlc_tx(); + self.context.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); assert!(!self.context.channel_transaction_parameters.channel_type_features.supports_anchors_nonzero_fee_htlc_tx()); } else if self.context.channel_type.supports_scid_privacy() { self.context.channel_type.clear_scid_privacy(); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7df125b93..79c5f017b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4235,13 +4235,19 @@ where PersistenceNotifierGuard::optionally_notify(&self.total_consistency_lock, &self.persistence_notifier, || { let mut should_persist = self.process_background_events(); - let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); + let normal_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); + let min_mempool_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::MempoolMinimum); let per_peer_state = self.per_peer_state.read().unwrap(); for (_cp_id, peer_state_mutex) in per_peer_state.iter() { let mut peer_state_lock = peer_state_mutex.lock().unwrap(); let peer_state = &mut *peer_state_lock; for (chan_id, chan) in peer_state.channel_by_id.iter_mut() { + let new_feerate = if chan.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { + min_mempool_feerate + } else { + normal_feerate + }; let chan_needs_persist = self.update_channel_fee(chan_id, chan, new_feerate); if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; } } @@ -4271,7 +4277,8 @@ where PersistenceNotifierGuard::optionally_notify(&self.total_consistency_lock, &self.persistence_notifier, || { let mut should_persist = self.process_background_events(); - let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); + let normal_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); + let min_mempool_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::MempoolMinimum); let mut handle_errors: Vec<(Result<(), _>, _)> = Vec::new(); let mut timed_out_mpp_htlcs = Vec::new(); @@ -4284,6 +4291,11 @@ where let pending_msg_events = &mut peer_state.pending_msg_events; let counterparty_node_id = *counterparty_node_id; peer_state.channel_by_id.retain(|chan_id, chan| { + let new_feerate = if chan.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { + min_mempool_feerate + } else { + normal_feerate + }; let chan_needs_persist = self.update_channel_fee(chan_id, chan, new_feerate); if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; } @@ -7250,7 +7262,7 @@ where let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); let peer_state = &mut *peer_state_lock; if let Some(chan) = peer_state.outbound_v1_channel_by_id.get_mut(&msg.channel_id) { - if let Ok(msg) = chan.maybe_handle_error_without_close(self.genesis_hash) { + if let Ok(msg) = chan.maybe_handle_error_without_close(self.genesis_hash, &self.fee_estimator) { peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { node_id: *counterparty_node_id, msg,