From 54d3d6c3481ba33d48870120f2b623dc60b628d9 Mon Sep 17 00:00:00 2001 From: jolah1 Date: Wed, 14 Oct 2026 08:01:47 +0100 Subject: [PATCH 1/2] Fall back to per-target defaults for missing fee estimates Esplora may return a non-empty estimate map that still has no usable entry for some of our confirmation targets, as `convert_fee_rate` only yields a value if the map holds a block count at or below the requested one. We'd then substitute 1 sat/vb, which is well below the default we'd otherwise use for urgent targets: `UrgentOnChainSweep` would end up at 250 sats/kwu instead of 5000. Note this isn't a case of a missing fallback, but of an actively harmful one: `OnchainFeeEstimator::estimate_fee_rate` already falls back to `get_fallback_rate_for_target` whenever it misses the cache, so inserting 1 sat/vb only serves to shadow the value we'd have used anyway. Here we therefore fall back to the target's own default. We deliberately skip the post-estimation adjustments for it, as there is no estimate to adjust, and so that we land on the same rate the cache-miss path would have given us. A non-positive entry is treated as no entry at all, as it is no more usable than a missing one and must not pin the target to the floor either. The defaults are sized for a chain that has a fee market, though, and `regtest`/`signet` don't have one: regtest Esplora serves an empty map, as `estimatesmartfee` has nothing to go on there, so the fallback would decide every rate rather than the odd sparse-map target. We therefore stand in the 1 sat/vb we have always used on those two networks, which is the same split `BitcoindChainSource` already makes, and adjust that stand-in exactly as we would an estimate, because that is what those networks have always cached: `MaximumFeeEstimate` stays at 2775 sats/kwu rather than dropping to the raw 250, which matters as LDK sizes a channel's dust exposure limit off it. `get_fallback_fee_rate_for_network` holds that decision so the remaining chain sources can share it. Finally, we log the targets we had to fall back to a default for. On `regtest`/`signet` that is every target on every update, so we keep quiet there, but anywhere else it means we're pricing transactions off defaults rather than off the chain, which shouldn't pass silently. To allow testing this without an Esplora server, we move the cache construction to `build_fee_rate_cache` and add coverage for sparse maps, for empty maps on and off Mainnet, for non-positive entries, for the network-dependent fallback, and for the unchanged complete-map behavior. This change was written with the assistance of Claude Code. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016RDZbFbxkpNupzvTFDsL43 --- src/chain/esplora.rs | 272 ++++++++++++++++++++++++++++++++++++++----- src/fee_estimator.rs | 79 ++++++++++++- 2 files changed, 318 insertions(+), 33 deletions(-) diff --git a/src/chain/esplora.rs b/src/chain/esplora.rs index 1c13f141fb..541f84f231 100644 --- a/src/chain/esplora.rs +++ b/src/chain/esplora.rs @@ -24,8 +24,9 @@ use crate::config::{ MAX_FULL_SCAN_STOP_GAP, MIN_FULL_SCAN_STOP_GAP, }; use crate::fee_estimator::{ - apply_post_estimation_adjustments, get_all_conf_targets, get_num_block_defaults_for_target, - OnchainFeeEstimator, + apply_post_estimation_adjustments, get_all_conf_targets, get_fallback_fee_rate_for_network, + get_num_block_defaults_for_target, ConfirmationTarget, OnchainFeeEstimator, + ONE_SAT_PER_VB_SATS_PER_KWU, }; use crate::io::utils::update_and_persist_node_metrics; use crate::logger::{log_bytes, log_debug, log_error, log_trace, log_warn, LdkLogger, Logger}; @@ -366,44 +367,40 @@ impl EsploraChainSource { Error::FeerateEstimationUpdateFailed })?; - if estimates.is_empty() && self.config.network == Network::Bitcoin { - // Ensure we fail if we didn't receive any estimates. - log_error!( + let (new_fee_rate_cache, fell_back_targets) = + build_fee_rate_cache(estimates, self.config.network).inspect_err(|_| { + log_error!( self.logger, "Failed to retrieve fee rate estimates: empty fee estimates are dissallowed on Mainnet.", ); - return Err(Error::FeerateEstimationUpdateFailed); - } - - let confirmation_targets = get_all_conf_targets(); - - let mut new_fee_rate_cache = HashMap::with_capacity(10); - for target in confirmation_targets { - let num_blocks = get_num_block_defaults_for_target(target); - - // Convert the retrieved fee rate and fall back to 1 sat/vb if we fail or it - // yields less than that. This is mostly necessary to continue on - // `signet`/`regtest` where we might not get estimates (or bogus values). - let converted_estimate_sat_vb = - esplora_client::convert_fee_rate(num_blocks, estimates.clone()) - .map_or(1.0, |converted| converted.max(1.0)); - - let fee_rate = FeeRate::from_sat_per_kwu((converted_estimate_sat_vb * 250.0) as u64); - - // LDK 0.0.118 introduced changes to the `ConfirmationTarget` semantics that - // require some post-estimation adjustments to the fee rates, which we do here. - let adjusted_fee_rate = apply_post_estimation_adjustments(target, fee_rate); - - new_fee_rate_cache.insert(target, adjusted_fee_rate); + })?; - log_trace!( + // On `regtest`/`signet` we expect to fall back for every target, as the backends serve no + // estimates there, so a warning would be pure noise. Elsewhere it means we're pricing + // transactions off defaults rather than off the chain, which is worth knowing about. + if !fell_back_targets.is_empty() + && !matches!(self.config.network, Network::Regtest | Network::Signet) + { + log_warn!( self.logger, - "Fee rate estimation updated for {:?}: {} sats/kwu", - target, - adjusted_fee_rate.to_sat_per_kwu(), + "Received no usable fee rate estimate for {:?}, falling back to their defaults.", + fell_back_targets, ); } + // Note we iterate the targets rather than the cache, as a `HashMap`'s order varies from + // run to run and we'd rather these lines stay comparable across restarts. + for target in get_all_conf_targets() { + if let Some(fee_rate) = new_fee_rate_cache.get(&target) { + log_trace!( + self.logger, + "Fee rate estimation updated for {:?}: {} sats/kwu", + target, + fee_rate.to_sat_per_kwu(), + ); + } + } + self.fee_estimator.set_fee_rate_cache(new_fee_rate_cache); log_debug!( @@ -538,3 +535,214 @@ impl Filter for EsploraChainSource { self.tx_sync.register_output(output); } } + +// Builds the fee rate cache from the estimates Esplora returned for us. +// +// Note that Esplora may hand us a sparse map that has no usable estimate for some of our +// confirmation targets. Rather than putting those on 1 sat/vb, which would silently downgrade +// urgent targets such as `UrgentOnChainSweep`, we fall back to what +// `get_fallback_fee_rate_for_network` gives us, just as the Electrum and Bitcoin Core sources do. +fn build_fee_rate_cache( + estimates: HashMap, network: Network, +) -> Result<(HashMap, Vec), Error> { + if estimates.is_empty() && network == Network::Bitcoin { + // Ensure we fail if we didn't receive any estimates. + return Err(Error::FeerateEstimationUpdateFailed); + } + + let confirmation_targets = get_all_conf_targets(); + + let mut new_fee_rate_cache = HashMap::with_capacity(10); + let mut fell_back_targets = Vec::new(); + for target in confirmation_targets { + let num_blocks = get_num_block_defaults_for_target(target); + + // Note a non-positive value is no more usable than a missing one, so we treat it as + // missing rather than letting it pin the target to 1 sat/vb. + let converted = esplora_client::convert_fee_rate(num_blocks, estimates.clone()) + .filter(|converted| *converted > 0.0); + + let fee_rate = match converted { + Some(converted) => { + // Enforce a lower bound of 1 sat/vb on the converted estimate. This is mostly + // necessary to continue on `signet`/`regtest` where we might get bogus values. + let estimate_sats_kwu = + ((converted * 250.0) as u64).max(ONE_SAT_PER_VB_SATS_PER_KWU); + let estimate = FeeRate::from_sat_per_kwu(estimate_sats_kwu); + + // LDK 0.0.118 introduced changes to the `ConfirmationTarget` semantics that + // require some post-estimation adjustments to the fee rates, which we do here. + apply_post_estimation_adjustments(target, estimate) + }, + // We have no estimate to adjust, so we fall back. + None => { + fell_back_targets.push(target); + get_fallback_fee_rate_for_network(target, network) + }, + }; + + new_fee_rate_cache.insert(target, fee_rate); + } + + Ok((new_fee_rate_cache, fell_back_targets)) +} + +#[cfg(test)] +mod tests { + use lightning::chain::chaininterface::ConfirmationTarget as LdkConfirmationTarget; + + use super::*; + use crate::fee_estimator::get_fallback_rate_for_target; + + // Esplora only gives us an estimate for a target if it knows a block count at or below the + // requested one, so a map holding nothing but high block counts starves our urgent targets. + fn sparse_estimates() -> HashMap { + HashMap::from([(144, 2.0), (1008, 1.0)]) + } + + #[test] + fn sparse_estimates_fall_back_to_target_defaults() { + let (cache, _) = build_fee_rate_cache(sparse_estimates(), Network::Bitcoin).unwrap(); + + // These have no estimate at or below their block count, so they have to end up on their + // own defaults rather than on the 1 sat/vb (i.e., 250 sats/kwu) relay floor. + for (target, expected) in [ + (ConfirmationTarget::Lightning(LdkConfirmationTarget::UrgentOnChainSweep), 5000), + (ConfirmationTarget::Lightning(LdkConfirmationTarget::MaximumFeeEstimate), 8000), + (ConfirmationTarget::OnchainPayment, 5000), + (ConfirmationTarget::ChannelFunding, 1000), + ] { + assert_eq!(cache.get(&target).unwrap().to_sat_per_kwu(), expected, "{:?}", target); + } + + // While the targets that do have an estimate keep using it. + assert_eq!( + cache + .get(&ConfirmationTarget::Lightning(LdkConfirmationTarget::ChannelCloseMinimum)) + .unwrap() + .to_sat_per_kwu(), + 500 + ); + } + + #[test] + fn empty_estimates_are_rejected_on_mainnet_only() { + assert!(matches!( + build_fee_rate_cache(HashMap::new(), Network::Bitcoin), + Err(Error::FeerateEstimationUpdateFailed) + )); + + // On testnet we carry on, with every target on its own default, and report all of them + // as fallen back so the caller can say so in the logs. + let (cache, fell_back) = build_fee_rate_cache(HashMap::new(), Network::Testnet).unwrap(); + assert_eq!(cache.len(), get_all_conf_targets().len()); + assert_eq!(fell_back.len(), get_all_conf_targets().len()); + for target in get_all_conf_targets() { + assert_eq!( + cache.get(&target).unwrap().to_sat_per_kwu(), + get_fallback_rate_for_target(target) as u64, + "{:?}", + target + ); + } + } + + #[test] + fn non_positive_estimates_fall_back_rather_than_pinning_the_floor() { + // A bogus entry is no more usable than a missing one, so it must not pin the target to + // 1 sat/vb - that is the very thing this fallback exists to avoid. + for bogus in [0.0, -1.0] { + let estimates = HashMap::from([(6, bogus), (144, 2.0)]); + let (cache, fell_back) = build_fee_rate_cache(estimates, Network::Bitcoin).unwrap(); + + let urgent = ConfirmationTarget::Lightning(LdkConfirmationTarget::UrgentOnChainSweep); + assert_eq!( + cache.get(&urgent).unwrap().to_sat_per_kwu(), + get_fallback_rate_for_target(urgent) as u64, + "{}", + bogus + ); + assert!(fell_back.contains(&urgent), "{}", bogus); + } + } + + #[test] + fn no_estimates_stay_on_one_sat_per_vb_on_regtest_and_signet() { + // `regtest`/`signet` have no fee market at all - regtest Esplora serves an empty map, as + // `estimatesmartfee` has nothing to go on - so the defaults would decide every rate + // there. We keep the 1 sat/vb we've always used instead, adjusted as an estimate would + // be, which is exactly what those networks cached before this change. + for network in [Network::Regtest, Network::Signet] { + for estimates in [HashMap::new(), sparse_estimates()] { + let (cache, _) = build_fee_rate_cache(estimates, network).unwrap(); + + assert_eq!( + cache + .get(&ConfirmationTarget::Lightning( + LdkConfirmationTarget::UrgentOnChainSweep + )) + .unwrap() + .to_sat_per_kwu(), + ONE_SAT_PER_VB_SATS_PER_KWU, + "{}", + network + ); + assert_eq!( + cache.get(&ConfirmationTarget::ChannelFunding).unwrap().to_sat_per_kwu(), + ONE_SAT_PER_VB_SATS_PER_KWU, + "{}", + network + ); + + // The one target the adjustment moves: LDK sizes its per-channel dust exposure + // limit off this, so caching the raw 250 here would cut that limit ~11x. + assert_eq!( + cache + .get(&ConfirmationTarget::Lightning( + LdkConfirmationTarget::MaximumFeeEstimate + )) + .unwrap() + .to_sat_per_kwu(), + 2775, + "{}", + network + ); + } + } + } + + #[test] + fn complete_estimates_are_used_and_adjusted() { + let estimates = HashMap::from([(1, 10.0), (6, 5.0), (12, 4.0), (144, 2.0), (1008, 1.0)]); + let (cache, _) = build_fee_rate_cache(estimates, Network::Bitcoin).unwrap(); + + // 5 sat/vb, no post-estimation adjustment. + assert_eq!( + cache + .get(&ConfirmationTarget::Lightning(LdkConfirmationTarget::UrgentOnChainSweep)) + .unwrap() + .to_sat_per_kwu(), + 1250 + ); + + // 10 sat/vb, bumped by `x * 1.1 + 2500`. + assert_eq!( + cache + .get(&ConfirmationTarget::Lightning(LdkConfirmationTarget::MaximumFeeEstimate)) + .unwrap() + .to_sat_per_kwu(), + 5250 + ); + + // 2 sat/vb, less 250 sats/kwu, but clamped back up to the relay floor. + assert_eq!( + cache + .get(&ConfirmationTarget::Lightning( + LdkConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee + )) + .unwrap() + .to_sat_per_kwu(), + 253 + ); + } +} diff --git a/src/fee_estimator.rs b/src/fee_estimator.rs index b785bfca40..a4175ec97e 100644 --- a/src/fee_estimator.rs +++ b/src/fee_estimator.rs @@ -8,7 +8,7 @@ use std::collections::HashMap; use std::sync::RwLock; -use bitcoin::FeeRate; +use bitcoin::{FeeRate, Network}; use lightning::chain::chaininterface::{ ConfirmationTarget as LdkConfirmationTarget, FeeEstimator as LdkFeeEstimator, FEERATE_FLOOR_SATS_PER_KW, @@ -122,6 +122,36 @@ pub(crate) fn get_fallback_rate_for_ldk_target(target: LdkConfirmationTarget) -> } } +/// 1 sat/vb in sats/kwu, i.e. the lowest rate we'd ever consider using. Note this sits just +/// below LDK's `FEERATE_FLOOR_SATS_PER_KW`, which `estimate_fee_rate` clamps reads to. +pub(crate) const ONE_SAT_PER_VB_SATS_PER_KWU: u64 = 250; + +/// Returns the fee rate to use for `target` when the chain source handed us no usable estimate +/// for it. +/// +/// Off `regtest`/`signet` we use the target's own default, as falling back to 1 sat/vb would +/// silently downgrade urgent targets such as `UrgentOnChainSweep`. We deliberately +/// don't run that default through `apply_post_estimation_adjustments`, mirroring what +/// `OnchainFeeEstimator::estimate_fee_rate` returns on a cache miss. +/// +/// On `regtest`/`signet` there is no fee market to speak of - the backends commonly serve no +/// estimates there at all - so pulling every target up to a mainnet-sized default would be wrong. +/// We stand in the 1 sat/vb we have always used there, and adjust it exactly as we would an +/// estimate, because that is what these networks have always cached: `MaximumFeeEstimate` ends up +/// at 2775 sats/kwu rather than the raw 250, and LDK derives its per-channel dust exposure limit +/// from that value. +pub(crate) fn get_fallback_fee_rate_for_network( + target: ConfirmationTarget, network: Network, +) -> FeeRate { + match network { + Network::Regtest | Network::Signet => apply_post_estimation_adjustments( + target, + FeeRate::from_sat_per_kwu(ONE_SAT_PER_VB_SATS_PER_KWU), + ), + _ => FeeRate::from_sat_per_kwu(get_fallback_rate_for_target(target) as u64), + } +} + pub(crate) fn get_all_conf_targets() -> [ConfirmationTarget; 10] { [ ConfirmationTarget::OnchainPayment, @@ -191,6 +221,53 @@ pub(crate) fn rbf_splice_feerates( mod tests { use super::*; + #[test] + fn fallback_fee_rates_are_network_aware() { + // Mainnet and testnet have a fee market, so a target with no usable estimate gets its own + // default rather than the relay floor. + for network in [Network::Bitcoin, Network::Testnet, Network::Testnet4] { + for target in get_all_conf_targets() { + assert_eq!( + get_fallback_fee_rate_for_network(target, network).to_sat_per_kwu(), + get_fallback_rate_for_target(target) as u64, + "{:?} on {}", + target, + network + ); + } + } + + // Regtest and signet don't, so we stand in 1 sat/vb there and avoid pulling every target + // up to a mainnet-sized default. The stand-in is adjusted as an estimate would be, which + // is what those networks have always cached. + for network in [Network::Regtest, Network::Signet] { + for target in get_all_conf_targets() { + let one_sat_per_vb = FeeRate::from_sat_per_kwu(ONE_SAT_PER_VB_SATS_PER_KWU); + assert_eq!( + get_fallback_fee_rate_for_network(target, network), + apply_post_estimation_adjustments(target, one_sat_per_vb), + "{:?} on {}", + target, + network + ); + } + + // Spelled out for the one target where the adjustment matters, as LDK derives its + // per-channel dust exposure limit from it: dropping this to the raw 250 would cut + // that limit ~11x on every regtest/signet node. + assert_eq!( + get_fallback_fee_rate_for_network( + ConfirmationTarget::Lightning(LdkConfirmationTarget::MaximumFeeEstimate), + network + ) + .to_sat_per_kwu(), + 2775, + "{}", + network + ); + } + } + #[test] fn rbf_splice_feerates_target_and_max() { let kwu = FeeRate::from_sat_per_kwu; From 260607e6721c2dead1bc9736889ed75f6cdfc15f Mon Sep 17 00:00:00 2001 From: jolah1 Date: Wed, 14 Oct 2026 08:01:55 +0100 Subject: [PATCH 2/2] Use the same fee estimate fallback for Electrum and Bitcoin Core A missing estimate should mean the same thing no matter which backend we happen to be on, so have the remaining two chain sources fall back through `get_fallback_fee_rate_for_network` as well. For Electrum that covers the results we can't use: a server that has no estimate for the requested block count answers `-1`, and a result may not be a number at all. Both used to be floored to 1 sat/vb, so the silent downgrade of urgent targets this series removes from the Esplora source was reachable here too, mainnet included. We report the targets we had to fall back for, as the Esplora source now does, and index into the batch results rather than zipping them with the targets, so that a short response could never silently drop the trailing targets from the cache. Note `batch_call` returns one result per request or an error, so that isn't a case we can hit today. For Bitcoin Core it means we no longer abandon the whole update when a single `estimatesmartfee` call fails off mainnet, which kept none of the estimates we did get, and that the fallback no longer runs through the post-estimation adjustments. The regtest/signet rates are unchanged, and mainnet still fails hard. This change was written with the assistance of Claude Code. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016RDZbFbxkpNupzvTFDsL43 --- src/chain/bitcoind.rs | 42 ++++------ src/chain/electrum.rs | 186 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 179 insertions(+), 49 deletions(-) diff --git a/src/chain/bitcoind.rs b/src/chain/bitcoind.rs index 9ed38c2128..2ec30cd6a9 100644 --- a/src/chain/bitcoind.rs +++ b/src/chain/bitcoind.rs @@ -37,8 +37,8 @@ use crate::config::{ DEFAULT_TX_BROADCAST_TIMEOUT_SECS, }; use crate::fee_estimator::{ - apply_post_estimation_adjustments, get_all_conf_targets, get_num_block_defaults_for_target, - ConfirmationTarget, OnchainFeeEstimator, + apply_post_estimation_adjustments, get_all_conf_targets, get_fallback_fee_rate_for_network, + get_num_block_defaults_for_target, ConfirmationTarget, OnchainFeeEstimator, }; use crate::io::utils::update_and_persist_node_metrics; use crate::logger::{log_bytes, log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; @@ -603,45 +603,39 @@ impl BitcoindChainSource { }; let fee_rate = match (fee_rate_update_res, self.config.network) { - (Ok(rate), _) => rate, + (Ok(rate), _) => { + // LDK 0.0.118 introduced changes to the `ConfirmationTarget` semantics that + // require some post-estimation adjustments to the fee rates, which we do here. + apply_post_estimation_adjustments(target, rate) + }, (Err(e), Network::Bitcoin) => { // Strictly fail on mainnet. log_error!(self.logger, "Failed to retrieve fee rate estimates: {}", e); return Err(Error::FeerateEstimationUpdateFailed); }, - (Err(e), n) if n == Network::Regtest || n == Network::Signet => { - // On regtest/signet we just fall back to the usual 1 sat/vb == 250 - // sat/kwu default. - log_error!( - self.logger, - "Failed to retrieve fee rate estimates: {}. Falling back to default of 1 sat/vb.", - e, - ); - FeeRate::from_sat_per_kwu(250) - }, - (Err(e), _) => { - // On testnet `estimatesmartfee` can be unreliable so we just skip in - // case of a failure, which will have us falling back to defaults. + (Err(e), network) => { + // Off mainnet we carry on with this target's fallback: on regtest/signet + // there is no fee market to begin with, and on testnet `estimatesmartfee` + // can simply be unreliable. Note this keeps whatever estimates we did get + // for the remaining targets. + let fallback_fee_rate = get_fallback_fee_rate_for_network(target, network); log_error!( self.logger, - "Failed to retrieve fee rate estimates: {}. Falling back to defaults.", + "Failed to retrieve fee rate estimates: {}. Falling back to {} sats/kwu.", e, + fallback_fee_rate.to_sat_per_kwu(), ); - return Ok(()); + fallback_fee_rate }, }; - // LDK 0.0.118 introduced changes to the `ConfirmationTarget` semantics that - // require some post-estimation adjustments to the fee rates, which we do here. - let adjusted_fee_rate = apply_post_estimation_adjustments(target, fee_rate); - - new_fee_rate_cache.insert(target, adjusted_fee_rate); + new_fee_rate_cache.insert(target, fee_rate); log_trace!( self.logger, "Fee rate estimation updated for {:?}: {} sats/kwu", target, - adjusted_fee_rate.to_sat_per_kwu(), + fee_rate.to_sat_per_kwu(), ); } diff --git a/src/chain/electrum.rs b/src/chain/electrum.rs index 86025998e8..6f4db24e16 100644 --- a/src/chain/electrum.rs +++ b/src/chain/electrum.rs @@ -32,8 +32,9 @@ use crate::config::{ }; use crate::error::Error; use crate::fee_estimator::{ - apply_post_estimation_adjustments, get_all_conf_targets, get_num_block_defaults_for_target, - ConfirmationTarget, OnchainFeeEstimator, + apply_post_estimation_adjustments, get_all_conf_targets, get_fallback_fee_rate_for_network, + get_num_block_defaults_for_target, ConfirmationTarget, OnchainFeeEstimator, + ONE_SAT_PER_VB_SATS_PER_KWU, }; use crate::io::utils::update_and_persist_node_metrics; use crate::logger::{log_bytes, log_debug, log_error, log_trace, log_warn, LdkLogger, Logger}; @@ -789,35 +790,41 @@ impl ElectrumRuntimeClient { } let mut new_fee_rate_cache = HashMap::with_capacity(10); - for (target, raw_fee_rate_btc_per_kvb) in - confirmation_targets.into_iter().zip(raw_estimates_btc_kvb.into_iter()) - { - // Parse the retrieved serde_json::Value and fall back to 1 sat/vb (10^3 / 10^8 = 10^-5 - // = 0.00001 btc/kvb) if we fail or it yields less than that. This is mostly necessary - // to continue on `signet`/`regtest` where we might not get estimates (or bogus - // values). - let fee_rate_btc_per_kvb = raw_fee_rate_btc_per_kvb - .as_f64() - .map_or(0.00001, |converted| converted.max(0.00001)); - - // Electrum, just like Bitcoin Core, gives us a feerate in BTC/KvB. - // Thus, we multiply by 25_000_000 (10^8 / 4) to get satoshis/kwu. - let fee_rate = { - let fee_rate_sat_per_kwu = (fee_rate_btc_per_kvb * 25_000_000.0).round() as u64; - FeeRate::from_sat_per_kwu(fee_rate_sat_per_kwu) - }; - - // LDK 0.0.118 introduced changes to the `ConfirmationTarget` semantics that - // require some post-estimation adjustments to the fee rates, which we do here. - let adjusted_fee_rate = apply_post_estimation_adjustments(target, fee_rate); + let mut fell_back_targets = Vec::new(); + for (index, target) in confirmation_targets.into_iter().enumerate() { + // `batch_call` gives us one result per request or an error, and the check above + // rejects anything else on mainnet. We still index rather than zip so that a short + // response could never silently drop the trailing targets from the cache. + let raw_fee_rate_btc_per_kvb = + raw_estimates_btc_kvb.get(index).and_then(|raw| raw.as_f64()); + + let (fee_rate, fell_back) = + fee_rate_from_estimate(target, raw_fee_rate_btc_per_kvb, self.config.network); + + if fell_back { + fell_back_targets.push(target); + } - new_fee_rate_cache.insert(target, adjusted_fee_rate); + new_fee_rate_cache.insert(target, fee_rate); log_trace!( self.logger, "Fee rate estimation updated for {:?}: {} sats/kwu", target, - adjusted_fee_rate.to_sat_per_kwu(), + fee_rate.to_sat_per_kwu(), + ); + } + + // On `regtest`/`signet` we expect to fall back for every target, so a warning would be + // pure noise there. Elsewhere it means we're pricing transactions off defaults rather + // than off the chain, which shouldn't pass silently. + if !fell_back_targets.is_empty() + && !matches!(self.config.network, Network::Regtest | Network::Signet) + { + log_warn!( + self.logger, + "Received no usable fee rate estimate for {:?}, falling back to their defaults.", + fell_back_targets, ); } @@ -825,6 +832,38 @@ impl ElectrumRuntimeClient { } } +// Converts a raw `estimatefee` result, as returned by the Electrum server for `target`, into the +// fee rate we'll cache for it, and whether we had to fall back for it. +// +// Note that Electrum serves `-1` when it has no estimate for the requested block count, and that a +// result may not be a number at all. In either case we have no usable estimate and fall back rather +// than putting the target on 1 sat/vb, which would silently downgrade urgent targets such as +// `UrgentOnChainSweep`. +fn fee_rate_from_estimate( + target: ConfirmationTarget, estimate_btc_per_kvb: Option, network: Network, +) -> (FeeRate, bool) { + // Electrum, just like Bitcoin Core, gives us a feerate in BTC/KvB. Thus, we multiply by + // 25_000_000 (10^8 / 4) to get satoshis/kwu. + let estimate_sats_kwu = estimate_btc_per_kvb + .filter(|converted| *converted > 0.0) + .map(|converted| (converted * 25_000_000.0).round() as u64); + + match estimate_sats_kwu { + Some(estimate_sats_kwu) => { + // Enforce a lower bound of 1 sat/vb on the converted estimate. This is mostly + // necessary to continue on `signet`/`regtest` where we might get bogus values. + let estimate = + FeeRate::from_sat_per_kwu(estimate_sats_kwu.max(ONE_SAT_PER_VB_SATS_PER_KWU)); + + // LDK 0.0.118 introduced changes to the `ConfirmationTarget` semantics that + // require some post-estimation adjustments to the fee rates, which we do here. + (apply_post_estimation_adjustments(target, estimate), false) + }, + // We have no estimate to adjust, so we fall back. + None => (get_fallback_fee_rate_for_network(target, network), true), + } +} + struct ConfirmGate { active: Mutex, } @@ -924,9 +963,106 @@ mod tests { use std::thread; use bitcoin::blockdata::constants::genesis_block; + use lightning::chain::chaininterface::ConfirmationTarget as LdkConfirmationTarget; use super::*; + #[test] + fn missing_estimates_fall_back_per_network() { + // `-1` is what Electrum serves when it has no estimate for the requested block count, + // and a short batch response leaves us without a value at all. Neither is a usable + // estimate. + for raw in [None, Some(-1.0)] { + // On mainnet/testnet the target's own default decides, rather than the relay floor + // silently downgrading urgent targets to 1 sat/vb. + for network in [Network::Bitcoin, Network::Testnet] { + let (fee_rate, fell_back) = fee_rate_from_estimate( + ConfirmationTarget::Lightning(LdkConfirmationTarget::UrgentOnChainSweep), + raw, + network, + ); + assert_eq!(fee_rate.to_sat_per_kwu(), 5000, "{:?} on {}", raw, network); + assert!(fell_back, "{:?} on {}", raw, network); + + let (fee_rate, fell_back) = + fee_rate_from_estimate(ConfirmationTarget::ChannelFunding, raw, network); + assert_eq!(fee_rate.to_sat_per_kwu(), 1000, "{:?} on {}", raw, network); + assert!(fell_back, "{:?} on {}", raw, network); + } + + // While on regtest/signet we stay on the 1 sat/vb stand-in, adjusted as an estimate + // would be - so `MaximumFeeEstimate` has to come out at 2775 here too, as LDK sizes + // a channel's dust exposure limit off it. + for network in [Network::Regtest, Network::Signet] { + assert_eq!( + fee_rate_from_estimate( + ConfirmationTarget::Lightning(LdkConfirmationTarget::MaximumFeeEstimate), + raw, + network + ) + .0 + .to_sat_per_kwu(), + 2775, + "{:?} on {}", + raw, + network + ); + assert_eq!( + fee_rate_from_estimate( + ConfirmationTarget::Lightning(LdkConfirmationTarget::UrgentOnChainSweep), + raw, + network + ) + .0 + .to_sat_per_kwu(), + ONE_SAT_PER_VB_SATS_PER_KWU, + "{:?} on {}", + raw, + network + ); + } + } + } + + #[test] + fn estimates_are_converted_floored_and_adjusted() { + // 0.00002 btc/kvb == 2 sat/vb == 500 sats/kwu, no post-estimation adjustment. + assert_eq!( + fee_rate_from_estimate( + ConfirmationTarget::Lightning(LdkConfirmationTarget::UrgentOnChainSweep), + Some(0.00002), + Network::Bitcoin + ) + .0 + .to_sat_per_kwu(), + 500 + ); + + // The same estimate bumped by `x * 1.1 + 2500` for `MaximumFeeEstimate`. + assert_eq!( + fee_rate_from_estimate( + ConfirmationTarget::Lightning(LdkConfirmationTarget::MaximumFeeEstimate), + Some(0.00002), + Network::Bitcoin + ) + .0 + .to_sat_per_kwu(), + 3050 + ); + + // A dust-but-positive estimate is floored at 1 sat/vb, not taken at face value. + assert_eq!( + fee_rate_from_estimate( + ConfirmationTarget::OnchainPayment, + Some(0.000001), + Network::Bitcoin + ) + .0 + .to_sat_per_kwu(), + ONE_SAT_PER_VB_SATS_PER_KWU + ); + } + struct RecordingConfirm { calls: AtomicUsize, relevant_txid: Txid,