fix(subgroups): a subgroup may be the whole group

"A subgroup cannot be the whole group. Leave at least one member out." That rule
shipped in Phase 8 and it was wrong twice.

**It refused something legitimate.** A subgroup is a logical division -- a group
deciding that some of its work belongs to a differently-keyed room -- not a group
carving out a smaller membership. Every member being in it is an ordinary case,
and no guard here had any business deciding otherwise.

**And it was a proxy, not a check.** The thing it stood in for is real: a ceremony
room is derived from its admins, so a subgroup over everybody lands in the room
the group's *own* ceremony was held in, and `proposeRitual` handing back that
ceremony would quietly make the child the parent. But set size does not detect
that. A parent whose membership has changed since its own ceremony derives a
different room -- so the sizes can match with no collision, and differ with one.

**Sharing the room was never the problem; sharing a ceremony was.**
`DkgSession.parentChatRoomId` already told two ceremonies apart, so the fix is to
scope the lookup by it rather than to forbid the selection.
`DkgSessionDao.getLatestSessionFor(room, parent)` replaces `...ForChatRoom` at the
three places that decide whether a ceremony already exists: `proposeRitual`'s
one-at-a-time guard, `refuseCeremonyRoom`, and the two repository observers the
ritual screen follows. A room may now hold the group's own ceremony and a
subgroup's at once.

Nothing below that lookup had to learn about the second one. A ceremony's
messages, approvals and transcript are already keyed by session id; only the
question "what is this room's current ceremony" was ever room-scoped, and that
question was always really "for what purpose".

One collision survives and it is degenerate: the same parent, over the same
admins, twice. Those two have nothing left to distinguish them -- which is another
way of saying they are one subgroup asked for twice, and that is what the message
now says.

`a subgroup that is the whole group is refused` becomes `a subgroup may be the
whole group`, and two new cases pin the scoping: the group's own ceremony sitting
in the derived room does not block a subgroup there, and the same parent asking
twice over the same admins still does while a different admin set is untouched.

`docs/subgroups.md` keeps the withdrawn rule struck through in the refusals table
with a section saying why, rather than quietly deleting it -- the reasoning that
led to it is the reasoning somebody would repeat.

397 common tests, 713 jvm tests, `m3Audit` meets every budget.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
2026-09-09 00:38:49 +02:00
parent ccaef5d36a
commit 1930d6aaef
8 changed files with 236 additions and 85 deletions

View File

@@ -27,6 +27,42 @@ interface DkgSessionDao {
@Query("SELECT * FROM DkgSession WHERE chatRoomId = :chatRoomId ORDER BY createdAt DESC LIMIT 1")
suspend fun getLatestSessionForChatRoom(chatRoomId: String): DkgSession?
/**
* The room's newest ceremony *for one purpose*: the group's own when
* [parentChatRoomId] is null, or the subgroup of that parent when it is not.
*
* A room can hold more than one. The ceremony room for a set of admins is
* derived from those admins, so a subgroup whose admins are everybody lands
* in the room the group's own ceremony was held in -- and that is a legitimate
* subgroup rather than a mistake, because a subgroup is a logical division and
* not a smaller membership.
*
* `DkgSession.parentChatRoomId` is what tells them apart, and it is enough
* because it is the only thing that differs: two ceremonies in one room are
* either one group's and one subgroup's, or two subgroups' of different
* parents. Two subgroups of the *same* parent over the *same* admins would
* still collide, and that pair is one subgroup asked for twice.
*
* Everything else about a ceremony is already keyed by session id -- the
* messages, the approvals, the transcript -- so nothing below this needed to
* learn about the second one.
*/
@Query(
"SELECT * FROM DkgSession WHERE chatRoomId = :chatRoomId AND " +
"(parentChatRoomId = :parentChatRoomId OR " +
"(:parentChatRoomId IS NULL AND parentChatRoomId IS NULL)) " +
"ORDER BY createdAt DESC LIMIT 1"
)
suspend fun getLatestSessionFor(chatRoomId: String, parentChatRoomId: String?): DkgSession?
@Query(
"SELECT * FROM DkgSession WHERE chatRoomId = :chatRoomId AND " +
"(parentChatRoomId = :parentChatRoomId OR " +
"(:parentChatRoomId IS NULL AND parentChatRoomId IS NULL)) " +
"ORDER BY createdAt DESC LIMIT 1"
)
fun observeLatestSessionFor(chatRoomId: String, parentChatRoomId: String?): Flow<DkgSession?>
/**
* Every ceremony this device came out of holding a share, newest first.
*

View File

@@ -28,8 +28,11 @@ class DatabaseDkgRepository(
): DkgRepository {
private val logger = Logger.withTag(TAG)
override fun observeLatestSessionForChatRoom(chatRoomId: String): Flow<DkgSession?> =
database.dkgSessionDao().observeLatestSessionForChatRoom(chatRoomId)
override fun observeLatestSessionForChatRoom(
chatRoomId: String,
parentChatRoomId: String?,
): Flow<DkgSession?> =
database.dkgSessionDao().observeLatestSessionFor(chatRoomId, parentChatRoomId)
override fun observeMessages(sessionId: String): Flow<List<DkgParticipantMessage>> =
database.dkgSessionDao().observeMessages(sessionId)
@@ -86,7 +89,10 @@ class DatabaseDkgRepository(
): Flow<List<LocalFrostSigningSession>> =
database.frostSigningSessionDao().observeSessionsForChatRoom(chatRoomId)
override fun observeSignedGroupKeyState(chatRoomId: String): Flow<GroupKeyState?> =
override fun observeSignedGroupKeyState(
chatRoomId: String,
parentChatRoomId: String?,
): Flow<GroupKeyState?> =
database.groupSignedEventDao()
.observeByKind(GroupKeyStateEvent.KIND)
.map { signedEvents ->
@@ -94,7 +100,7 @@ class DatabaseDkgRepository(
// rather than once: the ceremony has no key until it finishes,
// and this flow is running before it does.
val adminRoomId = database.dkgSessionDao()
.getLatestSessionForChatRoom(chatRoomId)
.getLatestSessionFor(chatRoomId, parentChatRoomId)
?.thresholdPublicKey
?.let { runCatching { SharedKeyDerivation.marmotGroupId(it) }.getOrNull() }
@@ -149,7 +155,7 @@ class DatabaseDkgRepository(
// rather than once: the ceremony has no key until it finishes,
// and this flow is running before it does.
val subgroupChatRoomId = database.dkgSessionDao()
.getLatestSessionForChatRoom(ceremonyChatRoomId)
.getLatestSessionFor(ceremonyChatRoomId, parentChatRoomId)
?.thresholdPublicKey
?.let { runCatching { SharedKeyDerivation.marmotGroupId(it) }.getOrNull() }
?: return@map null

View File

@@ -125,17 +125,24 @@ object ChillDkgRitualManager {
threshold: Int,
parentChatRoomId: HexKey? = null
): DkgSession {
// A room runs one ceremony at a time, and this is reachable twice now that
// a robust group opens one as it is created: the room id is derived from
// its members, so making the same group again lands back in the same room
// and asks again. A second proposal is a second participant set for every
// member to reconcile, and the key the first one produced would be left
// with nothing pointing at it. A failed ritual is not running, and is
// there to be replaced.
// A room runs one ceremony at a time *for one purpose*, and this is
// reachable twice now that a robust group opens one as it is created: the
// room id is derived from its members, so making the same group again
// lands back in the same room and asks again. A second proposal for the
// same purpose is a second participant set for every member to reconcile,
// and the key the first one produced would be left with nothing pointing
// at it. A failed ritual is not running, and is there to be replaced.
//
// Scoped by [parentChatRoomId] rather than by room alone. A subgroup's
// ceremony room is derived from its admins, so a subgroup whose admins are
// the whole group lands in the room the group's own ceremony was held in
// -- and that is a legitimate subgroup, since a subgroup is a logical
// division rather than a smaller membership. Refusing it on the room would
// hand back the *group's* key and quietly make the child the parent.
//
// Checked before the arguments are, because a running ritual makes the
// requested threshold moot -- it settled that question when it opened.
database.dkgSessionDao().getLatestSessionForChatRoom(localChatRoom.chatRoom.id)
database.dkgSessionDao().getLatestSessionFor(localChatRoom.chatRoom.id, parentChatRoomId)
?.takeIf { it.stage != DkgRitualStage.FAILED }
?.let { running ->
logger.i(

View File

@@ -157,21 +157,26 @@ object SubgroupManager {
return "Everyone in a subgroup has to be in this group first."
}
// A proper subset, and this is not fussiness. The ceremony room is derived
// from its members, so selecting the whole group derives the room this
// group's own ceremony was held in -- and `proposeRitual` would hand back
// that ceremony, making the "child" this very group.
if (admins.size == members.size) {
return "A subgroup cannot be the whole group. Leave at least one member out."
}
// Any completed or running ceremony in the room these admins derive is the
// same trap one step removed: two subgroups with exactly the same admins
// would share a key and therefore a room id.
// A subgroup *may* be the whole group. It is a logical division -- a group
// deciding that some of its work belongs to a differently-keyed room --
// rather than a smaller membership, so "everybody" is a normal answer and
// was never this function's business to refuse.
//
// What the old rule was standing in for is real and is checked below,
// precisely: the ceremony room is derived from its admins, so a subgroup
// over everybody lands in the room the group's own ceremony was held in.
// Sharing the room is fine; sharing a *ceremony* is not, and
// `DkgSession.parentChatRoomId` is what keeps them apart.
//
// Checking on set size was wrong twice over. It refused a legitimate
// subgroup, and it refused it on a proxy: a parent whose membership has
// changed since its own ceremony derives a different room, so the sizes
// could match with no collision at all, and could differ with one.
val ceremonyRoomId = ceremonyRoomIdFor(adminPublicKeys, coordinatorPublicKey)
val running = database.dkgSessionDao().getLatestSessionForChatRoom(ceremonyRoomId)
val running = database.dkgSessionDao()
.getLatestSessionFor(ceremonyRoomId, parentChatRoomId)
if (running != null && running.stage != DkgRitualStage.FAILED) {
return "These members already hold a shared key together. Change who is in the subgroup."
return "This group already has a subgroup run by exactly these members."
}
return null

View File

@@ -20,8 +20,19 @@ import kotlinx.coroutines.flow.flowOf
* not publish on their behalf until they say so.
*/
interface DkgRepository {
/** The room's current ritual — abandoned attempts are superseded by the newest. */
fun observeLatestSessionForChatRoom(chatRoomId: String): Flow<DkgSession?>
/**
* The room's current ritual for one purpose — the group's own when
* [parentChatRoomId] is null, or that parent's subgroup when it is not.
* Abandoned attempts are superseded by the newest.
*
* A room can hold two: a subgroup whose admins are the whole group derives
* the room the group's own ceremony ran in. See
* `DkgSessionDao.getLatestSessionFor`.
*/
fun observeLatestSessionForChatRoom(
chatRoomId: String,
parentChatRoomId: String? = null,
): Flow<DkgSession?>
fun observeMessages(sessionId: String): Flow<List<DkgParticipantMessage>>
@@ -92,7 +103,10 @@ interface DkgRepository {
* changes is the moment there is a room to make -- and the state it is
* about does not exist as a row until somebody makes one.
*/
fun observeSignedGroupKeyState(chatRoomId: String): Flow<GroupKeyState?>
fun observeSignedGroupKeyState(
chatRoomId: String,
parentChatRoomId: String? = null,
): Flow<GroupKeyState?>
/** Files the state the group signed for a room that now exists. */
suspend fun adoptGroupKeyState(chatRoomId: String): GroupKeyState?
@@ -140,7 +154,10 @@ interface DkgRepository {
companion object {
val NO_OP_DKG_REPOSITORY: DkgRepository = object : DkgRepository {
override fun observeLatestSessionForChatRoom(chatRoomId: String): Flow<DkgSession?> = flowOf(null)
override fun observeLatestSessionForChatRoom(
chatRoomId: String,
parentChatRoomId: String?,
): Flow<DkgSession?> = flowOf(null)
override fun observeMessages(sessionId: String): Flow<List<DkgParticipantMessage>> = flowOf(emptyList())
@@ -173,7 +190,10 @@ interface DkgRepository {
chatRoomId: String
): Flow<List<LocalFrostSigningSession>> = flowOf(emptyList())
override fun observeSignedGroupKeyState(chatRoomId: String): Flow<GroupKeyState?> =
override fun observeSignedGroupKeyState(
chatRoomId: String,
parentChatRoomId: String?,
): Flow<GroupKeyState?> =
flowOf(null)
override suspend fun adoptGroupKeyState(chatRoomId: String): GroupKeyState? = null

View File

@@ -134,7 +134,8 @@ class DkgRitualViewModel(
observeKeyState()
dkgRepository.observeLatestSessionForChatRoom(chatRoomId).collect { session ->
dkgRepository.observeLatestSessionForChatRoom(chatRoomId, parentChatRoomId)
.collect { session ->
val loaded = (dkgRitualUIState as? DkgRitualUIState.Loaded)
?: DkgRitualUIState.Loaded(
localChatRoom = localChatRoom,
@@ -212,7 +213,8 @@ class DkgRitualViewModel(
}
launch {
dkgRepository.observeSignedGroupKeyState(chatRoomId).collect { state ->
dkgRepository.observeSignedGroupKeyState(chatRoomId, parentChatRoomId)
.collect { state ->
val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return@collect
dkgRitualUIState = loaded.copy(isKeyStateSigned = state != null)

View File

@@ -330,56 +330,60 @@ class SubgroupManagerJvmTest {
}
@Test
fun `a subgroup that is the whole group is refused`(): Unit = runBlocking {
// The trap this exists for: the ceremony room is derived from its members,
// so selecting everybody derives the room this group's own ceremony was
// held in -- and `proposeRitual` would hand that ceremony back, making the
// "child" this very group.
fun `a subgroup may be the whole group`(): Unit = runBlocking {
// A subgroup is a logical division -- a group deciding that some of its
// work belongs to a differently-keyed room -- not a smaller membership.
// "Everybody" is a normal answer.
//
// This used to be refused on set size, which was wrong twice: it turned
// down a legitimate subgroup, and it did so on a proxy for a collision
// that a parent whose membership has changed would not even have.
val parent = parentWith(listOf(alice, bob))
assertNotNull(refusal(parent, setOf(alice, bob)))
assertNull(refusal(parent, setOf(alice, bob)))
}
@Test
fun `an admin set that already holds a ceremony is refused`(): Unit = runBlocking {
fun `the group's own ceremony does not block a subgroup in the same room`(): Unit =
runBlocking {
// The collision the size rule was standing in for, made concrete. A
// subgroup over everybody derives the room the group's own ceremony was
// held in -- so the room is shared, and only `parentChatRoomId` keeps
// the two ceremonies apart. Without that scoping `proposeRitual` would
// hand back the *group's* key and quietly make the child the parent.
val parent = parentWith(listOf(alice, bob))
val ceremonyRoomId = SubgroupManager.ceremonyRoomIdFor(setOf(alice, bob), user)
seedCeremonyRoom(ceremonyRoomId, listOf(alice, bob, user))
db.dkgSessionDao().upsert(ownCeremony(ceremonyRoomId))
assertNull(
refusal(parent, setOf(alice, bob)),
"the group's own ceremony is not this subgroup's",
)
}
@Test
fun `the same parent cannot have two subgroups over the same admins`(): Unit = runBlocking {
val parent = parentWith(listOf(alice, bob, carol))
assertNull(refusal(parent, setOf(alice, bob)))
// One admin set, one ceremony room, forever -- the id is a pure function
// of the members. A second subgroup with the same admins would come back
// with the first one's key and therefore the first one's room id.
//
// Room first, the way the picker stands one up: the ceremony room is a
// real NIP-17 room and `DkgSession.chatRoomId` is a foreign key onto it.
val ceremonyRoom = assertNotNull(
db.nostrNip17Dao().createNip17ChatRoom(
userPublicKey = user,
participantPublicKeys = listOf(alice, bob),
subject = "Translation team",
)
)
assertEquals(
SubgroupManager.ceremonyRoomIdFor(setOf(alice, bob), user),
ceremonyRoom.chatRoom.id,
"the derived ceremony room is the one createNip17ChatRoom lands on",
)
// The one collision that survives, and it is degenerate: same parent,
// same admins. The ceremony room is derived from the admins and the
// ceremonies in it are told apart by their parent, so this pair has
// nothing left to distinguish them -- which is another way of saying it
// is one subgroup asked for twice.
val ceremonyRoomId = SubgroupManager.ceremonyRoomIdFor(setOf(alice, bob), user)
seedCeremonyRoom(ceremonyRoomId, listOf(alice, bob, user))
db.dkgSessionDao().upsert(
DkgSession(
id = "s1",
chatRoomId = ceremonyRoom.chatRoom.id,
coordinatorPublicKey = user,
userPublicKey = user,
threshold = 2,
participantCount = 3,
hostPublicKey = user,
round1Random = "aa".repeat(32),
round2AuxRandom = "bb".repeat(32),
)
ownCeremony(ceremonyRoomId).copy(id = "s1", parentChatRoomId = parentRoomId)
)
assertNotNull(refusal(parent, setOf(alice, bob)))
// And a different admin set is a different room, so it is untouched.
assertNull(refusal(parent, setOf(alice, carol)))
}
@Test
@@ -445,6 +449,41 @@ class SubgroupManagerJvmTest {
// ---- fixtures ----------------------------------------------------------
/**
* The NIP-17 room a ceremony runs in, stood up the way the picker does it --
* room first, because `DkgSession.chatRoomId` is a foreign key onto it.
*/
private suspend fun seedCeremonyRoom(expectedId: String, members: List<String>) {
members.forEach { seedProfile(it) }
val room = assertNotNull(
db.nostrNip17Dao().createNip17ChatRoom(
userPublicKey = user,
participantPublicKeys = members.filterNot { it == user },
subject = "Translation team",
)
)
assertEquals(
expectedId,
room.chatRoom.id,
"the derived ceremony room is the one createNip17ChatRoom lands on",
)
}
/** A ceremony held in [chatRoomId] for no subgroup: the group's own. */
private fun ownCeremony(chatRoomId: String) = DkgSession(
id = "own-ceremony",
chatRoomId = chatRoomId,
coordinatorPublicKey = user,
userPublicKey = user,
threshold = 2,
participantCount = 3,
hostPublicKey = user,
round1Random = "aa".repeat(32),
round2AuxRandom = "bb".repeat(32),
)
/** A finished ceremony for the child, as far as these guards can tell. */
private fun completedChildCeremony() = DkgSession(
id = "child-ceremony",

View File

@@ -398,11 +398,16 @@ comment:
- Selecting an admin set that already holds a completed ceremony returns that
ceremony (`proposeRitual` hands back anything not `FAILED`), which would make
the "new" subgroup the old group under a new name — same key, same room id.
- Selecting *every* member of the parent is the pathological case of that: the
parent's own ceremony room is the room over all its members, so the child would
come out as the parent itself.
- Selecting *every* member of the parent lands in the parent's own ceremony room,
which is the room over all its members.
Refused in Phase 8, on both readings, before the button is offered.
The second is **not** refused, and the first is refused more narrowly than it
first shipped. A subgroup over everybody is legitimate -- a subgroup is a logical
division rather than a smaller membership -- and sharing a room was never the
problem; sharing a *ceremony* was. `DkgSession.parentChatRoomId` tells two
ceremonies in one room apart, and `DkgSessionDao.getLatestSessionFor` is scoped by
it, so what Phase 8 refuses is only the same parent asking twice over the same
admins. See [that phase](#phase-8--the-refusals).
### Why not the parent's Marmot room
@@ -784,16 +789,43 @@ a room nobody can replace.
| a threshold outside `ChatRoomType.quorumRange(adminCount)` | `t = 1` is a threshold key any one member signs with, and ChillDKG will happily generate one. `acceptProposal` already refuses such a proposal, so an unchecked one produces a ceremony every invitee silently drops |
| an empty or blank name | Phase 6 has nothing to put in `MarmotGroupData.name`, and the parent's admins would be certifying an unnamed hash |
| a selected member with no unconsumed key package | they cannot be welcomed into the room at step 4, and finding that out at step 4 wastes three ceremonies. Checked at the picker and again at confirm |
| the selection is not a proper subset of the parent's members | selecting everybody derives the parent's own ceremony room, and the "child" comes out as the parent |
| the ceremony room already holds a non-`FAILED` `DkgSession` | `proposeRitual` would return that ceremony, and the "new" subgroup would be the old one — same key, same id |
| ~~the selection is not a proper subset of the parent's members~~ | **withdrawn.** A subgroup is a logical division, not a smaller membership, so "everybody" is a normal answer. What this was standing in for is the row below, checked precisely rather than by set size |
| the ceremony room already holds a non-`FAILED` `DkgSession` **for this same parent** | `proposeRitual` would return that ceremony, and the "new" subgroup would be the old one — same key, same id. Scoped by `DkgSession.parentChatRoomId`, so a room may hold the group's own ceremony *and* a subgroup's |
| this device holds no share of the parent's key | it cannot open the certificate session; `proposeSigningBatch` throws, and throwing at the button is not a UI |
| this device is not an admin of the parent | a non-admin proposing the parent's signature is a proposal the parent's admins have to decline by hand |
| the parent has no signed key state and no resolvable key | there is nothing for the certificate to be signed with |
| a certificate already stands for this child | one certificate per child; a second is a `d`-tag replacement of the first, not a second subgroup |
The first and the third are the ones a user will actually meet, so both say what
to do: "pick at least two more people" and "a subgroup cannot be the whole group
— leave at least one member out".
The first is the one a user will actually meet, and it says what to do: "pick at
least two more people".
### Why "a subgroup cannot be the whole group" was withdrawn
It shipped, and it was wrong twice.
It refused something legitimate. A subgroup is a group deciding that some of its
work belongs to a differently-keyed room, not a group carving out a smaller
membership — so every member being in it is an ordinary case, and no rule here had
any business deciding otherwise.
And it was a proxy rather than a check. The thing it stood in for is real: a
ceremony room is derived from its admins, so a subgroup over everybody lands in
the room the group's *own* ceremony was held in, and `proposeRitual` handing back
that ceremony would make the child the parent. But set size does not detect that.
A parent whose membership has changed since its own ceremony derives a different
room, so the sizes can match with no collision and differ with one.
**Sharing the room was never the problem; sharing a ceremony was.**
`DkgSession.parentChatRoomId` already told them apart, so the fix was to scope the
lookup by it — `DkgSessionDao.getLatestSessionFor(room, parent)` — rather than to
forbid the selection. A room may now hold two ceremonies, and everything below
that lookup already keyed off the session id, so nothing else had to learn about
the second one.
One collision survives, and it is degenerate: the same parent, over the same
admins, twice. Those two have nothing left to distinguish them, which is another
way of saying they are one subgroup asked for twice, and that is what the message
now says.
## Phase 9 — the tests that prove it
@@ -871,12 +903,16 @@ about whether the child's chronicle may carry an event its own key did not sign,
which no chroniclable kind does today. That last one is the real work, and it is
why this is a follow-up rather than a line in Phase 5.
**One admin set, one subgroup.** Phase 4's collision is refused, not solved. Two
subgroups with exactly the same admins need the NIP-17 ceremony room to be
distinguishable by something other than its members, and
`ChatRoom.deriveChatRoomId` is a pure aggregate of member keys that the inbound
path recomputes. Changing it is a change to how every NIP-17 room in the app is
addressed, and it is not worth making for this.
**One parent, one admin set, one subgroup.** Narrower than it first shipped: a
room may hold two ceremonies, told apart by `DkgSession.parentChatRoomId`, so a
group's own ceremony and a subgroup over the same people coexist. What is still
refused is the same parent asking twice over the same admins, which have nothing
left to distinguish them.
Widening that further needs the NIP-17 ceremony room to be distinguishable by
something other than its members, and `ChatRoom.deriveChatRoomId` is a pure
aggregate of member keys that the inbound path recomputes. Changing it is a change
to how every NIP-17 room in the app is addressed.
It is also the limitation that disappears on its own: move the ceremony into the
parent's Marmot room, as [Phase 4](#why-not-the-parents-marmot-room) defers doing,