diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt index 35da1278..ca3413e3 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt @@ -172,6 +172,148 @@ abstract class MarmotOutboundDao( ) } + /** + * Adds every member in [peers] to a group in a single MLS commit. + * + * The shape to use whenever the whole membership is known up front -- creating a + * room, or opening the #admins room after a key ceremony. One commit, one + * Welcome carrying an EncryptedGroupSecrets per joiner, and crucially **no + * intermediate epoch for anybody to miss**. + * + * Inviting one at a time creates an epoch per member, and each of those commits + * races the previous member's Welcome. MarmotInboundManager refuses future-epoch + * messages outright -- no queue, no replay -- so a member who loses that race is + * silently stuck an epoch behind while the caller sees a successful invite. + * Batching removes the race rather than narrowing it, because no member ever has + * to process a commit for an epoch they were not yet in. + * + * Only batches when the group is still just its creator, which is exactly the + * case this exists for. Adding a batch to a group that already has members would + * take the deferred-welcome path, and MarmotCommitResult records one peer per + * commit -- so that case falls back to inviting sequentially, which is correct + * if not ideal. Batching there needs that model to hold a list first; see + * docs/marmot-membership.md. + * + * @return the members that could not be added, in the order given. + */ + @Transaction + open suspend fun addMembersToChatRoom( + localChatRoom: LocalChatRoom, + peers: List> + ): List { + if (peers.isEmpty()) return emptyList() + + val mlsGroup = localChatRoom.chatRoom.toMlsGroup() + ?: throw MarmotMissingChatGroupException( + "No MLS state for chat room ${localChatRoom.chatRoom.id}; cannot add members" + ) + + // A group that already has members cannot take this path -- see the kdoc. + if (mlsGroup.members().size != 1) { + val notAdded = mutableListOf() + + peers.forEach { (peerPublicKey, peerKeyPackage) -> + // Re-read between invites: each advances the epoch and persists new + // state, so a snapshot from before the previous one would build this + // commit on state the group has already left. + val current = database.chatRoomDao().findChatRoomById(localChatRoom.chatRoom.id) + if (current == null) { + notAdded.add(peerPublicKey) + return@forEach + } + + runCatching { + inviteMemberToChatRoom(current, peerPublicKey, peerKeyPackage) + }.onFailure { + logger.e("Failed to invite $peerPublicKey to ${localChatRoom.chatRoom.id}", it) + notAdded.add(peerPublicKey) + } + } + + return notAdded + } + + val relays = Relays.DefaultDMRelayList.map { it.url } + + // Participant rows must exist before any Welcome is sealed: + // sealGiftWrapPayload walks the room's participants to decide who to wrap + // for, so without these the Welcomes produce no gift wraps and sit unsealed. + database.participantDao().upsert( + peers.map { (peerPublicKey, _) -> + Participant( + participantPublicKey = peerPublicKey, + chatRoomId = localChatRoom.chatRoom.id, + relayHint = relays.first() + ) + } + ) + + peers.forEach { (peerPublicKey, peerKeyPackage) -> + val keyPackage = MlsKeyPackage.decodeTls(TlsReader(peerKeyPackage.tlsEncodedMarmotKeyPackage)) + + val credential = keyPackage.leafNode.credential + require(credential is Credential.Basic) { "KeyPackage must use BasicCredential" } + require(credential.identity.toHexKey() == peerPublicKey) { + "KeyPackage credential identity does not match memberPubKey" + } + + mlsGroup.proposeAdd(peerKeyPackage.tlsEncodedMarmotKeyPackage) + } + + val retainedBefore = mlsGroup.retainedSecrets() + val commitResult = mlsGroup.commit() + + database.marmotRetainedEpochSecretDao().insert( + MarmotRetainedEpochSecret( + chatRoomId = localChatRoom.chatRoom.id, + epoch = retainedBefore.epoch, + senderDataSecret = retainedBefore.senderDataSecret, + encryptionSecret = retainedBefore.encryptionSecret, + leafCount = retainedBefore.leafCount, + exporterSecret = retainedBefore.exporterSecret, + ) + ) + + // `commit` advanced the in-memory group. Persist before delivering, so a + // crash between the two leaves the group at the epoch the Welcomes describe + // rather than one behind it. + database.chatRoomDao().upsert( + localChatRoom.chatRoom.copy( + mlsGroupState = mlsGroup.saveState().encodeTls().toHex() + ) + ) + + val welcomeBytes = commitResult.welcomeBytes + if (welcomeBytes == null) { + logger.e("Batched commit for ${localChatRoom.chatRoom.id} produced no welcome") + return peers.map { it.first } + } + + // No commit is broadcast: the group was only its creator, so there is nobody + // to inform. The same Welcome blob goes to every joiner -- each finds its own + // EncryptedGroupSecrets entry by key package reference -- but delivery is per + // peer, because each Welcome event is tagged with that peer's key package. + val notAdded = mutableListOf() + + peers.forEach { (peerPublicKey, peerKeyPackage) -> + runCatching { + deliveryWelcome( + nostrGroupId = localChatRoom.chatRoom.id, + userPublicKey = localChatRoom.chatRoom.userPublicKey, + welcomeBytes = welcomeBytes, + peerKeyPackageEventId = peerKeyPackage.id, + relays = relays, + createdAt = Clock.System.now() + ) + }.onFailure { + logger.e("Failed to deliver the welcome to $peerPublicKey", it) + notAdded.add(peerPublicKey) + } + } + + return notAdded + } + private suspend fun inviteMember( nostrGroupId: HexKey, mlsGroup: MlsGroup, diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt index 1f0e7364..b12e3723 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt @@ -162,6 +162,14 @@ class DatabaseChatRepository( peerKeyPackage = peerKeyPackage ) } + override suspend fun addMembers( + localChatRoom: LocalChatRoom, + peers: List> + ): List = database.marmotOutboundDao().addMembersToChatRoom( + localChatRoom = localChatRoom, + peers = peers + ) + override suspend fun sendChatMessage( text: String, localChatRoom: LocalChatRoom, diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt index 2d74f0d2..4669fae9 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt @@ -67,6 +67,18 @@ interface ChatRepository { peerKeyPackage: MarmotKeyPackage, ) + /** + * Adds every member in one MLS commit, returning those that could not be added. + * + * Prefer this to looping [inviteMember] whenever the membership is known up + * front: one commit means no intermediate epoch for a member to miss, which is + * a silent failure rather than a loud one. See docs/marmot-membership.md. + */ + suspend fun addMembers( + localChatRoom: LocalChatRoom, + peers: List>, + ): List + suspend fun sendChatMessage( text: String, localChatRoom: LocalChatRoom, @@ -171,6 +183,12 @@ interface ChatRepository { TODO("Not yet implemented") } + override suspend fun addMembers( + localChatRoom: LocalChatRoom, + peers: List> + ): List = peers.map { it.first } + + override suspend fun sendChatMessage( text: String, localChatRoom: LocalChatRoom, diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt index d61750cc..0b28f8ae 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt @@ -359,6 +359,20 @@ class DkgRitualViewModel( * reported rather than treated as failure: a room with most of the group in it * is more useful than no room. */ + /** + * Adds every admin to the freshly created room in one commit, returning those + * that could not be added. + * + * Batched rather than invited one at a time: the whole membership is known here, + * and inviting sequentially creates an epoch per member, each of whose commits + * races the previous member's Welcome. A member who loses that race is silently + * stuck an epoch behind. See docs/marmot-membership.md. + * + * A Marmot invite still needs the invitee's published key package, so a member + * who has never published one cannot be added and has to be invited later. That + * is reported rather than treated as failure: a room with most of the group in + * it is more useful than no room. + */ private suspend fun inviteAdmins(groupId: String, peers: List): List { val keyPackages = coroutineScope { peers.map { publicKey -> @@ -372,38 +386,27 @@ class DkgRitualViewModel( }.awaitAll() } - val notAdded = mutableListOf() + val withoutKeyPackage = keyPackages.filter { it.second == null }.map { it.first } + withoutKeyPackage.forEach { logger.w("No key package for $it; leaving them out of $groupId") } - keyPackages.forEach { (publicKey, keyPackage) -> - if (keyPackage == null) { - logger.w("No key package for $publicKey; leaving them out of $groupId") - notAdded.add(publicKey) - return@forEach - } + val addable = keyPackages.mapNotNull { (publicKey, keyPackage) -> + keyPackage?.let { publicKey to it } + } + if (addable.isEmpty()) return withoutKeyPackage - // Re-read between invites: each one advances the MLS epoch and persists - // new state, so a snapshot taken before the previous invite would build - // this commit on state the group has already left. - val localChatRoom = chatRepository.getChatRoomByIdentifier(groupId) - if (localChatRoom == null) { - logger.e("Admin group $groupId disappeared mid-invite") - notAdded.add(publicKey) - return@forEach - } - - runCatching { - chatRepository.inviteMember( - localChatRoom = localChatRoom, - peerPublicKey = publicKey, - peerKeyPackage = keyPackage - ) - }.onFailure { - logger.e("Failed to invite $publicKey to admin group $groupId", it) - notAdded.add(publicKey) - } + val localChatRoom = chatRepository.getChatRoomByIdentifier(groupId) + if (localChatRoom == null) { + logger.e("Admin group $groupId disappeared before its members were added") + return peers } - return notAdded + val notAdded = runCatching { + chatRepository.addMembers(localChatRoom = localChatRoom, peers = addable) + }.onFailure { + logger.e("Failed to add members to admin group $groupId", it) + }.getOrElse { addable.map { (publicKey, _) -> publicKey } } + + return withoutKeyPackage + notAdded } override fun onCleared() { diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectChatRoomTypeViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectChatRoomTypeViewModel.kt index 7713c3df..038cd51f 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectChatRoomTypeViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectChatRoomTypeViewModel.kt @@ -348,38 +348,32 @@ class SelectChatRoomTypeViewModel( }.awaitAll() } - val notAdded = mutableListOf() + val withoutKeyPackage = keyPackages.filter { it.second == null }.map { it.first } + withoutKeyPackage.forEach { logger.w("No key package for $it; leaving them out of $chatRoomId") } - keyPackages.forEach { (publicKey, marmotKeyPackage) -> - if (marmotKeyPackage == null) { - logger.w("No key package for $publicKey; leaving them out of $chatRoomId") - notAdded.add(publicKey) - return@forEach - } + val addable = keyPackages.mapNotNull { (publicKey, keyPackage) -> + keyPackage?.let { publicKey to it } + } + if (addable.isEmpty()) return withoutKeyPackage - // Re-read the room before each invite: `inviteMember` advances the MLS epoch and - // persists the new state, so reusing the snapshot taken before the previous - // invite would build this commit on top of state the group has already left. - val localChatRoom = chatRepository.getChatRoomByIdentifier(chatRoomId) - if (localChatRoom == null) { - logger.e("Chat room $chatRoomId disappeared mid-invite") - notAdded.add(publicKey) - return@forEach - } - - runCatching { - chatRepository.inviteMember( - localChatRoom = localChatRoom, - peerPublicKey = publicKey, - peerKeyPackage = marmotKeyPackage - ) - }.onFailure { throwable -> - logger.e("Failed to invite $publicKey to $chatRoomId", throwable) - notAdded.add(publicKey) - } + // One commit for the whole membership rather than one per member. Inviting + // sequentially creates an epoch each, and each commit races the previous + // member's Welcome -- a race MarmotInboundManager resolves by dropping the + // commit outright, leaving that member an epoch behind while this reports + // success. See docs/marmot-membership.md. + val localChatRoom = chatRepository.getChatRoomByIdentifier(chatRoomId) + if (localChatRoom == null) { + logger.e("Chat room $chatRoomId disappeared before its members were added") + return memberPublicKeys } - return notAdded + val notAdded = runCatching { + chatRepository.addMembers(localChatRoom = localChatRoom, peers = addable) + }.onFailure { + logger.e("Failed to add members to $chatRoomId", it) + }.getOrElse { addable.map { (publicKey, _) -> publicKey } } + + return withoutKeyPackage + notAdded } companion object { diff --git a/docs/marmot-membership.md b/docs/marmot-membership.md index 99c6b025..c6feeb21 100644 --- a/docs/marmot-membership.md +++ b/docs/marmot-membership.md @@ -71,11 +71,12 @@ before Welcome 1 did — different transports, no ordering guarantee, one a gift wrap and the other a kind:445 — admin 1 dropped it and was stuck an epoch behind, while the coordinator saw two successful invites. -Deriving the flag narrows this: Welcome 1 is now sent before commit 2 exists at -all, so admin 1 is already at epoch 1 when it arrives. +Deriving the flag narrowed this, but did not close it: for n ≥ 3 the window +between Welcome 1 and commit 2 remained. -**It does not close it.** For n ≥ 3 the window between Welcome 1 and commit 2 -remains, and losing it is still silent. See "Batching every add" below. +**Batching closes it.** When the membership is known up front, every member goes +into one commit, so no member ever has to process a commit for an epoch they were +not yet in — the race has nothing left to lose. See below. ## The condition holds at any group size @@ -94,32 +95,48 @@ advance. ## Batching every add into one commit -This is the remaining work, and the only thing that closes the race rather than -narrowing it. +`MarmotOutboundDao.addMembersToChatRoom` stages every member with `proposeAdd` and +issues a single `commit()`. `MlsGroup.addMember` is just those two in one call, and +`pendingProposals` is a list, so nothing in MLS objected. -`MlsGroup.addMember` is `proposeAdd` + `commit()` in one call, but those are -separate functions and `pendingProposals` is a list. Staging every member with -`proposeAdd` and issuing a single `commit()` gives: +One commit produces **one** Welcome: `buildWelcome` emits an `EncryptedGroupSecrets` +per added member, and each joiner finds its own entry by key package reference. The +blob is shared; delivery is still per peer, because each Welcome event is tagged +with that peer's key package. -- one commit, which nobody has to have already joined to process -- n Welcomes carrying identical state -- no intermediate epoch for anyone to miss, so the race has nothing to lose +Both callers that know their membership up front now use it — +`SelectChatRoomTypeViewModel.inviteMembers` at room creation, and +`DkgRitualViewModel.inviteAdmins` for the `#admins` room. -This is the right shape whenever the whole membership is known up front, which is -exactly the case for a room created from a completed key ceremony. +### Why this needed no schema change -The cost is bookkeeping. `MarmotCommitResult` assumes one peer per commit — -`peerKeyPackageEventId` is singular — so batching means changing that model and -the ack-triggered fan-out in `DatabaseNostrRepository` to deliver several Welcomes -from one acknowledgement. +Batching at creation time means the single commit happens while the group is still +only its creator. That takes the immediate-Welcome branch: no commit is broadcast, +and `MarmotCommitResult` is never written. The bookkeeping that assumes one peer per +commit is simply not on the path. + +So `addMembersToChatRoom` batches **only** when `members().size == 1`, and falls +back to inviting sequentially otherwise. Batching into a group that already has +members would take the deferred branch, where `MarmotCommitResult.peerKeyPackageEventId` +is singular and `DatabaseNostrRepository`'s ack-triggered delivery expects one +Welcome. Making that work means holding a list of peers there and fanning out on +acknowledgement — still outstanding, and only needed for adding several members to +an established group, which nothing currently does. + +### Ordering within the batch + +The group state is persisted after `commit()` and before any Welcome is delivered, +so a crash between the two leaves the group at the epoch the Welcomes describe +rather than one behind it. ## Other things that bite -**Invites are sequential and each advances the epoch.** The room must be re-read -from the database between invites; a snapshot taken before the previous invite -builds its commit on state the group has already left. Both `inviteMembers` and -`inviteAdmins` do this, and both say so in a comment, because it is not obvious -and the symptom is a conflicting commit rather than an error. +**Sequential invites each advance the epoch.** Where they still happen — the +fallback in `addMembersToChatRoom` for a group that already has members, and any +direct `inviteMember` call — the room must be re-read from the database between +them. A snapshot taken before the previous invite builds its commit on state the +group has already left, and the symptom is a conflicting commit rather than an +error. **A member with no published key package cannot be added.** A Marmot invite needs the invitee's `MarmotKeyPackage`. Both call sites look it up with a timeout and