Closing w/ unacked htlcs

[pm47]

Hi,

I think there is something wrong with how unacked htlcs are handled by lightning-c during mutual closing. The spec is a little vague on this so I can't tell if lightning-c implementation does or doesn't comply. I guess it depends on how one side indicating that it wants to clear the channel (and thus will accept no new HTLCs) (bolt02, §4) is interpreted.

In file peer.c (lines 402 and 434), we go to STATE_MUTUAL_CLOSING when there are no htlcs in signed tx (actual check is done in function committed_to_htlcs).

But I don't see why we shouldn't keep the unacked htlcs. As soon as both parties have sent and received a close_clearing packet, and because they agree not to send any new htlcs after having sent their own close_clearing packet (bolt02, §4.1), the state is perfectly well-defined and all pending htlcs (sent & received) could be included.

What bothers me is that in the rest of the protocol we assume that a valid htlc will always be accepted (there is no way for a peer to say 'no'), so the fact that in this specific case some htlcs could be discarded doesn't seem very consistent.

What do you think?

Cheers,

Pierre

[pm47]

There might actually be a bug related to this. It is hidden because in lightning-c each update_add_htlc triggers an automatic update_commit after a very short (10ms) timeout so we almost never do have uncommitted htlcs.

If you increase the commit timeout to something like 1min (lightningd.c line 194) and follow this scenario:

A                 B
|      add        |
|---------------->|
|                 |
|    close        |
|<----------------|
|                 |

You'll see that A will complain about having received a bad signature.

[rustyrussell]

Good catch! I was flip-flopping on what to do with uncommitted HTLCs, to be honest.

If the sender wants them committed before the close, it's trivial for them to send a commit message. But as you state, there's no other undo function. It's tempting to just make it illegal for the moment, rather than undefined as it is now.

Either way, the implementation shouldn't do it.

[rustyrussell]

Reproduced, and tested. How's this?

https://github.com/ElementsProject/lightning/tree/issue-29-close-with-uncommitted

[pm47]

Hmm, I don't see how this solves this (starting with no pending htlcs on either side):

A                 B
| add      close  |
|------  ---------|
|      \/         |
|      /\         |
|<-----  -------->|
|                 |

Or am I missing something? Sorry I am not familiar enough with your code to quickly make an actual failing test.

[rustyrussell]

Revisiting this: We still have the semantics that you shouldn't mutual close with pending changes. If you really wanted to cancel pending changes, you could reconnect, and if you wanted to commit them, send a commit.

So I think I can close this?

[pm47]

We still have the semantics that you shouldn't mutual close with pending changes

Yes but there is still the concurrent add/close scenario below, which I think will happen frequently.

A                 B
| add      close  |
|------  ---------|
|      \/         |
|      /\         |
|<-----  -------->|
|                 |

if you wanted to commit them, send a commit

That would be fine by me, but the spec in its current state prevents this (one side indicating that it wants to clear the channel (and thus will accept no new HTLCs), bolt02, §4).

So the issue remains, although I agree this might not be high priority.

[rustyrussell]

Pierre-Marie Padiou notifications@github.com writes:

We still have the semantics that you shouldn't mutual close with pending changes

Yes but there is still the concurrent add/close scenario below, which I think will happen frequently.

A                 B
| add      close  |
|------  ---------|
|      \/         |
|      /\         |
|<-----  -------->|
|                 |

if you wanted to commit them, send a commit

That would be fine by me, but the spec in its current state prevents this (one side indicating that it wants to clear the channel (and thus will accept no new HTLCs), bolt02, §4).

So the issue remains, although I agree this might not be high priority.

In the above case, A will have to commit before sending the close, and
then B will instantly fail the new HTLC, ie:

A B
| add close |
|------ ---------|
| / |
| /\ |
|<----- -------->|

commit
\
\
-------->
revoke
---------
/
/
<-----
close fail
------ ---------
/
/\
<----- -------->

I'll fix it in the code (and in the new spec).

Thanks!
Rusty.