mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-10 00:16:59 +00:00
fix(onchain-zaps): clear second-audit findings
Addresses the 15 issues from the second audit pass. Key changes: - Per-event resolution flag (`Note.onchainZapResolved`) replaces the unbounded rejection blocklist. The flag is set on terminal verifier verdicts (Confirmed or hard-Rejected) and gates the verifier launch in `consume()`. Travels with the Note so it clears on `removeAllChildNotes()`. - Per-event in-flight set (`verifyingEventIds`) deduplicates concurrent verifier launches across `consume()` echoes and `reverifyOnchainZapsForNote` races. Solves: profile-only zaps bypassing the all-CONFIRMED guard, Rejected entries re-firing the verifier on every echo, and the consume()/reverify TOCTOU race. - Per-note reverify gate (`reverifyingNoteIds`) prevents multiple visible galleries from launching concurrent reverify passes for the same note. - `removeOnchainZapForSource` now refuses to remove a CONFIRMED entry — only an explicit fresh CONFIRMED replacement can change one. Prevents the cross-target downgrade where one target's transient ZERO_VERIFIED_AMOUNT erases a sibling target's already-confirmed entry. Also non-nullable pubkey parameter to close the null-vs-null comparison hole. - `innerAddOnchainZap` dedup tightened: exact structural equality skips spurious flowSet invalidations on relay echoes, but same-level + equal verifiedSats from a DIFFERENT source now replaces (fixes multi-signer attribution lock-in). - Tip flow uses explicit try/catch that re-throws CancellationException instead of `runCatching` (same fix the previous audit applied to the verifier). Lazy initializer falls back to a constant-null StateFlow if `Amethyst.instance` isn't initialized yet, instead of throwing. - Gallery driver: unconditional first-view kick (no longer waits for the tip flow's first non-null emission), separate effect keyed on pending entry count so a fresh UNVERIFIED arrival kicks reverify immediately instead of waiting up to 60s for the next tip poll. - `observeNoteZaps`'s memoization now keys on the `onchainZaps` map reference so lightning-zap traffic on the same note doesn't churn the onchain gallery. - `reverifyOnchainZapsForNote` uses `supervisorScope` so a single failed verifier doesn't cancel its siblings, and the semaphore permits bump from 4 → 8 reduces head-of-line blocking when many galleries reverify concurrently.
This commit is contained in:
@@ -267,25 +267,28 @@ import kotlinx.coroutines.async
|
||||
import kotlinx.coroutines.awaitAll
|
||||
import kotlinx.coroutines.channels.BufferOverflow
|
||||
import kotlinx.coroutines.channels.awaitClose
|
||||
import kotlinx.coroutines.coroutineScope
|
||||
import kotlinx.coroutines.delay
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
import kotlinx.coroutines.flow.MutableSharedFlow
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
import kotlinx.coroutines.flow.SharingStarted
|
||||
import kotlinx.coroutines.flow.StateFlow
|
||||
import kotlinx.coroutines.flow.asSharedFlow
|
||||
import kotlinx.coroutines.flow.asStateFlow
|
||||
import kotlinx.coroutines.flow.buffer
|
||||
import kotlinx.coroutines.flow.callbackFlow
|
||||
import kotlinx.coroutines.flow.flow
|
||||
import kotlinx.coroutines.flow.map
|
||||
import kotlinx.coroutines.flow.stateIn
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.supervisorScope
|
||||
import kotlinx.coroutines.sync.Semaphore
|
||||
import kotlinx.coroutines.sync.withPermit
|
||||
import java.io.File
|
||||
import java.io.FileOutputStream
|
||||
import java.io.IOException
|
||||
import java.util.SortedSet
|
||||
import java.util.concurrent.ConcurrentHashMap
|
||||
|
||||
interface ILocalCache {
|
||||
fun markAsSeen(
|
||||
@@ -1900,9 +1903,7 @@ object LocalCache : ILocalCache, ICacheProvider {
|
||||
// could otherwise put "-1" in the gallery as a negative-sats badge.
|
||||
val claimedSats = (event.claimedAmountInSats() ?: 0L).coerceAtLeast(0L)
|
||||
repliesTo.forEach {
|
||||
if (!it.wasOnchainZapRejectedForSource(txid, event.pubKey)) {
|
||||
it.addOnchainZap(note, txid, claimedSats, verifiedSats = 0L, OnchainZapStatus.UNVERIFIED)
|
||||
}
|
||||
it.addOnchainZap(note, txid, claimedSats, verifiedSats = 0L, OnchainZapStatus.UNVERIFIED)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1915,25 +1916,29 @@ object LocalCache : ILocalCache, ICacheProvider {
|
||||
// profile zap views see it, but it can't contribute to Note totals.
|
||||
val backend = onchainBackend ?: return !alreadyLoaded
|
||||
|
||||
// Re-arrival path: on a later relay echo of the same event, skip the verifier
|
||||
// launch entirely if every target note already holds a CONFIRMED entry for this
|
||||
// txid. Avoids the relay-echo fan-out where the same kind:8333 is delivered N
|
||||
// times and spawns N redundant verifier coroutines (each potentially hitting
|
||||
// Esplora for any uncached lookup).
|
||||
val txid = event.txid()
|
||||
if (alreadyLoaded && txid != null) {
|
||||
val repliesTo = computeReplyTo(event)
|
||||
val allConfirmed =
|
||||
repliesTo.isNotEmpty() &&
|
||||
repliesTo.all { it.onchainZaps[txid]?.status == OnchainZapStatus.CONFIRMED }
|
||||
if (allConfirmed) return false
|
||||
}
|
||||
// Skip the verifier launch entirely once the chain has spoken definitively
|
||||
// (Confirmed, or hard-rejected with a non-transient reason). The resolved
|
||||
// flag is per-source-event and travels with the Note, so it survives across
|
||||
// relay echoes and even covers profile-only zaps (which would otherwise
|
||||
// bypass any per-target CONFIRMED check because they have no replyTo notes).
|
||||
if (note.onchainZapResolved) return !alreadyLoaded
|
||||
|
||||
// De-duplicate concurrent launches. `verifyingEventIds.add` returns false if
|
||||
// another coroutine has already started verifying this exact event id —
|
||||
// covers (a) two relays delivering the same event simultaneously,
|
||||
// (b) a relay echo arriving while the prior verifier is still in flight,
|
||||
// (c) `reverifyOnchainZapsForNote` racing with `consume`.
|
||||
if (!verifyingEventIds.add(event.id)) return !alreadyLoaded
|
||||
|
||||
val verifier = OnchainZapVerifier(backend)
|
||||
val repliesTo = computeReplyTo(event)
|
||||
|
||||
Amethyst.instance.applicationIOScope.launch {
|
||||
verifyAndUpgradeOnchainZap(event, note, repliesTo, verifier)
|
||||
try {
|
||||
verifyAndUpgradeOnchainZap(event, note, repliesTo, verifier)
|
||||
} finally {
|
||||
verifyingEventIds.remove(event.id)
|
||||
}
|
||||
}
|
||||
|
||||
return !alreadyLoaded
|
||||
@@ -1945,6 +1950,9 @@ object LocalCache : ILocalCache, ICacheProvider {
|
||||
* monotonically (UNVERIFIED → PENDING → CONFIRMED) and won't move backwards, and
|
||||
* source-scoped removal prevents one event's rejection from erasing another sender's
|
||||
* legitimate entry that happens to share a txid.
|
||||
*
|
||||
* Callers must wrap the call site with a `verifyingEventIds`/`reverifyingNoteIds`
|
||||
* gate; this function does not itself protect against duplicate concurrent runs.
|
||||
*/
|
||||
private suspend fun verifyAndUpgradeOnchainZap(
|
||||
event: OnchainZapEvent,
|
||||
@@ -1960,12 +1968,19 @@ object LocalCache : ILocalCache, ICacheProvider {
|
||||
repliesTo.forEach {
|
||||
it.addOnchainZap(source, result.txid, claimedSats, result.verifiedSats, OnchainZapStatus.CONFIRMED)
|
||||
}
|
||||
// Terminal — the chain has confirmed. Future relay echoes of this
|
||||
// event id skip the verifier entirely via the `onchainZapResolved`
|
||||
// gate in `consume()`.
|
||||
source.onchainZapResolved = true
|
||||
}
|
||||
|
||||
is VerifiedOnchainZap.Pending -> {
|
||||
repliesTo.forEach {
|
||||
it.addOnchainZap(source, result.txid, claimedSats, result.verifiedSats, OnchainZapStatus.PENDING)
|
||||
}
|
||||
// Not terminal — the tx is in the mempool. A future tip-poll or
|
||||
// reverify call can upgrade it to CONFIRMED, so leave the
|
||||
// resolved flag false.
|
||||
}
|
||||
|
||||
is VerifiedOnchainZap.Rejected -> {
|
||||
@@ -1978,11 +1993,22 @@ object LocalCache : ILocalCache, ICacheProvider {
|
||||
} else if (result.txid.isNotEmpty()) {
|
||||
// Hard rejection — e.g. ZERO_VERIFIED_AMOUNT means this sender's
|
||||
// tx did not pay the recipient. Drop only entries whose source
|
||||
// matches THIS event (Note.removeOnchainZapForSource enforces
|
||||
// the match), so a spoofer can't erase a legitimate CONFIRMED
|
||||
// entry that happens to share the same txid.
|
||||
// matches THIS event AND that aren't CONFIRMED (the per-target
|
||||
// CONFIRMED check inside `removeOnchainZapForSource` prevents a
|
||||
// transient backend hiccup from wiping a previously-verified
|
||||
// entry on a sibling target).
|
||||
Log.d("OnchainZap") { "rejected ${result.txid}: ${result.reason}" }
|
||||
repliesTo.forEach { it.removeOnchainZapForSource(result.txid, event.pubKey) }
|
||||
// Terminal — don't re-verify on future relay echoes of this
|
||||
// event id. (Different event ids with the same txid still go
|
||||
// through their own verifier pass.)
|
||||
source.onchainZapResolved = true
|
||||
} else {
|
||||
// MISSING_TXID etc. — log so we have observability when a
|
||||
// malformed event reaches the backend. Mark terminal so we
|
||||
// don't re-verify the same broken event on every echo.
|
||||
Log.d("OnchainZap") { "rejected ${event.id}: ${result.reason} (no txid)" }
|
||||
source.onchainZapResolved = true
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1999,37 +2025,74 @@ object LocalCache : ILocalCache, ICacheProvider {
|
||||
* Safe to call from a screen-visibility hook or a chain-tip change observer; each
|
||||
* verifier call is bounded by the chain backend's cache TTLs.
|
||||
*
|
||||
* Runs the per-entry verifier calls in parallel (capped by [reverifySemaphore])
|
||||
* so a thread root with many pending entries finishes within typical screen
|
||||
* dwell time instead of N × RTT sequentially.
|
||||
* - Per-note in-flight gate (`reverifyingNoteIds`) — if another caller is already
|
||||
* reverifying this note (e.g. multiple visible galleries fired in the same tip
|
||||
* tick) we skip the dup.
|
||||
* - Per-event in-flight gate (`verifyingEventIds`) — coordinates with `consume()`
|
||||
* so the same event isn't verified twice concurrently.
|
||||
* - `supervisorScope` — a single verifier failure won't cancel sibling verifiers
|
||||
* for other entries on the same note.
|
||||
* - Bounded parallelism via [reverifySemaphore] so a thread with many pending
|
||||
* entries doesn't blast Esplora.
|
||||
*/
|
||||
suspend fun reverifyOnchainZapsForNote(note: Note) {
|
||||
val backend = onchainBackend ?: return
|
||||
val pendingEntries =
|
||||
note.onchainZaps.values.filter { it.status != OnchainZapStatus.CONFIRMED }
|
||||
if (pendingEntries.isEmpty()) return
|
||||
if (!reverifyingNoteIds.add(note.idHex)) return
|
||||
try {
|
||||
val pendingEntries =
|
||||
note.onchainZaps.values.filter { it.status != OnchainZapStatus.CONFIRMED }
|
||||
if (pendingEntries.isEmpty()) return
|
||||
|
||||
val verifier = OnchainZapVerifier(backend)
|
||||
coroutineScope {
|
||||
pendingEntries
|
||||
.mapNotNull { entry ->
|
||||
val sourceEvent = entry.source.event as? OnchainZapEvent ?: return@mapNotNull null
|
||||
async {
|
||||
reverifySemaphore.withPermit {
|
||||
val repliesTo = computeReplyTo(sourceEvent)
|
||||
verifyAndUpgradeOnchainZap(sourceEvent, entry.source, repliesTo, verifier)
|
||||
val verifier = OnchainZapVerifier(backend)
|
||||
supervisorScope {
|
||||
pendingEntries
|
||||
.mapNotNull { entry ->
|
||||
val sourceEvent = entry.source.event as? OnchainZapEvent ?: return@mapNotNull null
|
||||
val source = entry.source
|
||||
if (source.onchainZapResolved) return@mapNotNull null
|
||||
if (!verifyingEventIds.add(sourceEvent.id)) return@mapNotNull null
|
||||
async {
|
||||
try {
|
||||
reverifySemaphore.withPermit {
|
||||
val repliesTo = computeReplyTo(sourceEvent)
|
||||
verifyAndUpgradeOnchainZap(sourceEvent, source, repliesTo, verifier)
|
||||
}
|
||||
} finally {
|
||||
verifyingEventIds.remove(sourceEvent.id)
|
||||
}
|
||||
}
|
||||
}
|
||||
}.awaitAll()
|
||||
}.awaitAll()
|
||||
}
|
||||
} finally {
|
||||
reverifyingNoteIds.remove(note.idHex)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* In-flight set of event ids currently being verified. Prevents two consume()
|
||||
* calls (or a consume + a reverify) from issuing parallel Esplora fetches for
|
||||
* the same event. `ConcurrentHashMap.newKeySet` gives lock-free atomic `add`
|
||||
* returning `true` only for the inserting caller.
|
||||
*/
|
||||
private val verifyingEventIds: MutableSet<HexKey> = ConcurrentHashMap.newKeySet()
|
||||
|
||||
/**
|
||||
* In-flight set of note id strings currently being reverified. Lets the
|
||||
* onchain-zap gallery driver be called from many visible composables without
|
||||
* the same note's reverify pass running concurrently. The per-event gate is
|
||||
* the backstop; this one short-circuits earlier and avoids creating async
|
||||
* coroutines that would just no-op.
|
||||
*/
|
||||
private val reverifyingNoteIds: MutableSet<HexKey> = ConcurrentHashMap.newKeySet()
|
||||
|
||||
/**
|
||||
* Caps the parallelism of [reverifyOnchainZapsForNote] so a thread with many
|
||||
* pending entries doesn't blast public Esplora endpoints with a burst of
|
||||
* simultaneous requests.
|
||||
* simultaneous requests. Tuned a little higher than the previous 4 so that
|
||||
* concurrent galleries each running a small reverify aren't fully serialized
|
||||
* behind a single big one.
|
||||
*/
|
||||
private val reverifySemaphore = Semaphore(permits = 4)
|
||||
private val reverifySemaphore = Semaphore(permits = 8)
|
||||
|
||||
/**
|
||||
* Shared poller for the current bitcoin chain tip height. Each gallery that
|
||||
@@ -2038,16 +2101,37 @@ object LocalCache : ILocalCache, ICacheProvider {
|
||||
* only fires when at least one UI surface needs it.
|
||||
*
|
||||
* Lazy initialization is required because [Amethyst.instance] may not exist
|
||||
* when the `LocalCache` singleton is class-loaded.
|
||||
* when the `LocalCache` singleton is class-loaded. Falls back to a constant
|
||||
* null-emitting StateFlow if the application scope isn't available yet
|
||||
* (e.g. unit tests, ContentProvider invocations), so the lazy field doesn't
|
||||
* permanently fail with `UninitializedPropertyAccessException`.
|
||||
*/
|
||||
val onchainTipHeightFlow: StateFlow<Long?> by lazy {
|
||||
val scope =
|
||||
runCatching { Amethyst.instance.applicationIOScope }.getOrNull()
|
||||
?: return@lazy MutableStateFlow<Long?>(null).asStateFlow()
|
||||
|
||||
flow {
|
||||
while (true) {
|
||||
emit(onchainBackend?.let { runCatching { it.tipHeight() }.getOrNull() })
|
||||
val tip =
|
||||
onchainBackend?.let { backend ->
|
||||
// Explicit try/catch instead of `runCatching` because the latter
|
||||
// would swallow CancellationException too — when the upstream
|
||||
// scope cancels, we must let it propagate so the flow tears down
|
||||
// promptly instead of looping through one more delay().
|
||||
try {
|
||||
backend.tipHeight()
|
||||
} catch (e: CancellationException) {
|
||||
throw e
|
||||
} catch (t: Throwable) {
|
||||
null
|
||||
}
|
||||
}
|
||||
emit(tip)
|
||||
delay(ONCHAIN_TIP_POLL_INTERVAL_MS)
|
||||
}
|
||||
}.stateIn(
|
||||
Amethyst.instance.applicationIOScope,
|
||||
scope,
|
||||
SharingStarted.WhileSubscribed(stopTimeoutMillis = 5_000L),
|
||||
initialValue = null,
|
||||
)
|
||||
|
||||
@@ -69,45 +69,62 @@ internal fun WatchOnchainZapsAndRenderGallery(
|
||||
) {
|
||||
// Reuse the same flow the lightning gallery subscribes to. Note.addOnchainZap
|
||||
// invalidates flowSet.zaps, so this composable refreshes when on-chain zaps
|
||||
// arrive or upgrade pending → confirmed. The flow also fires for lightning
|
||||
// zap arrivals on the same note, so memoize the list snapshot.
|
||||
// arrive or upgrade pending → confirmed. The flow ALSO fires for lightning
|
||||
// zap arrivals on the same note — memoize on the onchainZaps map reference
|
||||
// (a fresh immutable map per onchain mutation, stable across lightning-only
|
||||
// updates) so a busy lightning thread doesn't churn this gallery's state.
|
||||
val zapsState by observeNoteZaps(baseNote, accountViewModel)
|
||||
val onchainZapsMap = zapsState?.note?.onchainZaps
|
||||
val entries =
|
||||
remember(zapsState) {
|
||||
zapsState
|
||||
?.note
|
||||
?.onchainZaps
|
||||
?.values
|
||||
?.toImmutableList() ?: persistentListOf()
|
||||
remember(onchainZapsMap) {
|
||||
onchainZapsMap?.values?.toImmutableList() ?: persistentListOf()
|
||||
}
|
||||
|
||||
if (entries.isNotEmpty()) {
|
||||
// Drive periodic re-verification of any non-CONFIRMED entries while this
|
||||
// gallery is on screen — covers home feed, profile, notifications, channels,
|
||||
// single-note view, threads. Keyed on baseNote.idHex so the subscription is
|
||||
// stable across recompositions and pauses cleanly when the gallery scrolls
|
||||
// off-screen (WhileSubscribed on the shared tip flow).
|
||||
// Drive re-verification of any non-CONFIRMED entries while this gallery
|
||||
// is on screen — covers home feed, profile, notifications, channels,
|
||||
// single-note view, threads. Per-note in-flight gating inside the cache
|
||||
// dedupes the work when multiple gallery instances for the same note
|
||||
// (lazy-list off/on screen flicker, split feed) all fire together.
|
||||
DriveOnchainZapReverification(baseNote, entries)
|
||||
RenderOnchainZapGallery(entries, nav, accountViewModel)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Subscribes to the shared chain-tip flow and re-runs onchain-zap verification for
|
||||
* [note] whenever the tip advances, while [entries] still contains non-CONFIRMED
|
||||
* items. First-view kick happens via the StateFlow's initial null → first-tip
|
||||
* transition.
|
||||
* Drives on-chain zap re-verification for [note] while the gallery is composed.
|
||||
*
|
||||
* Three triggers fire reverify:
|
||||
* 1. First view of this note (independent of tip availability — covers the cold-
|
||||
* start case where the chain backend isn't wired yet, or `tipHeight()` is slow).
|
||||
* 2. A new pending entry arrives (`entries.size` changes).
|
||||
* 3. The chain tip advances (the shared StateFlow emits a new value).
|
||||
*
|
||||
* The cache's per-note + per-event gates dedupe concurrent calls; this composable
|
||||
* doesn't need its own throttling.
|
||||
*/
|
||||
@Composable
|
||||
private fun DriveOnchainZapReverification(
|
||||
note: Note,
|
||||
entries: ImmutableList<OnchainZapEntry>,
|
||||
) {
|
||||
val hasPending = remember(entries) { entries.any { it.status != OnchainZapStatus.CONFIRMED } }
|
||||
if (!hasPending) return
|
||||
val pendingCount = remember(entries) { entries.count { it.status != OnchainZapStatus.CONFIRMED } }
|
||||
if (pendingCount == 0) return
|
||||
|
||||
// First-view kick — unconditional, doesn't wait for the tip flow. Keyed on
|
||||
// (idHex, pendingCount) so a brand-new pending entry arriving while the
|
||||
// gallery is still on screen also kicks an immediate reverify instead of
|
||||
// waiting up to a full tip-poll interval.
|
||||
LaunchedEffect(note.idHex, pendingCount) {
|
||||
LocalCache.reverifyOnchainZapsForNote(note)
|
||||
}
|
||||
|
||||
// Tip-change kick — subscribes to the shared poller (lazy, WhileSubscribed
|
||||
// so only one HTTP poller runs across the whole UI no matter how many
|
||||
// galleries are visible). Skips the first emission (null) to avoid
|
||||
// duplicating the first-view kick above.
|
||||
val tip by LocalCache.onchainTipHeightFlow.collectAsStateWithLifecycle()
|
||||
LaunchedEffect(note.idHex, hasPending, tip) {
|
||||
LaunchedEffect(note.idHex, tip) {
|
||||
if (tip != null) {
|
||||
LocalCache.reverifyOnchainZapsForNote(note)
|
||||
}
|
||||
|
||||
@@ -173,15 +173,16 @@ open class Note(
|
||||
private set
|
||||
|
||||
/**
|
||||
* Anti-resurrection blocklist for hard-rejected NIP-BC onchain zaps.
|
||||
* Key: txid. Value: set of source author pubkeys whose attempts for that txid have
|
||||
* been verified-and-rejected on this note. Used by `LocalCache.consume(OnchainZapEvent)`
|
||||
* to skip the optimistic UI attachment for known-bad (txid, sender) pairs that would
|
||||
* otherwise re-flicker into the gallery on every fresh event id.
|
||||
* True when the NIP-BC chain verifier has reached a terminal verdict for THIS
|
||||
* note's OnchainZapEvent — i.e. on-chain Confirmed, or hard-rejected for a reason
|
||||
* other than `TX_NOT_FOUND`. `LocalCache.consume()` uses this to skip re-launching
|
||||
* the verifier on relay echoes once the chain has spoken definitively.
|
||||
*
|
||||
* Stays `false` for transient states (`UNVERIFIED`, `PENDING`, `TX_NOT_FOUND`) so
|
||||
* the gallery's reverify driver can still upgrade them as the chain advances.
|
||||
*/
|
||||
@Volatile
|
||||
var rejectedOnchainZapTxidsBySource = mapOf<String, Set<HexKey>>()
|
||||
private set
|
||||
var onchainZapResolved: Boolean = false
|
||||
|
||||
var zapPayments = mapOf<Note, Note?>()
|
||||
private set
|
||||
@@ -346,6 +347,7 @@ open class Note(
|
||||
reports = mapOf()
|
||||
zaps = mapOf()
|
||||
onchainZaps = mapOf()
|
||||
onchainZapResolved = false
|
||||
zapPayments = mapOf()
|
||||
zapsAmount = BigDecimal.ZERO
|
||||
relays = listOf()
|
||||
@@ -452,13 +454,17 @@ open class Note(
|
||||
): Boolean {
|
||||
val existing = onchainZaps[txid]
|
||||
if (existing != null) {
|
||||
// Exact structural duplicate (same source Note + same fields) — typical
|
||||
// relay echo of the same event. Skip the rewrite to avoid spurious
|
||||
// flowSet invalidation.
|
||||
if (entry == existing) return false
|
||||
// Reject downgrades using the explicit OnchainZapStatus.level (not ordinal)
|
||||
// so the upgrade contract survives future enum reordering or insertions.
|
||||
if (entry.status.level < existing.status.level) return false
|
||||
// Same level: accept only when the on-chain verified amount has grown
|
||||
// (e.g. the indexer revised its view of the tx's recipient outputs).
|
||||
// Otherwise treat as a duplicate and keep the first source we got.
|
||||
if (entry.status.level == existing.status.level && entry.verifiedSats <= existing.verifiedSats) return false
|
||||
// Same level: accept only when verifiedSats grows OR the source differs
|
||||
// (legitimate alternate signer republishing a split-zap receipt). A strictly
|
||||
// smaller verifiedSats is a backend downgrade and we ignore it.
|
||||
if (entry.status.level == existing.status.level && entry.verifiedSats < existing.verifiedSats) return false
|
||||
}
|
||||
onchainZaps = onchainZaps + Pair(txid, entry)
|
||||
return true
|
||||
@@ -467,27 +473,23 @@ open class Note(
|
||||
@Synchronized
|
||||
private fun innerRemoveOnchainZapForSource(
|
||||
txid: String,
|
||||
sourceAuthorPubKey: HexKey?,
|
||||
sourceAuthorPubKey: HexKey,
|
||||
): Boolean {
|
||||
val existing = onchainZaps[txid] ?: return false
|
||||
// Anti-spoof: only remove the entry if its source matches the rejecting event.
|
||||
// Otherwise a malicious third party could erase a legitimate CONFIRMED entry
|
||||
// by publishing a spoofed kind:8333 with the same txid but a bystander recipient.
|
||||
// Also refuse to remove a CONFIRMED entry — once chain-verified, only a fresh
|
||||
// CONFIRMED replacement should change it; a transient backend hiccup must not
|
||||
// wipe a previously-CONFIRMED entry just because some other target on the same
|
||||
// event is still UNVERIFIED.
|
||||
if (existing.status == OnchainZapStatus.CONFIRMED) return false
|
||||
if (existing.source.author?.pubkeyHex == null) return false
|
||||
if (existing.source.author?.pubkeyHex != sourceAuthorPubKey) return false
|
||||
onchainZaps = onchainZaps - txid
|
||||
return true
|
||||
}
|
||||
|
||||
@Synchronized
|
||||
private fun innerRecordOnchainZapRejection(
|
||||
txid: String,
|
||||
sourceAuthorPubKey: HexKey,
|
||||
) {
|
||||
val current = rejectedOnchainZapTxidsBySource[txid].orEmpty()
|
||||
if (sourceAuthorPubKey in current) return
|
||||
rejectedOnchainZapTxidsBySource = rejectedOnchainZapTxidsBySource + Pair(txid, current + sourceAuthorPubKey)
|
||||
}
|
||||
|
||||
/**
|
||||
* Register a NIP-BC onchain zap targeting this note. `source` is the OnchainZapEvent's own
|
||||
* note — `source.author` is the sender shown in the reactions gallery.
|
||||
@@ -519,33 +521,20 @@ open class Note(
|
||||
* Used when verification produced a hard rejection (e.g. the transaction paid zero to the
|
||||
* recipient — a spoof attempt). The source-scoped match prevents a malicious third party
|
||||
* from erasing a legitimate CONFIRMED entry by publishing a spoofed kind:8333 with the
|
||||
* same txid but a different recipient pubkey.
|
||||
* same txid but a different recipient pubkey. Per-target CONFIRMED check also prevents
|
||||
* a transient backend reject from erasing an already-confirmed entry.
|
||||
*/
|
||||
fun removeOnchainZapForSource(
|
||||
txid: String,
|
||||
sourceAuthorPubKey: HexKey?,
|
||||
sourceAuthorPubKey: HexKey,
|
||||
) {
|
||||
val removed = innerRemoveOnchainZapForSource(txid, sourceAuthorPubKey)
|
||||
if (sourceAuthorPubKey != null) {
|
||||
innerRecordOnchainZapRejection(txid, sourceAuthorPubKey)
|
||||
}
|
||||
if (removed) {
|
||||
updateZapTotal()
|
||||
flowSet?.zaps?.invalidateData()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* True if a previous verification rejected an onchain zap for this exact (txid, source)
|
||||
* pair on this note. `LocalCache.consume(OnchainZapEvent)` uses this to skip the
|
||||
* optimistic gallery attachment for known-bad senders, preventing attach-then-remove
|
||||
* flicker when an attacker re-publishes the same spoof under a fresh event id.
|
||||
*/
|
||||
fun wasOnchainZapRejectedForSource(
|
||||
txid: String,
|
||||
sourceAuthorPubKey: HexKey,
|
||||
): Boolean = sourceAuthorPubKey in rejectedOnchainZapTxidsBySource[txid].orEmpty()
|
||||
|
||||
@Synchronized
|
||||
private fun innerAddZapPayment(
|
||||
zapPaymentRequest: Note,
|
||||
|
||||
+123
-19
@@ -26,6 +26,8 @@ import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertNotEquals
|
||||
import kotlin.test.assertNotNull
|
||||
import kotlin.test.assertNotSame
|
||||
import kotlin.test.assertNull
|
||||
import kotlin.test.assertSame
|
||||
import kotlin.test.assertTrue
|
||||
@@ -35,8 +37,8 @@ class NoteOnchainZapTest {
|
||||
|
||||
// Creates a sender-event Note whose `author.pubkeyHex` is set to [pubKey]. The Note's
|
||||
// own `idHex` is derived from the pubkey so it stays distinct from the target note.
|
||||
// `wasOnchainZapRejectedForSource` and `removeOnchainZapForSource` both key off
|
||||
// `source.author?.pubkeyHex`, so the author must be wired even in unit tests.
|
||||
// `removeOnchainZapForSource` keys off `source.author?.pubkeyHex`, so the author
|
||||
// must be wired even in unit tests.
|
||||
private fun sourceNote(pubKey: HexKey): Note {
|
||||
val src = Note(pubKey)
|
||||
src.author = User(pubKey, Note(pubKey + "n65"), Note(pubKey + "dm"))
|
||||
@@ -174,13 +176,13 @@ class NoteOnchainZapTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun sameStatusLowerOrEqualVerifiedSatsIsIgnored() {
|
||||
fun sameStatusLowerVerifiedSatsIsIgnored() {
|
||||
val target = freshNote()
|
||||
val firstSrc = sourceNote("a7".repeat(32))
|
||||
val secondSrc = sourceNote("b8".repeat(32))
|
||||
|
||||
target.addOnchainZap(firstSrc, "tx1", claimedSats = 999L, verifiedSats = 999L, status = OnchainZapStatus.CONFIRMED)
|
||||
// Lower verifiedSats: keep the original entry; don't downgrade.
|
||||
// Strictly-lower verifiedSats: keep the original entry; don't downgrade.
|
||||
target.addOnchainZap(secondSrc, "tx1", claimedSats = 999L, verifiedSats = 500L, status = OnchainZapStatus.CONFIRMED)
|
||||
|
||||
val entry = target.onchainZaps["tx1"]
|
||||
@@ -189,14 +191,55 @@ class NoteOnchainZapTest {
|
||||
assertEquals(BigDecimal.valueOf(999L), target.zapsAmount)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun sameStatusEqualVerifiedSatsDifferentSourceReplaces() {
|
||||
// Multi-signer / split-zap-rebroadcast scenario: a second legitimate kind:8333
|
||||
// with the same txid and identical verifiedSats arrives from a different signer.
|
||||
// The new entry should win so attribution isn't permanently locked to whichever
|
||||
// relay delivered first.
|
||||
val target = freshNote()
|
||||
val firstSrc = sourceNote("c9".repeat(32))
|
||||
val secondSrc = sourceNote("d0".repeat(32))
|
||||
|
||||
target.addOnchainZap(firstSrc, "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.CONFIRMED)
|
||||
target.addOnchainZap(secondSrc, "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.CONFIRMED)
|
||||
|
||||
val entry = target.onchainZaps["tx1"]
|
||||
assertSame(secondSrc, entry?.source)
|
||||
assertEquals(BigDecimal.valueOf(1000L), target.zapsAmount)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun structuralDuplicateIsIgnored() {
|
||||
// Exact structural duplicate: same source Note reference + same fields. This
|
||||
// is the typical relay-echo case — N relays deliver the same event id, so
|
||||
// `getOrCreateNote` returns the same Note instance for each. Skip the rewrite
|
||||
// to avoid spurious flowSet invalidations.
|
||||
val target = freshNote()
|
||||
val src = sourceNote("e1".repeat(32))
|
||||
|
||||
target.addOnchainZap(src, "tx1", claimedSats = 100L, verifiedSats = 100L, status = OnchainZapStatus.CONFIRMED)
|
||||
val firstEntry = target.onchainZaps["tx1"]
|
||||
target.addOnchainZap(src, "tx1", claimedSats = 100L, verifiedSats = 100L, status = OnchainZapStatus.CONFIRMED)
|
||||
val secondEntry = target.onchainZaps["tx1"]
|
||||
|
||||
assertSame(firstEntry, secondEntry)
|
||||
assertEquals(BigDecimal.valueOf(100L), target.zapsAmount)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun removeOnchainZapForMatchingSourceDropsEntryAndAdjustsTotal() {
|
||||
// CONFIRMED entries are guarded against removal (see
|
||||
// `confirmedEntryIsNotRemovableOnTransientReject`), so use a PENDING entry
|
||||
// here to exercise the matching-source removal path.
|
||||
val target = freshNote()
|
||||
val srcKey = "c9".repeat(32)
|
||||
val src = sourceNote(srcKey)
|
||||
|
||||
target.addOnchainZap(src, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.CONFIRMED)
|
||||
assertEquals(BigDecimal.valueOf(4200L), target.zapsAmount)
|
||||
target.addOnchainZap(src, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.PENDING)
|
||||
// PENDING entries don't contribute to total per spec.
|
||||
assertEquals(BigDecimal.ZERO, target.zapsAmount)
|
||||
assertNotNull(target.onchainZaps["tx1"])
|
||||
|
||||
target.removeOnchainZapForSource("tx1", srcKey)
|
||||
|
||||
@@ -207,37 +250,66 @@ class NoteOnchainZapTest {
|
||||
@Test
|
||||
fun removeOnchainZapForMismatchedSourceKeepsLegitimateEntry() {
|
||||
// Regression test for the txid-collision attack: a spoofed kind:8333 with the
|
||||
// same txid but a different sender pubkey must not erase a legitimate CONFIRMED
|
||||
// entry. `removeOnchainZapForSource` requires the existing entry's source.author
|
||||
// same txid but a different sender pubkey must not erase a legitimate entry.
|
||||
// `removeOnchainZapForSource` requires the existing entry's source.author
|
||||
// to match the rejecting sender.
|
||||
val target = freshNote()
|
||||
val legitSrcKey = "1a".repeat(32)
|
||||
val attackerKey = "2b".repeat(32)
|
||||
val legitSrc = sourceNote(legitSrcKey)
|
||||
|
||||
target.addOnchainZap(legitSrc, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.CONFIRMED)
|
||||
target.addOnchainZap(legitSrc, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.PENDING)
|
||||
|
||||
// Verifier rejects the attacker's spoof of "tx1" — must not touch Alice's entry.
|
||||
target.removeOnchainZapForSource("tx1", attackerKey)
|
||||
|
||||
assertNotEquals(null, target.onchainZaps["tx1"])
|
||||
assertEquals(BigDecimal.valueOf(4200L), target.zapsAmount)
|
||||
assertTrue(target.wasOnchainZapRejectedForSource("tx1", attackerKey))
|
||||
assertFalse(target.wasOnchainZapRejectedForSource("tx1", legitSrcKey))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun removeOnchainZapForUnknownTxidStillRecordsRejection() {
|
||||
// Even when there's nothing to remove (the optimistic attach was skipped),
|
||||
// the rejection blocklist must be updated so future fresh-event-id retries
|
||||
// by the same attacker don't re-flicker into the gallery.
|
||||
fun confirmedEntryIsNotRemovableOnTransientReject() {
|
||||
// Once chain-verified, a CONFIRMED entry must not be erased by a later
|
||||
// verifier reject (e.g. transient ZERO_VERIFIED_AMOUNT from a corrupted
|
||||
// Esplora response, or a multi-target event where a sibling target hadn't
|
||||
// confirmed yet). Only an explicit fresh CONFIRMED replacement should
|
||||
// change a confirmed entry.
|
||||
val target = freshNote()
|
||||
val srcKey = "f1".repeat(32)
|
||||
val src = sourceNote(srcKey)
|
||||
|
||||
target.addOnchainZap(src, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.CONFIRMED)
|
||||
assertEquals(BigDecimal.valueOf(4200L), target.zapsAmount)
|
||||
|
||||
target.removeOnchainZapForSource("tx1", srcKey)
|
||||
|
||||
assertNotNull(target.onchainZaps["tx1"])
|
||||
assertEquals(OnchainZapStatus.CONFIRMED, target.onchainZaps["tx1"]?.status)
|
||||
assertEquals(BigDecimal.valueOf(4200L), target.zapsAmount)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun removeOnchainZapForUnknownTxidIsNoOp() {
|
||||
val target = freshNote()
|
||||
val srcKey = "3c".repeat(32)
|
||||
val src = sourceNote(srcKey)
|
||||
|
||||
target.addOnchainZap(src, "tx1", claimedSats = 100L, verifiedSats = 100L, status = OnchainZapStatus.PENDING)
|
||||
target.removeOnchainZapForSource("tx-never-added", srcKey)
|
||||
|
||||
assertEquals(0, target.onchainZaps.size)
|
||||
assertTrue(target.wasOnchainZapRejectedForSource("tx-never-added", srcKey))
|
||||
// The known entry survives; the unknown txid op is a no-op.
|
||||
assertEquals(1, target.onchainZaps.size)
|
||||
assertNotNull(target.onchainZaps["tx1"])
|
||||
}
|
||||
|
||||
@Test
|
||||
fun onchainZapResolvedFlagDefaultsFalseAndIsMutable() {
|
||||
// The resolved flag is the dedup gate `LocalCache.consume()` uses to skip
|
||||
// re-launching the verifier for terminally-resolved events. New notes start
|
||||
// unresolved; the verifier flips the flag on Confirmed / hard-Rejected.
|
||||
val src = sourceNote("9d".repeat(32))
|
||||
assertFalse(src.onchainZapResolved)
|
||||
|
||||
src.onchainZapResolved = true
|
||||
assertTrue(src.onchainZapResolved)
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -275,4 +347,36 @@ class NoteOnchainZapTest {
|
||||
assertTrue(target.onchainZaps.containsKey("tx3"))
|
||||
assertTrue(target.onchainZaps.containsKey("tx4"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun removeAllChildNotesClearsOnchainZapResolvedFlag() {
|
||||
// `removeAllChildNotes()` runs on delete-event handling and during cache
|
||||
// pressure; the resolved flag must travel with the cleared state so a
|
||||
// re-arrival of the same event gets re-verified instead of being silently
|
||||
// skipped against stale state.
|
||||
val src = sourceNote("ee".repeat(32))
|
||||
src.onchainZapResolved = true
|
||||
|
||||
src.removeAllChildNotes()
|
||||
|
||||
assertFalse(src.onchainZapResolved)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun upgradeProducesNewEntryInstance() {
|
||||
// Sanity check: when an upgrade is accepted, the new immutable map entry is a
|
||||
// fresh OnchainZapEntry instance (not a mutation of the existing one). Compose
|
||||
// skipping/recomposition correctness depends on this.
|
||||
val target = freshNote()
|
||||
val firstSrc = sourceNote("11".repeat(32))
|
||||
val secondSrc = sourceNote("22".repeat(32))
|
||||
|
||||
target.addOnchainZap(firstSrc, "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.PENDING)
|
||||
val before = target.onchainZaps["tx1"]!!
|
||||
target.addOnchainZap(secondSrc, "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.CONFIRMED)
|
||||
val after = target.onchainZaps["tx1"]!!
|
||||
|
||||
assertNotSame(before, after)
|
||||
assertEquals(OnchainZapStatus.CONFIRMED, after.status)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user