From 9b614f4ff226da918d9f730a7531f40d2e333aab Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 17 Jul 2023 10:45:39 -0700 Subject: [PATCH 1/3] Drop use of RefCell in DebugIter The `RefCell` was necessary in a previous iteration of the code in which the iterator was not `Clone` so we needed interior mutability in order to consume the iterator. Now that it is `Clone`, we can drop it, as we're no longer mutating the original iterator. --- lightning/src/util/logger.rs | 11 ++++------- lightning/src/util/macro_logger.rs | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs index 722a81f7a..dbca9b785 100644 --- a/lightning/src/util/logger.rs +++ b/lightning/src/util/logger.rs @@ -173,18 +173,15 @@ impl<'a> core::fmt::Display for DebugBytes<'a> { /// /// This is not exported to bindings users as fmt can't be used in C #[doc(hidden)] -pub struct DebugIter + Clone>(pub core::cell::RefCell); +pub struct DebugIter + Clone>(pub I); impl + Clone> fmt::Display for DebugIter { fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { - use core::ops::DerefMut; write!(f, "[")?; - let iter_ref = self.0.clone(); - let mut iter = iter_ref.borrow_mut(); - for item in iter.deref_mut() { + let mut iter = self.0.clone(); + if let Some(item) = iter.next() { write!(f, "{}", item)?; - break; } - for item in iter.deref_mut() { + while let Some(item) = iter.next() { write!(f, ", {}", item)?; } write!(f, "]")?; diff --git a/lightning/src/util/macro_logger.rs b/lightning/src/util/macro_logger.rs index 4703bcdb3..e79980370 100644 --- a/lightning/src/util/macro_logger.rs +++ b/lightning/src/util/macro_logger.rs @@ -19,7 +19,7 @@ use crate::util::logger::DebugBytes; macro_rules! log_iter { ($obj: expr) => { - $crate::util::logger::DebugIter(core::cell::RefCell::new($obj)) + $crate::util::logger::DebugIter($obj) } } From 52ec0824011f4a2e6ee3f9edc447d9ac0aacc354 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 17 Jul 2023 10:56:16 -0700 Subject: [PATCH 2/3] Clarify log for commitment transaction already meeting required feerate --- lightning/src/chain/onchaintx.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 171caf700..f7071bf0c 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -633,11 +633,12 @@ impl OnchainTxHandler .compute_package_feerate(fee_estimator, conf_target, force_feerate_bump); if let Some(input_amount_sat) = output.funding_amount { let fee_sat = input_amount_sat - tx.output.iter().map(|output| output.value).sum::(); - if compute_feerate_sat_per_1000_weight(fee_sat, tx.weight() as u64) >= - package_target_feerate_sat_per_1000_weight - { - log_debug!(logger, "Commitment transaction {} already meets required feerate {} sat/kW", - tx.txid(), package_target_feerate_sat_per_1000_weight); + let commitment_tx_feerate_sat_per_1000_weight = + compute_feerate_sat_per_1000_weight(fee_sat, tx.weight() as u64); + if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight { + log_debug!(logger, "Pre-signed {} already has feerate {} sat/kW above required {} sat/kW", + log_tx!(tx), commitment_tx_feerate_sat_per_1000_weight, + package_target_feerate_sat_per_1000_weight); return Some((new_timer, 0, OnchainClaim::Tx(tx.clone()))); } } From c7d84d8ac87d581179a2e0073aee2299a397842d Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 17 Jul 2023 14:47:10 -0700 Subject: [PATCH 3/3] Add warning regarding remote fee estimators --- lightning/src/chain/chaininterface.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index 54cd33812..6aed834c1 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -59,6 +59,11 @@ pub enum ConfirmationTarget { /// A trait which should be implemented to provide feerate information on a number of time /// horizons. /// +/// If access to a local mempool is not feasible, feerate estimates should be fetched from a set of +/// third-parties hosting them. Note that this enables them to affect the propagation of your +/// pre-signed transactions at any time and therefore endangers the safety of channels funds. It +/// should be considered carefully as a deployment. +/// /// Note that all of the functions implemented here *must* be reentrant-safe (obviously - they're /// called from inside the library in response to chain events, P2P events, or timer events). pub trait FeeEstimator {