From ddb54fc2d2f539502ab36357470f8a9a9d9555f1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 24 Dec 2023 05:17:29 +0000 Subject: [PATCH] Stop including dust values in feerate affordability checks When we or our counterparty are updating the fees on the channel, we currently check that the resulting balance is sufficient not only to meet the reserve threshold, but also not push it below dust. This isn't required in the BOLTs and may lead to spurious force-closures (which would be a bit safer, but reserve should always exceed the dust threshold). Worse, the current logic is broken - it compares the output value in *billionths of satoshis* to the dust limit in satoshis. Thus, the code is borderline dead anyway, but can overflow for channels with several million Bitcoin, causing the fuzzer to get mad (and lead to spurious force-closures for few-billion-dollar channels). --- lightning/src/ln/channel.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 588995656..6e283f086 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -732,8 +732,8 @@ struct CommitmentStats<'a> { total_fee_sat: u64, // the total fee included in the transaction num_nondust_htlcs: usize, // the number of HTLC outputs (dust HTLCs *non*-included) htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction - local_balance_msat: u64, // local balance before fees but considering dust limits - remote_balance_msat: u64, // remote balance before fees but considering dust limits + local_balance_msat: u64, // local balance before fees *not* considering dust limits + remote_balance_msat: u64, // remote balance before fees *not* considering dust limits outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment } @@ -1728,13 +1728,13 @@ impl ChannelContext where SP::Target: SignerProvider { } } - let mut value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset; + let value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset; assert!(value_to_self_msat >= 0); // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to // "violate" their reserve value by couting those against it. Thus, we have to convert // everything to i64 before subtracting as otherwise we can overflow. - let mut value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; + let value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; assert!(value_to_remote_msat >= 0); #[cfg(debug_assertions)] @@ -1800,10 +1800,6 @@ impl ChannelContext where SP::Target: SignerProvider { htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap()); htlcs_included.append(&mut included_dust_htlcs); - // For the stats, trimmed-to-0 the value in msats accordingly - value_to_self_msat = if (value_to_self_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_self_msat }; - value_to_remote_msat = if (value_to_remote_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_remote_msat }; - CommitmentStats { tx, feerate_per_kw, -- 2.39.5