From 819322bbf91fde00a066827bb2a15f54b9d7e0df Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 14:24:28 +0000 Subject: [PATCH] fix(quartz/fs): port the same NIP correctness fixes from the sqlite store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the bugs that the SQLite review surfaced also live in the filesystem-backed event store. Bringing both stores back to parity: - NIP-01 lexical-id tiebreaker (FsSlots, FsEventStore): when two replaceables / addressables share `createdAt`, the lexically smaller id wins. The previous `existing.createdAt >= incoming.createdAt` check rejected equal-timestamp inserts unconditionally — which blocks the legitimate winner whenever it arrived second. - delete(Filter()) safe-by-default (FsEventStore): an empty filter used to enumerate every event and delete each one, so a stray `delete(Filter())` would wipe the entire on-disk store. Now both the single-filter and list-of-filters overloads short-circuit when every filter is empty, matching the SQLiteEventStore contract. - NIP-09 author check on tombstone install (FsEventStore, FsTombstones): SQLite's `reject_deleted_events` trigger checks `event_tags.pubkey_hash = NEW.pubkey_owner_hash`, so a stranger's kind-5 with an `e`/`a` tag pointing at someone else's event must not block them from re-publishing. The FS store used to install the tombstone unconditionally and then read it back without an author check. - id tombstones still install (so they can fire when the deletion arrives before its target), but `idTombstoneOwnerPubKey` is now compared against the candidate event's owner pubkey at insert time. GiftWrap parity preserved via FsIndexer.ownerPubKey, which returns the recipient like the SQLite `pubkey_owner_hash`. - addr tombstones are now only installed when `addr.pubKeyHex == deletion.pubKey`, since the address itself carries the owner identity. Tests: - FsSlotsTest: replaces "equal timestamp replaceable is rejected" (which pinned the buggy behaviour) with two tests covering the lexical-id tiebreaker in both insertion orders. - FsParityTest: new same-`createdAt` tiebreaker tests for both replaceable and addressable kinds, asserting FS and SQLite agree. - FsDeletionTest: - inverts "deletion by non-author does not cascade but still installs id tombstone" — the legitimate owner must be able to re-insert after the stranger's deletion. - new test that a stranger's `a`-tag deletion does not block the legitimate addressable owner from publishing a new version. - FsEventStoreTest: new `delete with empty filter is safe` test matching the SQLite-side contract added in batch A. https://claude.ai/code/session_01X163Nr31vGkvAXoJ3JgMov --- .../quartz/nip01Core/store/fs/FsEventStore.kt | 74 ++++++++++++------- .../quartz/nip01Core/store/fs/FsIndexer.kt | 12 ++- .../quartz/nip01Core/store/fs/FsSlots.kt | 20 ++--- .../quartz/nip01Core/store/fs/FsTombstones.kt | 34 ++++++++- .../nip01Core/store/fs/FsDeletionTest.kt | 45 +++++++++-- .../nip01Core/store/fs/FsEventStoreTest.kt | 17 +++++ .../quartz/nip01Core/store/fs/FsParityTest.kt | 52 +++++++++++++ .../quartz/nip01Core/store/fs/FsSlotsTest.kt | 28 ++++++- 8 files changed, 232 insertions(+), 50 deletions(-) diff --git a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStore.kt b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStore.kt index c5b2aefd1..7bd8ac2ba 100644 --- a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStore.kt +++ b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStore.kt @@ -111,10 +111,16 @@ open class FsEventStore( val slot = slots.slotPathFor(event) val existingSlot = slot?.let { slots.readSlot(it) } - if (existingSlot != null && existingSlot.createdAt >= event.createdAt) { - // Newer or equal-timestamp version already owns this slot. - // Matches ReplaceableModule / AddressableModule blocking - // behaviour in SQLite. + if (existingSlot != null && + ( + existingSlot.createdAt > event.createdAt || + (existingSlot.createdAt == event.createdAt && existingSlot.id <= event.id) + ) + ) { + // Existing slot winner outranks the incoming event under NIP-01 + // (later createdAt, or same createdAt with the lexically smaller + // id). Matches the ReplaceableModule / AddressableModule trigger + // condition in SQLite. return } @@ -178,12 +184,17 @@ open class FsEventStore( /** * NIP-09 pre-insert guard. Parity with SQLite's `reject_deleted_events` - * BEFORE INSERT trigger: id-scoped tombstones always block; address- - * scoped tombstones block when the existing cutoff is `>=` the new - * event's createdAt. + * BEFORE INSERT trigger: id and addr tombstones only block when the + * deletion's author matches the candidate event's owner pubkey — + * SQLite checks `event_tags.pubkey_hash = NEW.pubkey_owner_hash`. For + * GiftWraps the owner is the recipient (p-tag), matching `pubkey_ + * owner_hash`. Addr tombstones additionally enforce the + * `event.createdAt <= cutoff` window. */ private fun isBlockedByTombstone(event: Event): Boolean { - if (tombstones.hasIdTombstone(event.id)) return true + val ownerPubKey = indexer.ownerPubKey(event) + val tombstoneAuthor = tombstones.idTombstoneOwnerPubKey(event.id) + if (tombstoneAuthor != null && tombstoneAuthor == ownerPubKey) return true if (event is AddressableEvent && event.kind.isAddressable()) { val cutoff = tombstones.addrTombstoneCutoff(event.kind, event.pubKey, event.dTag()) if (cutoff != null && event.createdAt <= cutoff) return true @@ -227,22 +238,25 @@ open class FsEventStore( } for (addr in deletion.deleteAddresses()) { - // Cascade is only honoured when the address's author matches - // the deletion author — matching SQLite's WHERE pubkey = ?. - if (addr.pubKeyHex == deletion.pubKey) { - val slot = - when { - addr.kind.isReplaceable() -> layout.replaceableSlot(addr.kind, addr.pubKeyHex) - addr.kind.isAddressable() -> layout.addressableSlot(addr.kind, addr.pubKeyHex, addr.dTag) - else -> null - } - if (slot != null) { - val winner = slots.readSlot(slot) - if (winner != null && winner.createdAt <= deletion.createdAt) { - indexer.unlink(winner) - layout.canonical(winner.id).deleteIfExists() - slots.clear(slot) - } + // Per NIP-09 only the original author can delete their own + // addressable. If the deletion's author doesn't own the + // address, ignore both the cascade AND the tombstone install + // — otherwise a stranger's stray `a`-tag would block the + // legitimate owner from re-publishing. + if (addr.pubKeyHex != deletion.pubKey) continue + + val slot = + when { + addr.kind.isReplaceable() -> layout.replaceableSlot(addr.kind, addr.pubKeyHex) + addr.kind.isAddressable() -> layout.addressableSlot(addr.kind, addr.pubKeyHex, addr.dTag) + else -> null + } + if (slot != null) { + val winner = slots.readSlot(slot) + if (winner != null && winner.createdAt <= deletion.createdAt) { + indexer.unlink(winner) + layout.canonical(winner.id).deleteIfExists() + slots.clear(slot) } } tombstones.installAddr(addr.kind, addr.pubKeyHex, addr.dTag, deletion, deletionCanonical) @@ -323,17 +337,27 @@ open class FsEventStore( // Delete // ------------------------------------------------------------------ + /** + * Safe-by-default: an empty filter (or a list of only empty filters) + * deletes nothing, so a stray `delete(Filter())` cannot wipe the + * entire store. This is asymmetric with `query(Filter())` which + * intentionally returns every event — same contract as `SQLiteEventStore`. + */ override fun delete(filter: Filter) = lockManager.withWriteLock { + if (filter.isEmpty()) return@withWriteLock val ids = ArrayList() query(filter) { ids.add(it.id) } ids.forEach { deleteLocked(it) } } + /** See [delete] for the empty-filter contract. */ override fun delete(filters: List) = lockManager.withWriteLock { + val nonEmpty = filters.filterNot { it.isEmpty() } + if (nonEmpty.isEmpty()) return@withWriteLock val ids = HashSet() - query(filters) { ids.add(it.id) } + query(nonEmpty) { ids.add(it.id) } ids.forEach { deleteLocked(it) } } diff --git a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsIndexer.kt b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsIndexer.kt index 0fbb233c3..6a99bec6c 100644 --- a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsIndexer.kt +++ b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsIndexer.kt @@ -93,11 +93,17 @@ internal class FsIndexer( } } - fun ownerHash(event: Event): Long = + fun ownerHash(event: Event): Long = hasher.hash(ownerPubKey(event)) + + /** + * Pubkey-form counterpart of [ownerHash]. Same SQLite parity rule — + * recipient (p-tag) for GiftWraps, sender pubkey otherwise. + */ + fun ownerPubKey(event: Event): String = if (event is GiftWrapEvent) { - event.recipientPubKey()?.let { hasher.hash(it) } ?: hasher.hash(event.pubKey) + event.recipientPubKey() ?: event.pubKey } else { - hasher.hash(event.pubKey) + event.pubKey } private fun createLink( diff --git a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlots.kt b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlots.kt index 2d9d2b6df..e6755cddb 100644 --- a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlots.kt +++ b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlots.kt @@ -48,10 +48,12 @@ import kotlin.io.path.readText * Tnew = incoming event.createdAt * Told = current slot winner's createdAt (if any) * - * Tnew > Told → atomically install new slot, evict old - * Tnew <= Told → reject the insert entirely - * no existing slot → install new slot - * not replaceable/addr. → no-op (kinds that don't have slot semantics) + * Tnew > Told → atomically install new slot, evict old + * Tnew == Told && id_new < id_old → install new (NIP-01 lexical id tiebreaker) + * Tnew == Told && id_new >= id_old → reject the insert entirely + * Tnew < Told → reject the insert entirely + * no existing slot → install new slot + * not replaceable/addr. → no-op (kinds that don't have slot semantics) * * Eviction deletes the old canonical file and all its index hardlinks. */ @@ -76,17 +78,17 @@ internal class FsSlots( } /** - * Pre-insert check. Returns the *existing* winner if it blocks the - * insert (i.e. its createdAt is >= incoming.createdAt). Returns null - * when insertion may proceed — possibly with an evictable older winner, - * which the caller looks up separately via [readSlot]. + * Pre-insert check. Returns true when the existing slot winner + * outranks the incoming event. NIP-01 ranking is `(createdAt DESC, id ASC)` — + * later timestamp wins, and on a tie the lexicographically smaller id wins. */ fun shouldBlock( event: Event, slot: Path, ): Boolean { val existing = readSlot(slot) ?: return false - return existing.createdAt >= event.createdAt + return existing.createdAt > event.createdAt || + (existing.createdAt == event.createdAt && existing.id <= event.id) } fun readSlot(slot: Path): Event? { diff --git a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsTombstones.kt b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsTombstones.kt index 15095a479..e99f12b3c 100644 --- a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsTombstones.kt +++ b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsTombstones.kt @@ -42,12 +42,20 @@ import kotlin.io.path.readText * empty d for replaceables) * * Semantics: - * - An `id` tombstone unconditionally blocks re-insertion of the exact - * event id (matching SQLite's ExistsCheck on etag_hash). + * - An `id` tombstone blocks re-insertion of the exact event id only + * when the deletion's author matches the candidate event's owner — + * matching SQLite's `event_tags.pubkey_hash = NEW.pubkey_owner_hash` + * condition in `reject_deleted_events`. The id tombstone is therefore + * stored unconditionally (so the check survives "deletion arrives + * before its target") and authority is checked at lookup time via + * [idTombstoneOwnerPubKey]. * - An `addr` tombstone holds the kind-5 `createdAt` as a cutoff. An * insert is blocked iff `event.createdAt <= tombstone.createdAt`. * Newer events at the same address may pass (matching SQLite's - * `created_at >= NEW.created_at` trigger condition). + * `created_at >= NEW.created_at` trigger condition). Addr tombstones + * are only installed when `addr.pubkey == deletion.author`, since + * the address itself carries the owner identity (NIP-09: only the + * original author may delete their addressable). * - When multiple kind-5s target the same tombstone path, the one * with the larger `createdAt` wins (strongest cutoff). Atomic * rename swaps it in. @@ -58,7 +66,25 @@ import kotlin.io.path.readText internal class FsTombstones( private val layout: FsLayout, ) { - fun hasIdTombstone(id: HexKey): Boolean = layout.idTombstonePath(id).exists() + /** + * Returns the kind-5 deletion's author pubkey for an id tombstone, or + * null if there is no tombstone (or it can't be parsed). The caller + * must compare this against the candidate event's owner pubkey to + * decide whether to honour the tombstone — a stranger's kind-5 with + * an `e`-tag pointing at someone else's event MUST NOT block that + * event from being (re-)inserted. + */ + fun idTombstoneOwnerPubKey(id: HexKey): HexKey? { + val path = layout.idTombstonePath(id) + if (!path.exists()) return null + return try { + Event.fromJson(path.readText()).pubKey + } catch (_: java.io.IOException) { + null + } catch (_: com.fasterxml.jackson.core.JacksonException) { + null + } + } /** Returns the `createdAt` cutoff from the tombstone, or null if none exists. */ fun addrTombstoneCutoff( diff --git a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsDeletionTest.kt b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsDeletionTest.kt index 2d475ecd6..88f40c252 100644 --- a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsDeletionTest.kt +++ b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsDeletionTest.kt @@ -67,6 +67,7 @@ class FsDeletionTest { slug: String, body: String, ts: Long, + signer: NostrSignerSync = this.signer, ) = signer.sign( createdAt = ts, kind = LongTextNoteEvent.KIND, @@ -74,6 +75,12 @@ class FsDeletionTest { content = body, ) + private fun otherArticle( + slug: String, + body: String, + ts: Long, + ) = article(slug, body, ts, signer = otherSigner) + // ------------------------------------------------------------------ // Delete by id // ------------------------------------------------------------------ @@ -106,7 +113,7 @@ class FsDeletionTest { } @Test - fun `deletion by non-author does not cascade but still installs id tombstone`() { + fun `deletion by non-author neither cascades nor blocks legitimate re-insertion`() { // other signer authors a note val theirs = note("not yours", 10, signer = otherSigner) store.insert(theirs) @@ -118,12 +125,15 @@ class FsDeletionTest { // Cascade did NOT run — not our author. assertEquals(listOf(theirs.id), store.query(Filter(ids = listOf(theirs.id))).map { it.id }) - // But a tombstone is installed for the id, blocking future re-inserts. - // Re-inserting the note is effectively a no-op because it already - // exists. Instead we delete then try to re-insert. + // The id tombstone *is* installed (so it can fire if and when a + // future event with that id is owned by the deletion's author), + // but when the legitimate owner deletes the local copy and the + // event re-arrives from another relay, the tombstone must NOT + // block it — only same-author deletions can block re-insertion. + // Matches SQLite's `event_tags.pubkey_hash = NEW.pubkey_owner_hash`. store.delete(theirs.id) store.insert(theirs) - assertEquals(emptyList(), store.query(Filter(ids = listOf(theirs.id))).map { it.id }) + assertEquals(listOf(theirs.id), store.query(Filter(ids = listOf(theirs.id))).map { it.id }) } // ------------------------------------------------------------------ @@ -258,6 +268,31 @@ class FsDeletionTest { // Tombstone files use hardlinks to the kind-5 canonical // ------------------------------------------------------------------ + @Test + fun `non-author address deletion does not block legitimate addressable inserts`() { + // `otherSigner` (call them Bob) authors an addressable; the + // default `signer` (a stranger relative to Bob) then publishes a + // kind-5 with an `a` tag pointing at Bob's address. NIP-09 says + // only the address owner may delete it, so the stranger's event + // must NOT install an address tombstone — otherwise Bob couldn't + // publish a new version at the same address. Matches SQLite's + // `event_tags.pubkey_hash = NEW.pubkey_owner_hash` guard. + val v1 = otherArticle("shared", "v1", 10) + store.insert(v1) + assertEquals(listOf(v1.id), store.query(Filter(ids = listOf(v1.id))).map { it.id }) + + val strangerDel = + signer.sign(DeletionEvent.build(listOf(v1), createdAt = 20)) + store.insert(strangerDel) + + // Bob can still publish a newer version at the same address. Since + // the stranger's deletion was non-authoritative, no addr tombstone + // exists to block. + val v2 = otherArticle("shared", "v2", 30) + store.insert(v2) + assertEquals(listOf(v2.id), store.query(Filter(ids = listOf(v2.id))).map { it.id }) + } + @Test fun `id tombstone is a hardlink to the kind-5 event`() { val n = note("x", 10) diff --git a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStoreTest.kt b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStoreTest.kt index 1df0509ba..946123faf 100644 --- a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStoreTest.kt +++ b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsEventStoreTest.kt @@ -169,6 +169,23 @@ class FsEventStoreTest { assertTrue(store.count(Filter(ids = listOf(b.id))) == 1) } + @Test + fun `delete with empty filter is safe`() { + val a = signer.sign(TextNoteEvent.build("a", createdAt = 1)) + val b = signer.sign(TextNoteEvent.build("b", createdAt = 2)) + store.insert(a) + store.insert(b) + + // Empty filter: query returns everything, but delete must NOT + // wipe the store. Same safe-by-default contract as SQLiteEventStore. + assertEquals(2, store.count(Filter())) + store.delete(Filter()) + assertEquals(2, store.count(Filter())) + + store.delete(listOf(Filter(), Filter())) + assertEquals(2, store.count(Filter())) + } + @Test fun `staging dir is cleared on init`() { val staging = root.resolve(".staging") diff --git a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsParityTest.kt b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsParityTest.kt index 4fbf749b7..7b18122f2 100644 --- a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsParityTest.kt +++ b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsParityTest.kt @@ -231,6 +231,58 @@ class FsParityTest { assertParity(Filter(authors = listOf(signer.pubKey), kinds = listOf(LongTextNoteEvent.KIND))) } + @Test + fun `replaceable same-createdAt lexical id tiebreaker parity`() { + // Two kind-0 events with identical createdAt produce different ids + // because their content differs. NIP-01 says the lexically smaller + // id wins on a tie. Both stores must agree, regardless of insertion + // order. + val a = + signer.sign( + createdAt = 100, + kind = 0, + tags = emptyArray(), + content = "{\"name\":\"a\"}", + ) + val b = + signer.sign( + createdAt = 100, + kind = 0, + tags = emptyArray(), + content = "{\"name\":\"b\"}", + ) + + insertBoth(a) + insertBoth(b) + + assertParity( + Filter(authors = listOf(signer.pubKey), kinds = listOf(0)), + "loser-then-winner: lexically smaller id should win", + ) + assertParity( + Filter(ids = listOf(a.id, b.id)), + "the loser must not survive in the by-id query", + ) + } + + @Test + fun `addressable same-createdAt lexical id tiebreaker parity`() { + val a = article("tie", "version a", 100) + val b = article("tie", "version b", 100) + + insertBoth(a) + insertBoth(b) + + assertParity( + Filter( + authors = listOf(signer.pubKey), + kinds = listOf(LongTextNoteEvent.KIND), + tags = mapOf("d" to listOf("tie")), + ), + ) + assertParity(Filter(ids = listOf(a.id, b.id))) + } + // ------------------------------------------------------------------ // Deletion (NIP-09) // ------------------------------------------------------------------ diff --git a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlotsTest.kt b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlotsTest.kt index 9ded5a0fb..fd8eedeb0 100644 --- a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlotsTest.kt +++ b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/fs/FsSlotsTest.kt @@ -102,15 +102,35 @@ class FsSlotsTest { } @Test - fun `equal timestamp replaceable is rejected`() { + fun `equal timestamp replaceable resolves by lexical id (NIP-01)`() { val a = metadata("a", 100) val b = metadata("b", 100) - store.insert(a) - store.insert(b) + // NIP-01 tiebreaker: when createdAt ties, the lexically smaller + // id wins, regardless of insertion order. + val (winner, loser) = if (a.id < b.id) a to b else b to a + + // Loser inserted first, then winner — winner must replace. + store.insert(loser) + store.insert(winner) val got = store.query(Filter(authors = listOf(signer.pubKey), kinds = listOf(MetadataEvent.KIND))) assertEquals(1, got.size) - assertEquals(a.id, got.single().id) + assertEquals(winner.id, got.single().id) + } + + @Test + fun `equal timestamp replaceable rejects higher id when winner already present`() { + val a = metadata("a", 100) + val b = metadata("b", 100) + val (winner, loser) = if (a.id < b.id) a to b else b to a + + // Winner inserted first — loser must NOT take the slot. + store.insert(winner) + store.insert(loser) + + val got = store.query(Filter(authors = listOf(signer.pubKey), kinds = listOf(MetadataEvent.KIND))) + assertEquals(1, got.size) + assertEquals(winner.id, got.single().id) } @Test