From ceebf6256e339642c0380a6379ce0f2020d1a6cf Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 14 Sep 2023 17:38:22 -0700 Subject: [PATCH 1/3] Limit external claim feerate bumps Since we don't know the total input amount of an external claim (those which come anchor channels), we can't limit our feerate bumps by the amount of funds we have available to use. Instead, we choose to limit it by a margin of the new feerate estimate. --- lightning/src/chain/package.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index b66a2f70d33..382ffac3e8e 100644 --- a/lightning/src/chain/package.rs +++ b/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 From 38f18ceba6fa2e67222fc43e75d9bb3561bad109 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 18 Sep 2023 14:00:37 -0700 Subject: [PATCH 2/3] Account for existing input amounts throughout coin selection We'd previously ignore the existing amount transactions were already attempting to spend when deciding whether we should add more inputs throughout coin selection. This would result in us attaching more inputs than necessary to satisfy our target amount. In the case of HTLC transactions, we'd burn the HTLC amount completely, since the pre-signed transaction has zero fee (input amount == output amount). Along the way, we also fix the slight overpayment in anchor transactions. We now properly account for the fees the transaction already paid for, simply by pretending the fees are part of the anchor input amount. --- lightning/src/events/bump_transaction.rs | 34 ++++++++++-------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index 2d44275abb5..ec16a16af4d 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/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 { let mut locked_utxos = self.locked_utxos.lock().unwrap(); let mut eligible_utxos = utxos.iter().filter_map(|utxo| { @@ -569,7 +569,7 @@ where }).collect::>(); 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,20 @@ 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, }]; + + 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 { From 9736c709c0c7d02c27e4280e5f2dd4085bdf1169 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 18 Sep 2023 14:07:57 -0700 Subject: [PATCH 3/3] Sanity check fees on transactions produced by the bump event handler We add a few debug assertions to ensure we don't overpay fees by 5% more than expected. --- lightning/src/events/bump_transaction.rs | 44 ++++++++++++++++++++---- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index ec16a16af4d..35e9da60544 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -734,6 +734,8 @@ where 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::(); log_debug!(self.logger, "Peforming coin selection for commitment package (commitment and anchor transaction) targeting {} sat/kW", package_target_feerate_sat_per_1000_weight); @@ -747,10 +749,13 @@ where input: vec![anchor_descriptor.unsigned_tx_input()], output: vec![], }; + + #[cfg(debug_assertions)] + let total_satisfaction_weight = ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT + + coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::(); #[cfg(debug_assertions)] - let total_satisfaction_weight = - coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::() + - ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT; + let total_input_amount = must_spend_amount + + coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value).sum::(); self.process_coin_selection(&mut anchor_tx, coin_selection); let anchor_txid = anchor_tx.txid(); @@ -773,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::(); + // 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 {}", @@ -812,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::(); + #[cfg(debug_assertions)] + let must_spend_amount = must_spend.iter().map(|input| input.previous_utxo.value).sum::(); + 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 = must_spend_satisfaction_weight + + coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::(); #[cfg(debug_assertions)] - let total_satisfaction_weight = - coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::() + - must_spend_satisfaction_weight; + let total_input_amount = must_spend_amount + + coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value).sum::(); + self.process_coin_selection(&mut htlc_tx, coin_selection); #[cfg(debug_assertions)] @@ -846,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::(); + // 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));