From 043ab75bb4bd32205bbf890837f661dd3a547cb5 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 30 Jan 2024 14:45:44 -0800 Subject: [PATCH] Introduce FeerateStrategy enum for onchain claims This refactors the existing `force_feerate_bump` flag into an enum as we plan to introduce a new flag/enum variant in a future commit. --- lightning/src/chain/channelmonitor.rs | 4 +- lightning/src/chain/onchaintx.rs | 29 ++++++--- lightning/src/chain/package.rs | 90 ++++++++++++++------------- 3 files changed, 68 insertions(+), 55 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index cc010472..1cfcc3a4 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -44,7 +44,7 @@ use crate::chain::{BestBlock, WatchedOutput}; use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator}; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, ecdsa::WriteableEcdsaChannelSigner, SignerProvider, EntropySource}; -use crate::chain::onchaintx::{ClaimEvent, OnchainTxHandler}; +use crate::chain::onchaintx::{ClaimEvent, FeerateStrategy, OnchainTxHandler}; use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput}; use crate::chain::Filter; use crate::util::logger::{Logger, Record}; @@ -1765,7 +1765,7 @@ impl ChannelMonitor { let logger = WithChannelMonitor::from_impl(logger, &*inner); let current_height = inner.best_block.height; inner.onchain_tx_handler.rebroadcast_pending_claims( - current_height, &broadcaster, &fee_estimator, &logger, + current_height, FeerateStrategy::HighestOfPreviousOrNew, &broadcaster, &fee_estimator, &logger, ); } diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 05d59431..a629ecde 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -210,6 +210,15 @@ pub(crate) enum OnchainClaim { Event(ClaimEvent), } +/// Represents the different feerates a pending request can use when generating a claim. +pub(crate) enum FeerateStrategy { + /// We must pick the highest between the most recently used and the current feerate estimate. + HighestOfPreviousOrNew, + /// We must force a bump of the most recently used feerate, either by using the current feerate + /// estimate if it's higher, or manually bumping. + ForceBump, +} + /// OnchainTxHandler receives claiming requests, aggregates them if it's sound, broadcast and /// do RBF bumping if possible. #[derive(Clone)] @@ -474,8 +483,8 @@ impl OnchainTxHandler /// invoking this every 30 seconds, or lower if running in an environment with spotty /// connections, like on mobile. pub(super) fn rebroadcast_pending_claims( - &mut self, current_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator, - logger: &L, + &mut self, current_height: u32, feerate_strategy: FeerateStrategy, broadcaster: &B, + fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) where B::Target: BroadcasterInterface, @@ -488,7 +497,7 @@ impl OnchainTxHandler bump_requests.push((*claim_id, request.clone())); } for (claim_id, request) in bump_requests { - self.generate_claim(current_height, &request, false /* force_feerate_bump */, fee_estimator, logger) + self.generate_claim(current_height, &request, &feerate_strategy, fee_estimator, logger) .map(|(_, new_feerate, claim)| { let mut bumped_feerate = false; if let Some(mut_request) = self.pending_claim_requests.get_mut(&claim_id) { @@ -528,7 +537,7 @@ impl OnchainTxHandler /// Panics if there are signing errors, because signing operations in reaction to on-chain /// events are not expected to fail, and if they do, we may lose funds. fn generate_claim( - &mut self, cur_height: u32, cached_request: &PackageTemplate, force_feerate_bump: bool, + &mut self, cur_height: u32, cached_request: &PackageTemplate, feerate_strategy: &FeerateStrategy, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u32, u64, OnchainClaim)> where F::Target: FeeEstimator, @@ -577,7 +586,7 @@ impl OnchainTxHandler if cached_request.is_malleable() { if cached_request.requires_external_funding() { let target_feerate_sat_per_1000_weight = cached_request.compute_package_feerate( - fee_estimator, ConfirmationTarget::OnChainSweep, force_feerate_bump + fee_estimator, ConfirmationTarget::OnChainSweep, feerate_strategy, ); if let Some(htlcs) = cached_request.construct_malleable_package_with_external_funding(self) { return Some(( @@ -597,7 +606,7 @@ impl OnchainTxHandler let predicted_weight = cached_request.package_weight(&self.destination_script); if let Some((output_value, new_feerate)) = cached_request.compute_package_output( predicted_weight, self.destination_script.dust_value().to_sat(), - force_feerate_bump, fee_estimator, logger, + feerate_strategy, fee_estimator, logger, ) { assert!(new_feerate != 0); @@ -630,7 +639,7 @@ impl OnchainTxHandler let conf_target = ConfirmationTarget::OnChainSweep; let package_target_feerate_sat_per_1000_weight = cached_request - .compute_package_feerate(fee_estimator, conf_target, force_feerate_bump); + .compute_package_feerate(fee_estimator, conf_target, feerate_strategy); if let Some(input_amount_sat) = output.funding_amount { let fee_sat = input_amount_sat - tx.output.iter().map(|output| output.value).sum::(); let commitment_tx_feerate_sat_per_1000_weight = @@ -767,7 +776,7 @@ impl OnchainTxHandler // height timer expiration (i.e in how many blocks we're going to take action). for mut req in preprocessed_requests { if let Some((new_timer, new_feerate, claim)) = self.generate_claim( - cur_height, &req, true /* force_feerate_bump */, &*fee_estimator, &*logger, + cur_height, &req, &FeerateStrategy::ForceBump, &*fee_estimator, &*logger, ) { req.set_timer(new_timer); req.set_feerate(new_feerate); @@ -967,7 +976,7 @@ impl OnchainTxHandler log_trace!(logger, "Bumping {} candidates", bump_candidates.len()); for (claim_id, request) in bump_candidates.iter() { if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim( - cur_height, &request, true /* force_feerate_bump */, &*fee_estimator, &*logger, + cur_height, &request, &FeerateStrategy::ForceBump, &*fee_estimator, &*logger, ) { match bump_claim { OnchainClaim::Tx(bump_tx) => { @@ -1048,7 +1057,7 @@ impl OnchainTxHandler // `height` is the height being disconnected, so our `current_height` is 1 lower. let current_height = height - 1; if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim( - current_height, &request, true /* force_feerate_bump */, fee_estimator, logger + current_height, &request, &FeerateStrategy::ForceBump, fee_estimator, logger ) { request.set_timer(new_timer); request.set_feerate(new_feerate); diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index efc32bf7..52cdf9f3 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -29,7 +29,7 @@ use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; use crate::ln::msgs::DecodeError; use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, FEERATE_FLOOR_SATS_PER_KW}; use crate::sign::ecdsa::WriteableEcdsaChannelSigner; -use crate::chain::onchaintx::{ExternalHTLCClaim, OnchainTxHandler}; +use crate::chain::onchaintx::{FeerateStrategy, ExternalHTLCClaim, OnchainTxHandler}; use crate::util::logger::Logger; use crate::util::ser::{Readable, Writer, Writeable, RequiredWrapper}; @@ -963,7 +963,7 @@ impl PackageTemplate { /// which was used to generate the value. Will not return less than `dust_limit_sats` for the /// value. pub(crate) fn compute_package_output( - &self, predicted_weight: u64, dust_limit_sats: u64, force_feerate_bump: bool, + &self, predicted_weight: u64, dust_limit_sats: u64, feerate_strategy: &FeerateStrategy, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u64, u64)> where F::Target: FeeEstimator, @@ -974,7 +974,7 @@ impl PackageTemplate { // If old feerate is 0, first iteration of this claim, use normal fee calculation if self.feerate_previous != 0 { if let Some((new_fee, feerate)) = feerate_bump( - predicted_weight, input_amounts, self.feerate_previous, force_feerate_bump, + predicted_weight, input_amounts, self.feerate_previous, feerate_strategy, fee_estimator, logger, ) { return Some((cmp::max(input_amounts as i64 - new_fee as i64, dust_limit_sats as i64) as u64, feerate)); @@ -987,32 +987,31 @@ impl PackageTemplate { None } - /// Computes a feerate based on the given confirmation target. If a previous feerate was used, - /// the new feerate is below it, and `force_feerate_bump` is set, we'll use a 25% increase of - /// the previous feerate instead of the new feerate. + /// Computes a feerate based on the given confirmation target and feerate strategy. pub(crate) fn compute_package_feerate( &self, fee_estimator: &LowerBoundedFeeEstimator, conf_target: ConfirmationTarget, - force_feerate_bump: bool, + feerate_strategy: &FeerateStrategy, ) -> u32 where F::Target: FeeEstimator { let feerate_estimate = fee_estimator.bounded_sat_per_1000_weight(conf_target); if self.feerate_previous != 0 { - // Use the new fee estimate if it's higher than the one previously used. - if feerate_estimate as u64 > self.feerate_previous { - feerate_estimate - } else if !force_feerate_bump { - self.feerate_previous.try_into().unwrap_or(u32::max_value()) - } else { - // Our fee estimate has decreased, but our transaction remains unconfirmed after - // using our previous fee estimate. This may point to an unreliable fee estimator, - // so we choose to bump our previous feerate by 25%, making sure we don't use a - // lower feerate or overpay by a large margin by limiting it to 5x the new fee - // estimate. - let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); - let mut new_feerate = previous_feerate.saturating_add(previous_feerate / 4); - if new_feerate > feerate_estimate * 5 { - new_feerate = cmp::max(feerate_estimate * 5, previous_feerate); - } - new_feerate + let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); + match feerate_strategy { + FeerateStrategy::HighestOfPreviousOrNew => cmp::max(previous_feerate, feerate_estimate), + FeerateStrategy::ForceBump => if feerate_estimate > previous_feerate { + feerate_estimate + } else { + // Our fee estimate has decreased, but our transaction remains unconfirmed after + // using our previous fee estimate. This may point to an unreliable fee estimator, + // so we choose to bump our previous feerate by 25%, making sure we don't use a + // lower feerate or overpay by a large margin by limiting it to 5x the new fee + // estimate. + let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); + let mut new_feerate = previous_feerate.saturating_add(previous_feerate / 4); + if new_feerate > feerate_estimate * 5 { + new_feerate = cmp::max(feerate_estimate * 5, previous_feerate); + } + new_feerate + }, } } else { feerate_estimate @@ -1128,12 +1127,12 @@ fn compute_fee_from_spent_amounts(input_amounts: u64, predi /// Attempt to propose a bumping fee for a transaction from its spent output's values and predicted /// weight. If feerates proposed by the fee-estimator have been increasing since last fee-bumping -/// attempt, use them. If `force_feerate_bump` is set, we bump the feerate by 25% of the previous -/// feerate, or just use the previous feerate otherwise. If a feerate bump did happen, we also -/// verify that those bumping heuristics respect BIP125 rules 3) and 4) and if required adjust the -/// new fee to meet the RBF policy requirement. +/// attempt, use them. If we need to force a feerate bump, we manually bump the feerate by 25% of +/// the previous feerate. If a feerate bump did happen, we also verify that those bumping heuristics +/// respect BIP125 rules 3) and 4) and if required adjust the new fee to meet the RBF policy +/// requirement. fn feerate_bump( - predicted_weight: u64, input_amounts: u64, previous_feerate: u64, force_feerate_bump: bool, + predicted_weight: u64, input_amounts: u64, previous_feerate: u64, feerate_strategy: &FeerateStrategy, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u64, u64)> where @@ -1141,20 +1140,25 @@ where { // If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee... let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) = compute_fee_from_spent_amounts(input_amounts, predicted_weight, fee_estimator, logger) { - if new_feerate > previous_feerate { - (new_fee, new_feerate) - } else if !force_feerate_bump { - let previous_fee = previous_feerate * predicted_weight / 1000; - (previous_fee, previous_feerate) - } else { - // ...else just increase the previous feerate by 25% (because that's a nice number) - let bumped_feerate = previous_feerate + (previous_feerate / 4); - let bumped_fee = bumped_feerate * predicted_weight / 1000; - if input_amounts <= bumped_fee { - log_warn!(logger, "Can't 25% bump new claiming tx, amount {} is too small", input_amounts); - return None; - } - (bumped_fee, bumped_feerate) + match feerate_strategy { + FeerateStrategy::HighestOfPreviousOrNew => if new_feerate > previous_feerate { + (new_fee, new_feerate) + } else { + let previous_fee = previous_feerate * predicted_weight / 1000; + (previous_fee, previous_feerate) + }, + FeerateStrategy::ForceBump => if new_feerate > previous_feerate { + (new_fee, new_feerate) + } else { + // ...else just increase the previous feerate by 25% (because that's a nice number) + let bumped_feerate = previous_feerate + (previous_feerate / 4); + let bumped_fee = bumped_feerate * predicted_weight / 1000; + if input_amounts <= bumped_fee { + log_warn!(logger, "Can't 25% bump new claiming tx, amount {} is too small", input_amounts); + return None; + } + (bumped_fee, bumped_feerate) + }, } } else { log_warn!(logger, "Can't new-estimation bump new claiming tx, amount {} is too small", input_amounts); -- 2.30.2