diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt index 1e6b1a05f2..3a1e7d76ae 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt @@ -24,6 +24,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient import com.vitorpamplona.quartz.nip01Core.relay.client.auth.AuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.DEFAULT_AUTH_GRACE_MS import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMark +import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMarks import com.vitorpamplona.quartz.nip01Core.relay.client.auth.awaitAuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.hasAuthResponder import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.RelayConnectionListener @@ -202,7 +203,7 @@ suspend fun INostrClient.count( try { addConnectionListener(listener) - val authMarks = if (pendingOnAuthRequired) filters.keys.associateWith { authSuccessMark(it) } else emptyMap() + val authMarks = if (pendingOnAuthRequired) authSuccessMarks(filters.keys) else emptyMap() filters.forEach { (relay, filterList) -> val subId = newSubId() subIdToRelay[subId] = relay diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt index ef221707bb..b6739312cd 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt @@ -24,7 +24,7 @@ import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient import com.vitorpamplona.quartz.nip01Core.relay.client.auth.AuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.DEFAULT_AUTH_GRACE_MS -import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMark +import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMarks import com.vitorpamplona.quartz.nip01Core.relay.client.auth.awaitAuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.hasAuthResponder import com.vitorpamplona.quartz.nip01Core.relay.client.reqs.SubscriptionListener @@ -169,12 +169,22 @@ suspend fun INostrClient.fetchAllWithHooks( relay: NormalizedRelayUrl, forFilters: List?, ) { - // Keep the relay pending on an auth-required refusal: the authenticator answers the - // challenge and re-fires this subscription, so the post-auth events still arrive. - // Hand it to the resolver, which ends the relay as auth-refused if the challenge - // does not work out — the refusal stays bounded by the AUTH, not by the timeout. - if (pendingOnAuthRequired && MachineReadablePrefix.parse(message) == MachineReadablePrefix.AUTH_REQUIRED) { - authRefusalChannel.trySend(relay to message) + if (MachineReadablePrefix.parse(message) == MachineReadablePrefix.AUTH_REQUIRED) { + // Keep the relay pending: the authenticator answers the challenge and re-fires + // this subscription, so the post-auth events still arrive. The resolver ends it + // as auth-refused if the challenge does not work out — bounded by the AUTH, not + // by the timeout. + if (pendingOnAuthRequired) { + authRefusalChannel.trySend(relay to message) + return + } + // Not waiting — but still NAME the wall. What the relay said does not depend on + // whether we chose to answer it, and a caller reading `doneOut` wants to know it + // gave up on an auth wall rather than on a policy refusal it can do nothing + // about. This is what [fetchAllPages] already does with End.AUTH_REQUIRED, and + // leaving it as a plain `closed:` here is what would make + // [authRefusedRelays] silently miss every no-responder client. + doneChannel.trySend(relay to "$DONE_REASON_AUTH_REFUSED:$message") return } doneChannel.trySend(relay to "closed:$message") @@ -192,7 +202,7 @@ suspend fun INostrClient.fetchAllWithHooks( // resolver compares against these: an AUTH that lands after this point is one that // re-sent our subscription, whereas a connection that was already authenticated and // still refused us is being gated for a reason no further waiting fixes. - val authMarks = if (pendingOnAuthRequired) filters.keys.associateWith { authSuccessMark(it) } else emptyMap() + val authMarks = if (pendingOnAuthRequired) authSuccessMarks(filters.keys) else emptyMap() val collected = mutableListOf>() // One conflated token, armed by a delay()-based watchdog, ends the fetch // at the wall-clock ceiling. delay() keeps the cap on the coroutine clock diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt index 5b9ee79bad..e1faf08b42 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt @@ -24,7 +24,7 @@ import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient import com.vitorpamplona.quartz.nip01Core.relay.client.auth.AuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.DEFAULT_AUTH_GRACE_MS -import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMark +import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMarks import com.vitorpamplona.quartz.nip01Core.relay.client.auth.awaitAuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.hasAuthResponder import com.vitorpamplona.quartz.nip01Core.relay.client.reqs.SubscriptionListener @@ -160,7 +160,7 @@ suspend fun INostrClient.fetchFirst( } // Read before the REQ goes out — see [awaitAuthOutcome]'s `since`. - val authMarks = if (pendingOnAuthRequired) filters.keys.associateWith { authSuccessMark(it) } else emptyMap() + val authMarks = if (pendingOnAuthRequired) authSuccessMarks(filters.keys) else emptyMap() var result: Event? = null try { diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt index 20eb72ec8e..0d464e3c55 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt @@ -68,6 +68,30 @@ fun INostrClient.hasAuthResponder(): Boolean = authResponders().isNotEmpty() */ fun INostrClient.authSuccessMark(relay: NormalizedRelayUrl): Int = authResponders().successMark(relay) +/** + * The marks for a whole fan-out, as a map to be read with `marks[relay] ?: 0`. + * + * Only relays that have ALREADY authenticated get an entry: a relay whose mark is zero is + * indistinguishable from an absent one, and at fan-out sizes where this matters (a router + * sweeping thousands of urls per fetch) nearly every relay is at zero when the REQ goes out. + * Building the full map instead would allocate one entry per relay on every fetch, auth-gated + * or not, to record a value the lookup already defaults to. + * + * Returns an empty map when nothing answers AUTH here, so a caller pays nothing at all. + */ +fun INostrClient.authSuccessMarks(relays: Collection): Map { + val responders = authResponders() + if (responders.isEmpty()) return emptyMap() + var marks: MutableMap? = null + for (relay in relays) { + val mark = responders.successMark(relay) + if (mark != 0) { + (marks ?: HashMap().also { marks = it })[relay] = mark + } + } + return marks ?: emptyMap() +} + /** * Suspends until this relay's NIP-42 challenge resolves one way or the other, for a * caller whose REQ (or COUNT) just came back `CLOSED auth-required:`. diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt index daaed8a1cd..90e62f2991 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt @@ -80,7 +80,7 @@ interface IAuthStatus { * Shared so the default [authStateFlow] getter allocates nothing — it is read on * every fetch that touches an auth-gated relay. */ - val NO_AUTH_STATE: StateFlow> = MutableStateFlow(persistentMapOf()) + val NO_AUTH_STATE: StateFlow> = MutableStateFlow(persistentMapOf()).asStateFlow() } } @@ -228,7 +228,15 @@ class RelayAuthenticator( // re-hit an external (NIP-55) signer for every ledger-ALLOW account. Skip while an AUTH is // still in flight — the OK of the one we already sent runs [checkAuthResults] → syncFilters, // which re-drives the refused REQ; if it's still refused, that fresh CLOSED re-auths then. - if (!status.hasFinishedAllAuths()) return + // + // BOTH halves of "in flight" have to be checked. [hasFinishedAllAuths] only knows about AUTHs + // already SENT, and a signature is not instantaneous: the relay challenges at connect and + // refuses the first REQ before the signature comes back, so during that window the watcher is + // empty, this guard used to pass, and a second signing pass started for the same challenge — + // a second user-facing prompt on a NIP-55/NIP-46 signer, and two threads racing + // [RelayAuthStatus.saveAuthSubmission]'s check-then-put. See + // RelayAuthenticatorReauthOnClosedTest.aSlowSignatureIsNotSignedTwiceForOneChallenge. + if (!status.hasFinishedAllAuths() || status.isSigning()) return val challenge = status.lastChallenge() ?: return authenticate(relay, challenge, interactive = false) } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt index e8ae39933b..e4da2deaa8 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt @@ -36,6 +36,7 @@ import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.runBlocking import kotlin.test.Test import kotlin.test.assertEquals @@ -270,4 +271,62 @@ class RelayAuthenticatorReauthOnClosedTest { assertTrue(relay.sent.isEmpty(), "Without a stored challenge there is nothing to re-auth with") assertFalse(relay.sent.any { it is AuthCmd }) } + + /** + * A signature is not instantaneous, and the coalescing guard used to be blind to that. + * + * The relay challenges at connect and refuses the first REQ with `auth-required:` before + * the signature comes back — the ordinary case, not a corner one. [reauthenticateIfAuthRequired] + * checks [RelayAuthStatus.hasFinishedAllAuths], which only knows about AUTHs already SENT, so + * while the first signature is still being produced the watcher is empty, the guard passes, + * and a second signing pass starts for the very same challenge. + * + * That is the thing the guard's own comment says it exists to prevent — "Re-signing on each + * would re-hit an external (NIP-55) signer" — and for a NIP-55 or NIP-46 signer the whole gap + * is a user-facing prompt, so the cost is a SECOND prompt for one connection. It also puts two + * threads into [RelayAuthStatus.saveAuthSubmission]'s non-atomic check-then-put at once, which + * can let both AUTHs onto the wire. + * + * Runs on a real dispatcher with a signer that takes time; the sibling tests use + * [Dispatchers.Unconfined], which completes the signature inline and hides this entirely. + */ + @Test + fun aSlowSignatureIsNotSignedTwiceForOneChallenge() = + runBlocking { + val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val signer = NostrSignerInternal(KeyPair()) + val signCalls = + java.util.concurrent.atomic + .AtomicInteger(0) + + val client = CapturingClient() + RelayAuthenticator( + client = client, + scope = scope, + signWithAllLoggedInUsers = { _, template, _ -> + signCalls.incrementAndGet() + // Stands in for the signer round-trip — a NIP-55 prompt, a bunker hop. + kotlinx.coroutines.delay(300) + listOf(signer.sign(template)) + }, + ) + val listener = client.captured ?: error("RelayAuthenticator did not register a listener") + val relay = FakeRelayClient(NormalizedRelayUrl("wss://relay.example/")) + + listener.onConnecting(relay) + listener.onIncomingMessage(relay, "", AuthMessage("chal-1")) + // The REQ we already had open is refused while that signature is still being produced. + listener.onIncomingMessage( + relay, + "", + ClosedMessage("sub", MachineReadablePrefix.AUTH_REQUIRED.format("authenticate first")), + ) + + // Let both signing passes finish, whether one or two were started. + kotlinx.coroutines.delay(1_000) + + assertEquals(1, signCalls.get(), "one challenge must cost exactly one signature, not one per refusal") + assertEquals(1, authedPubKeys(relay).size, "and exactly one AUTH reaches the wire") + scope.cancel() + } } diff --git a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt index bc955fb98c..90c11a0a07 100644 --- a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt +++ b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt @@ -171,7 +171,10 @@ class Nip42AuthGatedFetchTest { } assertEquals(0, collected.size) - assertTrue(doneOut[relay]?.startsWith("closed:") == true, "was ${doneOut[relay]}") + // Named even though we never waited: what the relay said does not depend on whether + // anyone was there to answer it, so `authRefusedRelays()` sees a no-responder client + // exactly as it sees a declining one. + assertEquals(setOf(relay), doneOut.authRefusedRelays(), "was ${doneOut[relay]}") assertTrue(elapsed.inWholeMilliseconds < 1_000, "no waiting when nobody can answer; was $elapsed") } } @@ -200,9 +203,10 @@ class Nip42AuthGatedFetchTest { doneOut = doneOut, ) { _, _ -> true } - assertTrue( - doneOut[relay]?.startsWith("closed:") == true, - "opting out must end the relay on the CLOSED itself; was ${doneOut[relay]}", + assertEquals( + setOf(relay), + doneOut.authRefusedRelays(), + "opting out ends the relay on the CLOSED itself, but still names the wall; was ${doneOut[relay]}", ) } }