BOLT 2: forget the check about update_* messages, and check what must not happens during shutdown

[vincenzopalazzo]

The rationale around this PR that is a simplification of #970 is that a node should not care about what it receives but only about what it must not receive. Just to quote @TheBlueMatt

The actual Close Channel spec tell us exactly when we need to fail, we need just to remove the confusing part related to the update_* messages

So, the concept proposed is to use a black list of messages that a node should never receive during a shutdown, and the actual status the black list contains only is just an add_htlc.

lnprototest reference implementation: rustyrussell/lnprototest#37

[TheBlueMatt]

Any thoughts @Crypt-iQ ?

[Crypt-iQ]

The changes look fine, but I think we should specify when it's ok to send closing_signed

[TheBlueMatt]

Right, agreed, this only does half of what #970 was doing, we still need to clarify the "when can you send closing_signed" part.

[vincenzopalazzo]

The changes look fine, but I think we should specify when it's ok to send closing_signed

Good point! In this case, we can use the same idea to simplify the code, with the following way:

• (from the sender side) send a closing_signed message when the are no pending htlcs, and
• (from the receiver side) we can accept closing_signed only when there are no pending htlcs, and if a closing_signed when there are htlcs to resolve, the received will send a warning message (or error to fail the close operation?)

What do you think?

[TheBlueMatt]

Good point! In this case, we can use the same idea to simplify the code, with the following way:

Lets do this in a separate PR, probably #970, I think the parts of #970 that do this are fine, though can be a bit better-defined.

[t-bast]

ACK a16bfe9

Eclair already handles receiving update_fee after shutdown and actually also sends update_fee if the feerate changes a lot, so this would align with our behavior.

[vincenzopalazzo]

Another good thing that we can introduce here @t-bast is how manage the update_fee, how @Crypt-iQ a node can continue to send update_fee for any reason, and this can stop a node with the current master BOLT to close a channel.

However, we can also add a line to specify that update_fee need to stay in the range defined in the message description

• if the update_fee is too low for timely processing, OR is unreasonably large: MUST send a warning and close the connection, or send an error and fail the channel

Where the unreasonable definition is up to the implementation, in this way we will solve also the problem to when send the closing_signed because if the update fee are to much faster, the node can pic one that he choose defined in a reasonable time and send the closing_signed

[t-bast]

However, we can also add a line to specify that update_fee need to stay in the range defined in the message description

if the update_fee is too low for timely processing, OR is unreasonably large: MUST send a warning and close the connection, or send an error and fail the channel

That would be unnecessary, we only need to have this paragraph in the section about update_fee, no need to duplicate it here it would only bloat the requirements.

[Roasbeef]

Today lnd doesn't actually handle update_fee if it's sent after a shutdown. At this point we assume that no other updates can happen to a channel, so we ignore them (read the message and just drop it). I'll make an issue on our end to start handling it properly, since it is the case that you need it even w/ anchors if the relay fee grows too high.

[TheBlueMatt]

no other updates can happen to a channel

I assume this does not include HTLC removal updates? Just checking to make sure. This would imply if a peer removes an HTLC and sends a fee update you'll just force-close on them?

[Crypt-iQ]

no other updates can happen to a channel

I assume this does not include HTLC removal updates? Just checking to make sure. This would imply if a peer removes an HTLC and sends a fee update you'll just force-close on them?

lnd doesn't do shutdown until the channel state is totally clean. lnd won't force close if you send messages at this point, it will just drop them. this will be fixed once the spec is a little more clear

[TheBlueMatt]

lnd doesn't do shutdown until the channel state is totally clean.

Ah that's...an interesting interpretation of the spec. But in practice I'd imagine that means this isn't a big deal - update_fee after channel is clear should be really rare, but before of course it may be common. That said it does imply lnd is willing to relay/send over a channel that will always reject the HTLC :/

[vincenzopalazzo]

Mh! I jump on this PR because I was looking inside some work in lnprototest and I think this PR should add some additional info related to the message that is banned on the shutdown. or it is fine in this way?