WIP: Configure holder max htlc value in flight function of channel value #868
Conversation
As `get_holder_max_htlc_value_in_flight_msat` is a thents of `self.channel_value_satoshis`, there is no need to compute the minimum.
Codecov Report
@@ Coverage Diff @@
## main #868 +/- ##
==========================================
- Coverage 90.60% 90.60% -0.01%
==========================================
Files 51 51
Lines 27295 27298 +3
==========================================
+ Hits 24730 24732 +2
- Misses 2565 2566 +1
Continue to review full report at Codecov.
|
| fn get_holder_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 { | ||
| 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 |
There was a problem hiding this comment.
Should we bound by min(max(setting, 1), 90) (and update documentation to match)?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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) ?
| @@ -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 | |||
There was a problem hiding this comment.
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.
|
|
||
| Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis) | ||
| ); | ||
| self.get_holder_max_htlc_value_in_flight_msat() |
There was a problem hiding this comment.
Can we deprecate this method and instead make get_holder_max_htlc_value_in_flight_msat a pub fn ?
| @@ -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 { | |||
There was a problem hiding this comment.
Can you add a comment about this method "Returns a max in-flight inbound htlc value the remote can't overbid, required by us" ?
|
Are you still interested in working on this @jfroche |
|
Going ahead and closing this. If you are interested in continuing work on this, just leave and comment and we'll get it reopened! |
refs #850