Skip to content

Mind commit tx fee while assembling a route #2080

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

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
25 changes: 22 additions & 3 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2611,10 +2611,29 @@ 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,
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add some comments or maybe DRY this up with some of the existing tx fee construction logic? Its kinda hard to read what's going on here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it would be nice to re-use some of the send_htlc logic. Ultimately, I think it'd be useful to refactor Channel such that we have one method that can provide any type of balance we may need across LDK.

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_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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, if I add an assert!(one_htlc_difference_msat == 0); it seems to always be true.


outbound_capacity_msat -= max_reserved_commit_tx_fee_msat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to as aggressively change outbound_capacity_msat - the way I read the docs its really "what's the maximum amount you could send, possibly across multiple HTLCs, and in the future, over this channel". Thus, we don't need to consider the full current commitment tx fee, but rather maybe only one htlc's worth of commitment tx fee.

next_outbound_htlc_limit_msat is differentl, there we should be as accurate about the next htlc as possible.

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
Expand Down
13 changes: 13 additions & 0 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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 = 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.
}