fix(quartz): review fixes for group list move, NIP-46 rate limit, NIP-44 legacy padding, paging perf

- SimpleGroupListEvent.replace matches entries by group id + normalized relay on both sides (an unnormalized stored url is now removed) and keeps a private-only entry private instead of re-adding it as a public tag. - NostrConnectSignerService records the id of a rate-limited request it answered with an error, so a relay replay after restart is not serviced. - Nip44v2.unpad also accepts the padded length older builds produced with float math above 2^24; encryption stays spec-exact. - fetchAllPages appends page ids to a plain list and builds the dedup set only when a NIP-67 "auth" hint makes the relay re-serve the page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGR1u8SyzcUuekub39SBsc
2026-10-06 11:48:24 +00:00 · 2026-09-27 22:57:34 +00:00
parent 95894a9426
commit 6ccf754915
7 changed files with 176 additions and 17 deletions
@@ -360,8 +360,11 @@ suspend fun INostrClient.fetchAllPages(

        // Ids delivered on this page, kept only while an EOSE `"auth"` hint could still make
        // the relay re-serve the page after AUTH (at most once per walk), so the re-served
-        // copies of events already handed to [onEvent] are dropped. Reader-thread only.
-        val pageIds: HashSet<HexKey>? = if (pendingOnAuthRequired && !authRetried) HashSet() else null
+        // copies of events already handed to [onEvent] are dropped. The hint is rare, so the
+        // page only appends to a plain list (no hashing, no per-entry node); the lookup set
+        // is built from it once, on the first re-served event. Both are reader-thread only.
+        val pageIds: ArrayList<HexKey>? = if (pendingOnAuthRequired && !authRetried) ArrayList() else null
+        var reServedIds: HashSet<HexKey>? = null
        var reServing = false

        try {
@@ -393,7 +396,10 @@ suspend fun INostrClient.fetchAllPages(
                            if (boundary != null && event.createdAt == boundary && event.id in seenAtBoundary) return
                            // The relay re-serving this page after an EOSE "auth" hint: skip what
                            // this page already delivered.
-                            if (reServing && pageIds != null && event.id in pageIds) return
+                            if (reServing && pageIds != null) {
+                                val seenOnPage = reServedIds ?: HashSet(pageIds).also { reServedIds = it }
+                                if (event.id in seenOnPage) return
+                            }

                            // Count this event against every active filter it satisfies
                            // (one event can match more than one). Only a non-search filter
@@ -421,7 +427,10 @@ suspend fun INostrClient.fetchAllPages(
                            if (atLeastOne) {
                                onEvent(event)
                                delivered++
-                                pageIds?.add(event.id)
+                                if (pageIds != null) {
+                                    val seenOnPage = reServedIds
+                                    if (seenOnPage != null) seenOnPage.add(event.id) else pageIds.add(event.id)
+                                }
                                // Track the oldest advancing second and the ids delivered
                                // in it — that becomes the next boundary and its dedup set.
                                if (advancesCursor) {
@@ -28,6 +28,8 @@ import com.vitorpamplona.quartz.utils.Secp256k1Instance
 import com.vitorpamplona.quartz.utils.equalsConstantTime
 import kotlinx.coroutines.CancellationException
 import kotlin.io.encoding.Base64
+import kotlin.math.floor
+import kotlin.math.log2

 /**
 * NIP-44 v2 encryption.
@@ -160,6 +162,21 @@ class Nip44v2(
        return chunk * ((len - 1) / chunk + 1)
    }

+    /**
+     * The padded length earlier Amethyst builds produced: the spec formula evaluated in Float/Int
+     * math, which rounds `len - 1` to the nearest Float above 2^24 and so sometimes jumps a bucket.
+     * Only [unpad] uses it, to keep decrypting payloads those builds encrypted. Replicates the old
+     * code exactly, Int overflow included; lengths that never fit an Int could not have been padded.
+     */
+    private fun legacyCalcPaddedLen(len: Long): Long {
+        if (len <= 0 || len > Int.MAX_VALUE) return -1
+        val intLen = len.toInt()
+        if (intLen <= 32) return 32
+        val nextPower = 1 shl (floor(log2(intLen - 1f)) + 1).toInt()
+        val chunk = if (nextPower <= 256) 32 else nextPower / 8
+        return (chunk * (floor((intLen - 1f) / chunk).toInt() + 1)).toLong()
+    }
+
    fun pad(plaintext: String): ByteArray {
        val unpadded = plaintext.encodeToByteArray()
        val unpaddedLen = unpadded.size
@@ -204,7 +221,10 @@ class Nip44v2(
                "Invalid size $unpaddedLenExt not between $extMinPlaintextSize and $extMaxPlaintextSize"
            }

-            check(padded.size.toLong() == 6 + calcPaddedLen(unpaddedLenExt)) {
+            // Encryption is spec-exact, but earlier Amethyst builds padded with float math that picks a
+            // bigger bucket for some lengths above 2^24; accept those so old payloads still decrypt.
+            val paddedLen = padded.size.toLong() - 6
+            check(paddedLen == calcPaddedLen(unpaddedLenExt) || paddedLen == legacyCalcPaddedLen(unpaddedLenExt)) {
                "Invalid padding ${calcPaddedLen(unpaddedLenExt)} != $unpaddedLenExt"
            }

@@ -256,6 +256,9 @@ class NostrConnectSignerService(

                    RateLimiter.Decision.DENY_AND_NOTIFY -> {
                        Log.w("NIP46Signer") { "rate-limited request from ${event.pubKey.take(8)}…; replying with an error" }
+                        // The client is told this request failed, so a relay replaying it after a
+                        // restart must not get it serviced: persist its id like a serviced one.
+                        onHandledId?.invoke(event.id)
                        handleGate.acquire()
                        launch {
                            try {
@@ -116,8 +116,14 @@ class SimpleGroupListEvent(

        /**
         * Swaps [from] for [to] in one signed version — e.g. a NIP-29 group that migrated to another
-         * relay keeps its id but gets a new relay hint. [from] is dropped from both the public tags and
-         * the private items; [to] is added as a public tag.
+         * relay keeps its id but gets a new relay hint.
+         *
+         * Entries are matched by group id + *normalized* relay url on both sides, so a stored
+         * `wss://relay.example` (another client's spelling) is still found when [from] carries the
+         * normalized `wss://relay.example/`. Every copy of [from] and [to] is dropped from both the
+         * public tags and the private items, then [to] is added back where [from] lived: as a private
+         * item when [from] was only in the encrypted items (so a private membership stays private),
+         * otherwise as a public tag.
         */
        suspend fun replace(
            earlierVersion: SimpleGroupListEvent,
@@ -127,18 +133,31 @@ class SimpleGroupListEvent(
            createdAt: Long = TimeUtils.now(),
        ): SimpleGroupListEvent {
            val privateTags = earlierVersion.privateTags(signer) ?: throw SignerExceptions.UnauthorizedDecryptionException()
-            return resign(
-                privateTags = privateTags.remove(from.toTagIdOnly()),
-                tags =
-                    earlierVersion.tags
-                        .remove(from.toTagIdOnly())
-                        .remove(to.toTagIdOnly())
-                        .plus(to.toTagArray()),
-                signer = signer,
-                createdAt = createdAt,
-            )
+
+            val fromKey = groupKey(from.groupId, from.relayUrl)
+            val toKey = groupKey(to.groupId, to.relayUrl)
+            val isFrom = { tag: Array<String> -> tagGroupKey(tag) == fromKey }
+            val isFromOrTo = { tag: Array<String> -> tagGroupKey(tag).let { it == fromKey || it == toKey } }
+
+            val wasPrivateOnly = privateTags.any(isFrom) && earlierVersion.tags.none(isFrom)
+
+            val newPublic = earlierVersion.tags.remove(isFromOrTo)
+            val newPrivate = privateTags.remove(isFromOrTo)
+
+            return if (wasPrivateOnly) {
+                resign(tags = newPublic, privateTags = newPrivate.plus(to.toTagArray()), signer = signer, createdAt = createdAt)
+            } else {
+                resign(tags = newPublic.plus(to.toTagArray()), privateTags = newPrivate, signer = signer, createdAt = createdAt)
+            }
        }

+        private fun groupKey(
+            groupId: String,
+            relayUrl: String,
+        ) = groupId + "@" + (RelayUrlNormalizer.normalizeOrNull(relayUrl)?.url ?: relayUrl)
+
+        private fun tagGroupKey(tag: Array<String>): String? = GroupTag.parse(tag)?.let { groupKey(it.groupId, it.relayUrl) }
+
        suspend fun resign(
            tags: TagArray,
            privateTags: TagArray,
@@ -190,4 +190,38 @@ class Nip29SpecUpdatesTest {
            )
            assertEquals(2, moved.publicGroups().size)
        }
+
+    @Test
+    fun replaceMatchesAnUnnormalizedStoredRelayUrl() =
+        runTest {
+            val signer = NostrSignerInternal(KeyPair())
+            // Stored by another client without the trailing slash our normalizer adds.
+            val stored = GroupTag(gid, "wss://old.example.com", "Pizza")
+            val list = SimpleGroupListEvent.create(publicGroups = listOf(stored), signer = signer)
+
+            val moved =
+                SimpleGroupListEvent.replace(
+                    list,
+                    GroupTag(gid, "wss://old.example.com/", "Pizza"),
+                    GroupTag(gid, "wss://new.example.com/", "Pizza"),
+                    signer,
+                )
+
+            assertEquals(listOf(gid to "wss://new.example.com/"), moved.publicGroups().map { it.groupId to it.relayUrl })
+            assertEquals(emptyList(), moved.privateGroups(signer)?.map { it.groupId to it.relayUrl })
+        }
+
+    @Test
+    fun replaceKeepsAPrivateEntryPrivate() =
+        runTest {
+            val signer = NostrSignerInternal(KeyPair())
+            val publicOther = GroupTag("other", "wss://old.example.com/", null)
+            val secret = GroupTag(gid, "wss://old.example.com/", "Pizza")
+            val list = SimpleGroupListEvent.create(publicGroups = listOf(publicOther), privateGroups = listOf(secret), signer = signer)
+
+            val moved = SimpleGroupListEvent.replace(list, secret, GroupTag(gid, "wss://new.example.com/", "Pizza"), signer)
+
+            assertEquals(listOf("other" to "wss://old.example.com/"), moved.publicGroups().map { it.groupId to it.relayUrl })
+            assertEquals(listOf(gid to "wss://new.example.com/"), moved.privateGroups(signer)?.map { it.groupId to it.relayUrl })
+        }
 }
@@ -22,6 +22,8 @@ package com.vitorpamplona.quartz.nip44Encryption

 import com.vitorpamplona.quartz.utils.RandomInstance
 import kotlin.io.encoding.Base64
+import kotlin.math.floor
+import kotlin.math.log2
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
@@ -87,6 +89,48 @@ class Nip44v2PaddingTest {
        assertFailsWith<IllegalStateException> { nip44v2.unpad(padded2) }
    }

+    @Test
+    fun unpadAcceptsLegacyFloatPaddingAbove2e24() {
+        // Earlier builds padded 20,971,520 bytes to 25,165,824 (float bucket) instead of 20,971,520.
+        val len = 20_971_520
+        val legacy = extendedPadded(len, paddedLen = 25_165_824)
+        assertEquals(len, nip44v2.unpad(legacy).length)
+
+        // The spec-correct padding for the same length still decodes.
+        assertEquals(len, nip44v2.unpad(extendedPadded(len, paddedLen = 20_971_520)).length)
+
+        // Neither bucket: still rejected.
+        assertFailsWith<IllegalStateException> { nip44v2.unpad(extendedPadded(len, paddedLen = 20_971_520 + 32)) }
+    }
+
+    @Test
+    fun unpadAcceptsLegacyFloatPaddingAt2e25() {
+        // 2^25 - 1 rounds up to 2^25 as a Float, so the old math jumped to the 2^26 power bucket.
+        val len = 1 shl 25
+        assertEquals(41_943_040, extendedPaddedLenLegacy(len))
+        assertEquals(len, nip44v2.unpad(extendedPadded(len, paddedLen = 41_943_040)).length)
+    }
+
+    // The old Float formula, verbatim, as an independent oracle for the crafted arrays above.
+    private fun extendedPaddedLenLegacy(len: Int): Int {
+        val nextPower = 1 shl (floor(log2(len - 1f)) + 1).toInt()
+        val chunk = if (nextPower <= 256) 32 else nextPower / 8
+        return chunk * (floor((len - 1f) / chunk).toInt() + 1)
+    }
+
+    private fun extendedPadded(
+        len: Int,
+        paddedLen: Int,
+    ): ByteArray {
+        val padded = ByteArray(6 + paddedLen)
+        padded[2] = (len shr 24).toByte()
+        padded[3] = (len shr 16).toByte()
+        padded[4] = (len shr 8).toByte()
+        padded[5] = (len and 0xFF).toByte()
+        padded.fill('a'.code.toByte(), 6, 6 + len)
+        return padded
+    }
+
    @Test
    fun unpadRejectsZeroLength() {
        assertFailsWith<IllegalStateException> { nip44v2.unpad(ByteArray(2 + 32)) }
@@ -343,6 +343,36 @@ class NostrConnectSignerServiceTest {
            assertEquals(BunkerRequestProcessor.ERROR_RATE_LIMITED, replies[2].error)
        }

+    @Test
+    fun aRateLimitedRequestThatWasAnsweredIsRecordedAsHandled() =
+        runTest {
+            val client = LoopbackClient()
+            val signer = serverSigner()
+            val processor = BunkerRequestProcessor(signer, { setOf(relay) }, AllowAuthorizer())
+            val handled = mutableListOf<String>()
+            val service =
+                NostrConnectSignerService(
+                    client,
+                    signer,
+                    processor,
+                    setOf(relay),
+                    maxRequestsPerWindow = 1,
+                    rateWindowSeconds = 3600,
+                    onHandledId = { handled.add(it) },
+                )
+
+            backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { service.run() }
+
+            val serviced = request(BunkerRequestConnect(id = "ok", remoteKey = serverKey, secret = "s"))
+            val limited = request(BunkerRequestConnect(id = "limited", remoteKey = serverKey, secret = "s"))
+            client.deliver(serviced)
+            client.deliver(limited)
+
+            // The client was told `limited` failed; a relay replaying it after a restart must not get it
+            // serviced, so its id is persisted just like a serviced one.
+            assertEquals(listOf(serviced.id, limited.id), handled)
+        }
+
    @Test
    fun signEventWithoutParamsGetsAnErrorReply() =
        runTest {