Skip to content
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

Remove endpoint_id #573

Merged
merged 1 commit into from Feb 3, 2022
Merged

Remove endpoint_id #573

merged 1 commit into from Feb 3, 2022

Conversation

bifurcation
Copy link
Collaborator

@bifurcation bifurcation commented Feb 1, 2022

Fixes #558

This involved a couple of changes other than just removing the field and references to it:

  • Detecting duplicative Adds is now a little more ambiguous, since you can't just match on endpoint_id. However (a) this doesn't degrade the security of the protocol, just makes it a little less efficient (b) as discussed in #558, if applications want to sharpen it, they can add an endpoint_id-like identifier in an extension.
  • The External Commit language around resync got generalized to say that the KeyPackage needs to meet the requirements of an Update for the removed node, which captures the successor notions we've discussed.

I also noticed that when we forbade proposals by reference in #562, we didn't totally remove them. So I went ahead and fixed that here.

Copy link
Contributor

@kkohbrok kkohbrok left a comment

Thanks for taking care of the cleanup here after the discussion on #509.

If the application has to provide a function that determines if a given KeyPackage represents a valid update, we could use the same function to determine if an Add is a duplicate. Although admittedly, in that case one would have to compare with all other KeyPackages in the group and the function might not be very efficient.

Generally, I'm ok with this solution, though 👍🏻.

@bifurcation bifurcation merged commit b40a1a6 into main Feb 3, 2022
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants