diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/ChapterIndexing.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/ChapterIndexing.kt new file mode 100644 index 00000000..425da261 --- /dev/null +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/ChapterIndexing.kt @@ -0,0 +1,114 @@ +package press.mantra.compose.managers + +import co.touchlab.kermit.Logger +import kotlinx.coroutines.flow.first +import press.mantra.compose.nostr.nip30303.ChapterEvent +import press.mantra.compose.repository.FrostSigningRepository +import press.mantra.compose.repository.MantraRepository + +/** + * Where the next chapter of a version goes. + * + * The index is signed into the chapter and cannot be recounted on arrival -- a + * device applying one cannot recount from a version other members may have + * added to in a different order -- so the number has to be right at proposal + * time. "How many chapters are in the table" is not it. + * + * **A proposed chapter is not a row.** It becomes one when a quorum signs, + * which takes as long as it takes, and until then nothing in + * `MantraChapter` knows about it. So reading the table gives the same answer to + * two proposals made a second apart, and both get signed at the same index. + * At one chapter at a time that is a race nobody hits; an upload of twelve + * files hits it eleven times out of eleven. + * + * The information needed is already published. A session's items carry the + * unsigned events it is signing, and an unsigned event's id and fields are + * settled at proposal time -- that is the whole reason + * [FrostSigningRepository.unsignedEvents] exists rather than only + * `signedEvents`. So the answer is the table *and* the room's open proposals. + */ +object ChapterIndexing { + + private const val TAG = "ChapterIndexing" + + /** + * The index for the next chapter of [artifactVersionId], counting chapters + * this room has proposed but not yet signed. + * + * **One past the highest, not a count.** A count is wrong the moment the + * sequence has a gap, and gaps are expected here: a chapter that fails to + * reach a quorum never becomes a row, and its index is spent -- a + * re-proposed chapter is a different event. Ordering survives a gap, so + * leaving one is the cheap answer and renumbering would be the expensive + * wrong one. + * + * **Every session counts, whatever stage it is at.** A failed session's + * index is wasted, which costs a gap; a stalled session's chapter may still + * arrive, and stepping over it costs nothing. Filtering by stage would + * trade a harmless gap for a possible collision. + * + * **This fixes one device, not the group.** Two members proposing at the + * same moment still collide: neither sees the other's proposal until it + * arrives, and no amount of local reading will change that. That is + * tolerable where this is not -- an index orders reading, two chapters at + * index 4 sort arbitrarily between themselves, and nothing else breaks. The + * local case is worth fixing because an upload makes it certain rather than + * unlikely. + * + * Failing soft on purpose: if the proposals cannot be read, the table's + * answer is still better than nothing, and refusing to propose a chapter + * because its number could not be checked would be the wrong trade. + */ + suspend fun nextIndex( + mantraRepository: MantraRepository, + frostSigningRepository: FrostSigningRepository, + chatRoomId: String, + artifactVersionId: String, + ): Int { + val signed = mantraRepository + .getChaptersForArtifactVersion(artifactVersionId) + .maxOfOrNull { it.index } + + val proposed = runCatching { + proposedIndices(frostSigningRepository, chatRoomId, artifactVersionId).maxOrNull() + }.onFailure { error -> + Logger.withTag(TAG).e( + "Could not read this room's proposals; numbering the next chapter from the " + + "table alone, which may collide with a chapter already proposed", + error, + ) + }.getOrNull() + + val highest = listOfNotNull(signed, proposed).maxOrNull() ?: return 0 + return highest + 1 + } + + /** + * The indices of chapters this room has proposed for [artifactVersionId]. + * + * Read off the unsigned events rather than from anything stored about them, + * because there is nothing stored about them: a proposal's items are its + * only record until it is signed. Non-chapter items -- a dialect, a + * translation, the chunks beside a chapter -- are ignored by kind, and a + * chapter belonging to another version of the same artifact by its + * `artifactVersionId` tag. + */ + private suspend fun proposedIndices( + frostSigningRepository: FrostSigningRepository, + chatRoomId: String, + artifactVersionId: String, + ): List = + frostSigningRepository.observeSessionsForChatRoom(chatRoomId).first() + .flatMap { session -> frostSigningRepository.unsignedEvents(session.items) } + .filter { event -> event.kind == ChapterEvent.KIND } + .mapNotNull { event -> + ChapterEvent( + id = event.id, + pubKey = event.pubKey, + createdAt = event.createdAt, + tags = event.tags, + content = event.content, + sig = event.sig, + ).takeIf { it.artifactVersionId() == artifactVersionId }?.index() + } +} diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/AddChapterViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/AddChapterViewModel.kt index a7dc3e4a..fc529e3f 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/AddChapterViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/AddChapterViewModel.kt @@ -18,6 +18,7 @@ import kotlinx.coroutines.IO import kotlinx.coroutines.launch import press.mantra.compose.database.model.MantraArtifactVersion import press.mantra.compose.database.model.intermdiate.LocalChatRoom +import press.mantra.compose.managers.ChapterIndexing import press.mantra.compose.managers.FrostSigningManager import press.mantra.compose.managers.TranslationScaffold import press.mantra.compose.nostr.nip30303.ChapterEvent @@ -131,7 +132,17 @@ class AddChapterViewModel( // may have added to in a different order. The scaffolding proposed // below is placed at the same one, so a translation chapter agrees // with the chapter it stands for. - val chapterIndex = mantraRepository.getChaptersForArtifactVersion(artifactVersion.id).size + // + // Counting the room's open proposals as well as its rows, because a + // chapter proposed a second ago is not a row yet and reading the + // table alone would hand it this chapter's number -- see + // [ChapterIndexing.nextIndex]. + val chapterIndex = ChapterIndexing.nextIndex( + mantraRepository = mantraRepository, + frostSigningRepository = frostSigningRepository, + chatRoomId = chatRoomId, + artifactVersionId = artifactVersion.id, + ) val chapterEventTemplate = ChapterEvent.build( artifactVersionId = artifactVersion.id, diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/ChapterIndexingJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/ChapterIndexingJvmTest.kt new file mode 100644 index 00000000..673db3e7 --- /dev/null +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/ChapterIndexingJvmTest.kt @@ -0,0 +1,305 @@ +package press.mantra.compose.managers + +import androidx.room3.Room +import fr.acinq.bitcoin.ByteVector32 +import fr.acinq.bitcoin.PrivateKey +import fr.acinq.bitcoin.crypto.frost.Frost +import fr.acinq.bitcoin.crypto.frost.KeyMaterial +import kotlin.test.AfterTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.runBlocking +import press.mantra.compose.database.MantraDatabase +import press.mantra.compose.database.builder.getRoomDatabase +import press.mantra.compose.database.model.ChatRoom +import press.mantra.compose.database.model.DkgParticipantMessage +import press.mantra.compose.database.model.DkgSession +import press.mantra.compose.database.model.MantraArtifact +import press.mantra.compose.database.model.MantraArtifactVersion +import press.mantra.compose.database.model.MantraChapter +import press.mantra.compose.database.model.MantraDialect +import press.mantra.compose.database.model.NostrEvent +import press.mantra.compose.database.model.Profile +import press.mantra.compose.database.model.intermdiate.LocalChatRoom +import press.mantra.compose.database.model.types.DkgRitualStage +import press.mantra.compose.database.repository.DatabaseFrostSigningRepository +import press.mantra.compose.database.repository.DatabaseMantraRepository +import press.mantra.compose.extensions.toHex +import press.mantra.compose.nostr.dkg.DkgRitualEvents +import press.mantra.compose.nostr.nip30303.ChapterEvent +import press.mantra.compose.nostr.nip30303.ChunkEvent +import press.mantra.compose.repository.FrostSigningRepository +import press.mantra.compose.repository.MantraRepository +import press.mantra.compose.text.Markdown + +/** + * A chapter's index has to be right when it is proposed, because it is signed + * into the event and cannot be recounted on arrival. + * + * The test that matters here is the third one, and it fails against the old + * `getChaptersForArtifactVersion(...).size`: three chapters proposed back to + * back all read the same empty table and all get index 0. At one chapter at a + * time that is a race nobody hits. An upload of twelve files hits it every + * time, which is why this is a fix rather than a feature. + */ +class ChapterIndexingJvmTest { + private val participants = 3 + private val threshold = 2 + + private val keyMaterial: KeyMaterial = Frost.trustedDealerKeygen( + thresholdSecretKey = PrivateKey( + ByteVector32("1c0ffee0000000000000000000000000000000000000000000000000000000a1") + ), + nParticipants = participants, + threshold = threshold, + ) + + private val thresholdPublicKey = keyMaterial.thresholdPublicKey.value.toHex() + private val roomId = SharedKeyDerivation.marmotGroupId(thresholdPublicKey) + + private val ceremonyId = "ceremony".padEnd(64, '0') + private val members = listOf("a", "b", "c").map { it.repeat(64) } + private val hostKeys = listOf("2a", "2b", "2c").map { it.padEnd(66, '0') } + + private val proposer = members.first() + private val dialectId = "d".repeat(64) + private val artifactId = "a1".padEnd(64, '0') + private val artifactVersionId = "b".repeat(64) + private val otherVersionId = "c".repeat(64) + + private lateinit var db: MantraDatabase + private lateinit var room: LocalChatRoom + private lateinit var mantraRepository: MantraRepository + private lateinit var frostSigningRepository: FrostSigningRepository + + @AfterTest + fun closeDatabase() { + if (::db.isInitialized) db.close() + } + + private suspend fun openDevice() { + db = getRoomDatabase(Room.inMemoryDatabaseBuilder()) + val scope = CoroutineScope(Dispatchers.Default + SupervisorJob()) + mantraRepository = DatabaseMantraRepository(db, scope) + frostSigningRepository = DatabaseFrostSigningRepository(db, scope) + + val nostrEventId = "e$proposer".take(64) + db.nostrEventDao().upsert( + NostrEvent( + id = nostrEventId, + pubKey = proposer, + kind = 0, + tags = emptyArray(), + content = "{}", + sig = "0".repeat(128), + ) + ) + db.profileDao().upsert(Profile(publicKey = proposer, nostrEventId = nostrEventId)) + + val chatRoom = ChatRoom( + id = roomId, + userPublicKey = proposer, + subject = "#Ekklesia", + description = SharedKeyDerivation.describe("Admins of the group."), + mlsGroupState = null, + ) + db.chatRoomDao().upsert(chatRoom) + + db.dkgSessionDao().upsert( + DkgSession( + id = ceremonyId, + chatRoomId = roomId, + coordinatorPublicKey = proposer, + userPublicKey = proposer, + threshold = threshold, + participantCount = participants, + stage = DkgRitualStage.COMPLETE, + hostPublicKey = hostKeys[0], + round1Random = "1".repeat(64), + round2AuxRandom = "2".repeat(64), + thresholdPublicKey = thresholdPublicKey, + secretShare = keyMaterial.secretShares[0].value.toHex(), + publicShares = keyMaterial.publicShares.joinToString(",") { it.value.toHex() }, + ) + ) + members.forEachIndexed { index, member -> + db.dkgSessionDao().upsert( + DkgParticipantMessage( + sessionId = ceremonyId, + participantPublicKey = member, + kind = DkgRitualEvents.HOST_KEY, + payload = hostKeys[index], + ) + ) + } + + // The artifact a chapter hangs off, and a second version of it, so that + // "chapters of this version" can be told from "chapters in this room". + db.mantraDialectDao().upsert( + MantraDialect( + id = dialectId, + publicKey = roomId, + name = "Setswana", + country = "BW", + language = "tn", + signature = "", + chatRoomId = roomId, + ) + ) + db.mantraArtifactDao().upsert( + MantraArtifact( + id = artifactId, + publicKey = roomId, + name = "To Kill a Mockingbird", + url = "https://example.test/mockingbird", + visibility = "private", + dialectId = dialectId, + license = "cc", + chatRoomId = roomId, + signature = "", + ) + ) + listOf(artifactVersionId to "1.0", otherVersionId to "2.0").forEach { (id, label) -> + db.mantraArtifactVersionDao().upsert( + MantraArtifactVersion( + id = id, + artifactId = artifactId, + publicKey = roomId, + versionLabel = label, + chatRoomId = roomId, + signature = "", + ) + ) + } + + room = LocalChatRoom(chatRoom = chatRoom) + } + + private suspend fun insertChapter(index: Int, versionId: String = artifactVersionId) { + db.mantraChapterDao().upsert( + MantraChapter( + id = "${versionId.take(8)}chapter$index".padEnd(64, '0'), + artifactVersionId = versionId, + publicKey = roomId, + name = "Chapter $index", + originalText = "Signed already.", + index = index, + wordCount = 2, + characterCount = 15, + signature = "", + chatRoomId = roomId, + ) + ) + } + + /** Exactly what `AddChapterViewModel.addChapter` proposes, at a given index. */ + private suspend fun proposeChapterAt(index: Int, versionId: String = artifactVersionId) { + val originalText = "Paragraph one.\n\nParagraph two." + FrostSigningManager.proposeSigningBatch( + database = db, + localChatRoom = room, + userPublicKey = proposer, + lead = ChapterEvent.build( + artifactVersionId = versionId, + name = "Chapter $index", + originalText = originalText, + index = index, + wordCount = Markdown.wordCount(originalText), + characterCount = Markdown.characterCount(originalText), + ), + dependents = ChunkEvent::splitOf, + ) + } + + private suspend fun nextIndex(versionId: String = artifactVersionId): Int = + ChapterIndexing.nextIndex( + mantraRepository = mantraRepository, + frostSigningRepository = frostSigningRepository, + chatRoomId = roomId, + artifactVersionId = versionId, + ) + + @Test + fun `an empty version starts at zero`() = runBlocking { + openDevice() + + assertEquals(0, nextIndex()) + } + + @Test + fun `signed chapters are counted`() = runBlocking { + openDevice() + insertChapter(0) + insertChapter(1) + + assertEquals(2, nextIndex()) + } + + @Test + fun `three chapters proposed back to back take three different indices`() = runBlocking { + openDevice() + + // The whole point. Nothing is signed, so the table is empty throughout + // and the old `.size` answered 0 three times. + val allocated = (1..3).map { + val index = nextIndex() + proposeChapterAt(index) + index + } + + assertEquals(listOf(0, 1, 2), allocated) + } + + @Test + fun `a proposal counts alongside the rows`() = runBlocking { + openDevice() + insertChapter(0) + proposeChapterAt(1) + + assertEquals(2, nextIndex()) + } + + @Test + fun `one past the highest, not a count, so a gap is stepped over`() = runBlocking { + openDevice() + insertChapter(0) + insertChapter(5) + + // A chapter that failed to reach a quorum spent its index: a re-proposed + // chapter is a different event. Counting rows would hand out 2 and + // collide with nothing, but it would also renumber a version whose + // chapters are 0 and 5 into one whose next chapter is 2. + assertEquals(6, nextIndex()) + } + + @Test + fun `another version's chapters are not counted`() = runBlocking { + openDevice() + insertChapter(0, otherVersionId) + insertChapter(1, otherVersionId) + proposeChapterAt(7, otherVersionId) + + // Rows are already scoped by the query; proposals are not -- every + // session in the room comes back together -- so the version tag is what + // separates them, and getting it wrong would push every version's + // numbering up by the whole room's. + assertEquals(0, nextIndex()) + assertEquals(8, nextIndex(otherVersionId)) + } + + @Test + fun `the chunks beside a chapter are not mistaken for chapters`() = runBlocking { + openDevice() + + // A chapter proposal is a batch: one chapter and a chunk per paragraph. + // A chunk carries an index too -- its position in the chapter -- and + // reading those as chapter indices would have the second chapter of a + // two-paragraph first chapter land at 2 instead of 1. + proposeChapterAt(0) + + assertEquals(1, nextIndex()) + } +} diff --git a/docs/chapter-uploads.md b/docs/chapter-uploads.md index d5aefa5a..ec61cabe 100644 --- a/docs/chapter-uploads.md +++ b/docs/chapter-uploads.md @@ -380,25 +380,36 @@ and the last of these exists precisely for this: "A caller wanting an id … nee it while the first session is still running, and the id is settled at proposal time." -```kotlin -// MantraRepository, or a small ChapterIndexing object beside TranslationScaffold -/** - * The index the next chapter of [artifactVersionId] takes, counting chapters - * the room has proposed but not yet signed. - * - * Rows alone are wrong by however many proposals are in flight, which at N = 1 - * is almost never and during an upload is always. A proposed chapter's index is - * already fixed -- it is signed into the unsigned event the session will sign -- - * so it counts. - */ -suspend fun nextChapterIndex(chatRoomId: String, artifactVersionId: String): Int -``` +**Built as `ChapterIndexing.nextIndex`**, an object beside `TranslationScaffold` +rather than a method on `MantraRepository`, because it needs both repositories — +the table for what is signed and the signing repository for what is proposed — +and a repository method that reaches into another repository is the wrong shape. +It takes both, which also makes it testable without a view model. Implemented as: rows for the version, plus the unsigned `ChapterEvent`s of every -open session in the room whose `artifactVersionId()` matches, de-duplicated by -event id, and `maxOf(index) + 1` rather than a count — a count is wrong the -moment a gap exists, and after a chapter fails to reach quorum a gap is exactly -what there is. +session in the room whose `artifactVersionId()` matches, and `maxOf(index) + 1` +rather than a count — a count is wrong the moment a gap exists, and after a +chapter fails to reach quorum a gap is exactly what there is. + +Three details the build settled that the plan left open: + +- **Every session counts, whatever stage it is at.** Filtering to open ones + would trade a harmless gap (a failed session's index is spent, since a + re-proposed chapter is a different event) for a possible collision with a + stalled session that still completes. +- **Items are filtered by kind, which is not optional.** A chapter proposal is a + batch, and a chunk carries an `index` tag too — its position in the chapter. + Reading those as chapter indices would put the second chapter of a + two-paragraph first chapter at 2. There is a test for exactly that. +- **Proposals are not scoped by version and rows are.** The row query takes an + `artifactVersionId`; `observeSessionsForChatRoom` returns the whole room, so + the `artifactVersionId` tag is what separates one version's proposals from + another's. Getting it wrong would push every version's numbering up by the + whole room's. + +Failing soft on purpose: if the proposals cannot be read, the table's answer is +still better than nothing, and refusing to propose a chapter because its number +could not be checked would be the wrong trade. Then `AddChapterViewModel` calls it instead of `.size`, and the upload allocates `base + i` for its i-th file **once**, before proposing anything, rather than @@ -412,9 +423,11 @@ are tolerable: the index orders reading, two chapters at index 4 sort arbitrarily between themselves and nothing else breaks. Worth a sentence in the code so the next reader does not take the local fix for a global one. -**Test** (`jvmTest`, beside `ChapterBatchProposalJvmTest`): propose three -chapters back to back without completing any session, and assert the three -signed indices are 0, 1, 2 — which is the test that fails today. +**Tests** (`ChapterIndexingJvmTest`, beside `ChapterBatchProposalJvmTest`): +propose three chapters back to back without completing any session, and assert +the three indices are 0, 1, 2 — which is the test that fails today, since +nothing is signed so the table answers 0 three times. Plus the gap, the two +versions, and the chunk-vs-chapter confusion above. ---