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

HTLC fulfillment flow does not give signer timely validation information #2356

Closed
devrandom opened this issue Jun 14, 2023 · 12 comments · Fixed by #2753
Closed

HTLC fulfillment flow does not give signer timely validation information #2356

devrandom opened this issue Jun 14, 2023 · 12 comments · Fixed by #2753
Assignees
Milestone

Comments

@devrandom
Copy link
Member

When fulfilling an HTLC, LDK removes the incoming (previous-hop) one first and does not provide a preimage to the signer. This makes it impossible for a validating signer to ensure no funds are lost.

the options are, in order of preference:

  • remove the outgoing (next-hop) HTLC first, and provide the preimage during the first related signing operation
  • continue removing the incoming (previous-hop) HTLC first, and provide the preimage during the first related signing operation

The first option will simplify the signer logic, because it will maintain the invariant that incoming value is always >= outgoing value.

Logs from a relevant functional test of LDK+VLS are here: https://gitlab.com/lightning-signer/validating-lightning-signer/-/issues/331

@TheBlueMatt
Copy link
Collaborator

Because we remove the inbound edge in response to the update_fulfill_htlc rather than after the commitment dance from the outbound edge (and I don't think we want to change that) it'd be kinda hard to remove the outbound edge first. Thus, we'll just want to provide the preimage for local removes on the claim.

@TheBlueMatt
Copy link
Collaborator

Alternatively, we could just provide the preimage "raw" in the outgoing channel to indicate that the claim is "in-progress", but that's a bit confusing cause the claim could revert after a reconnect (at which point we get free money).

@devrandom
Copy link
Member Author

Alternatively, we could just provide the preimage "raw" in the outgoing channel to indicate that the claim is "in-progress", but that's a bit confusing cause the claim could revert after a reconnect (at which point we get free money).

well, either way. we don't care when we get the preimage, as long as it's before the incoming HTLC disappears. we just want to be able to tell that the incoming now belongs to us, and when it goes away we can ensure that our balance in that channel has increased by that amount, which keeps the payment balanced.

@TheBlueMatt TheBlueMatt added this to the 0.0.117 milestone Jun 15, 2023
@TheBlueMatt
Copy link
Collaborator

Sadly 0.0.117 exploded into a million tasks and this just isn't likely in the next few weeks if we want to get other important fixes out ASAP.

@TheBlueMatt TheBlueMatt modified the milestones: 0.0.117, 0.0.118 Aug 31, 2023
@TheBlueMatt TheBlueMatt modified the milestones: 0.0.118, 0.0.119 Oct 12, 2023
@TheBlueMatt
Copy link
Collaborator

Sorry, somehow this totally slipped off my plate. I think we do already provide the relevant preimage - EcdsaChannelSigner::sign_counterparty_commitment has a preimages argument which should always include the preimage for the removed HTLC. Does that suffice for what you need, or is there more (eg the hash or HTLC id) or is that not working in practice?

@devrandom
Copy link
Member Author

right, I added that back in 2022. I think this issue was opened because the preimages are not actually provided when the incoming HTLC is removed.

@TheBlueMatt
Copy link
Collaborator

Oh, no, you're right, they're not provided. So the rationale we had is we were only providing preimages for outgoing HTLCs since that's what the counterparty is providing us. However, for incoming HTLCs we didn't provide the preimages since they're driving a balance increase, and we were assuming that the signer would always happily accept a balance increase (and compare that later against the decrease from a counterparty claim). If that's not the case, its rather trivial to also provide the inbound HTLC preimages.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Nov 28, 2023
The VLS signer has a desire to see preimages for resolved forwarded
HTLCs when they are first claimed by us, even if that claim was for
the inbound edge (where claiming strictly increases our balance).

Luckily, providing that information is rather trivial, which we do
here.

Fixes lightningdevkit#2356
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Nov 28, 2023
The VLS signer has a desire to see preimages for resolved forwarded
HTLCs when they are first claimed by us, even if that claim was for
the inbound edge (where claiming strictly increases our balance).

Luckily, providing that information is rather trivial, which we do
here.

Fixes lightningdevkit#2356
@devrandom
Copy link
Member Author

it's not trivial to correlate the balance increase with the HTLC disappearing. there may be multiple HTLCs being added and removed. it's much easier to do the accounting on a per-HTLC basis than to try to figure out the global balance.

@TheBlueMatt
Copy link
Collaborator

Right, I was thinking that you'd compare the set of HTLCs in each commitment transaction to figure that out, which I believe we do provide in full.

@devrandom
Copy link
Member Author

devrandom commented Nov 29, 2023

but if we don't get the preimages at that point, we don't know which HTLCs are disappearing because failed and which are disappearing because fulfilled, which complicates the validation.

@TheBlueMatt
Copy link
Collaborator

Mmm, right, its still napscack. Anyway, #2753 should fix it for you.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Nov 30, 2023
The VLS signer has a desire to see preimages for resolved forwarded
HTLCs when they are first claimed by us, even if that claim was for
the inbound edge (where claiming strictly increases our balance).

Luckily, providing that information is rather trivial, which we do
here.

Fixes lightningdevkit#2356
@devrandom
Copy link
Member Author

great!

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Dec 4, 2023
The VLS signer has a desire to see preimages for resolved forwarded
HTLCs when they are first claimed by us, even if that claim was for
the inbound edge (where claiming strictly increases our balance).

Luckily, providing that information is rather trivial, which we do
here.

Fixes lightningdevkit#2356
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 a pull request may close this issue.

2 participants