fix(nip46): audit fixes — data race, cancellation, write amplification

Findings from an audit of the signer, all verified against the code:

- Data race: NostrConnectSignerService deduped request ids inside onEvent,
  which the relay pool invokes CONCURRENTLY from each relay's socket thread
  (PoolRequests dispatches listeners outside its lock). Two relays delivering
  the same subscription could mutate the LinkedHashSet at once → race / CME.
  Move dedup into the single consumer coroutine; onEvent now only does the
  thread-safe channel send.
- Swallowed cancellation: broad `catch (Exception)` around suspend calls in the
  processor, the service's decrypt + publish, and connectViaNostrConnect caught
  CancellationException too, breaking structured cancellation when the service
  restarts. Rethrow it first (matching the AccountCacheState convention).
- Write amplification: the ledger wrote last-used to that client's DataStore
  file on EVERY authorized request (unthrottled, unlike the relay-auth store).
  Coalesce to at most one write per client per 60s in the authorizer.
- Redundant resubscribe: the enable/relays collector lacked distinctUntilChanged,
  so a duplicate inbox-relay emission tore the subscription down and re-opened
  it on every relay for nothing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015FHr2mu5SiHwYNR7evYUuF

amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt

@@ -37,6 +37,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.server.BunkerRequestProcessor
 import com.vitorpamplona.quartz.nip46RemoteSigner.server.NostrConnectSignerService
 import com.vitorpamplona.quartz.utils.Log
 import com.vitorpamplona.quartz.utils.RandomInstance
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.flow.MutableStateFlow
@@ -44,6 +45,7 @@ import kotlinx.coroutines.flow.SharingStarted
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.collectLatest
 import kotlinx.coroutines.flow.combine
+import kotlinx.coroutines.flow.distinctUntilChanged
 import kotlinx.coroutines.flow.stateIn
 import kotlinx.coroutines.launch
 
@@ -121,6 +123,9 @@ class Nip46SignerState(
 
         scope.launch(Dispatchers.IO) {
             combine(settings.nip46SignerEnabled, listeningRelays) { enabled, relays -> enabled to relays }
+                // Inbox/relay StateFlows can re-emit an identical set; without this every duplicate
+                // would tear the subscription down and re-open it on every relay for no reason.
+                .distinctUntilChanged()
                 .collectLatest { (enabled, relays) ->
                     if (!enabled) return@collectLatest
                     if (!signer.isWriteable()) {
@@ -207,6 +212,8 @@ class Nip46SignerState(
 
             setEnabled(true)
             ConnectResult.Connected(offer.clientPubKey, offer.name)
+        } catch (e: CancellationException) {
+            throw e
         } catch (e: Exception) {
             Log.w("NIP46Signer") { "nostrconnect pairing failed: ${e.message}" }
             ConnectResult.Failed(e.message ?: "unknown error")

commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/napplet/signers/Nip46PermissionAuthorizer.kt

@@ -20,6 +20,8 @@
  */
 package com.vitorpamplona.amethyst.commons.napplet.signers
 
+import com.vitorpamplona.amethyst.commons.util.KmpLock
+import com.vitorpamplona.amethyst.commons.util.withLock
 import com.vitorpamplona.quartz.nip01Core.core.HexKey
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestConnect
@@ -30,6 +32,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestNip44Encrypt
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestSign
 import com.vitorpamplona.quartz.nip46RemoteSigner.server.Nip46ConnectDecision
 import com.vitorpamplona.quartz.nip46RemoteSigner.server.Nip46RequestAuthorizer
+import com.vitorpamplona.quartz.utils.TimeUtils
 
 /**
  * Bridges the NIP-46 signer core to Amethyst's shared "Connected Apps" trust
@@ -67,9 +70,30 @@ class Nip46PermissionAuthorizer(
     /** Invoked after a successful connect so the host can persist display metadata (name/url/image). */
     val onConnected: (suspend (clientPubKey: HexKey, request: BunkerRequestConnect) -> Unit)? = null,
 ) : Nip46RequestAuthorizer {
+    // A high-throughput client can authorize many signs per second; last-used is display-only,
+    // so coalesce the DataStore write to at most one per client per LAST_USED_THROTTLE_SECS
+    // instead of writing the whole per-client preferences file on every request.
+    private val lastUsedThrottle = mutableMapOf<String, Long>()
+    private val throttleLock = KmpLock()
+
     /** The ledger coordinate for [clientPubKey] under this account. */
     fun coordinateFor(clientPubKey: HexKey): String = coordinateFor(signerPubKey, clientPubKey)
 
+    private suspend fun touchLastUsed(coordinate: String) {
+        val now = TimeUtils.now()
+        val shouldWrite =
+            throttleLock.withLock {
+                val previous = lastUsedThrottle[coordinate] ?: 0L
+                if (now - previous >= LAST_USED_THROTTLE_SECS) {
+                    lastUsedThrottle[coordinate] = now
+                    true
+                } else {
+                    false
+                }
+            }
+        if (shouldWrite) ledger.updateLastUsed(coordinate, now)
+    }
+
     override suspend fun onConnect(
         clientPubKey: HexKey,
         request: BunkerRequestConnect,
@@ -82,7 +106,7 @@ class Nip46PermissionAuthorizer(
         if (!ledger.hasPolicy(coordinate)) {
             ledger.setPolicy(coordinate, defaultPolicyOnConnect)
         }
-        ledger.updateLastUsed(coordinate)
+        touchLastUsed(coordinate)
         onConnected?.invoke(clientPubKey, request)
 
         // Echo the offered secret when present (the client validates it); otherwise ack.
@@ -100,7 +124,7 @@ class Nip46PermissionAuthorizer(
         val op = request.toSignerOp() ?: return true
         val coordinate = coordinateFor(clientPubKey)
         val allowed = ledger.decide(coordinate, op) == NostrOpDecision.ALLOW
-        if (allowed) ledger.updateLastUsed(coordinate)
+        if (allowed) touchLastUsed(coordinate)
         return allowed
     }
 
@@ -115,6 +139,9 @@ class Nip46PermissionAuthorizer(
 
         private const val ACK = "ack"
 
+        /** Minimum seconds between persisted last-used updates for one client (write-coalescing). */
+        private const val LAST_USED_THROTTLE_SECS = 60L
+
         /**
          * The Connected-Apps ledger coordinate for a NIP-46 client, namespaced by
          * the user's signer so the same client paired with two accounts on one

quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt

@@ -44,6 +44,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponseGetRelays
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponsePong
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponsePublicKey
 import com.vitorpamplona.quartz.nip46RemoteSigner.ReadWrite
+import kotlinx.coroutines.CancellationException
 
 /**
  * The signer/bunker side of NIP-46: turns a decrypted [BunkerRequest] from a
@@ -130,6 +131,8 @@ class BunkerRequestProcessor(
                         else -> BunkerResponseError(request.id, "unsupported method: ${request.method}")
                     }
             }
+        } catch (e: CancellationException) {
+            throw e
         } catch (e: Exception) {
             BunkerResponseError(request.id, "${e::class.simpleName}: ${e.message}")
         }

quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt

@@ -31,6 +31,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponseError
 import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent
 import com.vitorpamplona.quartz.utils.Log
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.channels.Channel
 import kotlinx.coroutines.channels.Channel.Factory.UNLIMITED
 
@@ -76,8 +77,6 @@ class NostrConnectSignerService(
 
         val self = signer.pubKey
         val events = Channel<NostrConnectEvent>(UNLIMITED)
-        // Insertion-ordered so the oldest id can be evicted once the cap is hit.
-        val seen = LinkedHashSet<String>()
         val subId = newSubId()
         val listener =
             object : SubscriptionListener {
@@ -87,23 +86,31 @@ class NostrConnectSignerService(
                     relay: NormalizedRelayUrl,
                     forFilters: List<Filter>?,
                 ) {
-                    if (event is NostrConnectEvent && event.verifiedRecipientPubKey() == self && seen.add(event.id)) {
-                        if (seen.size > seenCap) {
-                            seen.iterator().let {
-                                it.next()
-                                it.remove()
-                            }
-                        }
+                    // onEvent is invoked CONCURRENTLY from each relay's socket thread, so it must
+                    // touch no shared mutable state here — the (thread-safe) channel send is all it
+                    // does; dedup happens in the single-threaded consumer below.
+                    if (event is NostrConnectEvent && event.verifiedRecipientPubKey() == self) {
                         events.trySend(event)
                     }
                 }
             }
 
+        // Insertion-ordered dedup, confined to this one consumer coroutine (never the relay threads);
+        // evicts the oldest id past the cap so a long-lived signer can't grow it without bound.
+        val seen = LinkedHashSet<String>()
         val filter = Filter(kinds = listOf(NostrConnectEvent.KIND), tags = mapOf("p" to listOf(self)))
         client.subscribe(subId, relays.associateWith { listOf(filter) }, listener)
         try {
             while (true) {
-                handle(events.receive())
+                val event = events.receive()
+                if (!seen.add(event.id)) continue // same request already handled (arrived on another relay)
+                if (seen.size > seenCap) {
+                    seen.iterator().let {
+                        it.next()
+                        it.remove()
+                    }
+                }
+                handle(event)
             }
         } finally {
             client.unsubscribe(subId)
@@ -116,6 +123,8 @@ class NostrConnectSignerService(
         val request =
             try {
                 event.decryptMessage(signer) as? BunkerRequest ?: return
+            } catch (e: CancellationException) {
+                throw e
             } catch (e: Exception) {
                 Log.w("NIP46Signer") { "could not decrypt request ${event.id.take(8)}: ${e.message}" }
                 return
@@ -128,6 +137,8 @@ class NostrConnectSignerService(
         try {
             val reply = NostrConnectEvent.create(response, client, signer)
             this.client.publish(reply, relays)
+        } catch (e: CancellationException) {
+            throw e
         } catch (e: Exception) {
             Log.w("NIP46Signer") { "failed to send reply for ${request.method}: ${e.message}" }
         }