From 1d8b7d7c8f8444e06baa5c8461997463c6feee94 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 21:30:43 +0000 Subject: [PATCH] feat(nip46): batched consent via concurrent request dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third refinement from the Primal comparison — and the one that needed an architecture change, not just UI. Quartz: NostrConnectSignerService now fans each request into a child coroutine under a Semaphore(maxConcurrentHandles=16) instead of handling them inline, so a request awaiting a consent prompt no longer blocks other clients' auto-allowed traffic and several prompts can be pending at once. Intake (dedup, staleness, rate-limit, seen-id persistence) stays on the single consumer. Two guards keep it safe: BunkerRequestProcessor serializes the actual crypto with a Mutex (authorization — the prompt — runs unlocked, only sign/encrypt/decrypt holds the lock) so an external NIP-55 signer never sees concurrent IPC ops; and Nip46PermissionAuthorizer serializes first-connect consent so two connects can't stack dialogs. Covered by BunkerRequestProcessorConcurrencyTest (crypto never overlaps; a blocked prompt doesn't stall another client's signing). Amethyst: SignerConsentCoordinator is now a shared pending StateFlow; one SignerConsentActivity observes it and shows the rich single-request dialog (1 pending) or a batched checkbox list with select-all + a Remember toggle + Allow/Deny selected (>1). Dismissing the sheet denies every still-open request (fail closed). Needs on-device validation (burst batching, no concurrent external-signer IPC, fail-closed on dismiss) — see the device checklist. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015FHr2mu5SiHwYNR7evYUuF --- ...026-07-16-nip46-signer-device-checklist.md | 30 ++- .../consent/SignerConsentActivity.kt | 191 +++++++++++++++--- .../consent/SignerConsentCoordinator.kt | 77 ++++--- amethyst/src/main/res/values/strings.xml | 9 + .../nip46/Nip46PermissionAuthorizer.kt | 41 +++- .../server/BunkerRequestProcessor.kt | 15 +- .../server/NostrConnectSignerService.kt | 37 +++- .../BunkerRequestProcessorConcurrencyTest.kt | 172 ++++++++++++++++ 8 files changed, 499 insertions(+), 73 deletions(-) create mode 100644 quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt diff --git a/amethyst/plans/2026-07-16-nip46-signer-device-checklist.md b/amethyst/plans/2026-07-16-nip46-signer-device-checklist.md index 885f69b607..c8ee2aedf5 100644 --- a/amethyst/plans/2026-07-16-nip46-signer-device-checklist.md +++ b/amethyst/plans/2026-07-16-nip46-signer-device-checklist.md @@ -109,13 +109,29 @@ that loop via `collectLatest`. - **FIXED — unbounded first-connect prompt.** `Nip46ConsentBridge.requestConnect` now has the same 120s `withTimeoutOrNull` as `requestOp`, so an ignored first-connect dialog can no longer wedge the loop forever. -- **Consent blocks other clients (bounded).** While one prompt is open, other - clients' requests queue in the 256-deep DROP_LATEST channel and, past that, - drop. Bounded by the 120s timeouts. A proper fix is to dispatch prompt-needing - requests to child jobs (keeping dedup/rate-limit/`decide()` on the loop - thread, serializing only the dialogs) so auto-allowed traffic keeps flowing — - deferred because it risks stacked dialogs + concurrent external-signer ops and - needs on-device validation. +- **FIXED — consent no longer blocks other clients (needs on-device + validation).** The service now fans each request out into a child coroutine + under a `Semaphore(maxConcurrentHandles=16)`; dedup/staleness/rate-limit stay on + the single consumer, only `handle()` runs concurrently. So a request awaiting a + prompt no longer stalls auto-allowed traffic, and several prompts can be pending + at once. Two guards keep this safe: (1) the identity signer's crypto is + serialized by `BunkerRequestProcessor.cryptoLock` — authorization (the prompt) + runs UNLOCKED, only the sign/encrypt/decrypt holds the lock — so an external + NIP-55 app never sees concurrent IPC ops; (2) first-connect consent is + serialized by `Nip46PermissionAuthorizer.connectLock` so two connects can't stack + dialogs. Per-op prompts batch: the shared `SignerConsentCoordinator.pending` + flow drives one dialog (1 pending) or a checkbox list (>1). Covered by + `BunkerRequestProcessorConcurrencyTest`, but the on-device paths below still need + a real run: + - [ ] **Burst batching:** a client fires several dangerous-kind requests at once + → one batched sheet with checkboxes + select-all, Allow/Deny selected, + "Remember" toggle. Approving a subset leaves the rest pending. + - [ ] **Auto-allowed keeps flowing:** while a prompt sits open, a REASONABLE + auto-allowed request from another app still gets signed and answered. + - [ ] **No concurrent external-signer ops:** with a NIP-55 external signer, two + approved requests do not drive overlapping IPC (they serialize). + - [ ] **Fail-closed on dismiss:** backing out of the batched sheet denies every + still-open request (not just the selected ones). - **Relay-set change cancels in-flight work.** A `logout` (or a new nostrconnect pairing) mutates the listen set → `collectLatest` restarts the service → cancels the in-flight `handle()`. Practical impact is low (a logout ACK is lost diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt index 549bf0ec03..cfb22ec85c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt @@ -40,13 +40,16 @@ import androidx.compose.foundation.text.selection.SelectionContainer import androidx.compose.foundation.verticalScroll import androidx.compose.material3.Button import androidx.compose.material3.ButtonDefaults +import androidx.compose.material3.Checkbox import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.MaterialTheme import androidx.compose.material3.OutlinedButton import androidx.compose.material3.Surface +import androidx.compose.material3.Switch import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -54,12 +57,14 @@ import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalConfiguration +import androidx.compose.ui.res.pluralStringResource import androidx.compose.ui.res.stringResource import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.unit.dp import androidx.compose.ui.window.Dialog import androidx.compose.ui.window.DialogProperties +import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.commons.connectedApps.signers.SignerOpGrant import com.vitorpamplona.amethyst.commons.favorites.FavoriteApp @@ -76,41 +81,40 @@ import com.vitorpamplona.quartz.nip59Giftwrap.rumors.RumorAssembler import com.vitorpamplona.quartz.utils.TimeUtils class SignerConsentActivity : ComponentActivity() { - private var token: String? = null - private var decided = false - override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) - val token = intent.getStringExtra(SignerConsentCoordinator.EXTRA_TOKEN) - this.token = token - val info = token?.let { SignerConsentCoordinator.infoFor(it) } - if (token == null || info == null) { - finish() - return - } - setContent { AmethystTheme { - SignerConsentDialog( - info = info, - onGrant = { grant -> - decided = true - SignerConsentCoordinator.complete(token, grant) - finish() - }, - onDismiss = { - decided = true - SignerConsentCoordinator.cancel(token) - finish() - }, - ) + // The signer services requests concurrently, so more than one may await consent. Observe + // the shared queue: one request shows the rich dialog, several show a batched list. When + // the queue empties (all decided), close. + val pending by SignerConsentCoordinator.pending.collectAsStateWithLifecycle() + LaunchedEffect(pending.isEmpty()) { if (pending.isEmpty()) finish() } + when { + pending.isEmpty() -> Unit + pending.size == 1 -> { + val p = pending.first() + SignerConsentDialog( + info = p.info, + onGrant = { SignerConsentCoordinator.complete(p.token, it) }, + onDismiss = { SignerConsentCoordinator.complete(p.token, SignerOpGrant.DenyOnce) }, + ) + } + else -> + BatchedConsentDialog( + pending = pending, + onResolve = { tokens, grant -> SignerConsentCoordinator.completeAll(tokens, grant) }, + onDismiss = { SignerConsentCoordinator.denyAllPending() }, + ) + } } } } - override fun finish() { - if (!decided) token?.let { SignerConsentCoordinator.cancel(it) } - super.finish() + override fun onDestroy() { + super.onDestroy() + // Torn down for good (back / dismiss), not a config change: fail every still-open request closed. + if (isFinishing) SignerConsentCoordinator.denyAllPending() } } @@ -355,3 +359,136 @@ private fun SignerConsentDialog( } } } + +/** + * Shown when more than one request is awaiting consent at once (the signer services requests + * concurrently). Lists each with a checkbox — all selected by default — and resolves the selected + * ones together as Allow or Deny. "Remember" makes an Allow persist per-op ([SignerOpGrant.AllowForOp]); + * off is a one-time [SignerOpGrant.AllowOnce]. Requests left unselected stay pending and re-render + * (as this list, or the single-request dialog once one remains). + */ +@Composable +private fun BatchedConsentDialog( + pending: List, + onResolve: (tokens: List, grant: SignerOpGrant) -> Unit, + onDismiss: () -> Unit, +) { + val maxHeight = LocalConfiguration.current.screenHeightDp.dp * 0.85f + var selected by remember(pending.size) { mutableStateOf(pending.map { it.token }.toSet()) } + var rememberChoice by remember { mutableStateOf(true) } + + Dialog( + onDismissRequest = onDismiss, + properties = DialogProperties(usePlatformDefaultWidth = false), + ) { + Surface( + modifier = + Modifier + .fillMaxWidth() + .padding(horizontal = 16.dp) + .heightIn(max = maxHeight), + shape = MaterialTheme.shapes.extraLarge, + color = MaterialTheme.colorScheme.surface, + tonalElevation = 6.dp, + ) { + Column(modifier = Modifier.padding(vertical = 20.dp)) { + Text( + pluralStringResource(R.plurals.nip46_signer_batch_title, pending.size, pending.size), + style = MaterialTheme.typography.titleLarge, + modifier = Modifier.padding(horizontal = 24.dp), + ) + TextButton( + onClick = { + selected = if (selected.size == pending.size) emptySet() else pending.map { it.token }.toSet() + }, + contentPadding = PaddingValues(horizontal = 20.dp, vertical = 2.dp), + ) { + Text( + stringResource( + if (selected.size == pending.size) R.string.nip46_signer_batch_select_none else R.string.nip46_signer_batch_select_all, + ), + style = MaterialTheme.typography.labelLarge, + ) + } + + Column( + modifier = + Modifier + .weight(1f, fill = false) + .verticalScroll(rememberScrollState()), + ) { + pending.forEach { p -> + Row( + modifier = + Modifier + .fillMaxWidth() + .padding(horizontal = 12.dp, vertical = 2.dp), + verticalAlignment = Alignment.CenterVertically, + horizontalArrangement = Arrangement.spacedBy(4.dp), + ) { + Checkbox( + checked = p.token in selected, + onCheckedChange = { on -> selected = if (on) selected + p.token else selected - p.token }, + ) + Column(modifier = Modifier.weight(1f)) { + Text( + "${p.info.appletTitle} · ${p.info.operationSummary}", + style = MaterialTheme.typography.bodyMedium, + maxLines = 1, + ) + if (p.info.contentPreview.isNotBlank()) { + Text( + p.info.contentPreview, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + maxLines = 1, + ) + } + } + } + } + } + + Row( + modifier = Modifier.fillMaxWidth().padding(horizontal = 24.dp, vertical = 4.dp), + verticalAlignment = Alignment.CenterVertically, + horizontalArrangement = Arrangement.spacedBy(8.dp), + ) { + Switch(checked = rememberChoice, onCheckedChange = { rememberChoice = it }) + Text( + stringResource(R.string.nip46_signer_batch_remember), + style = MaterialTheme.typography.bodyMedium, + ) + } + + Spacer(Modifier.height(8.dp)) + HorizontalDivider() + Spacer(Modifier.height(8.dp)) + + Button( + onClick = { + val tokens = pending.filter { it.token in selected } + // Per-op remember uses each request's own op; one-time is a single AllowOnce. + if (rememberChoice) { + tokens.forEach { onResolve(listOf(it.token), SignerOpGrant.AllowForOp(it.info.op)) } + } else { + onResolve(tokens.map { it.token }, SignerOpGrant.AllowOnce) + } + }, + enabled = selected.isNotEmpty(), + modifier = Modifier.fillMaxWidth().padding(horizontal = 24.dp), + ) { + Text(stringResource(R.string.nip46_signer_batch_allow, selected.size)) + } + OutlinedButton( + onClick = { onResolve(pending.filter { it.token in selected }.map { it.token }, SignerOpGrant.DenyOnce) }, + enabled = selected.isNotEmpty(), + modifier = Modifier.fillMaxWidth().padding(horizontal = 24.dp), + colors = ButtonDefaults.outlinedButtonColors(contentColor = MaterialTheme.colorScheme.error), + ) { + Text(stringResource(R.string.nip46_signer_batch_deny, selected.size)) + } + } + } + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt index 71fc5549d3..ed8e1f9c86 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt @@ -28,6 +28,9 @@ import com.vitorpamplona.amethyst.commons.connectedApps.signers.SignerOpGrant import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.signers.EventTemplate import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.update import java.util.UUID import java.util.concurrent.ConcurrentHashMap @@ -61,17 +64,29 @@ data class SignerConsentInfo( val previewTemplate: EventTemplate? = null, ) +/** One pending per-operation consent request, as the batched sheet renders it. */ +data class PendingConsent( + val token: String, + val info: SignerConsentInfo, +) + /** - * Bridges the broker to the per-operation signer consent UI. - * A dismissed dialog resolves to [SignerOpGrant.DenyOnce] — fails closed. + * Bridges the broker to the per-operation signer consent UI. The signer services requests + * concurrently (so their prompts can batch), so several requests can await consent at once: they all + * land in [pending], one [SignerConsentActivity] observes that list and shows a single-request dialog + * or a batched list, and each resolved token completes its own deferred. A dismissed/ignored request + * resolves to [SignerOpGrant.DenyOnce] — fails closed. */ object SignerConsentCoordinator { - private class Pending( - val info: SignerConsentInfo, - val deferred: CompletableDeferred, - ) + private val deferreds = ConcurrentHashMap>() + private val _pending = MutableStateFlow>(emptyList()) - private val pending = ConcurrentHashMap() + /** The live set of requests awaiting the user's decision; the Activity renders this. */ + val pending: StateFlow> = _pending + + // A stable notification id (one prompt notification for the whole batch, updated as requests + // arrive) so concurrent requests don't each post their own. + private val batchNotificationId = "nip46-signer-consent".hashCode() suspend fun requestConsent( context: Context, @@ -79,48 +94,54 @@ object SignerConsentCoordinator { ): SignerOpGrant { val token = UUID.randomUUID().toString() val deferred = CompletableDeferred() - pending[token] = Pending(info, deferred) + deferreds[token] = deferred + _pending.update { it + PendingConsent(token, info) } // Fast path when Amethyst already owns the foreground: open the dialog directly. When the app // is backgrounded this is silently dropped by Android 12+ BAL, so the full-screen-intent - // notification below is what actually surfaces the prompt. Wrapped because a blocked launch - // can throw on some OEMs rather than no-op. + // notification is what surfaces the prompt. Both are idempotent — the Activity is singleTop and + // observes [pending], and the notification uses a stable id, so concurrent requests just refresh + // the one prompt. Wrapped because a BAL-blocked launch can throw on some OEMs rather than no-op. runCatching { context.startActivity( Intent(context, SignerConsentActivity::class.java) - .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) - .putExtra(EXTRA_TOKEN, token), + .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_SINGLE_TOP), ) } - - val notificationId = - SignerConsentNotifier.show( - context = context, - activityClass = SignerConsentActivity::class.java, - extraKey = EXTRA_TOKEN, - token = token, - titleRes = R.string.nip46_signer_notif_sign_title, - ) + SignerConsentNotifier.show( + context = context, + activityClass = SignerConsentActivity::class.java, + extraKey = EXTRA_TOKEN, + token = "nip46-signer-consent", + titleRes = R.string.nip46_signer_notif_sign_title, + ) return try { deferred.await() } finally { - pending.remove(token) - SignerConsentNotifier.cancel(context, notificationId) + deferreds.remove(token) + _pending.update { list -> list.filterNot { it.token == token } } + if (_pending.value.isEmpty()) SignerConsentNotifier.cancel(context, batchNotificationId) } } - fun infoFor(token: String): SignerConsentInfo? = pending[token]?.info - fun complete( token: String, grant: SignerOpGrant, ) { - pending[token]?.deferred?.complete(grant) + deferreds[token]?.complete(grant) } - fun cancel(token: String) { - pending[token]?.deferred?.complete(SignerOpGrant.DenyOnce) + fun completeAll( + tokens: Collection, + grant: SignerOpGrant, + ) { + tokens.forEach { complete(it, grant) } + } + + /** Deny every still-open request — used when the user dismisses the whole sheet. Fails closed. */ + fun denyAllPending() { + deferreds.values.forEach { it.complete(SignerOpGrant.DenyOnce) } } const val EXTRA_TOKEN = "napplet_signer_consent_token" diff --git a/amethyst/src/main/res/values/strings.xml b/amethyst/src/main/res/values/strings.xml index 47e9bea0ca..83b1dda911 100644 --- a/amethyst/src/main/res/values/strings.xml +++ b/amethyst/src/main/res/values/strings.xml @@ -952,6 +952,15 @@ Sign for other apps Turn on signer Live + + %1$d signing request + %1$d signing requests + + Select all + Select none + Remember these for each app + Allow %1$d + Deny %1$d Recent activity No requests serviced yet. denied diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt index f09df51598..1eb84daa24 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt @@ -39,6 +39,8 @@ 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 +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock /** * Bridges the NIP-46 signer core to Amethyst's shared "Connected Apps" trust @@ -110,6 +112,12 @@ class Nip46PermissionAuthorizer( private val lastUsedThrottle = mutableMapOf() private val throttleLock = KmpLock() + // Serializes first-connect consent. The service now handles requests concurrently so per-op prompts + // can batch, but the connect prompt is a separate single dialog — this keeps two clients connecting + // at once from stacking two connect dialogs; the second waits for the first to resolve. A coroutine + // Mutex (not KmpLock) because the guarded region awaits a user dialog and must suspend, not block. + private val connectLock = Mutex() + /** The ledger coordinate for [clientPubKey] under this account. */ fun coordinateFor(clientPubKey: HexKey): String = coordinateFor(signerPubKey, clientPubKey) @@ -137,16 +145,31 @@ class Nip46PermissionAuthorizer( } val coordinate = coordinateFor(clientPubKey) - if (!ledger.hasPolicy(coordinate)) { - // First contact: ask the user (if a prompt is wired) which trust level to grant; a - // headless signer with no prompt falls back to the non-interactive default. - when (val consent = connectConsent?.invoke(coordinate, clientPubKey, request)) { - null -> ledger.setPolicy(coordinate, defaultPolicyOnConnect) - is AppConnectResult.Connected -> ledger.setPolicy(coordinate, consent.policy) - AppConnectResult.Blocked -> return Nip46ConnectDecision.Reject("blocked by user") - AppConnectResult.Cancelled -> return Nip46ConnectDecision.Reject("connection declined") + // Serialize first-contact consent so two concurrent connects don't stack dialogs. The + // hasPolicy re-check inside the lock also means a client that connected on another in-flight + // request isn't prompted twice. + val rejection = + connectLock.withLock { + if (ledger.hasPolicy(coordinate)) { + null + } else { + // First contact: ask the user (if a prompt is wired) which trust level to grant; a + // headless signer with no prompt falls back to the non-interactive default. + when (val consent = connectConsent?.invoke(coordinate, clientPubKey, request)) { + null -> { + ledger.setPolicy(coordinate, defaultPolicyOnConnect) + null + } + is AppConnectResult.Connected -> { + ledger.setPolicy(coordinate, consent.policy) + null + } + AppConnectResult.Blocked -> "blocked by user" + AppConnectResult.Cancelled -> "connection declined" + } + } } - } + if (rejection != null) return Nip46ConnectDecision.Reject(rejection) touchLastUsed(coordinate) onConnected?.invoke(clientPubKey, request) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt index 951a6ecc69..973f288b45 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt @@ -45,6 +45,8 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponsePong import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponsePublicKey import com.vitorpamplona.quartz.nip46RemoteSigner.ReadWrite import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock /** * The signer/bunker side of NIP-46: turns a decrypted [BunkerRequest] from a @@ -72,6 +74,15 @@ class BunkerRequestProcessor( val relays: suspend () -> Set, val authorizer: Nip46RequestAuthorizer, ) { + /** + * Serializes the actual crypto ([signer] `sign`/`nip04|44_*`) across concurrent [process] calls. + * The service may run several requests at once so their consent prompts can batch, but the identity + * signer — especially an external NIP-55 app reached over IPC — must not see concurrent operations, + * so only the crypto runs under this lock. Authorization (which may open a user prompt and block for + * a long time) runs OUTSIDE the lock, so a pending prompt never stalls other clients' signing. + */ + private val cryptoLock = Mutex() + /** * Fulfils a single decrypted [request] sent by [clientPubKey], returning the * response to encrypt and send back. Never throws — signer/authorizer errors @@ -147,7 +158,9 @@ class BunkerRequestProcessor( // external signer is gone — so refuse rather than prompt or hang on a key we can't use. BunkerResponseError(request.id, ERROR_ACCOUNT_UNAVAILABLE) } else if (authorizer.authorize(clientPubKey, request)) { - block() + // Authorization ran unlocked (it may have blocked on a user prompt); the crypto itself runs + // under [cryptoLock] so concurrent authorized requests don't hit the signer at the same time. + cryptoLock.withLock { block() } } else { BunkerResponseError(request.id, ERROR_UNAUTHORIZED) } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt index ed7b05825b..c94e8bd684 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt @@ -33,8 +33,12 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.TimeUtils import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.channels.BufferOverflow import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.launch +import kotlinx.coroutines.supervisorScope +import kotlinx.coroutines.sync.Semaphore /** * Runs a NIP-46 remote signer ("bunker") for one account: subscribes to the @@ -97,6 +101,15 @@ class NostrConnectSignerService( * kept small so an app restart re-signs as little as possible (relays replay only this far back). */ val maxRequestAgeSeconds: Long = 30, + /** + * How many requests may be in-flight (past dedup/rate-limit) at once. Each request is handled in + * its own child coroutine so that a request awaiting a user consent prompt does NOT block other + * clients' auto-allowed traffic — and so several prompts can be pending together and be approved in + * one batch. Intake (dedup, staleness, rate-limit) stays on the single consumer; only [handle] fans + * out. The actual crypto is still serialized inside [BunkerRequestProcessor]. This bounds how many + * child coroutines (and pending prompts) can accumulate under a flood. + */ + val maxConcurrentHandles: Int = 16, /** * Event ids serviced in a previous run, used to seed the in-memory dedup set so a relay replaying * stored requests across an app restart is caught by EXACT event id — immune to client clock skew, @@ -155,6 +168,14 @@ class NostrConnectSignerService( return } + // supervisorScope: each request is handled in a child coroutine so a prompt awaiting the user + // doesn't block the loop or other clients; a failing child never tears down the loop or siblings. + supervisorScope { + runLoop() + } + } + + private suspend fun kotlinx.coroutines.CoroutineScope.runLoop() { val self = transportSigner.pubKey // Bounded + DROP_LATEST so a flood bounds memory instead of growing an unlimited queue. val events = Channel(capacity = maxQueue, onBufferOverflow = BufferOverflow.DROP_LATEST) @@ -182,6 +203,8 @@ class NostrConnectSignerService( // Seed the dedup set with ids serviced in a prior run so a relay replaying stored requests after // a restart is caught by exact id (see [initialSeen]). val seen = LinkedHashSet(initialSeen) + // Bounds how many requests can be in-flight (and how many prompts can be pending) at once. + val handleGate = Semaphore(maxConcurrentHandles) // Only ask relays for recent requests: kind-24133 is ephemeral, but relays that store it would // otherwise replay every old request each time we (re)subscribe. See [maxRequestAgeSeconds]. val filter = Filter(kinds = listOf(NostrConnectEvent.KIND), tags = mapOf("p" to listOf(self)), since = TimeUtils.now() - maxRequestAgeSeconds) @@ -209,9 +232,21 @@ class NostrConnectSignerService( Log.w("NIP46Signer") { "rate-limited request from ${event.pubKey.take(8)}…" } continue } - handle(event) // Remember this id (persisted by the host) so a later restart won't re-service the replay. + // Done on the single consumer — BEFORE fanning out — because the host's seen-id store is + // not synchronized; a request we decided to service here should not be re-prompted after a + // restart even if it is ultimately denied (the in-memory `seen` already covers this run). onHandledId?.invoke(event.id) + // Acquire BEFORE launching so intake applies backpressure at the cap instead of spawning + // unbounded child coroutines; dedup/rate-limit above already ran on this single consumer. + handleGate.acquire() + launch { + try { + handle(event) + } finally { + handleGate.release() + } + } } } finally { client.unsubscribe(subId) diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt new file mode 100644 index 0000000000..f64a4fc7a6 --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt @@ -0,0 +1,172 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.nip46RemoteSigner.server + +import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer +import com.vitorpamplona.quartz.nip01Core.signers.EventTemplate +import com.vitorpamplona.quartz.nip01Core.signers.NostrSigner +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestConnect +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestSign +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponse +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponseEvent +import com.vitorpamplona.quartz.nip57Zaps.LnZapPrivateEvent +import com.vitorpamplona.quartz.nip57Zaps.LnZapRequestEvent +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Concurrency guarantees the service relies on when it fans requests out into child coroutines so + * their consent prompts can batch: the identity signer's crypto must never run concurrently (an + * external NIP-55 app can't take overlapping IPC ops), while authorization — which may block on a + * user prompt for a long time — must NOT hold that lock, so a pending prompt can't stall other + * clients' signing. + */ +class BunkerRequestProcessorConcurrencyTest { + private val userPubKey = "a".repeat(64) + private val clientPubKey = "c".repeat(64) + private val relay = RelayUrlNormalizer.normalizeOrNull("wss://relay.example.com")!! + + private fun signTemplate(id: String) = BunkerRequestSign(id, EventTemplate(createdAt = 1L, kind = 1, tags = emptyArray(), content = "hi")) + + /** A signer whose sign() parks on [signGate] so tests can observe how many run at once. */ + private class GatedSigner( + pubKey: HexKey, + val signGate: CompletableDeferred, + ) : NostrSigner(pubKey) { + var inFlight = 0 + var maxConcurrent = 0 + var signCount = 0 + + val canned = Event(id = "e".repeat(64), pubKey = pubKey, createdAt = 1L, kind = 1, tags = emptyArray(), content = "s", sig = "f".repeat(128)) + + override fun isWriteable() = true + + @Suppress("UNCHECKED_CAST") + override suspend fun sign( + createdAt: Long, + kind: Int, + tags: Array>, + content: String, + ): T { + inFlight++ + maxConcurrent = maxOf(maxConcurrent, inFlight) + signGate.await() + inFlight-- + signCount++ + return canned as T + } + + override suspend fun nip04Encrypt( + plaintext: String, + toPublicKey: HexKey, + ) = "" + + override suspend fun nip04Decrypt( + ciphertext: String, + fromPublicKey: HexKey, + ) = "" + + override suspend fun nip44Encrypt( + plaintext: String, + toPublicKey: HexKey, + ) = "" + + override suspend fun nip44Decrypt( + ciphertext: String, + fromPublicKey: HexKey, + ) = "" + + override suspend fun decryptZapEvent(event: LnZapRequestEvent): LnZapPrivateEvent = throw NotImplementedError() + + override suspend fun deriveKey(nonce: HexKey): HexKey = throw NotImplementedError() + + override suspend fun signPsbt(psbtHex: String): String = throw NotImplementedError() + + override fun hasForegroundSupport() = false + } + + /** authorize() parks on the deferred returned by [gateFor] (null = allow immediately). */ + private class GatedAuthorizer( + val gateFor: (BunkerRequest) -> CompletableDeferred?, + ) : Nip46RequestAuthorizer { + override suspend fun onConnect( + clientPubKey: HexKey, + request: BunkerRequestConnect, + ) = Nip46ConnectDecision.Accept("ack") + + override suspend fun authorize( + clientPubKey: HexKey, + request: BunkerRequest, + ): Boolean = gateFor(request)?.await() ?: true + } + + @Test + fun cryptoIsSerializedAcrossConcurrentAuthorizedRequests() = + runTest { + val signGate = CompletableDeferred() + val signer = GatedSigner(userPubKey, signGate) + val processor = BunkerRequestProcessor(signer, { setOf(relay) }, GatedAuthorizer { null }) + + launch { processor.process(clientPubKey, signTemplate("1")) } + launch { processor.process(clientPubKey, signTemplate("2")) } + testScheduler.advanceUntilIdle() + + // Both were authorized instantly, but only one may be inside the signer at a time. + assertEquals(1, signer.inFlight, "only one sign holds the crypto lock") + assertEquals(1, signer.maxConcurrent, "crypto never overlapped") + + signGate.complete(Unit) + testScheduler.advanceUntilIdle() + assertEquals(2, signer.signCount, "both eventually signed, one after the other") + assertEquals(1, signer.maxConcurrent, "still never overlapped") + } + + @Test + fun aBlockedPromptDoesNotStallAnotherClientsSigning() = + runTest { + val signGate = CompletableDeferred().apply { complete(Unit) } // signing itself never blocks here + val signer = GatedSigner(userPubKey, signGate) + val prompt = CompletableDeferred() // stands in for a user consent dialog left open + val blocked = signTemplate("blocked") + val processor = + BunkerRequestProcessor(signer, { setOf(relay) }, GatedAuthorizer { if (it === blocked) prompt else null }) + + launch { processor.process(clientPubKey, blocked) } // parks in authorize(), never touching the lock + var fastResult: BunkerResponse? = null + launch { fastResult = processor.process(clientPubKey, signTemplate("fast")) } + testScheduler.advanceUntilIdle() + + // The auto-allowed request signed and returned while the prompt is still open. + assertTrue(fastResult is BunkerResponseEvent, "auto-allowed request completed while a prompt was pending") + assertEquals(1, signer.signCount) + + prompt.complete(true) + testScheduler.advanceUntilIdle() + assertEquals(2, signer.signCount, "the prompted request signs once approved") + } +}