Separate auxiliary HTLC data from holder commitment transaction#3774
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Basically LGTM, a few minor questions and one comment about unnecessary clones.
| struct HolderCommitmentHTLCData { | ||
| // These must be sorted in increasing output index order to match the expected order of the | ||
| // HTLCs in the `CommitmentTransaction`. | ||
| nondust_htlc_sources: Vec<HTLCSource>, |
There was a problem hiding this comment.
I know we want to go ahead and assume that dust/non-dust are universal concepts, but do we want to also assume that HTLC ordering is fixed across splices? I know it is in non-custom-commitments, but we'd be assuming it in custom commitments which is yet another footgun in the API there. Also not sure how big a difference it would be to avoid the assumption, presumably some nontrivial work. CC @tankyleo.
There was a problem hiding this comment.
Yes happy to dig into relaxing this ordering requirement on top of this commit so we have a better idea of how much work that would take.
There was a problem hiding this comment.
Also not sure how big a difference it would be to avoid the assumption, presumably some nontrivial work.
Yeah it would be a good amount of work that would delay splicing if we wanted to pursue it now.
Has there been any progress on this @tankyleo?
There was a problem hiding this comment.
@wpaulino This is the commit where I last stopped, clunky and ugly, passes the test suite.
There was a problem hiding this comment.
@TheBlueMatt At this point, I think 1) it would be non-trivial work to avoid this assumption 2) we have higher priority use-cases for custom commitments right now that do not need to relax this ordering.
So I think we can relax this ordering later. Let me know if you agree.
|
👋 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3774 +/- ##
==========================================
+ Coverage 89.75% 89.77% +0.02%
==========================================
Files 159 160 +1
Lines 128906 129114 +208
Branches 128906 129114 +208
==========================================
+ Hits 115697 115915 +218
+ Misses 10512 10499 -13
- Partials 2697 2700 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
looking good ! some nits and questions, will take another pass.
wpaulino
force-pushed
the
separate-holder-commitment-htlc-data
branch
from
7fefc6a to
ce146e7
Compare
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
LGTM ! Just got some nits, feel free to drop them.
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
We shouldn't track our `HTLCSource`s within our `HolderCommitmentTransaction`s duplicatively for each `FundingScope`. With splicing, we may have alternative holder commitment transactions, but they must all have the same set of non-dust and dust HTLCs as the pre-spliced commitment transaction. Different sets of HTLCs are only possible with a change to the dust limit on commitment transactions, which the splicing protocol does not currently support. This commit moves the `nondust_htlc_sources` and `dust_htlcs` fields out from each `FundingScope` into the `ChannelMonitor`, such that they can be reused for each `FundingScope`. This remains as a backwards compatible change, the underlying stored data is not changed, but where it lives in memory is.
wpaulino
force-pushed
the
separate-holder-commitment-htlc-data
branch
from
ce146e7 to
2f431d6
Compare
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
4 participants
We shouldn't track our
HTLCSources within ourHolderCommitmentTransactions duplicatively for eachFundingScope. With splicing, we may have alternative holder commitment transactions, but they must all have the same set of non-dust and dust HTLCs as the pre-spliced commitment transaction. Different sets of HTLCs are only possible with a change to the dust limit on commitment transactions, which the splicing protocol does not currently support.This commit moves the
nondust_htlc_sourcesanddust_htlcsfields out from eachFundingScopeinto theChannelMonitor, such that they can be reused for eachFundingScope. This remains as a backwards compatible change, the underlying stored data is not changed, but where it lives in memory is.