Separate auxiliary HTLC data from holder commitment transaction #3774
Conversation
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.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
| struct HolderCommitment { | ||
| tx: HolderCommitmentTransaction, | ||
| #[derive(Clone, Default, PartialEq)] | ||
| 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.
| @@ -1612,6 +1640,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
| balances_empty_height: None, | |||
|
|
|||
| failed_back_htlc_ids: new_hash_set(), | |||
|
|
|||
| current_holder_htlc_data: HolderCommitmentHTLCData::default(), | |||
There was a problem hiding this comment.
nit: I really hate default for stuff like this. Its much less clear to the reader compared to something like empty.
| @@ -938,15 +940,15 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment | |||
|
|
|||
| let mut missing_nondust_source = false; | |||
| let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.nondust_htlcs().len()); | |||
| let dust_htlcs = holder_signed_tx.htlc_outputs.into_iter().filter_map(|(htlc, _, source)| { | |||
| let dust_htlcs = holder_signed_tx.htlc_outputs.iter().filter_map(|(htlc, _, source)| { | |||
There was a problem hiding this comment.
This and the extra clones aren't necessary - we can pass HolderSignedTx by value and skip them in the deserialization logic.
|
👋 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3774 +/- ##
==========================================
- Coverage 89.15% 0 -89.16%
==========================================
Files 157 0 -157
Lines 124109 0 -124109
Branches 124109 0 -124109
==========================================
- Hits 110646 0 -110646
+ Misses 10788 0 -10788
+ Partials 2675 0 -2675 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.