From fa5e14e605ca02d2bfe9129b4e7f5dc8a221aefe Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 08:50:12 +0000 Subject: [PATCH] fix(marmot): mint a rotated KeyPackage the way the first one was minted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MIP-00 replaces a KeyPackage as soon as a Welcome consumes it, so rotation is not a rare path — it runs right after the first group we are ever invited to. It went through the legacy generator, so from that moment on the only KeyPackage on relays for us was a MIP-era one with no account identity proof. A current-profile peer refuses that outright ("member KeyPackage identity or profile is invalid") and keeps inviting from whatever stale copy it still has cached, so an account went silently uninvitable one join after it was set up. Rotation and first publication now share one mint path, so a replacement cannot land on a different profile than the KeyPackage it replaces. Harness, two tests that were reporting our bugs as theirs and one that was reporting the reverse: - Test 16 asked `wn keys publish` to rotate. That verb is the idempotent retry of the durable stable-slot replacement — with nothing pending it republishes the same event id, so there is no rotation to observe. `wn keys rotate` is the one that mints. - Test 13 swallowed `wn keys check`'s output, so "no prior KP for A" read as a missing fixture when it was MDK refusing what we had published. The raw answer goes to the log now and the message says what actually happened. - Test 09 polled `reactions.by_emoji`, which belongs to the materialized timeline; `wn messages list` reads the raw app-event log, where a reaction is its own kind:7 entry with an "e" tag naming the anchor. The reaction had been arriving and being stored correctly the whole time. The MDK 0.9.20 interop harness is now green, 17 of 17, twice in a row from a clean state. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016kCuA6tc4JQzHPCDd39GHq --- cli/tests/marmot/tests-extras.sh | 45 ++++-- .../amethyst/commons/marmot/MarmotManager.kt | 44 +++-- .../MarmotKeyPackageRotationProfileTest.kt | 152 ++++++++++++++++++ quartz/plans/2026-09-08-marmot-spec-resync.md | 72 ++++++--- 4 files changed, 262 insertions(+), 51 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotKeyPackageRotationProfileTest.kt diff --git a/cli/tests/marmot/tests-extras.sh b/cli/tests/marmot/tests-extras.sh index e9781fc830..54c962e20e 100644 --- a/cli/tests/marmot/tests-extras.sh +++ b/cli/tests/marmot/tests-extras.sh @@ -60,16 +60,26 @@ test_09_reply_react_unreact() { record_result "$id" fail "amy marmot message react failed"; return fi - # Round-trip: B should surface amy's kind:7 reaction. wn aggregates - # reactions onto the anchor message (`.reactions.by_emoji[]`), - # not as a standalone entry whose `.content` is the emoji — so polling - # `messages list` for an entry whose content equals "🍕" would never - # match, even when the reaction was successfully decrypted. Look for - # the emoji under any message's `reactions.by_emoji` keys instead. + # Round-trip: B should surface amy's kind:7 reaction. `wn messages list` + # reads the raw app-event log, where a reaction is its own kind:7 entry + # carrying the emoji and an "e" tag naming the anchor — the aggregated + # `reactions.by_emoji` summary belongs to the materialized timeline, which + # this command does not project. Match the raw shape, and accept an + # aggregated one too so a wn that starts summarising here still passes. local deadline=$(( $(date +%s) + 90 )) saw=0 while [[ $(date +%s) -lt $deadline ]]; do local payload payload=$(wn_b_json messages list "$mls_gid" --limit 50 2>/dev/null || true) + if [[ -n "$payload" ]] && \ + printf '%s' "$payload" \ + | jq_list messages \ + | jq -e --arg anchor "$msg_id" --arg emoji "🍕" \ + 'select(.kind == 7) + | select((.plaintext // .content // "") == $emoji) + | select([(.tags // [])[] | select(.[0] == "e") | .[1]] | index($anchor))' \ + >/dev/null 2>&1; then + saw=1; break + fi if [[ -n "$payload" ]] && \ printf '%s' "$payload" \ | jq_list messages | jq -e '(.reactions.by_emoji // {}) | keys[]?' \ @@ -200,11 +210,15 @@ test_13_keypackage_rotation() { banner "Test 13 — KeyPackage rotation" local id="13 keypackage rotation" - local before - before=$(wn_b --json keys check "$A_NPUB" 2>/dev/null \ - | jq -r '.result.key_package.key_package_event_id // .result.event_id // empty') + # `keys check` resolves A's KeyPackage the way an invite would, so an empty + # answer here is a real finding, not a missing fixture: it means MDK looked + # at what A published and refused it. Keep the raw JSON in the log. + local before raw + raw=$(wn_b --json keys check "$A_NPUB" 2>&1) + printf 'wn keys check %s -> %s\n' "$A_NPUB" "$raw" >>"$LOG_FILE" + before=$(printf '%s' "$raw" | jq -r '.result.key_package.key_package_event_id // .result.event_id // empty') if [[ -z "$before" ]]; then - record_result "$id" fail "no prior KP for A"; return + record_result "$id" fail "wn cannot resolve a KeyPackage for A"; return fi amy_json marmot key-package publish >/dev/null || { @@ -373,11 +387,12 @@ test_16_wn_keypackage_rotation() { record_result "$id" fail "no prior KP visible to amy for B"; return fi - # Ask B to rotate. `wn keys publish` writes a new kind:443 with a fresh - # created_at; the old event may or may not be evicted depending on the - # relay's retention policy, so both may coexist for a while. - wn_b keys publish >/dev/null 2>&1 || { - record_result "$id" fail "wn_b keys publish failed"; return + # Ask B to rotate. It has to be `keys rotate` ("force mint and publish a + # fresh replacement"), not `keys publish` — the latter is the idempotent + # retry of the durable stable-slot replacement, so with nothing pending it + # republishes the same event id and there is no rotation to observe. + wn_b keys rotate >/dev/null 2>&1 || { + record_result "$id" fail "wn_b keys rotate failed"; return } local deadline=$(( $(date +%s) + 60 )) after="" diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt index 94aab47d43..b1187d3abb 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt @@ -1126,8 +1126,22 @@ class MarmotManager( * — which is what kept us uninvitable. */ currentProfile: Boolean = true, + ): KeyPackageEvent = mintKeyPackageEventForSlot(keyPackageRotationManager.getOrCreateSlotDTag(slotName), relays, currentProfile) + + /** + * Mint, sign and index a KeyPackage for an already-resolved d-tag slot. + * + * Both the first publication and every rotation go through here, and that + * is the point: a replacement minted any other way can end up on a + * different protocol profile than the KeyPackage it replaces, which makes + * the account uninvitable to peers that require the current one. + */ + @OptIn(ExperimentalEncodingApi::class) + private suspend fun mintKeyPackageEventForSlot( + dTag: String, + relays: List, + currentProfile: Boolean, ): KeyPackageEvent { - val dTag = keyPackageRotationManager.getOrCreateSlotDTag(slotName) val identity = signer.pubKey.hexToByteArray() val bundle = if (currentProfile) { @@ -1184,26 +1198,22 @@ class MarmotManager( * Rotate consumed KeyPackage slots. * Returns list of KeyPackageEvents to publish. */ - @OptIn(ExperimentalEncodingApi::class) - suspend fun rotateConsumedKeyPackages(relays: List): List { + suspend fun rotateConsumedKeyPackages( + relays: List, + currentProfile: Boolean = true, + ): List { val pendingSlots = keyPackageRotationManager.pendingRotationSlots() if (pendingSlots.isEmpty()) return emptyList() - val identity = signer.pubKey.hexToByteArray() + // Same mint path as the first publication. MIP-00 makes us replace a + // KeyPackage the moment a Welcome consumes it, so this runs right + // after the first group we are ever invited to — minting the + // replacement any other way would silently downgrade the only + // KeyPackage on relays for us, and a peer that requires the current + // profile refuses it and can never add us again. return pendingSlots.map { slot -> - val bundle = keyPackageRotationManager.rotateSlot(identity, slot) - val keyPackageBase64 = Base64.encode(KeyPackageUtils.frameKeyPackage(bundle.keyPackage)) - val keyPackageRef = bundle.keyPackage.reference().toHexKey() - - val template = - KeyPackageEvent.build( - keyPackageBase64 = keyPackageBase64, - dTagSlot = slot, - keyPackageRef = keyPackageRef, - relays = relays, - ) - val signed = signer.sign(template) - keyPackageRotationManager.recordPublishedEventId(slot, signed.id) + val signed = mintKeyPackageEventForSlot(slot, relays, currentProfile) + keyPackageRotationManager.clearPendingRotation(slot) signed } } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotKeyPackageRotationProfileTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotKeyPackageRotationProfileTest.kt new file mode 100644 index 0000000000..68a0166fd7 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotKeyPackageRotationProfileTest.kt @@ -0,0 +1,152 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.marmot + +import com.vitorpamplona.quartz.marmot.mip00KeyPackages.KeyPackageBundleStore +import com.vitorpamplona.quartz.marmot.mip00KeyPackages.KeyPackageUtils +import com.vitorpamplona.quartz.marmot.mls.group.MarmotMessageStore +import com.vitorpamplona.quartz.marmot.mls.group.MlsGroupStateStore +import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer +import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * A rotation must publish the same kind of KeyPackage the first publication + * did. + * + * MIP-00 makes us replace a KeyPackage as soon as a Welcome consumes it, so + * rotation is not a rare path — it runs right after the first group we are + * ever invited to. Minting the replacement through the legacy generator meant + * that from that moment on the only KeyPackage on relays for us was a + * MIP-era one, without the account identity proof (`0x8009`) that a + * current-profile peer requires. MDK then refuses it outright + * (`member KeyPackage identity or profile is invalid`) and keeps inviting from + * whatever stale copy it still has cached, so the account silently becomes + * uninvitable one join after it was set up. + */ +class MarmotKeyPackageRotationProfileTest { + private val relay = + RelayUrlNormalizer.normalizeOrNull("wss://example.invalid/") + ?: error("test relay must normalize") + + @Test + fun aRotatedKeyPackageStaysOnTheCurrentProfile() = + runBlocking { + val signer = NostrSignerInternal(KeyPair()) + val manager = MarmotManager(signer, RotationStateStore(), RotationMessageStore(), RotationBundleStore()) + + val first = manager.generateKeyPackageEvent(listOf(relay)) + assertTrue(first.isCurrentProfile(), "the first publication is current-profile") + + // A Welcome consumed it, which is what schedules the replacement. + manager.keyPackageRotationManager.markConsumedByEventId(first.id) + assertTrue(manager.needsKeyPackageRotation()) + + val rotated = manager.rotateConsumedKeyPackages(listOf(relay)) + assertEquals(1, rotated.size, "one consumed slot means one replacement") + val replacement = rotated.single() + + assertTrue( + replacement.isCurrentProfile(), + "the replacement must carry the account identity proof too, or no current-profile " + + "peer can add us after our first join", + ) + assertEquals( + first.dTag(), + replacement.dTag(), + "the replacement lands in the same addressable slot", + ) + assertTrue( + replacement.id != first.id, + "the replacement must actually be a different KeyPackage", + ) + assertTrue( + KeyPackageUtils.isValid(replacement), + "and it must validate under the same MIP-00 rules a peer applies", + ) + assertTrue( + !manager.needsKeyPackageRotation(), + "the slot is no longer pending once its replacement is minted", + ) + } +} + +// Minimal in-memory stores, matching the ones the other MarmotManager tests +// use; the file-backed implementations live in the platform modules. + +private class RotationStateStore : MlsGroupStateStore { + private val states = mutableMapOf() + private val retained = mutableMapOf>() + + override suspend fun save( + nostrGroupId: String, + state: ByteArray, + ) { + states[nostrGroupId] = state + } + + override suspend fun load(nostrGroupId: String): ByteArray? = states[nostrGroupId] + + override suspend fun delete(nostrGroupId: String) { + states.remove(nostrGroupId) + retained.remove(nostrGroupId) + } + + override suspend fun listGroups(): List = states.keys.toList() + + override suspend fun saveRetainedEpochs( + nostrGroupId: String, + retainedSecrets: List, + ) { + retained[nostrGroupId] = retainedSecrets + } + + override suspend fun loadRetainedEpochs(nostrGroupId: String): List = retained[nostrGroupId] ?: emptyList() +} + +private class RotationMessageStore : MarmotMessageStore { + override suspend fun appendMessage( + nostrGroupId: String, + innerEventJson: String, + ) = Unit + + override suspend fun loadMessages(nostrGroupId: String): List = emptyList() + + override suspend fun delete(nostrGroupId: String) = Unit +} + +private class RotationBundleStore : KeyPackageBundleStore { + private var snapshot: ByteArray? = null + + override suspend fun save(snapshot: ByteArray) { + this.snapshot = snapshot + } + + override suspend fun load(): ByteArray? = snapshot + + override suspend fun delete() { + snapshot = null + } +} diff --git a/quartz/plans/2026-09-08-marmot-spec-resync.md b/quartz/plans/2026-09-08-marmot-spec-resync.md index 945b79efd7..648d3df42a 100644 --- a/quartz/plans/2026-09-08-marmot-spec-resync.md +++ b/quartz/plans/2026-09-08-marmot-spec-resync.md @@ -586,16 +586,15 @@ Writing the producer side immediately found two bugs the reader-side tests could ## Interop status (2026-09-09) -The MDK 0.9.20 harness runs end to end. It went **1 passed / 12 failed** to -**9 passed** over this pass, and the failures that remain are named below rather -than lumped together. +The MDK 0.9.20 harness runs end to end and is **green: 17 of 17**, twice in a +row from a clean state. It started this pass at **1 passed / 12 failed**. The +defects it found are below — every one of them a place where two +implementations have to agree on something one implementation alone never +disagrees with, which is why none of them was visible to any same-implementation +test we have. ### Defects the harness found in our own code -Each of these was invisible to every same-implementation test we have, because -each is a place where two implementations have to agree on something one -implementation alone never disagrees with. - 1. **A Commit rebuilt our own leaf from defaults.** The UpdatePath leaf replaces OUR leaf — same member, new key material — so its capabilities and extensions must carry over. `buildLeafNode` was called with neither, so the very first @@ -628,24 +627,59 @@ implementation alone never disagrees with. exists to prevent. `PublishOutcome.UNKNOWN` keeps the record and holds the group. -5. **The harness was parsing a wire format `wn` no longer speaks.** MDK 0.9.x +5. **We treated a last-resort KeyPackage as single-use.** We publish every + KeyPackage marked last resort and then dropped its private keys the moment + one Welcome consumed it. OpenMLS deletes a consumed bundle only + `if !key_package.last_resort()`; MDK marks all of its own last resort, caches + the peer KeyPackage it resolved, and invites from that cached copy every time + after. So the first invite addressed to us worked and every one after it died + on "No matching KeyPackageBundle". Two things had to be fixed together: the + last-resort check had to read the current profile's carrier (a + `last_resort_key_package` component inside the KeyPackage-level + `app_data_dictionary`, not the MIP-era `0x000A` extension type), and the + Welcome lookup had to trust the MLS KeyPackageRefs over the Nostr `e` tag, + which MDK stamps from its stale cached copy. + +6. **Every rotation downgraded us off the current profile.** MIP-00 replaces a + KeyPackage as soon as a Welcome consumes it, so rotation runs right after the + first group we are ever invited to — and it minted the replacement through + the legacy generator. From that moment the only KeyPackage on relays for us + had no account identity proof, MDK refused it outright, and the account was + silently uninvitable one join after setup. Rotation and first publication now + share one mint path. + +7. **The harness was parsing a wire format `wn` no longer speaks.** MDK 0.9.x returns `{"ok":true,"result":{"invites":[…]}}`; iterating `.result` walked that object's three VALUES, so every poll matched nothing and reported "never received invite" for welcomes that had arrived and been accepted. +### Harness defects (not ours) + +- **Runs inherited each other's state.** wnd wipes B's and C's data dirs on + start, but A's amy home and the relay's SQLite file survived, and the + leftovers are not inert — a KeyPackage from an earlier run is still on the + relay to be invited with, and old kind:445 events still arrive undecryptable. + Tests 03 and 08 failed on a dirty tree and passed on a clean one. Every run + now starts from empty stores; `--reuse-state` opts out and `--tests "…"` runs + a subset. +- **Test 16 asked `wn keys publish` to rotate.** That verb is the idempotent + retry of the durable stable-slot replacement, so with nothing pending it + republishes the same event id and there is no rotation to observe. + `wn keys rotate` is the one that mints. +- **Test 09 polled the wrong surface.** `wn messages list` reads the raw + app-event log, where a reaction is its own kind:7 entry with an `e` tag naming + the anchor; `reactions.by_emoji` is the materialized timeline's aggregate, + which that command does not project. The reaction had been arriving and being + stored correctly the whole time. + ### What is NOT done -- **Test 03 gets further but does not pass.** We join MDK's group and see its - name; the first kind-445 after the join is not delivered. Tests 05/12/14/15 - fail behind it with "A never received invite". -- **Tests 13 and 16 (KeyPackage rotation) fail.** `wn keys check` finds no prior - KeyPackage for A at that point in the run, and amy keeps seeing B's - pre-rotation KeyPackage. -- **Test 09 fails on the relay, not on us**: `disconnected before OK` from the - local nostr-rs-relay under the load of a full run. The durable ingest markers - added in this pass cut a large part of that load (a backdated gift wrap used - to be re-unwrapped on every sync, forever) but the test has not been re-run - since. +- **The local relay drops a publish occasionally.** One run in several, + `amy` gets `disconnected before OK` from nostr-rs-relay and reports the send + as unconfirmed even though the event is on the relay a moment later. It is a + harness-relay flake, not a protocol failure, and it costs whichever test is + running at the time. Worth making the CLI's publish confirmation tolerate a + reconnect rather than papering over it in the tests. - **Agent-text-stream is receive-only.** We decode the `0x8006` policy, derive per-stream record keys, open records and fold the transcript, and we advertise the `0xF2D1` receive capability. We do NOT advertise `send` (`0xF2D2`) or