Merge pull request #1611 from TheBlueMatt/2022-07-lower-bounded-estimator-nit
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Mon, 25 Jul 2022 21:11:07 +0000 (21:11 +0000)
committerGitHub <noreply@github.com>
Mon, 25 Jul 2022 21:11:07 +0000 (21:11 +0000)
Use a separate (non-trait) fee-estimation fn in LowerBoundedEstimator

1  2 
lightning/src/chain/chaininterface.rs
lightning/src/chain/channelmonitor.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs

index 546e1b2df53e915a6fc04aca800127117aef2a98,be86261f0c0300bafd1deb5d495838716acd1391..cbf609485ce1984800fa991e22d952e33ab5db56
@@@ -52,23 -52,18 +52,18 @@@ 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.
  /// See the following Core Lightning commit for an explanation:
 -/// https://github.com/ElementsProject/lightning/commit/2e687b9b352c9092b5e8bd4a688916ac50b44af0
 +/// <https://github.com/ElementsProject/lightning/commit/2e687b9b352c9092b5e8bd4a688916ac50b44af0>
  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 {
        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 +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]
                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);
        }
  }
index 8dd3d4b43b680755581d9bc94ac2c2881035e763,f91af7cc66cd3c9a3ea70b07c9cb5aef82bcca86..8a6f02452ec1abb6c61812ac4bc1974e54a07df2
@@@ -965,13 -965,6 +965,13 @@@ impl<Signer: Sign> Writeable for Channe
  }
  
  impl<Signer: Sign> ChannelMonitor<Signer> {
 +      /// For lockorder enforcement purposes, we need to have a single site which constructs the
 +      /// `inner` mutex, otherwise cases where we lock two monitors at the same time (eg in our
 +      /// PartialEq implementation) we may decide a lockorder violation has occurred.
 +      fn from_impl(imp: ChannelMonitorImpl<Signer>) -> Self {
 +              ChannelMonitor { inner: Mutex::new(imp) }
 +      }
 +
        pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_script: Option<Script>,
                          on_counterparty_tx_csv: u16, destination_script: &Script, funding_info: (OutPoint, Script),
                          channel_parameters: &ChannelTransactionParameters,
                let mut outputs_to_watch = HashMap::new();
                outputs_to_watch.insert(funding_info.0.txid, vec![(funding_info.0.index as u32, funding_info.1.clone())]);
  
 -              ChannelMonitor {
 -                      inner: Mutex::new(ChannelMonitorImpl {
 -                              latest_update_id: 0,
 -                              commitment_transaction_number_obscure_factor,
 +              Self::from_impl(ChannelMonitorImpl {
 +                      latest_update_id: 0,
 +                      commitment_transaction_number_obscure_factor,
  
 -                              destination_script: destination_script.clone(),
 -                              broadcasted_holder_revokable_script: None,
 -                              counterparty_payment_script,
 -                              shutdown_script,
 +                      destination_script: destination_script.clone(),
 +                      broadcasted_holder_revokable_script: None,
 +                      counterparty_payment_script,
 +                      shutdown_script,
  
 -                              channel_keys_id,
 -                              holder_revocation_basepoint,
 -                              funding_info,
 -                              current_counterparty_commitment_txid: None,
 -                              prev_counterparty_commitment_txid: None,
 +                      channel_keys_id,
 +                      holder_revocation_basepoint,
 +                      funding_info,
 +                      current_counterparty_commitment_txid: None,
 +                      prev_counterparty_commitment_txid: None,
  
 -                              counterparty_commitment_params,
 -                              funding_redeemscript,
 -                              channel_value_satoshis,
 -                              their_cur_per_commitment_points: None,
 +                      counterparty_commitment_params,
 +                      funding_redeemscript,
 +                      channel_value_satoshis,
 +                      their_cur_per_commitment_points: None,
  
 -                              on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
 +                      on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
  
 -                              commitment_secrets: CounterpartyCommitmentSecrets::new(),
 -                              counterparty_claimable_outpoints: HashMap::new(),
 -                              counterparty_commitment_txn_on_chain: HashMap::new(),
 -                              counterparty_hash_commitment_number: HashMap::new(),
 +                      commitment_secrets: CounterpartyCommitmentSecrets::new(),
 +                      counterparty_claimable_outpoints: HashMap::new(),
 +                      counterparty_commitment_txn_on_chain: HashMap::new(),
 +                      counterparty_hash_commitment_number: HashMap::new(),
  
 -                              prev_holder_signed_commitment_tx: None,
 -                              current_holder_commitment_tx: holder_commitment_tx,
 -                              current_counterparty_commitment_number: 1 << 48,
 -                              current_holder_commitment_number,
 +                      prev_holder_signed_commitment_tx: None,
 +                      current_holder_commitment_tx: holder_commitment_tx,
 +                      current_counterparty_commitment_number: 1 << 48,
 +                      current_holder_commitment_number,
  
 -                              payment_preimages: HashMap::new(),
 -                              pending_monitor_events: Vec::new(),
 -                              pending_events: Vec::new(),
 +                      payment_preimages: HashMap::new(),
 +                      pending_monitor_events: Vec::new(),
 +                      pending_events: Vec::new(),
  
 -                              onchain_events_awaiting_threshold_conf: Vec::new(),
 -                              outputs_to_watch,
 +                      onchain_events_awaiting_threshold_conf: Vec::new(),
 +                      outputs_to_watch,
  
 -                              onchain_tx_handler,
 +                      onchain_tx_handler,
  
 -                              lockdown_from_offchain: false,
 -                              holder_tx_signed: false,
 -                              funding_spend_seen: false,
 -                              funding_spend_confirmed: None,
 -                              htlcs_resolved_on_chain: Vec::new(),
 +                      lockdown_from_offchain: false,
 +                      holder_tx_signed: false,
 +                      funding_spend_seen: false,
 +                      funding_spend_confirmed: None,
 +                      htlcs_resolved_on_chain: Vec::new(),
  
 -                              best_block,
 -                              counterparty_node_id: Some(counterparty_node_id),
 +                      best_block,
 +                      counterparty_node_id: Some(counterparty_node_id),
  
 -                              secp_ctx,
 -                      }),
 -              }
 +                      secp_ctx,
 +              })
        }
  
        #[cfg(test)]
                &self,
                updates: &ChannelMonitorUpdate,
                broadcaster: &B,
-               fee_estimator: &F,
+               fee_estimator: F,
                logger: &L,
        ) -> Result<(), ()>
        where
@@@ -1949,10 -1944,10 +1949,10 @@@ impl<Signer: Sign> ChannelMonitorImpl<S
                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());
                                },
                                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 } => {
@@@ -3366,58 -3361,60 +3366,58 @@@ impl<'a, Signer: Sign, K: KeysInterface
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
  
 -              Ok((best_block.block_hash(), ChannelMonitor {
 -                      inner: Mutex::new(ChannelMonitorImpl {
 -                              latest_update_id,
 -                              commitment_transaction_number_obscure_factor,
 +              Ok((best_block.block_hash(), ChannelMonitor::from_impl(ChannelMonitorImpl {
 +                      latest_update_id,
 +                      commitment_transaction_number_obscure_factor,
  
 -                              destination_script,
 -                              broadcasted_holder_revokable_script,
 -                              counterparty_payment_script,
 -                              shutdown_script,
 +                      destination_script,
 +                      broadcasted_holder_revokable_script,
 +                      counterparty_payment_script,
 +                      shutdown_script,
  
 -                              channel_keys_id,
 -                              holder_revocation_basepoint,
 -                              funding_info,
 -                              current_counterparty_commitment_txid,
 -                              prev_counterparty_commitment_txid,
 +                      channel_keys_id,
 +                      holder_revocation_basepoint,
 +                      funding_info,
 +                      current_counterparty_commitment_txid,
 +                      prev_counterparty_commitment_txid,
  
 -                              counterparty_commitment_params,
 -                              funding_redeemscript,
 -                              channel_value_satoshis,
 -                              their_cur_per_commitment_points,
 +                      counterparty_commitment_params,
 +                      funding_redeemscript,
 +                      channel_value_satoshis,
 +                      their_cur_per_commitment_points,
  
 -                              on_holder_tx_csv,
 +                      on_holder_tx_csv,
  
 -                              commitment_secrets,
 -                              counterparty_claimable_outpoints,
 -                              counterparty_commitment_txn_on_chain,
 -                              counterparty_hash_commitment_number,
 +                      commitment_secrets,
 +                      counterparty_claimable_outpoints,
 +                      counterparty_commitment_txn_on_chain,
 +                      counterparty_hash_commitment_number,
  
 -                              prev_holder_signed_commitment_tx,
 -                              current_holder_commitment_tx,
 -                              current_counterparty_commitment_number,
 -                              current_holder_commitment_number,
 +                      prev_holder_signed_commitment_tx,
 +                      current_holder_commitment_tx,
 +                      current_counterparty_commitment_number,
 +                      current_holder_commitment_number,
  
 -                              payment_preimages,
 -                              pending_monitor_events: pending_monitor_events.unwrap(),
 -                              pending_events,
 +                      payment_preimages,
 +                      pending_monitor_events: pending_monitor_events.unwrap(),
 +                      pending_events,
  
 -                              onchain_events_awaiting_threshold_conf,
 -                              outputs_to_watch,
 +                      onchain_events_awaiting_threshold_conf,
 +                      outputs_to_watch,
  
 -                              onchain_tx_handler,
 +                      onchain_tx_handler,
  
 -                              lockdown_from_offchain,
 -                              holder_tx_signed,
 -                              funding_spend_seen: funding_spend_seen.unwrap(),
 -                              funding_spend_confirmed,
 -                              htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
 +                      lockdown_from_offchain,
 +                      holder_tx_signed,
 +                      funding_spend_seen: funding_spend_seen.unwrap(),
 +                      funding_spend_confirmed,
 +                      htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
  
 -                              best_block,
 -                              counterparty_node_id,
 +                      best_block,
 +                      counterparty_node_id,
  
 -                              secp_ctx,
 -                      }),
 -              }))
 +                      secp_ctx,
 +              })))
        }
  }
  
@@@ -3537,7 -3534,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
index ca0836f63275308d2a78b9d28d89fe15f5b809d3,b698a919b01c4868f7ee0b7a65da9b718eb6a85a..972707a0bc3732f8729d29b96bf91165b2223231
@@@ -917,7 -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);
                // 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
                // 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
@@@ -6572,7 -6572,7 +6572,7 @@@ mod tests 
        use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
        use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
        use ln::features::{InitFeatures, ChannelTypeFeatures};
 -      use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
 +      use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate, MAX_VALUE_MSAT};
        use ln::script::ShutdownScript;
        use ln::chan_utils;
        use ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
                                flags: 0,
                                cltv_expiry_delta: 100,
                                htlc_minimum_msat: 5,
 -                              htlc_maximum_msat: OptionalField::Absent,
 +                              htlc_maximum_msat: MAX_VALUE_MSAT,
                                fee_base_msat: 110,
                                fee_proportional_millionths: 11,
                                excess_data: Vec::new(),
index c764c7cafeabf306b707f81cb86a83ec753059fc,979cea395278cfa33a1981d205ae22f72beda7ac..56d705ef71e2ba64ec95a53e6d99dd9dd9eb775d
@@@ -48,7 -48,7 +48,7 @@@ use routing::router::{PaymentParameters
  use ln::msgs;
  use ln::msgs::NetAddress;
  use ln::onion_utils;
 -use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT, OptionalField};
 +use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT};
  use ln::wire::Encode;
  use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner, Recipient};
  use util::config::{UserConfig, ChannelConfig};
@@@ -2397,7 -2397,7 +2397,7 @@@ impl<Signer: Sign, M: Deref, T: Deref, 
                        flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
                        cltv_expiry_delta: chan.get_cltv_expiry_delta(),
                        htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
 -                      htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
 +                      htlc_maximum_msat: chan.get_announced_htlc_max_msat(),
                        fee_base_msat: chan.get_outbound_forwarding_fee_base_msat(),
                        fee_proportional_millionths: chan.get_fee_proportional_millionths(),
                        excess_data: Vec::new(),
                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();
                        {
                        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();
                                        return;
                                }
                                mem::drop(channel_state_lock);
 -                              let retry = if let Some(payment_params_data) = payment_params {
 +                              let mut retry = if let Some(payment_params_data) = payment_params {
                                        let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
                                        Some(RouteParameters {
                                                payment_params: payment_params_data.clone(),
                                                        // TODO: If we decided to blame ourselves (or one of our channels) in
                                                        // process_onion_failure we should close that channel as it implies our
                                                        // next-hop is needlessly blaming us!
 +                                                      if let Some(scid) = short_channel_id {
 +                                                              retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
 +                                                      }
                                                        events::Event::PaymentPathFailed {
                                                                payment_id: Some(payment_id),
                                                                payment_hash: payment_hash.clone(),
                                                // ChannelDetails.
                                                // TODO: For non-temporary failures, we really should be closing the
                                                // channel here as we apparently can't relay through them anyway.
 +                                              let scid = path.first().unwrap().short_channel_id;
 +                                              retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
                                                events::Event::PaymentPathFailed {
                                                        payment_id: Some(payment_id),
                                                        payment_hash: payment_hash.clone(),
                                                        network_update: None,
                                                        all_paths_failed,
                                                        path: path.clone(),
 -                                                      short_channel_id: Some(path.first().unwrap().short_channel_id),
 +                                                      short_channel_id: Some(scid),
                                                        retry,
  #[cfg(test)]
                                                        error_code: Some(*failure_code),