Address first comments from @mulmarta

mls-rs-core/src/group/proposal_type.rs

@@ -58,7 +58,7 @@ impl ProposalType {
     pub const GROUP_CONTEXT_EXTENSIONS: ProposalType = ProposalType(7);
 
     #[cfg(feature = "replace_proposal")]
-    pub const REPLACE: ProposalType = ProposalType(8);
+    pub const REPLACE: ProposalType = ProposalType(0xff01);
 
     /// Default proposal types defined
     /// in [RFC 9420](https://www.rfc-editor.org/rfc/rfc9420.html#name-leaf-node-contents)

mls-rs/src/group/mod.rs

@@ -41,7 +41,6 @@ pub use state::GroupState;
 #[cfg(feature = "by_ref_proposal")]
 use crate::{
     crypto::{HpkePublicKey, HpkeSecretKey},
-    map::SmallMap,
 };
 
 #[cfg(feature = "replace_proposal")]
@@ -260,6 +259,7 @@ impl NewMemberInfo {
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq, MlsEncode, MlsDecode, MlsSize)]
 struct PendingUpdate {
+    epoch: u64,
     secret_key: HpkeSecretKey,
     signer: Option<SignatureSecretKey>,
 }
@@ -743,17 +743,10 @@ where
                     .map(|p| p.proposal.leaf_node.public_key.clone()),
             );
 
+            // The duplicate update filtering above assures that there is at most one self-update.
             let mut updated_leaves = updated_leaves;
             let self_update = updated_leaves.next();
 
-            if updated_leaves.count() > 0 {
-                // XXX(RLB) This error code is wrong; might need a new value?  Or just not bother
-                // checking, assuming checks have been done upstream?  But then we might also need a
-                // new error code there.  In any case, we need to enforce that there are not multiple
-                // Update **or Replace** proposals for the same leaf.
-                return Err(MlsError::InvalidCommitSelfUpdate);
-            }
-
             if let Some(leaf_pk) = self_update {
                 // Update the leaf in the private tree if this is our update
                 // let pending_update = self.pending_updates.get(&leaf_pk);
@@ -914,14 +907,15 @@ where
         let new_leaf_node_extensions =
             leaf_node_extensions.unwrap_or(leaf_node.ungreased_extensions());
         let properties = self.config.leaf_properties(new_leaf_node_extensions);
+        let epoch = self.current_epoch();
 
         #[cfg(feature = "replace_proposal")]
         let mut properties = properties;
 
         #[cfg(feature = "replace_proposal")]
         properties
             .extensions
-            .set_from(LeafNodeEpochExt::new(self.current_epoch()))?;
+            .set_from(LeafNodeEpochExt::new(epoch))?;
 
         let secret_key = leaf_node
             .update(
@@ -934,7 +928,11 @@ where
             )
             .await?;
 
-        let pending_update = PendingUpdate { secret_key, signer };
+        let pending_update = PendingUpdate {
+            epoch,
+            secret_key,
+            signer,
+        };
 
         // Store the secret key in the pending updates storage for later
         self.pending_updates
@@ -2013,12 +2011,32 @@ where
         #[cfg(feature = "by_ref_proposal")]
         self.state.proposals.clear();
 
-        // Clear the pending updates list (but only if they can't show up in a Replace)
+        // If the only way our leaf can change is via Update, just clear the cache
         #[cfg(all(feature = "by_ref_proposal", not(feature = "replace_proposal")))]
         {
             self.pending_updates = Default::default();
         }
 
+        // If Updates can span epochs via Replace, only clear out leaf nodes that cannot possibly
+        // be used again, namely those that have been committed or those with an earlier epoch than
+        // the current leaf.
+        #[cfg(feature = "replace_proposal")]
+        {
+            // Delete any cached state for the current public key
+            let current_leaf_pk = self.current_user_leaf_node()?.public_key.clone();
+            self.pending_updates.remove(&current_leaf_pk);
+
+            // If the current leaf node contains an epoch value, delete any cached state for
+            // updates from prior epochs.
+            self.current_user_leaf_node()?
+                .extensions
+                .get_as::<LeafNodeEpochExt>()?
+                .map(|epoch_ext| {
+                    self.pending_updates
+                        .retain(|_pk, upd| upd.epoch >= epoch_ext.epoch);
+                });
+        }
+
         self.pending_commit = None;
 
         Ok(())

mls-rs/src/group/proposal_filter/filtering.rs

@@ -290,9 +290,13 @@ where
                                 valid.then_some(()).ok_or(MlsError::InvalidSuccessor)
                             });
 
-                        // If the old leaf has a leaf_node_epoch extension, then the new leaf must
-                        // have one as well, and the new value must be at least as big as the old
-                        // value.
+                        // XXX(RLB) It's not clear that this is the right policy.  On the one hand,
+                        // it allows for the epoch locking to be turned off.  On the other hand,
+                        // this can be abused to roll back to any leaf that doesn't have an epoch
+                        // marker.
+
+                        // If both the old and new leaves have `leaf_node_epoch` extensions, then
+                        // the new value must be at least as big as the old value.
                         let old_epoch = old_leaf.extensions.get_as::<LeafNodeEpochExt>().ok()?;
                         let new_epoch = leaf.extensions.get_as::<LeafNodeEpochExt>().ok()?;
                         let epoch_successor = old_epoch
@@ -440,7 +444,7 @@ fn filter_out_duplicate_updates(
     let mut seen = vec![];
     proposals.retain_by_type::<UpdateProposal, _, _>(|p| {
         let Sender::Member(index) = p.sender else {
-            unreachable!()
+            return Err(MlsError::InvalidSender);
         };
 
         let fresh = !seen.contains(&index);
@@ -655,7 +659,7 @@ pub(crate) fn proposer_can_propose(
         ),
         #[cfg(feature = "by_ref_proposal")]
         (Sender::External(_), ProposalSource::ByValue) => false,
-        #[cfg(feature = "by_ref_proposal")]
+        #[cfg(all(feature = "by_ref_proposal", not(feature = "replace_proposal")))]
         (Sender::External(_), _) => matches!(
             proposal_type,
             ProposalType::ADD
@@ -664,6 +668,16 @@ pub(crate) fn proposer_can_propose(
                 | ProposalType::PSK
                 | ProposalType::GROUP_CONTEXT_EXTENSIONS
         ),
+        #[cfg(all(feature = "by_ref_proposal", feature = "replace_proposal"))]
+        (Sender::NewMemberCommit, ProposalSource::ByValue | ProposalSource::Local) => matches!(
+            proposal_type,
+            ProposalType::ADD
+                | ProposalType::REMOVE
+                | ProposalType::REPLACE
+                | ProposalType::RE_INIT
+                | ProposalType::PSK
+                | ProposalType::GROUP_CONTEXT_EXTENSIONS
+        ),
         (Sender::NewMemberCommit, ProposalSource::ByValue | ProposalSource::Local) => matches!(
             proposal_type,
             ProposalType::REMOVE | ProposalType::PSK | ProposalType::EXTERNAL_INIT

mls-rs/src/map.rs

@@ -62,6 +62,13 @@ mod map_impl {
         fn find(&self, key: &K) -> Option<usize> {
             self.0.iter().position(|(k, _)| k == key)
         }
+
+        pub fn retain<F>(&mut self, mut f: F)
+        where
+            F: FnMut(&K, &mut V) -> bool,
+        {
+            self.0.retain_mut(|(k, v)| f(k, v));
+        }
     }
 }