From f60d96e00ae252a9ac22d13af046a1188a3d4c2c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 19:58:54 +0000 Subject: [PATCH] feat(cordn): mark the searched people a coordinator can actually reach Inviting failed only at the tap: cordn adds a member by spending a KeyPackage they published to this coordinator, and the search offered everyone in the address book with no hint which of them qualified. `kp_list` is the only non-destructive way to learn who did -- `kp_take` by stable identity consumes one of their single-use packages (spec/00.md, coordinator retrieval behavior), so it can never be a probe. It takes no arguments and answers with every identity the coordinator holds a package for, which means one call covers everybody and per-person probing would be the same download repeated. So CordnKeyPackages now keeps that response as a snapshot: identities for everyone, full entries for us, reused for a minute. Cost is unchanged. topUp/ensureLastResort/listPublished already made this exact call and discarded everyone else's rows; they now go through refresh(), which warms the snapshot on the way past. The badge is fetched only once the field is in use, so opening group info downloads nothing. Publish decisions never read the reused copy. Minting against a stale listing is the one place staleness does damage -- the coordinator keeps one last-resort package per identity, so a second publish silently evicts the first and strands every invite that referenced it. publishNew/withdraw drop the snapshot. Only identities are retained for other accounts, not their kp_refs: a Welcome is addressed to a ref obtained by taking the package, never to one read out of a listing, and on a busy coordinator those refs are the bulk of an unpaginated response. The badge marks the yes and says nothing about the no, because the answer has three states: published here, not published here, and a lookup that never came back. Only membership was something we were told, so an unmarked row asserts nothing and stays fully tappable -- a "cannot be added" marker would turn an unreachable coordinator into a claim about a person. Not exercised on a device. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012BfD4txdnsaPRXmNXbup9n --- .../amethyst/model/cordn/CordnRuntime.kt | 20 ++++ .../chats/cordnGroup/CordnGroupInfoScreen.kt | 53 +++++++-- amethyst/src/main/res/values/strings.xml | 1 + .../commons/cordn/CordnKeyPackages.kt | 106 +++++++++++++++++- .../commons/cordn/CordnKeyPackagesTest.kt | 60 ++++++++++ 5 files changed, 226 insertions(+), 14 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/cordn/CordnRuntime.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/cordn/CordnRuntime.kt index f0308dfce6..28b35f98fa 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/cordn/CordnRuntime.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/cordn/CordnRuntime.kt @@ -643,6 +643,26 @@ class CordnRuntime( } } + /** + * Which identities [coordinatorPubKey] can deliver a Welcome to, or null. + * + * For annotating people you are *about* to invite. cordn can only add a + * member by spending a KeyPackage they published to this coordinator, and + * `kp_list` is the only non-destructive way to learn who did -- `kp_take` + * by identity consumes one of their single-use packages + * (`spec/00.md` §"Coordinator retrieval behavior"), so it can never be + * used as a probe. One call answers for everybody, which is why this hands + * back the whole set instead of taking a pubkey. + * + * Null is **not** an empty set. It means the question went unanswered, and + * a caller that renders it as "cannot be added" turns a network failure + * into a claim about a person. Distinguish them. + */ + suspend fun identitiesWithKeyPackages(coordinatorPubKey: HexKey): Set? { + val session = registry.sessionOrNull(coordinatorPubKey) ?: return null + return session.keyPackages.identitiesWithKeyPackages() + } + /** Publishes one KeyPackage. An attributable act under the account key (§8.4). */ suspend fun publishKeyPackage( coordinatorPubKey: HexKey, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt index 12a01340ac..134ed0b893 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt @@ -411,9 +411,18 @@ private fun CordnGroupInfo( // everybody would build commits the rest of the group drops on // receipt. if (canAdminister) { + // Asked only once the field is in use. `kp_list` is unpaginated, + // so opening this screen must not download the coordinator's + // whole table on behalf of somebody who never types. + val searching = memberSearch.length > 2 + val reachable by + produceState?>(null, runtime, coordinatorPubKey, searching) { + if (searching) value = runtime?.identitiesWithKeyPackages(coordinatorPubKey) + } CordnAddMember( userSuggestions = userSuggestions, search = memberSearch, + reachable = reachable, onSearchChange = { memberSearch = it adminError = null @@ -537,16 +546,28 @@ private fun CordnGroupInfo( * ## The search finding somebody is not a promise * * cordn can only add a member by spending a KeyPackage they published **to - * this coordinator** (`spec/00.md`), and nothing on this device can know - * whether they did until the coordinator is asked. So the note says so before - * the attempt, and `CordnGroupManager.invite` says which of the two went wrong - * after it -- "the coordinator holds no KeyPackage for ..." is a different - * problem from a name that matched nobody, and they are worth telling apart. + * this coordinator** (`spec/00.md`). [reachable] is who the coordinator says + * did, so the common disappointment is visible before the tap rather than + * after it, and `CordnGroupManager.invite` still says which of the two went + * wrong when one does -- "the coordinator holds no KeyPackage for ..." is a + * different problem from a name that matched nobody. + * + * ## Why the badge marks the yes and says nothing about the no + * + * [reachable] has three states, not two: a set, an empty set, and null for a + * lookup that never came back (`CordnRuntime.identitiesWithKeyPackages`). Only + * membership in it is something we were told; absence covers both "has not + * published here" and "we could not ask". Marking the yes therefore asserts + * exactly what we verified, and an unmarked row asserts nothing -- where a + * "cannot be added" marker would turn an unreachable coordinator into a claim + * about a person. Unmarked rows stay fully tappable for the same reason: the + * listing can be up to a minute stale, and the coordinator gets the last word. */ @Composable private fun CordnAddMember( userSuggestions: UserSuggestionState, search: String, + reachable: Set?, onSearchChange: (String) -> Unit, busy: Boolean, accountViewModel: AccountViewModel, @@ -587,12 +608,22 @@ private fun CordnAddMember( ) }, trailingContent = { user -> - IconButton(onClick = { onInvite(user) }) { - Icon( - symbol = MaterialSymbols.PersonAdd, - contentDescription = stringRes(R.string.cordn_info_add_member), - tint = MaterialTheme.colorScheme.primary, - ) + Row(verticalAlignment = Alignment.CenterVertically) { + if (reachable?.contains(user.pubkeyHex) == true) { + Icon( + symbol = MaterialSymbols.Key, + contentDescription = stringRes(R.string.cordn_info_has_key_package), + modifier = Modifier.size(16.dp), + tint = MaterialTheme.colorScheme.primary, + ) + } + IconButton(onClick = { onInvite(user) }) { + Icon( + symbol = MaterialSymbols.PersonAdd, + contentDescription = stringRes(R.string.cordn_info_add_member), + tint = MaterialTheme.colorScheme.primary, + ) + } } }, ) diff --git a/amethyst/src/main/res/values/strings.xml b/amethyst/src/main/res/values/strings.xml index d1690e6ea6..5117970a66 100644 --- a/amethyst/src/main/res/values/strings.xml +++ b/amethyst/src/main/res/values/strings.xml @@ -406,6 +406,7 @@ Last announced %1$s ago Add someone Name, npub or name@domain + Can be added on this coordinator They can only be added if they published a key package to this coordinator. What this coordinator can see Got it diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackages.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackages.kt index 35a0b1f549..f86800ce4d 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackages.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackages.kt @@ -29,9 +29,13 @@ import com.vitorpamplona.quartz.mls.messages.KeyPackageBundle import com.vitorpamplona.quartz.mls.messages.KeyPackageBundleCodec import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.core.toHexKey +import com.vitorpamplona.quartz.utils.TimeUtils import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import kotlin.coroutines.cancellation.CancellationException import kotlin.io.encoding.Base64 import kotlin.io.encoding.ExperimentalEncodingApi @@ -122,6 +126,89 @@ class CordnKeyPackages( val at: Long, ) + private val snapshotLock = Mutex() + private var snapshot: Snapshot? = null + + /** + * What `kp_list` reported, the last time we asked. + * + * `kp_list` takes no arguments and returns every KeyPackage the coordinator + * holds for **every** identity (`spec/00.md` leaves retrieval scoping to the + * coordinator; the reference implementation answers with the whole table). + * So one call answers a question about anybody, and asking it once per + * person would be the same download repeated. + * + * [identities] keeps only pubkeys, not the entries. Another account's + * `kp_ref` is useless to us -- a Welcome is addressed to a ref we obtained + * by *taking* the package, never to one we read out of a listing -- and on + * a busy coordinator those refs are the bulk of the response. Dropping them + * bounds what we retain even though nothing bounds what arrives. + */ + class Snapshot( + /** Every identity the coordinator holds at least one KeyPackage for. */ + val identities: Set, + /** The full entries for this account, which are the ones we act on. */ + val mine: List, + /** When this was fetched, for [snapshot]'s age check. */ + val at: Long, + ) + + /** + * Refetches unconditionally and replaces the snapshot. + * + * Every decision about whether to *publish* reads through here rather than + * through [snapshot]. Minting against a stale listing is the one place + * staleness does damage: the coordinator keeps a single last-resort package + * per identity, so publishing a second silently evicts the first and + * strands every invite that referenced it. + */ + suspend fun refresh(): Snapshot = snapshotLock.withLock { fetchLocked() } + + /** + * The snapshot, refetched if it is older than [maxAgeSeconds]. + * + * For reads that only *describe* what the coordinator holds. A minute of + * staleness costs at most a missing badge for somebody who published within + * it, and the act itself still fails loudly at the coordinator if the + * listing was wrong -- whereas every refresh is another full-table + * download, so asking less often is the cheap side of the trade. + */ + suspend fun snapshot(maxAgeSeconds: Long = SNAPSHOT_TTL_SECONDS): Snapshot = + snapshotLock.withLock { + snapshot?.takeIf { TimeUtils.now() - it.at < maxAgeSeconds } ?: fetchLocked() + } + + /** + * Which identities the coordinator can deliver a Welcome to, or null. + * + * Null means **we do not know** -- the coordinator is unreachable, throttled + * us, or answered with more than the transport admits. It does not mean + * nobody has published. Callers must keep those apart: rendering a failed + * lookup as "this person cannot be added" states something about a person + * on the strength of a network error. + */ + suspend fun identitiesWithKeyPackages(): Set? = + try { + snapshot().identities + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + null + } + + /** Must hold [snapshotLock]. */ + private suspend fun fetchLocked(): Snapshot { + val all = coordinator.listKeyPackages() + return Snapshot( + identities = all.mapTo(mutableSetOf()) { it.pubKey }, + mine = all.filter { it.pubKey == accountPubKey }, + at = TimeUtils.now(), + ).also { snapshot = it } + } + + /** Forgets the snapshot after we changed what the coordinator holds. */ + private suspend fun invalidate() = snapshotLock.withLock { snapshot = null } + /** Reloads which refs we hold private halves for. Call at startup. */ suspend fun restore() { _published.value = store.list().toSet() @@ -150,7 +237,7 @@ class CordnKeyPackages( * is actually available the moment anyone invites us — and it is what is * available that decides whether the next invitation can happen. */ - suspend fun listPublished(): List = coordinator.listKeyPackages().filter { it.pubKey == accountPubKey } + suspend fun listPublished(): List = refresh().mine /** * Generates a KeyPackage, keeps its private half, and publishes it. @@ -184,6 +271,7 @@ class CordnKeyPackages( throw e } + invalidate() return Published(result.keyPackageRef, result.lastResort || lastResort, result.at) } @@ -219,6 +307,7 @@ class CordnKeyPackages( val removed = coordinator.removeKeyPackages(keyPackageRefs) removed.forEach { store.delete(it) } _published.value = _published.value - removed.toSet() + invalidate() return removed } @@ -233,7 +322,7 @@ class CordnKeyPackages( */ suspend fun topUp(minimum: Int = DEFAULT_POOL): List { require(minimum >= 0) { "a KeyPackage pool cannot be negative" } - val theirs = coordinator.listKeyPackages().filter { it.pubKey == accountPubKey && !it.lastResort } + val theirs = refresh().mine.filter { !it.lastResort } val missing = minimum - theirs.size if (missing <= 0) return emptyList() return List(missing) { publishNew().keyPackageRef } @@ -248,7 +337,7 @@ class CordnKeyPackages( * not the norm. */ suspend fun ensureLastResort(): String? { - val existing = coordinator.listKeyPackages().filter { it.pubKey == accountPubKey && it.lastResort } + val existing = refresh().mine.filter { it.lastResort } // Only one, and only one we can still open: a last-resort package whose // private half this device never had is useless to it. val usable = existing.firstOrNull { store.load(it.keyPackageRef) != null } @@ -292,5 +381,16 @@ class CordnKeyPackages( * last-resort package is the safety net. */ const val DEFAULT_POOL = 5 + + /** + * How long a [Snapshot] may be reused by [snapshot]. + * + * Bounded by what a refresh costs rather than by how fast the answer + * changes: `kp_list` is unpaginated, so each one downloads the + * coordinator's whole table. A minute is long enough that opening a + * screen repeatedly costs one call and short enough that somebody who + * has just published becomes visible while you are still looking. + */ + const val SNAPSHOT_TTL_SECONDS = 60L } } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackagesTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackagesTest.kt index 1075121fe7..c4553df8a5 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackagesTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnKeyPackagesTest.kt @@ -21,6 +21,8 @@ package com.vitorpamplona.amethyst.commons.cordn import com.vitorpamplona.quartz.cordn.groups.CordnCredential +import com.vitorpamplona.quartz.cordn.spec00Coordinator.AvailableKeyPackage +import com.vitorpamplona.quartz.cordn.spec00Coordinator.ICoordinator import com.vitorpamplona.quartz.cordn.spec00Coordinator.KeyPackagePublication import com.vitorpamplona.quartz.mls.codec.TlsReader import com.vitorpamplona.quartz.mls.messages.KeyPackageBundleCodec @@ -335,4 +337,62 @@ class CordnKeyPackagesTest : CordnTransportHarness() { assertTrue(driving { alice.keyPackages.listPublished() }.all { it.pubKey == alice.pubKey }) } + + @Test + fun `the identity set spans everybody, where listPublished is scoped to us`() = + runTest { + // The two read one `kp_list` response and keep different halves of + // it. Scoping the identity set the way listPublished is scoped + // would answer "can I add Bob" with our own packages, which is + // always no. + val alice = Account() + val bob = Account() + driving { alice.keyPackages.publishNew() } + driving { bob.keyPackages.publishNew() } + + val reachable = assertNotNull(driving { alice.keyPackages.identitiesWithKeyPackages() }) + assertTrue(alice.pubKey in reachable, "ourselves") + assertTrue(bob.pubKey in reachable, "somebody else") + } + + @Test + fun `publishing shows up straight away rather than at the end of the snapshot's life`() = + runTest { + // The snapshot is reused for a minute, so our own publish has to + // drop it. Otherwise the account that just published a package is + // told for the next minute that it has none. + val alice = Account() + assertTrue(driving { alice.keyPackages.identitiesWithKeyPackages() }?.contains(alice.pubKey) == false) + + driving { alice.keyPackages.publishNew() } + + assertTrue(driving { alice.keyPackages.identitiesWithKeyPackages() }?.contains(alice.pubKey) == true) + } + + @Test + fun `a coordinator that cannot answer is unknown, not nobody`() = + runTest { + // The distinction the whole return type exists for. An empty set + // says the coordinator holds nothing for anyone; null says we never + // found out. A caller that folds them together renders a network + // failure as a claim that somebody cannot be added. + val alice = Account() + driving { alice.keyPackages.publishNew() } + + val blind = + CordnKeyPackages( + accountPubKey = alice.pubKey, + coordinator = UnreachableList(alice.coordinatorClient), + store = InMemoryCordnKeyPackageStore(), + ) + + assertNull(driving { blind.identitiesWithKeyPackages() }) + } + + /** A coordinator whose `kp_list` is the one call that fails. */ + private class UnreachableList( + delegate: ICoordinator, + ) : ICoordinator by delegate { + override suspend fun listKeyPackages(): List = throw IllegalStateException("kp_list unreachable") + } }