Merge pull request #1611 from TheBlueMatt/2022-07-lower-bounded-estimator-nit

Use a separate (non-trait) fee-estimation fn in LowerBoundedEstimator

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -140,7 +140,7 @@ impl chain::Watch<EnforcingSigner> for TestChainMonitor {
 		};
 		let deserialized_monitor = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingSigner>)>::
 			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));

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<D: Deref> 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.
@@ -68,18 +60,19 @@ 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<F: Deref>(pub F) where F::Target: FeeEstimator;
 
 impl<F: Deref> LowerBoundedFeeEstimator<F> where F::Target: FeeEstimator {
 	/// Creates a new `LowerBoundedFeeEstimator` which wraps the provided fee_estimator
 	pub fn new(fee_estimator: F) -> Self {
 		LowerBoundedFeeEstimator(fee_estimator)
 	}
-}
 
-impl<F: Deref> FeeEstimator for LowerBoundedFeeEstimator<F> where F::Target: FeeEstimator {
-	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,
@@ -107,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]
@@ -116,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);
 	}
 }

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));
 				}

lightning/src/chain/channelmonitor.rs

@@ -1139,7 +1139,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 		&self,
 		updates: &ChannelMonitorUpdate,
 		broadcaster: &B,
-		fee_estimator: &F,
+		fee_estimator: F,
 		logger: &L,
 	) -> Result<(), ()>
 	where
@@ -1949,10 +1949,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
 	}
 
-	pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &L) -> Result<(), ()>
+	pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&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());
@@ -1990,7 +1990,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 				},
 				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 } => {
@@ -3537,7 +3537,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

lightning/src/chain/package.rs

@@ -778,13 +778,13 @@ fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(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)",

lightning/src/ln/channel.rs

@@ -917,7 +917,7 @@ impl<Signer: Sign> Channel<Signer> {
 			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<Signer: Sign> Channel<Signer> {
 		// 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<Signer: Sign> Channel<Signer> {
 		// 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

lightning/src/ln/channelmanager.rs

@@ -3577,7 +3577,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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();