Merge pull request lightningdevkit#2587 from wpaulino/anchors-fee-fixes

Anchor outputs fee fixes

Author: TheBlueMatt

lightning/src/chain/package.rs

@@ -976,14 +976,23 @@ impl PackageTemplate {
 	) -> 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...
+			// 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 {
-				// ...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())
+				// 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

lightning/src/events/bump_transaction.rs

@@ -14,7 +14,7 @@
 use alloc::collections::BTreeMap;
 use core::ops::Deref;
 
-use crate::chain::chaininterface::{BroadcasterInterface, compute_feerate_sat_per_1000_weight, fee_for_weight, FEERATE_FLOOR_SATS_PER_KW};
+use crate::chain::chaininterface::{BroadcasterInterface, fee_for_weight};
 use crate::chain::ClaimId;
 use crate::io_extras::sink;
 use crate::ln::channel::ANCHOR_OUTPUT_VALUE_SATOSHI;
@@ -542,7 +542,7 @@ where
 	fn select_confirmed_utxos_internal(
 		&self, utxos: &[Utxo], claim_id: ClaimId, force_conflicting_utxo_spend: bool,
 		tolerate_high_network_feerates: bool, target_feerate_sat_per_1000_weight: u32,
-		preexisting_tx_weight: u64, target_amount_sat: u64,
+		preexisting_tx_weight: u64, input_amount_sat: u64, target_amount_sat: u64,
 	) -> Result<CoinSelection, ()> {
 		let mut locked_utxos = self.locked_utxos.lock().unwrap();
 		let mut eligible_utxos = utxos.iter().filter_map(|utxo| {
@@ -569,7 +569,7 @@ where
 		}).collect::<Vec<_>>();
 		eligible_utxos.sort_unstable_by_key(|(utxo, _)| utxo.output.value);
 
-		let mut selected_amount = 0;
+		let mut selected_amount = input_amount_sat;
 		let mut total_fees = fee_for_weight(target_feerate_sat_per_1000_weight, preexisting_tx_weight);
 		let mut selected_utxos = Vec::new();
 		for (utxo, fee_to_spend_utxo) in eligible_utxos {
@@ -632,13 +632,14 @@ where
 
 		let preexisting_tx_weight = 2 /* segwit marker & flag */ + total_input_weight +
 			((BASE_TX_SIZE + total_output_size) * WITNESS_SCALE_FACTOR as u64);
+		let input_amount_sat: u64 = must_spend.iter().map(|input| input.previous_utxo.value).sum();
 		let target_amount_sat = must_pay_to.iter().map(|output| output.value).sum();
 		let do_coin_selection = |force_conflicting_utxo_spend: bool, tolerate_high_network_feerates: bool| {
 			log_debug!(self.logger, "Attempting coin selection targeting {} sat/kW (force_conflicting_utxo_spend = {}, tolerate_high_network_feerates = {})",
 				target_feerate_sat_per_1000_weight, force_conflicting_utxo_spend, tolerate_high_network_feerates);
 			self.select_confirmed_utxos_internal(
 				&utxos, claim_id, force_conflicting_utxo_spend, tolerate_high_network_feerates,
-				target_feerate_sat_per_1000_weight, preexisting_tx_weight, target_amount_sat,
+				target_feerate_sat_per_1000_weight, preexisting_tx_weight, input_amount_sat, target_amount_sat,
 			)
 		};
 		do_coin_selection(false, false)
@@ -724,27 +725,22 @@ where
 		commitment_tx: &Transaction, commitment_tx_fee_sat: u64, anchor_descriptor: &AnchorDescriptor,
 	) -> Result<(), ()> {
 		// Our commitment transaction already has fees allocated to it, so we should take them into
-		// account. We compute its feerate and subtract it from the package target, using the result
-		// as the target feerate for our anchor transaction. Unfortunately, this results in users
-		// overpaying by a small margin since we don't yet know the anchor transaction size, and
-		// avoiding the small overpayment only makes our API even more complex.
-		let commitment_tx_sat_per_1000_weight: u32 = compute_feerate_sat_per_1000_weight(
-			commitment_tx_fee_sat, commitment_tx.weight() as u64,
-		);
-		let anchor_target_feerate_sat_per_1000_weight = core::cmp::max(
-			package_target_feerate_sat_per_1000_weight - commitment_tx_sat_per_1000_weight,
-			FEERATE_FLOOR_SATS_PER_KW,
-		);
-
-		log_debug!(self.logger, "Peforming coin selection for anchor transaction targeting {} sat/kW",
-			anchor_target_feerate_sat_per_1000_weight);
+		// account. We do so by pretending the commitment tranasction's fee and weight are part of
+		// the anchor input.
+		let mut anchor_utxo = anchor_descriptor.previous_utxo();
+		anchor_utxo.value += commitment_tx_fee_sat;
 		let must_spend = vec![Input {
 			outpoint: anchor_descriptor.outpoint,
-			previous_utxo: anchor_descriptor.previous_utxo(),
+			previous_utxo: anchor_utxo,
 			satisfaction_weight: commitment_tx.weight() as u64 + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT,
 		}];
+		#[cfg(debug_assertions)]
+		let must_spend_amount =	must_spend.iter().map(|input| input.previous_utxo.value).sum::<u64>();
+
+		log_debug!(self.logger, "Peforming coin selection for commitment package (commitment and anchor transaction) targeting {} sat/kW",
+			package_target_feerate_sat_per_1000_weight);
 		let coin_selection = self.utxo_source.select_confirmed_utxos(
-			claim_id, must_spend, &[], anchor_target_feerate_sat_per_1000_weight,
+			claim_id, must_spend, &[], package_target_feerate_sat_per_1000_weight,
 		)?;
 
 		let mut anchor_tx = Transaction {
@@ -753,10 +749,13 @@ where
 			input: vec![anchor_descriptor.unsigned_tx_input()],
 			output: vec![],
 		};
+
 		#[cfg(debug_assertions)]
-		let total_satisfaction_weight =
-			coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::<u64>() +
-				ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT;
+		let total_satisfaction_weight = ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT +
+			coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::<u64>();
+		#[cfg(debug_assertions)]
+		let total_input_amount = must_spend_amount +
+			coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value).sum::<u64>();
 
 		self.process_coin_selection(&mut anchor_tx, coin_selection);
 		let anchor_txid = anchor_tx.txid();
@@ -779,6 +778,16 @@ where
 			// never underestimate.
 			assert!(expected_signed_tx_weight >= signed_tx_weight &&
 				expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight);
+
+			let expected_package_fee = fee_for_weight(package_target_feerate_sat_per_1000_weight,
+				signed_tx_weight + commitment_tx.weight() as u64);
+			let package_fee = total_input_amount -
+				anchor_tx.output.iter().map(|output| output.value).sum::<u64>();
+			// Our fee should be within a 5% error margin of the expected fee based on the
+			// feerate and transaction weight and we should never pay less than required.
+			let fee_error_margin = expected_package_fee * 5 / 100;
+			assert!(package_fee >= expected_package_fee &&
+				package_fee - fee_error_margin <= expected_package_fee);
 		}
 
 		log_info!(self.logger, "Broadcasting anchor transaction {} to bump channel close with txid {}",
@@ -818,16 +827,24 @@ where
 
 		log_debug!(self.logger, "Peforming coin selection for HTLC transaction targeting {} sat/kW",
 			target_feerate_sat_per_1000_weight);
+
 		#[cfg(debug_assertions)]
 		let must_spend_satisfaction_weight =
 			must_spend.iter().map(|input| input.satisfaction_weight).sum::<u64>();
+		#[cfg(debug_assertions)]
+		let must_spend_amount =	must_spend.iter().map(|input| input.previous_utxo.value).sum::<u64>();
+
 		let coin_selection = self.utxo_source.select_confirmed_utxos(
 			claim_id, must_spend, &htlc_tx.output, target_feerate_sat_per_1000_weight,
 		)?;
+
 		#[cfg(debug_assertions)]
-		let total_satisfaction_weight =
-			coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::<u64>() +
-				must_spend_satisfaction_weight;
+		let total_satisfaction_weight = must_spend_satisfaction_weight +
+			coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::<u64>();
+		#[cfg(debug_assertions)]
+		let total_input_amount = must_spend_amount +
+			coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value).sum::<u64>();
+
 		self.process_coin_selection(&mut htlc_tx, coin_selection);
 
 		#[cfg(debug_assertions)]
@@ -852,6 +869,15 @@ where
 			// never underestimate.
 			assert!(expected_signed_tx_weight >= signed_tx_weight &&
 				expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight);
+
+			let expected_signed_tx_fee = fee_for_weight(target_feerate_sat_per_1000_weight, signed_tx_weight);
+			let signed_tx_fee = total_input_amount -
+				htlc_tx.output.iter().map(|output| output.value).sum::<u64>();
+			// Our fee should be within a 5% error margin of the expected fee based on the
+			// feerate and transaction weight and we should never pay less than required.
+			let fee_error_margin = expected_signed_tx_fee * 5 / 100;
+			assert!(signed_tx_fee >= expected_signed_tx_fee &&
+				signed_tx_fee - fee_error_margin <= expected_signed_tx_fee);
 		}
 
 		log_info!(self.logger, "Broadcasting {}", log_tx!(htlc_tx));