Set `ChannelUpdate` `htlc_maximum_msat` using the peer's value #1444

ViktorTigerstrom · 2022-04-21T22:33:47Z

Use the counterparty_max_htlc_value_in_flight_msat value, and not the
holder_max_htlc_value_in_flight_msat value when creating the
htlc_maximum_msat value for ChannelUpdate messages.

BOLT 7 specifies that the field MUST be less than or equal to
max_htlc_value_in_flight_msat received from the peer, which we
currently are not guaranteed to adhere to by using the holder (our) value.

https://github.com/lightning/bolts/blob/cf4fddd99eae9f15a527dfb785b47cd3c625c0fd/07-routing-gossip.md?plain=1#L469

TheBlueMatt

LG, needs a test, may be easiest to make the in-flight limit % a config option while you're at it.

codecov-commenter · 2022-04-21T22:59:53Z

Codecov Report

Merging #1444 (4cc1fc2) into main (dc8479a) will increase coverage by 0.01%.
The diff coverage is 98.85%.

❗ Current head 4cc1fc2 differs from pull request most recent head 224d470. Consider uploading reports for the commit 224d470 to get more accurate results

@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
+ Coverage   90.88%   90.89%   +0.01%     
==========================================
  Files          75       75              
  Lines       41547    41704     +157     
  Branches    41547    41704     +157     
==========================================
+ Hits        37760    37907     +147     
- Misses       3787     3797      +10

Impacted Files	Coverage Δ
lightning/src/util/config.rs	`44.00% <0.00%> (-0.90%)`	⬇️
lightning/src/ln/channel.rs	`88.55% <100.00%> (+0.16%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.09% <100.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.70% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`95.52% <0.00%> (-0.04%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc8479a...224d470. Read the comment docs.

ViktorTigerstrom · 2022-04-21T23:04:17Z

LG, needs a test, may be easiest to make the in-flight limit % a config option while you're at it.

Sure sounds good, I'll fix that! Won't have time to address that until this weekend though unfortunately, I hope that's ok :)

vincenzopalazzo

LGTM

ViktorTigerstrom

Sorry for the delay in continuing this PR! Added a few comments which may be helpful when reviewing it.

If you think that it's more suitable that I create a new PR with the 2 first commits, I can of course do that!

ViktorTigerstrom · 2022-04-26T22:56:03Z

lightning/src/util/config.rs

+	/// Should be set to a value between 1-100, where the value signals the percantage to be used.
+	///
+	/// Default value: 10.
+	pub holder_max_htlc_value_in_flight_msat_channel_value_percent: u8,


Wasn't sure if we want this as an integer value or not, to only allow the user to set whole percentage values. So let me know if I should change this!

Also, tried to come up with a shorter name than holder_max_htlc_value_in_flight_msat_channel_value_percent, but all the alternatives I came up with ended up loosing some meaning. Please suggest a better name if you can think of any :)!

Maybe max_inbound_htlc_value_in_flight_percent_of_channel - I don't like the holder terminology so much in public things like this if we can avoid it "inbound_htlc" seems more clear because, well, its an inbound HTLC. That said, I don't really care if the name is long, as long as its descriptive.

Also this should be in ChannelHandshakeConfig.

Ok! Given that we don't serialize the ChannelHandshakeConfig in the Channel struct, should I implement the channel serialization as follows:

If the max_inbound_htlc_value_in_flight_percent_of_channel has been modified to any other value than the default, we write the holder_max_htlc_value_in_flight_msat into the tlv fields (as it's been configured), despite the implications that will mean for backwards compatibility?

ViktorTigerstrom · 2022-04-26T23:01:49Z

lightning/src/ln/channel.rs

 		let serialized_holder_selected_reserve =
 			if self.holder_selected_channel_reserve_satoshis != Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis)
 			{ Some(self.holder_selected_channel_reserve_satoshis) } else { None };
 		let serialized_holder_htlc_max_in_flight =
-			if self.holder_max_htlc_value_in_flight_msat != Self::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis)
+			if self.holder_max_htlc_value_in_flight_msat != Self::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis, self.config)


Note:
For this to work, this assumes that the channel config will remain not being possible to update for channels which are already created. If we in the future make the channel config updatable for live channels, this will start setting a Some value for serialized_holder_htlc_max_in_flight for channels where the holder_max_htlc_value_in_flight_msat_channel_value_percent has been changed, which will break backwards compatibility for those channels. Though, given that the actual holder_max_htlc_value_in_flight_msat shouldn't be updatable after it's been set, it makes no sense that the config percentage should be either.

If this is a problem, please let me know :)

TheBlueMatt

Nice! Thanks for tackling.

TheBlueMatt · 2022-04-27T14:17:25Z

lightning/src/util/config.rs

+	/// Should be set to a value between 1-100, where the value signals the percantage to be used.
+	///
+	/// Default value: 10.
+	pub holder_max_htlc_value_in_flight_msat_channel_value_percent: u8,


Maybe max_inbound_htlc_value_in_flight_percent_of_channel - I don't like the holder terminology so much in public things like this if we can avoid it "inbound_htlc" seems more clear because, well, its an inbound HTLC. That said, I don't really care if the name is long, as long as its descriptive.

TheBlueMatt · 2022-04-27T14:18:55Z

lightning/src/util/config.rs

+	/// Sets how many the percent of the channel value the holder_max_htlc_value_in_flight_msat
+	/// will be assigned as.
+	///
+	/// Should be set to a value between 1-100, where the value signals the percantage to be used.


What happens if the value is not within that range? Do we panic? Do we simply bound it to that range, etc.

TheBlueMatt · 2022-04-27T14:19:16Z

lightning/src/util/config.rs

+	/// Should be set to a value between 1-100, where the value signals the percantage to be used.
+	///
+	/// Default value: 10.
+	pub holder_max_htlc_value_in_flight_msat_channel_value_percent: u8,


Also this should be in ChannelHandshakeConfig.

TheBlueMatt · 2022-04-27T14:36:19Z

lightning/src/util/config.rs

@@ -269,6 +269,13 @@ pub struct ChannelConfig {
 	/// [`Normal`]: crate::chain::chaininterface::ConfirmationTarget::Normal
 	/// [`Background`]: crate::chain::chaininterface::ConfirmationTarget::Background
 	pub force_close_avoidance_max_fee_satoshis: u64,
+	/// Sets how many the percent of the channel value the holder_max_htlc_value_in_flight_msat


Need way more docs here - without knowing anything about the lightning protocol, a user reading this documentation should be able to identify what the imlications of setting this value higher or lower will be.

It should probably link to ChannelConfig::cltv_expiry_delta and ChannelHandshakeConfig::our_to_self_delay, noting that this limits the HTLC exposure for inbound HTLCs only, It may also note that there is currently no ability to limit the outbound HTLC exposure.

Ok thanks, will update!

TheBlueMatt · 2022-04-27T14:37:43Z

lightning/src/ln/channel.rs

@@ -865,6 +870,13 @@ impl<Signer: Sign> Channel<Signer> {
 			return Err(APIError::APIMisuseError { err: format!("Holder selected channel  reserve below implemention limit dust_limit_satoshis {}", holder_selected_channel_reserve_satoshis) });
 		}

+		// Check that config has valid values set
+		if !Self::config_htlc_in_flight_value_is_valid(config.channel_options){


Lets just bound the value instead of failing here, no?

Do you mean that if the user sets a value that's < 1, we should set the holder_max_htlc_value_in_flight_msat using 1% when creating the channel (and 100% if > 100)?

Or do you mean that we should strictly bind the ChannelHandshakeConfig to only allow a max_inbound_htlc_value_in_flight_percent_of_channel value between 1-100?

A strict binding would be a much nicer solution if there is an easy way to do that in Rust, but as I'm still quite new to Rust, I'm not aware an easy solution/type for that, and my searches yielded no great results. I was also a bit afraid if such a solution would create any implications for the language bindings, which is why I opted for dealing with it in the Channel struct instead. If you have any tips of how to solve this though, I'd be very thankful for those!

Do you mean that if the user sets a value that's < 1, we should set the holder_max_htlc_value_in_flight_msat using 1% when creating the channel (and 100% if > 100)?

This. I think the "most" rust-y way to do this would be to check the value when constructing the ChannelHandshakeConfig, but I dunno that we really want to go that far here - there's value, at least in the bindings, to just having a POD struct here. The docs should be really clear that it needs to be between 1-100, but if users set it to something crazy we can just bound it for them and use 1 or 100.

ViktorTigerstrom · 2022-04-28T21:36:06Z

Addressed the latest feedback! I wasn't completely sure what you wanted me to mention about ChannelConfig::cltv_expiry_delta and ChannelHandshakeConfig::our_to_self_delay, so if I understood incorrectly, please let me know :)!

Also, let me know if I should rebase on upstream as there is now a small conflict.

Additionally, let me know if I should modify this test function to use the new config option instead of directly modifying the Channel::holder_max_htlc_value_in_flight_msat value :) :

rust-lightning/lightning/src/ln/functional_tests.rs

Line 138 in f53d13b

fn do_test_counterparty_no_reserve(send_from_initiator: bool) {

TheBlueMatt

Code looks good, some comments on making sure our docs are super clear.

TheBlueMatt · 2022-04-29T18:49:27Z

lightning/src/ln/channel.rs

 		let serialized_holder_selected_reserve =
 			if self.holder_selected_channel_reserve_satoshis != Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis)
 			{ Some(self.holder_selected_channel_reserve_satoshis) } else { None };
 		let serialized_holder_htlc_max_in_flight =
-			if self.holder_max_htlc_value_in_flight_msat != Self::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis)
+			if self.holder_max_htlc_value_in_flight_msat != Self::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis, &UserConfig::default().own_channel_config)


Instead of making this code depend on us not changing the default in UserConfig lets just make it an explicit 10% or maybe make a constant to define OLD_LDK_MAX_IN_FLIGHT_PERCENT or something?

TheBlueMatt · 2022-04-29T18:51:24Z

lightning/src/util/config.rs

@@ -47,6 +47,28 @@ 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,
+	/// Sets how many the percent of the channel value we will cap the total value of outstanding


nit: s/how many the percent/the percentage/.

TheBlueMatt · 2022-04-29T18:52:14Z

lightning/src/util/config.rs

+	/// channel value in whole percentages.
+	///
+	/// Note that if configured to another value than the default value 10, any new channels
+	/// created with the non default value won't be backwards compatible with LDK versions prior


This should describe what will happen, not just that its "not backwards compatible". ie "will cause versions of LDK prior to 0.0.100 to refuse to read the ChannelManager" or so.

TheBlueMatt · 2022-04-29T18:53:27Z

lightning/src/util/config.rs

+	/// to 0.0.100.
+	///
+	/// Note that this caps the total value for inbound HTLCs in-flight only, and there's currently
+	/// no availability to configure the cap for the total value of outbound HTLCs in-flight.


nit: s/availability/way/

TheBlueMatt · 2022-04-29T18:59:18Z

lightning/src/util/config.rs

+	///
+	/// Note that this caps the total value for inbound HTLCs in-flight only, and there's currently
+	/// no availability to configure the cap for the total value of outbound HTLCs in-flight.
+	/// This effects the in-flight HTLC balance, which [`ChannelConfig::cltv_expiry_delta`] is


This and the next sentence are pretty confusing IMO. Maybe something like "Note that the online requirements for ensuring the safety of HTLC-encumbered funds are different from the non-HTLC-encumbered funds. This makes this an important knob to restrict exposure to loss due to being offline for too long. See our_to_self_delay and cltv_expiry_delta for more info".

TheBlueMatt · 2022-04-29T18:59:59Z

Yes, please rebase to resolve the conflict!

ViktorTigerstrom · 2022-04-29T20:15:18Z

Thanks a lot for the detailed feedback on the documentation @TheBlueMatt! Updated the docs according to the feedback. There were now 3 "Note that" statements in a row, so made it into a list instead to make the text flow better. In case you don't like that idea I'll of course change it back :).

Squashed the previous commits, and pushed the new changes in a seperate fixup commit!

TheBlueMatt · 2022-04-29T20:22:21Z

Cool, lets get a second reviewer on this before we move much more forward.

valentinewallace

Still going through the documentation, but the tests and code look good!

valentinewallace · 2022-05-02T18:13:13Z

lightning/src/ln/channel.rs

@@ -745,6 +745,10 @@ pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;

 pub const ANCHOR_OUTPUT_VALUE_SATOSHI: u64 = 330;

+/// The percentage of the channel value `holder_max_htlc_value_in_flight_msat` used to be set to,
+/// before this was made configurable.
+pub const OLD_LDK_MAX_IN_FLIGHT_PERCENT: u8 = 10;


I suggest MAX_IN_FLIGHT_PERCENT_LEGACY. Could we mention the release that this was deprecated from, as well?

valentinewallace · 2022-05-02T19:27:43Z

lightning/src/ln/channel.rs

+	/// `channel_value_satoshis` in msat, based on how many of the percent its been configured to
+	/// be set to by the user.


s/based on how many of the percent its been configured to be set to by the user/[ChannelHandkshakeConfig::max_inbound_htlc_value_in_flight_percent_of_channel] (double check that link lol)

Thanks, edited it to:
set through [ChannelHandkshakeConfig::max_inbound_htlc_value_in_flight_percent_of_channel]
But can remove the "set through" if you prefer :)!

lightning/src/ln/channel.rs

ViktorTigerstrom · 2022-05-02T22:26:36Z

Thanks a lot for the review @valentinewallace! Addressed your suggestion in the latest commit!

ViktorTigerstrom · 2022-05-02T22:33:18Z

lightning/src/util/config.rs

+	///
+	/// Note that:
+	/// * If configured to another value than the default value 10, any new channels
+	/// created with the non default value will cause versions of LDK prior to 0.0.100 to refuse to


Hmm could you please check the part about "LDK prior to 0.0.100" here? The holder_max_htlc_value_in_flight_msat was added to the TLV fields in LDK 0.0.104 ( f69311c ).

As I'm still a bit uncertain about our serialization:
As the field was added with an even number (6), does that mean that LDK 0.0.103 and prior will also fail when deserializing the Channel (hence this should be changed)?

Ah okay, I think since it was added in 0.0.104, I think this should be s/100/104?

As the field was added with an even number (6), does that mean that LDK 0.0.103 and prior will also fail when deserializing the Channel (hence this should be changed)?

That's my understanding, "even number == you must understand this type or fail to deserialize"

Thanks for clarifying :)

ViktorTigerstrom · 2022-05-02T22:41:15Z

Sorry, fixed a small spelling mistake in the latest commit.

valentinewallace

Think I'm ACK after these last minor comments

valentinewallace · 2022-05-03T15:41:37Z

lightning/src/util/config.rs

+	///
+	/// Note that:
+	/// * If configured to another value than the default value 10, any new channels
+	/// created with the non default value will cause versions of LDK prior to 0.0.100 to refuse to


Ah okay, I think since it was added in 0.0.104, I think this should be s/100/104?

As the field was added with an even number (6), does that mean that LDK 0.0.103 and prior will also fail when deserializing the Channel (hence this should be changed)?

That's my understanding, "even number == you must understand this type or fail to deserialize"

valentinewallace · 2022-05-03T15:59:24Z

lightning/src/util/config.rs

+	/// Note that:
+	/// * If configured to another value than the default value 10, any new channels
+	/// created with the non default value will cause versions of LDK prior to 0.0.100 to refuse to
+	/// read the ChannelManager.


nit: s/ChannelManager/ChannelManager

valentinewallace · 2022-05-03T16:03:08Z

lightning/src/util/config.rs

+	/// * The online requirements for ensuring the safety of HTLC-encumbered funds are
+	/// different from the non-HTLC-encumbered funds. This makes this an important knob to restrict
+	/// exposure to loss due to being offline for too long.
+	/// See [`ChannelHandshakeConfig::our_to_self_delay`] and [`ChannelConfig::cltv_expiry_delta`]
+	/// for more information.


suggestion: s/The online requirements for ensuring/The requirements for your node being online to ensure (just wanna make sure it's clear)

Add a config field `ChannelHandshakeConfig::max_inbound_htlc_value_in_flight_percent_of_channel` which sets the percentage of the channel value we cap the total value of outstanding inbound HTLCs to. This field can be set to a value between 1-100, where the value corresponds to the percent of the channel value in whole percentages. Note that: * If configured to another value than the default value 10, any new channels created with the non default value will cause versions of LDK prior to 0.0.104 to refuse to read the `ChannelManager`. * This caps the total value for inbound HTLCs in-flight only, and there's currently no way to configure the cap for the total value of outbound HTLCs in-flight. * The requirements for your node being online to ensure the safety of HTLC-encumbered funds are different from the non-HTLC-encumbered funds. This makes this an important knob to restrict exposure to loss due to being offline for too long. See `ChannelHandshakeConfig::our_to_self_delay` and `ChannelConfig::cltv_expiry_delta` for more information. Default value: 10. Minimum value: 1, any values less than 1 will be treated as 1 instead. Maximum value: 100, any values larger than 100 will be treated as 100 instead.

Use the `counterparty_max_htlc_value_in_flight_msat` value, and not the `holder_max_htlc_value_in_flight_msat` value when creating the `htlc_maximum_msat` value for `ChannelUpdate` messages. BOLT 7 specifies that the field MUST be less than or equal to `max_htlc_value_in_flight_msat` received from the peer, which we currently are not guaranteed to adhere to by using the holder value.

ViktorTigerstrom · 2022-05-03T20:45:30Z

Addressed the latest feedback, and updated the commit message for the first commit.

I squashed the fixup commits now as the new changes were quite minor. I hope that's ok :)

TheBlueMatt reviewed Apr 21, 2022

View reviewed changes

vincenzopalazzo previously approved these changes Apr 21, 2022

View reviewed changes

ViktorTigerstrom dismissed vincenzopalazzo’s stale review via fc984f0 April 26, 2022 22:50

ViktorTigerstrom force-pushed the 2022-04-use-counterparty-htlc-max-for-chan-updates branch from b8b23e7 to fc984f0 Compare April 26, 2022 22:50

ViktorTigerstrom commented Apr 26, 2022

View reviewed changes

TheBlueMatt reviewed Apr 27, 2022

View reviewed changes

TheBlueMatt reviewed Apr 29, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-04-use-counterparty-htlc-max-for-chan-updates branch from 3779ccf to d362cb5 Compare April 29, 2022 20:11

TheBlueMatt added the Seeking Code Review label Apr 29, 2022

valentinewallace self-requested a review May 2, 2022 19:37

valentinewallace reviewed May 2, 2022

View reviewed changes

valentinewallace removed the Seeking Code Review label May 2, 2022

ViktorTigerstrom commented May 2, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-04-use-counterparty-htlc-max-for-chan-updates branch from 4cc1fc2 to 4ea58ef Compare May 2, 2022 22:38

TheBlueMatt mentioned this pull request May 3, 2022

Reject outbound channels if the total reserve is larger than funding #1463

Merged

TheBlueMatt assigned valentinewallace May 3, 2022

valentinewallace reviewed May 3, 2022

View reviewed changes

ViktorTigerstrom added 4 commits May 3, 2022 22:35

Add test for configurable in-flight limit

d13061c

Add correct ChannelUpdate htlc_maximum_msat test

224d470

ViktorTigerstrom force-pushed the 2022-04-use-counterparty-htlc-max-for-chan-updates branch from 4ea58ef to 224d470 Compare May 3, 2022 20:43

valentinewallace approved these changes May 3, 2022

View reviewed changes

TheBlueMatt approved these changes May 3, 2022

View reviewed changes

TheBlueMatt merged commit 6418c9e into lightningdevkit:main May 3, 2022

TheBlueMatt mentioned this pull request May 13, 2022

Replace Channel::get_holder_max_htlc_value_in_flight_msat with configuration options #850

Closed

ViktorTigerstrom mentioned this pull request Jun 7, 2022

Cut 0.0.107 #1525

Merged

		/// `channel_value_satoshis` in msat, based on how many of the percent its been configured to
		/// be set to by the user.

Set ChannelUpdate htlc_maximum_msat using the peer's value #1444

Set ChannelUpdate htlc_maximum_msat using the peer's value #1444

Conversation

ViktorTigerstrom commented Apr 21, 2022

TheBlueMatt left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 21, 2022 • edited Loading

Codecov Report

ViktorTigerstrom commented Apr 21, 2022

vincenzopalazzo left a comment

Choose a reason for hiding this comment

ViktorTigerstrom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

ViktorTigerstrom Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Apr 28, 2022 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 29, 2022

ViktorTigerstrom commented Apr 29, 2022

TheBlueMatt commented Apr 29, 2022

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented May 2, 2022

ViktorTigerstrom May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented May 2, 2022

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented May 3, 2022

Set `ChannelUpdate` `htlc_maximum_msat` using the peer's value #1444

Set `ChannelUpdate` `htlc_maximum_msat` using the peer's value #1444

TheBlueMatt left a comment •

edited

Loading

codecov-commenter commented Apr 21, 2022 •

edited

Loading

ViktorTigerstrom left a comment •

edited

Loading

ViktorTigerstrom Apr 27, 2022 •

edited

Loading

ViktorTigerstrom Apr 26, 2022 •

edited

Loading

ViktorTigerstrom Apr 27, 2022 •

edited

Loading

ViktorTigerstrom commented Apr 28, 2022 •

edited

Loading

ViktorTigerstrom May 2, 2022 •

edited

Loading