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

[bug]: lnd should allow HTLC receiver to dip into its channel reserve #8249

Open
t-bast opened this issue Dec 4, 2023 · 5 comments · May be fixed by #9100
Open

[bug]: lnd should allow HTLC receiver to dip into its channel reserve #8249

t-bast opened this issue Dec 4, 2023 · 5 comments · May be fixed by #9100
Assignees
Labels
commitments Commitment transactions containing the state of the channel feature request Requests for new features HTLC P2 should be fixed if one has time
Milestone

Comments

@t-bast
Copy link
Contributor

t-bast commented Dec 4, 2023

Since the recent on-chain fees spike, we've noticed that many of our lnd peers increased the commit tx feerate to the point where they are just above their channel reserve, without keeping a buffer for the increased commit tx fees for an incoming HTLC. Those channels are thus stuck, because of the very old issue described in lightning/bolts#740

Since we implemented splicing in eclair, we realized that the receiver of an HTLC should always accept incoming HTLCs that make the receiver temporarily drop below their reserve (because of the increased commit tx fees). It is the sender that is taking a risk by doing so. The spec does not tell receivers to treat this as an error, but lnd (and cln) do. This is very annoying, because we could unblock those channels if lnd accepted incoming HTLCs in that situation. Could you relax that requirement instead of sending an invalid update error?

@t-bast t-bast added bug Unintended code behaviour needs triage labels Dec 4, 2023
@ziggie1984
Copy link
Collaborator

ziggie1984 commented Dec 4, 2023

I think this will be covered in #8096, where we intorduce the feebuffer, but I am not sure whether we should allow going below the reserve because with the feebuffer its very unlikely to happen anyways.
Let's imagine we have 1 outgoing + 1 incoming htlc, both of them are added to the commitment in one cycle and would need to dip the local into its reserve to succeed. Would it be allowed, because if I add the outgoing first and then the incoming, your requirement would hold (always allow the incoming to dip the opener into its reserves) but what if we apply them in the other order, incoming first than outgoing ? Should we fail then ?
How do you handle this case, or the case you described at the beginning in eclair ?

@ziggie1984 ziggie1984 self-assigned this Dec 4, 2023
@t-bast
Copy link
Contributor Author

t-bast commented Dec 4, 2023

I think this will be covered in #8096, where we intorduce the feebuffer

Good, that will ensure future channel won't end up stuck. But that doesn't help unblock existing channels (until the feerate decreases and an update_fee decreases the commitment feerate), and won't be sufficient when implementing splicing (when the non-initiator splices a large amount in).

we should allow going below the reserve because with the feebuffer its very unlikely to happen anyways.

As a receiver you should always allow it, you are 100% benefiting from this if your peer lets you go below your reserve. I don't see any reason to disallow it (that's why the spec does not tell you to disallow it).

Let's imagine we have 1 outgoing + 1 incoming htlc, both of them are added to the commitment in one cycle

I think you're assuming that you perform those checks when sending/receiving commit_sig, but I believe that's incorrect, those checks must be done when sending/receiving update_add_htlc. You still must not send an HTLC that would make you go below your reserve, but you should allow receiving an HTLC that would make you go below your reserve. There can't be any race here because of that asymmetry between sending and receiving.

In eclair, we simply do the following:

  • when receiving an HTLC, we don't care if the additional fee makes us go below our own reserve (but we check that our peer doesn't go below their reserve)
  • when sending an HTLC:
    • if it would make us go below our reserve, we don't send it
    • if it makes the remote peer go below their reserve, we allow up to 5 pending HTLCs to try unblocking the channel

@t-bast
Copy link
Contributor Author

t-bast commented Dec 4, 2023

@ziggie1984 ziggie1984 added this to the v0.18.0 milestone Dec 4, 2023
@ziggie1984 ziggie1984 added feature request Requests for new features and removed bug Unintended code behaviour feature request Requests for new features needs triage labels Dec 5, 2023
@saubyk saubyk added HTLC commitments Commitment transactions containing the state of the channel P1 MUST be fixed or reviewed labels Dec 11, 2023
@ziggie1984
Copy link
Collaborator

#8096 introduced the feebuffer, but allowing the remote side to dip us below channelreserve and vice versa will be done in the follow-up PR.

@t-bast
Copy link
Contributor Author

t-bast commented Jan 8, 2024

Thanks!

@Roasbeef Roasbeef modified the milestones: v0.18.0, v0.18.1 Apr 25, 2024
@saubyk saubyk modified the milestones: v0.18.1, 0.19.0 May 9, 2024
@saubyk saubyk added this to lnd v0.19 Jun 27, 2024
@saubyk saubyk moved this to Todo in lnd v0.19 Jun 27, 2024
@saubyk saubyk added P2 should be fixed if one has time and removed P1 MUST be fixed or reviewed labels Aug 29, 2024
@ziggie1984 ziggie1984 linked a pull request Sep 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commitments Commitment transactions containing the state of the channel feature request Requests for new features HTLC P2 should be fixed if one has time
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

4 participants