diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCache.kt index d3cfd355c9..09c4d10423 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCache.kt @@ -25,6 +25,7 @@ import com.vitorpamplona.quartz.nip47WalletConnect.Nip47WalletConnect import com.vitorpamplona.quartz.nip47WalletConnect.events.NwcInfoEvent import com.vitorpamplona.quartz.utils.TimeUtils import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch @@ -37,16 +38,23 @@ import java.util.concurrent.ConcurrentHashMap * supported RPC methods, and whether it emits notifications. * * Entries expire after [ttlSeconds] (default 2 days) so a wallet that later - * changes its advertised capabilities is eventually re-checked. Reads never block - * on the network: + * changes its advertised capabilities is eventually re-checked. Four entry + * points, in increasing order of how much they will wait: * * - [current] returns whatever is cached (possibly stale, possibly null) with no - * side effect — for the payment hot path. + * side effect and never blocks — for callers that can act on "don't know". * - [refreshIfStale] triggers a background fetch when the entry is missing or * expired, and returns immediately — call it right before using a wallet so a * stale entry self-heals without holding up the transaction. - * - [getFresh] is the suspending variant for callers that can await (e.g. the - * notification watcher deciding whether to open a subscription). + * - [currentOrFetch] waits only when nothing at all is cached, and returns a + * stale entry as-is — for callers where "don't know" and "no" are different + * answers, such as NIP-44 negotiation. + * - [getFresh] waits whenever the entry is missing *or* expired, for a caller + * that must not act on a stale answer. No production caller needs that today. + * + * Every fetching path funnels through one request per wallet, so the startup + * warm-up, a payment waiting on a cold cache and the notification watcher join + * the same call rather than racing each other. * * A completed fetch — including a definitive "wallet published no info event" * (null) — is cached with a timestamp. A *failed* fetch (network error/timeout) @@ -65,7 +73,11 @@ class NwcInfoCache( ) private val cache = ConcurrentHashMap() - private val inFlight = ConcurrentHashMap.newKeySet() + + // Fetches in progress, keyed like [cache]. Every path that fetches goes through + // [fetchOnce], so the startup warm-up, a payment waiting on a cold cache and the + // notification watcher all join one request per wallet instead of racing. + private val inFlight = ConcurrentHashMap>() private fun isFresh(entry: Entry): Boolean = now() - entry.fetchedAt < ttlSeconds @@ -80,15 +92,9 @@ class NwcInfoCache( fun refreshIfStale(uri: Nip47WalletConnect.Nip47URINorm) { val entry = cache[uri.pubKeyHex] if (entry != null && isFresh(entry)) return - if (!inFlight.add(uri.pubKeyHex)) return + if (inFlight.containsKey(uri.pubKeyHex)) return - scope.launch(Dispatchers.IO) { - try { - fetchAndStore(uri) - } finally { - inFlight.remove(uri.pubKeyHex) - } - } + scope.launch(Dispatchers.IO) { fetchOnce(uri) } } /** @@ -99,7 +105,56 @@ class NwcInfoCache( suspend fun getFresh(uri: Nip47WalletConnect.Nip47URINorm): NwcInfoEvent? { val entry = cache[uri.pubKeyHex] if (entry != null && isFresh(entry)) return entry.info - return fetchAndStore(uri) + return fetchOnce(uri) + } + + /** + * Returns whatever is cached, waiting for a fetch only when there is nothing + * cached at all. + * + * This is the encryption-negotiation entry point. [current] answers "what does + * this wallet advertise" from memory, but on a cold cache it answers null, and + * a null there is indistinguishable from "no NIP-44" — so the caller silently + * downgrades to NIP-04 on the first transaction after every app start. Waiting + * once, only when nothing is known, removes that. + * + * A stale entry is returned as-is without waiting: it still says which + * encryption the wallet advertises, and [refreshIfStale] self-heals it in the + * background for next time. + */ + suspend fun currentOrFetch(uri: Nip47WalletConnect.Nip47URINorm): NwcInfoEvent? { + val entry = cache[uri.pubKeyHex] + if (entry != null) { + refreshIfStale(uri) + return entry.info + } + return fetchOnce(uri) + } + + /** + * Runs [fetchAndStore] for [uri] exactly once, however many callers ask at + * once; the rest await that one result. + * + * Cancellation reaches only the caller that owns the fetch — awaiters are + * separate coroutines that are still alive, and get the last cached value (or + * null) rather than being cancelled along with it. + */ + private suspend fun fetchOnce(uri: Nip47WalletConnect.Nip47URINorm): NwcInfoEvent? { + val key = uri.pubKeyHex + val ours = CompletableDeferred() + // putIfAbsent returns the previous entry, so a non-null result means + // someone else already owns this wallet's fetch. + inFlight.putIfAbsent(key, ours)?.let { return it.await() } + + var info: NwcInfoEvent? = null + try { + info = fetchAndStore(uri) + } finally { + // Non-suspending, so awaiters are released even if we are cancelled. + inFlight.remove(key, ours) + ours.complete(info) + } + return info } private suspend fun fetchAndStore(uri: Nip47WalletConnect.Nip47URINorm): NwcInfoEvent? { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcSignerState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcSignerState.kt index 61e6f41178..c6e9af96cf 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcSignerState.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcSignerState.kt @@ -136,17 +136,21 @@ class NwcSignerState( } /** - * Non-blocking read of the negotiated encryption preference for a wallet. - * NIP-47 says a client "should always prefer nip44 if supported by the wallet - * service". Returns true only when the cached info event advertises `nip44_v2`; - * otherwise NIP-04 (the legacy default). Also nudges a background refresh so a - * stale/expired entry self-heals for the next transaction without blocking this - * one. + * The negotiated encryption preference for a wallet. NIP-47 says a client + * "should always prefer nip44 if supported by the wallet service", so a false + * here has to mean "the wallet does not offer NIP-44" — not "we have not asked + * yet". + * + * That distinction is why this waits. The info cache is per-account and held in + * memory only, so it starts empty on every app launch, and reading it without + * waiting made the first transaction to each wallet after every launch fall + * back to NIP-04 even against a wallet advertising `nip44_v2`. Only a cold + * cache waits: a stale entry still says what the wallet advertises and is used + * as-is while it refreshes in the background. */ - private fun prefersNip44(uri: Nip47WalletConnect.Nip47URINorm?): Boolean { + private suspend fun prefersNip44(uri: Nip47WalletConnect.Nip47URINorm?): Boolean { uri ?: return false - infoCache?.refreshIfStale(uri) - return infoCache?.current(uri)?.encryptionSchemes()?.any { it.equals("nip44_v2", ignoreCase = true) } ?: false + return infoCache?.currentOrFetch(uri)?.encryptionSchemes()?.any { it.equals("nip44_v2", ignoreCase = true) } ?: false } fun hasWalletConnectSetup(): Boolean = settings.nwcWallets.value.isNotEmpty() diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCacheTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCacheTest.kt index c74f7e02f2..8a6a7f45a4 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCacheTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCacheTest.kt @@ -23,13 +23,19 @@ package com.vitorpamplona.amethyst.model.nip47WalletConnect import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer import com.vitorpamplona.quartz.nip47WalletConnect.Nip47WalletConnect import com.vitorpamplona.quartz.nip47WalletConnect.events.NwcInfoEvent +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test +import java.util.concurrent.atomic.AtomicInteger class NwcInfoCacheTest { private var clock = 1_000L @@ -142,4 +148,110 @@ class NwcInfoCacheTest { cache.getFresh(uri("wallet1")) assertNotNull(cache.current(uri("wallet1"))) } + + // --- one fetch per wallet, however many callers ask at once --- + + /** + * A fetch that reports when it has started and then blocks until released, so + * a test can put callers into a known interleaving without sleeping: caller + * one is provably inside the fetch before the others arrive. + */ + private class GatedFetch( + private val result: NwcInfoEvent?, + ) { + val calls = AtomicInteger(0) + val started = CompletableDeferred() + val release = CompletableDeferred() + + suspend fun fetch(uri: Nip47WalletConnect.Nip47URINorm): NwcInfoEvent? { + calls.incrementAndGet() + started.complete(Unit) + release.await() + return result + } + } + + @Test + fun `concurrent getFresh callers share one fetch`() = + runBlocking { + val fetcher = GatedFetch(info("pay_invoice")) + val cache = NwcInfoCache(fetch = fetcher::fetch, scope = scope, ttlSeconds = 100, now = { clock }) + + val results = + coroutineScope { + val first = async(Dispatchers.IO) { cache.getFresh(uri("wallet1")) } + fetcher.started.await() + val rest = (1..4).map { async(Dispatchers.IO) { cache.getFresh(uri("wallet1")) } } + fetcher.release.complete(Unit) + (listOf(first) + rest).awaitAll() + } + + assertEquals(1, fetcher.calls.get()) + assertTrue(results.all { it != null }) + } + + @Test + fun `getFresh joins a fetch already started by refreshIfStale`() = + runBlocking { + val fetcher = GatedFetch(info("pay_invoice")) + val cache = NwcInfoCache(fetch = fetcher::fetch, scope = scope, ttlSeconds = 100, now = { clock }) + + cache.refreshIfStale(uri("wallet1")) + fetcher.started.await() + + val joined = + coroutineScope { + val caller = async(Dispatchers.IO) { cache.getFresh(uri("wallet1")) } + fetcher.release.complete(Unit) + caller.await() + } + + assertEquals(1, fetcher.calls.get()) + assertNotNull(joined) + } + + // --- currentOrFetch: wait only when there is nothing cached at all --- + + @Test + fun `currentOrFetch waits for the first fetch when nothing is cached`() = + runBlocking { + val fetcher = GatedFetch(info("pay_invoice")) + val cache = NwcInfoCache(fetch = fetcher::fetch, scope = scope, ttlSeconds = 100, now = { clock }) + + val result = + coroutineScope { + val caller = async(Dispatchers.IO) { cache.currentOrFetch(uri("wallet1")) } + fetcher.started.await() + fetcher.release.complete(Unit) + caller.await() + } + + assertEquals(1, fetcher.calls.get()) + assertNotNull("a cold cache must resolve before the caller decides on encryption", result) + } + + @Test + fun `currentOrFetch returns a stale entry without waiting`() = + runBlocking { + var calls = 0 + val cache = + NwcInfoCache( + fetch = { + calls++ + info("pay_invoice") + }, + scope = scope, + ttlSeconds = 100, + now = { clock }, + ) + + assertNotNull(cache.currentOrFetch(uri("wallet1"))) + assertEquals(1, calls) + + // Past the TTL the entry is stale, but it still answers the question it + // is asked (which encryption the wallet advertises), so the caller must + // get it back without a round-trip. The background refresh self-heals. + clock += 1_000 + assertNotNull(cache.currentOrFetch(uri("wallet1"))) + } }