From 4f3e9fd1cd6e7112611d54182f17d131e08e9517 Mon Sep 17 00:00:00 2001 From: davotoula Date: Thu, 27 Aug 2026 08:25:19 +0200 Subject: [PATCH] fix(nwc): stop downgrading to NIP-04 on a cold info cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NIP-47 says a client "should always prefer nip44 if supported by the wallet service", so prefersNip44() returning false has to mean "the wallet does not offer NIP-44" — not "we have not asked yet". It meant both. NwcInfoCache is per-account and in memory only, so it starts empty on every app launch, and prefersNip44 read it without waiting. The first transaction to each wallet after each launch therefore went out as NIP-04 even against a wallet advertising nip44_v2 — a silent downgrade to deprecated encryption on a payment request. The startup warm-up narrows the window but does not close it: it only covers the default wallet, and it races the user's tap. Add currentOrFetch(), which waits only when nothing at all is cached and returns a stale entry as-is — staleness never caused the downgrade, since a stale entry already says what the wallet advertises, so waiting on it would buy nothing. prefersNip44 becomes suspend and uses it; both call sites were already suspend. Funnel every fetching path through one request per wallet. getFresh() went straight to the network with no deduplication — only the background refresh was guarded, and by a plain key set that could not be awaited. Without this, making the payment path wait would have had it race the startup warm-up and issue a second concurrent fetch for the same wallet. Verified by mutation: reverting currentOrFetch to the old non-waiting read fails the cold-cache tests, and removing the single-flight fails the deduplication tests. The prefersNip44 call site itself is a two-line swap covered by those cache tests — NwcSignerState has no test harness and building one for it was out of proportion to the change. --- .../model/nip47WalletConnect/NwcInfoCache.kt | 85 ++++++++++--- .../nip47WalletConnect/NwcSignerState.kt | 22 ++-- .../nip47WalletConnect/NwcInfoCacheTest.kt | 112 ++++++++++++++++++ 3 files changed, 195 insertions(+), 24 deletions(-) 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"))) + } }