Merge branch 'mantra' into claude/long-running-chat-sync-8983dc
mantra had moved on ~30 commits, several of them in exactly this area — and it turns out both branches independently found the same bug and drew the same conclusion about the same filter. **The overlap.**f38a5f1fixed the three kind:1059 filters that named the wrong pubkey, including the two `authors=[userPublicKey]` requests in NostrDao that could never match a wrap signed by a throwaway key. This branch deleted those same two blocks, inverting the same `if` to the `== null` case, for the same reason. The code merged to the same shape; only the comments conflicted, and they are combined. **Nip17Filters wins, and the live subscription now defers to it.**ad3304aextracted the inbox filter to one definition precisely because it had been wrong in three call sites, with the no-`since` reasoning this branch arrived at separately. Keeping a fourth copy inside LiveSubscriptionManager would recreate the problem that commit exists to solve, so: - queueCatchUpSynchronization now calls Nip17Filters.inbox() instead of building an identical SynchronizationFilter with its own limit constant, - Nip17Filters gains liveInbox(), the same shape as a quartz Filter for a REQ rather than a SynchronizationFilter for the queue, and giftWrapFilter() defers to it. Two types for one filter is not duplication worth removing — the queue stores one and hashes it for computeId, a live subscription puts the other on the wire — but they belong side by side, because drift here means one of them quietly stops matching mail. **ChatMessageListViewModel keeps this branch's resolution.** mantra had it refresh our own inbox on open (Nip17Filters.inbox on our DM relays, purpose "chat"); this branch removed that call entirely. Both were right when written, and the merge is where the second becomes true: LiveSubscriptionManager holds exactly that filter open on exactly those relays for the whole account and reconciles it on every foreground, so opening a chat has nothing left to ask for. The redundancy is now recorded in the comment where the branch used to be, so it reads as superseded rather than dropped. Discovery — the kind-10050 lookup for a participant we cannot yet address — is untouched, and the purpose is no longer a conditional now that only one case reaches it. The commonTest coroutines-test dependency arrived on both sides; the comment gives both reasons. Verified: 154 tests pass, both branches' suites included — Nip17FiltersTest and the marmot direct-message suites alongside this branch's 46. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -9,9 +9,12 @@ 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 |
|
||||
| [marmot-direct-messages.md](./marmot-direct-messages.md) | a one-to-one message inside a group as a stock NIP-59 gift wrap — what its MIP-03 carve-out costs, why the sender cannot read their own, and the one query that would broadcast it |
|
||||
| [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 |
|
||||
| [long-running-sync.md](./long-running-sync.md) | the chat subscriptions that stay open instead of pulling once per screen — why the request queue could not simply hold one, and how the group filter follows the room list |
|
||||
| [dead-code.md](./dead-code.md) | code in the sync and relay stack that nothing calls, why each piece is still there, and which of it is a bug rather than a leftover |
|
||||
|
||||
Start with the ceremony if you are new to this area; the next two both assume it.
|
||||
The last two stand alone, and the dead-code inventory reads as a follow-up to the
|
||||
sync note.
|
||||
Start with the ceremony if you are new to this area; the Marmot notes all 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. The
|
||||
sync note stands alone, and the dead-code inventory reads as a follow-up to it.
|
||||
|
||||
360
docs/marmot-direct-messages.md
Normal file
360
docs/marmot-direct-messages.md
Normal file
@@ -0,0 +1,360 @@
|
||||
# Direct messages inside a Marmot group
|
||||
|
||||
A one-to-one message carried as an MLS application message: an ordinary NIP-59 gift
|
||||
wrap, ephemeral-keyed and signed the way NIP-59 says, addressed to one member and
|
||||
never broadcast to a relay. Every member of the group sees that a private message
|
||||
was sent and to whom. Only the recipient can read it.
|
||||
|
||||
Read [Why a throwaway key, and what it costs](#why-a-throwaway-key-and-what-it-costs)
|
||||
before changing anything here. Two consequences of that choice reach into every
|
||||
part of this, and one of them cannot be undone.
|
||||
|
||||
## What a direct message is, outside in
|
||||
|
||||
```
|
||||
kind:445 GroupEvent ephemeral signer, h-tag = nostrGroupId → relays
|
||||
└─ ChaCha20-Poly1305 MLS exporter secret → group
|
||||
└─ MLS PrivateMessage ContentType.APPLICATION → group
|
||||
│ sender authenticated by MLS leaf ────────┐
|
||||
└─ kind:1059 wrap pubkey = throwaway, signed by it │ → group
|
||||
└─ NIP-44 conversation key: throwaway ↔ recipient │
|
||||
└─ kind:13 seal signed by the sender ───── bound to ─────┘
|
||||
└─ NIP-44 → recipient
|
||||
└─ kind:14 rumor → recipient
|
||||
```
|
||||
|
||||
The group reaches the fourth layer and stops. Everything above it is what a
|
||||
bystander renders from: a recipient, a time, a wall — and a sender who comes from
|
||||
the MLS frame rather than from anything written in the wrap.
|
||||
|
||||
The two outer layers are unchanged. A direct message is an ordinary kind:445 to
|
||||
anyone watching a relay and an ordinary application message to `MlsGroup`; the
|
||||
whole feature is what goes in as the application payload.
|
||||
|
||||
`MarmotDirectMessage` builds and opens it, as pure functions of their arguments
|
||||
with no database and no MLS state — which is what makes it testable, since
|
||||
Room-backed code cannot be unit-tested in this project.
|
||||
|
||||
## Why a throwaway key, and what it costs
|
||||
|
||||
The wrap uses a fresh random key and is signed by it, exactly as NIP-59 specifies
|
||||
and exactly as `GiftWrapEvent.create` builds one. Nothing in the wrap names the
|
||||
sender.
|
||||
|
||||
**What it does not buy: hiding the sender from the group.** MLS authenticates every
|
||||
application message to a leaf. `mlsGroup.memberIdentityHex(decrypted.senderLeafIndex)`
|
||||
yields the sender's real pubkey no matter what the inner event claims, and there is
|
||||
no way to send an application message that does not. The throwaway key removes a
|
||||
*redundant* copy of an identity the MLS frame already proves — it does not remove
|
||||
the identity.
|
||||
|
||||
**What it does buy:** a wrap lifted out of its MLS frame — a log line, a database
|
||||
export, a crash dump — is not attributable to anyone. It is also a well-formed
|
||||
NIP-59 gift wrap rather than a Marmot-shaped variant, which is worth something to
|
||||
anyone reading the payload with ordinary nostr tooling.
|
||||
|
||||
Two costs follow, and both are load-bearing.
|
||||
|
||||
### It requires a carve-out in a security check
|
||||
|
||||
`MarmotInboundManager.processPrivateMessage` rejects any inner application event
|
||||
whose `pubKey` is not the MLS sender's credential identity:
|
||||
|
||||
```
|
||||
MIP-03: inner event pubkey (…) does not match MLS sender identity (…)
|
||||
```
|
||||
|
||||
That is MIP-03, not local policy, and it exists so a member cannot mint events
|
||||
claiming a different author. A throwaway-keyed wrap cannot satisfy a check about
|
||||
its author, so kind:1059 is exempt from it:
|
||||
|
||||
```kotlin
|
||||
if (innerEvent.kind != GiftWrapEvent.KIND && innerEvent.pubKey != senderIdentity) {
|
||||
return GroupEventResult.Error(groupId, "MIP-03: inner event pubkey …")
|
||||
}
|
||||
```
|
||||
|
||||
`senderIdentity` is now *required* rather than merely compared. Under the old check
|
||||
a null identity failed only because the comparison failed; attribution depends on
|
||||
it outright, so it fails on its own.
|
||||
|
||||
**The check is not weakened, it is relocated and strengthened.** What replaces it
|
||||
for kind:1059 is `seal.pubKey == senderIdentity` on a seal whose `verify()` passes
|
||||
— a signature bound to an MLS leaf, rather than a plaintext field compared to one.
|
||||
The attack it stops is a member re-wrapping a seal they were legitimately sent and
|
||||
passing it to a third party as its author's; that fails because the MLS frame says
|
||||
who actually sent *this* one. Forging one outright needs the other member's private
|
||||
key.
|
||||
|
||||
A seal is harder to relabel than it looks, and not because of the signature.
|
||||
NIP-44 derives the conversation key from the pubkey being claimed, so writing
|
||||
another member's key over a seal makes it undecryptable by the person it was
|
||||
encrypted for — it fails at the decryption, before any check runs, and comes out as
|
||||
a message the recipient simply cannot read. The label is bound to the key rather
|
||||
than asserted alongside it. `verify()` is what catches the remaining case: a seal
|
||||
altered after signing in a way that survives decryption. Both have tests, because
|
||||
the first was asserted the wrong way round until one of them failed.
|
||||
|
||||
> **Compatibility.** Every member's client needs this carve-out. A Marmot client
|
||||
> implementing MIP-03 as written drops these messages as impersonation — silently,
|
||||
> as a `GroupEventResult.Error` — so a group with one unpatched member has one
|
||||
> member who never receives a direct message and is never told why. This is a
|
||||
> divergence from the spec, and a reason to raise it upstream rather than carry it
|
||||
> indefinitely.
|
||||
|
||||
### The sender cannot read their own messages back
|
||||
|
||||
The throwaway private key is discarded at send time, so nothing can reopen the wrap
|
||||
afterwards — not even the person who built it. NIP-17 solves this by sending a
|
||||
second wrap addressed to yourself; here that would be a second application message,
|
||||
and so a second *"sent a private message"* line in everyone else's transcript, so it
|
||||
is not available.
|
||||
|
||||
What follows:
|
||||
|
||||
- The sending device keeps its own copy, because `sendChatMessage` writes the
|
||||
plaintext `ChatMessage` row on the way out. Normal use is unaffected.
|
||||
- A second device, or a reinstall, gets **nothing** — the sender's own half of
|
||||
every conversation is unreadable to them anywhere it was not typed. The
|
||||
recipient's half is unaffected.
|
||||
- On a re-sync the sender's own message comes back as an opaque wrap they cannot
|
||||
open, indistinguishable from a bystander's view. `ChatMessage.directMessage`
|
||||
guards that case explicitly and files nothing, because a bystander line would
|
||||
replace the words on the row `sendChatMessage` wrote — the only copy there is.
|
||||
`NostrDao.persistInboundChatMessage` carries the same guard, for the same reason,
|
||||
on the NIP-17 path.
|
||||
|
||||
Do not solve this by storing the throwaway private key. A per-message private key at
|
||||
rest is strictly worse than the identity-keyed wrap it was chosen over, and it
|
||||
reintroduces the attribution the throwaway key exists to remove. There is a test —
|
||||
`the sender cannot reopen their own message` — whose only job is to make that
|
||||
reversal fail loudly rather than ship.
|
||||
|
||||
## The one way to broadcast this by accident
|
||||
|
||||
Nothing sweeps events to relays on its own. Broadcast is driven by
|
||||
`BroadcastNostrEventRequest` rows joined to `NostrEvent`, inserted explicitly. So
|
||||
the requirement "this must never be broadcast" reduces to one query:
|
||||
|
||||
```sql
|
||||
SELECT * FROM GiftWrapPayload WHERE publicKey = :publicKey AND giftWrapSealId IS NULL
|
||||
```
|
||||
|
||||
`GiftWrapPayloadDao.observeUnsealedGiftWrapPayloads`. The notary watches it, and
|
||||
`DatabaseChatRepository.sealGiftWrapPayload` seals every row it returns and hands it
|
||||
to `NostrNip17Dao.persistAndBroadcastGiftWrap` — which inserts a
|
||||
`BroadcastNostrEventRequest` per relay. **Write one `GiftWrapPayload` row authored
|
||||
by the local user with a null seal id and the direct message leaves the device.**
|
||||
|
||||
Hence the rule:
|
||||
|
||||
> A Marmot direct message never writes to `GiftWrapPayload` or `GiftWrapMessage`
|
||||
> at all. Its outbound queue is `MarmotInnerEvent`, and so is its inbound record.
|
||||
|
||||
The risk is sharper here than it would be with a Marmot-shaped payload, because what
|
||||
this builds is a genuine, well-formed, correctly signed NIP-59 gift wrap. It is
|
||||
indistinguishable from something the NIP-17 path would be right to publish. Nothing
|
||||
but the tables it is kept out of stops it going to a relay.
|
||||
|
||||
`GiftWrapMessage` could not be written anyway without a `NostrEvent` row — its
|
||||
foreign key — and `NostrEvent` is the broadcast join target. The rule is also
|
||||
enforced at the other end: `sealGiftWrapPayload` refuses any payload whose room has
|
||||
a non-null `mlsGroupState`, and logs. An MLS room should never produce a NIP-17 gift
|
||||
wrap for any reason, and an invariant in code is what stops a later refactor from
|
||||
walking a direct message onto a relay without reading this page first.
|
||||
|
||||
## Attribution comes from MLS, not from the payload
|
||||
|
||||
Because nothing in the wrap names the sender, every sender-derived field is read
|
||||
from the MLS frame. This is not a workaround; it is the correct source, and it is
|
||||
what makes the carve-out above safe.
|
||||
|
||||
`GroupEventResult.ApplicationMessage` carries `senderLeafIndex` but not the resolved
|
||||
identity, and `ChatMessage.fromGroupEventResult` has no `MlsGroup` to resolve it
|
||||
with. The obvious fix — adding `senderIdentity` to the result — is not available:
|
||||
quartz is a **binary dependency** here (`com.vitorpamplona.quartz:quartz:1.14.0`),
|
||||
and the local checkout at `~/Documents/development/nostr/amethyst/quartz` is a
|
||||
reference copy, not a build input. Changing the result type would mean publishing a
|
||||
fork.
|
||||
|
||||
So `NostrDao` resolves it at the call site instead and passes it in. It holds the
|
||||
group, the leaf index is already on the result, and an application message advances
|
||||
no epoch, so the tree has not moved by the time it reads it. Same value, no fork.
|
||||
|
||||
This also settled a bug that predates the feature. All fourteen arms of
|
||||
`fromGroupEventResult` computed:
|
||||
|
||||
```kotlin
|
||||
isUserMessage = activeKeyPair.pubKey.toHex() == groupEvent.pubKey
|
||||
```
|
||||
|
||||
`groupEvent.pubKey` is the kind:445's signer, and that is a fresh random ephemeral
|
||||
key on every send — `NostrSignerInternal(KeyPair())` in
|
||||
`encryptAndSendMarmotInnerEvent` — so it could never equal the active key. It was
|
||||
false for every Marmot message in every room, which meant every message a member
|
||||
sent themselves rendered as somebody else's: wrong side of the transcript, wrong
|
||||
colour, and no delivery status, which is drawn only for our own lines. All fourteen
|
||||
now read `senderIdentity`. For the four results that carry no leaf index — the
|
||||
commit and proposal statuses — it is null and yields false, exactly as before.
|
||||
|
||||
## The message's identity is the rumor id
|
||||
|
||||
The queued `MarmotInnerEvent` for a direct message **is the rumor** — kind:14,
|
||||
plaintext, p-tagged — so its id is a hash of exactly the fields the recipient will
|
||||
have after unwrapping.
|
||||
|
||||
This matters in two places. It keeps the outbound link intact, which is the
|
||||
difference between a message being sent and silently not being sent (see
|
||||
[Two bugs this uncovered](#two-bugs-this-uncovered)). And it gives the recipient a
|
||||
stable id for dedupe across redelivery and re-sync.
|
||||
|
||||
It does **not** give sender and recipient a shared identity for the same message the
|
||||
way an identity-keyed wrap would, because the sender never re-derives the rumor from
|
||||
an echo they cannot open. Two devices belonging to the sender do not converge; they
|
||||
simply do not both have the message.
|
||||
|
||||
The wire event — the wrap — has its own id, so a direct message leaves two rows on a
|
||||
recipient's device: one for the artifact that travelled and one for the message it
|
||||
carried.
|
||||
|
||||
## Timestamps are not fuzzed
|
||||
|
||||
NIP-59 randomises the wrap's and the seal's `created_at` by up to two days to
|
||||
frustrate correlation at a relay, and both `GiftWrapEvent.create` and
|
||||
`SealedRumorEvent.create` default to `TimeUtils.randomWithTwoDays()`.
|
||||
|
||||
The wrap never reaches a relay, and the kind:445 around it already carries the true
|
||||
time. Fuzzing would do nothing but scatter the *"sent a private message"* line up to
|
||||
two days out of position in every other member's transcript. `MarmotDirectMessage`
|
||||
passes the real message time to all three layers — a default to override, not one to
|
||||
accept, and the easiest thing here to get wrong by omission. There is a test for it.
|
||||
|
||||
## Three decisions
|
||||
|
||||
| decision | taken | what it costs |
|
||||
|---|---|---|
|
||||
| Is the recipient visible to the group? | yes — the `p` tag stays on the wrap | The group learns who messages whom, and how often. Omitting the tag is genuinely cheap here: the recipient decrypts against the wrap's own throwaway pubkey, so they need no tag to find their own mail, and one failed NIP-44 decrypt per member per message is the whole cost. What it takes is the named bystander line — *"sent a private message"* with no recipient — and any ordering by conversation. Worth revisiting; not free, but close. |
|
||||
| Is the wrap signed? | yes — by the throwaway key, per NIP-59 | Nothing, which is the point. The signer is meaningless and discarded, so the signature attributes nothing, and keeping it means the payload is a real NIP-59 gift wrap that `GiftWrapEvent.create` builds and `unwrapOrNull` opens with no special cases. |
|
||||
| What kind is the rumor? | `ChatMessageEvent.KIND` (14) | Group messages here are kind:9 (`ChatEvent`) — `sendChatMessage` maps 14 → 9 on the way in for an ordinary message and leaves a direct message at 14. It is a NIP-17 chat message that happens to travel inside a group, and the kind is what tells the two apart on the way back in. Costs one more arm in the inbound switch. |
|
||||
|
||||
## The path a message takes
|
||||
|
||||
**Out.** `sendChatMessage` resolves the recipient against the room's participants —
|
||||
refusing a non-member, whose wrap nobody could open, and refusing yourself, whose
|
||||
wrap could never be read back — and writes two rows: the queued `MarmotInnerEvent`
|
||||
(the rumor) and the `ChatMessage` holding the plaintext. The notary picks the queued
|
||||
row up, `MarmotOutboundDao` calls `MarmotDirectMessage.wrap` instead of assembling a
|
||||
bare rumor, and everything downstream is unchanged. Once sent, the plaintext is
|
||||
scrubbed from the queued row: it is already on the `ChatMessage`, and a second copy
|
||||
would be cleartext left in a table that otherwise holds nothing but wire events.
|
||||
|
||||
**In.** `MarmotInboundManager` decrypts and resolves the sender's identity, and
|
||||
`MarmotDirectMessage.classify` decides what the wrap is to this device — ours,
|
||||
readable, unreadable, or rejected. `ChatMessage.directMessage` turns that into rows.
|
||||
The split is so the decision can be tested: only the filing needs a database, and
|
||||
Room-backed code cannot be unit-tested here. The recipient opens the wrap,
|
||||
validates the seal against `senderIdentity`, stores the rumor as its own
|
||||
`MarmotInnerEvent` keyed on the rumor's id, and gets the words. A bystander cannot
|
||||
open it and gets a line with no content. The sender gets nothing, because their
|
||||
local row is the only copy.
|
||||
|
||||
A failed validation drops the message and logs; it does not throw. The caller is
|
||||
inside `storeNostrEvent`'s transaction, and a forged direct message should cost its
|
||||
own line, not the whole event.
|
||||
|
||||
**On screen.** Tapping somebody else's message in a group offers *Reply privately*,
|
||||
which arms the composer; sending clears it. An armed composer shows a chip naming
|
||||
the recipient, a placeholder that says who it is going to, and a tinted field, and
|
||||
the chip's close button is the one tap back to the room. Three signals rather than
|
||||
one because the failure this guards against — sending to the room what was meant for
|
||||
one person, or the reverse — cannot be taken back once it is on the wire.
|
||||
|
||||
## Two bugs this uncovered
|
||||
|
||||
### The wrap's id is not the rumor's
|
||||
|
||||
`encryptAndSendMarmotInnerEvent` found the chat message to link and broadcast with
|
||||
`getChatMessagesByMarmotInnerEventId(innerEvent.id)` — the *recomputed* id. For a
|
||||
direct message that is the wrap's id, not the queued rumor's, so the lookup returned
|
||||
null, the `ChatMessage` was never linked, **and no `BroadcastNostrEventRequest` was
|
||||
ever inserted**. The message would have been encrypted, persisted, and never sent,
|
||||
with no error anywhere.
|
||||
|
||||
It now keys on `marmotInnerEvent.id` — what `sendChatMessage` wrote into
|
||||
`ChatMessage.marmotInnerEventId`, and identical to `innerEvent.id` for every
|
||||
non-direct message, so nothing else moved.
|
||||
|
||||
### `isUserMessage` was always false for Marmot messages
|
||||
|
||||
Covered under [Attribution comes from MLS](#attribution-comes-from-mls-not-from-the-payload).
|
||||
It stopped being merely cosmetic here: with nothing in the payload naming the
|
||||
sender, `senderIdentity` is the only source of attribution there is.
|
||||
|
||||
## Where it lives
|
||||
|
||||
| file | what |
|
||||
|---|---|
|
||||
| `nostr/MarmotDirectMessage.kt` | wrap / open, the pure seam both directions call |
|
||||
| `commonTest/.../MarmotDirectMessageTest.kt` | the envelope: ten cases, against real secp256k1 and real NIP-44 |
|
||||
| `commonTest/.../MarmotDirectMessageDeliveryTest.kt` | what a device does with an arriving wrap, including every forgery it must refuse |
|
||||
| `commonTest/.../MarmotMip03CarveOutTest.kt` | the kind:1059 exemption and its edges |
|
||||
| `managers/MarmotInboundManager.kt` | the kind:1059 carve-out; requires the sender identity |
|
||||
| `database/model/ChatMessage.kt` | `TYPE_DIRECT_MESSAGE`; the kind:1059 arm and its three outcomes |
|
||||
| `database/dao/NostrDao.kt` | resolves `senderIdentity` and passes it in |
|
||||
| `database/repository/DatabaseChatRepository.kt` | queues the rumor; refuses to seal an MLS room's payload |
|
||||
| `database/dao/MarmotOutboundDao.kt` | wraps on the way out; scrubs the plaintext |
|
||||
| `database/model/MarmotInnerEvent.kt` | `directMessageRecipientPublicKey`, the outbound signal |
|
||||
| `ui/view/model/ChatMessageListViewModel.kt` | armed state; the two renderings |
|
||||
| `ui/composable/ChatRoomMessagingScreen.kt` | the recipient chip |
|
||||
|
||||
Untouched, deliberately: `NostrNip17Dao`, `GiftWrapMessage`, `GiftWrapSeal`,
|
||||
`GiftWrapPayload`, and every broadcast path.
|
||||
|
||||
## What this does not do
|
||||
|
||||
Each of these will be reported as a bug at some point. They are not.
|
||||
|
||||
**The group learns that a direct message happened, and to whom.** Only the contents
|
||||
are private, and the sender is authenticated by MLS whatever the wrap says. That is
|
||||
the design, and the UI should say so in words somewhere a user meets before their
|
||||
first private message — it does not yet.
|
||||
|
||||
**The sender cannot read their own messages anywhere they were not typed.** The
|
||||
throwaway key is gone. A reinstall or a second device recovers the recipient's half
|
||||
of a conversation and none of its own.
|
||||
|
||||
**Other Marmot clients drop these messages.** Until the carve-out is upstream, a
|
||||
group needs every member on a client that carries it, and an unpatched member fails
|
||||
silently.
|
||||
|
||||
**The inner layer is not forward secret.** The kind:445 envelope inherits MLS epoch
|
||||
forward secrecy; the NIP-44 layers inside do not. A recipient's identity key that
|
||||
leaks opens every direct message they still hold, including ones sent years earlier.
|
||||
This is strictly weaker than the group messages sitting beside them in the same
|
||||
room.
|
||||
|
||||
**A removed member keeps what they already have.** Removal advances the epoch; it
|
||||
does not reach back into their device.
|
||||
|
||||
**Disappearing messages apply at the envelope only.** The group's
|
||||
`disappearingMessageSecs` puts a NIP-40 expiration on the kind:445, which relays
|
||||
honour. Local rows are unaffected, exactly as for group messages today.
|
||||
|
||||
**One recipient per message.** Several would mean several wraps, and so several
|
||||
bystander lines for one message. Worth doing; worth designing first.
|
||||
|
||||
**No reactions, receipts, replies or attachments.** A reply needs an `e` tag inside
|
||||
the rumor and is a small addition later. A reaction is a design question rather than
|
||||
a coding one, because the reaction itself would be visible to the group.
|
||||
|
||||
**No member picker.** The only way to start a private message is to reply to one the
|
||||
member already sent, so you cannot open a conversation with somebody who has not
|
||||
spoken.
|
||||
|
||||
**Three things have no automated test**, all of them for want of infrastructure
|
||||
rather than by choice. The recipient validation in `sendChatMessage` (refusing a
|
||||
non-member, refusing yourself) and the outbound id lookup both need a database; the
|
||||
invariant the lookup depends on — that the queued row's id is the rumor's — is
|
||||
tested in its place. The two transcript renderings need Compose UI testing, which
|
||||
this project has no dependency on. Anything touching a real `MlsGroup` is likewise
|
||||
untested: the carve-out is tested through `mip03Rejection`, not through a group.
|
||||
194
docs/mls-skipped-keys.md
Normal file
194
docs/mls-skipped-keys.md
Normal file
@@ -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<Pair<Int, Int>, 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<Int, SenderRatchetState> = senderState.toMap()
|
||||
|
||||
fun importSenderStates(states: Map<Int, SenderRatchetState>) {
|
||||
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<Pair<Int, Int>, KeyNonceGeneration> = skippedKeys.toMap()
|
||||
|
||||
fun importSkippedKeys(keys: Map<Pair<Int, Int>, 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.
|
||||
Reference in New Issue
Block a user