Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose htlc id to htlc_accepted hook #5303

Merged
merged 1 commit into from
Jun 18, 2022

Conversation

fiatjaf
Copy link
Contributor

@fiatjaf fiatjaf commented Jun 2, 2022

I believe this is necessary for low-level stuff that deals with MPP, for example, although I am not entirely sure.

@cdecker
Copy link
Member

cdecker commented Jun 5, 2022

Not quite sure what the use here would be, as pay already implements the MPP receiving logic without requiring this extra information. However, I'm also not opposed to including it as it can have use-cases (accounting making sure we didn't miss an HTLC for example?).

@cdecker cdecker self-requested a review June 5, 2022 13:39
@fiatjaf
Copy link
Contributor Author

fiatjaf commented Jun 5, 2022

If one is implementing a bridge between two networks they must be able to relay multiple HTLCs with the same payment hash, and because of that they should be able to know which one to fail/fulfill based on the id, not the payment hash.

Or I could have misunderstood everything.

@cdecker
Copy link
Member

cdecker commented Jun 6, 2022

I think you can track them yourself: each HTLC is a hook call. In the hook handler you can assign any unused partid for the HTLC you insert into the other node, it doesn't have to match the one in the incoming part at all. You then wait or poll with the partid you chose and then relay the result by returning from the hook handler.

Also come to think of it the HTLC id and the partid are inherently different concepts, one is a monotonic increasing counter for HTLCs on that channel and the other is a differentiator for the sender to know which part it is tracking in the context of a payment. Notice also that if you happen to forward HTLCs from two different incoming channels you may end up with HTLC id collisions which would be hard to debug.

@fiatjaf
Copy link
Contributor Author

fiatjaf commented Jun 6, 2022

But I don't get the partid as part of the htlc_accepted hook. partids are only visible to the sender and the receiver, no?

Imagine I get two htlc_accepted calls for the same hash, cltv expiry and amount from the same channel A to be relayed to the same channel B. The only way I can differentiate the two is by the JSONRPC message I got each, which would be fine except for the fact that if I go offline I'll receive these two htlc_accepted again and now I don't know which one is which.

Notice also that if you happen to forward HTLCs from two different incoming channels you may end up with HTLC id collisions which would be hard to debug.

Yes, but I can easily track them by [channel_id, htlc_id].

@cdecker
Copy link
Member

cdecker commented Jun 7, 2022

Oh, I see this is related to being able to distinguish them and associate them with downstream effects, without having to differentiate them by amount which may also have a collision. Very good catch, I didn't see this :-)

Sounds good to me ^^

Do you mind adding a tiny Changelog-Added to your commit message so it'll show up in the changelog on release too?

ACK b3bb188

@fiatjaf
Copy link
Contributor Author

fiatjaf commented Jun 7, 2022

OK, done!

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 94522c5

Changelog-Added: Plugins: `htlc_accepted` now exposes the `short_channel_id` for the channel from which that HTLC is coming from and the low-level per-channel HTLC `id`, which are necessary for bridging two different Lightning Networks when MPP is involved.
@fiatjaf
Copy link
Contributor Author

fiatjaf commented Jun 7, 2022

OK, I've also added the incoming short_channel_id now as I just realized that wasn't present.

@rustyrussell
Copy link
Contributor

Good catch!

Ack 5404c20

@rustyrussell rustyrussell merged commit 1eaec22 into ElementsProject:master Jun 18, 2022
@fiatjaf fiatjaf deleted the expose-htlc-id branch June 18, 2022 14:50
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.

4 participants