mls: replace save_group_relay with replace_group_relays

This PR fixes a bug where Group relays weren't being updated by NostrDataExtension updates. Additionally, I've created a simple test framework for testing both nostr-mls storage adapters against the same tests to ensure correctness and consistency.

Pull-Request: #1056
Signed-off-by: Yuki Kishimoto <[email protected]>

Author: erskingardner

Cargo.lock

@@ -28,6 +28,7 @@
 ### Breaking changes
 
 - Remove group type from groups
+- Replaced `save_group_relay` with `replace_group_relays` trait method (https://github.com/rust-nostr/nostr/pull/1056)
 
 ### Changed
 

mls/nostr-mls-memory-storage/CHANGELOG.md

@@ -2,7 +2,7 @@
 
 use std::collections::BTreeSet;
 
-use nostr::PublicKey;
+use nostr::{PublicKey, RelayUrl};
 use nostr_mls_storage::groups::error::{GroupError, InvalidGroupState};
 use nostr_mls_storage::groups::types::*;
 use nostr_mls_storage::groups::GroupStorage;
@@ -72,37 +72,47 @@ impl GroupStorage for NostrMlsMemoryStorage {
 
     fn group_relays(&self, mls_group_id: &GroupId) -> Result<BTreeSet<GroupRelay>, GroupError> {
         // Check if the group exists first
-        self.find_group_by_mls_group_id(mls_group_id)?;
+        if self.find_group_by_mls_group_id(mls_group_id)?.is_none() {
+            return Err(GroupError::InvalidParameters(format!(
+                "Group with MLS ID {:?} not found",
+                mls_group_id
+            )));
+        }
 
         let cache = self.group_relays_cache.read();
         match cache.peek(mls_group_id).cloned() {
             Some(relays) => Ok(relays),
-            None => Err(GroupError::InvalidState(InvalidGroupState::NoRelays)),
+            // If not in cache but group exists, return empty set
+            None => Ok(BTreeSet::new()),
         }
     }
 
-    fn save_group_relay(&self, group_relay: GroupRelay) -> Result<(), GroupError> {
+    fn replace_group_relays(
+        &self,
+        group_id: &GroupId,
+        relays: BTreeSet<RelayUrl>,
+    ) -> Result<(), GroupError> {
         // Check if the group exists first
-        self.find_group_by_mls_group_id(&group_relay.mls_group_id)?;
+        if self.find_group_by_mls_group_id(group_id)?.is_none() {
+            return Err(GroupError::InvalidParameters(format!(
+                "Group with MLS ID {:?} not found",
+                group_id
+            )));
+        }
 
         let mut cache = self.group_relays_cache.write();
 
-        // Try to get the existing relays for the group
-        match cache.get_mut(&group_relay.mls_group_id) {
-            // If the group exists, add the new relay to the vector
-            Some(existing_relays) => {
-                // Add the new relay if it doesn't already exist
-                existing_relays.insert(group_relay);
-            }
-            // If the group doesn't exist, create a new vector with the new relay
-            None => {
-                // Update the cache with the new vector
-                cache.put(
-                    group_relay.mls_group_id.clone(),
-                    BTreeSet::from([group_relay]),
-                );
-            }
-        };
+        // Convert RelayUrl set to GroupRelay set
+        let group_relays: BTreeSet<GroupRelay> = relays
+            .into_iter()
+            .map(|relay_url| GroupRelay {
+                mls_group_id: group_id.clone(),
+                relay_url,
+            })
+            .collect();
+
+        // Replace the entire relay set for this group
+        cache.put(group_id.clone(), group_relays);
 
         Ok(())
     }

mls/nostr-mls-memory-storage/src/groups.rs

@@ -310,19 +310,9 @@ mod tests {
         nostr_storage.save_group(group.clone()).unwrap();
         let relay_url1 = RelayUrl::parse("wss://relay1.example.com").unwrap();
         let relay_url2 = RelayUrl::parse("wss://relay2.example.com").unwrap();
-        let group_relay1 = GroupRelay {
-            mls_group_id: mls_group_id.clone(),
-            relay_url: relay_url1,
-        };
-        let group_relay2 = GroupRelay {
-            mls_group_id: mls_group_id.clone(),
-            relay_url: relay_url2,
-        };
+        let relays = BTreeSet::from([relay_url1, relay_url2]);
         nostr_storage
-            .save_group_relay(group_relay1.clone())
-            .unwrap();
-        nostr_storage
-            .save_group_relay(group_relay2.clone())
+            .replace_group_relays(&mls_group_id, relays)
             .unwrap();
         let found_relays = nostr_storage.group_relays(&mls_group_id).unwrap();
         assert_eq!(found_relays.len(), 2);
@@ -337,11 +327,6 @@ mod tests {
                 panic!("Group relays not found in cache");
             }
         }
-        nostr_storage
-            .save_group_relay(group_relay1.clone())
-            .unwrap();
-        let found_relays_after_duplicate = nostr_storage.group_relays(&mls_group_id).unwrap();
-        assert_eq!(found_relays_after_duplicate.len(), 2);
     }
 
     #[test]

mls/nostr-mls-memory-storage/src/lib.rs

@@ -28,6 +28,7 @@
 ### Breaking changes
 
 - Remove group type from groups
+- Replaced `save_group_relay` with `replace_group_relays` trait method (https://github.com/rust-nostr/nostr/pull/1056)
 
 ### Changed
 

mls/nostr-mls-sqlite-storage/CHANGELOG.md

@@ -2,7 +2,7 @@
 
 use std::collections::BTreeSet;
 
-use nostr::PublicKey;
+use nostr::{PublicKey, RelayUrl};
 use nostr_mls_storage::groups::error::GroupError;
 use nostr_mls_storage::groups::types::{Group, GroupExporterSecret, GroupRelay};
 use nostr_mls_storage::groups::GroupStorage;
@@ -192,29 +192,42 @@ impl GroupStorage for NostrMlsSqliteStorage {
         Ok(relays)
     }
 
-    fn save_group_relay(&self, group_relay: GroupRelay) -> Result<(), GroupError> {
+    fn replace_group_relays(
+        &self,
+        group_id: &GroupId,
+        relays: BTreeSet<RelayUrl>,
+    ) -> Result<(), GroupError> {
         // First verify the group exists
-        if self
-            .find_group_by_mls_group_id(&group_relay.mls_group_id)?
-            .is_none()
-        {
+        if self.find_group_by_mls_group_id(group_id)?.is_none() {
             return Err(GroupError::InvalidParameters(format!(
                 "Group with MLS ID {:?} not found",
-                group_relay.mls_group_id
+                group_id
             )));
         }
 
         let conn_guard = self.db_connection.lock().map_err(into_group_err)?;
 
-        conn_guard
-            .execute(
-                "INSERT OR REPLACE INTO group_relays (mls_group_id, relay_url) VALUES (?, ?)",
-                params![
-                    group_relay.mls_group_id.as_slice(),
-                    group_relay.relay_url.as_str()
-                ],
+        // Use a transaction for atomicity
+        let tx = conn_guard.unchecked_transaction().map_err(into_group_err)?;
+
+        // Clear existing relays for this group
+        tx.execute(
+            "DELETE FROM group_relays WHERE mls_group_id = ?",
+            params![group_id.as_slice()],
+        )
+        .map_err(into_group_err)?;
+
+        // Insert new relays
+        for relay_url in relays {
+            tx.execute(
+                "INSERT INTO group_relays (mls_group_id, relay_url) VALUES (?, ?)",
+                params![group_id.as_slice(), relay_url.as_str()],
             )
             .map_err(into_group_err)?;
+        }
+
+        // Commit the transaction
+        tx.commit().map_err(into_group_err)?;
 
         Ok(())
     }
@@ -276,7 +289,6 @@ impl GroupStorage for NostrMlsSqliteStorage {
 mod tests {
     use aes_gcm::aead::OsRng;
     use aes_gcm::{Aes128Gcm, KeyInit};
-    use nostr::RelayUrl;
     use nostr_mls_storage::groups::types::GroupState;
 
     use super::*;
@@ -335,51 +347,8 @@ mod tests {
         assert_eq!(all_groups.len(), 1);
     }
 
-    #[test]
-    fn test_group_relay() {
-        let storage = NostrMlsSqliteStorage::new_in_memory().unwrap();
-
-        // Create a test group
-        let mls_group_id = GroupId::from_slice(&[1, 2, 3, 4]);
-        let mut nostr_group_id = [0u8; 32];
-        nostr_group_id[0..13].copy_from_slice(b"test_group_12");
-
-        let group = Group {
-            mls_group_id: mls_group_id.clone(),
-            nostr_group_id,
-            name: "Test Group".to_string(),
-            description: "A test group".to_string(),
-            admin_pubkeys: BTreeSet::new(),
-            last_message_id: None,
-            last_message_at: None,
-            epoch: 0,
-            state: GroupState::Active,
-            image_url: None,
-            image_key: None,
-            image_nonce: None,
-        };
-
-        // Save the group
-        storage.save_group(group).unwrap();
-
-        // Create a group relay
-        let relay_url = RelayUrl::parse("wss://relay.example.com").unwrap();
-        let group_relay = GroupRelay {
-            mls_group_id: mls_group_id.clone(),
-            relay_url,
-        };
-
-        // Save the group relay
-        storage.save_group_relay(group_relay).unwrap();
-
-        // Get group relays
-        let relays = storage.group_relays(&mls_group_id).unwrap();
-        assert_eq!(relays.len(), 1);
-        assert_eq!(
-            relays.first().unwrap().relay_url.to_string(),
-            "wss://relay.example.com"
-        );
-    }
+    // Note: Comprehensive storage functionality tests are now in nostr-mls-storage/tests/
+    // using shared test functions to ensure consistency between storage implementations
 
     #[test]
     fn test_group_exporter_secret() {

mls/nostr-mls-sqlite-storage/src/groups.rs

@@ -28,11 +28,17 @@
 ### Breaking changes
 
 - Remove group type from groups
+- Remove `save_group_relay` method (https://github.com/rust-nostr/nostr/pull/1056)
 
 ### Changed
 
 - Upgrade openmls to v0.7.0
 
+### Added
+
+- Added `replace_group_relays` to make relay replace for groups an atomic operation (https://github.com/rust-nostr/nostr/pull/1056)
+- Comprehensive consistency testing framework for testing all nostr-mls-storage implementations for correctness and consistency (https://github.com/rust-nostr/nostr/pull/1056)
+
 ## v0.43.0 - 2025/07/28
 
 No notable changes in this release.

mls/nostr-mls-storage/CHANGELOG.md

@@ -17,3 +17,8 @@ openmls = { version = "0.7.0", default-features = false }
 openmls_traits = { version = "0.4.0", default-features = false }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true, features = ["std"] }
+
+[dev-dependencies]
+nostr-mls-sqlite-storage = { version = "0.43", path = "../nostr-mls-sqlite-storage" }
+nostr-mls-memory-storage = { version = "0.43", path = "../nostr-mls-memory-storage" }
+openmls_memory_storage = "0.4"

mls/nostr-mls-storage/Cargo.toml

@@ -9,7 +9,7 @@
 
 use std::collections::BTreeSet;
 
-use nostr::PublicKey;
+use nostr::{PublicKey, RelayUrl};
 use openmls::group::GroupId;
 
 pub mod error;
@@ -45,8 +45,13 @@ pub trait GroupStorage {
     /// Get all relays for a group
     fn group_relays(&self, group_id: &GroupId) -> Result<BTreeSet<GroupRelay>, GroupError>;
 
-    /// Save a group relay
-    fn save_group_relay(&self, group_relay: GroupRelay) -> Result<(), GroupError>;
+    /// Replace all relays for a group with the provided set
+    /// This operation is atomic - either all relays are replaced or none are changed
+    fn replace_group_relays(
+        &self,
+        group_id: &GroupId,
+        relays: BTreeSet<RelayUrl>,
+    ) -> Result<(), GroupError>;
 
     /// Get an exporter secret for a group and epoch
     fn get_group_exporter_secret(

mls/nostr-mls-storage/src/groups/mod.rs

@@ -11,6 +11,8 @@ pub mod groups;
 pub mod messages;
 pub mod welcomes;
 
+pub mod test_utils;
+
 use self::groups::GroupStorage;
 use self::messages::MessageStorage;
 use self::welcomes::WelcomeStorage;