Introduce FeerateStrategy enum for onchain claims
authorWilmer Paulino <wilmer@wilmerpaulino.com>
Tue, 30 Jan 2024 22:45:44 +0000 (14:45 -0800)
committerWilmer Paulino <wilmer@wilmerpaulino.com>
Wed, 7 Feb 2024 23:42:28 +0000 (15:42 -0800)
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
lightning/src/chain/onchaintx.rs
lightning/src/chain/package.rs

index cc0104720745913f75f6985ca013db3ba064553d..1cfcc3a4b8746c7459ce462034dda15ea86342d2 100644 (file)
@@ -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<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
                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,
                );
        }
 
index 05d59431fc3a815585554eae7ffd5323d72b4112..a629ecdefc273295563f5e77c57a16ea03a7c0e5 100644 (file)
@@ -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<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
        /// invoking this every 30 seconds, or lower if running in an environment with spotty
        /// connections, like on mobile.
        pub(super) fn rebroadcast_pending_claims<B: Deref, F: Deref, L: Logger>(
-               &mut self, current_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>,
-               logger: &L,
+               &mut self, current_height: u32, feerate_strategy: FeerateStrategy, broadcaster: &B,
+               fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
        )
        where
                B::Target: BroadcasterInterface,
@@ -488,7 +497,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                        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<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
        /// 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<F: Deref, L: Logger>(
-               &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<F>, logger: &L,
        ) -> Option<(u32, u64, OnchainClaim)>
        where F::Target: FeeEstimator,
@@ -577,7 +586,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                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<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                        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<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 
                                        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::<u64>();
                                                let commitment_tx_feerate_sat_per_1000_weight =
@@ -767,7 +776,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                // 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<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                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<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                        // `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);
index efc32bf7d40d9adcdfaff19ef23e3dc66b363bd7..52cdf9f37fd8a57a00837faa74eab572c5929fa4 100644 (file)
@@ -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<F: Deref, L: Logger>(
-               &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<F>, 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<F: Deref>(
                &self, fee_estimator: &LowerBoundedFeeEstimator<F>, 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<F: Deref, L: Logger>(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<F: Deref, L: Logger>(
-       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<F>, 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);