Skip to content

WIP: Configure holder max htlc value in flight function of channel value #868

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

Closed
wants to merge 2 commits into from
Closed

WIP: Configure holder max htlc value in flight function of channel value #868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2873ca6
Nine tenths are always bigger than one thents
jfroche Apr 7, 2021
73089c5
Configure holder max htlc value in flight function of channel value
jfroche Apr 7, 2021
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
26 changes: 12 additions & 14 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ pub(super) struct Channel<Signer: Sign> {
pub(super) counterparty_max_htlc_value_in_flight_msat: u64,
#[cfg(not(test))]
counterparty_max_htlc_value_in_flight_msat: u64,
//get_holder_max_htlc_value_in_flight_msat(): u64,
holder_max_htlc_value_percentage: u64,
/// minimum channel reserve for self to maintain - set by them.
counterparty_selected_channel_reserve_satoshis: u64,
// get_holder_selected_channel_reserve_satoshis(channel_value_sats: u64): u64
Expand Down Expand Up @@ -483,8 +483,8 @@ macro_rules! secp_check {

impl<Signer: Sign> Channel<Signer> {
// Convert constants + channel value to limits:
fn get_holder_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 {
Copy link

Choose a reason for hiding this comment

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

Can you add a comment about this method "Returns a max in-flight inbound htlc value the remote can't overbid, required by us" ?

channel_value_satoshis * 1000 / 10 //TODO
fn get_holder_max_htlc_value_in_flight_msat(&self) -> u64 {
self.channel_value_satoshis * 1000 * self.holder_max_htlc_value_percentage / 100
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we bound by min(max(setting, 1), 90) (and update documentation to match)?

Copy link

@gotcha gotcha Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Make sense.

In #850 @TheBlueMatt mentioned sthing regarding an absolute value which we did not understand:

some kind of "min htlc_max_in_flight as a percentage of channel value" and "min htlc_max_in_filght absolute value" knobs

Do we need to dig further or can we forget it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its fine to skip it, I was just generally trying to say "maybe just a % isn't a sufficiently flexible knob, so we may need more options", but in hindsight, I'm not sure it matters, I think just the way it is is fine.

}

/// Returns a minimum channel reserve value the remote needs to maintain,
Expand Down Expand Up @@ -582,6 +582,7 @@ impl<Signer: Sign> Channel<Signer> {
counterparty_max_htlc_value_in_flight_msat: 0,
counterparty_selected_channel_reserve_satoshis: 0,
counterparty_htlc_minimum_msat: 0,
holder_max_htlc_value_percentage: config.own_channel_config.our_htlc_max_in_flight_percentage,
holder_htlc_minimum_msat: if config.own_channel_config.our_htlc_minimum_msat == 0 { 1 } else { config.own_channel_config.our_htlc_minimum_msat },
counterparty_max_accepted_htlcs: 0,
minimum_depth: 0, // Filled in in accept_channel
Expand Down Expand Up @@ -820,6 +821,7 @@ impl<Signer: Sign> Channel<Signer> {
counterparty_max_htlc_value_in_flight_msat: cmp::min(msg.max_htlc_value_in_flight_msat, msg.funding_satoshis * 1000),
counterparty_selected_channel_reserve_satoshis: msg.channel_reserve_satoshis,
counterparty_htlc_minimum_msat: msg.htlc_minimum_msat,
holder_max_htlc_value_percentage: config.own_channel_config.our_htlc_max_in_flight_percentage,
holder_htlc_minimum_msat: if config.own_channel_config.our_htlc_minimum_msat == 0 { 1 } else { config.own_channel_config.our_htlc_minimum_msat },
counterparty_max_accepted_htlcs: msg.max_accepted_htlcs,
minimum_depth: config.own_channel_config.minimum_depth,
Expand Down Expand Up @@ -1941,7 +1943,7 @@ impl<Signer: Sign> Channel<Signer> {
if inbound_htlc_count + 1 > OUR_MAX_HTLCS as u32 {
return Err(ChannelError::Close(format!("Remote tried to push more than our max accepted HTLCs ({})", OUR_MAX_HTLCS)));
}
let holder_max_htlc_value_in_flight_msat = Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis);
let holder_max_htlc_value_in_flight_msat = self.get_holder_max_htlc_value_in_flight_msat();
if htlc_inbound_value_msat + msg.amount_msat > holder_max_htlc_value_in_flight_msat {
return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", holder_max_htlc_value_in_flight_msat)));
}
Expand Down Expand Up @@ -3328,14 +3330,7 @@ impl<Signer: Sign> Channel<Signer> {

/// Allowed in any state (including after shutdown)
pub fn get_announced_htlc_max_msat(&self) -> u64 {
return cmp::min(
// Upper bound by capacity. We make it a bit less than full capacity to prevent attempts
// to use full capacity. This is an effort to reduce routing failures, because in many cases
// channel might have been used to route very small values (either by honest users or as DoS).
self.channel_value_satoshis * 1000 * 9 / 10,

Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis)
);
self.get_holder_max_htlc_value_in_flight_msat()
Copy link

Choose a reason for hiding this comment

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

Can we deprecate this method and instead make get_holder_max_htlc_value_in_flight_msat a pub fn ?

}

/// Allowed in any state (including after shutdown)
Expand Down Expand Up @@ -3699,7 +3694,7 @@ impl<Signer: Sign> Channel<Signer> {
funding_satoshis: self.channel_value_satoshis,
push_msat: self.channel_value_satoshis * 1000 - self.value_to_self_msat,
dust_limit_satoshis: self.holder_dust_limit_satoshis,
max_htlc_value_in_flight_msat: Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
max_htlc_value_in_flight_msat: self.get_holder_max_htlc_value_in_flight_msat(),
channel_reserve_satoshis: Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis),
htlc_minimum_msat: self.holder_htlc_minimum_msat,
feerate_per_kw: self.feerate_per_kw as u32,
Expand Down Expand Up @@ -3733,7 +3728,7 @@ impl<Signer: Sign> Channel<Signer> {
msgs::AcceptChannel {
temporary_channel_id: self.channel_id,
dust_limit_satoshis: self.holder_dust_limit_satoshis,
max_htlc_value_in_flight_msat: Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
max_htlc_value_in_flight_msat: self.get_holder_max_htlc_value_in_flight_msat(),
channel_reserve_satoshis: Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis),
htlc_minimum_msat: self.holder_htlc_minimum_msat,
minimum_depth: self.minimum_depth,
Expand Down Expand Up @@ -4502,6 +4497,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
self.counterparty_dust_limit_satoshis.write(writer)?;
self.holder_dust_limit_satoshis.write(writer)?;
self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;
self.holder_max_htlc_value_percentage.write(writer)?;
self.counterparty_selected_channel_reserve_satoshis.write(writer)?;
self.counterparty_htlc_minimum_msat.write(writer)?;
self.holder_htlc_minimum_msat.write(writer)?;
Expand Down Expand Up @@ -4672,6 +4668,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
let counterparty_dust_limit_satoshis = Readable::read(reader)?;
let holder_dust_limit_satoshis = Readable::read(reader)?;
let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
let holder_max_htlc_value_percentage = Readable::read(reader)?;
let counterparty_selected_channel_reserve_satoshis = Readable::read(reader)?;
let counterparty_htlc_minimum_msat = Readable::read(reader)?;
let holder_htlc_minimum_msat = Readable::read(reader)?;
Expand Down Expand Up @@ -4752,6 +4749,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
counterparty_dust_limit_satoshis,
holder_dust_limit_satoshis,
counterparty_max_htlc_value_in_flight_msat,
holder_max_htlc_value_percentage,
counterparty_selected_channel_reserve_satoshis,
counterparty_htlc_minimum_msat,
holder_htlc_minimum_msat,
Expand Down
3 changes: 3 additions & 0 deletions lightning/src/util/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ pub struct ChannelHandshakeConfig {
/// Default value: 1. If the value is less than 1, it is ignored and set to 1, as is required
/// by the protocol.
pub our_htlc_minimum_msat: u64,
/// Set to percentage of channel value used to control the channel maximum htlc value
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a bit more specific and should include some guidance on how to set it. Something like "The maximum amount we will accept for any single incoming HTLC on our channels, expressed as a percentage of the value of the channel. Payments to or routed through us above this value will be rejected before even reaching us. If this value is too large and we are a routing node, a malicious third-party could tie up our entire channel balance in a single HTLC which they refuse to resolve quickly." This should also mention the current default value.

Copy link

Choose a reason for hiding this comment

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

Note, I don't think this comment is accurate on the last part. IIRC, "htlc_max_value_in_flight_msat" is the global inbound HTLC balance, not a single HTLC. Setting a low-percentage doesn't prevent to tie up the entire channel balance in a single HTLC. Also I think it should say inbound liquidity instead of channel balance, our channel balance is the outbound liquidity ?

W.r.t to the default value, I would be more generous like 15% or 20%. Let say you want to preserve liquidity equilibrium in both direction. If you reach 35/75 in one direction, you trigger a rebalancing action like a circular loop to another one of your channel. A max_htlc_value of 15% means even at 35, you still have 20% of inbound liquidity buffer to keep the channel operational, assuming the in-flight 15% settled before the rebalance operation succeed.

Or what's your thinking on how a user should selection this value ? Otherwise, if you want to limit security risk, just open a smaller channel to avoid wasting almost-never used liquidity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops! You're totally right. As to guidance, that's a much tougher question if I read the variable name correctly :).

I'm not sure its right to focus on DoS here, its more about the our_to_self_delay vs cltv_expiry_delta distinction in my mind - in my mind, the biggest thing users should know is this is the max % of their channel who's security is controlled by cltv_expiry_delta, the rest being secured under the our_to_self_delay-block-limit. Its true re-balancing is a concern, but isn't it equally a concern with multiple payments? Maybe we should put rebalancing notes in htlc_maximum_msat (which I believe we currently don't expose, but should)?

Copy link

@ariard ariard Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

in my mind, the biggest thing users should know is this is the max % of their channel who's security is controlled by cltv_expiry_delta, the rest being secured under the our_to_self_delay-block-limit.

Yep, csv_delay vs cltv expiry delta sounds a good rational for guidance.

Its true re-balancing is a concern, but isn't it equally a concern with multiple payments?

I think unbalancing can be provoked as well due to multiple payments than a single one and "htlc_max_value_in_flight_msat" scope either but i would say such rebalancing concern should be noted there as we indirectly set hltc_maximum_msat in function of htlc_max_value_in_flight_msat (see get_announced_htlc_max_msat) ?

pub our_htlc_max_in_flight_percentage: u64,
}

impl Default for ChannelHandshakeConfig {
Expand All @@ -55,6 +57,7 @@ impl Default for ChannelHandshakeConfig {
minimum_depth: 6,
our_to_self_delay: BREAKDOWN_TIMEOUT,
our_htlc_minimum_msat: 1,
our_htlc_max_in_flight_percentage: 10,
}
}
}
Expand Down