From 70c53a8fcddd267c958bbb752965a3c08087a9df Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Mon, 10 Aug 2026 10:25:25 -0400 Subject: [PATCH] fix(concord): make the invite-list writes actually durable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A high-effort audit of the branch found that the durability guarantees the previous commits claimed were not the guarantees the code provided. Three of these are in the code written to close the last review, and they defeat exactly what those commits set out to fix. `INostrClient.publish` returns Unit — it queues an event and never reports acceptance; `publishAndConfirm` is the confirming variant. So every `runCatching { publish(...); true }` was true whenever local signing worked. That made minting's "record the link before handing out the URL" gate decorative, and made revoke worse than decorative: it reported success for a tombstone no relay stored, then recorded the kind-13303 tombstone, whose merge drops the entry — destroying the only `signer_sk` that could ever retire the link while the link stayed live. Both paths, and the Refounding re-mint, now confirm. `fetchAll` returns an empty list on cannot-connect / CLOSED / idle-timeout, so "a relay served us and had nothing" and "nobody answered" were the same observation. Reading the second as "no list yet" reintroduced, one layer below, the wipe the null-vs-empty work existed to prevent. `fetchAllWithHooks` gains a `doneOut` of per-relay terminal reasons plus `anyRelayServed()`, and both clients now only treat an empty read as an empty list when a relay actually reached EOSE. The rest: - `drainConcordRekeys` discarded the entry `adoptConcordRoot` now returns, so only the account that *launched* a rotation re-minted its links. An admin who was merely re-keyed left every link they had handed out on the dead root, and anyone stranded behind one could never recover — which is the branch's headline goal, holding only for the rotator. - The join-time ban gate fetched the Control Plane from `bundle.relays` alone (stale metadata refuses a community we can plainly reach) with a single un-paged REQ (truncated at the relay's filter cap, so a missing older ban edition fails the gate OPEN, re-admitting the account it exists to refuse). Now unions in the relays that just served the bundle, and pages. - `decodeOrNull` failed the whole document for one structurally incompatible entry. Since null now means "refuse to write", that converted the old silent data loss into a permanent write lock on a coordinate that never ages out. Unreadable entries are carried verbatim instead, so they neither block the account nor get dropped on re-encode. - The list read took the newest event of any kind and then cast, so one stray event at the coordinate read as "unreadable" forever. Filters by kind first. - The Refounding refresh did one full round trip per link, serially, inside a user-visible rotation. One pooled REQ over every link signer, then concurrent confirmed re-mints, classified per coordinate so one link's tombstone cannot decide another's status. Verified on a tablet: mint, list, revoke and the cross-client refusal still work end to end — and with the community relay killed, revoke now reports "The link couldn't be revoked" and leaves the entry intact, where before it would have claimed success and destroyed the key. Co-Authored-By: Claude Opus 5 (1M context) --- .../amethyst/model/AccountConcordActions.kt | 138 +++++++++++++----- .../com/vitorpamplona/amethyst/cli/Context.kt | 3 + .../amethyst/cli/commands/ConcordCommands.kt | 15 +- .../commons/actions/ConcordActions.kt | 10 ++ .../cord05Invites/ConcordInviteList.kt | 96 +++++++++--- .../NostrClientFetchAllWithHooksExt.kt | 21 +++ .../cord05Invites/ConcordInviteListTest.kt | 37 +++++ 7 files changed, 255 insertions(+), 65 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt index 35816e52ce..1bf9339ae4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt @@ -54,8 +54,11 @@ import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray import com.vitorpamplona.quartz.nip01Core.core.toHexKey +import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.anyRelayServed import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.fetchAll import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.fetchAllPagesFromPool +import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.fetchAllWithHooks +import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.publishAndConfirm import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer @@ -65,6 +68,9 @@ import com.vitorpamplona.quartz.nipC7Chats.ChatEvent import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.RandomInstance import com.vitorpamplona.quartz.utils.TimeUtils +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.coroutineScope import java.util.concurrent.ConcurrentHashMap /** Name of the default Concord community Admin role minted by "Make admin". */ @@ -191,12 +197,29 @@ class AccountConcordActions( val relays = account.outboxRelays.flow.value if (relays.isEmpty()) return null val filter = Filter(kinds = listOf(ConcordInviteListEvent.KIND), authors = listOf(account.signer.pubKey)) + // Terminal reasons, not just events: `fetchAll` returns an empty list both when a relay + // served us and had nothing AND when nothing answered at all (cannot-connect, CLOSED, idle + // timeout). Treating the second as "no list yet" is precisely how a read-merge-write wipes + // the signer_sk of every link it failed to read, so the two must be told apart. + val reasons = mutableMapOf() + val events = + account.client.fetchAllWithHooks( + filters = relays.associateWith { listOf(filter) }, + doneOut = reasons, + ) { _, _ -> true } + val newest = - account.client - .fetchAll(filters = relays.associateWith { listOf(filter) }) + events + .mapNotNull { it.second as? ConcordInviteListEvent } + // Filter by kind BEFORE picking the newest: taking the newest of anything and then + // casting means one stray event at this coordinate reads as "unreadable" forever. .maxByOrNull { it.createdAt } - ?: return ConcordInviteListDocument.EMPTY // nothing published yet — safe to start one - return (newest as? ConcordInviteListEvent)?.decrypt(account.signer) + ?: return if (reasons.anyRelayServed()) { + ConcordInviteListDocument.EMPTY // a relay answered and had nothing — safe to start one + } else { + null // nobody answered; we know nothing about what is published + } + return newest.decrypt(account.signer) } /** @@ -216,9 +239,11 @@ class AccountConcordActions( Log.w("Concord") { "Refusing to write the invite list: could not read the current one (would drop other links' signer_sk)" } return false } + // publishAndConfirm, never publish: `INostrClient.publish` returns Unit — it queues the event + // and never reports acceptance — so a `runCatching { publish(); true }` is true whenever + // local signing worked, and every caller's "did the record land?" gate becomes decorative. return runCatching { - account.client.publish(ConcordInviteListEvent.create(account.signer, ConcordInviteList.merge(base, patch), TimeUtils.now()), publishTo) - true + account.client.publishAndConfirm(ConcordInviteListEvent.create(account.signer, ConcordInviteList.merge(base, patch), TimeUtils.now()), publishTo) }.onFailure { Log.w("Concord", "invite list publish failed", it) }.getOrDefault(false) } @@ -243,30 +268,43 @@ class AccountConcordActions( val list = readConcordInviteList() ?: return 0 val tombstoned = list.tombstones.mapTo(HashSet()) { it.token } val now = TimeUtils.now() - var count = 0 - for (link in list.entries) { - if (link.communityId != entry.id) continue - // An elapsed or retired link can no longer be joined; re-posting it would only resurrect - // a dead URL at a live epoch. - if (link.isExpired(now) || link.token in tombstoned) continue - runCatching { - val token = link.token.hexToByteArray() - val wraps = account.client.fetchAll(filters = relays.associateWith { listOf(ConcordActions.bundleFilter(link.signerPubKeyHex())) }) - // Honour a revocation published at this coordinate, and carry the live bundle's own - // fields forward — only the epoch's key material changes. - val current = ConcordActions.classifyInvite(wraps, token) as? InviteBundleStatus.Live ?: return@runCatching - val moved = - current.invite.copy( - communityRoot = entry.root, - rootEpoch = entry.rootEpoch, - controlPk = entry.controlPk, - relays = entry.relays, - ) - account.client.publish(ConcordActions.remintBundleAt(link.signerSk.hexToByteArray(), token, moved, now), relays) - count++ - }.onFailure { Log.w("Concord", "invite refresh failed for ${entry.id}", it) } + + // An elapsed or retired link can no longer be joined; re-posting it would only resurrect a + // dead URL at a live epoch. + val links = list.entries.filter { it.communityId == entry.id && !it.isExpired(now) && it.token !in tombstoned } + if (links.isEmpty()) return 0 + + // One REQ for every link's bundle rather than a round trip each. This runs inside the + // user-visible Refounding, and a serial fetch per link makes a removal take time linear in + // how many links the creator ever minted, each able to wait out its own idle timeout. + val byAuthor = links.associateBy { it.signerPubKeyHex().lowercase() } + val wraps = account.client.fetchAll(filters = relays.associateWith { listOf(ConcordActions.bundlesFilter(byAuthor.keys.toList())) }) + val wrapsByAuthor = wraps.groupBy { it.pubKey.lowercase() } + + return coroutineScope { + byAuthor + .map { (author, link) -> + async { + runCatching { + val token = link.token.hexToByteArray() + // Classify per coordinate, never over the pooled set: one link's newer + // revocation tombstone must not decide another link's status. + val current = ConcordActions.classifyInvite(wrapsByAuthor[author].orEmpty(), token) as? InviteBundleStatus.Live ?: return@runCatching false + val moved = + current.invite.copy( + communityRoot = entry.root, + rootEpoch = entry.rootEpoch, + controlPk = entry.controlPk, + relays = entry.relays, + ) + // Confirmed: a link counted as moved but never stored is a link its + // holders can no longer redeem, reported as a success. + account.client.publishAndConfirm(ConcordActions.remintBundleAt(link.signerSk.hexToByteArray(), token, moved, now), relays) + }.onFailure { Log.w("Concord", "invite refresh failed for ${entry.id}", it) }.getOrDefault(false) + } + }.awaitAll() + .count { it } } - return count } /** @@ -393,10 +431,12 @@ class AccountConcordActions( val relays = entry.relays.mapNotNullTo(mutableSetOf()) { RelayUrlNormalizer.normalizeOrNull(it) }.ifEmpty { account.outboxRelays.flow.value } if (relays.isEmpty()) return false + // Confirmed, not fire-and-forget. A `publish` that returns Unit would report success for a + // tombstone no relay stored — and the list write below would then drop this entry on merge, + // destroying the only `signer_sk` that could ever retire the link while the link stays live. val published = runCatching { - account.client.publish(ConcordActions.revokeBundleAt(link.signerSk.hexToByteArray(), TimeUtils.now()), relays) - true + account.client.publishAndConfirm(ConcordActions.revokeBundleAt(link.signerSk.hexToByteArray(), TimeUtils.now()), relays) }.onFailure { Log.w("Concord", "invite revocation failed for $communityId", it) }.getOrDefault(false) if (!published) return false @@ -477,7 +517,15 @@ class AccountConcordActions( // other door into the same room. // // Fails CLOSED on an unreadable plane: the banlist is only knowable once the bundle yields - // the root, and no verdict means no join. + // the root, and no verdict means no join. Two things make that safe to insist on rather than + // a way to brick valid invites: + // + // - the plane is fetched over the SAME relays that just served the bundle, not the relay + // list inside the bundle alone, which can be stale (a moved relay, a link minted before a + // relay change) and would otherwise refuse a community we can plainly reach; + // - it is PAGED, because a single REQ is truncated at the relay's per-filter cap. A missing + // older ban edition fails the gate open — it re-admits the very account it exists to + // refuse — so the one direction we must not economise on is completeness. val joinKeys = ConcordActions.controlPlaneKeys( communityRoot = bundle.communityRoot.hexToByteArray(), @@ -485,12 +533,14 @@ class AccountConcordActions( rootEpoch = bundle.rootEpoch, controlPk = bundle.controlPk, ) - val joinRelays = bundle.relays.mapNotNullTo(mutableSetOf()) { RelayUrlNormalizer.normalizeOrNull(it) }.ifEmpty { relays } - val joinEditions = - ConcordActions.controlEditions( - account.client.fetchAll(filters = joinRelays.associateWith { listOf(ConcordActions.planeFilter(joinKeys.address)) }), - joinKeys, - ) + // Union, not `ifEmpty`: the relays that served the bundle are known-good for this community, + // and the bundle's own list is the one that goes stale. + val joinRelays = bundle.relays.mapNotNullTo(mutableSetOf()) { RelayUrlNormalizer.normalizeOrNull(it) } + relays + val planeWraps = mutableListOf() + account.client.fetchAllPagesFromPool( + filters = joinRelays.associateWith { listOf(ConcordActions.planeFilter(joinKeys.address)) }, + ) { event, _ -> planeWraps.add(event) } + val joinEditions = ConcordActions.controlEditions(planeWraps, joinKeys) if (joinEditions.isEmpty()) return ConcordInviteResult.NotReachable if (AuthorityResolver.resolve(joinEditions, bundle.owner).isBanned(account.signer.pubKey)) { return ConcordInviteResult.Banned @@ -1210,7 +1260,17 @@ class AccountConcordActions( // who has themselves been banned could still rotate the whole community. val authorized = authority.isOwner(received.rotator) || authority.hasPermission(received.rotator, ConcordPermissions.BAN) if (!authorized) continue - adoptConcordRoot(entry, received.newRoot, received.newEpoch, received.newControlPk, received.newControlRoot) + val adopted = adoptConcordRoot(entry, received.newRoot, received.newEpoch, received.newControlPk, received.newControlRoot) + + // Move our own links onto the epoch we just adopted. Rotating is not the only way to end + // up on a new epoch — being re-keyed is the common one — and a link creator who is merely + // re-keyed would otherwise leave every link they handed out pointing at the dead root, + // which is exactly the orphaning this branch exists to stop. Stranded recovery reads the + // bundle's epoch, so a link nobody re-mints is a member nobody can recover. + adopted?.let { next -> + val moved = refreshConcordInviteLinks(next) + if (moved > 0) Log.i("Concord") { "Rekey ${next.id}: refreshed $moved invite link(s) to epoch ${received.newEpoch}" } + } } } diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt index 02b482bebf..185cbc7f6d 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt @@ -582,12 +582,15 @@ class Context( diagnoseSlow: Boolean = false, deadOut: MutableMap? = null, pendingOnAuthRequired: Boolean = false, + /** Per-relay terminal reason, so a caller can tell an empty answer from no answer. */ + doneOut: MutableMap? = null, ): List> = client.fetchAllWithHooks( filters = filters, idleTimeoutMs = idleTimeoutMs, pendingOnAuthRequired = pendingOnAuthRequired, deadOut = deadOut, + doneOut = doneOut, onTimeout = if (diagnoseSlow) { { stalled, doneReasons, collected -> logSlowDrain(idleTimeoutMs, stalled, doneReasons, collected) } diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/ConcordCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/ConcordCommands.kt index 77047c4e5b..51ad02fd7e 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/ConcordCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/ConcordCommands.kt @@ -43,6 +43,7 @@ import com.vitorpamplona.quartz.concord.cord05Invites.InviteBundleStatus import com.vitorpamplona.quartz.concord.crypto.ControlPlaneKeys import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray import com.vitorpamplona.quartz.nip01Core.core.toHexKey +import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.anyRelayServed import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer @@ -724,13 +725,19 @@ object ConcordCommands { val relays = ctx.outboxRelays() if (relays.isEmpty()) return null val filter = Filter(kinds = listOf(ConcordInviteListEvent.KIND), authors = listOf(ctx.signer.pubKey)) + // Terminal reasons, not just events: a drain returns nothing both when a relay served us and + // had nothing AND when nobody answered. Reading the second as "no list yet" is how the + // read-merge-write below wipes the signer_sk of every link it failed to read. + val reasons = mutableMapOf() val newest = ctx - .drain(relays.associateWith { listOf(filter) }) - .map { it.second } + .drain(relays.associateWith { listOf(filter) }, doneOut = reasons) + // Filter by kind BEFORE picking the newest — a stray event at this coordinate would + // otherwise make the list read as unreadable and refuse every later write. + .mapNotNull { it.second as? ConcordInviteListEvent } .maxByOrNull { it.createdAt } - ?: return ConcordInviteListDocument.EMPTY // nothing published yet — safe to start one - return (newest as? ConcordInviteListEvent)?.decrypt(ctx.signer) + ?: return if (reasons.anyRelayServed()) ConcordInviteListDocument.EMPTY else null + return newest.decrypt(ctx.signer) } /** diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt index dfb51935d7..e11a4278fa 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt @@ -201,6 +201,16 @@ object ConcordActions { /** The public invite bundle for a link signer. */ fun bundleFilter(linkSignerPubKeyHex: HexKey): Filter = Filter(kinds = listOf(ConcordInviteBundleEvent.KIND), authors = listOf(linkSignerPubKeyHex)) + /** + * The bundles of several links at once — one REQ over every link signer instead of a round trip + * per link, which is what a Refounding needs when it re-mints a creator's whole set. + * + * Partition the result by `pubKey` before classifying: [ConcordInviteBundle.classify] resolves a + * single coordinate, so handing it a pooled set would let one link's revocation tombstone decide + * another link's status purely by being newer. + */ + fun bundlesFilter(linkSignerPubKeyHexes: List): Filter = Filter(kinds = listOf(ConcordInviteBundleEvent.KIND), authors = linkSignerPubKeyHexes) + /** Pending direct invites addressed to the given member (indexed by k=3313). */ fun directInvitesFilter(memberPubKeyHex: HexKey): Filter = Filter(kinds = listOf(ConcordStreamEnvelope.KIND_WRAP), tags = mapOf("p" to listOf(memberPubKeyHex), "k" to listOf(ConcordDirectInvite.KIND.toString()))) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteList.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteList.kt index 5b930eb261..f136aed3fd 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteList.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteList.kt @@ -30,9 +30,11 @@ import kotlinx.serialization.KSerializer import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable import kotlinx.serialization.descriptors.elementNames +import kotlinx.serialization.json.JsonArray import kotlinx.serialization.json.JsonElement import kotlinx.serialization.json.JsonObject import kotlinx.serialization.json.JsonTransformingSerializer +import kotlinx.serialization.json.jsonArray import kotlinx.serialization.json.jsonObject private val NoExtras: JsonObject = JsonObject(emptyMap()) @@ -77,11 +79,19 @@ class ConcordInviteListTombstone( val residue: JsonObject = NoExtras, ) -/** The decoded kind-13303 document: live [entries], [tombstones], and document-level [residue]. */ +/** + * The decoded kind-13303 document: live [entries], [tombstones], and document-level [residue]. + * + * [opaqueEntries] holds entries that did not type-check — a wrong-typed field from another client or + * a newer schema. They are carried verbatim rather than dropped (re-encoding without them would + * delete somebody's `signer_sk`) and rather than failing the whole read (which would refuse every + * future mint and revoke for this account until someone else repaired the list). + */ class ConcordInviteListDocument( val entries: List = emptyList(), val tombstones: List = emptyList(), val residue: JsonObject = NoExtras, + val opaqueEntries: List = emptyList(), ) { companion object { val EMPTY = ConcordInviteListDocument() @@ -160,41 +170,80 @@ object ConcordInviteList { private object WireDocumentSerializer : ExtrasPreserving(WireDocument.serializer()) /** - * Decodes the plaintext document, or **null** when it cannot be parsed. + * Decodes the plaintext document, or **null** when the document itself cannot be read. * * Null rather than an empty document on purpose: this list is replaceable, so a caller that * treats "I could not read it" as "it is empty" and republishes destroys every `signer_sk` it * did not manage to read — secrets that cannot be regenerated, orphaning every outstanding * invite at a dead epoch. Callers MUST distinguish the two (see [ConcordInviteList.merge]'s - * callers). Each field still defaults, so one odd entry does not abort the whole array. + * callers). + * + * Null is reserved for a *document-level* failure — not JSON, or `entries`/`tombstones` present + * but not arrays. A single entry that does not type-check is kept verbatim in + * [ConcordInviteListDocument.opaqueEntries] instead: failing the whole read for one odd row + * would refuse every future mint and revoke for the account, permanently, since a replaceable + * coordinate never ages out — turning the old silent data loss into a permanent write lock. */ fun decodeOrNull(json: String): ConcordInviteListDocument? = try { - val doc = ConcordJson.instance.decodeFromString(WireDocumentSerializer, json) - ConcordInviteListDocument( - entries = - doc.entries.map { + val root = ConcordJson.instance.parseToJsonElement(json).jsonObject + val opaque = mutableListOf() + + val entries = + (root["entries"]?.jsonArray ?: JsonArray(emptyList())).mapNotNull { element -> + val obj = element.jsonObject + try { + val it = ConcordJson.instance.decodeFromJsonElement(WireEntrySerializer, obj) ConcordInviteListEntry(it.token, it.signerSk, it.communityId, it.url, it.label, it.createdAt, it.expiresAt, it.extras) - }, - tombstones = doc.tombstones.map { ConcordInviteListTombstone(it.token, it.communityId, it.extras) }, - residue = doc.extras, + } catch (_: Exception) { + opaque.add(obj) + null + } + } + + val tombstones = + (root["tombstones"]?.jsonArray ?: JsonArray(emptyList())).mapNotNull { element -> + try { + val it = ConcordJson.instance.decodeFromJsonElement(WireTombstoneSerializer, element.jsonObject) + ConcordInviteListTombstone(it.token, it.communityId, it.extras) + } catch (_: Exception) { + // A tombstone we cannot read must not silently un-retire its link, but we + // have no token to key it by, so it can only ride along as document residue. + null + } + } + + ConcordInviteListDocument( + entries = entries, + tombstones = tombstones, + residue = JsonObject(root - "entries" - "tombstones"), + opaqueEntries = opaque, ) } catch (_: Exception) { null } - fun encode(doc: ConcordInviteListDocument): String = - ConcordJson.instance.encodeToString( - WireDocumentSerializer, - WireDocument( - entries = - doc.entries.map { - WireEntry(it.token, it.signerSk, it.communityId, it.url, it.label, it.createdAt, it.expiresAt, it.residue) - }, - tombstones = doc.tombstones.map { WireTombstone(it.token, it.communityId, it.residue) }, - extras = doc.residue, - ), - ) + fun encode(doc: ConcordInviteListDocument): String { + val wire = + ConcordJson.instance + .encodeToJsonElement( + WireDocumentSerializer, + WireDocument( + entries = + doc.entries.map { + WireEntry(it.token, it.signerSk, it.communityId, it.url, it.label, it.createdAt, it.expiresAt, it.residue) + }, + tombstones = doc.tombstones.map { WireTombstone(it.token, it.communityId, it.residue) }, + extras = doc.residue, + ), + ).jsonObject + + // Entries we could not type ride back out untouched. Dropping them here is the data loss + // this whole class exists to prevent — they are somebody's link signer too. + if (doc.opaqueEntries.isEmpty()) return ConcordJson.instance.encodeToString(JsonObject.serializer(), wire) + val entries = JsonArray((wire["entries"]?.jsonArray ?: JsonArray(emptyList())) + doc.opaqueEntries) + return ConcordJson.instance.encodeToString(JsonObject.serializer(), JsonObject(wire + ("entries" to entries))) + } /** * Merges [patch] onto [base], keyed by `token` — the spec's own merge key. A token present in @@ -218,6 +267,9 @@ object ConcordInviteList { entries = entries.values.toList(), tombstones = tombstones.values.toList(), residue = JsonObject(base.residue + patch.residue), + // Untyped entries survive the merge for the same reason they survive a decode: we cannot + // read them, so we are in no position to decide they are disposable. + opaqueEntries = (base.opaqueEntries + patch.opaqueEntries).distinct(), ) } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt index 62d13ce5e9..f28864dca0 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt @@ -79,6 +79,14 @@ suspend fun INostrClient.fetchAllWithHooks( subscriptionId: String = newSubId(), pendingOnAuthRequired: Boolean = false, deadOut: MutableMap? = null, + /** + * Receives the terminal reason per relay ("eose", "closed:…", "cannot:…"), so a caller can + * tell "a relay served us and had nothing" from "nobody served us". An empty result alone + * cannot: both look like zero events, and treating the second as the first is how a + * read-merge-write on a replaceable event destroys the entries it failed to read. See + * [anyRelayServed]. + */ + doneOut: MutableMap? = null, onTimeout: ((stalled: Set, doneReasons: Map, collected: List>) -> Unit)? = null, /** * Hard wall-clock ceiling. The idle window alone is unbounded when a relay @@ -237,9 +245,22 @@ suspend fun INostrClient.fetchAllWithHooks( classifyDrainFailure(reason)?.let { out[relay] = it } } } + doneOut?.putAll(doneReasons) return collected } +/** The terminal reason recorded when a relay finished serving a subscription normally. */ +const val DONE_REASON_EOSE = "eose" + +/** + * True when at least one relay completed the fetch normally, i.e. answered and reached EOSE. + * + * Read against the map filled by `fetchAllWithHooks`'s `doneOut`. An empty event list means + * "nothing matched" only when this is true; otherwise it means "nobody told us", and a caller + * that overwrites a replaceable event on that basis deletes whatever it could not read. + */ +fun Map.anyRelayServed(): Boolean = values.any { it == DONE_REASON_EOSE } + /** * [fetchAllPagesFromPool] with a suspending per-event hook: paginates every relay * to completion (each on its own `until` cursor, up to [maxConcurrentRelays] at diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteListTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteListTest.kt index 7a3904bdbc..b3a6dbde5e 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteListTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord05Invites/ConcordInviteListTest.kt @@ -123,6 +123,43 @@ class ConcordInviteListTest { assertEquals(null, ConcordInviteList.decodeOrNull("{\"entries\":\"wrong type\"}")) } + @Test + fun oneUnreadableEntryDoesNotFailTheWholeDocumentOrGetDropped() { + // One structurally incompatible entry — a newer schema turning a scalar into an object, the + // realistic version, since the lenient parser already coerces plain scalar mismatches — used + // to null the whole document. Because null now means "refuse to write", that turned a single + // odd row into a permanent lock on mint and revoke for the account: a replaceable coordinate + // never ages out, so nothing would ever clear it. + val mixed = + """ + { "entries": [ + { "token": "aa", "signer_sk": "bb", "community_id": "cc", "url": "u1" }, + { "token": {"v": "dd"}, "signer_sk": "dd", "community_id": "cc", "url": "u2", "mark": "keepme" } + ], + "tombstones": [] } + """.trimIndent() + + val doc = ConcordInviteList.decodeOrNull(mixed) + assertEquals(listOf("aa"), doc!!.entries.map { it.token }, "the readable entry still decodes") + assertEquals(1, doc.opaqueEntries.size, "the unreadable entry is kept, not discarded") + + // And it survives a re-encode: dropping it would delete somebody's signer_sk, which is the + // exact data loss this class exists to prevent. + assertTrue(ConcordInviteList.encode(doc).contains("keepme"), "unreadable entry lost on re-encode") + } + + @Test + fun aMergeCarriesUnreadableEntriesThrough() { + val base = ConcordInviteList.decodeOrNull("""{"entries":[{"token":{"v":7},"mark":"opaque"}],"tombstones":[]}""")!! + val patch = ConcordInviteListDocument(entries = listOf(ConcordInviteListEntry("t", "sk", "c", "u"))) + + val merged = ConcordInviteList.merge(base, patch) + + assertEquals(listOf("t"), merged.entries.map { it.token }) + // We cannot read it, so we are in no position to decide it is disposable. + assertTrue(ConcordInviteList.encode(merged).contains("opaque"), "merge dropped an unreadable entry") + } + @Test fun anUnreadableListIsDistinguishableFromAnEmptyOne() { // The whole point of the null: a caller must be able to tell "I could not read it" from