Skip to content

Conversation

@tankyleo
Copy link
Contributor

    Place htlc index data in funding scopes, and the rest in channel monitor
    
    This commit introduces a per-commitment id for each HTLC, which allows
    channel monitor to map the HTLC indices in a funding scope to all the
    HTLCs it knows about for a particular commitment.
    
    This is an arbitrary identifier that has no relationship with the
    `id` field from `update_add_htlc`.
    
    For now, this per-commitment id is set by monitor, as the `htlc_outputs`
    field of `LatestCounterpartyCommitmentTXInfo` holds information on
    which HTLCs were assigned which index, if any.
    
    Splicing and custom transactions will replace
    `LatestCounterpartyCommitmentTXInfo` with
    `LatestCounterpartyCommitmentTXS`. When the switch is made, the
    per-commitment id of each HTLC for a commitment will be set by channel.
    Channel will give these ids to transactions builders, and these will in
    turn place the non-dust HTLC index information in each
    `CommitmentTransaction`.
    
    This commit makes no changes to the serialization of channel monitor,
    we leave this for a follow-up.

This commit introduces a per-commitment id for each HTLC, which allows
channel monitor to map the HTLC indices in a funding scope to all the
HTLCs it knows about for a particular commitment.

This is an arbitrary identifier that has no relationship with the
`id` field from `update_add_htlc`.

For now, this per-commitment id is set by monitor, as the `htlc_outputs`
field of `LatestCounterpartyCommitmentTXInfo` holds information on
which HTLCs were assigned which index, if any.

Splicing and custom transactions will replace
`LatestCounterpartyCommitmentTXInfo` with
`LatestCounterpartyCommitmentTXS`. When the switch is made, the
per-commitment id of each HTLC for a commitment will be set by channel.
Channel will give these ids to transactions builders, and these will in
turn place the non-dust HTLC index information in each
`CommitmentTransaction`.

This commit makes no changes to the serialization of channel monitor,
we leave this for a follow-up.
@ldk-reviews-bot
Copy link

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

/// The set of outpoints in each counterparty commitment transaction. We always need at least
/// the payment hash from `HTLCOutputInCommitment` to claim even a revoked commitment
/// transaction broadcast as we need to be able to construct the witness script in all cases.
counterparty_htlc_data: HashMap<Txid, Vec<(HTLCOutputData, Option<Box<HTLCSource>>)>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to index this by the per commitment number now, or is that what you meant by follow-up work? This will still result in duplicate HTLC data/sources when there are multiple funding scopes present.

As we clean this up, I wonder if we should go even further, by tracking the HTLC data in a separate map that is indexed by HTLC id. This would prevent us from storing duplicate sources for HTLCs that exist in more than one commitment. counterparty_htlc_data could then just track commitment numbers that map to the set of HTLC IDs found within the commitment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to index this by the per commitment number now, or is that what you meant by follow-up work? This will still result in duplicate HTLC data/sources when there are multiple funding scopes present.

I was thinking of picking a single txid for each commitment to use in this map, and then each funding scope remembers the key it should use in that map for each commitment. Could be the same as its specific txid for that commitment, but could be different.

It's awkward I agree, would be nice to do this by commitment number.

I tried indexing this map by the commitment number, but I am not sure how I would index current entries in this map into a new map keyed by their commitment number.

To fix this I have some experimental (crazy?) commits with two data structures online, as we would only be interested in the "legacy" data structure in case of a cheat - ie practically never ?

As we clean this up, I wonder if we should go even further, by tracking the HTLC data in a separate map that is indexed by HTLC id. This would prevent us from storing duplicate sources for HTLCs that exist in more than one commitment. counterparty_htlc_data could then just track commitment numbers that map to the set of HTLC IDs found within the commitment.

I like this! We'd need to come up with something for existing entries - they've got no ID, no commitment number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or is that what you meant by follow-up work?

By follow-up, among other things, I mean that the serialization of channel monitor will need to be upgraded for whatever new data structures we come up with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More precisely for the current design: the serialization would have to be upgraded to persist the ID-index map in each funding scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried indexing this map by the commitment number, but I am not sure how I would index current entries in this map into a new map keyed by their commitment number.

Yeah I don't think we can migrate them. We should add new data structures to track the new data format and we'll just have to deal with a fallback to the legacy format for the older commitments for the time being. After a release or two, we can stop writing the legacy format and just rely on the new one.

@tankyleo
Copy link
Contributor Author

Closed after offline discussion today - we focus on trimming down the updates instead.

@tankyleo tankyleo closed this Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants