Produce a single monitor update per commitment number

[tankyleo]

    Produce a single monitor update per commitment number
    
    Such updates will contain a single HTLC-source table for that
    commitment number, together with a vector of commitment transactions
    created at that commitment number. This vector will grow with the number
    of pending splices for a channel.
    
    The HTLC-source table produced by channel will stop storing information
    on which HTLCs were assigned which output index. Instead, this
    information will be stored in each commitment transaction in the
    monitor update.
    
    This commit deduplicates the `HTLCSource` collection for each commitment
    number, but `HTLCOutputData` would store information already stored in
    each `CommitmentTransaction` in a monitor update.
    
    This is a partial rework of b8a03cd. That commit has not been shipped in
    a release yet.

[ldk-reviews-bot]

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

[tankyleo]

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

[tankyleo]

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs                        
                         │                                                     
                         │                                                     
                         │                                                     
                         │                                                     
                         └►──────► TX Builder ─────────► Commitment Transcation

[TheBlueMatt]

Design makes sense to me.

[wpaulino]

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

[wpaulino]

Should be even, and the members below should be odd

[wpaulino]

Should be odd

[wpaulino]

Should be odd

[codecov]

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	6.89%	27 Missing ⚠️
lightning/src/ln/channel.rs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3738      +/-   ##
==========================================
+ Coverage   89.12%   89.19%   +0.06%     
==========================================
  Files         156      156              
  Lines      123514   123984     +470     
  Branches   123514   123984     +470     
==========================================
+ Hits       110086   110582     +496     
+ Misses      10749    10734      -15     
+ Partials     2679     2668      -11     

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

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

[tankyleo]

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

[TheBlueMatt]

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

[tankyleo]

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

[TheBlueMatt]

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.