multi: store bool to determine retransmission ordering

[Crypt-iQ]

With this pull request, we store a boolean per channel under the new lastWasRevokeKey. This key determines if the last material update our channel sent was a CommitSig or RevokeAndAck. When the link calls SignNextCommitment, false is stored under the key. When the link calls RevokeAndAck, true is stored under the key.

For new channels with no updates yet exchanged, nothing is stored and so the key contains nil.

For existing channels updating to this patch, the default nil is also stored. The next update sent will store something under the key.

Fixes #4799
Fixes #4613

[cfromknecht]

first pass looks solid! nice to see that the fix was straightforward. a few nits but otherwise looks good

[cfromknecht]

a comment here would be useful to explain how these two orderings differ

[Crypt-iQ]

Comment added

[halseth]

why is this? Since Bob was not restarted?

Shouldn't bob handle receiving this from Alice?

[Crypt-iQ]

We'll get a duplicate htlc id if Bob accepts (0) while Bob is expecting 1. It's because Bob is represented as LightningChannel

[halseth]

I see. Would i make sense to also restart Bob?

[Crypt-iQ]

Yes, restarting Bob now. Makes the tests cleaner

[halseth]

I find this a bit hard to follow, possibly because we are assuming something about the updates that have been added in an earlier switch statement.

Could an alternative be to split this up

var owedRevocation, owedCommitUpdates []lnwire.Message

switch {
case ...:
    owedRevocation = append(owedRevocation, revokeAndAck)
}

switch {
case ...:
    owedCommitUpdates = append(owedCommitUpdates, commitSig)
}

var updates []lnwire.Message
if lastWasRevoke {
    updates = append(owedCommitUpdates, owedRevocation)
else {
    updates = append(owedRevocation, owedCommitUpdates) 
}

[Crypt-iQ]

I chose this way because this does not account for OweCommitment. Would it then be added to owedRevocation? - Then the name doesn't describe what it does. The switch case msg.NextLocalCommitHeight == remoteTipHeight cannot be called if OweCommitment is called so that doesn't change our current logic if a revocation exists or not.

[Roasbeef]

I chose this way because this does not account for OweCommitment

So won't this end up re ordering the OweCommitment sig if we need to generate+send it?

[Roasbeef]

The switch case msg.NextLocalCommitHeight == remoteTipHeight cannot be called if OweCommitment is called so that doesn't change our current logic if a revocation exists or not.

Ah ok, make sense.

[halseth]

I wonder if we could also encode the OweCommitment case above using this new key?

[Crypt-iQ]

If OweCommitment is called, then this block of code cannot be called

[Crypt-iQ]

Sorry, I don't understand the suggestion

[Roasbeef]

I think he means modify the logic above that determines if we need to send a new sig to use the new value added in the change. IMO we should just try to make the smallest change here, so we can isolate the impact of the diff itself.

[halseth]

I see. Would i make sense to also restart Bob?

[cfromknecht]

LGTM, nice find on this being absent from the spec and a simple fix 🥂

[Roasbeef]

Excellent find and fix!

To our knowledge, there's just one lingering issue in this area. However, we know this PR in itself improves the situation, so it makes sense to move forward with it.