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.

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

Mind commit tx fee while assembling a route #2080

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 35 additions & 3 deletions lightning/src/ln/channel.rs
Expand Up @@ -2612,17 +2612,49 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
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,
- 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() {
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's an outbound HTLC, why are we relying on the success dust limit? The timeout dust limit should apply to us, and the success dust limit to the counterparty.

}

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

We may also want to set fee_spike_buffer_htlc to Some since we can't actually send out an HTLC that would prevent us from sending another one later on.

let htlc_dust = HTLCCandidate::new(real_dust_limit_success_msat - 1000, HTLCInitiator::RemoteOffered);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be - 1 since we always round down?

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;
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 {
inbound_capacity_msat: cmp::max(self.channel_value_satoshis as i64 * 1000
- self.value_to_self_msat as i64
- self.get_inbound_pending_htlc_stats(None).pending_htlcs_value_msat as i64
- 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing, but this doesn't take into account the max number of HTLCs we can actually send out. In that case this should be 0.

self.counterparty_max_htlc_value_in_flight_msat as i64
- outbound_stats.pending_htlcs_value_msat as i64),
0) as u64,
Expand Down
13 changes: 13 additions & 0 deletions lightning/src/ln/functional_test_utils.rs
Expand Up @@ -1609,6 +1609,19 @@ macro_rules! get_route_and_payment_hash {
}}
}

#[cfg(test)]
#[macro_export]
macro_rules! get_route_or_error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no reason this shouldn't be a function rather than a macro.

($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 {
Expand Down
24 changes: 24 additions & 0 deletions lightning/src/ln/payment_tests.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -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 = 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) * 1000);
assert_eq!(route.paths.len(), 1);
// TODO: test other limits, e.g. that our router doesn't ignore counterparty_selected_channel_reserve_satoshis.
}