Extend OnchainTxHandler::generate_claim to not manually bump feerates

In the next commit, we plan to extend the `OnchainTxHandler` to retry
pending claims on every timer tick. These timer ticks 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.

Author: wpaulino

lightning/src/chain/onchaintx.rs

@@ -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, manually_bump_feerate: 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, manually_bump_feerate
+					);
 					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(),
+				manually_bump_feerate, 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, manually_bump_feerate);
 							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 /* manually_bump_feerate */, &*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 /* manually_bump_feerate */, &*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 /* manually_bump_feerate */, fee_estimator, &&*logger
+			) {
 				request.set_timer(new_timer);
 				request.set_feerate(new_feerate);
 				match bump_claim {

lightning/src/chain/package.rs

@@ -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, manually_bump_feerate: 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, manually_bump_feerate,
+				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 `manually_bump_feerate` 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,
+		manually_bump_feerate: 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 !manually_bump_feerate {
+				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())
@@ -948,29 +958,43 @@ fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(input_amounts: u64, predic
 /// 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,
+fn feerate_bump<F: Deref, L: Deref>(
+	predicted_weight: usize, input_amounts: u64, previous_feerate: u64, manually_bump_feerate: 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 !manually_bump_feerate {
+			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

lightning/src/ln/functional_tests.rs

@@ -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);