From ca9357147e521a15702d1f05c687c54f9a65bf2b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 13 Jul 2022 16:52:27 +0000 Subject: [PATCH 1/3] Use a separate (non-trait) fee-estimation fn in LowerBoundedEstimator This should make it somewhat more difficult to accidentally use a straight fee estimator when we actually want a LowerBoundedFeeEstimator by not having the types be exchangeable at all. --- lightning/src/chain/chaininterface.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index 2ec7f318ff5..ab659cbca13 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -68,7 +68,10 @@ pub const MIN_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 4000; pub const FEERATE_FLOOR_SATS_PER_KW: u32 = 253; /// Wraps a `Deref` to a `FeeEstimator` so that any fee estimations provided by it -/// are bounded below by `FEERATE_FLOOR_SATS_PER_KW` (253 sats/KW) +/// are bounded below by `FEERATE_FLOOR_SATS_PER_KW` (253 sats/KW). +/// +/// Note that this does *not* implement [`FeeEstimator`] to make it harder to accidentally mix the +/// two. pub(crate) struct LowerBoundedFeeEstimator(pub F) where F::Target: FeeEstimator; impl LowerBoundedFeeEstimator where F::Target: FeeEstimator { @@ -76,10 +79,8 @@ impl LowerBoundedFeeEstimator where F::Target: FeeEstimator { pub fn new(fee_estimator: F) -> Self { LowerBoundedFeeEstimator(fee_estimator) } -} -impl FeeEstimator for LowerBoundedFeeEstimator where F::Target: FeeEstimator { - fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 { + pub fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 { cmp::max( self.0.get_est_sat_per_1000_weight(confirmation_target), FEERATE_FLOOR_SATS_PER_KW, From a491200f96760315fa420e433d0e60c4e6503a97 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 21 Jul 2022 21:38:00 +0000 Subject: [PATCH 2/3] Avoid blanket implementing FeeEstimator for Deref This simplifies things for bindings (and, to some extent, downstream users) by exploiting the fact that we can always "clone" a reference to a struct by dereferencing and then creating a new reference. --- fuzz/src/chanmon_consistency.rs | 2 +- lightning/src/chain/chaininterface.rs | 8 -------- lightning/src/chain/chainmonitor.rs | 2 +- lightning/src/chain/channelmonitor.rs | 12 ++++++------ 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 9d5ccaa8105..71c1ebc3304 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -140,7 +140,7 @@ impl chain::Watch for TestChainMonitor { }; let deserialized_monitor = <(BlockHash, channelmonitor::ChannelMonitor)>:: read(&mut Cursor::new(&map_entry.get().1), &*self.keys).unwrap().1; - deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap(); + deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap(); let mut ser = VecWriter(Vec::new()); deserialized_monitor.write(&mut ser).unwrap(); map_entry.insert((update.update_id, ser.0)); diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index ab659cbca13..50570dfdf11 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -52,14 +52,6 @@ pub trait FeeEstimator { fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32; } -// We need `FeeEstimator` implemented so that in some places where we only have a shared -// reference to a `Deref` to a `FeeEstimator`, we can still wrap it. -impl FeeEstimator for D where D::Target: FeeEstimator { - fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 { - (**self).get_est_sat_per_1000_weight(confirmation_target) - } -} - /// Minimum relay fee as required by bitcoin network mempool policy. pub const MIN_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 4000; /// Minimum feerate that takes a sane approach to bitcoind weight-to-vbytes rounding. diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index e6b5733520a..239affc73dd 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -636,7 +636,7 @@ where C::Target: chain::Filter, Some(monitor_state) => { let monitor = &monitor_state.monitor; log_trace!(self.logger, "Updating ChannelMonitor for channel {}", log_funding_info!(monitor)); - let update_res = monitor.update_monitor(&update, &self.broadcaster, &self.fee_estimator, &self.logger); + let update_res = monitor.update_monitor(&update, &self.broadcaster, &*self.fee_estimator, &self.logger); if update_res.is_err() { log_error!(self.logger, "Failed to update ChannelMonitor for channel {}.", log_funding_info!(monitor)); } diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 80cd9cb9d45..f91af7cc66c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1134,7 +1134,7 @@ impl ChannelMonitor { &self, updates: &ChannelMonitorUpdate, broadcaster: &B, - fee_estimator: &F, + fee_estimator: F, logger: &L, ) -> Result<(), ()> where @@ -1944,10 +1944,10 @@ impl ChannelMonitorImpl { self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0)); } - pub fn update_monitor(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &L) -> Result<(), ()> + pub fn update_monitor(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: F, logger: &L) -> Result<(), ()> where B::Target: BroadcasterInterface, - F::Target: FeeEstimator, - L::Target: Logger, + F::Target: FeeEstimator, + L::Target: Logger, { log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} changes.", log_funding_info!(self), self.latest_update_id, updates.update_id, updates.updates.len()); @@ -1985,7 +1985,7 @@ impl ChannelMonitorImpl { }, ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => { log_trace!(logger, "Updating ChannelMonitor with payment preimage"); - let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator); + let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&*fee_estimator); self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage, broadcaster, &bounded_fee_estimator, logger) }, ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => { @@ -3534,7 +3534,7 @@ mod tests { let broadcaster = TestBroadcaster::new(Arc::clone(&nodes[1].blocks)); assert!( - pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger) + pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &chanmon_cfgs[1].fee_estimator, &nodes[1].logger) .is_err()); // Even though we error'd on the first update, we should still have generated an HTLC claim // transaction From af7e9b608d7a78dbb1a87a31a5b45c758f15c0ec Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 25 Jul 2022 18:08:44 +0000 Subject: [PATCH 3/3] Change `LowerBoundedFeeEstimator` fn name to make it hard to swap This change the method name on `LowerBoundedFeeEstimator` to further differentiate it from the generic `FeeEstimator` trait. --- lightning/src/chain/chaininterface.rs | 6 +++--- lightning/src/chain/package.rs | 6 +++--- lightning/src/ln/channel.rs | 10 +++++----- lightning/src/ln/channelmanager.rs | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index 50570dfdf11..be86261f0c0 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -72,7 +72,7 @@ impl LowerBoundedFeeEstimator where F::Target: FeeEstimator { LowerBoundedFeeEstimator(fee_estimator) } - pub fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 { + pub fn bounded_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 { cmp::max( self.0.get_est_sat_per_1000_weight(confirmation_target), FEERATE_FLOOR_SATS_PER_KW, @@ -100,7 +100,7 @@ mod tests { let test_fee_estimator = &TestFeeEstimator { sat_per_kw }; let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator); - assert_eq!(fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background), FEERATE_FLOOR_SATS_PER_KW); + assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background), FEERATE_FLOOR_SATS_PER_KW); } #[test] @@ -109,6 +109,6 @@ mod tests { let test_fee_estimator = &TestFeeEstimator { sat_per_kw }; let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator); - assert_eq!(fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background), sat_per_kw); + assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background), sat_per_kw); } } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index b0cfa9bf000..30530303e59 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -778,13 +778,13 @@ fn compute_fee_from_spent_amounts(input_amounts: u64, predic where F::Target: FeeEstimator, L::Target: Logger, { - let mut updated_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64; + let mut updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64; let mut fee = updated_feerate * (predicted_weight as u64) / 1000; if input_amounts <= fee { - updated_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal) as u64; + updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal) as u64; fee = updated_feerate * (predicted_weight as u64) / 1000; if input_amounts <= fee { - updated_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background) as u64; + updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background) as u64; fee = updated_feerate * (predicted_weight as u64) / 1000; if input_amounts <= fee { log_error!(logger, "Failed to generate an on-chain punishment tx as even low priority fee ({} sat) was more than the entire claim balance ({} sat)", diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index f612cb97943..b698a919b01 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -917,7 +917,7 @@ impl Channel { return Err(APIError::APIMisuseError { err: format!("Holder selected channel reserve below implemention limit dust_limit_satoshis {}", holder_selected_channel_reserve_satoshis) }); } - let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); + let feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); let value_to_self_msat = channel_value_satoshis * 1000 - push_msat; let commitment_tx_fee = Self::commit_tx_fee_msat(feerate, MIN_AFFORDABLE_HTLC_COUNT, opt_anchors); @@ -1064,11 +1064,11 @@ impl Channel { // 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.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10); + 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.get_est_sat_per_1000_weight(ConfirmationTarget::Background); + let lower_limit = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background); // 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 @@ -4022,8 +4022,8 @@ impl Channel { // Propose a range from our current Background feerate to our Normal feerate plus our // force_close_avoidance_max_fee_satoshis. // If we fail to come to consensus, we'll have to force-close. - let mut proposed_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); - let normal_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); + let mut proposed_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background); + let normal_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); let mut proposed_max_feerate = if self.is_outbound() { normal_feerate } else { u32::max_value() }; // The spec requires that (when the channel does not have anchors) we only send absolute diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index bae408d29f0..979cea39527 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3577,7 +3577,7 @@ impl ChannelMana PersistenceNotifierGuard::optionally_notify(&self.total_consistency_lock, &self.persistence_notifier, || { let mut should_persist = NotifyOption::SkipPersist; - let new_feerate = self.fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); + let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); let mut handle_errors = Vec::new(); { @@ -3616,7 +3616,7 @@ impl ChannelMana let mut should_persist = NotifyOption::SkipPersist; if self.process_background_events() { should_persist = NotifyOption::DoPersist; } - let new_feerate = self.fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); + let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal); let mut handle_errors = Vec::new(); let mut timed_out_mpp_htlcs = Vec::new();