From 2c806f0e13d774ff18d5a181daf62980343a1bd6 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Tue, 7 Mar 2023 10:01:05 +0200 Subject: [PATCH 1/4] Mind commit tx fee while assembling a route Otherwise the resulting path might overshoot and thus fail at payment sending time. --- lightning/src/ln/channel.rs | 25 ++++++++++++++++++++--- lightning/src/ln/functional_test_utils.rs | 13 ++++++++++++ lightning/src/ln/payment_tests.rs | 24 ++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e3641a6204a..0fe9943e0ec 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2611,10 +2611,29 @@ impl Channel { } balance_msat -= outbound_stats.pending_htlcs_value_msat; - let outbound_capacity_msat = cmp::max(self.value_to_self_msat as i64 - - outbound_stats.pending_htlcs_value_msat as i64 - - self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000, + let mut outbound_capacity_msat = cmp::max(self.value_to_self_msat as i64 + - outbound_stats.pending_htlcs_value_msat as i64 + - self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000, 0) as u64; + if self.is_outbound() { + // Mind commit tx fee. + let mut real_dust_limit_success_sat = self.holder_dust_limit_satoshis; + if !self.opt_anchors() { + real_dust_limit_success_sat += self.feerate_per_kw as u64 * htlc_success_tx_weight(false) / 1000; + } + + let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_sat, HTLCInitiator::RemoteOffered); + let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_above_dust, None); + let htlc_dust = HTLCCandidate::new(real_dust_limit_success_sat - 1, HTLCInitiator::RemoteOffered); + let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_dust, None); + let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat; + + outbound_capacity_msat -= max_reserved_commit_tx_fee_msat; + if outbound_capacity_msat < real_dust_limit_success_sat { + outbound_capacity_msat += one_htlc_difference_msat; + outbound_capacity_msat = std::cmp::max(real_dust_limit_success_sat - 1, outbound_capacity_msat); + } + } AvailableBalances { inbound_capacity_msat: cmp::max(self.channel_value_satoshis as i64 * 1000 - self.value_to_self_msat as i64 diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index fd867bc6ab7..3668eaf4312 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1609,6 +1609,19 @@ macro_rules! get_route_and_payment_hash { }} } +#[cfg(test)] +#[macro_export] +macro_rules! get_route_or_error { + ($send_node: expr, $recv_node: expr, $recv_value: expr) => {{ + let payment_params = $crate::routing::router::PaymentParameters::from_node_id($recv_node.node.get_our_node_id(), TEST_FINAL_CLTV) + .with_features($recv_node.node.invoice_features()); + let (payment_preimage, payment_hash, payment_secret) = $crate::get_payment_preimage_hash!($recv_node, Some($recv_value)); + let route_or_error = $crate::get_route!($send_node, payment_params, $recv_value, TEST_FINAL_CLTV); + (route_or_error, payment_hash, payment_preimage, payment_secret) + }} +} + + #[macro_export] #[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))] macro_rules! expect_payment_claimable { diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 86648e5fb72..4dd556d2614 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -20,6 +20,7 @@ use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_T use crate::ln::features::InvoiceFeatures; use crate::ln::msgs; use crate::ln::msgs::ChannelMessageHandler; +use crate::ln::msgs::{LightningError, ErrorAction}; use crate::ln::outbound_payment::Retry; use crate::routing::gossip::{EffectiveCapacity, RoutingFees}; use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RouteParameters}; @@ -2752,3 +2753,26 @@ fn test_threaded_payment_retries() { } } } + +#[test] +fn test_route_minds_commit_tx_fee() { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let amount_sats = 100_000; + // Less than 10% should be on our side so that we don't hit the htlc limit of 10%. + let our_amount_sats = amount_sats * 9 / 100; + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, amount_sats, (amount_sats - our_amount_sats) * 1000); + + let extra_htlc_cost = 1365; + let route_err1 = get_route_or_error!(&nodes[0], nodes[1], (our_amount_sats - extra_htlc_cost) * 1000).0; + + if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = route_err1 { + assert_eq!(err, "Failed to find a sufficient route to the given destination"); + } else { panic!(); } + + let (route, _, _, _) = get_route_and_payment_hash!(&nodes[0], nodes[1], (our_amount_sats - extra_htlc_cost - 1) * 1000); + assert_eq!(route.paths.len(), 1); + // TODO: test other limits, e.g. that our router doesn't ignore counterparty_selected_channel_reserve_satoshis. +} From 6577686088acaf2e3af2073dac2e3fa3b0d38bed Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Thu, 9 Mar 2023 17:29:53 +0200 Subject: [PATCH 2/4] f add assert and make i work It's very unclear to me how it worked in the first place --- lightning/src/ln/channel.rs | 13 +++++++------ lightning/src/ln/payment_tests.rs | 6 +++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0fe9943e0ec..7ef05c4e12d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2617,21 +2617,22 @@ impl Channel { 0) as u64; if self.is_outbound() { // Mind commit tx fee. - let mut real_dust_limit_success_sat = self.holder_dust_limit_satoshis; + let mut real_dust_limit_success_msat = self.holder_dust_limit_satoshis * 1000; if !self.opt_anchors() { - real_dust_limit_success_sat += self.feerate_per_kw as u64 * htlc_success_tx_weight(false) / 1000; + real_dust_limit_success_msat += self.feerate_per_kw as u64 * htlc_success_tx_weight(false); } - let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_sat, HTLCInitiator::RemoteOffered); + let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_msat, HTLCInitiator::RemoteOffered); let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_above_dust, None); - let htlc_dust = HTLCCandidate::new(real_dust_limit_success_sat - 1, HTLCInitiator::RemoteOffered); + let htlc_dust = HTLCCandidate::new(real_dust_limit_success_msat - 1000, HTLCInitiator::RemoteOffered); let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_dust, None); let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat; + assert!(one_htlc_difference_msat != 0); outbound_capacity_msat -= max_reserved_commit_tx_fee_msat; - if outbound_capacity_msat < real_dust_limit_success_sat { + if outbound_capacity_msat < real_dust_limit_success_msat { outbound_capacity_msat += one_htlc_difference_msat; - outbound_capacity_msat = std::cmp::max(real_dust_limit_success_sat - 1, outbound_capacity_msat); + outbound_capacity_msat = std::cmp::max(real_dust_limit_success_msat - 1, outbound_capacity_msat); } } AvailableBalances { diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 4dd556d2614..61a04b9392b 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2765,14 +2765,14 @@ fn test_route_minds_commit_tx_fee() { let our_amount_sats = amount_sats * 9 / 100; create_announced_chan_between_nodes_with_value(&nodes, 0, 1, amount_sats, (amount_sats - our_amount_sats) * 1000); - let extra_htlc_cost = 1365; - let route_err1 = get_route_or_error!(&nodes[0], nodes[1], (our_amount_sats - extra_htlc_cost) * 1000).0; + let extra_htlc_cost = 1452; + let route_err1 = get_route_or_error!(&nodes[0], nodes[1], (our_amount_sats - extra_htlc_cost + 1) * 1000).0; if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = route_err1 { assert_eq!(err, "Failed to find a sufficient route to the given destination"); } else { panic!(); } - let (route, _, _, _) = get_route_and_payment_hash!(&nodes[0], nodes[1], (our_amount_sats - extra_htlc_cost - 1) * 1000); + let (route, _, _, _) = get_route_and_payment_hash!(&nodes[0], nodes[1], (our_amount_sats - extra_htlc_cost) * 1000); assert_eq!(route.paths.len(), 1); // TODO: test other limits, e.g. that our router doesn't ignore counterparty_selected_channel_reserve_satoshis. } From c1bca703335fcea90ba28a7d62ef260a69b17822 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Thu, 9 Mar 2023 17:35:54 +0200 Subject: [PATCH 3/4] f don't change outbound_capacity_msat Instead, only change next_outbound_htlc_limit_msat --- lightning/src/ln/channel.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 7ef05c4e12d..84e81ca4fbe 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2611,10 +2611,13 @@ impl Channel { } balance_msat -= outbound_stats.pending_htlcs_value_msat; - let mut outbound_capacity_msat = cmp::max(self.value_to_self_msat as i64 + let outbound_capacity_msat = cmp::max(self.value_to_self_msat as i64 - outbound_stats.pending_htlcs_value_msat as i64 - self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000, 0) as u64; + + let mut available_capacity_msat = outbound_capacity_msat; + if self.is_outbound() { // Mind commit tx fee. let mut real_dust_limit_success_msat = self.holder_dust_limit_satoshis * 1000; @@ -2626,13 +2629,13 @@ impl Channel { let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_above_dust, None); let htlc_dust = HTLCCandidate::new(real_dust_limit_success_msat - 1000, HTLCInitiator::RemoteOffered); let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_dust, None); - let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat; - assert!(one_htlc_difference_msat != 0); - outbound_capacity_msat -= max_reserved_commit_tx_fee_msat; - if outbound_capacity_msat < real_dust_limit_success_msat { - outbound_capacity_msat += one_htlc_difference_msat; - outbound_capacity_msat = std::cmp::max(real_dust_limit_success_msat - 1, outbound_capacity_msat); + available_capacity_msat -= max_reserved_commit_tx_fee_msat; + if available_capacity_msat < real_dust_limit_success_msat { + let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat; + assert!(one_htlc_difference_msat != 0); + available_capacity_msat += one_htlc_difference_msat; + available_capacity_msat = std::cmp::max(real_dust_limit_success_msat - 1, available_capacity_msat); } } AvailableBalances { @@ -2642,7 +2645,7 @@ impl Channel { - self.holder_selected_channel_reserve_satoshis as i64 * 1000, 0) as u64, outbound_capacity_msat, - next_outbound_htlc_limit_msat: cmp::max(cmp::min(outbound_capacity_msat as i64, + next_outbound_htlc_limit_msat: cmp::max(cmp::min(available_capacity_msat as i64, self.counterparty_max_htlc_value_in_flight_msat as i64 - outbound_stats.pending_htlcs_value_msat as i64), 0) as u64, From 862db1bb5d073ef4e902368de96ef585c11fd60b Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Thu, 9 Mar 2023 18:35:48 +0200 Subject: [PATCH 4/4] f add comments --- lightning/src/ln/channel.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 84e81ca4fbe..99967e2299d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2619,7 +2619,13 @@ impl Channel { let mut available_capacity_msat = outbound_capacity_msat; if self.is_outbound() { - // Mind commit tx fee. + // We should mind channel commit tx fee when computing how much of the available capacity + // can be used in the next htlc. Mirrors the logic in send_htlc. + // + // The fee depends on whether the amount we will be sending is above dust or not, + // and the answer will in turn change the amount itself — making it a circular + // dependency. + // This complicates the computation around dust-values, up to the one-htlc-value. let mut real_dust_limit_success_msat = self.holder_dust_limit_satoshis * 1000; if !self.opt_anchors() { real_dust_limit_success_msat += self.feerate_per_kw as u64 * htlc_success_tx_weight(false); @@ -2630,6 +2636,9 @@ impl Channel { let htlc_dust = HTLCCandidate::new(real_dust_limit_success_msat - 1000, HTLCInitiator::RemoteOffered); let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_dust, None); + // We will first subtract the fee as if we were above-dust. Then, if the resulting + // value ends up being below dust, we have this fee available again. In that case, + // match the value to right-below-dust. available_capacity_msat -= max_reserved_commit_tx_fee_msat; if available_capacity_msat < real_dust_limit_success_msat { let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat;