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

Can't mutual close with pending HTLC #6039

Closed
thomash-acinq opened this issue Nov 30, 2021 · 9 comments · Fixed by #8167
Closed

Can't mutual close with pending HTLC #6039

thomash-acinq opened this issue Nov 30, 2021 · 9 comments · Fixed by #8167
Assignees
Labels
bug Unintended code behaviour channel closing Related to the closing of channels cooperatively and uncooperatively P1 MUST be fixed or reviewed spec
Milestone

Comments

@thomash-acinq
Copy link

Background

When trying to close a channel with a lnd node, the node returned the error cannot co-op close channel w/ active htlcs

Steps to reproduce

Open a channel to a lnd node, add a HTLC , send a shutdown message.

Expected behaviour

The node should wait for the HTLC to be resolved, not add any new HTLC, and then reply with a shutdown message.
BOLT 2: Channel Close

Actual behaviour

The lnd node returns an error: cannot co-op close channel w/ active htlcs

@Crypt-iQ
Copy link
Collaborator

We're aware and plan to fix this in the future

@Roasbeef
Copy link
Member

Related issue: #4819

@Roasbeef Roasbeef added this to the v0.15.0 milestone Nov 30, 2021
@Roasbeef
Copy link
Member

On the API level we return this error so the user gets quick feedback that a co-op close isn't possible at that time. On the p2p level, we currently use the same codepath, but should instead defer and wait till things clear up (or the channel force closes?).

@Roasbeef Roasbeef added bug Unintended code behaviour spec channel closing Related to the closing of channels cooperatively and uncooperatively labels Nov 30, 2021
@Crypt-iQ
Copy link
Collaborator

This is one area of the spec I had trouble with. I think the spec could use better wording here also.

@saubyk
Copy link
Collaborator

saubyk commented Mar 13, 2023

prioed 17

@saubyk saubyk modified the milestones: v0.16.1, v0.17.0 Mar 13, 2023
@saubyk saubyk added this to lnd v0.17 Mar 26, 2023
@saubyk saubyk moved this to 🏗 In progress in lnd v0.17 Mar 26, 2023
@saubyk saubyk moved this from 🏗 In progress to 🔖 Ready in lnd v0.17 Mar 26, 2023
@ziggie1984
Copy link
Collaborator

I like this potential feature and would like to implement it, is it up for grabs ?

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Jun 7, 2023

No this is not up for grabs

@saubyk saubyk removed this from lnd v0.17 Jun 15, 2023
@saubyk saubyk modified the milestones: v0.17.0, v0.17.1 Jun 15, 2023
@saubyk saubyk removed this from the High Priority milestone Aug 8, 2023
@saubyk saubyk added this to lnd v0.18 Aug 17, 2023
@saubyk saubyk moved this to 🔖 Ready in lnd v0.18 Aug 17, 2023
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Aug 17, 2023
LND hasn't properly handled shutdown messages ever, and
force-closes any time we send one while HTLCs are still present.
The issue is tracked at
lightningnetwork/lnd#6039 and has had
multiple patches to fix it but none so far have managed to land
upstream. The issue appears to be very low priority for the LND
team despite being marked "P1".

We're not going to bother handling this in a sensible way, instead
simply repeated the Shutdown message on repeat until morale
improves.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Aug 17, 2023
LND hasn't properly handled shutdown messages ever, and
force-closes any time we send one while HTLCs are still present.
The issue is tracked at
lightningnetwork/lnd#6039 and has had
multiple patches to fix it but none so far have managed to land
upstream. The issue appears to be very low priority for the LND
team despite being marked "P1".

We're not going to bother handling this in a sensible way, instead
simply repeated the Shutdown message on repeat until morale
improves.
@ProofOfKeags ProofOfKeags self-assigned this Aug 25, 2023
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Aug 31, 2023
LND hasn't properly handled shutdown messages ever, and
force-closes any time we send one while HTLCs are still present.
The issue is tracked at
lightningnetwork/lnd#6039 and has had
multiple patches to fix it but none so far have managed to land
upstream. The issue appears to be very low priority for the LND
team despite being marked "P1".

We're not going to bother handling this in a sensible way, instead
simply repeated the Shutdown message on repeat until morale
improves.
@ProofOfKeags
Copy link
Collaborator

Ok. I think I have a decent handle on what's going on here. The issue is that when we receive the shutdown message, we attempt a link shutdown which errors if the channel state is "unclean" (has HTLCs), and then rather than handling that error differently and put the channel into a flushing state, we just propagate that error all the way up the call stack. As far as I can tell, there isn't a proper update flushing state in the codebase as is, so I believe I will have to add this.

I believe the appropriate thing to do here is to make it a property of the channel link. If we do that then we can handle the shutdown message in a way that puts the link into this flushing state, and we can similarly handle it from the gRPC interface as well.

waterson pushed a commit to waterson/rust-lightning that referenced this issue Sep 7, 2023
LND hasn't properly handled shutdown messages ever, and
force-closes any time we send one while HTLCs are still present.
The issue is tracked at
lightningnetwork/lnd#6039 and has had
multiple patches to fix it but none so far have managed to land
upstream. The issue appears to be very low priority for the LND
team despite being marked "P1".

We're not going to bother handling this in a sensible way, instead
simply repeated the Shutdown message on repeat until morale
improves.
@Roasbeef
Copy link
Member

@ProofOfKeags yep, we've done that here in this PR: #6760. We ultimately decided the implementation was more invasive than ideal, to instead go with a simplified approach.

@saubyk saubyk linked a pull request Nov 9, 2023 that will close this issue
7 tasks
@saubyk saubyk moved this from 🔖 Ready to 🏗 In progress in lnd v0.18 Nov 9, 2023
This was referenced Nov 10, 2023
@saubyk saubyk added this to the v0.18.0 milestone Nov 14, 2023
@saubyk saubyk moved this from 🏗 In progress to 👀 In review in lnd v0.18 Jan 7, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in lnd v0.18 Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unintended code behaviour channel closing Related to the closing of channels cooperatively and uncooperatively P1 MUST be fixed or reviewed spec
Projects
Status: Done
6 participants