mantra/mantra-kmp
1
0
Fork 0
Code Issues 2 Pull Requests Actions 1 Packages Projects Releases 1 Wiki Activity

fix: keep a room's MlsGroup alive so a late message can still be read

Browse Source 
Two events published in the same second reliably lose one of them. The
receiver stores the kind:445 and produces nothing from it -- no inner
event, no chat line, no error anybody sees, because MarmotGroupEvent is
written before the message is decrypted and so survives while everything
downstream silently does not.

Observed as a FROST signing session that never started on the receiver:
proposeSigning publishes the proposal and then the proposer's own nonce,
the relay handed them back in the other order, and the proposal was
dropped. The nonce is still sitting there filed against a session that
will never exist. The same bug ate a dialect earlier, which then took out
the artifact referencing it via a foreign key.

MLS is specified to tolerate this. RFC 9420 says a receiver that gets
generation N+1 before N keeps the intermediate keys so the older message
can still be read, and quartz's SecretTree does exactly that, in a
private skippedKeys map. What it does not do is persist it:
exportSenderStates() returns the ratchet positions only, so saveState()
drops the cache. NostrDao rebuilt the group from stored state for every
inbound event, so the cache was empty every single time, and generation N
arriving after N+1 failed `require(generation >= applicationGeneration)`
and was swallowed. Terminal -- the key is derived from a ratchet that has
moved past it, and nothing asks the sender to resend.

This keeps the instance alive instead. MlsGroupCache holds one MlsGroup
per room, and the inbound path goes through it, so skippedKeys survives
from one message to the next. That covers the case that actually bites --
a burst arriving in one sync, decrypted one after another against the
same tree -- which is what every bursty flow needs: proposeRitual sends
two, addArtifact sends two, and addChapter sends one per paragraph plus
one, of which only the ones arriving in ascending generation order
survived.

Reuse is conditional on the stored state still being exactly what the
cache last wrote. Sending a message advances the sender ratchet and saves;
so does adding a member. When that happens the cache rebuilds rather than
carrying on from a group that has been overtaken -- which is what keeps
this from being worse than no cache at all: the fallback is always the old
behaviour, never a diverged ratchet.

One lock per room, not one overall, because the group is mutable and
decryption advances it: two events for the same room decrypted at once
would corrupt the tree, and a busy room should not hold up a quiet one.

**This is a mitigation, not the fix.** It does not survive a restart, and
it does not survive another writer, so a long enough reorder still loses
the message. The fix belongs in quartz -- carry skippedKeys through
saveState/restore -- and quartz is a mavenCentral binary, not a fork, so
it cannot be made here. docs/mls-skipped-keys.md has the analysis, the
patch, the migration constraint on the persisted state format, and the
three ways to actually land it.

Not verified end to end: the proposal that exposed this cannot be
recovered, since its generation is already past, so confirming the fix
needs a fresh burst.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
2026-09-05 23:09:53 +02:00
parent 3e4166f13d
commit d110737f9a
4 changed files with 339 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/NostrDao.kt
View File

@@ -33,6 +33,7 @@ import press.mantra.compose.managers.ChillDkgRitualManager
import press.mantra.compose.nostr.dkg.DkgRitualEvents
import press.mantra.compose.nostr.frost.FrostSigningEvents
import press.mantra.compose.managers.FrostSigningManager
import press.mantra.compose.managers.MlsGroupCache
import press.mantra.compose.managers.MarmotInboundManager
import co.touchlab.kermit.Logger
import kotlinx.coroutines.CancellationException
@@ -388,9 +389,21 @@ abstract class NostrDao(
val localChatRoom = database.chatRoomDao().findChatRoomById(chatRoomId)
if (localChatRoom != null) {
val mlsGroup = localChatRoom.chatRoom.toMlsGroup()
if (mlsGroup != null) {
// Through the cache rather than rebuilt here, so the secret
// tree's skipped-generation keys survive from one message to
// the next. Two events published in the same instant arrive in
// whatever order the relay feels like, and rebuilding between
// them loses the earlier one for good -- see MlsGroupCache.
val handled = MlsGroupCache.withGroup(
chatRoomId = chatRoomId,
storedStateHex = localChatRoom.chatRoom.mlsGroupState,
build = { localChatRoom.chatRoom.toMlsGroup() },
save = { stateHex ->
database.chatRoomDao().upsert(
localChatRoom.chatRoom.copy(mlsGroupState = stateHex)
)
}
) { mlsGroup ->
val memberPubkeys = mlsGroup.members().mapNotNull { (leafIndex, leafNode) ->
val pubkey = when (val cred = leafNode.credential) {
@@ -437,12 +450,6 @@ abstract class NostrDao(
}
}
// Save the mls chatRoom state...
database.chatRoomDao().upsert(
localChatRoom.chatRoom.copy(
mlsGroupState = mlsGroup.saveState().encodeTls().toHex()
)
)
ChatMessage.fromGroupEventResult(
database = database,
activeKeyPair = activeKeyPair,
@@ -477,7 +484,11 @@ abstract class NostrDao(
} else {
throw MarmotNotMemberOfChatGroupException("We are not a member of the chat room ${localChatRoom.chatRoom.id}")
}
} else {
}
// Null means the room has no usable group state, which is what
// a failed toMlsGroup() meant before the cache existed.
if (handled == null) {
throw MarmotMissingNostrGroupDataExtension("Couldn't find chatRoom for $nostrEvent")
}
} else {

121
composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt Normal file
View File

@@ -0,0 +1,121 @@
package press.mantra.compose.managers
import co.touchlab.kermit.Logger
import com.vitorpamplona.quartz.marmot.mls.group.MlsGroup
import press.mantra.compose.extensions.toHex
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
/**
* Keeps a room's [MlsGroup] alive between messages instead of rebuilding it
* from the stored state every time.
*
* ### The bug this exists for
*
* MLS is specified to tolerate out-of-order delivery within an epoch: a
* receiver that gets generation N+1 before N derives and caches the key for N
* so the older message can still be read when it turns up. Quartz's
* `SecretTree` does exactly that, in a private `skippedKeys` map.
*
* `SecretTree.exportSenderStates()` does not include that map, so
* `MlsGroup.saveState()` does not carry it. Rebuilding the group from stored
* state therefore throws the skipped keys away, and a message for a generation
* the ratchet has already passed fails
* `require(generation >= state.applicationGeneration)` and is dropped. There is
* no recovering it afterwards: the key is gone and the sender will not resend.
*
* Nostr relays offer no ordering whatsoever, so this is not an edge case. Two
* events published in the same second race, and exactly one survives — which is
* how a signing session's proposal was lost while the nonce sent immediately
* behind it arrived fine.
*
* ### What this fixes, and what it does not
*
* Holding the instance means `skippedKeys` survives for as long as the process
* does and nothing else writes the room's state. That covers the case that
* actually bites — a burst of messages arriving in one sync — because they are
* decrypted one after another against the same tree.
*
* It does not survive a restart, and it does not survive another writer, so
* reordering across app launches still loses messages. The real fix is for
* `exportSenderStates` to carry the skipped keys; see
* `docs/mls-skipped-keys.md`.
*
* ### Staleness
*
* The group is only reused when the stored state is still exactly what this
* cache last wrote. Anything else that saves a room's state — sending a message
* advances the sender ratchet and saves, so does adding a member — changes the
* hex, and the next read rebuilds rather than carrying on from a group that has
* been overtaken. Losing the skipped keys there is the same behaviour as
* before this existed, so the fallback is never worse than not caching.
*/
object MlsGroupCache {
private const val TAG = "MlsGroupCache"
private val logger = Logger.withTag(TAG)
private class Entry(
val group: MlsGroup,
/** The state hex this cache last wrote, for spotting another writer. */
var stateHex: String,
)
private val entries = mutableMapOf<String, Entry>()
/**
* Serialises use of one room's group.
*
* The group is mutable and decryption advances it, so two events for the
* same room being decrypted at once would corrupt the ratchet. One lock per
* room rather than one overall, so a busy room cannot hold up a quiet one.
*
* Held across database work, which is safe here because a caller only ever
* takes this lock while it is already running -- it never waits on a
* resource the holder is waiting for.
*/
private val locks = mutableMapOf<String, Mutex>()
private val locksGuard = Mutex()
private suspend fun lockFor(chatRoomId: String): Mutex =
locksGuard.withLock { locks.getOrPut(chatRoomId) { Mutex() } }
/**
* Runs [block] against the room's live group, then stores whatever state it
* left behind.
*
* [storedStateHex] is the room's state as the database currently has it, and
* [build] turns it into a group. [save] is handed the state to persist; it
* runs inside the lock so the stored state and the cached instance cannot
* disagree.
*
* Returns null without calling [block] when the room has no usable group
* state, which is the same thing a failed `toMlsGroup()` meant before.
*/
suspend fun <T> withGroup(
chatRoomId: String,
storedStateHex: String?,
build: () -> MlsGroup?,
save: suspend (String) -> Unit,
block: suspend (MlsGroup) -> T,
): T? = lockFor(chatRoomId).withLock {
val cached = entries[chatRoomId]
val group = if (cached != null && cached.stateHex == storedStateHex) {
cached.group
} else {
if (cached != null) {
logger.d("Room $chatRoomId was written elsewhere; rebuilding its group")
}
build() ?: return@withLock null
}
val result = block(group)
val stateHex = group.saveState().encodeTls().toHex()
save(stateHex)
entries[chatRoomId] = Entry(group = group, stateHex = stateHex)
result
}
}