Include htlc min/max for ChannelDetails and ChannelCounterparty #1378

ViktorTigerstrom · 2022-03-22T23:38:48Z

Closes #1287.

Continues the implementation after PR #1297 was closed.

Note:
Please specifically check the serialization I've implemented, as I'm definitely not that confident that I'm doing it correctly.

Pushing the PR at this state to get some feedback of wether it looks ok. But I'll make sure to add these fields for the invoice creation in lightning-invoice/src/utils.rs as well as some tests later if this looks ok.

TheBlueMatt · 2022-03-23T23:07:16Z

lightning/src/ln/channel.rs

+		let holder_reserve = self.holder_selected_channel_reserve_satoshis;
+
+		cmp::min(
+			Some((self.channel_value_satoshis - counterparty_reserve + holder_reserve) * 1000),


We should subtract both reserves, not add ours.

Oh my 🤦... Thanks! Was a bit too tired when I wrote that, as well as the inversed state check above :)

TheBlueMatt · 2022-03-23T23:08:53Z

lightning/src/ln/channel.rs

+		if self.channel_state >= ChannelState::TheirInitSent as u32 {
+			return None;
+		}
+		let counterparty_reserve = self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0);


If we're past TheirInitSent this is always defined, you could instead just None-check it and return based on that instead of doing a state check.

TheBlueMatt · 2022-03-24T00:26:58Z

lightning/src/ln/channelmanager.rs

@@ -1299,6 +1303,10 @@ pub struct ChannelDetails {
 	pub is_usable: bool,
 	/// True if this channel is (or will be) publicly-announced.
 	pub is_public: bool,
+	/// The smallest value HTLC (in msat) we will accept, for this channel.
+	pub htlc_minimum_msat: u64,


Maybe lets call these "inbound" or something (like "inbound_htlc_minimum_msat"), to make it clearer without needing to look at docs what they're talking about.

Will do, thanks!

TheBlueMatt · 2022-03-24T00:28:19Z

lightning/src/ln/channelmanager.rs

@@ -6075,6 +6087,8 @@ impl_writeable_tlv_based!(ChannelDetails, {
 	(28, is_funding_locked, required),
 	(30, is_usable, required),
 	(32, is_public, required),
+	(34, htlc_minimum_msat, required),


We shouldn't be adding new required even values for these, I think, no? Even means "old versions will fail to read" (instead of ignore), and required means we'll fail to read if its missing (ie old versions).

Ok thanks, I'll make this an odd value instead to ignore it for old versions.

TheBlueMatt · 2022-03-24T00:30:30Z

lightning/src/ln/channel.rs

 		match &self.counterparty_forwarding_info {
 			Some(info) => {
 				1u8.write(writer)?;
 				info.fee_base_msat.write(writer)?;
 				info.fee_proportional_millionths.write(writer)?;
 				info.cltv_expiry_delta.write(writer)?;
+				// Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.


I'm not really sure what's going on here anymore. We shouldn't be adding new stuff.

Ok thanks! Would it be more correct to only add this as a TLV Optional field instead?

Yes, we can only add new fields via TLV now.

ViktorTigerstrom · 2022-03-28T17:11:45Z

Thanks for the feedback! Sorry for the delay in continuing this PR. I've been out traveling, but will pick this up again tomorrow.

codecov-commenter · 2022-03-29T23:45:53Z

Codecov Report

Merging #1378 (9769122) into main (e0b9b74) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9769122 differs from pull request most recent head dd4a3ed. Consider uploading reports for the commit dd4a3ed to get more accurate results

@@           Coverage Diff           @@
##             main    #1378   +/-   ##
=======================================
  Coverage   90.87%   90.87%           
=======================================
  Files          74       74           
  Lines       41405    41477   +72     
  Branches    41405    41477   +72     
=======================================
+ Hits        37626    37693   +67     
- Misses       3779     3784    +5

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`96.85% <100.00%> (+0.12%)`	⬆️
lightning/src/ln/channel.rs	`88.41% <100.00%> (+0.04%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.73% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`92.59% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`95.56% <0.00%> (+0.01%)`	⬆️
lightning/src/util/events.rs	`33.56% <0.00%> (+0.11%)`	⬆️

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 e0b9b74...dd4a3ed. Read the comment docs.

ViktorTigerstrom · 2022-03-29T23:53:43Z

Addressed the latest feedback, and also added htlc min/max to the create invoice functions.
Hope it looks better!

Also, please check that the htcl max value we use to create ChannelUpdate msgs is correct:

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

Line 2557 in 7671ae5

htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),

I intentionally kept this as it is, due to the comment in the get_announced_htlc_max_msat function:
https://github.com/lightningdevkit/rust-lightning/blob/7671ae54522197dbca495c4c7b3fec5c814a1841/lightning/src/ln/channel.rs#L4330:L4338

Is this the value we prefer to use, or would we rather use the htlc_max_msat value from the get_holder_htlc_maximum_msat function I've created for ChannelUpdate msgs after it's available?

TheBlueMatt

Sorry for the delay in reviewing this!

TheBlueMatt · 2022-04-01T15:39:38Z

lightning/src/ln/channelmanager.rs

@@ -6075,6 +6087,8 @@ impl_writeable_tlv_based!(ChannelDetails, {
 	(28, is_funding_locked, required),
 	(30, is_usable, required),
 	(32, is_public, required),
+	(33, inbound_htlc_minimum_msat, required),
+	(34, inbound_htlc_maximum_msat, option),


We shouldn't be adding new even fields here unless we really need to - even means old versions of LDK will refuse to read the new objects which we don't generally want.

TheBlueMatt · 2022-04-01T15:45:53Z

lightning/src/ln/channel.rs

+		// Read fields for `CounterpartyForwardingInfo`. As the `outbound_htlc_maximum_msat` is a
+		// TLV Optional variant, we construct the `CounterpartyForwardingInfo` struct after it ha
+		// been read.
+		let (has_forwarding_info, fee_base_msat, fee_proportional_millionths, cltv_expiry_delta) = match <u8 as Readable>::read(reader)? {


This may be more readable if we just create a CounterpartyForwardingInfo object with the htlc_maximum_msat and htlc_minimum_msat fields initialized to some default.

TheBlueMatt · 2022-04-01T15:46:35Z

lightning/src/ln/channel.rs

+		// Construct the `CounterpartyForwardingInfo` after the TLV Optional variant for
+		// `outbound_htlc_maximum_msat` has been read.
+		let counterparty_forwarding_info = match has_forwarding_info {
+			false => None,


I dont think we can do this - if we're reading an object serialized by an older version of LDK this would imply we jump back to the channel being uninitialized and we'll panic on some unwraps above - we need to initialize the fields to some default.

TheBlueMatt · 2022-04-01T15:46:58Z

lightning/src/ln/channelmanager.rs

@@ -6048,6 +6058,8 @@ impl_writeable_tlv_based!(CounterpartyForwardingInfo, {
 	(2, fee_base_msat, required),
 	(4, fee_proportional_millionths, required),
 	(6, cltv_expiry_delta, required),
+	(7, outbound_htlc_minimum_msat, required),
+	(8, outbound_htlc_maximum_msat, option),


Same here, can't add new even fields.

TheBlueMatt · 2022-04-01T15:47:12Z

lightning/src/ln/channelmanager.rs

@@ -6048,6 +6058,8 @@ impl_writeable_tlv_based!(CounterpartyForwardingInfo, {
 	(2, fee_base_msat, required),
 	(4, fee_proportional_millionths, required),
 	(6, cltv_expiry_delta, required),
+	(7, outbound_htlc_minimum_msat, required),


We shouldnt add new required fields - this will mean we refuse to read objects serialized by older LDK versions.

ViktorTigerstrom · 2022-04-03T23:52:55Z

Thanks a lot for the review and feedback! Updated the PR with the requested changes.

@TheBlueMatt do you possibly have any tips of how I can manually test that my code is backwards compatible (or any other way to tell that it is)? It would be really helpful and I'd appreciate it a lot, so I don't end up making the same mistakes for future PRs :).

In commits I pushed 5 days ago ( 7c25533 ), I tried testing it by setting up a sample node with an older version of LDK that I set up a few channels with. I then upgraded LDK for the node to the my local version with the new min/max fields support, set up one more channel, and then finally downgraded LDK to the old version again. The serialization/deserialization of the channels seemed successful throughout the process from what I could tell, but I might be verifying that in an incorrect way....

ViktorTigerstrom · 2022-04-04T00:02:59Z

lightning/src/ln/channel.rs

+			counterparty_forwarding_info = Some(CounterpartyForwardingInfo {
+				fee_base_msat: cf_info.fee_base_msat,
+				fee_proportional_millionths: cf_info.fee_proportional_millionths,
+				cltv_expiry_delta: cf_info.cltv_expiry_delta,
+				outbound_htlc_minimum_msat: cf_info.outbound_htlc_minimum_msat,
+				outbound_htlc_maximum_msat: outbound_htlc_maximum_msat
+			});


Made this create a new CounterpartyForwardingInfo object with the retrieved outbound_htlc_maximum_msat value, to not require the outbound_htlc_maximum_msat field for CounterpartyForwardingInfo be mutable. But I can of course change that if we prefer the alternative!

It should be less code to make it mutable - can just change the as_ref(), above, to as_mut() and it'll be mutable :)

TheBlueMatt

Nice this looks great now! Feel free to squash down the fixup commits and we'll get you another reviewer ;)

TheBlueMatt · 2022-04-04T18:44:40Z

lightning/src/ln/channel.rs

@@ -5829,6 +5848,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 			(17, self.announcement_sigs_state, required),
 			(19, self.latest_inbound_scid_alias, option),
 			(21, self.outbound_scid_alias, required),
+			(23, self.counterparty_forwarding_info.as_ref().map(|info| info.outbound_htlc_maximum_msat).unwrap_or(None), option),


nit: You don't need the unwrap_or(None) - its already None if the unwrap fails :)

Removing the unwrap_or(None) would result in Some(Some(outbound_htlc_maximum_msat_value)) if a value for outbound_htlc_maximum_msat exists, as both counterparty_forwarding_info and outbound_htlc_maximum_msat are optional fields. And we need it to result in a Optional<u64> value only. But perhaps there is a neater way to solve that :)

TheBlueMatt · 2022-04-04T18:46:15Z

lightning/src/ln/channelmanager.rs

@@ -1159,6 +1159,10 @@ pub struct CounterpartyForwardingInfo {
 	/// such that the outgoing HTLC is forwardable to this counterparty. See `msgs::ChannelUpdate`'s
 	/// `cltv_expiry_delta` for more details.
 	pub cltv_expiry_delta: u16,
+	/// The smallest value HTLC (in msat) the remote peer will accept, for this channel.


May be worth noting that this is only None for objects serialized by versions of LDK prior to 0.0.107 (assuming this lands for 107).

TheBlueMatt · 2022-04-04T18:46:47Z

@TheBlueMatt do you possibly have any tips of how I can manually test that my code is backwards compatible (or any other way to tell that it is)? It would be really helpful and I'd appreciate it a lot, so I don't end up making the same mistakes for future PRs :).

Currently we've been doing this on a very ad-hoc basis. Its not good, not at all. #1089 exists to track the wishlist item of having some test framework for it, but one thing you can do today is build a functional test that runs a node to where you want, then dumps out the serialized data. Run it on the last release and then get the hex and check that in to a new test as input.

ViktorTigerstrom · 2022-04-04T21:53:15Z

Currently we've been doing this on a very ad-hoc basis. Its not good, not at all. #1089 exists to track the wishlist item of having some test framework for it, but one thing you can do today is build a functional test that runs a node to where you want, then dumps out the serialized data. Run it on the last release and then get the hex and check that in to a new test as input.

Ok, thanks a lot for clarifying! Ah I see, that would be very helpful :). Great thanks, I'll remember that from now on!

ViktorTigerstrom · 2022-04-04T22:05:35Z

Addressed the latest feedback, and squashed the commits!

Ps. can you please verify that I'm handling which htcl max value we use to create ChannelUpdate msgs correctly, which I mentioned in my comment above :)? #1378 (comment)

TheBlueMatt

Sorry for the review delay here. I think this is basically good modulo one comment below.

TheBlueMatt · 2022-04-11T17:31:35Z

lightning/src/ln/channelmanager.rs

@@ -1159,6 +1159,11 @@ pub struct CounterpartyForwardingInfo {
 	/// such that the outgoing HTLC is forwardable to this counterparty. See `msgs::ChannelUpdate`'s
 	/// `cltv_expiry_delta` for more details.
 	pub cltv_expiry_delta: u16,
+	/// The smallest value HTLC (in msat) the remote peer will accept, for this channel. This field
+	/// is only None for `CounterpartyForwardingInfo` objects serialized prior to LDK 0.0.107
+	pub outbound_htlc_minimum_msat: Option<u64>,


Now that we have this here we should probably delete the copy at Channel::counterparty_htlc_minimum_msat - its entirely redundant and somewhat confusing to have two copies of the same field in the same object.

Sorry for the delay in continuing this PR. @TheBlueMatt I'd need some guidance on how to continue given your comment.

In the current version of LDK, the Channel::counterparty_htlc_minimum_msat value is a non optional field that is assigned through the OpenChannel msg for inbound channels in the new_from_req function, and through the AcceptChannel msg in the accept_channel function in for outbound channels. The new Channel::counterparty_forwarding_info::outbound_htlc_minimum_msat would be an Optional field that's assigned at a later stage in the message flow.

The Channel::counterparty_forwarding_info field is first assigned through the first ChannelUpdate msg in the channel_update function, which with this PR will assign the new outbound_htlc_minimum_msat field.
The channel_update function never updates the value for the Channel::counterparty_htlc_minimum_msat field in the current version of LDK, which already proves your point about why it's a bad idea two have two copies of the same field, even though they should be the same value in practice.

I can't really assign the Channel::counterparty_forwarding_info::outbound_htlc_minimum_msat field through the info in the OpenChannel msg or the AcceptChannel msg, as the rest of the information to construct the Channel::counterparty_forwarding_info field doesn't exist yet, unless I assign dummy values to the other fields of the CounterpartyForwardingInfo.

Given the above, how do we want to proceed?

Simply ignoring the values for htlc_minimum_msat in the OpenChannel and AcceptChannel msgs is quite bad and it's a bit of a downgrade from the current version of LDK, as the value would then only be available in the Channel struct at a later state in the message flow (after the first ChannelUpdate msg).
But the alternative of creating a Channel::counterparty_forwarding_info field with dummy values is also bad practice. To avoid assigning dummy values, I guess we could make all fields of CounterpartyForwardingInfo optional, that that's not great either. How should this be handled?

How should the htlc_minimum_msat value passed in the ChannelUpdate msg be handled? Should it be ignored like the current version of LDK, or would we like it to update the channel's value for every ChannelUpdate msg received?

(Or is all the above maybe a sign that the outbound_htlc_minimum_msat isn't really suitable to place in the CounterpartyForwardingInfo struct?)

Ah! Sorry for the confusion here, yea. It really sucks the protocol has two somewhat redundant fields for this :(. I think, however, we should set the outbound_htlc_minimum_msat field to the maximum of what the peer told us in their init and what's in the channel update.

Would the ChannelUpdate be more relevant than what was in the OpenChannel/AcceptChannel message? Just wondering why we would take the max if we later learn a different value.

Ok thanks, so would we like to keep the Channel::counterparty_htlc_minimum_msat field then as the protocol also has two fields for htlc_minimum_msat (and maybe rename it to counterparty_init_htlc_minimum_msat)?

Or should we just stick to the counterparty_forwarding_info::outbound_htlc_minimum_msat field and assign it in when receiving the OpenChannel/AcceptChannel msgs as well?

Okay, I'm an idiot, for the fourth time here.The htlc_min/max included in the channel_update we receive from our peer is re-announcing our limits. The updates can also be things announced publicly and give routing information for how to get from our peer to us, so it has to be the limits we apply on HTLCs. Our current code is correct to ignore them - we set the inbound HTLC limits, not our counterparty.

Oh! Ok, then assigning the counterparty_forwarding_info::outbound_htlc_minimum_msat based on the ChannelUpdate msg like my code currently does is also incorrect. I guess it should be assigned using the same logic as the get_holder_htlc_maximum_msat function I've created, but using counterparty_max_htlc_value_in_flight_msat instead of holder_max_htlc_value_in_flight_msat correct?

Hmmm, yea, that's kinda tricky. I definitely think the public API needs to remain the same - forwarding_info in the ChannelDetails needs to remain None until we have the feerate info from our counterparty. How that translates to storage inside the Channel, though, I don't have a super strong opinion. Another thing to think about is maybe moving the new fields to ChannelCounterparty instead of CounterpartyForwardingInfo. This would line up a bit better with the docs on ChannelCounterparty::forwarding_info anyway, which says "Information on the fees and requirements that the counterparty requires when forwarding payments to us through this channel" - the new fields are about forwarding outbound from us.

Ok thanks! Yes definitely makes sense that we don't want to change the public API.

Great, removing counterparty_htlc_minimum_msat from the forwarding_info would help a lot. Would we be ok with keeping the Channel::counterparty_htlc_minimum_msat field in the Channel struct then, and use the public functions of the Channel to fetch that value when creating the ChannelCounterparty in ChannelManager::list_channels_with_filter, to assign the new ChannelCounterparty::outbound_htlc_minimum_msat field?

Yep, sounds good.

jkczyz · 2022-04-15T20:41:44Z

lightning/src/ln/channel.rs

+		match &self.counterparty_selected_channel_reserve_satoshis {
+			None => None,
+			Some(counterparty_reserve) => {


Rust's Option::map function is pretty handy for removing this boilerplate.

self.counterparty_selected_channel_reserve_satoshis.map(|counterparty_reserve| { // ... })

Ah nice, thanks!

jkczyz · 2022-04-15T20:42:14Z

lightning/src/ln/channel.rs

+					Some((self.channel_value_satoshis - counterparty_reserve - holder_reserve) * 1000),
+					Some(self.holder_max_htlc_value_in_flight_msat)


Which would also allow you to remove the Some wrappings here.

jkczyz · 2022-04-15T20:44:12Z

lightning/src/ln/channelmanager.rs

@@ -1299,6 +1299,11 @@ pub struct ChannelDetails {
 	pub is_usable: bool,
 	/// True if this channel is (or will be) publicly-announced.
 	pub is_public: bool,
+	/// The smallest value HTLC (in msat) we will accept, for this channel. This field
+	/// is only None for `ChannelDetails` objects serialized prior to LDK 0.0.107


nit: Add ticks around None

jkczyz · 2022-04-15T21:00:28Z

lightning/src/ln/channelmanager.rs

@@ -1159,6 +1159,11 @@ pub struct CounterpartyForwardingInfo {
 	/// such that the outgoing HTLC is forwardable to this counterparty. See `msgs::ChannelUpdate`'s
 	/// `cltv_expiry_delta` for more details.
 	pub cltv_expiry_delta: u16,
+	/// The smallest value HTLC (in msat) the remote peer will accept, for this channel. This field
+	/// is only None for `CounterpartyForwardingInfo` objects serialized prior to LDK 0.0.107
+	pub outbound_htlc_minimum_msat: Option<u64>,


Would the ChannelUpdate be more relevant than what was in the OpenChannel/AcceptChannel message? Just wondering why we would take the max if we later learn a different value.

ViktorTigerstrom

Addressed the latest feedback. I have added some comments regarding my implementation that might be helpful during the review, now that the Channel::counterparty_htlc_minimum_msat field has been removed.

ViktorTigerstrom · 2022-04-17T22:05:33Z

lightning/src/ln/channel.rs

+			counterparty_forwarding_info: Some(CounterpartyForwardingInfo {
+				fee_base_msat: 0,
+				fee_proportional_millionths: 0,
+				cltv_expiry_delta: 0,
+				outbound_htlc_minimum_msat: Some(msg.htlc_minimum_msat),
+				outbound_htlc_maximum_msat: None,
+			}),


As I mentioned in the conversation above, I need to create a CounterpartyForwardingInfo object already here now, in order to be able to store the msg.htlc_minimum_msat value now that the Channel::counterparty_htlc_minimum_msat has been removed. This forced me to set dummy values for fee_base_msat, fee_proportional_millionths and cltv_expiry_delta as we don't have the values for those yet. This isn't great... But making all of them optional values isn't that great either... Not sure what's best here.

ViktorTigerstrom · 2022-04-17T22:05:56Z

lightning/src/ln/channel.rs

+		self.counterparty_forwarding_info = Some(CounterpartyForwardingInfo {
+			fee_base_msat: 0,
+			fee_proportional_millionths: 0,
+			cltv_expiry_delta: 0,
+			outbound_htlc_minimum_msat: Some(msg.htlc_minimum_msat),
+			outbound_htlc_maximum_msat: self.get_counterparty_htlc_maximum_msat(),
+		});


The same as above applies here.

ViktorTigerstrom · 2022-04-17T22:14:50Z

lightning/src/ln/channel.rs

@@ -4303,7 +4317,22 @@ impl<Signer: Sign> Channel<Signer> {

 	/// Allowed in any state (including after shutdown)
 	pub fn get_counterparty_htlc_minimum_msat(&self) -> u64 {


Decided to keep the return type of this function as u64, and not bleed out to the rest of the project that this is now stored as a Option<u64> type, and return 0 in case it doesn't exist, as this was the same value it was initially set to previously in the new_outbound function.

The only time the outbound_htlc_minimum_msat shouldn't exist, is when when an outbound channel has been created by the new_outbound function, which has not yet been accepted by the peer.

If you'd prefer that we make this function private and force that the field is accessed directly through the CounterpartyForwardingInfo::outbound_htlc_minimum_msat that's also an option. Or if you think it's more appropriate that this function returns a Option<u64> type, I can of course change to that as well.

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

lightning-invoice/src/utils.rs

@@ -591,46 +594,6 @@ mod test {
 		match_invoice_routes(Some(5000), &nodes[0], scid_aliases);
 	}

-	#[test]
-	fn test_forwarding_info_not_assigned_channel_excluded_from_hints() {


This test required that the Channel::counterparty_forwarding_info field wasn't assigned until the first ChannelUpdate msg. The Channel::counterparty_forwarding_info field will now be assigned as soon as the channel is accepted by both parties.

ViktorTigerstrom · 2022-04-17T22:30:17Z

lightning-invoice/src/utils.rs

@@ -922,62 +919,6 @@ mod test {
 		);
 	}

-	#[test]
-	#[cfg(feature = "std")]
-	fn test_multi_node_forwarding_info_not_assigned_channel_excluded_from_hints() {


This test required that the Channel::counterparty_forwarding_info field wasn't assigned until the first ChannelUpdate msg. The Channel::counterparty_forwarding_info field will now be assigned as soon as the channel is accepted by both parties.

ViktorTigerstrom · 2022-04-19T22:08:39Z

Added the outbound htlc min/max to the ChannelCounterparty struct instead of the CounterpartyForwardingInfo struct, as discussed in the feedback.

Had to rebase on upstream to not make the checks fail, so made the decision to squash the commits as also much of the previous code was dropped in addressing the feedback. Sorry about that!

TheBlueMatt

One API note but looks good.

TheBlueMatt · 2022-04-20T02:34:09Z

lightning/src/ln/channelmanager.rs

@@ -1670,6 +1680,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						features: InitFeatures::empty(),
 						unspendable_punishment_reserve: to_remote_reserve_satoshis,
 						forwarding_info: channel.counterparty_forwarding_info(),
+						outbound_htlc_minimum_msat: Some(channel.get_counterparty_htlc_minimum_msat()),


We should check the channel state and only assign this if the counterparty has given us a value. We could change the channel internal API to return an option too but that's a good chunk more work.

jkczyz

LGTM

ViktorTigerstrom · 2022-04-20T20:50:25Z

Addressed the latest feedback! Let me know if it looks ok and I'll squash the commit.

jkczyz · 2022-04-20T20:55:46Z

lightning/src/ln/channelmanager.rs

+						// message (as they are always the first message from the counterparty).
+						// Else `Channel::get_counterparty_htlc_minimum_msat` could return the
+						// default `0` value set by `Channel::new_outbound`.
+						outbound_htlc_minimum_msat: channel.have_received_message().then(|| channel.get_counterparty_htlc_minimum_msat()),


Won't be able to use then because our MSRV is 1.41.1. Will have to use an if-else expression.

Oh ok thanks! I'll update immediately

ViktorTigerstrom · 2022-04-20T21:55:40Z

Hmmmm sorry I just want to raise my comment #1378 (comment) once more as I never received any direct reply to it:

Is my assumption correct that we still want to use Channel::get_announced_htlc_max_msat when constructing the ChannelUpdate msgs, or would we want to use my new Channel::get_holder_htlc_maximum_msat function?
Given that we use our channel values (in Channel::get_announced_htlc_max_msat) to set the htlc_maximum_msat field for the ChannelUpdate msg we create (and we are not reannouncing the counterparties' values as we do for the htlc_minimum_msat field), shouldn't we use the htlc_maximum_msat field from the ChannelUpdate message the peer sends us to set the value for the ChannelCounterparty::outbound_htlc_maximum_msat, and not the value from the Channel::get_counterparty_htlc_maximum_msat function like my current code does?

TheBlueMatt · 2022-04-21T01:50:41Z

Is my assumption correct that we still want to use Channel::get_announced_htlc_max_msat when constructing the ChannelUpdate msgs, or would we want to use my new Channel::get_holder_htlc_maximum_msat function?

That is correct, use the existing one.

Given that we use our channel values (in Channel::get_announced_htlc_max_msat) to set the htlc_maximum_msat field for the ChannelUpdate msg we create (and we are not reannouncing the counterparties' values as we do for the htlc_minimum_msat field), shouldn't we use the htlc_maximum_msat field from the ChannelUpdate message the peer sends us to set the value for the ChannelCounterparty::outbound_htlc_maximum_msat, and not the value from the Channel::get_counterparty_htlc_maximum_msat function like my current code does?

Gah, yep, that's an issue, but the wrong solution - we need to use the counterparty max_value_in_flight for the announced_htlc_max_msat.

TheBlueMatt · 2022-04-21T02:37:38Z

That said, maybe lets fix that in a followup PR - in the mean time can you squash the fixup here and we can land this?

ViktorTigerstrom · 2022-04-21T07:02:46Z

Gah, yep, that's an issue, but the wrong solution - we need to use the counterparty max_value_in_flight for the announced_htlc_max_msat.

Ah ok!

That said, maybe lets fix that in a followup PR - in the mean time can you squash the fixup here and we can land this?

Sure sounds good! Squashed without changes.

ViktorTigerstrom · 2022-04-21T10:29:42Z

Updated the docs for ChannelCounterparty::outbound_htlc_minimum_msat as I just remembered it was outdated to:

/// The smallest value HTLC (in msat) the remote peer will accept, for this channel. This field
/// is only `None` before we have received either the `OpenChannel` or `AcceptChannel` message
/// from the remote peer, or for `ChannelCounterparty` objects serialized prior to LDK 0.0.107.

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from 2814c10 to 8caa487 Compare March 23, 2022 09:13

TheBlueMatt reviewed Mar 24, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from 0724de0 to d4987dd Compare March 29, 2022 23:51

TheBlueMatt reviewed Apr 1, 2022

View reviewed changes

ViktorTigerstrom commented Apr 4, 2022

View reviewed changes

TheBlueMatt reviewed Apr 4, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from 3bd0191 to d426d6f Compare April 4, 2022 22:01

TheBlueMatt reviewed Apr 11, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label Apr 15, 2022

jkczyz self-requested a review April 15, 2022 16:56

jkczyz reviewed Apr 15, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch 2 times, most recently from 34131c4 to b3dc51d Compare April 17, 2022 22:24

ViktorTigerstrom commented Apr 17, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from b3dc51d to c15c0a5 Compare April 17, 2022 22:47

TheBlueMatt removed the Seeking Code Review label Apr 18, 2022

Add inbound htlc min/max to ChannelDetails

6644ef1

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from c15c0a5 to cb5835c Compare April 19, 2022 22:04

ViktorTigerstrom changed the title ~~Include htlc min/max for ChannelDetails and CounterpartyForwardingInfo~~ Include htlc min/max for ChannelDetails and ChannelCounterparty Apr 19, 2022

TheBlueMatt previously approved these changes Apr 20, 2022

View reviewed changes

TheBlueMatt assigned jkczyz Apr 20, 2022

jkczyz previously approved these changes Apr 20, 2022

View reviewed changes

ViktorTigerstrom dismissed stale reviews from jkczyz and TheBlueMatt via d259043 April 20, 2022 20:46

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from d259043 to 7bc9462 Compare April 20, 2022 20:51

jkczyz reviewed Apr 20, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from 7bc9462 to 9769122 Compare April 20, 2022 21:00

jkczyz previously approved these changes Apr 20, 2022

View reviewed changes

ViktorTigerstrom dismissed jkczyz’s stale review via dd4a3ed April 21, 2022 07:00

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from 9769122 to dd4a3ed Compare April 21, 2022 07:00

ViktorTigerstrom added 2 commits April 21, 2022 12:27

Add outbound min/max to ChannelCounterparty

63f0a31

Add htlc min/max to create invoice functions

3c67687

ViktorTigerstrom force-pushed the 2022-03-include-htlc-min-max branch from dd4a3ed to 3c67687 Compare April 21, 2022 10:28

jkczyz approved these changes Apr 21, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Apr 21, 2022

Use correct value for htlc_max in channel_update messages #1443

Closed

TheBlueMatt approved these changes Apr 21, 2022

View reviewed changes

TheBlueMatt merged commit 637fb88 into lightningdevkit:main Apr 21, 2022

ViktorTigerstrom mentioned this pull request Jun 7, 2022

Cut 0.0.107 #1525

Merged

		Some((self.channel_value_satoshis - counterparty_reserve - holder_reserve) * 1000),
		Some(self.holder_max_htlc_value_in_flight_msat)

		@@ -4303,7 +4317,22 @@ impl<Signer: Sign> Channel<Signer> {

		/// Allowed in any state (including after shutdown)
		pub fn get_counterparty_htlc_minimum_msat(&self) -> u64 {

Include htlc min/max for ChannelDetails and ChannelCounterparty #1378

Include htlc min/max for ChannelDetails and ChannelCounterparty #1378

Uh oh!

Conversation

ViktorTigerstrom commented Mar 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom commented Mar 28, 2022

Uh oh!

codecov-commenter commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ViktorTigerstrom commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom commented Apr 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Apr 4, 2022

Uh oh!

ViktorTigerstrom commented Apr 4, 2022

Uh oh!

ViktorTigerstrom commented Apr 4, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Mar 28, 2022 •

edited

Loading

ViktorTigerstrom Mar 28, 2022 •

edited

Loading

codecov-commenter commented Mar 29, 2022 •

edited

Loading

ViktorTigerstrom commented Mar 29, 2022 •

edited

Loading

ViktorTigerstrom Apr 4, 2022 •

edited

Loading

ViktorTigerstrom Apr 15, 2022 •

edited

Loading

ViktorTigerstrom Apr 15, 2022 •

edited

Loading