Expose HTLCStats in ChannelDetails #2349

wvanlint · 2023-06-09T23:17:31Z

Exposes details around in-flight HTLCs through HTLCStats in ChannelDetails. This enables measuring saturation of a channel with regards to max_accepted_htlcs and max_htlc_value_in_flight_msat.

codecov-commenter · 2023-06-10T01:15:57Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (d401848) 90.48% compared to head (039c38d) 90.47%.

❗ Current head 039c38d differs from pull request most recent head 0f5cd35. Consider uploading reports for the commit 0f5cd35 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2349      +/-   ##
==========================================
- Coverage   90.48%   90.47%   -0.01%     
==========================================
  Files         104      104              
  Lines       53920    53930      +10     
  Branches    53920    53930      +10     
==========================================
+ Hits        48790    48795       +5     
- Misses       5130     5135       +5

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`93.28% <ø> (+0.07%)`	⬆️
lightning/src/ln/channel.rs	`89.68% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`86.78% <100.00%> (+0.03%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tnull

Generally makes sense to me, thanks for having a go at this!

lightning/src/ln/channel.rs

tnull · 2023-06-12T09:25:05Z

lightning/src/ln/channel.rs

 }

+impl_writeable_tlv_based!(HTLCStats, {
+	(0, pending_htlcs, required),
+	(1, pending_htlcs_value_msat, required),


When introducing mandatory fields for a new structure they usually should be even-numbered as this makes older nodes which for whatever reason encounter the unknown field stop and error rather than ignore and continue deserialization.

Ah I didn't realize this follows BOLT 1 serialization, done.

lightning/src/ln/channel.rs

tnull · 2023-06-12T09:30:48Z

lightning/src/ln/channel.rs

+	/// The total dust exposure for the local node.
+	pub on_holder_tx_dust_exposure_msat: u64,
+	/// The total value of pending, outgoing HTLC's being added that are awaiting remote revocation.
+	/// See `ChannelState::AwaitingRemoteRevoke`.


ChannelState is not public, so we shouldn't refer to it in the public API. If it has some relevant docs/information, we need to replicate them here in brief.

Added additional explanation instead of the reference.

lightning/src/ln/channel.rs

tnull · 2023-06-12T09:39:36Z

lightning/src/ln/channelmanager.rs

@@ -7190,6 +7200,8 @@ impl Writeable for ChannelDetails {
 			(35, self.inbound_htlc_maximum_msat, option),
 			(37, user_channel_id_high_opt, option),
 			(39, self.feerate_sat_per_1000_weight, option),
+			(40, self.incoming_htlc_stats, option),


These fields should both be odd: older nodes should just ignore them if they would ever encounter a ChannelDetails that was serialized with version 0.0.116 or above.

tnull · 2023-06-12T09:42:24Z

lightning/src/ln/channelmanager.rs

+	/// Statistics on pending incoming HTLCs.
+	///
+	/// This field is only `None` for `ChannelDetails` objects serialized prior to LDK 0.0.116.
+	pub incoming_htlc_stats: Option<HTLCStats>,


As the channel module is not public, HTLCStats is currently not exposed. We need to expose the object itself if it is now exposed through ChannelDetails.

Moved HTLCStats to the channelmanager module for that purpose.

TheBlueMatt · 2023-06-12T19:11:02Z

This needs a lot more motivation, I'm not sure I understand the goal here - I'm not sure whether this is intended to work towards #2087 or #2264 or something else. I'm also afraid these are a bit hard to discipher for a user, there's a lot of states an HTLC can be in, and the HTLCStats struct doesn't do a great job of indicating how much of each state an HTLC can be in.

wvanlint · 2023-06-12T22:31:09Z

This needs a lot more motivation, I'm not sure I understand the goal here - I'm not sure whether this is intended to work towards #2087 or #2264 or something else.

The main goal is to enable measuring the saturation of a channel with regards to max_accepted_htlcs and max_htlc_value_in_flight_msat, so I think this corresponds closest to #2087.

I'm also afraid these are a bit hard to discipher for a user, there's a lot of states an HTLC can be in, and the HTLCStats struct doesn't do a great job of indicating how much of each state an HTLC can be in.

Does this refer to Inbound/OutboundHTLCState? Should there be more granular aggregation in HTLCStats? For our purposes, the coarse granularity of HTLCStats might be sufficient.

TheBlueMatt · 2023-06-13T23:05:30Z

The main goal is to enable measuring the saturation of a channel with regards to max_accepted_htlcs and max_htlc_value_in_flight_msat

Ah! Okay, that's much more straightforward, so let's not expose all the nuance - lets try to break down the value into buckets such that it always totals to the current channel value eg: our_unencumbered_value, their_unencumbered_value, our_reserve, their_reserve, our_outbound_htlcs, and their_outbound_htlcs, (both as amounts and counts). There will always be some overhead for fees that we could try to write out but its a pita.

I think such an API will be much easier to use than the current HTLCStats, mostly because it would avoid the user having any idea what the "holding cell" is and the dust exposure being broken out.

That fails to communicate the dust exposure (ala #2264) but accomplishes what you want. Bonus points for solving both in one go - instead of returning the htlc amount/count, return the actual list of HTLCs with extra info (is it dust? its payment hash, CLTV expiry, etc).

lightning/src/ln/channelmanager.rs

wvanlint · 2023-07-22T00:01:19Z

That fails to communicate the dust exposure (ala #2264) but accomplishes what you want. Bonus points for solving both in one go - instead of returning the htlc amount/count, return the actual list of HTLCs with extra info (is it dust? its payment hash, CLTV expiry, etc).

Will handle both use cases by returning the list of pending HTLC's in #2442.

wvanlint force-pushed the htlc_stats_in_channel_details branch 2 times, most recently from 2d30dad to 374cec2 Compare June 9, 2023 23:54

tnull reviewed Jun 12, 2023

View reviewed changes

wvanlint force-pushed the htlc_stats_in_channel_details branch from 374cec2 to 039c38d Compare June 12, 2023 18:57

Expose HTLCStats in ChannelDetails

0f5cd35

wvanlint force-pushed the htlc_stats_in_channel_details branch from 039c38d to 0f5cd35 Compare June 12, 2023 21:24

tlulu reviewed Jul 17, 2023

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

wvanlint closed this Jul 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose HTLCStats in ChannelDetails #2349

Expose HTLCStats in ChannelDetails #2349

wvanlint commented Jun 9, 2023 •

edited

Loading

codecov-commenter commented Jun 10, 2023 •

edited

Loading

tnull left a comment

tnull Jun 12, 2023

wvanlint Jun 12, 2023

tnull Jun 12, 2023

wvanlint Jun 12, 2023 •

edited

Loading

tnull Jun 12, 2023

wvanlint Jun 12, 2023

tnull Jun 12, 2023

wvanlint Jun 12, 2023

TheBlueMatt commented Jun 12, 2023

wvanlint commented Jun 12, 2023 •

edited

Loading

TheBlueMatt commented Jun 13, 2023

wvanlint commented Jul 22, 2023

Expose HTLCStats in ChannelDetails #2349

Expose HTLCStats in ChannelDetails #2349

Conversation

wvanlint commented Jun 9, 2023 • edited Loading

codecov-commenter commented Jun 10, 2023 • edited Loading

Codecov Report

tnull left a comment

Choose a reason for hiding this comment

tnull Jun 12, 2023

Choose a reason for hiding this comment

wvanlint Jun 12, 2023

Choose a reason for hiding this comment

tnull Jun 12, 2023

Choose a reason for hiding this comment

wvanlint Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

tnull Jun 12, 2023

Choose a reason for hiding this comment

wvanlint Jun 12, 2023

Choose a reason for hiding this comment

tnull Jun 12, 2023

Choose a reason for hiding this comment

wvanlint Jun 12, 2023

Choose a reason for hiding this comment

TheBlueMatt commented Jun 12, 2023

wvanlint commented Jun 12, 2023 • edited Loading

TheBlueMatt commented Jun 13, 2023

wvanlint commented Jul 22, 2023

wvanlint commented Jun 9, 2023 •

edited

Loading

codecov-commenter commented Jun 10, 2023 •

edited

Loading

wvanlint Jun 12, 2023 •

edited

Loading

wvanlint commented Jun 12, 2023 •

edited

Loading