Revert separate non-dust HTLC sources for holder commitments
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.
👋 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 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 69 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| lightning/src/ln/channel.rs | 81.81% | 2 Missing :warning: |
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
🔔 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.
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?
So given we're gonna do https://github.com/lightningdevkit/rust-lightning/pull/3738, is the point of this change to have less diff across the two update types?
Correct.
In https://github.com/lightningdevkit/rust-lightning/pull/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-https://github.com/lightningdevkit/rust-lightning/pull/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.
👋 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.
Closing in favor of #3774.