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 ca3413e3..caad421b 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 @@ -435,17 +435,13 @@ abstract class MarmotOutboundDao( // Save commitResult... in case we need to broadcast welcomeEvent after relay acknowledgement... database.marmotCommitResultDao().upsert( - MarmotCommitResult( - id = commitEvent.id, - isOneMemberInitialGroupCreation = isOneMemberInitialGroupCreation, + MarmotCommitResult.from( + commitEventId = commitEvent.id, + commitResult = commitResult, chatRoomId = nostrGroupId, - commitBytes = commitResult.commitBytes, - preCommitExporterSecret = commitResult.preCommitExporterSecret, - welcomeBytes = commitResult.welcomeBytes, - framedCommitBytes = commitResult.preCommitExporterSecret, - groupInfoBytes = commitResult.groupInfoBytes, userPublicKey = userPublicKey, peerKeyPackageEventId = peerKeyPackage.id, + isOneMemberInitialGroupCreation = isOneMemberInitialGroupCreation, createdAt = Instant.fromEpochSeconds(commitEvent.createdAt) ) ) diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/NostrDao.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/NostrDao.kt index 26a35736..6dc36dfd 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/NostrDao.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/NostrDao.kt @@ -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 { diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/MarmotCommitResult.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/MarmotCommitResult.kt index 160a7a5c..ef749688 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/MarmotCommitResult.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/MarmotCommitResult.kt @@ -8,6 +8,7 @@ import press.mantra.compose.database.model.traits.SoftDeletableEntity import press.mantra.compose.database.model.traits.TimestampedEntity import press.mantra.compose.database.model.traits.UserViewableEntity import co.touchlab.kermit.Logger +import com.vitorpamplona.quartz.marmot.mls.messages.CommitResult import com.vitorpamplona.quartz.nip01Core.core.HexKey import kotlin.time.Clock import kotlin.time.Instant @@ -72,6 +73,40 @@ data class MarmotCommitResult( // TODO: Rename this to GiftWrapPayload... companion object { const val TAG = "MarmotCommitResult" + /** + * The persisted record of a commit, built from the [CommitResult] that produced it. + * + * The five payload fields are carried over from quartz verbatim -- same names, same + * order, same `ByteArray` type on both sides of the copy -- so a value taken from the + * wrong field of the right object typechecks and reaches the database unnoticed. + * `framedCommitBytes = commitResult.preCommitExporterSecret` survived exactly that way, + * storing the group's pre-commit exporter secret in the column documented to hold a + * broadcastable MLS envelope. + * + * Mapping here rather than at the call site means it is written once, in declaration + * order, and pinned by MarmotCommitResultMappingTest. + */ + fun from( + commitEventId: HexKey, + commitResult: CommitResult, + chatRoomId: HexKey, + userPublicKey: HexKey, + peerKeyPackageEventId: HexKey, + isOneMemberInitialGroupCreation: Boolean, + createdAt: Instant, + ): MarmotCommitResult = MarmotCommitResult( + id = commitEventId, + userPublicKey = userPublicKey, + peerKeyPackageEventId = peerKeyPackageEventId, + chatRoomId = chatRoomId, + isOneMemberInitialGroupCreation = isOneMemberInitialGroupCreation, + commitBytes = commitResult.commitBytes, + welcomeBytes = commitResult.welcomeBytes, + groupInfoBytes = commitResult.groupInfoBytes, + framedCommitBytes = commitResult.framedCommitBytes, + preCommitExporterSecret = commitResult.preCommitExporterSecret, + createdAt = createdAt, + ) } override fun equals(other: Any?): Boolean { diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt new file mode 100644 index 00000000..efe838a1 --- /dev/null +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt @@ -0,0 +1,152 @@ +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 val cache = LiveInstanceCache { it.saveState().encodeTls().toHex() } + + /** + * 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. + * + * 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 withGroup( + chatRoomId: String, + storedStateHex: String?, + build: () -> MlsGroup?, + save: suspend (String) -> Unit, + block: suspend (MlsGroup) -> T, + ): T? = cache.withInstance( + key = chatRoomId, + storedState = storedStateHex, + build = build, + save = save, + block = block + ) +} + +/** + * One live instance per key, reused only while the stored state is still the one + * this cache last wrote. + * + * Split out from [MlsGroupCache] so the decision it makes can be tested without + * standing up an MLS group. That decision is the whole safety argument: reuse + * when nothing else has written, rebuild when something has, and never carry on + * with an instance whose last use failed part-way through. + */ +internal class LiveInstanceCache( + /** The persisted form of an instance, for spotting another writer. */ + private val stateOf: (T) -> String, +) { + private val logger = Logger.withTag("LiveInstanceCache") + + private class Entry(val instance: T, val state: String) + + private val entries = mutableMapOf>() + + /** + * Serialises use of one key's instance. + * + * The instance is mutable and [block] advances it, so two callers running at + * once would corrupt it. One lock per key rather than one overall, so a busy + * key cannot hold up a quiet one. + * + * Held across [block], which may touch the database. Safe here because a + * caller only ever takes this lock while it is already running -- it never + * waits on a resource the holder is itself waiting for. + */ + private val locks = mutableMapOf() + private val locksGuard = Mutex() + + private suspend fun lockFor(key: String): Mutex = + locksGuard.withLock { locks.getOrPut(key) { Mutex() } } + + suspend fun withInstance( + key: String, + storedState: String?, + build: () -> T?, + save: suspend (String) -> Unit, + block: suspend (T) -> R, + ): R? = lockFor(key).withLock { + val cached = entries[key] + + val instance = if (cached != null && cached.state == storedState) { + cached.instance + } else { + if (cached != null) { + logger.d("$key was written elsewhere; rebuilding") + } + // Dropped before the block runs, so a build that fails does not leave + // the old instance behind to be picked up by the next caller. + entries.remove(key) + build() ?: return@withLock null + } + + // Deliberately not in a finally: an instance whose use threw part-way is + // in an unknown state, and the next caller should rebuild from whatever + // was last persisted rather than carry on with it. + val result = block(instance) + + val state = stateOf(instance) + save(state) + entries[key] = Entry(instance = instance, state = state) + + result + } + + /** How many instances are held. For tests. */ + internal fun size(): Int = entries.size +} diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/MarmotCommitResultMappingTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/MarmotCommitResultMappingTest.kt new file mode 100644 index 00000000..74cd329e --- /dev/null +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/MarmotCommitResultMappingTest.kt @@ -0,0 +1,134 @@ +package press.mantra.compose.database.model + +import com.vitorpamplona.quartz.marmot.mls.messages.CommitResult +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.time.Instant + +/** + * Where a commit's bytes land when the row that records it is written. + * + * `MarmotCommitResult` carries quartz's `CommitResult` payload fields verbatim -- + * `commitBytes`, `welcomeBytes`, `groupInfoBytes`, `framedCommitBytes`, + * `preCommitExporterSecret`, the same names and all of them `ByteArray`. A value + * taken from the wrong field of the right object therefore typechecks, and + * `framedCommitBytes = commitResult.preCommitExporterSecret` reached the database + * that way and sat there unnoticed: the column documented to hold a broadcastable + * `MlsMessage(PublicMessage(FramedContent(commit)))` envelope held 32 bytes of the + * group's pre-commit exporter secret instead. + * + * Nothing caught it because nothing read the column. The bytes that reached the + * relay come off the in-memory `CommitResult`, so the wire stayed correct while the + * record of it did not, and the row is written precisely so that the + * acknowledgement path in `DatabaseNostrRepository` can pick work back up later. A + * rebroadcast reading `framedCommitBytes` would have published noise the group + * decrypts, fails to parse, and drops -- silent, which is this subsystem's + * characteristic failure. + * + * So the routing is pinned here. Every payload gets a distinct, self-identifying + * value: a field that ends up in the wrong column names both halves of the mistake + * when it fails, rather than comparing equal by accident. + */ +class MarmotCommitResultMappingTest { + private val commitBytes = "raw-commit".encodeToByteArray() + private val framedCommitBytes = "framed-commit-envelope".encodeToByteArray() + private val welcomeBytes = "welcome".encodeToByteArray() + private val groupInfoBytes = "group-info".encodeToByteArray() + + /** Stands in for `MLS-Exporter("marmot", "group-event", 32)` at the pre-commit epoch. */ + private val preCommitExporterSecret = ByteArray(32) { 0x5E } + + private val commitEventId = "a".repeat(64) + private val chatRoomId = "b".repeat(64) + private val userPublicKey = "c".repeat(64) + private val peerKeyPackageEventId = "d".repeat(64) + private val createdAt = Instant.fromEpochSeconds(1_700_000_000) + + private fun commitResult( + framedCommitBytes: ByteArray = this.framedCommitBytes, + preCommitExporterSecret: ByteArray = this.preCommitExporterSecret, + ) = CommitResult( + commitBytes = commitBytes, + welcomeBytes = welcomeBytes, + groupInfoBytes = groupInfoBytes, + framedCommitBytes = framedCommitBytes, + preCommitExporterSecret = preCommitExporterSecret, + ) + + private fun map(commitResult: CommitResult) = MarmotCommitResult.from( + commitEventId = commitEventId, + commitResult = commitResult, + chatRoomId = chatRoomId, + userPublicKey = userPublicKey, + peerKeyPackageEventId = peerKeyPackageEventId, + isOneMemberInitialGroupCreation = false, + createdAt = createdAt, + ) + + @Test + fun `every payload field lands in its own column`() { + val row = map(commitResult()) + + assertContentEquals(commitBytes, row.commitBytes, "commitBytes") + assertContentEquals(welcomeBytes, row.welcomeBytes, "welcomeBytes") + assertContentEquals(groupInfoBytes, row.groupInfoBytes, "groupInfoBytes") + assertContentEquals(framedCommitBytes, row.framedCommitBytes, "framedCommitBytes") + assertContentEquals( + preCommitExporterSecret, + row.preCommitExporterSecret, + "preCommitExporterSecret" + ) + } + + @Test + fun `the framed commit column never holds the exporter secret`() { + // The regression. Stated as the invariant rather than as an equality check, + // so it keeps holding for a CommitResult this test did not anticipate. + val row = map(commitResult()) + + assertFalse( + row.framedCommitBytes.contentEquals(row.preCommitExporterSecret), + "the group's exporter secret was stored as the framed commit" + ) + } + + @Test + fun `a CommitResult that never framed its commit still stores a commit`() { + // quartz defaults framedCommitBytes to commitBytes, and the entity repeats that + // default. Whichever of the two a row ends up with, it must be a commit -- the + // fallback must not quietly become the secret either. + val unframed = CommitResult( + commitBytes = commitBytes, + welcomeBytes = welcomeBytes, + groupInfoBytes = groupInfoBytes, + preCommitExporterSecret = preCommitExporterSecret, + ) + + val row = map(unframed) + + assertContentEquals(commitBytes, row.framedCommitBytes) + assertFalse( + row.framedCommitBytes.contentEquals(row.preCommitExporterSecret), + "the group's exporter secret was stored as the framed commit" + ) + } + + @Test + fun `the bookkeeping the acknowledgement path reads is carried through`() { + // DatabaseNostrRepository finds this row by the commit event's id and delivers the + // welcome using chatRoomId, userPublicKey and peerKeyPackageEventId. All four are + // supplied by the caller rather than the CommitResult, so they are checked here to + // keep the argument order of `from` honest -- every one of them is a 64-char hex + // string, and swapping two would otherwise typecheck as silently as the bug did. + val row = map(commitResult()) + + assertEquals(commitEventId, row.id) + assertEquals(chatRoomId, row.chatRoomId) + assertEquals(userPublicKey, row.userPublicKey) + assertEquals(peerKeyPackageEventId, row.peerKeyPackageEventId) + assertEquals(createdAt, row.createdAt) + assertFalse(row.isOneMemberInitialGroupCreation) + } +} diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/RumorIdAgreementTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/RumorIdAgreementTest.kt new file mode 100644 index 00000000..ea0437ab --- /dev/null +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/RumorIdAgreementTest.kt @@ -0,0 +1,174 @@ +package press.mantra.compose.database.model + +import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.crypto.EventHasher +import com.vitorpamplona.quartz.nip01Core.signers.EventTemplate +import kotlin.test.Test +import kotlin.test.assertEquals +import press.mantra.compose.nostr.nip30303.ArtifactEvent +import press.mantra.compose.nostr.nip30303.ArtifactVersionEvent +import press.mantra.compose.nostr.nip30303.ChapterEvent +import press.mantra.compose.nostr.nip30303.ChunkEvent +import press.mantra.compose.nostr.nip30303.DialectEvent +import press.mantra.compose.nostr.nip30303.SubmissionEvent +import press.mantra.compose.nostr.nip30303.TranslationArtifactVersionEvent +import press.mantra.compose.nostr.nip30303.tags.ArtifactIdTag + +/** + * The row on disk and the payload on the wire have to be the same event. + * + * `MantraDao` writes an entity whose id comes from `MantraX.fromXEventTemplate`, + * and separately builds the rumor it submits with `rumorOf`, which hashes the + * template itself. Both are supposed to produce one id. Nothing checks that they + * do, and nothing would notice if they stopped: + * + * - the submission would carry a `payloadId` naming an event nobody has, + * - `MarmotInnerEvent.payloadEventId` would stop matching the row it carries, + * so `deleteByPayloadEventId` would silently un-queue nothing and superseded + * translations would go out anyway, + * - and every receiver would create a *second* row rather than converging on + * the sender's, because entity ids are content hashes and the two sides would + * be hashing different things. + * + * All of that is silent. The ids are opaque hex either way. + */ +class RumorIdAgreementTest { + private val author = "a".repeat(64) + private val chatRoomId = "room" + private val other = "b".repeat(64) + + /** Exactly what `MantraDao.rumorOf` does, and it must stay exactly that. */ + private fun rumorIdOf(template: EventTemplate<*>): String = EventHasher.hashId( + pubKey = author, + createdAt = template.createdAt, + kind = template.kind, + tags = template.tags, + content = template.content + ) + + @Test + fun `a dialect's row and its rumor agree`() { + val template = DialectEvent.build(name = "Sesotho", country = "Lesotho", language = "st") + + val entity = MantraDialect.fromDialectEventTemplate( + dialectEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `an artifact's row and its rumor agree`() { + val template = ArtifactEvent.build( + name = "In Detention", + url = "example.com", + visibility = "private", + license = "cc", + dialectId = other + ) + + val entity = MantraArtifact.fromArtifactEventTemplate( + artifactEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `an artifact version's row and its rumor agree`() { + val template = ArtifactVersionEvent.build(content = "1.0") { + addUnique(ArtifactIdTag.assemble(other)) + } + + val entity = MantraArtifactVersion.fromArtifactVersionEventTemplate( + artifactVersionEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `a chapter's and a chunk's rows agree with their rumors`() { + val chapter = ChapterEvent.build( + artifactVersionId = other, + name = "Chapter 1", + originalText = "some text", + index = 0, + wordCount = 2, + characterCount = 9 + ) + val chunk = ChunkEvent.build( + chapterId = other, + text = "some text", + index = 0, + wordCount = 2, + characterCount = 9 + ) + + assertEquals( + rumorIdOf(chapter), + MantraChapter.fromChapterEventTemplate(chapter, chatRoomId, author)?.id + ) + assertEquals( + rumorIdOf(chunk), + MantraChunk.fromChunkEventTemplate(chunk, chatRoomId, author)?.id + ) + } + + @Test + fun `a translation version's row and its rumor agree`() { + val template = TranslationArtifactVersionEvent.build( + artifactVersionId = other, + dialectId = other, + name = "Sesotho", + visibility = "private", + license = "cc" + ) + + val entity = MantraTranslationArtifactVersion.fromTranslationArtifactVersionEventTemplate( + translationArtifactVersionEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `the submission names the id the row was written under`() { + // The end of the chain the rest of this file checks a link of: what a + // receiver reads out of the envelope has to be the id the sender stored. + val template = DialectEvent.build(name = "Sesotho", country = "Lesotho", language = "st") + val entity = MantraDialect.fromDialectEventTemplate(template, chatRoomId, author) + + val payload = Event( + id = rumorIdOf(template), + pubKey = author, + createdAt = template.createdAt, + kind = template.kind, + tags = template.tags, + content = template.content, + sig = "" + ) + val submission = SubmissionEvent.build(payload = payload) + + val readBack = SubmissionEvent( + id = "f".repeat(64), + pubKey = author, + createdAt = submission.createdAt, + tags = submission.tags, + content = submission.content, + sig = "" + ) + + assertEquals(entity?.id, readBack.payloadId()) + assertEquals(entity?.id, readBack.payload()?.id) + assertEquals(DialectEvent.KIND, readBack.payloadKind()) + } +} diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/managers/LiveInstanceCacheTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/managers/LiveInstanceCacheTest.kt new file mode 100644 index 00000000..c2931f4d --- /dev/null +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/managers/LiveInstanceCacheTest.kt @@ -0,0 +1,201 @@ +package press.mantra.compose.managers + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlin.test.assertSame +import kotlinx.coroutines.runBlocking + +/** + * The decision behind keeping an MLS group alive between messages. + * + * This cache exists because quartz drops a secret tree's skipped-generation keys + * on save, so rebuilding a group between two messages loses any message that + * arrives late -- permanently, and silently. See `docs/mls-skipped-keys.md`. + * + * Every one of these failures is invisible at runtime. Reuse too eagerly and a + * group carries on from a ratchet another writer has already moved, which + * corrupts decryption rather than failing it. Reuse too rarely and the cache + * does nothing at all, and the bug it was written for comes straight back with + * no symptom to notice. So the rule is asserted rather than reasoned about. + */ +class LiveInstanceCacheTest { + /** Stands in for an MlsGroup: mutable, and its persisted form is its content. */ + private class Group(var state: String) { + /** How many times this particular instance was handed to a caller. */ + var uses: Int = 0 + } + + private fun cache() = LiveInstanceCache { it.state } + + @Test + fun `reuses the instance while nothing else has written`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + val first = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it.uses++; it } + ) + + val second = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it.uses++; it } + ) + + // The same object, not merely an equal one: what has to survive is the + // in-memory skipped-key map, which no amount of rebuilding recovers. + assertSame(first, second) + assertEquals(1, built) + assertEquals(2, second?.uses) + } + + @Test + fun `rebuilds when something else wrote the stored state`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + val first = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it } + ) + + // Sending a message advances the sender ratchet and saves; adding a + // member does too. Carrying on from an instance that has been overtaken + // would diverge the ratchet, which is worse than not caching at all. + stored = "written by someone else" + + val second = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("written by someone else") }, + save = { stored = it }, + block = { it } + ) + + assertEquals(2, built) + assertEquals(false, first === second) + } + + @Test + fun `persists whatever the block left behind`() = runBlocking { + val cache = cache() + var stored: String? = "start" + + cache.withInstance( + key = "room", + storedState = stored, + build = { Group("start") }, + save = { stored = it }, + block = { it.state = "advanced" } + ) + + assertEquals("advanced", stored) + } + + @Test + fun `the state it records is the one it compares against next time`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + repeat(3) { + cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + // Every use moves the instance on, as decrypting a message does. + block = { group -> group.state = "advanced ${group.uses++}" } + ) + } + + // Recording the pre-block state instead would make every call look like + // somebody else had written, quietly turning the cache off. + assertEquals(1, built) + } + + @Test + fun `does not run the block, or cache anything, when there is nothing to build`() = runBlocking { + val cache = cache() + var ran = false + + val result = cache.withInstance( + key = "room", + storedState = null, + build = { null }, + save = { }, + block = { ran = true } + ) + + assertNull(result) + assertEquals(false, ran) + assertEquals(0, cache.size()) + } + + @Test + fun `an instance whose use threw is not handed to the next caller`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + assertFailsWith { + cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { error("decryption blew up half way") } + ) + } + + cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it } + ) + + // Half-advanced and never persisted: the next caller has to start from + // what is actually on disk, not from whatever the failure left in memory. + assertEquals(2, built) + } + + @Test + fun `rooms are cached independently`() = runBlocking { + val cache = cache() + var storedA: String? = "a" + var storedB: String? = "b" + + val a = cache.withInstance( + key = "roomA", + storedState = storedA, + build = { Group("a") }, + save = { storedA = it }, + block = { it } + ) + val b = cache.withInstance( + key = "roomB", + storedState = storedB, + build = { Group("b") }, + save = { storedB = it }, + block = { it } + ) + + assertEquals(2, cache.size()) + assertEquals(false, a === b) + } +} diff --git a/docs/README.md b/docs/README.md index 892a9ba8..683c112f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -9,5 +9,8 @@ silent, or a decision that looked arbitrary and was not. | [shared-key-ceremony.md](./shared-key-ceremony.md) | ChillDKG over NIP-17: the rounds, the approval gates, the chat transcript, participant ordering | | [shared-key-derivation.md](./shared-key-derivation.md) | deriving further keys from the group's threshold key with FROST tweaks — why not BIP32, why no chain code, and the one rule that must not be broken | | [marmot-membership.md](./marmot-membership.md) | how members join an MLS group, and the epoch race that makes a missing member look like a successful invite | +| [mls-skipped-keys.md](./mls-skipped-keys.md) | why a group event that arrives a moment late is dropped for good, which flows trigger it, the quartz fix, and the partial mitigation in this app | Start with the ceremony if you are new to this area; the other two both assume it. +Read the skipped-keys note before debugging any "the other device never got it" +report — it is silent, and it looks like every other kind of delivery failure. diff --git a/docs/mls-skipped-keys.md b/docs/mls-skipped-keys.md new file mode 100644 index 00000000..fbf61175 --- /dev/null +++ b/docs/mls-skipped-keys.md @@ -0,0 +1,194 @@ +# Messages are lost when two arrive out of order + +A group event that a relay hands back a moment late is dropped and cannot be +recovered. Two messages published in the same second reliably lose one of them. + +This is a conformance gap in quartz's MLS implementation, not in this app. What +this app can do about it from outside the library is partial, and is described +at the end. + +## Symptom + +The receiver stores the kind:445 group event and produces nothing from it. No +inner event, no chat line, no error the user sees. `MarmotGroupEvent` is written +*before* the message is decrypted, so the row survives while everything +downstream of it silently does not: + +``` +receiver, room 6d8ec3ad ("Frosty (#admins)") + + 20:36:55 kind 9 chat message decrypted, applied + 20:37:34 kind 30321 nonce decrypted, applied + 20:37:34 kind 30320 proposal group event stored, no inner event +``` + +Both 20:37:34 events were published by the same sender in the same +`proposeSigning` call. Every message that arrived on its own decrypted fine; the +back-to-back pair lost exactly one. + +Downstream the failure reads as something else entirely. In the case above a +FROST signing session never started on the receiver, because the proposal that +opens one never arrived — leaving a nonce filed against a session that will +never exist. An earlier instance of the same bug dropped a dialect, and the +artifact referencing it then failed a foreign key and rolled back its whole +transaction. + +## Cause + +MLS is specified to tolerate out-of-order delivery inside an epoch. RFC 9420 +§9.1: a receiver that gets generation `N+1` before `N` derives the intermediate +keys and keeps them, so the older message can still be read when it turns up. + +Quartz does implement this. `SecretTree` caches them: + +```kotlin +// SecretTree.kt +private val skippedKeys = mutableMapOf, KeyNonceGeneration>() + +fun applicationKeyNonceForGeneration(leafIndex: Int, generation: Int): KeyNonceGeneration { + val cachedKey = skippedKeys.remove(Pair(leafIndex, generation)) + if (cachedKey != null) { /* ...replay check... */ return cachedKey } + + val state = getOrInitSender(leafIndex) + require(generation >= state.applicationGeneration) { + "Generation $generation already consumed (current: ${state.applicationGeneration})" + } + ... +} +``` + +The gap is that the cache is never persisted: + +```kotlin +// SecretTree.kt +fun exportSenderStates(): Map = senderState.toMap() + +fun importSenderStates(states: Map) { + senderState.putAll(states) +} +``` + +`exportSenderStates()` returns the ratchet *positions* only. `MlsGroup.saveState()` +calls it (`senderRatchetStates = secretTree.exportSenderStates()`) and +`MlsGroup.restore()` calls `importSenderStates`. So `skippedKeys` exists only in +one `SecretTree` instance's memory. + +That would be harmless if the group instance outlived the messages. It does not: +`NostrDao` rebuilds it from stored state for every inbound event and saves it +back afterwards. So the sequence is + +1. generation 1 arrives, ratchet advances 0 → 2, generation 0's key goes into + `skippedKeys` +2. `saveState()` — `skippedKeys` is dropped on the floor +3. generation 0 arrives, a fresh tree is restored with + `applicationGeneration = 2`, the cache is empty, `require` fails +4. the exception is swallowed, the event yields no `ApplicationMessage` + +Step 3 is terminal. The key is derived from a ratchet that has moved past it and +cannot be recovered, and nothing asks the sender to resend. + +Verified against the published artifact rather than a checkout: +`quartz-1.14.0-sources.jar`, `commonMain/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt`. + +## Why it is not an edge case here + +Nostr relays make no ordering guarantee at all, and negentropy reconciliation +hands back a room's backlog in whatever order it likes. Any two messages close +enough together can swap. + +Several flows publish in bursts, and each of them is a reliable trigger: + +| flow | messages in one pass | +|---|---| +| `FrostSigningManager.proposeSigning` | proposal, then the proposer's nonce | +| `ChillDkgRitualManager.proposeRitual` | proposal, then the host key | +| `MantraDao.addArtifact` | the artifact, then its first version | +| `MantraDao.addChapter` | the chapter, then one per paragraph chunk | + +`addChapter` is the worst of these: a chapter with twenty paragraphs publishes +twenty-one events at once, and only the ones that happen to arrive in ascending +generation order survive. + +## The fix, in quartz + +Carry the skipped keys through `saveState`/`restore` alongside the ratchet +positions. + +**1. Export and import them.** In `SecretTree`: + +```kotlin +fun exportSkippedKeys(): Map, KeyNonceGeneration> = skippedKeys.toMap() + +fun importSkippedKeys(keys: Map, KeyNonceGeneration>) { + skippedKeys.putAll(keys) +} +``` + +`MAX_SKIPPED_KEYS` already bounds the map, so the serialised size is bounded by +the same constant and needs no separate cap. + +**2. Put them in the group state.** `MlsGroup.saveState()` already writes +`senderRatchetStates = secretTree.exportSenderStates()`; add a sibling field, and +have `restore()` call `importSkippedKeys` next to its existing +`importSenderStates`. + +**3. Keep old state readable.** The persisted state is a TLS-encoded struct that +existing installs already hold, so the new field has to be optional: absent means +an empty map, which is exactly the behaviour today. Without that, every device +with a stored group is broken by the upgrade. + +**4. Consumed-generation replay protection.** `consumedGenerations` guards +against a replayed message re-using a cached key. It is in-memory too, so it +should travel with the skipped keys or the guard weakens across restarts. Worth +deciding deliberately rather than by omission. + +A test worth having with it: save and restore a group between the two messages +of an out-of-order pair, and assert the older one still decrypts. That is the +property, and it is invisible to any test that keeps one instance alive. + +### Getting the change into this build + +Quartz is **not** a local fork. It is `com.vitorpamplona.quartz:quartz`, pinned +in `gradle/libs.versions.toml` and resolved from mavenCentral; +`settings.gradle.kts` only `includeBuild`s `lightning-kmp-app`. Nothing in this +repository can change it. + +There is a full amethyst clone at `~/Documents/development/nostr/amethyst` whose +`SecretTree.kt` was byte-identical to published 1.14.0 when this was written, so +the patch itself is a small delta against a known-good base. Landing it means one +of: + +- **Upstream it.** It is a genuine RFC 9420 conformance gap and affects any + client that reloads group state per message, which is the ordinary shape for a + mobile app. Slowest, and the only option that leaves this repo's build + reproducible. +- **Patch the clone and publish to mavenLocal**, then add `mavenLocal()` here and + pin the patched version. Fast, but the build then depends on a patched crypto + library built from one machine's filesystem. +- **Wire quartz as a composite build**, the way `lightning-kmp-app` is. Same + coupling to a path outside the repo, but the source is at least visible. + +## What this app does in the meantime + +`MlsGroupCache` keeps a room's `MlsGroup` instance alive between messages instead +of rebuilding it from stored state each time, so `skippedKeys` survives for as +long as the process does. The inbound path in `NostrDao` goes through it. + +This covers the case that actually bites — a burst arriving in one sync, decrypted +one after another against the same tree — and it is what makes the flows in the +table above work. + +It is not the fix, and it is worth being precise about what it leaves broken: + +- **A restart loses the cache.** Messages skipped before the app closed cannot be + read after it reopens. +- **Another writer invalidates it.** Sending a message advances the sender ratchet + and saves the room's state; adding a member does too. The cache reuses its + instance only while the stored state is still exactly what it last wrote, and + rebuilds otherwise — dropping the skipped keys at that point, exactly as before. +- **Nothing helps a long reorder.** A message the relay holds back until after a + restart or an outbound send is gone. + +The staleness check is what keeps the cache from being *worse* than no cache: a +group that has been overtaken by another writer is never carried on with, so the +fallback is always the old behaviour rather than a diverged ratchet.