From 248a52726786bafa855c1081f95e2cbd0614f453 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sat, 5 Sep 2026 15:55:54 +0200 Subject: [PATCH 1/4] fix: store the framed commit on MarmotCommitResult, not the exporter secret `MarmotOutboundDao.inviteMember` persisted the commit row with framedCommitBytes = commitResult.preCommitExporterSecret, two lines below the argument that value belongs to, which was already assigning it correctly. It now reads `commitResult.framedCommitBytes`. The row is written on the deferred branch, after the kind:445 commit has gone out and while the welcome waits on a relay acknowledgement, so what it holds is meant to be the record of what was published. ## Why the compiler had nothing to say `MarmotCommitResult` carries quartz's `CommitResult` payload fields verbatim -- `commitBytes`, `welcomeBytes`, `groupInfoBytes`, `framedCommitBytes`, `preCommitExporterSecret`, same names, same order, same defaults. Both of the fields in question are `ByteArray`, so the wrong field of the right object is indistinguishable from the correct one at the type level. The call site lists its named arguments in a different order than the declaration, which is what put `preCommitExporterSecret` and `framedCommitBytes` two lines apart. The entity also repeats quartz's `framedCommitBytes: ByteArray = commitBytes` default, so the explicit argument was overriding a fallback that -- while still the raw commit rather than the framed envelope -- was at least a commit. ## What it cost, and what it would have cost Nothing so far. `framedCommitBytes` has exactly two references in the tree: this assignment, and `encryptedCommitEvent` at the top of the same branch, which takes `commitResult.framedCommitBytes` from the in-memory `CommitResult` rather than from the row. The bytes that reached the relay were always the right ones; the wrong ones only ever sat in the column. They would stop merely sitting there as soon as anything reads the row back. `DatabaseNostrRepository` already reloads these rows on acknowledgement, at `getMarmotCommitRequestById`, to pick up `welcomeBytes` and fire `deliveryWelcome`. An ack-triggered rebroadcast or a replay reaching one field further along would publish 32 bytes of exporter secret where a `MlsMessage(PublicMessage(FramedContent(commit)))` envelope was expected: not a message recipients drop, but a group key on a relay. The smaller half holds whether or not anything ever reads it. The group's pre-commit `MLS-Exporter("marmot", "group-event", 32)` output was being written to a second column that is not intended to hold key material, doubling its footprint at rest alongside the `preCommitExporterSecret` field that exists for it. Only at rest -- the ack path logs the row, but the data class has no `toString` override, so `ByteArray` prints as an identity hash rather than contents. ## Scope `MarmotCommitResult` has a single construction site in the codebase, the one changed here, so there is no second copy of this to fix. Worth checking rather than assuming: the shape that produced it -- adjacent `ByteArray` fields with identical names on both sides of the copy -- reproduces anywhere the entity is built again. Co-Authored-By: Claude Opus 5 --- .../press/mantra/compose/database/dao/MarmotOutboundDao.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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..c03944bd 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 @@ -442,7 +442,7 @@ abstract class MarmotOutboundDao( commitBytes = commitResult.commitBytes, preCommitExporterSecret = commitResult.preCommitExporterSecret, welcomeBytes = commitResult.welcomeBytes, - framedCommitBytes = commitResult.preCommitExporterSecret, + framedCommitBytes = commitResult.framedCommitBytes, groupInfoBytes = commitResult.groupInfoBytes, userPublicKey = userPublicKey, peerKeyPackageEventId = peerKeyPackage.id, From d110737f9add7148ab0954ae9a7848239c326660 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sat, 5 Sep 2026 23:09:53 +0200 Subject: [PATCH 2/4] fix: keep a room's MlsGroup alive so a late message can still be read 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 --- .../mantra/compose/database/dao/NostrDao.kt | 31 ++- .../mantra/compose/managers/MlsGroupCache.kt | 121 +++++++++++ docs/README.md | 3 + docs/mls-skipped-keys.md | 194 ++++++++++++++++++ 4 files changed, 339 insertions(+), 10 deletions(-) create mode 100644 composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt create mode 100644 docs/mls-skipped-keys.md 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 18685171..0aaba9be 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/managers/MlsGroupCache.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt new file mode 100644 index 00000000..6429fe16 --- /dev/null +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt @@ -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() + + /** + * 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() + 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 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 + } +} 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. From 42dd38cfc400dff40199849ac8735ea0cd3f890f Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sat, 5 Sep 2026 23:18:54 +0200 Subject: [PATCH 3/4] test: pin the two invariants this session left unguarded Both are silent when broken, which is why they are worth asserting rather than reasoning about. **The cache's reuse decision.** MlsGroupCache exists because quartz drops a secret tree's skipped-generation keys on save, so rebuilding a group between two messages loses any that arrives late. Its safety argument is one comparison: reuse while the stored state is still what the cache last wrote, rebuild when it is not. Get that wrong in either direction and nothing complains -- reuse too eagerly and a group carries on from a ratchet another writer already moved, which corrupts decryption rather than failing it; reuse too rarely and the cache does nothing and the original bug is back with no symptom. That decision is now a generic LiveInstanceCache with MlsGroupCache as a typed facade over it, so it can be tested without standing up an MLS group. Splitting it also made two behaviours explicit that were previously incidental: a failed build no longer leaves the old instance behind, and an instance whose use threw is deliberately not cached -- it is half-advanced and never persisted, so the next caller has to start from disk. **Rumor and row ids agreeing.** MantraDao writes an entity whose id comes from fromXEventTemplate and separately builds the rumor it submits with rumorOf, which hashes the template itself. Both are meant to produce one id and nothing checked it. Diverging would mean submissions naming an event nobody has, deleteByPayloadEventId silently un-queuing nothing so superseded translations go out anyway, and every receiver creating a second row instead of converging on the sender's -- all of it invisible, since the ids are opaque hex either way. Asserted per kind, plus the whole chain out through the submission envelope. Both suites were mutation-checked rather than trusted: inverting the staleness comparison fails one cache test, recording the pre-block state fails another, and hashing the rumor under a different author fails all six id tests. Still uncovered, and not cheaply fixable: FrostSigningManager's and MantraDao's state machines both need a Room harness, and commonTest has none. The FROST crypto path is covered by FrostSigningRoundTest; the message-driven parts around it are not. Co-Authored-By: Claude Opus 5 --- .../mantra/compose/managers/MlsGroupCache.kt | 111 ++++++---- .../database/model/RumorIdAgreementTest.kt | 174 +++++++++++++++ .../compose/managers/LiveInstanceCacheTest.kt | 201 ++++++++++++++++++ 3 files changed, 446 insertions(+), 40 deletions(-) create mode 100644 composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/RumorIdAgreementTest.kt create mode 100644 composeApp/src/commonTest/kotlin/press/mantra/compose/managers/LiveInstanceCacheTest.kt diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt index 6429fe16..efe838a1 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt @@ -51,43 +51,14 @@ import kotlinx.coroutines.sync.withLock * 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() - - /** - * 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() - private val locksGuard = Mutex() - - private suspend fun lockFor(chatRoomId: String): Mutex = - locksGuard.withLock { locks.getOrPut(chatRoomId) { Mutex() } } + 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; it - * runs inside the lock so the stored state and the cached instance cannot - * disagree. + * [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. @@ -98,24 +69,84 @@ object MlsGroupCache { build: () -> MlsGroup?, save: suspend (String) -> Unit, block: suspend (MlsGroup) -> T, - ): T? = lockFor(chatRoomId).withLock { - val cached = entries[chatRoomId] + ): T? = cache.withInstance( + key = chatRoomId, + storedState = storedStateHex, + build = build, + save = save, + block = block + ) +} - val group = if (cached != null && cached.stateHex == storedStateHex) { - cached.group +/** + * 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("Room $chatRoomId was written elsewhere; rebuilding its group") + 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 } - val result = block(group) + // 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 stateHex = group.saveState().encodeTls().toHex() - save(stateHex) - entries[chatRoomId] = Entry(group = group, stateHex = stateHex) + 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/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) + } +} From fb21678813c28ab62b4312d87d3e91e4e310bd12 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sat, 5 Sep 2026 23:24:21 +0200 Subject: [PATCH 4/4] test: pin where a commit's bytes land when the row recording it is written The mis-routed `framedCommitBytes` fixed in the previous commit was invisible for one reason: nothing anywhere covered the persisted row. The bytes that reach a relay come off the in-memory `CommitResult`, so the wire path stayed correct and the stored path was wrong, and no test looked at the stored path. ## Why the mapping moved before it could be tested A test that built `MarmotCommitResult` itself would have been writing its own copy of the mapping and asserting against that. It would have passed against the buggy code, because the bug was at the call site the test was not using. So the mapping is now `MarmotCommitResult.from`, called by `MarmotOutboundDao.inviteMember` and exercised directly by the test. That also removes the shape that produced the bug rather than just the instance of it: the old call site listed its named arguments in an order different from the declaration, which is what put `preCommitExporterSecret` and `framedCommitBytes` two lines apart. `from` lists the payload in declaration order, in one place, so there is no second site to get wrong. ## What is covered Four tests, each payload given a distinct self-identifying value so that a field arriving in the wrong column names both halves of the mistake instead of comparing equal by accident: - every payload field lands in its own column. - the framed commit column never holds the exporter secret -- the regression, stated as an invariant rather than an equality so it keeps holding for a `CommitResult` this test did not anticipate. - a `CommitResult` that never framed its commit still stores a commit. quartz defaults `framedCommitBytes` to `commitBytes` and the entity repeats that default; the fallback must not quietly become the secret either. - the bookkeeping `DatabaseNostrRepository` reads back on acknowledgement is carried through. `id`, `chatRoomId`, `userPublicKey` and `peerKeyPackageEventId` are all 64-char hex, so two of them swapped in `from` would typecheck exactly as silently as the original bug. Checked by reintroducing `framedCommitBytes = commitResult.preCommitExporterSecret` into `from`: three of the four fail. A green suite that would stay green against the bug it names is not coverage. ## What is not covered, and why That the bytes published equal the bytes stored -- the property one level above this one -- still is not. It needs the DAO, and the DAO needs Room: `commonTest` carries only `kotlin.test`, the room3 KSP processor is registered for the android and ios targets alone with `kspJvm` commented out, and `getInMemoryDatabaseBuilder` wants a `PlatformContext` no unit test has. That is a Robolectric or instrumented target, which is a larger change than this fix earns and is better decided on its own merits than smuggled in here. The ack-triggered rebroadcast that would have turned the bug into a live fault does not exist yet, so there is nothing to test there either. When it is written, the invariant it needs is already asserted. Co-Authored-By: Claude Opus 5 --- .../compose/database/dao/MarmotOutboundDao.kt | 12 +- .../database/model/MarmotCommitResult.kt | 35 +++++ .../model/MarmotCommitResultMappingTest.kt | 134 ++++++++++++++++++ 3 files changed, 173 insertions(+), 8 deletions(-) create mode 100644 composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/MarmotCommitResultMappingTest.kt 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 c03944bd..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.framedCommitBytes, - 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/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/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) + } +}