Add `ChannelContext::get_commitment_stats` #3682

tankyleo · 2025-03-24T23:03:34Z

    Add `ChannelContext::get_commitment_stats`
    
    It can be useful to get the stats on a potential commitment transaction
    without actually building it. Therefore, this commit splits the stats
    calculations from the actual build of a commitment transaction.
    
    This introduces an extra loop over the pending htlcs, but current
    network behavior produces very few concurrent htlcs on channels.
    Furthermore, each iteration of the loop in the stats calculation is very
    cheap.

Requested by @wpaulino in #3641

    Rename `CommitmentTransaction::htlcs` to `nondust_htlcs`

    Remove unnecessary clones of the HTLC-HTLCSource table

    Require all HTLC success states to hold their corresponding preimage
    
    This allows us to DRY the code that calculates the
    `value_to_self_msat_offset` in `ChannelContext::build_commitment_stats`.
    
    HTLC success states have held their corresponding preimage since
    0.0.105, and the release notes of 0.1 already require users running
    0.0.123 and earlier to resolve their HTLCs before upgrading to 0.1. So
    this change is fully compatible with existing upgrade paths to
    the yet-to-be-shipped 0.2 release.

ldk-reviews-bot · 2025-03-24T23:03:37Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-03-25T13:23:28Z

Why is this draft?

tankyleo · 2025-03-25T15:14:47Z

Why is this draft?

@TheBlueMatt I don't feel great about the code duplication between get_commitment_stats and build_commitment_transaction. Both functions filter for the non-dust HTLCs, but they do different things with them.

Let me know what you think.

tankyleo · 2025-03-25T16:02:45Z

I'm working on further cleanups now.

tankyleo · 2025-03-25T20:02:45Z

@wpaulino the PR is in a better spot now, let me know what you think.

lightning/src/ln/channel.rs

codecov · 2025-03-27T15:32:41Z

Codecov Report

Attention: Patch coverage is 98.00995% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.16%. Comparing base (2e435de) to head (71df614).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	86.66%	2 Missing ⚠️
lightning/src/ln/channel.rs	98.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3682      +/-   ##
==========================================
- Coverage   89.18%   89.16%   -0.03%     
==========================================
  Files         155      155              
  Lines      120796   120861      +65     
  Branches   120796   120861      +65     
==========================================
+ Hits       107731   107760      +29     
- Misses      10415    10446      +31     
- Partials     2650     2655       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tankyleo · 2025-03-27T16:15:58Z

@TheBlueMatt take a look when you can thank you!

TheBlueMatt · 2025-03-28T13:23:27Z

lightning/src/ln/channel.rs

@@ -223,6 +223,25 @@ impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> {
 	}
 }

+impl InboundHTLCState {
+	fn as_str(&self) -> &str {


Heh, if its just for logging I think we can #[derive(Debug)] :)

Debug would log all their inner contents too - don't think that's what we want here ?

I would have to derive Debug in a bunch more places.

Actually I'll just move that function to an impl fmt::Debug for InboundHTLCState :)

Went with an impl fmt::Display here, seems this corresponds most to what we want.

TheBlueMatt · 2025-03-28T13:25:01Z

lightning/src/ln/channel.rs

-	/// have not yet committed it. Such HTLCs will only be included in transactions which are being
-	/// generated by the peer which proposed adding the HTLCs, and thus we need to understand both
-	/// which peer generated this transaction and "to whom" this transaction flows.
+	fn for_each_inbound<F>(&self, generated_by_local: bool, mut for_each: F)


The fn name here doesn't really communicate what it does/is used for. Maybe inbound_htlcs_in_commitment? It might also be more Rust-y to return an impl Iterator and let the callsite do the for, rather than doing it as a callback.

We initially had something like this is it closer to what you have in mind?

for htlc in self.pending_inbound_htlcs.iter() { if htlc.state.included_in_commitment(generated_by_local) { .. } else { .. } }

some other options for that specific function's return value:

one iterator yielding (included, htlc)

two iterators each yielding htlc, one for included, one for excluded.

Went with the very first option above.

TheBlueMatt

Basically LGTM, one nit. Sorry somehow I lost track of this one.

TheBlueMatt · 2025-04-02T18:08:59Z

lightning/src/ln/channel.rs

+		macro_rules! count_nondust_htlc {
+			($htlc: expr, $outbound: expr) => {
+				if $htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, self.get_channel_type()) {
+					log_trace!(logger, "   ...not counting {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat);


Hmm, this is gonna end up doubling the amount we log when we build a commitment - instead of logging once per HTLC we're now logging twice, once when building the stats and once when we build the commitment itself. IMO we should make one of the two methods silent (which depends a bit on how you intend to use commitment_stats going forward, which isn't clear from the commit msg.

No problem ! Thank you I added the motivation for commitment_stats in the commit message, and removed all logs from that function. I prefer to let callers of this function do the logging depending on the use case.

wpaulino · 2025-04-02T22:49:58Z

lightning/src/ln/channel.rs

+			} else {
+				htlc_success_tx_weight(features)
+			};
+			feerate_per_kw as u64 * htlc_tx_weight / 1000


Nit: could note that we intentionally round down here as required by the spec

thanks will do

wpaulino · 2025-04-02T22:54:24Z

lightning/src/ln/channel.rs

-						value_to_self_msat_offset += htlc.amount_msat as i64;
-					},
-					_ => {},
+				if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_preimage)) = htlc.state {


Nit: use preimage() instead and check for Some?

wpaulino · 2025-04-02T22:57:25Z

lightning/src/ln/channel.rs

-						value_to_self_msat_offset -= htlc.amount_msat as i64;
-					},
-					_ => {},
+					OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_))


Same here regarding preimage

Both of these work so far - I just need to check that this comment here is true for the outbound states:

enum OutboundHTLCOutcome { /// LDK version 0.0.105+ will always fill in the preimage here. Success(Option<PaymentPreimage>), Failure(HTLCFailReason), } ``

@TheBlueMatt what do you think of replacing this if let ... with a if htlc.state.preimage().is_some() call ?

Would assume no upgrades from 0.0.104 and below straight to 0.2

We would now rely heavily on this statement being true: OutboundHTLCOutcome::Success always has a preimage filled in.

Right now if this is violated, a VLS will be missing a preimage.

Would assume no upgrades from 0.0.104 and below straight to 0.2

I believe we already require that for various things from some of wilmer's splicing work to, almost, the 0.1 "Nodes with pending forwarded HTLCs or unclaimed payments cannot be upgraded directly from 0.0.123 or earlier to 0.1"

We would now rely heavily on this statement being true: OutboundHTLCOutcome::Success always has a preimage filled in.

We should probably just enforce it in the deserialization step.

TheBlueMatt

Needs rebase, it seems

tankyleo · 2025-04-04T01:20:46Z

A question for you in an above comment Matt thanks :)

It can be useful to get the stats on a potential commitment transaction without actually building it. Therefore, this commit splits the stats calculations from the actual build of a commitment transaction. This introduces an extra loop over the pending htlcs when actually building a commitment transaction, but current network behavior produces very few concurrent htlcs on channels. Furthermore, each iteration of the loop in the stats calculation is very cheap. The motivating use case for `build_commitment_stats` is to calculate the balances of the channel parties in order to validate the `funding_contribution_satoshis` field of `splice_init` and `splice_ack` messages without building a full commitment transaction.

This allows us to DRY the code that calculates the `value_to_self_msat_offset` in `ChannelContext::build_commitment_stats`. HTLC success states have held their corresponding preimage since 0.0.105, and the release notes of 0.1 already require users running 0.0.123 and earlier to resolve their HTLCs before upgrading to 0.1. So this change is fully compatible with existing upgrade paths to the yet-to-be-shipped 0.2 release.

tankyleo · 2025-04-04T21:03:48Z

lightning/src/ln/channel.rs

+			OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage)) |
+			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage)) |
+			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage)) => {
+			debug_assert!(preimage.is_some());


I am not 100 sure if this debug assert and those below should be hard asserts. I went with debug because from what I can see so far, lacking preimages can lead to force closes, but not loss of funds.

ldk-reviews-bot · 2025-04-07T00:00:11Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot requested a review from TheBlueMatt March 24, 2025 23:14

tankyleo force-pushed the commitment-stats-less-duplication branch 2 times, most recently from 0af1b43 to 31300c3 Compare March 25, 2025 04:01

tankyleo marked this pull request as draft March 25, 2025 04:08

tankyleo force-pushed the commitment-stats-less-duplication branch from d6b1f51 to c540347 Compare March 25, 2025 19:52

tankyleo marked this pull request as ready for review March 25, 2025 19:59

wpaulino reviewed Mar 25, 2025

View reviewed changes

tankyleo force-pushed the commitment-stats-less-duplication branch from c540347 to c9b30cb Compare March 26, 2025 00:06

tankyleo requested a review from wpaulino March 26, 2025 03:12

tankyleo force-pushed the commitment-stats-less-duplication branch 3 times, most recently from e637050 to 4d6bf13 Compare March 26, 2025 17:40

wpaulino reviewed Mar 26, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

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

tankyleo force-pushed the commitment-stats-less-duplication branch 3 times, most recently from 5a212d8 to d682615 Compare March 27, 2025 15:24

tankyleo force-pushed the commitment-stats-less-duplication branch from d682615 to 6f80f99 Compare March 27, 2025 16:14

TheBlueMatt reviewed Mar 28, 2025

View reviewed changes

tankyleo mentioned this pull request Mar 29, 2025

Remove exclusive reference and generic from CommitmentTransaction API #3689

Draft

tankyleo force-pushed the commitment-stats-less-duplication branch 2 times, most recently from a6c6eac to 71df614 Compare April 1, 2025 17:47

tankyleo requested a review from TheBlueMatt April 1, 2025 17:48

TheBlueMatt reviewed Apr 2, 2025

View reviewed changes

tankyleo force-pushed the commitment-stats-less-duplication branch from 71df614 to 6d8fb6e Compare April 2, 2025 21:23

tankyleo requested review from TheBlueMatt and wpaulino April 2, 2025 21:28

wpaulino reviewed Apr 2, 2025

View reviewed changes

TheBlueMatt reviewed Apr 3, 2025

View reviewed changes

tankyleo force-pushed the commitment-stats-less-duplication branch from 6d8fb6e to 793d4d2 Compare April 3, 2025 18:58

tankyleo requested a review from TheBlueMatt April 3, 2025 22:18

TheBlueMatt removed their request for review April 4, 2025 12:38

tankyleo added 4 commits April 4, 2025 20:42

Rename CommitmentTransaction::htlcs to nondust_htlcs

14f8107

Remove unnecessary clones of the HTLC-HTLCSource table

ea8533f

tankyleo force-pushed the commitment-stats-less-duplication branch from 0c7bab6 to 285f1b9 Compare April 4, 2025 20:57

tankyleo requested a review from wpaulino April 4, 2025 21:02

tankyleo commented Apr 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ChannelContext::get_commitment_stats` #3682

Add `ChannelContext::get_commitment_stats` #3682

tankyleo commented Mar 24, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 24, 2025 •

edited

Loading

TheBlueMatt commented Mar 25, 2025

tankyleo commented Mar 25, 2025

tankyleo commented Mar 25, 2025

tankyleo commented Mar 25, 2025

codecov bot commented Mar 27, 2025 •

edited

Loading

tankyleo commented Mar 27, 2025

TheBlueMatt Mar 28, 2025

tankyleo Mar 28, 2025

tankyleo Mar 31, 2025

tankyleo Apr 1, 2025

TheBlueMatt Mar 28, 2025

tankyleo Mar 28, 2025

tankyleo Apr 1, 2025

TheBlueMatt left a comment

TheBlueMatt Apr 2, 2025

tankyleo Apr 2, 2025

wpaulino Apr 2, 2025

tankyleo Apr 3, 2025

wpaulino Apr 2, 2025

wpaulino Apr 2, 2025

tankyleo Apr 3, 2025

tankyleo Apr 3, 2025

TheBlueMatt Apr 4, 2025

TheBlueMatt left a comment

tankyleo commented Apr 4, 2025

tankyleo Apr 4, 2025

ldk-reviews-bot commented Apr 7, 2025

Add ChannelContext::get_commitment_stats #3682

Are you sure you want to change the base?

Add ChannelContext::get_commitment_stats #3682

Conversation

tankyleo commented Mar 24, 2025 • edited Loading

ldk-reviews-bot commented Mar 24, 2025 • edited Loading

TheBlueMatt commented Mar 25, 2025

tankyleo commented Mar 25, 2025

tankyleo commented Mar 25, 2025

tankyleo commented Mar 25, 2025

codecov bot commented Mar 27, 2025 • edited Loading

Codecov Report

tankyleo commented Mar 27, 2025

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

tankyleo commented Apr 4, 2025

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 7, 2025

Add `ChannelContext::get_commitment_stats` #3682

Add `ChannelContext::get_commitment_stats` #3682

tankyleo commented Mar 24, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 24, 2025 •

edited

Loading

codecov bot commented Mar 27, 2025 •

edited

Loading