Clarify relative order of some messages after reestablish

[t-bast]

The existing requirements were not specifying the case where both a commitment_signed and revoke_and_ack need to be retransmitted.

This is an important case to specify because if the relative order is not preserved, the channel will close.
See for example the following two scenarios.

Rev then Sig

A            B
 <---add-----
 ----add---->
 <---sig-----
 ----rev----x
 ----sig----x

A needs to retransmit:

----add-->
----rev-->
----sig-->

With some leeway on when she sends the add: it can be either before or after rev, she can choose to not send it at all, or update the fees. The important point is that sig must occur after rev.

Sig then Rev

A            B
 <---add-----
 ----add---->
 ----sig----x
 <---sig-----
 ----rev----x

A needs to retransmit:

----add-->
----sig-->
----rev-->

With some leeway on the add, she may choose to omit it, update the fees, etc (but all of these before the sig). The important point is that sig must occur before rev.

Fixes #794

[Crypt-iQ]

ACK fc07aab
I don't know the language format of the spec too well, but this would fix the issue.

[Roasbeef]

Interestingly, I considered this case when first implementing our retransmission ordering and arbitrarily chose an ordering: https://github.com/lightningnetwork/lnd/blob/master/lnwallet/channel.go#L3810

[Roasbeef]

However the ordering may not be the same with how we implement it atm, since we always do revoke->sig if we need to send both.

[Roasbeef]

Nice work! I thought initially that we need to prescribe a global ordering, but reading this I realize that the main requirement is that the sender itself be consistent in their retransmission ordering.

[Roasbeef]

Perhaps we should also spell out the recommended persistence requirements here as well? This was something lnd did incorrectly in a few places (not storing what we need to in order to properly reconstruct a commitment to validate a retransmission. However np with also leaving this off for a more fundamental path to make things more explicit generally.

[Roasbeef]

Ah re-reading again, there is a light requirement re naive persistence down below where the diff touches the spec.

[t-bast]

Yes, I didn't update that paragraph about persistence that rusty wrote a while ago, but I could, let me know what changes you think would make sense there (if any)!

[Roasbeef]

One question that comes to mind: do we need to care about backwards compatibility for this at all? I'm leaning towards no, as this is the "right" way, although switching to this may create a period of instability (spurious force closes) in the network).

[manreo]

Fixes lightningnetwork/lnd#4613 ?

although switching to this may create a period of instability (spurious force closes) in the network).

There are already spurious force closes... So I think it does not matter

[t-bast]

@Roasbeef do you think I need to add some changes, or is the last state of the PR ok to you?