Skip to content

Commit

Permalink
Fix unused proposals when receiving commit
Browse files Browse the repository at this point in the history
Unused proposals were previously made of the proposals in the received
commit that would cause the commit to be invalid and thus were excluded.
But given that invalid proposals when receiving a commit cause the
entire commit to be rejected, it means that unused proposals were always
empty when receiving a commit.

Unused proposals are now made of the proposals in the receiver's cache
that are not in the received commit.
  • Loading branch information
stefunctional committed Jan 24, 2024
1 parent 63b454c commit 8d5b9e3
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 22 deletions.
51 changes: 30 additions & 21 deletions mls-rs/src/group/proposal_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use mls_rs_core::{error::IntoAnyError, psk::PreSharedKeyStorage};
#[cfg(feature = "by_ref_proposal")]
#[derive(Debug, Clone, MlsSize, MlsEncode, MlsDecode, PartialEq)]
pub struct CachedProposal {
proposal: Proposal,
sender: Sender,
pub(crate) proposal: Proposal,
pub(crate) sender: Sender,
}

#[cfg(feature = "by_ref_proposal")]
Expand Down Expand Up @@ -89,21 +89,21 @@ impl ProposalCache {
sender: Sender,
additional_proposals: Vec<Proposal>,
) -> ProposalBundle {
let mut proposals = ProposalBundle::default();

for (r, p) in &self.proposals {
proposals.add(
p.proposal.clone(),
p.sender,
ProposalSource::ByReference(r.clone()),
);
}

for p in additional_proposals.into_iter() {
proposals.add(p, sender, ProposalSource::ByValue);
}

proposals
self.proposals
.iter()
.map(|(r, p)| {
(
p.proposal.clone(),
p.sender,
ProposalSource::ByReference(r.clone()),
)
})
.chain(
additional_proposals
.into_iter()
.map(|p| (p, sender, ProposalSource::ByValue)),
)
.collect()
}

pub fn resolve_for_commit(
Expand Down Expand Up @@ -249,8 +249,13 @@ impl GroupState {
.await?;

#[cfg(feature = "by_ref_proposal")]
let rejected_proposals =
rejected_proposals(all_proposals, &applier_output.applied_proposals);
let rejected_proposals = rejected_proposals(
match direction {
CommitDirection::Send => all_proposals,
CommitDirection::Receive => self.proposals.proposals.iter().collect(),
},
&applier_output.applied_proposals,
);

let mut group_context = self.context.clone();
group_context.epoch += 1;
Expand Down Expand Up @@ -508,13 +513,14 @@ pub(crate) mod test_utils {

context.extensions = group_extensions.clone();

let state = GroupState::new(
let mut state = GroupState::new(
context,
public_tree.clone(),
Vec::new().into(),
ConfirmationTag::empty(cipher_suite_provider).await,
);

state.proposals.proposals = self.proposals.clone();
let proposals = self.resolve_for_commit(sender, proposal_list)?;

state
Expand Down Expand Up @@ -4194,7 +4200,10 @@ mod tests {
.unwrap();

let [p] = &state.rejected_proposals[..] else {
panic!("Expected single rejected proposal");
panic!(
"Expected single rejected proposal but got {:?}",
state.rejected_proposals
);
};

assert_eq!(p.proposal_ref(), Some(&proposal_ref));
Expand Down
43 changes: 42 additions & 1 deletion mls-rs/src/group/proposal_filter/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::{
};

#[cfg(feature = "by_ref_proposal")]
use crate::group::{LeafIndex, ProposalRef, UpdateProposal};
use crate::group::{proposal_cache::CachedProposal, LeafIndex, ProposalRef, UpdateProposal};

#[cfg(feature = "psk")]
use crate::group::PreSharedKeyProposal;
Expand Down Expand Up @@ -434,6 +434,47 @@ impl ProposalBundle {
}
}

impl FromIterator<(Proposal, Sender, ProposalSource)> for ProposalBundle {
fn from_iter<I>(iter: I) -> Self
where
I: IntoIterator<Item = (Proposal, Sender, ProposalSource)>,
{
let mut bundle = ProposalBundle::default();
for (proposal, sender, source) in iter {
bundle.add(proposal, sender, source);
}
bundle
}
}

#[cfg(feature = "by_ref_proposal")]
impl<'a> FromIterator<(&'a ProposalRef, &'a CachedProposal)> for ProposalBundle {
fn from_iter<I>(iter: I) -> Self
where
I: IntoIterator<Item = (&'a ProposalRef, &'a CachedProposal)>,
{
iter.into_iter()
.map(|(r, p)| {
(
p.proposal.clone(),
p.sender,
ProposalSource::ByReference(r.clone()),
)
})
.collect()
}
}

#[cfg(feature = "by_ref_proposal")]
impl<'a> FromIterator<&'a (ProposalRef, CachedProposal)> for ProposalBundle {
fn from_iter<I>(iter: I) -> Self
where
I: IntoIterator<Item = &'a (ProposalRef, CachedProposal)>,
{
iter.into_iter().map(|pair| (&pair.0, &pair.1)).collect()
}
}

#[cfg_attr(
all(feature = "ffi", not(test)),
safer_ffi_gen::ffi_type(clone, opaque)
Expand Down

0 comments on commit 8d5b9e3

Please sign in to comment.