laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-06 11:48:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(nwc): stop downgrading to NIP-04 on a cold info cache

Browse Source 
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.
This commit is contained in:
davotoula
2026-08-28 10:37:19 +02:00
parent b29519e4e6
commit 4f3e9fd1cd
3 changed files with 195 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCache.kt
+70 -15
View File
@@ -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<HexKey, Entry>()
private val inFlight = ConcurrentHashMap.newKeySet<HexKey>()
// 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<HexKey, CompletableDeferred<NwcInfoEvent?>>()
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<NwcInfoEvent?>()
// 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? {
amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcSignerState.kt
+13 -9
View File
@@ -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()
amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip47WalletConnect/NwcInfoCacheTest.kt
+112
View File
@@ -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<Unit>()
val release = CompletableDeferred<Unit>()
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")))
}
}