Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anchor outputs fee fixes #2587

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Anchor outputs fee fixes #2587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ceebf62
Limit external claim feerate bumps
wpaulino Sep 15, 2023
38f18ce
Account for existing input amounts throughout coin selection
wpaulino Sep 18, 2023
9736c70
Sanity check fees on transactions produced by the bump event handler
wpaulino Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions lightning/src/chain/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, the new code now upper bounds our effective new_feeratee to a multiplicand of feerate_estimate. If FeeEstimator is broken or our underlying inbound transaction-relay has been partitioned from the rest of the network and the sat_per_1000_weight we got is 1 sat-per-byte, we might never confirm our transaction ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. There's not really anything else we can do - I hit a case where a commitment transaction didn't get into the mempool because of its low feerate, and the package logic here ended up reaching a feerate of u32::max_value(), which would have spent all available funds on one transaction the second the commitment transaction got into the mempool. Even after package relay a similar issue exists if the user is offline for some time. Trusting the fee estimator sucks, indeed, but spending all available user funds on a single transaction sucks more. We could bump the minimum here to add a constant and ignore the fee estimator if the constant is > feerate_estimate * 5, but that's really the best we can do.

new_feerate = cmp::max(feerate_estimate * 5, previous_feerate);
}
new_feerate
}
} else {
feerate_estimate
Expand Down
78 changes: 52 additions & 26 deletions lightning/src/events/bump_transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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| {
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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();
Expand All @@ -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 {}",
Expand Down Expand Up @@ -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)]
Expand All @@ -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));
Expand Down