Remove exclusive reference and generic from `CommitmentTransaction` API #3689

tankyleo · 2025-03-29T00:49:22Z

    Remove exclusive reference and generic from `CommitmentTransaction` API
    
    This commit reworks how channel is told about which HTLCs were assigned
    which index upon the build of a `CommitmentTransaction`.
    
    Previously, `CommitmentTransaction` was given an exclusive reference to
    channel's HTLC-source table, and `CommitmentTransaction` populated the
    transaction output indices in that table via this exclusive reference.
    
    As a result, the public API of `CommitmentTransaction` included a
    generic parameter, and an exclusive reference.
    
    We remove both of these in preparation for the upcoming `TxBuilder`
    trait. This cleans up the API, and makes it more bindings-friendly.
    
    Henceforth, channel populates the HTLC-source table via a brute-force
    search of each htlc in `CommitmentTransaction`. This is an O(n^2)
    operation, but n is small enough that we ignore the performance hit.
    
    We also take this opportunity to cleanup how we build and sort the
    commitment transaction outputs together with the htlc output data
    in `CommitmentTransaction`. The goal is to keep the number of vector
    allocations to a minimum; we now only allocate a single vector to hold
    the transaction outputs, and do all the sorting in-place.

ldk-reviews-bot · 2025-03-29T00:49:24Z

👋 Thanks for assigning @TheBlueMatt 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.

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

codecov · 2025-04-10T23:26:43Z

Codecov Report

Attention: Patch coverage is 99.15254% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (ef0fcab) to head (75f81b5).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	98.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3689      +/-   ##
==========================================
+ Coverage   89.07%   89.21%   +0.13%     
==========================================
  Files         156      156              
  Lines      123507   124130     +623     
  Branches   123507   124130     +623     
==========================================
+ Hits       110017   110742     +725     
+ Misses      10801    10724      -77     
+ Partials     2689     2664      -25

☔ 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.

TheBlueMatt

One last real comment

lightning/src/ln/chan_utils.rs

ldk-reviews-bot · 2025-04-14T00:00:55Z

🔔 1st Reminder

Hey @TheBlueMatt @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 · 2025-04-14T00:00:56Z

🔔 1st Reminder

Hey @TheBlueMatt @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.

tankyleo · 2025-04-14T02:08:55Z

@TheBlueMatt In this last commit here I delete the txout_htlc_pairs allocation let me know if that approach is promising.

TheBlueMatt

New commit basically LTGM.

lightning/src/ln/chan_utils.rs

TheBlueMatt

Basically LGTM, Probably needs another look from @wpaulino otherwise I'm happy.

TheBlueMatt · 2025-04-14T20:11:25Z

lightning/src/ln/chan_utils.rs

 	///
-	/// This is not exported to bindings users due to the generic though we likely should expose a version without
-	pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<secp256k1::All>) -> CommitmentTransaction {
+	/// All HTLCs MUST be above the dust limit for the channel.\


stray , I think? note that if you dont leave an extra line between paragraphs, rustdoc assumes you meant for them to be in the same paragraph and ignores newlines in the source.

rustdoc actually turns \ into a single newline character.

Heh, TIL, but it also makes the source unreadable :)

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

lightning/src/ln/chan_utils.rs

This commit reworks how channel is told about which HTLCs were assigned which index upon the build of a `CommitmentTransaction`. Previously, `CommitmentTransaction` was given an exclusive reference to channel's HTLC-source table, and `CommitmentTransaction` populated the transaction output indices in that table via this exclusive reference. As a result, the public API of `CommitmentTransaction` included a generic parameter, and an exclusive reference. We remove both of these in preparation for the upcoming `TxBuilder` trait. This cleans up the API, and makes it more bindings-friendly. Henceforth, channel populates the HTLC-source table via a brute-force search of each htlc in `CommitmentTransaction`. This is an O(n^2) operation, but n is small enough that we ignore the performance hit. We also take this opportunity to cleanup how we build and sort the commitment transaction outputs together with the htlc output data in `CommitmentTransaction`. The goal is to keep the number of vector allocations to a minimum; we now only allocate a single vector to hold the transaction outputs, and do all the sorting in-place.

TheBlueMatt

Probably worth adding the sort in the rebuild case, but gonna go ahead and land it.

TheBlueMatt · 2025-04-16T01:02:38Z

lightning/src/ln/chan_utils.rs

+		let (obscured_commitment_transaction_number, txins) = Self::build_inputs(self.commitment_number, channel_parameters);
+
+		// First rebuild the htlc outputs, note that these are already sorted
+		let mut outputs = Self::build_htlc_outputs(keys, &self.nondust_htlcs, channel_parameters.channel_type_features());


It would be nice to resort the HTLCs here before we return. The point of rebuild_transaction is to avoid trusting the pre-built commitment transaction entirely and build it from the HTLC list.

Should I get this done now ?

Can we have fn verify take a &mut self ? To highlight that this call will resort the HTLCs if necessary. Then fn rebuild_transaction can call build_outputs_and_htlcs just like fn new without any new memory allocations.

Ah looks like this upsets the validation APIs of the signer that take a &CommitmentTransaction

Maybe we make it return a Result? Can happen in a followup but up to you if you want to do it now.

TheBlueMatt · 2025-04-16T01:08:10Z

Oops, sorry lol, didn't mean for the nit on docs to be blocking.

tankyleo · 2025-04-16T01:09:39Z

Ah all good I do agree with the aesthetics.

tankyleo force-pushed the small-n-squared branch from 1cc3c50 to fd57c35 Compare March 29, 2025 00:50

tankyleo changed the title ~~Remove generic and mutable references from commitment transaction API~~ Remove generic and mutable references from CommitmentTransaction API Mar 29, 2025

tankyleo force-pushed the small-n-squared branch from fd57c35 to ecc7dcf Compare March 29, 2025 00:58

tankyleo changed the title ~~Remove generic and mutable references from CommitmentTransaction API~~ Remove generic and exclusive references from CommitmentTransaction API Mar 29, 2025

tankyleo changed the title ~~Remove generic and exclusive references from CommitmentTransaction API~~ Remove generic and exclusive reference from CommitmentTransaction API Mar 29, 2025

tankyleo force-pushed the small-n-squared branch 2 times, most recently from c8a753e to b04cc76 Compare March 29, 2025 01:07

tankyleo changed the title ~~Remove generic and exclusive reference from CommitmentTransaction API~~ Remove exclusive reference and generic from CommitmentTransaction API Mar 29, 2025

tankyleo mentioned this pull request Mar 10, 2025

Custom Transactions Tracking Issue #3603

Open

20 tasks

tankyleo force-pushed the small-n-squared branch 4 times, most recently from f4a3cc3 to acad6d8 Compare April 8, 2025 18:34

tankyleo force-pushed the small-n-squared branch from acad6d8 to c86b528 Compare April 9, 2025 18:00

tankyleo marked this pull request as ready for review April 9, 2025 18:25

ldk-reviews-bot requested a review from wpaulino April 9, 2025 18:26

tankyleo commented Apr 9, 2025

View reviewed changes

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

wpaulino reviewed Apr 10, 2025

View reviewed changes

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

tankyleo force-pushed the small-n-squared branch from c86b528 to fcdacc9 Compare April 10, 2025 23:17

tankyleo requested review from wpaulino and TheBlueMatt April 10, 2025 23:31

TheBlueMatt reviewed Apr 11, 2025

View reviewed changes

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

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

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

tankyleo force-pushed the small-n-squared branch from fcdacc9 to 7e3a4e1 Compare April 11, 2025 21:17

tankyleo requested a review from TheBlueMatt April 11, 2025 21:23

tankyleo force-pushed the small-n-squared branch 4 times, most recently from 1d7a1d8 to 262351c Compare April 12, 2025 03:47

tankyleo force-pushed the small-n-squared branch from 262351c to 6ba5280 Compare April 12, 2025 04:06

tankyleo force-pushed the small-n-squared branch 3 times, most recently from f021734 to 1621c12 Compare April 14, 2025 06:16

TheBlueMatt reviewed Apr 14, 2025

View reviewed changes

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

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

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

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

tankyleo force-pushed the small-n-squared branch from 1621c12 to 258b277 Compare April 14, 2025 19:17

tankyleo requested a review from TheBlueMatt April 14, 2025 19:22

tankyleo force-pushed the small-n-squared branch from 258b277 to ee42e2a Compare April 14, 2025 20:11

TheBlueMatt reviewed Apr 14, 2025

View reviewed changes

tankyleo force-pushed the small-n-squared branch 2 times, most recently from 8bc75b9 to 5252437 Compare April 15, 2025 02:10

wpaulino reviewed Apr 15, 2025

View reviewed changes

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

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

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

tankyleo force-pushed the small-n-squared branch 2 times, most recently from b82a9d5 to b44f272 Compare April 15, 2025 18:45

tankyleo requested a review from wpaulino April 15, 2025 18:45

tankyleo force-pushed the small-n-squared branch 2 times, most recently from 2f7567d to 5e67a1c Compare April 15, 2025 19:01

wpaulino previously approved these changes Apr 15, 2025

View reviewed changes

tankyleo requested a review from TheBlueMatt April 15, 2025 21:08

tankyleo dismissed wpaulino’s stale review via 75f81b5 April 16, 2025 00:47

tankyleo force-pushed the small-n-squared branch from 5e67a1c to 75f81b5 Compare April 16, 2025 00:47

TheBlueMatt approved these changes Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove exclusive reference and generic from `CommitmentTransaction` API #3689

Remove exclusive reference and generic from `CommitmentTransaction` API #3689

tankyleo commented Mar 29, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 29, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading

TheBlueMatt left a comment

ldk-reviews-bot commented Apr 14, 2025

ldk-reviews-bot commented Apr 14, 2025

tankyleo commented Apr 14, 2025 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt left a comment

TheBlueMatt Apr 14, 2025

tankyleo Apr 14, 2025

TheBlueMatt Apr 16, 2025

TheBlueMatt left a comment

TheBlueMatt Apr 16, 2025

tankyleo Apr 16, 2025

tankyleo Apr 16, 2025

tankyleo Apr 16, 2025 •

edited

Loading

TheBlueMatt Apr 16, 2025

TheBlueMatt commented Apr 16, 2025

tankyleo commented Apr 16, 2025

Remove exclusive reference and generic from CommitmentTransaction API #3689

Are you sure you want to change the base?

Remove exclusive reference and generic from CommitmentTransaction API #3689

Conversation

tankyleo commented Mar 29, 2025 • edited Loading

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

codecov bot commented Apr 10, 2025 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 14, 2025

ldk-reviews-bot commented Apr 14, 2025

tankyleo commented Apr 14, 2025 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Apr 14, 2025

Choose a reason for hiding this comment

tankyleo Apr 14, 2025

Choose a reason for hiding this comment

TheBlueMatt Apr 16, 2025

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Apr 16, 2025

Choose a reason for hiding this comment

tankyleo Apr 16, 2025

Choose a reason for hiding this comment

tankyleo Apr 16, 2025

Choose a reason for hiding this comment

tankyleo Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt Apr 16, 2025

Choose a reason for hiding this comment

TheBlueMatt commented Apr 16, 2025

tankyleo commented Apr 16, 2025

Remove exclusive reference and generic from `CommitmentTransaction` API #3689

Remove exclusive reference and generic from `CommitmentTransaction` API #3689

tankyleo commented Mar 29, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 29, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading

tankyleo commented Apr 14, 2025 •

edited

Loading

tankyleo Apr 16, 2025 •

edited

Loading