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

option_data_loss_protect: concretely define `my_current_per_commitment_point` #550

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@niftynei
Copy link
Contributor

niftynei commented Jan 18, 2019

Make it more obvious what the expected value of my_current_per_commitment_point is.

@pm47

pm47 approved these changes Jan 18, 2019

@niftynei

This comment has been minimized.

Copy link
Contributor Author

niftynei commented Jan 29, 2019

@Roasbeef did you want to add some clarification to this?

@@ -1154,6 +1154,8 @@ The sending node:
- MUST set `next_remote_revocation_number` to the commitment number of the
next `revoke_and_ack` message it expects to receive.
- if it supports `option_data_loss_protect`:
- MUST set `my_current_per_commitment_point` to its commitment point for

This comment has been minimized.

@Roasbeef

Roasbeef Feb 4, 2019

Member

I think this would be more clear as:

to its current unrevoked commitment point (sent in the last revoke_and_ack message) used by its channel peer to create the last commitment it received from them

As this is the point that the party who lost data will need to use once the surviving party broadcasts their commitment on chain. In our codebase, we call it LocalUnrevokedCommitPoint for this reason. Main thing that IMO makes it easier to grasp is that this is the current unrevoked point for that party.

This comment has been minimized.

@rustyrussell

rustyrussell Feb 4, 2019

Collaborator

How about we append:

(the commitment_point corresponding to the commitment transaction the sender would use to unilaterally close)

This comment has been minimized.

@niftynei

niftynei Feb 4, 2019

Author Contributor

amended to include clarification about being commitment transaction sender would use to unilaterally close

option_data_loss_protect: concretely define
`my_current_per_commitment_point`

Make it more obvious what the expected value of
`my_current_per_commitment_point` is.

@niftynei niftynei force-pushed the niftynei:nifty/my_current_per_commitment_point branch from d019238 to cd2a016 Feb 4, 2019

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