Extend OnchainTxHandler::generate_claim to optionally force feerate bump
authorWilmer Paulino <wilmer@wilmerpaulino.com>
Thu, 20 Apr 2023 21:16:24 +0000 (14:16 -0700)
committerWilmer Paulino <wilmer@wilmerpaulino.com>
Fri, 21 Apr 2023 21:34:40 +0000 (14:34 -0700)
In the next commit, we plan to extend the `OnchainTxHandler` to retry
pending claims on a timer. This timer may fire with much more frequency
than incoming blocks, so we want to avoid manually bumping feerates
(currently by 25%) each time our fee estimator provides a lower feerate
than before.

lightning/src/chain/onchaintx.rs
lightning/src/chain/package.rs
lightning/src/ln/functional_tests.rs

index cd0cb08eab3522c00a057a7e9043c45c6a2a43a0..aece19c8371566bee10913d7f0404bbdba3ea62c 100644 (file)
@@ -489,9 +489,13 @@ 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: Deref>(&mut self, cur_height: u32, cached_request: &PackageTemplate, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u32, u64, OnchainClaim)>
-               where F::Target: FeeEstimator,
-                                       L::Target: Logger,
+       fn generate_claim<F: Deref, L: Deref>(
+               &mut self, cur_height: u32, cached_request: &PackageTemplate, force_feerate_bump: bool,
+               fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
+       ) -> Option<(u32, u64, OnchainClaim)>
+       where
+               F::Target: FeeEstimator,
+               L::Target: Logger,
        {
                let request_outpoints = cached_request.outpoints();
                if request_outpoints.is_empty() {
@@ -538,8 +542,9 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                        #[cfg(anchors)]
                        { // Attributes are not allowed on if expressions on our current MSRV of 1.41.
                                if cached_request.requires_external_funding() {
-                                       let target_feerate_sat_per_1000_weight = cached_request
-                                               .compute_package_feerate(fee_estimator, ConfirmationTarget::HighPriority);
+                                       let target_feerate_sat_per_1000_weight = cached_request.compute_package_feerate(
+                                               fee_estimator, ConfirmationTarget::HighPriority, force_feerate_bump
+                                       );
                                        if let Some(htlcs) = cached_request.construct_malleable_package_with_external_funding(self) {
                                                return Some((
                                                        new_timer,
@@ -558,7 +563,8 @@ 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(), fee_estimator, logger,
+                               predicted_weight, self.destination_script.dust_value().to_sat(),
+                               force_feerate_bump, fee_estimator, logger,
                        ) {
                                assert!(new_feerate != 0);
 
@@ -601,7 +607,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                                                        // counterparty's latest commitment don't have any HTLCs present.
                                                        let conf_target = ConfirmationTarget::HighPriority;
                                                        let package_target_feerate_sat_per_1000_weight = cached_request
-                                                               .compute_package_feerate(fee_estimator, conf_target);
+                                                               .compute_package_feerate(fee_estimator, conf_target, force_feerate_bump);
                                                        Some((
                                                                new_timer,
                                                                package_target_feerate_sat_per_1000_weight as u64,
@@ -700,7 +706,9 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                // Generate claim transactions and track them to bump if necessary at
                // 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, &*fee_estimator, &*logger) {
+                       if let Some((new_timer, new_feerate, claim)) = self.generate_claim(
+                               cur_height, &req, true /* force_feerate_bump */, &*fee_estimator, &*logger,
+                       ) {
                                req.set_timer(new_timer);
                                req.set_feerate(new_feerate);
                                let package_id = match claim {
@@ -893,7 +901,9 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                // Build, bump and rebroadcast tx accordingly
                log_trace!(logger, "Bumping {} candidates", bump_candidates.len());
                for (package_id, request) in bump_candidates.iter() {
-                       if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim(cur_height, &request, &*fee_estimator, &*logger) {
+                       if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim(
+                               cur_height, &request, true /* force_feerate_bump */, &*fee_estimator, &*logger,
+                       ) {
                                match bump_claim {
                                        OnchainClaim::Tx(bump_tx) => {
                                                log_info!(logger, "Broadcasting RBF-bumped onchain {}", log_tx!(bump_tx));
@@ -973,7 +983,9 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
                        }
                }
                for ((_package_id, _), ref mut request) in bump_candidates.iter_mut() {
-                       if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim(height, &request, fee_estimator, &&*logger) {
+                       if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim(
+                               height, &request, true /* force_feerate_bump */, fee_estimator, &&*logger
+                       ) {
                                request.set_timer(new_timer);
                                request.set_feerate(new_feerate);
                                match bump_claim {
index 786451ee50c834923f79e34bae478e7800f12c2b..5974a65d2a3e7527102c57cd8781a66d4a4b21e6 100644 (file)
@@ -762,16 +762,23 @@ impl PackageTemplate {
        /// Returns value in satoshis to be included as package outgoing output amount and feerate
        /// 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: Deref>(&self, predicted_weight: usize, dust_limit_sats: u64, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u64, u64)>
-               where F::Target: FeeEstimator,
-                     L::Target: Logger,
+       pub(crate) fn compute_package_output<F: Deref, L: Deref>(
+               &self, predicted_weight: usize, dust_limit_sats: u64, force_feerate_bump: bool,
+               fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
+       ) -> Option<(u64, u64)>
+       where
+               F::Target: FeeEstimator,
+               L::Target: Logger,
        {
                debug_assert!(self.malleability == PackageMalleability::Malleable, "The package output is fixed for non-malleable packages");
                let input_amounts = self.package_amount();
                assert!(dust_limit_sats as i64 > 0, "Output script must be broadcastable/have a 'real' dust limit.");
                // 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, fee_estimator, logger) {
+                       if let Some((new_fee, feerate)) = feerate_bump(
+                               predicted_weight, input_amounts, self.feerate_previous, force_feerate_bump,
+                               fee_estimator, logger,
+                       ) {
                                return Some((cmp::max(input_amounts as i64 - new_fee as i64, dust_limit_sats as i64) as u64, feerate));
                        }
                } else {
@@ -784,16 +791,19 @@ impl PackageTemplate {
 
        #[cfg(anchors)]
        /// Computes a feerate based on the given confirmation target. If a previous feerate was used,
-       /// and the new feerate is below it, we'll use a 25% increase of the previous feerate instead of
-       /// the new one.
+       /// 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.
        pub(crate) fn compute_package_feerate<F: Deref>(
                &self, fee_estimator: &LowerBoundedFeeEstimator<F>, conf_target: ConfirmationTarget,
+               force_feerate_bump: bool,
        ) -> u32 where F::Target: FeeEstimator {
                let feerate_estimate = fee_estimator.bounded_sat_per_1000_weight(conf_target);
                if self.feerate_previous != 0 {
                        // If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee...
                        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 {
                                // ...else just increase the previous feerate by 25% (because that's a nice number)
                                (self.feerate_previous + (self.feerate_previous / 4)).try_into().unwrap_or(u32::max_value())
@@ -945,32 +955,47 @@ fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(input_amounts: u64, predic
 
 /// 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. Otherwise, blindly bump the feerate by 25% of the previous feerate. 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: Deref>(predicted_weight: usize, input_amounts: u64, previous_feerate: u64, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u64, u64)>
-       where F::Target: FeeEstimator,
-             L::Target: Logger,
+/// 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.
+fn feerate_bump<F: Deref, L: Deref>(
+       predicted_weight: usize, input_amounts: u64, previous_feerate: u64, force_feerate_bump: bool,
+       fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
+) -> Option<(u64, u64)>
+where
+       F::Target: FeeEstimator,
+       L::Target: Logger,
 {
        // If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee...
-       let new_fee = if let Some((new_fee, _)) = compute_fee_from_spent_amounts(input_amounts, predicted_weight, fee_estimator, logger) {
-               let updated_feerate = new_fee / (predicted_weight as u64 * 1000);
-               if updated_feerate > previous_feerate {
-                       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 as u64) / 1000;
+                       (previous_fee, previous_feerate)
                } else {
                        // ...else just increase the previous feerate by 25% (because that's a nice number)
-                       let new_fee = previous_feerate * (predicted_weight as u64) / 750;
-                       if input_amounts <= new_fee {
+                       let bumped_feerate = previous_feerate + (previous_feerate / 4);
+                       let bumped_fee = bumped_feerate * (predicted_weight as u64) / 1000;
+                       if input_amounts <= bumped_fee {
                                log_warn!(logger, "Can't 25% bump new claiming tx, amount {} is too small", input_amounts);
                                return None;
                        }
-                       new_fee
+                       (bumped_fee, bumped_feerate)
                }
        } else {
                log_warn!(logger, "Can't new-estimation bump new claiming tx, amount {} is too small", input_amounts);
                return None;
        };
 
+       // Our feerates should never decrease. If it hasn't changed though, we just need to
+       // rebroadcast/re-sign the previous claim.
+       debug_assert!(new_feerate >= previous_feerate);
+       if new_feerate == previous_feerate {
+               return Some((new_fee, new_feerate));
+       }
+
        let previous_fee = previous_feerate * (predicted_weight as u64) / 1000;
        let min_relay_fee = MIN_RELAY_FEE_SAT_PER_1000_WEIGHT * (predicted_weight as u64) / 1000;
        // BIP 125 Opt-in Full Replace-by-Fee Signaling
index 401667f31f6dc80611fb3ef7e8422ff8f0994fbf..17f8e1597b7c0b2c34f064656c8ebf3de91e0a50 100644 (file)
@@ -2991,9 +2991,9 @@ fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
                if nodes[1].connect_style.borrow().skips_blocks() {
                        assert_eq!(txn.len(), 1);
                } else {
-                       assert_eq!(txn.len(), 2); // Extra rebroadcast of timeout transaction
+                       assert_eq!(txn.len(), 3); // Two extra fee bumps for timeout transaction
                }
-               check_spends!(txn[0], commitment_tx[0]);
+               txn.iter().for_each(|tx| check_spends!(tx, commitment_tx[0]));
                assert_eq!(txn[0].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
                txn.remove(0)
        };
@@ -7510,7 +7510,7 @@ fn test_bump_penalty_txn_on_remote_commitment() {
        assert_ne!(feerate_preimage, 0);
 
        // After exhaustion of height timer, new bumped claim txn should have been broadcast, check it
-       connect_blocks(&nodes[1], 15);
+       connect_blocks(&nodes[1], 1);
        {
                let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
                assert_eq!(node_txn.len(), 1);