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

Demand revoke_and_ack before more updates #552

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@OrfeasLitos
Copy link
Contributor

OrfeasLitos commented Jan 18, 2019

Fixes #526

@cdecker
Copy link
Collaborator

cdecker left a comment

More clarity about what to do with updates before receiving the revoke_and_ack is needed

@@ -953,6 +953,9 @@ fee changes).
to BIP69 lexicographic ordering of the commitment transaction.
- if it has not recently received a message from the remote node:
- SHOULD use `ping` and await the reply `pong` before sending `commitment_signed`.
- after sending a `commitment_signed` message:
- MUST NOT process any `update` message from the other party before receiving the
`revoke_and_ack` that corresponds to the `commitment_signed` sent.

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

This should also clarify what happens if the counterparty continues to send updates. Should it queue them up? Should it fail the channel?

This comment has been minimized.

@OrfeasLitos

OrfeasLitos Jan 21, 2019

Author Contributor

I propose that further update messages should be simply ignored. I've updated the text accordingly.

@OrfeasLitos OrfeasLitos force-pushed the OrfeasLitos:require-revoke-and-ack branch from 80cc605 to 55b91c9 Jan 21, 2019

@nayuta-gondo

This comment has been minimized.

Copy link
Contributor

nayuta-gondo commented Jan 21, 2019

My explanation is not enough in #526.
I do not feel a problem with the update_ messages before revoke_and_ack after commitment_signed.
I think they should be held until the next commitment_signed.
The problem is a new commitment_signed before revoke_and_ack returns.

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Jan 21, 2019

Failing the channel, queuing or ignoring received update messages are three options then. A fourth is to create a new message in the lines of expecting_revocation. This message should be sent by the party that expects to receive a revoke_and_ack message (along with relevant data) whenever it receives an update message.

  • Failing the channel is too harsh and would probably lead to channels being closed too often.
  • I dislike queuing because it opens a DoS vector.
  • Ignoring messages is the simplest solution given that there exist no edge cases where the two nodes reach a weird state because of bad timing of messages.
  • If such an edge case does exist, then the new expecting_revocation message would save things.

TL;DR: I support ignoring messages, which is not too harsh and secure at the same time. If it leads to inconsistencies, then let's add a new expecting_revocation message.

@SomberNight

This comment has been minimized.

Copy link

SomberNight commented Jan 21, 2019

If you ignore the message, how do you expect the sender of that message to realise it was ignored? I would not be surprised if ignoring the message would lead to some other protocol violation/inconsistency which then would result in the channel being closed/failed.

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Jan 21, 2019

Ignoring the message is a good idea in the following example: A sends commitment_signed but the message is delayed. B sends update before receiving commitment_signed. B then realizes that the latest update will be ignored and thus sends a revoke_and_ack.

However in this example, if A's first message is dropped, then B will never receive commitment_signed. This will probably lead to the channel failing. So ignoring the update is probably a bad idea, you are right. A better solution for this example would be for A to resend commitment_signed to B upon receiving the update. I'm not sure if a simple retransmission of commitment_signed is enough, or the new message expecting_revocation is required to cover some other cases.

Skimming through https://github.com/lightningnetwork/lightning-rfc/blob/master/02-peer-protocol.md#message-retransmission , it is not clear whether the case we discuss is covered.

@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator

TheBlueMatt commented Jan 21, 2019

I strongly think we should be failing the channel if a counterparty is misbehaving and sending multiple updates without us having sent an RAA. The problem is the "ACK" part of RAA - if they send multiple updates before our RAA then there is no way for us (or them) to have any idea what we're ACK'ing, and, thus, which updates should be included in our next commitment_signed. This is pretty obviously very busted behavior, so we should probably not be talking to a node like that.

@nayuta-gondo

This comment has been minimized.

Copy link
Contributor

nayuta-gondo commented Jan 21, 2019

RAA's ACK will be ACK for CS?
It can be said that it is for the update_s committed by the CS.
update_s sent after a CS will not be committed by the CS.
In the time series, I think it is clear which update_s are included in which CS.

@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator

TheBlueMatt commented Jan 21, 2019

To quote the intro section to revoke_and_ack:

This message also implicitly serves as an acknowledgment of receipt of the commitment_signed, so this is a logical time for the commitment_signed sender to apply (to its own commitment) any pending updates it sent before that commitment_signed.
@rustyrussell

This comment has been minimized.

Copy link
Collaborator

rustyrussell commented Jan 21, 2019

This is wrong. You can certainly send more updated (and we do!) before receiving revoke_and_ack. But you can't send another commitment_signed. If you try to implement that you'll see why: you don't know the per-commitment point for the N+1'th commitment, so can't create the tx.

@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator

TheBlueMatt commented Jan 21, 2019

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Jan 22, 2019

But one question still remains: After sending commitment_signed and before receiving revoke_and_ack, what should a node do on receiving an update_?

  • Ignore it?
  • Fail the channel?
  • Drop it and resend commitment_signed?
  • Queue it and wait for revoke_and_ack?
  • A combination of the above?
    It is important that this behavior be specified, unless we want nodes to implement it in incompatible ways.
@rustyrussell

This comment has been minimized.

Copy link
Collaborator

rustyrussell commented Feb 4, 2019

You apply it to your local commitment, as always. It has nothing (much) to do with sending commitment_signed, which is about applying changes to the remote update.

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Feb 4, 2019

So, to sum it all up, a local node that has just sent a commitment_signed is waiting for a revoke_and_ack (which may come very far in the future, not a problem), and also accepts update_s from the remote and it applies them (adds add and removes fulfill) to its local commitment. It can also send new update_s (given that they don't fulfill non-irrevocably-commited htlcs). The one thing that the local node can't do is send another commitment_signed. Once it receives the expected revoke_and_ack, the local node may send a new commitment_signed with the signatures to all the htlcs that have been applied (sent or received) in the meanwhile.

Is this all correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment