Separate auxiliary HTLC data from holder commitment transaction #3774
Conversation
|
👋 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.
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.
|
👋 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.74% -0.01%
==========================================
Files 159 159
Lines 128906 128909 +3
Branches 128906 128909 +3
==========================================
- Hits 115697 115695 -2
- Misses 10512 10516 +4
- Partials 2697 2698 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
7fefc6a to
ce146e7
Compare
| ) | ||
| }}; | ||
| ($self: expr, PREV) => {{ | ||
| if let Some(tx) = &$self.funding.prev_holder_commitment_tx { |
There was a problem hiding this comment.
nit: we could replace this if .. else and the one below with a map, your call.
| let mut sources = $htlc_data.nondust_htlc_sources.iter(); | ||
| let nondust_htlcs = $commitment_tx.nondust_htlcs().iter().map(move |htlc| { | ||
| let mut source = None; | ||
| if htlc.offered && htlc.transaction_output_index.is_some() { |
There was a problem hiding this comment.
nit: the transaction output index is always some here.
| type Error = (); | ||
| fn try_from(value: (HolderCommitmentTransaction, HolderSignedTx)) -> Result<Self, Self::Error> { | ||
| fn try_from(value: (&HolderCommitmentTransaction, HolderSignedTx)) -> Result<Self, Self::Error> { |
There was a problem hiding this comment.
nit: The &HolderCommitmentTransaction is only needed to set the capacity of nondust_htlc_sources further below. Would it be simpler if instead of the tuple we took only HolderSignedTx as a parameter here, and set the capacity of nondust_htlc_sources to holder_signed_tx.htlc_outputs.len() ?
On the other hand, I also agree we want to be extra conservative with memory allocations for HTLC sources.
| // The auxiliary HTLC data associated with a holder commitment transaction. This includes | ||
| // non-dust HTLC sources, along with dust HTLCs and their sources. Note that this assumes any | ||
| // alternative holder commitment transactions, like in the case of splicing, must maintain the | ||
| // same set of non-dust and dust HTLCs. |
There was a problem hiding this comment.
nit: Let's also make this note here ?
// We also assume here that while the indices of non-dust HTLCs might change across splices,
// their ordering with respect to each other remains the same.
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
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.