From 02117643c415e33cc3f16bc0dc43cb942110a0d1 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sun, 6 Sep 2026 01:06:05 +0200 Subject: [PATCH] fix: send a group event because it was queued, not because the chat mentions it No FROST signing message has ever reached another participant. The proposal was built, MLS-encrypted, wrapped under the exporter secret, signed as a kind:445, written to NostrEvent and MarmotGroupEvent, and its queue row marked processed -- and then never handed to a relay, by a branch that was never about delivery at all. **The gate.** The tail of MarmotOutboundDao.encryptAndSendMarmotInnerEvent looked up the transcript row for the queued rumor and did everything else inside it: val chatMessageOrNull = database.chatMessageDao() .getChatMessagesByMarmotInnerEventId(marmotInnerEvent.id) chatMessageOrNull?.let { chatMessage -> ... relation, marmotGroupEventId ... val ids = database.broadcastNostrEventRequestDao().insert(...) } The BroadcastNostrEventRequest rows are the only thing that puts a kind:445 on a relay -- observeBroadcastNostrEventRequestsByStatus("pending") is what the broadcaster watches, and nothing else inserts them for this path. So the question "does the chat have a line for this?" was silently answering the question "should the group receive this?". **Why FROST always lost.** A signing message has no ChatMessage by design. FrostSigningManager.broadcast queues the rumor alone, and announce() writes its milestone lines with marmotInnerEventId = null on purpose: each device writes its own transcript from the messages it has already received, so the lines cost no traffic and cannot disagree with the session they describe. The inbound half states the same intent from the other side -- ChatMessage.applyInnerEvent returns null for every FrostSigningEvents kind, because a row there would be a second, worse account of what the manager already narrates. That is every kind in the family, not just the proposal: nonces, the signer set, partial signatures, the finished signature and the failure notice all go through the same broadcast(). A session could not have completed even if a proposal had somehow arrived. **GroupKeyStateManager.announce had it too.** Same shape, same silence: a room's kind:30326 announcement of which key it signs with was queued, encrypted and dropped. a909108 added it so members would stop rederiving; no member has ever received one. **Why the neighbours worked, and hid it.** The DKG rides NIP-17 gift wraps through a different path entirely, so a room could finish a ceremony, hold a real shared key, and report canSign() == true with the signing transport dead beneath it. nip30303 submissions work because MantraDao.sendMarmotInnerEvent pairs every queued rumor with a ChatMessage carrying its id -- not as a delivery mechanism, just because a submission is also something a member did. FROST was the first traffic to use the group path without a chat line, which is why this reads as a FROST bug and is not one. **Why it went unnoticed.** Nothing failed. The coordinator's own device is fully convinced: proposeSigning writes the session, announceStarted puts a line in the chat, advance() runs, and publishOwn records the coordinator's own nonce and announces that step too. From the proposer's side a session nobody else can see is indistinguishable from one waiting on slow peers. Unlike 65e4a3a, the queue did not block. marmotGroupEventId is set before the transcript lookup, so the row left the queue cleanly and the next one was picked up. Every message was lost individually, in silence, with no backlog to notice. **The fix.** The broadcast insert is hoisted out of the branch, and the decision it was tangled with is lifted into MarmotDelivery.plan: given a group event, a relay list, and a chat message or null, what has to be written. A group event is sent because it was queued; a chat line is linked because a member said something. The DAO now computes that plan and executes it, with the insert as a plain unconditional statement ahead of the bookkeeping that legitimately does depend on there being a line. The extraction is not decoration. encryptAndSendMarmotInnerEvent is Room-backed and cannot be stood up in a unit test, which is exactly how the gate survived; separating the decision from the filing of it is the same move MarmotDirectMessage.classify exists for, and for the same stated reason. **Tests.** MarmotDeliveryTest, six of them. The two that matter are "a signing proposal goes out, though nothing in the chat points at it" and "the send does not depend on the transcript", which asserts the broadcast list is identical with and without a chat message. The rest pin the supporting facts: one request per relay naming the event, every request written pending because that is the only status the broadcaster looks at, the linkage that does depend on a chat line, and an empty relay list as the sole legitimate way to produce an empty broadcast list -- so that an empty list always reads as "nowhere to send it" and never as "nothing to send". **Not covered, deliberately.** These pin the decision, not the call site. Re- nesting the insert inside chatMessageOrNull?.let would leave MarmotDelivery correct and every test passing. Closing that needs the DAO itself under test: BundledSQLiteDriver is on the classpath and getInMemoryDatabaseBuilder exists, but its android actual wants a real Context, testDebugUnitTest is plain JVM, and there is no androidUnitTest source set or Robolectric. That is its own change, not one to smuggle in here. Verified: :composeApp:compileDebugKotlinAndroid succeeds; 160 tests pass, 154 before these six. The inbound half was read rather than assumed -- NostrDao dispatches FrostSigningEvents kinds to processSigningPayload, inbound rumors are stored with marmotGroupEventId set so they cannot re-enter the outbound queue, and the out-of-order replay path is intact. Outbound was the only break. That two participants now actually see a proposal is inference from the code, not an observation: it wants two devices. Co-Authored-By: Claude Opus 5 --- .../compose/database/dao/MarmotOutboundDao.kt | 30 ++--- .../mantra/compose/nostr/MarmotDelivery.kt | 60 ++++++++++ .../compose/nostr/MarmotDeliveryTest.kt | 107 ++++++++++++++++++ 3 files changed, 183 insertions(+), 14 deletions(-) create mode 100644 composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/MarmotDelivery.kt create mode 100644 composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/MarmotDeliveryTest.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 653d12ee..e6c5049f 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 @@ -22,6 +22,7 @@ import press.mantra.compose.database.model.intermdiate.LocalChatRoom import press.mantra.compose.extensions.exporterSecret import press.mantra.compose.extensions.toHex import press.mantra.compose.managers.MarmotInboundManager.EPOCH_RETENTION_WINDOW +import press.mantra.compose.nostr.MarmotDelivery import press.mantra.compose.nostr.MarmotDirectMessage import press.mantra.compose.nostr.Relays import co.touchlab.kermit.Logger @@ -653,13 +654,24 @@ abstract class MarmotOutboundDao( // Keyed on the queued row, not on `innerEvent.id`. For a direct message those // differ -- the row is the rumor, the wire event is the wrap built around it -- - // and the wrap's id matches no ChatMessage, so this lookup would come back null, - // the message would never be linked, and no BroadcastNostrEventRequest would ever - // be inserted. Encrypted, stored, and silently never sent. They are the same value - // for every other kind of message. + // and the wrap's id matches no ChatMessage, so this lookup comes back null. That + // costs only the transcript linkage, not the send. val chatMessageOrNull = database.chatMessageDao().getChatMessagesByMarmotInnerEventId(marmotInnerEvent.id) logger.d("chatMessageOrNull: $chatMessageOrNull") + + val delivery = MarmotDelivery.plan( + groupEventId = groupEvent.id, + relays = Relays.DefaultDMRelayList, // TODO: Get these from localChatRoom... + chatMessage = chatMessageOrNull, + ) + + // Sync broadcast to all the required relays. Unconditional, and before any + // transcript bookkeeping: see MarmotDelivery for what gating it on a chat line + // cost the signing sessions that have none. + val broadcastNostrEventRequestIds = + database.broadcastNostrEventRequestDao().insert(delivery.broadcasts) + chatMessageOrNull?.let { chatMessage -> database.chatMessageNostrEventRelationDao().upsert( ChatMessageNostrEventRelation( @@ -674,16 +686,6 @@ abstract class MarmotOutboundDao( ) ) - // Sync broadcast to all the required relays... - val broadcastNostrEventRequestIds = database.broadcastNostrEventRequestDao().insert( - Relays.DefaultDMRelayList.map { // TODO: Get these from localChatRoom... - BroadcastNostrEventRequest( - nostrEventId = groupEvent.id, - relayURL = it.url - ) - } - ) - broadcastNostrEventRequestIds.forEach { broadcastNostrEventRequestId -> database.chatMessageBroadcastNostrEventRequestRelationDao().upsert( ChatMessageBroadcastNostrEventRequestRelation( diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/MarmotDelivery.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/MarmotDelivery.kt new file mode 100644 index 00000000..c41d8a76 --- /dev/null +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/MarmotDelivery.kt @@ -0,0 +1,60 @@ +package press.mantra.compose.nostr + +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl +import press.mantra.compose.database.model.BroadcastNostrEventRequest +import press.mantra.compose.database.model.ChatMessage + +/** + * What has to be written once a queued inner event has been encrypted into a + * group event: where it is sent, and which transcript row -- if any -- the send + * belongs to. + * + * The two are independent, and this exists to say so. A group event is sent + * because it was queued; a chat line is linked because a member said something. + * Letting the second decide the first is what silenced FROST signing: the + * broadcast rows were written inside `chatMessageOrNull?.let { }`, and protocol + * traffic has no ChatMessage -- `ChatMessage.applyInnerEvent` returns null for + * every [press.mantra.compose.nostr.frost.FrostSigningEvents] kind and for + * [press.mantra.compose.nostr.frost.GroupKeyStateEvent], and + * `FrostSigningManager` queues its messages without one. So every signing + * proposal was MLS-encrypted, wrapped, persisted and marked processed, and then + * never sent to a relay. Nothing failed; the other participants simply never saw + * it. + * + * Pulled out of `MarmotOutboundDao.encryptAndSendMarmotInnerEvent` because that + * function is Room-backed and cannot be unit tested, which is precisely how the + * gate survived. The decision is separated from the filing of it for the same + * reason [MarmotDirectMessage.classify] is. + */ +data class MarmotDelivery( + /** + * One request per relay, always. An empty list here means the event is on + * disk and going nowhere. + */ + val broadcasts: List, + /** + * The chat line this send belongs to, or null when the row is protocol + * traffic -- something the room did rather than something a member said. + */ + val transcriptChatMessageId: Long?, +) { + /** True when nothing in the chat points at this event, which is not a reason to withhold it. */ + val isProtocolTraffic: Boolean get() = transcriptChatMessageId == null + + companion object { + fun plan( + groupEventId: HexKey, + relays: Collection, + chatMessage: ChatMessage?, + ): MarmotDelivery = MarmotDelivery( + broadcasts = relays.map { relay -> + BroadcastNostrEventRequest( + nostrEventId = groupEventId, + relayURL = relay.url, + ) + }, + transcriptChatMessageId = chatMessage?.id, + ) + } +} diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/MarmotDeliveryTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/MarmotDeliveryTest.kt new file mode 100644 index 00000000..5df982dc --- /dev/null +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/MarmotDeliveryTest.kt @@ -0,0 +1,107 @@ +package press.mantra.compose.nostr + +import press.mantra.compose.database.model.ChatMessage +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Whether an encrypted group event actually leaves the device. + * + * The bug this pins did not throw, log, or fail a build. `MarmotOutboundDao` + * wrote the broadcast rows inside `chatMessageOrNull?.let { }`, so an event with + * no chat line pointing at it was MLS-encrypted, wrapped, persisted, its queue + * row marked processed -- and never sent. FROST signing proposals and the room's + * key announcement are exactly that: traffic the room generates, which + * deliberately writes no ChatMessage of its own. Every proposal was silently + * delivered to nobody. + * + * The DAO around this is Room-backed and cannot be stood up here, which is how + * the gate survived unnoticed in the first place. So the decision is tested + * where it can be seen, and the DAO does nothing with it but write what it says. + */ +class MarmotDeliveryTest { + private val groupEventId = "a".repeat(64) + private val relays = Relays.DefaultDMRelayList + + private val chatLine = ChatMessage( + id = 42L, + senderPublicKey = "b".repeat(64), + isUserMessage = true, + giftWrapPayloadId = null, + marmotGroupEventId = null, + marmotInnerEventId = "c".repeat(64), + chatRoomId = "room", + content = "the vote is at six", + ) + + @Test + fun `a signing proposal goes out, though nothing in the chat points at it`() { + // The regression. A FROST message has no ChatMessage by design -- the manager + // writes its own transcript lines from what arrives, so a row here would be a + // second, worse account of the same thing -- and that must not be the reason the + // group never hears about it. + val delivery = MarmotDelivery.plan(groupEventId, relays, chatMessage = null) + + assertTrue(delivery.isProtocolTraffic) + assertEquals(relays.size, delivery.broadcasts.size) + assertTrue(delivery.broadcasts.isNotEmpty(), "an event on disk and going nowhere") + } + + @Test + fun `the send does not depend on the transcript`() { + // Said as directly as it can be said: the two questions are independent. Anything + // that makes a broadcast conditional on a chat line fails here. + val protocol = MarmotDelivery.plan(groupEventId, relays, chatMessage = null) + val spoken = MarmotDelivery.plan(groupEventId, relays, chatMessage = chatLine) + + assertContentEquals( + protocol.broadcasts.map { it.relayURL }, + spoken.broadcasts.map { it.relayURL }, + ) + assertEquals(protocol.broadcasts.size, spoken.broadcasts.size) + } + + @Test + fun `every relay gets a request, naming the event`() { + val delivery = MarmotDelivery.plan(groupEventId, relays, chatMessage = chatLine) + + assertContentEquals( + relays.map { it.url }, + delivery.broadcasts.map { it.relayURL }, + ) + assertTrue(delivery.broadcasts.all { it.nostrEventId == groupEventId }) + } + + @Test + fun `requests are queued pending, which is all the broadcaster looks at`() { + // `observeBroadcastNostrEventRequestsByStatus("pending")` is the only thing that + // picks these up. A request written in any other state is as unsent as no request. + val delivery = MarmotDelivery.plan(groupEventId, relays, chatMessage = null) + + assertTrue(delivery.broadcasts.all { it.status == "pending" }) + } + + @Test + fun `a member's message is linked to its chat line`() { + // The bookkeeping that legitimately does depend on there being a chat line: the + // transcript needs to know which event carried the words, so a sent message can + // be shown as sent. + val delivery = MarmotDelivery.plan(groupEventId, relays, chatMessage = chatLine) + + assertFalse(delivery.isProtocolTraffic) + assertEquals(chatLine.id, delivery.transcriptChatMessageId) + } + + @Test + fun `no relays is the only way an event stays home`() { + // Worth pinning as the single legitimate empty case, so an empty broadcast list + // is always read as "nowhere to send it" and never as "nothing to send". + val delivery = MarmotDelivery.plan(groupEventId, relays = emptyList(), chatMessage = chatLine) + + assertTrue(delivery.broadcasts.isEmpty()) + assertEquals(chatLine.id, delivery.transcriptChatMessageId) + } +}