Revert separate non-dust HTLC sources for holder commitments #3745

wpaulino · 2025-04-17T22:25:42Z

We previously provided non-dust HTLC sources to avoid storing duplicate non-dust HTLC data in the htlc_outputs Vec where all HTLCs would be tracked in a holder commitment update. With splicing, we'll unfortunately be forced to store redundant copies of non-dust HTLC data within the commitment transaction for each relevant FundingScope. As a result, providing non-dust HTLC sources separately no longer provides any benefits. In the future, we also plan to rework how the HTLC data for holder and counterparty commitments are tracked to avoid storing duplicate HTLCSources.

Along the way, this commit also omits setting the Option<Signature> for non-dust HTLCs, as they are already tracked within the HolderCommitmentTransaction.

ldk-reviews-bot · 2025-04-17T22:25:45Z

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

codecov · 2025-04-17T22:35:27Z

Codecov Report

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

Project coverage is 89.11%. Comparing base (c6921fa) to head (4d0eca1).
Report is 29 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3745      +/-   ##
==========================================
- Coverage   89.15%   89.11%   -0.04%     
==========================================
  Files         156      157       +1     
  Lines      123837   123923      +86     
  Branches   123837   123923      +86     
==========================================
+ Hits       110408   110440      +32     
- Misses      10754    10806      +52     
- Partials     2675     2677       +2

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

ldk-reviews-bot · 2025-04-21T00:00:39Z

🔔 1st Reminder

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

TheBlueMatt · 2025-04-21T19:26:41Z

Sorry for the delay here. So given we're gonna do #3738, is the point of this change to have less diff across the two update types? In #3738 it was proposed that we finally split HTLCOutputInCommitment so that we don't deal with the spurious output index field anymore as well as avoids the redundant signature field. The PR details on this one explain that this is about splicing, but post-#3738 it wouldn't be used for splicing, and its not clear to me that this simplifies the code (eg back to the 0.1 state) much?

wpaulino · 2025-04-21T19:35:38Z

So given we're gonna do #3738, is the point of this change to have less diff across the two update types?

Correct.

In #3738 it was proposed that we finally split HTLCOutputInCommitment so that we don't deal with the spurious output index field anymore as well as avoids the redundant signature field. The PR details on this one explain that this is about splicing, but post-#3738 it wouldn't be used for splicing, and its not clear to me that this simplifies the code (eg back to the 0.1 state) much?

This change isn't about splicing specifically, it just notes that the goal of having separate non-dust HTLC sources is unnecessary because splicing monitor updates will have duplicate HTLC data tracked in each CommitmentTransaction anyway.

So this change keeps both the legacy LatestHolderCommitmentTXInfo and the new LatestHolderCommitmentTX closer together, as we'll eventually replace the latter with the former.

TheBlueMatt · 2025-04-22T00:08:09Z

lightning/src/ln/channel.rs

@@ -3845,8 +3835,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {

 		Ok(LatestHolderCommitmentTXInfo {
 			commitment_tx: holder_commitment_tx,
-			htlc_outputs: dust_htlcs,
-			nondust_htlc_sources,
+			htlc_outputs,


Does LDK 0.1 support handling the case with htlc_sources containing nondust HTLC sources with None for the signature? Or does it get confused cause it only expects either a separate nondust_htlc_sources or Some for some of the signatures?

Oops yeah, I had misread how this worked. We do still need to track the signatures redundantly for backwards compat.

TheBlueMatt · 2025-04-22T00:17:44Z

lightning/src/chain/channelmonitor.rs

-		/// `Option<Signature>` for backwards compatibility.
+		// Includes both dust and non-dust HTLCs. The `Option<Signature>` is always `None`, as they
+		// are already tracked within the `HolderCommitmentTransaction` above. We still have to
+		// track it for backwards compatibility though.
 		htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,


Is it worth just doing the sigs at write-time?

$ git diff diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a1acfd7df..687ebc61d 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -571,7 +571,7 @@ pub(crate) enum ChannelMonitorUpdateStep { // Includes both dust and non-dust HTLCs. The `Option<Signature>` is always `None`, as they // are already tracked within the `HolderCommitmentTransaction` above. We still have to // track it for backwards compatibility though. - htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, + htlc_outputs: Vec<(HTLCOutputInCommitment, Option<HTLCSource>)>, claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>, }, LatestCounterpartyCommitmentTXInfo { @@ -629,7 +629,12 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (0, LatestHolderCommitmentTXInfo) => { (0, commitment_tx, required), (1, claimed_htlcs, optional_vec), - (2, htlc_outputs, required_vec), + (2, legacy_htlc_outputs, (legacy, crate::util::ser::WithoutLength<Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>>, |us| { + if let &Self::LatestHolderCommitmentTXInfo { htlc_outputs, .. } = us { + Some(crate::util::ser::IterableOwned(htlc_outputs.iter().map(|(a, b)| (a, None::<Signature>, b)))) + } else { unreachable!() } + })), + (99999999, htlc_outputs, (static_value, legacy_htlc_outputs.ok_or(DecodeError::InvalidValue)?.0.into_iter().map(|(a, _, b)| (a, b)).collect())) }, (1, LatestCounterpartyCommitmentTXInfo) => { (0, commitment_txid, required), diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index c3cf2044d..a2727e2ba 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -56,7 +56,7 @@ macro_rules! _encode_tlv { if let Some(v) = &value { let encoded_value = v.encode(); let mut read_slice = &encoded_value[..]; - let _: $fieldty = $crate::util::ser::Readable::read(&mut read_slice) + let _: $fieldty = $crate::util::ser::LengthReadable::read_from_fixed_length_buffer(&mut read_slice) .expect("Failed to read written TLV, check types"); assert!(read_slice.is_empty(), "Reading written TLV was short, check types"); }

Don't think we can do this anymore without unrolling the macro, since we'd need to write it with the sigs, but do the legacy thing for reads.

ldk-reviews-bot · 2025-04-22T00:17:54Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

tankyleo

I also have Matt's question about potential downgrades to 0.1 from here.

If none of the HTLC signatures are set, 0.1 assumes that the sources are provided separately by nondust_htlc_sources. But this is a wrong assumption to make after this commit.

lightning/src/chain/channelmonitor.rs

tankyleo · 2025-04-28T22:47:14Z

lightning/src/chain/channelmonitor.rs

@@ -638,7 +630,6 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
 		(0, commitment_tx, required),
 		(1, claimed_htlcs, optional_vec),
 		(2, htlc_outputs, required_vec),
-		(4, nondust_htlc_sources, optional_vec),


Double checking: we can remove this even field because we have never written it.

Correct since we never released a version with it written

Thanks feel free to resolve

We previously provided non-dust HTLC sources to avoid storing duplicate non-dust HTLC data in the `htlc_outputs` `Vec` where all HTLCs would be tracked in a holder commitment update. With splicing, we'll unfortunately be forced to store redundant copies of non-dust HTLC data within the commitment transaction for each relevant `FundingScope`. As a result, providing non-dust HTLC sources separately no longer provides any benefits. In the future, we also plan to rework how the HTLC data for holder and counterparty commitments are tracked to avoid storing duplicate `HTLCSource`s.

tankyleo

I'm nearly onboard, some more nits / questions

tankyleo · 2025-04-29T21:11:42Z

lightning/src/chain/channelmonitor.rs

-			debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.trust().nondust_htlcs().len());
-			for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().nondust_htlcs().iter()) {
-				debug_assert_eq!(a, b);
-			}
-			debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.counterparty_htlc_sigs.len());
-			for (a, b) in htlc_outputs.iter().filter_map(|(_, s, _)| s.as_ref()).zip(holder_commitment_tx.counterparty_htlc_sigs.iter()) {
-				debug_assert_eq!(a, b);
-			}


I would have kept these debug statements around - would you prefer we remove them ?

tankyleo · 2025-04-29T21:57:51Z

lightning/src/chain/channelmonitor.rs

-		if missing_nondust_source {
-			return Err(());
-		}
-
 		Ok(Self {


Want to add these debug checks above this line ?

Suggested change

Ok(Self {

debug_assert!(

holder_commitment_tx.nondust_htlcs().iter().zip(holder_signed_tx.htlc_outputs.iter().map(|(htlc, _, _)| htlc))

.all(|(htlc_a, htlc_b)| htlc_a == htlc_b)

);

debug_assert!(

holder_commitment_tx.counterparty_htlc_sigs.iter().zip(holder_signed_tx.htlc_outputs.iter().map(|(_, sig, _)| sig.as_ref().unwrap()))

.all(|(sig_a, sig_b)| sig_a == sig_b)

);

Ok(Self {

tankyleo · 2025-04-29T21:58:33Z

lightning/src/chain/channelmonitor.rs

@@ -638,7 +630,6 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
 		(0, commitment_tx, required),
 		(1, claimed_htlcs, optional_vec),
 		(2, htlc_outputs, required_vec),
-		(4, nondust_htlc_sources, optional_vec),


Thanks feel free to resolve

tankyleo · 2025-04-29T22:18:19Z

lightning/src/ln/channel.rs

 			}
-			debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
+			htlc_outputs.push((htlc, counterparty_htlc_sig, source_opt.cloned()));


A comment like this above this line:

// For backwards compatibility, set the signature of non-dust HTLCs here

tankyleo · 2025-04-29T22:38:04Z

lightning/src/chain/channelmonitor.rs

-	// HTLCs in the `CommitmentTransaction`.
-	nondust_htlc_sources: Vec<HTLCSource>,
-	dust_htlcs: Vec<(HTLCOutputInCommitment, Option<HTLCSource>)>,
+	htlcs: Vec<(HTLCOutputInCommitment, Option<HTLCSource>)>,


// Non-dust `HTLCOutputInCommitment`'s are stored in both `tx`, and `htlcs`; we accept this tradeoff (since it allows us to keep the HTLC-source pairs intact / things will make more sense in splicing)

TheBlueMatt

Hmm, I guess I'm not really clear on if we need to do this now? Seems like it kinda sucks to go back to duplicating the signatures in the HTLC output list and and how much code does it actually clean up to map htlc_outputs from a 3-tuple to a pair vs joining the dust and non-dust outputs?

ldk-reviews-bot requested a review from joostjager April 17, 2025 22:35

wpaulino requested review from TheBlueMatt and removed request for joostjager April 18, 2025 02:18

TheBlueMatt reviewed Apr 22, 2025

View reviewed changes

tankyleo reviewed Apr 28, 2025

View reviewed changes

wpaulino force-pushed the revert-separate-nondusts-htlc-sources branch from 6dad376 to 4d0eca1 Compare April 29, 2025 18:41

wpaulino requested review from TheBlueMatt and tankyleo April 29, 2025 18:42

tankyleo reviewed Apr 29, 2025

View reviewed changes

TheBlueMatt reviewed Apr 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert separate non-dust HTLC sources for holder commitments #3745

Revert separate non-dust HTLC sources for holder commitments #3745

wpaulino commented Apr 17, 2025

ldk-reviews-bot commented Apr 17, 2025 •

edited

Loading

codecov bot commented Apr 17, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 21, 2025

TheBlueMatt commented Apr 21, 2025

wpaulino commented Apr 21, 2025

TheBlueMatt Apr 22, 2025

wpaulino Apr 29, 2025

TheBlueMatt Apr 22, 2025

wpaulino Apr 29, 2025

ldk-reviews-bot commented Apr 22, 2025

tankyleo left a comment

tankyleo Apr 28, 2025

wpaulino Apr 29, 2025

tankyleo Apr 29, 2025

tankyleo left a comment

tankyleo Apr 29, 2025

tankyleo Apr 29, 2025

tankyleo Apr 29, 2025

tankyleo Apr 29, 2025

tankyleo Apr 29, 2025

TheBlueMatt left a comment

Revert separate non-dust HTLC sources for holder commitments #3745

Are you sure you want to change the base?

Revert separate non-dust HTLC sources for holder commitments #3745

Conversation

wpaulino commented Apr 17, 2025

ldk-reviews-bot commented Apr 17, 2025 • edited Loading

codecov bot commented Apr 17, 2025 • edited Loading

Codecov Report

ldk-reviews-bot commented Apr 21, 2025

TheBlueMatt commented Apr 21, 2025

wpaulino commented Apr 21, 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

ldk-reviews-bot commented Apr 22, 2025

tankyleo 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

tankyleo 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

TheBlueMatt left a comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 17, 2025 •

edited

Loading

codecov bot commented Apr 17, 2025 •

edited

Loading