-
Notifications
You must be signed in to change notification settings - Fork 137
Drop max_inbound_htlc_value_in_flight_percent_of_channel
#878
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,9 +26,7 @@ use lightning::ln::channelmanager::{PaymentId, TrustedChannelFeatures}; | |
| use lightning::ln::types::ChannelId; | ||
| use lightning::routing::gossip::NodeId; | ||
| use lightning::sign::EntropySource; | ||
| use lightning::util::config::{ | ||
| ChannelConfigOverrides, ChannelConfigUpdate, ChannelHandshakeConfigUpdate, | ||
| }; | ||
| use lightning::util::config::{ChannelConfigOverrides, ChannelConfigUpdate}; | ||
| use lightning::util::errors::APIError; | ||
| use lightning::util::persist::KVStore; | ||
| use lightning::util::ser::{Readable, ReadableArgs, Writeable, Writer}; | ||
|
|
@@ -1273,20 +1271,24 @@ where | |
| if lsp_node_id == counterparty_node_id { | ||
| // When we're an LSPS2 client, allow claiming underpaying HTLCs as the LSP will skim off some fee. We'll | ||
| // check that they don't take too much before claiming. | ||
| // | ||
| // We also set maximum allowed inbound HTLC value in flight | ||
| // to 100%. We should eventually be able to set this on a per-channel basis, but for | ||
| // now we just bump the default for all channels. | ||
| channel_override_config = Some(ChannelConfigOverrides { | ||
| handshake_overrides: Some(ChannelHandshakeConfigUpdate { | ||
| max_inbound_htlc_value_in_flight_percent_of_channel: Some(100), | ||
| ..Default::default() | ||
| }), | ||
| update_overrides: Some(ChannelConfigUpdate { | ||
| accept_underpaying_htlcs: Some(true), | ||
| ..Default::default() | ||
| }), | ||
| ..Default::default() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: as elsewhere, maybe a quick |
||
| }); | ||
|
|
||
| // LSPS2 channels are unannounced; rely on LDK's default of 100% | ||
| // inbound HTLC value-in-flight so the LSP can forward the initial | ||
| // payment in full. | ||
| debug_assert_eq!( | ||
| self.channel_manager | ||
| .get_current_config() | ||
| .channel_handshake_config | ||
| .unannounced_channel_max_inbound_htlc_value_in_flight_percentage, | ||
| 100 | ||
| ); | ||
| } | ||
| } | ||
| let res = if allow_0conf { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1195,13 +1195,16 @@ impl Node { | |
| let mut user_config = default_user_config(&self.config); | ||
| user_config.channel_handshake_config.announce_for_forwarding = announce_for_forwarding; | ||
| user_config.channel_config = (channel_config.unwrap_or_default()).clone().into(); | ||
| // We set the max inflight to 100% for private channels. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: similar here, would it be worth adding a |
||
| // FIXME: LDK will default to this behavior soon, too, at which point we should drop this | ||
| // manual override. | ||
|
|
||
| // Unannounced channels rely on LDK's default of 100% inbound HTLC value-in-flight | ||
| // to support large initial payments via LSPS2. | ||
| if !announce_for_forwarding { | ||
| user_config | ||
| .channel_handshake_config | ||
| .max_inbound_htlc_value_in_flight_percent_of_channel = 100; | ||
| debug_assert_eq!( | ||
| user_config | ||
| .channel_handshake_config | ||
| .unannounced_channel_max_inbound_htlc_value_in_flight_percentage, | ||
| 100 | ||
| ); | ||
| } | ||
|
|
||
| let push_msat = push_to_counterparty_msat.unwrap_or(0); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -781,13 +781,16 @@ where | |
|
|
||
| let mut config = self.channel_manager.get_current_config().clone(); | ||
|
|
||
| // We set these LSP-specific values during Node building, here we're making sure it's actually set. | ||
| // If we act as an LSPS2 service, the HTLC-value-in-flight must be 100% of the | ||
| // channel value to ensure we can forward the initial payment. That cap only | ||
| // applies to unannounced channels, so the channel must also be unannounced. | ||
| debug_assert_eq!( | ||
| config | ||
| .channel_handshake_config | ||
| .max_inbound_htlc_value_in_flight_percent_of_channel, | ||
| .unannounced_channel_max_inbound_htlc_value_in_flight_percentage, | ||
| 100 | ||
| ); | ||
| debug_assert!(!config.channel_handshake_config.announce_for_forwarding); | ||
| debug_assert!(config.accept_forwards_to_priv_channels); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we add a This is to make sure that the 100% cap actually gets applied to the channel opened by the LSPS2 service. |
||
|
|
||
| // We set the forwarding fee to 0 for now as we're getting paid by the channel fee. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm here I don't think setting 100% here would have helped as that maximum only applies to the inbound HTLCs on that channel. To make sure that 100% of the value can be allocated to outbound HTLCs from the LSP's perspective as the comment describes, the user needs to set their
max_inboundto 100%.So seems to me the intention of the code did not match the comment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... yeah it does seem that way.