Uh oh!
There was an error while loading. Please reload this page.
Revert separate non-dust HTLC sources for holder commitments - #3745
Revert separate non-dust HTLC sources for holder commitments#3745wpaulino wants to merge 1 commit into
Conversation
👋 Thanks for assigning @tankyleo as a reviewer! |
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
ldk-reviews-bot
commented
Apr 21, 2025
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
TheBlueMatt
commented
Apr 21, 2025
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 |
wpaulino
commented
Apr 21, 2025
Correct.
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 So this change keeps both the legacy |
| commitment_tx: holder_commitment_tx, | ||
| htlc_outputs: dust_htlcs, | ||
| nondust_htlc_sources, | ||
| htlc_outputs, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oops yeah, I had misread how this worked. We do still need to track the signatures redundantly for backwards compat.
| // 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>)>, |
There was a problem hiding this comment.
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");
}There was a problem hiding this comment.
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
commented
Apr 22, 2025
👋 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
left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
| (0, commitment_tx, required), | ||
| (1, claimed_htlcs, optional_vec), | ||
| (2, htlc_outputs, required_vec), | ||
| (4, nondust_htlc_sources, optional_vec), |
There was a problem hiding this comment.
Double checking: we can remove this even field because we have never written it.
There was a problem hiding this comment.
Correct since we never released a version with it written
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.
6dad376 to
4d0eca1Comparetankyleo
left a comment
There was a problem hiding this comment.
I'm nearly onboard, some more nits / questions
| 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); | ||
| } |
There was a problem hiding this comment.
I would have kept these debug statements around - would you prefer we remove them ?
| return Err(()); | ||
| } | ||
| Ok(Self { |
There was a problem hiding this comment.
Want to add these debug checks above this line ?
| 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{ |
| (0, commitment_tx, required), | ||
| (1, claimed_htlcs, optional_vec), | ||
| (2, htlc_outputs, required_vec), | ||
| (4, nondust_htlc_sources, optional_vec), |
| counterparty_htlc_sig = Some(msg.htlc_signatures[idx]); | ||
| } | ||
| debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere"); | ||
| htlc_outputs.push((htlc, counterparty_htlc_sig, source_opt.cloned())); |
There was a problem hiding this comment.
A comment like this above this line:
// For backwards compatibility, set the signature of non-dust HTLCs here
| // HTLCs in the `CommitmentTransaction`. | ||
| nondust_htlc_sources: Vec<HTLCSource>, | ||
| dust_htlcs: Vec<(HTLCOutputInCommitment, Option<HTLCSource>)>, | ||
| htlcs: Vec<(HTLCOutputInCommitment, Option<HTLCSource>)>, |
There was a problem hiding this comment.
// 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
left a comment
There was a problem hiding this comment.
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?
wpaulino
commented
May 12, 2025
Closing in favor of #3774. |
We previously provided non-dust HTLC sources to avoid storing duplicate non-dust HTLC data in the
htlc_outputsVecwhere 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 relevantFundingScope. 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 duplicateHTLCSources.Along the way, this commit also omits setting the
Option<Signature>for non-dust HTLCs, as they are already tracked within theHolderCommitmentTransaction.