rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

Revert separate non-dust HTLC sources for holder commitments

Open wpaulino opened this issue 8 months ago • 6 comments

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.

wpaulino avatar Apr 17 '25 22:04 wpaulino

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

ldk-reviews-bot avatar Apr 17 '25 22:04 ldk-reviews-bot

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.

codecov[bot] avatar Apr 17 '25 22:04 codecov[bot]

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

ldk-reviews-bot avatar Apr 21 '25 00:04 ldk-reviews-bot

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?

TheBlueMatt avatar Apr 21 '25 19:04 TheBlueMatt

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.

wpaulino avatar Apr 21 '25 19:04 wpaulino

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

ldk-reviews-bot avatar Apr 22 '25 00:04 ldk-reviews-bot

Closing in favor of #3774.

wpaulino avatar May 12 '25 16:05 wpaulino