sub group branching with new group builder#1913
sub group branching with new group builder#1913franziskuskiefer wants to merge 1 commit intomainfrom
Conversation
keks
left a comment
keks
left a comment
There was a problem hiding this comment.
Some early feedback. I think this PR is doing a lot, and maybe we can talk offline about the bigger approach here, which builders are responsible for what, etc.
| /// Adds a PreSharedKey proposal for the provided [`PreSharedKeyId`]s to the | ||
| /// list of proposals to be committed. | ||
| /// | ||
| /// Note that this should not be used for sub-group branching, as those PSKs | ||
| /// are not allowed in regular proposals. Please use [`Self::branch()`] instead. | ||
| pub fn propose_psks(mut self, psk_ids: impl IntoIterator<Item = PreSharedKeyId>) -> Self { | ||
| self.stage.own_proposals.extend( | ||
| psk_ids | ||
| .into_iter() | ||
| .map(|psk_id| Proposal::psk(PreSharedKeyProposal::new(psk_id))), | ||
| ); | ||
| self | ||
| } | ||
|
|
||
| /// Branches the group by adding a branch PreSharedKey proposals. | ||
| pub fn branch(mut self, psk_id: PreSharedKeyId, secret: ResumptionPskSecret) -> Self { | ||
| self = self.propose_psks([psk_id]); | ||
|
|
||
| // We clear the resumption PSK store first and then add the branching PSK. | ||
| self.group.borrow_mut().resumption_psk_store.clear(); | ||
| self.group | ||
| .borrow_mut() | ||
| .resumption_psk_store | ||
| .add(0.into(), secret); | ||
| self | ||
| } |
There was a problem hiding this comment.
Should these already check that the PSKs are suitable for the use case and do not contradict?
There was a problem hiding this comment.
it could ... but we need the check later again because more psks may have been added after this. So I'm not sure if checking here gives us so much.
| /// Adds a PreSharedKey proposal for the provided [`PreSharedKeyId`]s to the list of proposals | ||
| /// to be committed. | ||
| /// | ||
| /// Note that this should not be used for sub-group branching, as those PSKs | ||
| /// are not allowed in regular proposals. Please use [`Self::branch()`] instead. | ||
| pub fn propose_psks(mut self, psk_ids: impl IntoIterator<Item = PreSharedKeyId>) -> Self { | ||
| self.stage.own_proposals.extend( | ||
| psk_ids | ||
| .into_iter() | ||
| .map(|psk_id| Proposal::psk(PreSharedKeyProposal::new(psk_id))), | ||
| ); | ||
| self | ||
| } | ||
|
|
||
| /// Branches the group by adding a branch PreSharedKey proposals. | ||
| pub fn branch(mut self, psk_id: PreSharedKeyId) -> Self { | ||
| self.stage.branch = true; | ||
| self = self.propose_psks([psk_id]); | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think these should be here. The LoadedPsk phase is too late to add more proposals, especially PSK proposals, because then these are not loaded anymore and the later computations won't have the keys (just the IDs).
There was a problem hiding this comment.
Oh yeah, this bit shouldn't be here.
|
|
||
| /// The state after decrypting the group secrets in the welcome message. | ||
| pub struct Decrypted<'a> { | ||
| /// The initial state. |
There was a problem hiding this comment.
| /// The initial state. | |
| /// The previous state. |
|
|
||
| /// The state after loading and generating the PSK secret. | ||
| pub struct Psks<'a> { | ||
| /// The initial state. |
There was a problem hiding this comment.
| /// The initial state. | |
| /// The previous state. |
2 participants
We shouldn't just add another way of processing welcomes (it's also not done all the way right now). Moving to a builder style like for the commits makes sense to me. Also for other things like #1843. This is for discussion and we should decide on the style we want to support going forward.
cc @keks