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.

tankyleo · 2025-04-09T18:29:27Z

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 {
+	/// The broadcaster and countersignatory amounts MUST be EITHER 0 or ABOVE DUST.
+	pub fn new(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, nondust_htlcs: &Vec<HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<secp256k1::All>) -> CommitmentTransaction {


@TheBlueMatt Let me know how this API looks - would bindings be happier if we took the nondust_htlcs by value instead of by reference ?

Passing it by value might make sense, given build_outputs_and_htlcs is basically cloneing it anyway?

But otherwise I think this makes sense.

Thank you I switched it to a pass-by-value below.

wpaulino · 2025-04-10T20:43:35Z

lightning/src/ln/channel.rs

+			let htlc = htlcs_included
+				.iter_mut()
+				.filter(|(htlc, _source)| htlc.transaction_output_index.is_none())
+				.filter_map(|(htlc, _source)| {
+					if htlc.is_data_equal(nondust_htlc) {
+						Some(htlc)
+					} else {
+						None
+					}
+				})
+				.nth(0)


Won't this always pick the first HTLC if we have multiple with the same data (but different indices of course)? This only works if we reuse the iterator, but we seem to be using a new one each iteration of the loop.

Never mind, missed that we're filtering for the first one without an output index assigned. We should be able to use find_map instead of filter_map though so we stop iterating once we get a match.

Afk now will look more closely - I was thinking nth 0 stops the iteration on the first match.

It does, TIL https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d810bda4b86bfc0f7012a2d1c52ff1de.

find is still more idiomatic in this case though.

Certainly thanks, should be addressed below.

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

Codecov Report

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

Project coverage is 89.09%. Comparing base (ef0fcab) to head (5252437).
Report is 14 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3689      +/-   ##
==========================================
+ Coverage   89.07%   89.09%   +0.02%     
==========================================
  Files         156      156              
  Lines      123507   123661     +154     
  Branches   123507   123661     +154     
==========================================
+ Hits       110017   110179     +162     
+ Misses      10801    10800       -1     
+ Partials     2689     2682       -7

☔ 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

TheBlueMatt · 2025-04-11T18:39:10Z

lightning/src/ln/chan_utils.rs

@@ -614,6 +614,13 @@ impl HTLCOutputInCommitment {
 	pub const fn to_bitcoin_amount(&self) -> Amount {
 		Amount::from_sat(self.amount_msat / 1000)
 	}
+
+	pub(crate) fn is_data_equal(&self, other: &HTLCOutputInCommitment) -> bool {


Might be worth a quick doc just to make obvious that the intent is to only ignore one field and why.

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2025-04-11T19:51:42Z

lightning/src/ln/chan_utils.rs

+	fn build_outputs_and_htlcs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: &Vec<HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters) -> (Vec<TxOut>, Vec<HTLCOutputInCommitment>) {
+		let txout_htlc_pairs: Vec<(TxOut, Option<&HTLCOutputInCommitment>)> = Self::build_txout_htlc_pairs(keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters);
+		let mut outputs: Vec<TxOut> = Vec::with_capacity(txout_htlc_pairs.len());
+		let mut nondust_htlcs: Vec<HTLCOutputInCommitment> = Vec::with_capacity(txout_htlc_pairs.len());


Now that we're passed nondust_htlcs by ownership, can we at least have build_txout_htlc_pairs hold references to the specific HTLCOutputInCommitments and then update the transaction_output_index by mutable reference (then sorting) rather than allocating a new nondust_htlcs vec?

I went with shared references to avoid a clone of self.nondust_htlcs in rebuild_transaction. But we could make build_txout_htlc_pairs generic over & and &mut with a T: Borrow<HTLCOutputInCommitment> ?

Thanks again - let me know how the latest commit sounds - I'd like to continue passing &self.nondust_htlcs to build_txout_htlc_pairs in rebuild_transaction.

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.

TheBlueMatt · 2025-04-14T13:52:01Z

lightning/src/ln/chan_utils.rs

 		let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat);
 		let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat);
 		let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);

-		// Sort outputs and populate output indices while keeping track of the auxiliary data
-		let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters);
+		let (outputs, nondust_htlcs) = Self::build_outputs_and_htlcs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters);


nit: No need to keep passing nondust_htlcs down by ownership and returning it, might read a bit cleaner if you just pass a mutable reference when we're looking at internal methods.

TheBlueMatt · 2025-04-14T14:03:29Z

lightning/src/ln/chan_utils.rs

+		// Sort outputs in BIP-69 order (amount, scriptPubkey).  Tie-breaks based on HTLC
+		// CLTV expiration height.
+		//
+		// CLN uses dumb sort... so we do too.


ha, "they do it" is never a good reason to do anything. We should explain that we're okay with N^2 for N=483*2

TheBlueMatt · 2025-04-14T14:04:34Z

lightning/src/ln/chan_utils.rs

+
+			// Initialize the transaction output index; we will update it when we
+			// add the non-htlc transaction outputs.
+			nondust_htlcs[i].transaction_output_index = Some(i as u32);


This can move out of the top loop and we can just do a pass at the end to set the output indicies to the index used as long as we're swapping both arrays together.

TheBlueMatt · 2025-04-14T14:09:22Z

lightning/src/ln/chan_utils.rs

+		//
+		// CLN uses dumb sort... so we do too.
+		for i in 0..txouts.len() {
+			// Find the minimum element


Oh come on, at least do some naive CS 101 sorting algorithm that has a better best-case.

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.

TheBlueMatt · 2025-04-14T20:23:20Z

lightning/src/ln/chan_utils.rs

 		let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat);
 		let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat);
 		let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);

-		// Sort outputs and populate output indices while keeping track of the auxiliary data
-		let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters);
+		// Build the BIP-69 ordered outputs of the transaction.


nit: we are not technically ordering the outputs per BIP 69, we are ordering it with additional metadata (the CLTV) and also BIP 69 is underspecified (even going so far as to mis-define lexicographical ordering!). Best to avoid mentioning it (or every implementing it).

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.

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

wpaulino reviewed Apr 10, 2025

View reviewed changes

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

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

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 from ee42e2a to 8bc75b9 Compare April 14, 2025 20:58

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

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

tankyleo Apr 9, 2025

TheBlueMatt Apr 10, 2025

TheBlueMatt Apr 10, 2025

tankyleo Apr 10, 2025

wpaulino Apr 10, 2025 •

edited

Loading

tankyleo Apr 10, 2025

wpaulino Apr 10, 2025 •

edited

Loading

tankyleo Apr 10, 2025

codecov bot commented Apr 10, 2025 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Apr 11, 2025

TheBlueMatt Apr 11, 2025

tankyleo Apr 11, 2025 •

edited

Loading

tankyleo Apr 11, 2025

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 Apr 14, 2025

TheBlueMatt Apr 14, 2025

TheBlueMatt Apr 14, 2025

TheBlueMatt Apr 14, 2025

TheBlueMatt left a comment

TheBlueMatt Apr 14, 2025

tankyleo Apr 14, 2025

TheBlueMatt Apr 14, 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

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

wpaulino Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 10, 2025 • edited Loading

Codecov Report

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

tankyleo Apr 11, 2025 • edited Loading

Choose a reason for hiding this 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

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

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

wpaulino Apr 10, 2025 •

edited

Loading

wpaulino Apr 10, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading

tankyleo Apr 11, 2025 •

edited

Loading

tankyleo commented Apr 14, 2025 •

edited

Loading