From dd203a5537f8223afaff284860215b565ad4d39f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 20:07:46 +0000 Subject: [PATCH 1/4] fix(onchain-zaps): attach optimistically and re-verify on tip change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Outgoing onchain zaps never appeared in the sender's thread view because LocalCache.consume(OnchainZapEvent) ran the chain verifier milliseconds after the broadcast — before the backend's indexer had picked up the transaction. The resulting TX_NOT_FOUND rejection skipped addOnchainZap, and the duplicate guard blocked re-verification when the same event later echoed back from relays. Attach kind:8333 entries optimistically as UNVERIFIED with the claimed amount so the sender sees their zap on the thread immediately, then upgrade to PENDING/CONFIRMED as the chain catches up. Hard rejections (zero-paid-to-recipient, missing tags) drop the entry; transient TX_NOT_FOUND keeps it UNVERIFIED for a later retry. ThreadScreen now re-verifies non-confirmed entries on view and again whenever the chain tip advances. --- .../amethyst/model/LocalCache.kt | 143 ++++++++++++----- .../amethyst/ui/note/OnchainZapGallery.kt | 15 +- .../loggedIn/threadview/ThreadScreen.kt | 48 ++++++ .../amethyst/commons/model/Note.kt | 63 +++++--- .../amethyst/commons/model/OnchainZapEntry.kt | 57 +++++-- .../commons/model/NoteOnchainZapTest.kt | 146 +++++++++++++++--- 6 files changed, 379 insertions(+), 93 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt index 2d9671bf6d..62db0432d7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt @@ -26,6 +26,7 @@ import android.util.LruCache import androidx.compose.runtime.Stable import com.vitorpamplona.amethyst.Amethyst import com.vitorpamplona.amethyst.commons.model.Channel +import com.vitorpamplona.amethyst.commons.model.OnchainZapStatus import com.vitorpamplona.amethyst.commons.model.cache.ICacheProvider import com.vitorpamplona.amethyst.commons.model.cache.LargeSoftCache import com.vitorpamplona.amethyst.commons.model.emphChat.EphemeralChatChannel @@ -1848,52 +1849,118 @@ object LocalCache : ILocalCache, ICacheProvider { wasVerified: Boolean, ): Boolean { val note = getOrCreateNote(event.id) - if (note.event != null) return false + val alreadyLoaded = note.event != null - if (!(wasVerified || justVerify(event))) return false + if (!alreadyLoaded) { + if (!(wasVerified || justVerify(event))) return false - // Anti-spoofing: NIP-BC requires rejecting self-zaps. - val recipient = event.recipient() ?: return false - if (event.pubKey.equals(recipient, ignoreCase = true)) return false + // Anti-spoofing: NIP-BC requires rejecting self-zaps. + val recipient = event.recipient() ?: return false + if (event.pubKey.equals(recipient, ignoreCase = true)) return false - val author = getOrCreateUser(event.pubKey) - val repliesTo = computeReplyTo(event) - note.loadEvent(event, author, repliesTo) - refreshNewNoteObservers(note) + val author = getOrCreateUser(event.pubKey) + val repliesTo = computeReplyTo(event) + note.loadEvent(event, author, repliesTo) - // Verification needs a chain backend. Without one (e.g. before Account - // wires its EsploraBackend) the event is still cached so subscriptions - // and profile zap views see it, but it can't contribute to Note totals. - val backend = onchainBackend ?: return true - val verifier = OnchainZapVerifier(backend) - - Amethyst.instance.applicationIOScope.launch { - try { - when (val result = verifier.verify(event)) { - is VerifiedOnchainZap.Confirmed -> { - repliesTo.forEach { - it.addOnchainZap(note, result.txid, result.verifiedSats, confirmed = true) - } - } - - is VerifiedOnchainZap.Pending -> { - repliesTo.forEach { - it.addOnchainZap(note, result.txid, result.verifiedSats, confirmed = false) - } - } - - is VerifiedOnchainZap.Rejected -> { - Log.d("OnchainZap") { - "rejected ${result.txid}: ${result.reason}" - } - } + // Optimistic attachment: surface the zap on the thread immediately, even + // before the chain confirms it. Crucial for the sender's own outgoing zap, + // where consumption happens milliseconds after broadcast and the chain + // backend usually hasn't indexed the tx yet. + val txid = event.txid() + val claimedSats = event.claimedAmountInSats() ?: 0L + if (txid != null) { + repliesTo.forEach { + it.addOnchainZap(note, txid, claimedSats, verifiedSats = 0L, OnchainZapStatus.UNVERIFIED) } - } catch (t: Throwable) { - Log.w("OnchainZap", "verification failed for ${event.id}", t) } + + refreshNewNoteObservers(note) + } else { + // A later arrival of the same event. The note is already cached, but the + // verifier may not have produced a CONFIRMED result yet. Fall through to + // re-run verification — `verifyAndUpgradeOnchainZap` is a no-op once an + // entry on every target note has reached CONFIRMED. } - return true + // Verification needs a chain backend. Without one (e.g. before Account + // wires its EsploraBackend) the event stays cached so subscriptions + // and profile zap views see it, but it can't contribute to Note totals. + val backend = onchainBackend ?: return !alreadyLoaded + val verifier = OnchainZapVerifier(backend) + val repliesTo = computeReplyTo(event) + + Amethyst.instance.applicationIOScope.launch { + verifyAndUpgradeOnchainZap(event, note, repliesTo, verifier) + } + + return !alreadyLoaded + } + + /** + * Run the chain verifier for a single onchain zap event and apply the result to every + * target note. Designed to be safe to call repeatedly — the [Note] entries upgrade + * monotonically (UNVERIFIED → PENDING → CONFIRMED) and won't move backwards. + */ + private suspend fun verifyAndUpgradeOnchainZap( + event: OnchainZapEvent, + source: Note, + repliesTo: List, + verifier: OnchainZapVerifier, + ) { + val txid = event.txid() ?: return + val claimedSats = event.claimedAmountInSats() ?: 0L + try { + when (val result = verifier.verify(event)) { + is VerifiedOnchainZap.Confirmed -> { + repliesTo.forEach { + it.addOnchainZap(source, result.txid, claimedSats, result.verifiedSats, OnchainZapStatus.CONFIRMED) + } + } + + is VerifiedOnchainZap.Pending -> { + repliesTo.forEach { + it.addOnchainZap(source, result.txid, claimedSats, result.verifiedSats, OnchainZapStatus.PENDING) + } + } + + is VerifiedOnchainZap.Rejected -> { + if (result.reason == VerifiedOnchainZap.Rejected.Reason.TX_NOT_FOUND) { + // Transient — the tx may not have propagated to the backend's + // indexer yet. Leave the entry as UNVERIFIED so a later + // reverifyOnchainZapsForNote() call (e.g. on chain tip change) + // can promote it. + Log.d("OnchainZap") { "tx not yet indexed for ${event.id} (${result.txid}); will retry" } + } else { + // Permanent — e.g. ZERO_VERIFIED_AMOUNT means the tx did not + // actually pay the recipient. Drop the optimistic entry so + // spoof attempts disappear from the gallery. + Log.d("OnchainZap") { "rejected ${result.txid}: ${result.reason}" } + repliesTo.forEach { it.removeOnchainZap(result.txid) } + } + } + } + } catch (t: Throwable) { + Log.w("OnchainZap", "verification failed for ${event.id}", t) + } + } + + /** + * Re-run onchain-zap verification for every non-CONFIRMED entry attached to [note]. + * 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. + */ + suspend fun reverifyOnchainZapsForNote(note: Note) { + val backend = onchainBackend ?: return + val pendingEntries = + note.onchainZaps.values.filter { it.status != OnchainZapStatus.CONFIRMED } + if (pendingEntries.isEmpty()) return + + val verifier = OnchainZapVerifier(backend) + pendingEntries.forEach { entry -> + val sourceEvent = entry.source.event as? OnchainZapEvent ?: return@forEach + val repliesTo = computeReplyTo(sourceEvent) + verifyAndUpgradeOnchainZap(sourceEvent, entry.source, repliesTo, verifier) + } } private fun attachZapToLiveActivityChannel( diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt index e799e06b2e..b42f3bdbb4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt @@ -36,6 +36,7 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.draw.alpha import androidx.compose.ui.unit.dp import com.vitorpamplona.amethyst.commons.model.OnchainZapEntry +import com.vitorpamplona.amethyst.commons.model.OnchainZapStatus import com.vitorpamplona.amethyst.model.Note import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.observeNoteZaps import com.vitorpamplona.amethyst.ui.navigation.navs.INav @@ -122,18 +123,20 @@ private fun OnchainZapEntryRow( accountViewModel: AccountViewModel, ) { val user = entry.source.author + val displaySats = entry.displaySats val amountText = - remember(entry.verifiedSats) { - showAmount(BigDecimal.valueOf(entry.verifiedSats)) + remember(displaySats) { + showAmount(BigDecimal.valueOf(displaySats)) } - val avatarAlpha = if (entry.confirmed) 1f else 0.6f + val isConfirmed = entry.status == OnchainZapStatus.CONFIRMED + val avatarAlpha = if (isConfirmed) 1f else 0.6f Box( modifier = Size35Modifier.clickable { onOnchainZapEntryClick(entry, nav) }, contentAlignment = Alignment.BottomCenter, ) { - // Only the avatar dims for pending entries. The amount overlay and clock - // badge stay at full opacity so they remain readable. + // Only the avatar dims for unverified/pending entries. The amount overlay + // and clock badge stay at full opacity so they remain readable. Box(modifier = Modifier.alpha(avatarAlpha)) { WatchUserMetadataAndFollowsAndRenderUserProfilePictureOrDefaultAuthor( user, @@ -143,7 +146,7 @@ private fun OnchainZapEntryRow( CrossfadeToDisplayAmount(amountText) - if (!entry.confirmed) { + if (!isConfirmed) { // TopStart so the badge doesn't collide with the FollowingIcon // that WatchUserMetadataAndFollowsAndRenderUserProfilePicture // paints at TopEnd for followed users. diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt index c301d1e88c..50e054848c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt @@ -22,8 +22,11 @@ package com.vitorpamplona.amethyst.ui.screen.loggedIn.threadview import androidx.compose.material3.Text import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.lifecycle.viewmodel.compose.viewModel import com.vitorpamplona.amethyst.R +import com.vitorpamplona.amethyst.commons.model.Note +import com.vitorpamplona.amethyst.commons.model.OnchainZapStatus import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.EventFinderFilterAssemblerSubscription import com.vitorpamplona.amethyst.ui.components.LoadNote import com.vitorpamplona.amethyst.ui.feeds.WatchLifecycleAndUpdateModel @@ -34,6 +37,7 @@ import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.threadview.dal.ThreadFeedViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.threadview.datasources.ThreadFilterAssemblerSubscription import com.vitorpamplona.amethyst.ui.stringRes +import kotlinx.coroutines.delay @Composable fun ThreadScreen( @@ -56,6 +60,7 @@ fun ThreadScreen( if (it != null) { // this will force loading every post from this thread. EventFinderFilterAssemblerSubscription(it, accountViewModel) + ReverifyOnchainZapsWhileVisible(it, accountViewModel) } } @@ -72,3 +77,46 @@ fun ThreadScreen( ThreadFeedView(noteId, feedViewModel, accountViewModel, nav) } } + +/** + * Drives re-verification of NIP-BC onchain zaps while the thread is on screen. + * + * The chain backend often hasn't indexed a transaction at the moment its kind:8333 + * receipt is first consumed (especially for the user's own outgoing zap, broadcast + * milliseconds earlier). We attach those entries optimistically as UNVERIFIED in + * `LocalCache.consume(OnchainZapEvent)`; this composable re-runs the verifier on + * view and then again whenever the chain tip advances, upgrading entries to + * PENDING/CONFIRMED as the chain catches up. + * + * The polling interval is aligned with [com.vitorpamplona.quartz.nipBCOnchainZaps + * .chain.CachingOnchainBackend]'s tip-height TTL — anything shorter would just hit + * the cache and do no useful work. + */ +@Composable +private fun ReverifyOnchainZapsWhileVisible( + note: Note, + accountViewModel: AccountViewModel, +) { + LaunchedEffect(note) { + val cache = accountViewModel.account.cache + val backend = cache.onchainBackend ?: return@LaunchedEffect + + // First pass on view: covers entries attached optimistically just before navigation. + cache.reverifyOnchainZapsForNote(note) + + var lastTip: Long? = null + while (true) { + val pending = note.onchainZaps.values.any { it.status != OnchainZapStatus.CONFIRMED } + if (!pending) break + + val tip = runCatching { backend.tipHeight() }.getOrNull() + if (tip != null && tip != lastTip) { + lastTip = tip + cache.reverifyOnchainZapsForNote(note) + } + delay(TIP_POLL_INTERVAL_MS) + } + } +} + +private const val TIP_POLL_INTERVAL_MS = 60_000L diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt index a6a7a6d656..1ea4321c66 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt @@ -157,12 +157,12 @@ open class Note( var zapsAmount: BigDecimal = BigDecimal.ZERO /** - * NIP-BC verified onchain zaps targeting this note. - * Key: Bitcoin txid (lowercase 64-char hex). Value: verified entry with the source - * OnchainZapEvent note (so `source.author` identifies the sender), the verified - * satoshis paid to the recipient (NOT the sender-claimed amount), and a confirmed - * flag. Confirmed and pending entries live here together; `updateZapTotal` only - * counts confirmed amounts. + * NIP-BC onchain zaps targeting this note. + * Key: Bitcoin txid (lowercase 64-char hex). Value: entry with the source + * OnchainZapEvent note (so `source.author` identifies the sender), the sender-claimed + * amount, the on-chain verified amount, and a verification status. + * Unverified, pending, and confirmed entries live here together; `updateZapTotal` + * only counts CONFIRMED amounts. */ var onchainZaps = mapOf() private set @@ -436,36 +436,61 @@ open class Note( ): Boolean { val existing = onchainZaps[txid] if (existing != null) { - // Same-state duplicate: keep the first source and amount we got. - if (existing.confirmed == entry.confirmed) return false - // Downgrade confirmed → pending: never accept. States differ here, - // so existing.confirmed alone is sufficient to identify the downgrade. - if (existing.confirmed) return false - // Else: existing pending + incoming confirmed — fall through to upgrade. + // Same-state duplicate: keep the first source/amount/status we got. + if (existing.status == entry.status) return false + // Reject downgrades. The status enum is naturally ordered + // UNVERIFIED < PENDING < CONFIRMED — accept only forward transitions. + if (entry.status.ordinal <= existing.status.ordinal) return false } onchainZaps = onchainZaps + Pair(txid, entry) return true } + @Synchronized + private fun innerRemoveOnchainZap(txid: String): Boolean { + if (!onchainZaps.containsKey(txid)) return false + onchainZaps = onchainZaps - txid + return true + } + /** - * Register a NIP-BC verified onchain zap targeting this note. `verifiedSats` MUST come - * from on-chain verification (sum of outputs paying the recipient's derived Taproot - * address), not the sender-claimed `amount` tag. `source` is the OnchainZapEvent's own + * 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. + * + * Call with [OnchainZapStatus.UNVERIFIED] (and `verifiedSats = 0`) at consumption time + * to attach the zap optimistically so the user immediately sees their outgoing zap + * is processing. Call again with [OnchainZapStatus.PENDING] or [OnchainZapStatus.CONFIRMED] + * (and the verified output sum) once the chain backend confirms it. + * + * `verifiedSats` MUST come from on-chain verification (sum of outputs paying the + * recipient's derived Taproot address), not the sender-claimed `amount` tag. */ fun addOnchainZap( source: Note, txid: String, + claimedSats: Long, verifiedSats: Long, - confirmed: Boolean, + status: OnchainZapStatus, ) { - val inserted = innerAddOnchainZap(txid, OnchainZapEntry(source, verifiedSats, confirmed)) + val inserted = innerAddOnchainZap(txid, OnchainZapEntry(source, claimedSats, verifiedSats, status)) if (inserted) { updateZapTotal() flowSet?.zaps?.invalidateData() } } + /** + * Remove a previously-attached onchain zap entry. Used when verification produced a + * hard rejection (e.g. the transaction paid zero to the recipient — a spoof attempt). + */ + fun removeOnchainZap(txid: String) { + val removed = innerRemoveOnchainZap(txid) + if (removed) { + updateZapTotal() + flowSet?.zaps?.invalidateData() + } + } + @Synchronized private fun innerAddZapPayment( zapPaymentRequest: Note, @@ -679,9 +704,9 @@ open class Note( } // NIP-BC onchain zaps — verified amounts only, confirmed only. - // Pending/unconfirmed entries are tracked but excluded from the total per spec. + // Unverified/pending entries are tracked but excluded from the total per spec. onchainZaps.values.forEach { entry -> - if (entry.confirmed) { + if (entry.status == OnchainZapStatus.CONFIRMED) { sumOfAmounts += BigDecimal.valueOf(entry.verifiedSats) } } diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt index 5f0d8e3a87..6ab652e1b5 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt @@ -23,22 +23,57 @@ package com.vitorpamplona.amethyst.commons.model import androidx.compose.runtime.Stable /** - * Per-(note, txid) verified NIP-BC onchain zap state. + * NIP-BC onchain zap verification state. + * + * The chain backend may not have indexed the transaction at the moment we first + * see the zap event — especially for the sender's own outgoing zaps, where we + * consume the kind:8333 event milliseconds after broadcasting the transaction. + * Tracking the verification status as a state machine lets us attach the zap + * to the thread optimistically and upgrade it as the chain catches up. + */ +enum class OnchainZapStatus { + /** Not yet checked against the chain (or the chain didn't have the tx yet). */ + UNVERIFIED, + + /** Verified against the chain, 0 confirmations (in mempool). */ + PENDING, + + /** Verified against the chain, ≥1 confirmation. */ + CONFIRMED, +} + +/** + * Per-(note, txid) NIP-BC onchain zap state. * * @property source The OnchainZapEvent note that contributed this entry. `source.author` is the - * sender shown in the reactions gallery. When pending → confirmed upgrades - * happen, the upgrading event's note replaces the existing `source`. + * sender shown in the reactions gallery. When an entry is upgraded + * (UNVERIFIED → PENDING/CONFIRMED, PENDING → CONFIRMED), the upgrading + * event's note replaces the existing `source`. + * @property claimedSats Sender-claimed amount from the kind:8333 event's `amount` tag. Shown to + * the user while the zap is UNVERIFIED so they have feedback that + * a zap is processing. * @property verifiedSats Satoshis verified to have paid the recipient's derived Taproot - * address on chain. NEVER the sender-claimed `amount` tag. - * @property confirmed True when the transaction has at least one confirmation. Unconfirmed - * zaps are tracked but excluded from aggregate totals per the NIP-BC - * spec ("Unconfirmed transactions MAY be displayed as pending... - * SHOULD either exclude them from aggregate totals or clearly label - * them as pending"). + * address on chain. Zero while [status] is [OnchainZapStatus.UNVERIFIED]. + * NEVER the sender-claimed `amount` tag. + * @property status See [OnchainZapStatus]. Only [OnchainZapStatus.CONFIRMED] entries are added + * to the note's aggregate zap total. */ @Stable data class OnchainZapEntry( val source: Note, + val claimedSats: Long, val verifiedSats: Long, - val confirmed: Boolean, -) + val status: OnchainZapStatus, +) { + /** + * Amount to display to the user. Once verified we always prefer the on-chain truth; while + * unverified we fall back to the sender-claimed amount so the user has some feedback. + */ + val displaySats: Long + get() = + if (status == OnchainZapStatus.UNVERIFIED) { + claimedSats + } else { + verifiedSats + } +} diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt index b70afc7dca..c907ed3040 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt @@ -24,6 +24,7 @@ import java.math.BigDecimal import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue @@ -32,18 +33,48 @@ class NoteOnchainZapTest { private fun sourceNote(idHex: String) = Note(idHex) + @Test + fun unverifiedEntryIsStoredWithClaimedAmountAndDoesNotAffectTotal() { + val target = freshNote() + val src = sourceNote("a".repeat(64)) + + target.addOnchainZap( + source = src, + txid = "tx1", + claimedSats = 1000L, + verifiedSats = 0L, + status = OnchainZapStatus.UNVERIFIED, + ) + + val entry = target.onchainZaps["tx1"] + assertEquals(1, target.onchainZaps.size) + assertSame(src, entry?.source) + assertEquals(1000L, entry?.claimedSats) + assertEquals(0L, entry?.verifiedSats) + assertEquals(OnchainZapStatus.UNVERIFIED, entry?.status) + assertEquals(1000L, entry?.displaySats) + assertEquals(BigDecimal.ZERO, target.zapsAmount) + } + @Test fun pendingEntryIsStoredWithSourceAndDoesNotAffectTotal() { val target = freshNote() val src = sourceNote("b".repeat(64)) - target.addOnchainZap(source = src, txid = "tx1", verifiedSats = 1000L, confirmed = false) + target.addOnchainZap( + source = src, + txid = "tx1", + claimedSats = 1000L, + verifiedSats = 1000L, + status = OnchainZapStatus.PENDING, + ) val entry = target.onchainZaps["tx1"] assertEquals(1, target.onchainZaps.size) assertSame(src, entry?.source) assertEquals(1000L, entry?.verifiedSats) - assertEquals(false, entry?.confirmed) + assertEquals(OnchainZapStatus.PENDING, entry?.status) + assertEquals(1000L, entry?.displaySats) assertEquals(BigDecimal.ZERO, target.zapsAmount) } @@ -52,24 +83,46 @@ class NoteOnchainZapTest { val target = freshNote() val src = sourceNote("c".repeat(64)) - target.addOnchainZap(source = src, txid = "tx1", verifiedSats = 5000L, confirmed = true) + target.addOnchainZap( + source = src, + txid = "tx1", + claimedSats = 5000L, + verifiedSats = 5000L, + status = OnchainZapStatus.CONFIRMED, + ) - assertEquals(true, target.onchainZaps["tx1"]?.confirmed) + assertEquals(OnchainZapStatus.CONFIRMED, target.onchainZaps["tx1"]?.status) assertEquals(BigDecimal.valueOf(5000L), target.zapsAmount) } + @Test + fun unverifiedThenPendingReplacesSourceAndAmountsAndKeepsTotalAtZero() { + val target = freshNote() + val firstSrc = sourceNote("d".repeat(64)) + val secondSrc = sourceNote("e".repeat(64)) + + target.addOnchainZap(firstSrc, "tx1", claimedSats = 2500L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) + target.addOnchainZap(secondSrc, "tx1", claimedSats = 2500L, verifiedSats = 2500L, status = OnchainZapStatus.PENDING) + + val entry = target.onchainZaps["tx1"] + assertSame(secondSrc, entry?.source) + assertEquals(OnchainZapStatus.PENDING, entry?.status) + assertEquals(2500L, entry?.verifiedSats) + assertEquals(BigDecimal.ZERO, target.zapsAmount) + } + @Test fun pendingThenConfirmedReplacesSourceAndUpdatesTotal() { val target = freshNote() val firstSrc = sourceNote("d".repeat(64)) val secondSrc = sourceNote("e".repeat(64)) - target.addOnchainZap(firstSrc, "tx1", 2500L, confirmed = false) - target.addOnchainZap(secondSrc, "tx1", 2500L, confirmed = true) + target.addOnchainZap(firstSrc, "tx1", claimedSats = 2500L, verifiedSats = 2500L, status = OnchainZapStatus.PENDING) + target.addOnchainZap(secondSrc, "tx1", claimedSats = 2500L, verifiedSats = 2500L, status = OnchainZapStatus.CONFIRMED) val entry = target.onchainZaps["tx1"] assertSame(secondSrc, entry?.source) - assertEquals(true, entry?.confirmed) + assertEquals(OnchainZapStatus.CONFIRMED, entry?.status) assertEquals(BigDecimal.valueOf(2500L), target.zapsAmount) } @@ -79,25 +132,40 @@ class NoteOnchainZapTest { val firstSrc = sourceNote("f".repeat(64)) val secondSrc = sourceNote("0".repeat(64)) - target.addOnchainZap(firstSrc, "tx1", 7500L, confirmed = true) - target.addOnchainZap(secondSrc, "tx1", 7500L, confirmed = false) + target.addOnchainZap(firstSrc, "tx1", claimedSats = 7500L, verifiedSats = 7500L, status = OnchainZapStatus.CONFIRMED) + target.addOnchainZap(secondSrc, "tx1", claimedSats = 7500L, verifiedSats = 7500L, status = OnchainZapStatus.PENDING) val entry = target.onchainZaps["tx1"] assertSame(firstSrc, entry?.source) - assertEquals(true, entry?.confirmed) + assertEquals(OnchainZapStatus.CONFIRMED, entry?.status) assertEquals(BigDecimal.valueOf(7500L), target.zapsAmount) } + @Test + fun pendingThenUnverifiedIsIgnored() { + val target = freshNote() + val firstSrc = sourceNote("7".repeat(64)) + val secondSrc = sourceNote("6".repeat(64)) + + target.addOnchainZap(firstSrc, "tx1", claimedSats = 100L, verifiedSats = 100L, status = OnchainZapStatus.PENDING) + target.addOnchainZap(secondSrc, "tx1", claimedSats = 100L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) + + val entry = target.onchainZaps["tx1"] + assertSame(firstSrc, entry?.source) + assertEquals(OnchainZapStatus.PENDING, entry?.status) + assertEquals(100L, entry?.verifiedSats) + } + @Test fun sameStateDuplicateIsIgnored() { val target = freshNote() val firstSrc = sourceNote("1".repeat(64)) val secondSrc = sourceNote("2".repeat(64)) - target.addOnchainZap(firstSrc, "tx1", 100L, confirmed = true) + target.addOnchainZap(firstSrc, "tx1", claimedSats = 100L, verifiedSats = 100L, status = OnchainZapStatus.CONFIRMED) // Mismatched verifiedSats so we can detect a regression that silently // overwrites the first entry with the second. - target.addOnchainZap(secondSrc, "tx1", 999L, confirmed = true) + target.addOnchainZap(secondSrc, "tx1", claimedSats = 999L, verifiedSats = 999L, status = OnchainZapStatus.CONFIRMED) val entry = target.onchainZaps["tx1"] assertSame(firstSrc, entry?.source) @@ -105,6 +173,32 @@ class NoteOnchainZapTest { assertEquals(BigDecimal.valueOf(100L), target.zapsAmount) } + @Test + fun removeOnchainZapDropsEntryAndAdjustsTotal() { + val target = freshNote() + val src = sourceNote("7".repeat(64)) + + target.addOnchainZap(src, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.CONFIRMED) + assertEquals(BigDecimal.valueOf(4200L), target.zapsAmount) + + target.removeOnchainZap("tx1") + + assertNull(target.onchainZaps["tx1"]) + assertEquals(BigDecimal.ZERO, target.zapsAmount) + } + + @Test + fun removeOnchainZapForUnknownTxidIsNoOp() { + val target = freshNote() + val src = sourceNote("8".repeat(64)) + + target.addOnchainZap(src, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.CONFIRMED) + target.removeOnchainZap("tx-never-added") + + assertEquals(1, target.onchainZaps.size) + assertEquals(BigDecimal.valueOf(4200L), target.zapsAmount) + } + @Test fun hasZapsBoostsOrReactionsReturnsTrueWhenOnlyOnchainZapPresent() { val target = freshNote() @@ -112,17 +206,29 @@ class NoteOnchainZapTest { assertFalse(target.hasZapsBoostsOrReactions()) - target.addOnchainZap(source = src, txid = "tx1", verifiedSats = 6416L, confirmed = true) + target.addOnchainZap( + source = src, + txid = "tx1", + claimedSats = 6416L, + verifiedSats = 6416L, + status = OnchainZapStatus.CONFIRMED, + ) assertTrue(target.hasZapsBoostsOrReactions()) } @Test - fun hasZapsBoostsOrReactionsReturnsTrueWhenOnlyPendingOnchainZapPresent() { + fun hasZapsBoostsOrReactionsReturnsTrueWhenOnlyUnverifiedOnchainZapPresent() { val target = freshNote() val src = sourceNote("8".repeat(64)) - target.addOnchainZap(source = src, txid = "tx1", verifiedSats = 1000L, confirmed = false) + target.addOnchainZap( + source = src, + txid = "tx1", + claimedSats = 1000L, + verifiedSats = 0L, + status = OnchainZapStatus.UNVERIFIED, + ) assertTrue(target.hasZapsBoostsOrReactions()) } @@ -130,12 +236,14 @@ class NoteOnchainZapTest { @Test fun updateZapTotalSumsConfirmedAcrossMultipleTxids() { val target = freshNote() - target.addOnchainZap(sourceNote("3".repeat(64)), "tx1", 1000L, confirmed = true) - target.addOnchainZap(sourceNote("4".repeat(64)), "tx2", 2000L, confirmed = true) - target.addOnchainZap(sourceNote("5".repeat(64)), "tx3", 9999L, confirmed = false) + target.addOnchainZap(sourceNote("3".repeat(64)), "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.CONFIRMED) + target.addOnchainZap(sourceNote("4".repeat(64)), "tx2", claimedSats = 2000L, verifiedSats = 2000L, status = OnchainZapStatus.CONFIRMED) + target.addOnchainZap(sourceNote("5".repeat(64)), "tx3", claimedSats = 9999L, verifiedSats = 9999L, status = OnchainZapStatus.PENDING) + target.addOnchainZap(sourceNote("6".repeat(64)), "tx4", claimedSats = 1234L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) assertEquals(BigDecimal.valueOf(3000L), target.zapsAmount) - assertEquals(3, target.onchainZaps.size) + assertEquals(4, target.onchainZaps.size) assertTrue(target.onchainZaps.containsKey("tx3")) + assertTrue(target.onchainZaps.containsKey("tx4")) } } From 73f1e6ae9c0ec8dcf694bba1594303a36d207631 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 21:34:36 +0000 Subject: [PATCH 2/4] fix(onchain-zaps): harden against spoofing, fix re-verify lifecycle, audit cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the 15 findings from the high-effort code review on top of the optimistic-attach fix. Notable behavior changes: - Per-source removal: `Note.removeOnchainZapForSource(txid, pubkey)` only drops an entry whose source matches, preventing a spoofed kind:8333 with the same txid but a bystander recipient from erasing a legitimate CONFIRMED entry. Rejected (txid, sender) pairs are recorded so a fresh event id from the same attacker no longer re-flickers into the gallery. - Sender-only optimistic attach: only the user's own outgoing zap (relay == null path) gets the optimistic UNVERIFIED entry. Incoming zaps render only after on-chain verification, so an attacker-controlled `amount` tag can't briefly mislead viewers. `claimedSats` is clamped >= 0. - Reverification across every screen: the chain-tip poller moves from the thread screen into `LocalCache.onchainTipHeightFlow` (lazy, shared, WhileSubscribed). The onchain-zap gallery itself drives reverification whenever it composes with non-CONFIRMED entries — covers home feed, notifications, profile, channel and single-note views. The gallery observes the tip flow and the note's zap state, so new arrivals while the gallery is on screen are picked up too. - Verifier fan-out + parallelism: re-arrivals skip the verifier launch when every target note already holds a CONFIRMED entry for the txid. `reverifyOnchainZapsForNote` now runs verifier calls in parallel, capped by a 4-permit semaphore. - Monotonic upgrade based on explicit `OnchainZapStatus.level` instead of `ordinal`, with a unit test locking the order. Same-level entries with a larger `verifiedSats` are accepted so a stale indexer estimate isn't permanent. - Cancellation propagation: `catch (Throwable)` rethrows `CancellationException` in `verifyAndUpgradeOnchainZap` so screen-scoped callers tear down cleanly. - Memory visibility: `Note.onchainZaps` is `@Volatile` since the reverification driver reads it on Main while the IO scope writes. --- .../amethyst/model/LocalCache.kt | 148 ++++++++++++--- .../amethyst/ui/note/OnchainZapGallery.kt | 55 +++++- .../loggedIn/threadview/ThreadScreen.kt | 48 ----- .../amethyst/commons/model/Note.kt | 79 ++++++-- .../amethyst/commons/model/OnchainZapEntry.kt | 36 ++-- .../commons/model/NoteOnchainZapTest.kt | 173 ++++++++++-------- 6 files changed, 352 insertions(+), 187 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt index 62db0432d7..35619d5e96 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt @@ -263,15 +263,25 @@ import com.vitorpamplona.quartz.utils.TimeUtils import com.vitorpamplona.quartz.utils.cache.LargeCache import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers +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.SharingStarted +import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asSharedFlow 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.sync.Semaphore +import kotlinx.coroutines.sync.withPermit import java.io.File import java.io.FileOutputStream import java.io.IOException @@ -286,6 +296,13 @@ interface ILocalCache { } } +/** + * Polling interval for the shared onchain chain-tip flow. Aligned with + * `CachingOnchainBackend.tipHeightTtlSeconds` (60s) — polling faster would + * just hit the cache and do no useful work. + */ +private const val ONCHAIN_TIP_POLL_INTERVAL_MS = 60_000L + object LocalCache : ILocalCache, ICacheProvider { val antiSpam = AntiSpamFilter() @@ -1851,6 +1868,13 @@ object LocalCache : ILocalCache, ICacheProvider { val note = getOrCreateNote(event.id) val alreadyLoaded = note.event != null + // `relay == null` means this event was generated locally by the signed-in user + // and routed through `justConsumeMyOwnEvent` — see `Account.sendOnchainZap` → + // `LocalCache.justConsumeMyOwnEvent`. Only these get the optimistic gallery + // attachment; for incoming zaps from others, the sender-claimed `amount` tag + // is untrusted and could mislead the viewer until the chain verifier responds. + val isOwnEvent = relay == null + if (!alreadyLoaded) { if (!(wasVerified || justVerify(event))) return false @@ -1862,30 +1886,49 @@ object LocalCache : ILocalCache, ICacheProvider { val repliesTo = computeReplyTo(event) note.loadEvent(event, author, repliesTo) - // Optimistic attachment: surface the zap on the thread immediately, even - // before the chain confirms it. Crucial for the sender's own outgoing zap, - // where consumption happens milliseconds after broadcast and the chain - // backend usually hasn't indexed the tx yet. - val txid = event.txid() - val claimedSats = event.claimedAmountInSats() ?: 0L - if (txid != null) { - repliesTo.forEach { - it.addOnchainZap(note, txid, claimedSats, verifiedSats = 0L, OnchainZapStatus.UNVERIFIED) + if (isOwnEvent) { + // Optimistic attachment for the sender's own zap: surface it on the + // thread immediately, before the chain backend has indexed the tx. + // Crucial because `OnchainZapSender.send` consumes the kind:8333 + // milliseconds after broadcasting the tx — the verifier would + // otherwise return TX_NOT_FOUND and the entry would never appear + // until a later re-verification pass. + val txid = event.txid() + if (txid != null) { + // Clamp claimedSats to a non-negative value. `amount` tag parses + // via toLongOrNull() with no sign check, so a malicious sender + // 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) + } + } } } refreshNewNoteObservers(note) - } else { - // A later arrival of the same event. The note is already cached, but the - // verifier may not have produced a CONFIRMED result yet. Fall through to - // re-run verification — `verifyAndUpgradeOnchainZap` is a no-op once an - // entry on every target note has reached CONFIRMED. } - // Verification needs a chain backend. Without one (e.g. before Account - // wires its EsploraBackend) the event stays cached so subscriptions - // and profile zap views see it, but it can't contribute to Note totals. + // Verification needs a chain backend. Without one (e.g. before AppModules + // wires its EsploraBackend) the event stays cached so subscriptions and + // 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 + } + val verifier = OnchainZapVerifier(backend) val repliesTo = computeReplyTo(event) @@ -1899,7 +1942,9 @@ object LocalCache : ILocalCache, ICacheProvider { /** * Run the chain verifier for a single onchain zap event and apply the result to every * target note. Designed to be safe to call repeatedly — the [Note] entries upgrade - * monotonically (UNVERIFIED → PENDING → CONFIRMED) and won't move backwards. + * 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. */ private suspend fun verifyAndUpgradeOnchainZap( event: OnchainZapEvent, @@ -1907,8 +1952,8 @@ object LocalCache : ILocalCache, ICacheProvider { repliesTo: List, verifier: OnchainZapVerifier, ) { - val txid = event.txid() ?: return - val claimedSats = event.claimedAmountInSats() ?: 0L + // Clamp claimedSats to non-negative; see consume() for rationale. + val claimedSats = (event.claimedAmountInSats() ?: 0L).coerceAtLeast(0L) try { when (val result = verifier.verify(event)) { is VerifiedOnchainZap.Confirmed -> { @@ -1930,16 +1975,21 @@ object LocalCache : ILocalCache, ICacheProvider { // reverifyOnchainZapsForNote() call (e.g. on chain tip change) // can promote it. Log.d("OnchainZap") { "tx not yet indexed for ${event.id} (${result.txid}); will retry" } - } else { - // Permanent — e.g. ZERO_VERIFIED_AMOUNT means the tx did not - // actually pay the recipient. Drop the optimistic entry so - // spoof attempts disappear from the gallery. + } 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. Log.d("OnchainZap") { "rejected ${result.txid}: ${result.reason}" } - repliesTo.forEach { it.removeOnchainZap(result.txid) } + repliesTo.forEach { it.removeOnchainZapForSource(result.txid, event.pubKey) } } } } } catch (t: Throwable) { + // Never swallow cancellation — it must propagate so screen-scoped callers + // (the gallery's reverification driver) can tear down cleanly. + if (t is CancellationException) throw t Log.w("OnchainZap", "verification failed for ${event.id}", t) } } @@ -1948,6 +1998,10 @@ object LocalCache : ILocalCache, ICacheProvider { * Re-run onchain-zap verification for every non-CONFIRMED entry attached to [note]. * 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. */ suspend fun reverifyOnchainZapsForNote(note: Note) { val backend = onchainBackend ?: return @@ -1956,13 +2010,49 @@ object LocalCache : ILocalCache, ICacheProvider { if (pendingEntries.isEmpty()) return val verifier = OnchainZapVerifier(backend) - pendingEntries.forEach { entry -> - val sourceEvent = entry.source.event as? OnchainZapEvent ?: return@forEach - val repliesTo = computeReplyTo(sourceEvent) - verifyAndUpgradeOnchainZap(sourceEvent, entry.source, repliesTo, verifier) + 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) + } + } + }.awaitAll() } } + /** + * Caps the parallelism of [reverifyOnchainZapsForNote] so a thread with many + * pending entries doesn't blast public Esplora endpoints with a burst of + * simultaneous requests. + */ + private val reverifySemaphore = Semaphore(permits = 4) + + /** + * Shared poller for the current bitcoin chain tip height. Each gallery that + * holds non-CONFIRMED onchain zaps subscribes to this flow and re-verifies its + * entries whenever the tip advances. Lazy + `WhileSubscribed` so the HTTP call + * 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. + */ + val onchainTipHeightFlow: StateFlow by lazy { + flow { + while (true) { + emit(onchainBackend?.let { runCatching { it.tipHeight() }.getOrNull() }) + delay(ONCHAIN_TIP_POLL_INTERVAL_MS) + } + }.stateIn( + Amethyst.instance.applicationIOScope, + SharingStarted.WhileSubscribed(stopTimeoutMillis = 5_000L), + initialValue = null, + ) + } + private fun attachZapToLiveActivityChannel( event: LnZapEvent, note: Note, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt index b42f3bdbb4..8726ccba70 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt @@ -29,14 +29,17 @@ import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.size import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.alpha import androidx.compose.ui.unit.dp +import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.commons.model.OnchainZapEntry import com.vitorpamplona.amethyst.commons.model.OnchainZapStatus +import com.vitorpamplona.amethyst.model.LocalCache import com.vitorpamplona.amethyst.model.Note import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.observeNoteZaps import com.vitorpamplona.amethyst.ui.navigation.navs.INav @@ -79,10 +82,38 @@ internal fun WatchOnchainZapsAndRenderGallery( } 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). + 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. + */ +@Composable +private fun DriveOnchainZapReverification( + note: Note, + entries: ImmutableList, +) { + val hasPending = remember(entries) { entries.any { it.status != OnchainZapStatus.CONFIRMED } } + if (!hasPending) return + + val tip by LocalCache.onchainTipHeightFlow.collectAsStateWithLifecycle() + LaunchedEffect(note.idHex, hasPending, tip) { + if (tip != null) { + LocalCache.reverifyOnchainZapsForNote(note) + } + } +} + @Composable private fun RenderOnchainZapGallery( entries: ImmutableList, @@ -123,14 +154,24 @@ private fun OnchainZapEntryRow( accountViewModel: AccountViewModel, ) { val user = entry.source.author - val displaySats = entry.displaySats - val amountText = - remember(displaySats) { - showAmount(BigDecimal.valueOf(displaySats)) - } val isConfirmed = entry.status == OnchainZapStatus.CONFIRMED val avatarAlpha = if (isConfirmed) 1f else 0.6f + // Anti-spoof: `claimedSats` comes from the kind:8333 `amount` tag, which is + // attacker-controlled for incoming zaps. Only show the claimed amount for the + // signed-in user's own outgoing zap (where the user knows what they sent) — + // otherwise show the on-chain verified amount, or nothing while still unverified. + val displaySats = + when { + isConfirmed || entry.status == OnchainZapStatus.PENDING -> entry.verifiedSats + user != null && accountViewModel.isLoggedUser(user.pubkeyHex) -> entry.claimedSats + else -> 0L + } + val amountText = + remember(displaySats) { + if (displaySats > 0L) showAmount(BigDecimal.valueOf(displaySats)) else "" + } + Box( modifier = Size35Modifier.clickable { onOnchainZapEntryClick(entry, nav) }, contentAlignment = Alignment.BottomCenter, @@ -144,7 +185,9 @@ private fun OnchainZapEntryRow( ) } - CrossfadeToDisplayAmount(amountText) + if (amountText.isNotEmpty()) { + CrossfadeToDisplayAmount(amountText) + } if (!isConfirmed) { // TopStart so the badge doesn't collide with the FollowingIcon diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt index 50e054848c..c301d1e88c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/threadview/ThreadScreen.kt @@ -22,11 +22,8 @@ package com.vitorpamplona.amethyst.ui.screen.loggedIn.threadview import androidx.compose.material3.Text import androidx.compose.runtime.Composable -import androidx.compose.runtime.LaunchedEffect import androidx.lifecycle.viewmodel.compose.viewModel import com.vitorpamplona.amethyst.R -import com.vitorpamplona.amethyst.commons.model.Note -import com.vitorpamplona.amethyst.commons.model.OnchainZapStatus import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.EventFinderFilterAssemblerSubscription import com.vitorpamplona.amethyst.ui.components.LoadNote import com.vitorpamplona.amethyst.ui.feeds.WatchLifecycleAndUpdateModel @@ -37,7 +34,6 @@ import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.threadview.dal.ThreadFeedViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.threadview.datasources.ThreadFilterAssemblerSubscription import com.vitorpamplona.amethyst.ui.stringRes -import kotlinx.coroutines.delay @Composable fun ThreadScreen( @@ -60,7 +56,6 @@ fun ThreadScreen( if (it != null) { // this will force loading every post from this thread. EventFinderFilterAssemblerSubscription(it, accountViewModel) - ReverifyOnchainZapsWhileVisible(it, accountViewModel) } } @@ -77,46 +72,3 @@ fun ThreadScreen( ThreadFeedView(noteId, feedViewModel, accountViewModel, nav) } } - -/** - * Drives re-verification of NIP-BC onchain zaps while the thread is on screen. - * - * The chain backend often hasn't indexed a transaction at the moment its kind:8333 - * receipt is first consumed (especially for the user's own outgoing zap, broadcast - * milliseconds earlier). We attach those entries optimistically as UNVERIFIED in - * `LocalCache.consume(OnchainZapEvent)`; this composable re-runs the verifier on - * view and then again whenever the chain tip advances, upgrading entries to - * PENDING/CONFIRMED as the chain catches up. - * - * The polling interval is aligned with [com.vitorpamplona.quartz.nipBCOnchainZaps - * .chain.CachingOnchainBackend]'s tip-height TTL — anything shorter would just hit - * the cache and do no useful work. - */ -@Composable -private fun ReverifyOnchainZapsWhileVisible( - note: Note, - accountViewModel: AccountViewModel, -) { - LaunchedEffect(note) { - val cache = accountViewModel.account.cache - val backend = cache.onchainBackend ?: return@LaunchedEffect - - // First pass on view: covers entries attached optimistically just before navigation. - cache.reverifyOnchainZapsForNote(note) - - var lastTip: Long? = null - while (true) { - val pending = note.onchainZaps.values.any { it.status != OnchainZapStatus.CONFIRMED } - if (!pending) break - - val tip = runCatching { backend.tipHeight() }.getOrNull() - if (tip != null && tip != lastTip) { - lastTip = tip - cache.reverifyOnchainZapsForNote(note) - } - delay(TIP_POLL_INTERVAL_MS) - } - } -} - -private const val TIP_POLL_INTERVAL_MS = 60_000L diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt index 1ea4321c66..04d6316657 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt @@ -163,10 +163,26 @@ open class Note( * amount, the on-chain verified amount, and a verification status. * Unverified, pending, and confirmed entries live here together; `updateZapTotal` * only counts CONFIRMED amounts. + * + * `@Volatile` ensures cross-thread visibility: writes happen on `applicationIOScope` + * (inside the @Synchronized inner methods) and reads happen on the Compose Main + * thread (gallery recomposition + the reverification driver's `any { … }` check). */ + @Volatile var onchainZaps = mapOf() 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. + */ + @Volatile + var rejectedOnchainZapTxidsBySource = mapOf>() + private set + var zapPayments = mapOf() private set @@ -436,29 +452,48 @@ open class Note( ): Boolean { val existing = onchainZaps[txid] if (existing != null) { - // Same-state duplicate: keep the first source/amount/status we got. - if (existing.status == entry.status) return false - // Reject downgrades. The status enum is naturally ordered - // UNVERIFIED < PENDING < CONFIRMED — accept only forward transitions. - if (entry.status.ordinal <= existing.status.ordinal) 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 } onchainZaps = onchainZaps + Pair(txid, entry) return true } @Synchronized - private fun innerRemoveOnchainZap(txid: String): Boolean { - if (!onchainZaps.containsKey(txid)) return false + private fun innerRemoveOnchainZapForSource( + txid: String, + 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. + 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. * * Call with [OnchainZapStatus.UNVERIFIED] (and `verifiedSats = 0`) at consumption time - * to attach the zap optimistically so the user immediately sees their outgoing zap + * to attach the zap optimistically so the user immediately sees their own outgoing zap * is processing. Call again with [OnchainZapStatus.PENDING] or [OnchainZapStatus.CONFIRMED] * (and the verified output sum) once the chain backend confirms it. * @@ -480,17 +515,37 @@ open class Note( } /** - * Remove a previously-attached onchain zap entry. Used when verification produced a - * hard rejection (e.g. the transaction paid zero to the recipient — a spoof attempt). + * Remove a previously-attached onchain zap entry whose source matches [sourceAuthorPubKey]. + * 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. */ - fun removeOnchainZap(txid: String) { - val removed = innerRemoveOnchainZap(txid) + fun removeOnchainZapForSource( + txid: String, + 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, diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt index 6ab652e1b5..dded73ec19 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/OnchainZapEntry.kt @@ -30,16 +30,22 @@ import androidx.compose.runtime.Stable * consume the kind:8333 event milliseconds after broadcasting the transaction. * Tracking the verification status as a state machine lets us attach the zap * to the thread optimistically and upgrade it as the chain catches up. + * + * [level] establishes the monotonic upgrade order independently of declaration + * order. The `Note.addOnchainZap` upgrade guard compares [level] (not + * `ordinal`), so future contributors can safely reorder or insert states. */ -enum class OnchainZapStatus { +enum class OnchainZapStatus( + val level: Int, +) { /** Not yet checked against the chain (or the chain didn't have the tx yet). */ - UNVERIFIED, + UNVERIFIED(0), /** Verified against the chain, 0 confirmations (in mempool). */ - PENDING, + PENDING(1), /** Verified against the chain, ≥1 confirmation. */ - CONFIRMED, + CONFIRMED(2), } /** @@ -49,9 +55,11 @@ enum class OnchainZapStatus { * sender shown in the reactions gallery. When an entry is upgraded * (UNVERIFIED → PENDING/CONFIRMED, PENDING → CONFIRMED), the upgrading * event's note replaces the existing `source`. - * @property claimedSats Sender-claimed amount from the kind:8333 event's `amount` tag. Shown to - * the user while the zap is UNVERIFIED so they have feedback that - * a zap is processing. + * @property claimedSats Sender-claimed amount from the kind:8333 event's `amount` tag. This + * value is UNTRUSTED — only display it for the signed-in user's own + * outgoing zaps (where the user knows what they sent). Never render + * it for incoming zaps from other senders, as a spoofed amount tag + * would mislead the viewer. * @property verifiedSats Satoshis verified to have paid the recipient's derived Taproot * address on chain. Zero while [status] is [OnchainZapStatus.UNVERIFIED]. * NEVER the sender-claimed `amount` tag. @@ -64,16 +72,4 @@ data class OnchainZapEntry( val claimedSats: Long, val verifiedSats: Long, val status: OnchainZapStatus, -) { - /** - * Amount to display to the user. Once verified we always prefer the on-chain truth; while - * unverified we fall back to the sender-claimed amount so the user has some feedback. - */ - val displaySats: Long - get() = - if (status == OnchainZapStatus.UNVERIFIED) { - claimedSats - } else { - verifiedSats - } -} +) diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt index c907ed3040..ca62a12f00 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt @@ -20,31 +20,44 @@ */ package com.vitorpamplona.amethyst.commons.model +import com.vitorpamplona.quartz.nip01Core.core.HexKey import java.math.BigDecimal import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNotEquals import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue class NoteOnchainZapTest { - private fun freshNote(idHex: String = "a".repeat(64)) = Note(idHex) + private fun freshNote(idHex: String = "f".repeat(64)) = Note(idHex) - private fun sourceNote(idHex: String) = Note(idHex) + // 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. + private fun sourceNote(pubKey: HexKey): Note { + val src = Note(pubKey) + src.author = User(pubKey, Note(pubKey + "n65"), Note(pubKey + "dm")) + return src + } @Test - fun unverifiedEntryIsStoredWithClaimedAmountAndDoesNotAffectTotal() { + fun enumLevelOrderingIsMonotonic() { + // Lock the documented status ordering. The upgrade guard in `innerAddOnchainZap` + // depends on this — if someone reorders the enum without updating levels, the + // guard silently breaks. This test fails before that happens. + assertTrue(OnchainZapStatus.UNVERIFIED.level < OnchainZapStatus.PENDING.level) + assertTrue(OnchainZapStatus.PENDING.level < OnchainZapStatus.CONFIRMED.level) + } + + @Test + fun unverifiedEntryIsStoredAndDoesNotAffectTotal() { val target = freshNote() val src = sourceNote("a".repeat(64)) - target.addOnchainZap( - source = src, - txid = "tx1", - claimedSats = 1000L, - verifiedSats = 0L, - status = OnchainZapStatus.UNVERIFIED, - ) + target.addOnchainZap(src, "tx1", claimedSats = 1000L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) val entry = target.onchainZaps["tx1"] assertEquals(1, target.onchainZaps.size) @@ -52,7 +65,6 @@ class NoteOnchainZapTest { assertEquals(1000L, entry?.claimedSats) assertEquals(0L, entry?.verifiedSats) assertEquals(OnchainZapStatus.UNVERIFIED, entry?.status) - assertEquals(1000L, entry?.displaySats) assertEquals(BigDecimal.ZERO, target.zapsAmount) } @@ -61,20 +73,13 @@ class NoteOnchainZapTest { val target = freshNote() val src = sourceNote("b".repeat(64)) - target.addOnchainZap( - source = src, - txid = "tx1", - claimedSats = 1000L, - verifiedSats = 1000L, - status = OnchainZapStatus.PENDING, - ) + target.addOnchainZap(src, "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.PENDING) val entry = target.onchainZaps["tx1"] assertEquals(1, target.onchainZaps.size) assertSame(src, entry?.source) assertEquals(1000L, entry?.verifiedSats) assertEquals(OnchainZapStatus.PENDING, entry?.status) - assertEquals(1000L, entry?.displaySats) assertEquals(BigDecimal.ZERO, target.zapsAmount) } @@ -83,36 +88,30 @@ class NoteOnchainZapTest { val target = freshNote() val src = sourceNote("c".repeat(64)) - target.addOnchainZap( - source = src, - txid = "tx1", - claimedSats = 5000L, - verifiedSats = 5000L, - status = OnchainZapStatus.CONFIRMED, - ) + target.addOnchainZap(src, "tx1", claimedSats = 5000L, verifiedSats = 5000L, status = OnchainZapStatus.CONFIRMED) assertEquals(OnchainZapStatus.CONFIRMED, target.onchainZaps["tx1"]?.status) assertEquals(BigDecimal.valueOf(5000L), target.zapsAmount) } @Test - fun unverifiedThenPendingReplacesSourceAndAmountsAndKeepsTotalAtZero() { + fun unverifiedThenPendingUpgradesSourceAmountAndStatus() { val target = freshNote() val firstSrc = sourceNote("d".repeat(64)) val secondSrc = sourceNote("e".repeat(64)) target.addOnchainZap(firstSrc, "tx1", claimedSats = 2500L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) - target.addOnchainZap(secondSrc, "tx1", claimedSats = 2500L, verifiedSats = 2500L, status = OnchainZapStatus.PENDING) + target.addOnchainZap(secondSrc, "tx1", claimedSats = 2500L, verifiedSats = 2400L, status = OnchainZapStatus.PENDING) val entry = target.onchainZaps["tx1"] assertSame(secondSrc, entry?.source) assertEquals(OnchainZapStatus.PENDING, entry?.status) - assertEquals(2500L, entry?.verifiedSats) + assertEquals(2400L, entry?.verifiedSats) assertEquals(BigDecimal.ZERO, target.zapsAmount) } @Test - fun pendingThenConfirmedReplacesSourceAndUpdatesTotal() { + fun pendingThenConfirmedUpgradesSourceAndUpdatesTotal() { val target = freshNote() val firstSrc = sourceNote("d".repeat(64)) val secondSrc = sourceNote("e".repeat(64)) @@ -129,8 +128,8 @@ class NoteOnchainZapTest { @Test fun confirmedThenPendingIsIgnored() { val target = freshNote() - val firstSrc = sourceNote("f".repeat(64)) - val secondSrc = sourceNote("0".repeat(64)) + val firstSrc = sourceNote("a1".repeat(32)) + val secondSrc = sourceNote("b2".repeat(32)) target.addOnchainZap(firstSrc, "tx1", claimedSats = 7500L, verifiedSats = 7500L, status = OnchainZapStatus.CONFIRMED) target.addOnchainZap(secondSrc, "tx1", claimedSats = 7500L, verifiedSats = 7500L, status = OnchainZapStatus.PENDING) @@ -144,8 +143,8 @@ class NoteOnchainZapTest { @Test fun pendingThenUnverifiedIsIgnored() { val target = freshNote() - val firstSrc = sourceNote("7".repeat(64)) - val secondSrc = sourceNote("6".repeat(64)) + val firstSrc = sourceNote("c3".repeat(32)) + val secondSrc = sourceNote("d4".repeat(32)) target.addOnchainZap(firstSrc, "tx1", claimedSats = 100L, verifiedSats = 100L, status = OnchainZapStatus.PENDING) target.addOnchainZap(secondSrc, "tx1", claimedSats = 100L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) @@ -157,62 +156,98 @@ class NoteOnchainZapTest { } @Test - fun sameStateDuplicateIsIgnored() { + fun sameStatusHigherVerifiedSatsReplacesEntry() { + // The indexer can revise its view of the recipient outputs upward across calls + // (delayed mempool propagation, cached partial response). A larger verifiedSats + // for the same status MUST replace the existing entry so the gallery doesn't + // freeze on a stale low estimate. val target = freshNote() - val firstSrc = sourceNote("1".repeat(64)) - val secondSrc = sourceNote("2".repeat(64)) + val firstSrc = sourceNote("e5".repeat(32)) + val secondSrc = sourceNote("f6".repeat(32)) - target.addOnchainZap(firstSrc, "tx1", claimedSats = 100L, verifiedSats = 100L, status = OnchainZapStatus.CONFIRMED) - // Mismatched verifiedSats so we can detect a regression that silently - // overwrites the first entry with the second. - target.addOnchainZap(secondSrc, "tx1", claimedSats = 999L, verifiedSats = 999L, status = OnchainZapStatus.CONFIRMED) + target.addOnchainZap(firstSrc, "tx1", claimedSats = 5000L, verifiedSats = 1000L, status = OnchainZapStatus.PENDING) + target.addOnchainZap(secondSrc, "tx1", claimedSats = 5000L, verifiedSats = 2000L, status = OnchainZapStatus.PENDING) val entry = target.onchainZaps["tx1"] - assertSame(firstSrc, entry?.source) - assertEquals(100L, entry?.verifiedSats) - assertEquals(BigDecimal.valueOf(100L), target.zapsAmount) + assertSame(secondSrc, entry?.source) + assertEquals(2000L, entry?.verifiedSats) } @Test - fun removeOnchainZapDropsEntryAndAdjustsTotal() { + fun sameStatusLowerOrEqualVerifiedSatsIsIgnored() { val target = freshNote() - val src = sourceNote("7".repeat(64)) + 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. + target.addOnchainZap(secondSrc, "tx1", claimedSats = 999L, verifiedSats = 500L, status = OnchainZapStatus.CONFIRMED) + + val entry = target.onchainZaps["tx1"] + assertSame(firstSrc, entry?.source) + assertEquals(999L, entry?.verifiedSats) + assertEquals(BigDecimal.valueOf(999L), target.zapsAmount) + } + + @Test + fun removeOnchainZapForMatchingSourceDropsEntryAndAdjustsTotal() { + 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.removeOnchainZap("tx1") + target.removeOnchainZapForSource("tx1", srcKey) assertNull(target.onchainZaps["tx1"]) assertEquals(BigDecimal.ZERO, target.zapsAmount) } @Test - fun removeOnchainZapForUnknownTxidIsNoOp() { + 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 + // to match the rejecting sender. val target = freshNote() - val src = sourceNote("8".repeat(64)) + val legitSrcKey = "1a".repeat(32) + val attackerKey = "2b".repeat(32) + val legitSrc = sourceNote(legitSrcKey) - target.addOnchainZap(src, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.CONFIRMED) - target.removeOnchainZap("tx-never-added") + target.addOnchainZap(legitSrc, "tx1", claimedSats = 4200L, verifiedSats = 4200L, status = OnchainZapStatus.CONFIRMED) - assertEquals(1, target.onchainZaps.size) + // 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. + val target = freshNote() + val srcKey = "3c".repeat(32) + + target.removeOnchainZapForSource("tx-never-added", srcKey) + + assertEquals(0, target.onchainZaps.size) + assertTrue(target.wasOnchainZapRejectedForSource("tx-never-added", srcKey)) } @Test fun hasZapsBoostsOrReactionsReturnsTrueWhenOnlyOnchainZapPresent() { val target = freshNote() - val src = sourceNote("9".repeat(64)) + val src = sourceNote("4d".repeat(32)) assertFalse(target.hasZapsBoostsOrReactions()) - target.addOnchainZap( - source = src, - txid = "tx1", - claimedSats = 6416L, - verifiedSats = 6416L, - status = OnchainZapStatus.CONFIRMED, - ) + target.addOnchainZap(src, "tx1", claimedSats = 6416L, verifiedSats = 6416L, status = OnchainZapStatus.CONFIRMED) assertTrue(target.hasZapsBoostsOrReactions()) } @@ -220,15 +255,9 @@ class NoteOnchainZapTest { @Test fun hasZapsBoostsOrReactionsReturnsTrueWhenOnlyUnverifiedOnchainZapPresent() { val target = freshNote() - val src = sourceNote("8".repeat(64)) + val src = sourceNote("5e".repeat(32)) - target.addOnchainZap( - source = src, - txid = "tx1", - claimedSats = 1000L, - verifiedSats = 0L, - status = OnchainZapStatus.UNVERIFIED, - ) + target.addOnchainZap(src, "tx1", claimedSats = 1000L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) assertTrue(target.hasZapsBoostsOrReactions()) } @@ -236,10 +265,10 @@ class NoteOnchainZapTest { @Test fun updateZapTotalSumsConfirmedAcrossMultipleTxids() { val target = freshNote() - target.addOnchainZap(sourceNote("3".repeat(64)), "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.CONFIRMED) - target.addOnchainZap(sourceNote("4".repeat(64)), "tx2", claimedSats = 2000L, verifiedSats = 2000L, status = OnchainZapStatus.CONFIRMED) - target.addOnchainZap(sourceNote("5".repeat(64)), "tx3", claimedSats = 9999L, verifiedSats = 9999L, status = OnchainZapStatus.PENDING) - target.addOnchainZap(sourceNote("6".repeat(64)), "tx4", claimedSats = 1234L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) + target.addOnchainZap(sourceNote("60".repeat(32)), "tx1", claimedSats = 1000L, verifiedSats = 1000L, status = OnchainZapStatus.CONFIRMED) + target.addOnchainZap(sourceNote("71".repeat(32)), "tx2", claimedSats = 2000L, verifiedSats = 2000L, status = OnchainZapStatus.CONFIRMED) + target.addOnchainZap(sourceNote("82".repeat(32)), "tx3", claimedSats = 9999L, verifiedSats = 9999L, status = OnchainZapStatus.PENDING) + target.addOnchainZap(sourceNote("93".repeat(32)), "tx4", claimedSats = 1234L, verifiedSats = 0L, status = OnchainZapStatus.UNVERIFIED) assertEquals(BigDecimal.valueOf(3000L), target.zapsAmount) assertEquals(4, target.onchainZaps.size) From 0313dcf3fa1d359ad4707558edffffd5a4d8a1ca Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 22:42:25 +0000 Subject: [PATCH 3/4] fix(onchain-zaps): clear second-audit findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../amethyst/model/LocalCache.kt | 170 +++++++++++++----- .../amethyst/ui/note/OnchainZapGallery.kt | 57 +++--- .../amethyst/commons/model/Note.kt | 65 +++---- .../commons/model/NoteOnchainZapTest.kt | 142 +++++++++++++-- 4 files changed, 314 insertions(+), 120 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt index 35619d5e96..1163b08b42 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt @@ -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 = 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 = 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 by lazy { + val scope = + runCatching { Amethyst.instance.applicationIOScope }.getOrNull() + ?: return@lazy MutableStateFlow(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, ) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt index 8726ccba70..98b67d0a32 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt @@ -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, ) { - 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) } diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt index 04d6316657..d250ae7d2a 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/Note.kt @@ -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>() - private set + var onchainZapResolved: Boolean = false var zapPayments = mapOf() 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, diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt index ca62a12f00..59064d5512 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/NoteOnchainZapTest.kt @@ -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) + } } From daa83959b618f873b2eabec2ae4e6e79e1e28480 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 23 May 2026 16:13:19 +0000 Subject: [PATCH 4/4] refactor(onchain-zaps): extract verification coordinator from LocalCache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the asynchronous chain-verification side of NIP-BC onchain zaps out of LocalCache into a dedicated OnchainZapResolver class living alongside other NIP-specific subpackages under model/nipBCOnchainZaps/. LocalCache shrinks by ~240 lines and now owns only the synchronous event-dispatch responsibility: loading the event, attaching the optimistic UNVERIFIED entry for the sender's own zap, and delegating the verifier launch to the resolver. The resolver owns: - launchVerification(event, source, repliesTo) — async fire-and-forget - reverifyOnchainZapsForNote(note) — used by the gallery's screen-driven loop - onchainTipHeightFlow — shared chain-tip poller, lazy + WhileSubscribed - verifyingEventIds / reverifyingNoteIds — in-flight de-duplication - reverifySemaphore — parallelism cap OnchainZapGallery now calls LocalCache.onchainZapResolver.{reverifyOnchainZaps ForNote, onchainTipHeightFlow} directly. consume(OnchainZapEvent) passes the already-computed repliesTo into launchVerification so the new-event path doesn't recompute it on the verifier side. No behavior change — all 22 onchain-zap tests still pass. --- .../amethyst/model/LocalCache.kt | 267 ++-------------- .../nipBCOnchainZaps/OnchainZapResolver.kt | 295 ++++++++++++++++++ .../amethyst/ui/note/OnchainZapGallery.kt | 8 +- 3 files changed, 320 insertions(+), 250 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/model/nipBCOnchainZaps/OnchainZapResolver.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt index 1163b08b42..e3ffb07729 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt @@ -42,6 +42,7 @@ import com.vitorpamplona.amethyst.commons.services.nwc.NwcPaymentTracker import com.vitorpamplona.amethyst.isDebug import com.vitorpamplona.amethyst.model.LocalCache.observeEvents import com.vitorpamplona.amethyst.model.nip51Lists.HiddenUsersState +import com.vitorpamplona.amethyst.model.nipBCOnchainZaps.OnchainZapResolver import com.vitorpamplona.amethyst.service.BundledInsert import com.vitorpamplona.amethyst.service.checkNotInMainThread import com.vitorpamplona.amethyst.ui.note.dateFormatter @@ -251,8 +252,6 @@ import com.vitorpamplona.quartz.nipACWebRtcCalls.events.CallRenegotiateEvent import com.vitorpamplona.quartz.nipB0WebBookmarks.WebBookmarkEvent import com.vitorpamplona.quartz.nipB7Blossom.BlossomServersEvent import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.OnchainBackend -import com.vitorpamplona.quartz.nipBCOnchainZaps.verify.OnchainZapVerifier -import com.vitorpamplona.quartz.nipBCOnchainZaps.verify.VerifiedOnchainZap import com.vitorpamplona.quartz.nipBCOnchainZaps.zap.OnchainZapEvent import com.vitorpamplona.quartz.nipC0CodeSnippets.CodeSnippetEvent import com.vitorpamplona.quartz.nipC7Chats.ChatEvent @@ -263,32 +262,19 @@ import com.vitorpamplona.quartz.utils.TimeUtils import com.vitorpamplona.quartz.utils.cache.LargeCache import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.async -import kotlinx.coroutines.awaitAll import kotlinx.coroutines.channels.BufferOverflow import kotlinx.coroutines.channels.awaitClose -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( @@ -299,13 +285,6 @@ interface ILocalCache { } } -/** - * Polling interval for the shared onchain chain-tip flow. Aligned with - * `CachingOnchainBackend.tipHeightTtlSeconds` (60s) — polling faster would - * just hit the cache and do no useful work. - */ -private const val ONCHAIN_TIP_POLL_INTERVAL_MS = 60_000L - object LocalCache : ILocalCache, ICacheProvider { val antiSpam = AntiSpamFilter() @@ -329,6 +308,15 @@ object LocalCache : ILocalCache, ICacheProvider { @Volatile var onchainBackend: OnchainBackend? = null + /** + * NIP-BC on-chain zap verification coordinator. Owns the chain-tip poller flow, + * the in-flight de-duplication of verifier calls across consume/reverify paths, + * and the parallelism cap. `consume(OnchainZapEvent)` delegates the async + * verification side here; the gallery's reverification driver also calls in here + * directly. + */ + val onchainZapResolver = OnchainZapResolver(this) + /** * Resolver for LNURL provider metadata used by [consume]`(LnZapEvent)` to * validate NIP-57 Appendix F. `null` skips the receipt-signer check (the @@ -1877,6 +1865,7 @@ object LocalCache : ILocalCache, ICacheProvider { // attachment; for incoming zaps from others, the sender-claimed `amount` tag // is untrusted and could mislead the viewer until the chain verifier responds. val isOwnEvent = relay == null + var repliesTo: List? = null if (!alreadyLoaded) { if (!(wasVerified || justVerify(event))) return false @@ -1886,8 +1875,9 @@ object LocalCache : ILocalCache, ICacheProvider { if (event.pubKey.equals(recipient, ignoreCase = true)) return false val author = getOrCreateUser(event.pubKey) - val repliesTo = computeReplyTo(event) - note.loadEvent(event, author, repliesTo) + val resolvedRepliesTo = computeReplyTo(event) + repliesTo = resolvedRepliesTo + note.loadEvent(event, author, resolvedRepliesTo) if (isOwnEvent) { // Optimistic attachment for the sender's own zap: surface it on the @@ -1902,7 +1892,7 @@ object LocalCache : ILocalCache, ICacheProvider { // via toLongOrNull() with no sign check, so a malicious sender // could otherwise put "-1" in the gallery as a negative-sats badge. val claimedSats = (event.claimedAmountInSats() ?: 0L).coerceAtLeast(0L) - repliesTo.forEach { + resolvedRepliesTo.forEach { it.addOnchainZap(note, txid, claimedSats, verifiedSats = 0L, OnchainZapStatus.UNVERIFIED) } } @@ -1911,232 +1901,15 @@ object LocalCache : ILocalCache, ICacheProvider { refreshNewNoteObservers(note) } - // Verification needs a chain backend. Without one (e.g. before AppModules - // wires its EsploraBackend) the event stays cached so subscriptions and - // profile zap views see it, but it can't contribute to Note totals. - val backend = onchainBackend ?: return !alreadyLoaded - - // 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 { - try { - verifyAndUpgradeOnchainZap(event, note, repliesTo, verifier) - } finally { - verifyingEventIds.remove(event.id) - } - } + // Async chain verification is delegated to OnchainZapResolver, which owns the + // in-flight gates (so two relay echoes don't double-fetch) and the chain-tip + // polling flow used by the gallery driver. Reusing the repliesTo already + // computed above avoids the second computeReplyTo pass on the new-event path. + onchainZapResolver.launchVerification(event, note, repliesTo ?: computeReplyTo(event)) return !alreadyLoaded } - /** - * Run the chain verifier for a single onchain zap event and apply the result to every - * target note. Designed to be safe to call repeatedly — the [Note] entries upgrade - * 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, - source: Note, - repliesTo: List, - verifier: OnchainZapVerifier, - ) { - // Clamp claimedSats to non-negative; see consume() for rationale. - val claimedSats = (event.claimedAmountInSats() ?: 0L).coerceAtLeast(0L) - try { - when (val result = verifier.verify(event)) { - is VerifiedOnchainZap.Confirmed -> { - 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 -> { - if (result.reason == VerifiedOnchainZap.Rejected.Reason.TX_NOT_FOUND) { - // Transient — the tx may not have propagated to the backend's - // indexer yet. Leave the entry as UNVERIFIED so a later - // reverifyOnchainZapsForNote() call (e.g. on chain tip change) - // can promote it. - Log.d("OnchainZap") { "tx not yet indexed for ${event.id} (${result.txid}); will retry" } - } 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 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 - } - } - } - } catch (t: Throwable) { - // Never swallow cancellation — it must propagate so screen-scoped callers - // (the gallery's reverification driver) can tear down cleanly. - if (t is CancellationException) throw t - Log.w("OnchainZap", "verification failed for ${event.id}", t) - } - } - - /** - * Re-run onchain-zap verification for every non-CONFIRMED entry attached to [note]. - * 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. - * - * - 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 - 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) - 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() - } - } 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 = 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 = 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. 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 = 8) - - /** - * Shared poller for the current bitcoin chain tip height. Each gallery that - * holds non-CONFIRMED onchain zaps subscribes to this flow and re-verifies its - * entries whenever the tip advances. Lazy + `WhileSubscribed` so the HTTP call - * 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. 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 by lazy { - val scope = - runCatching { Amethyst.instance.applicationIOScope }.getOrNull() - ?: return@lazy MutableStateFlow(null).asStateFlow() - - flow { - while (true) { - 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( - scope, - SharingStarted.WhileSubscribed(stopTimeoutMillis = 5_000L), - initialValue = null, - ) - } - private fun attachZapToLiveActivityChannel( event: LnZapEvent, note: Note, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nipBCOnchainZaps/OnchainZapResolver.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nipBCOnchainZaps/OnchainZapResolver.kt new file mode 100644 index 0000000000..b22c441921 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nipBCOnchainZaps/OnchainZapResolver.kt @@ -0,0 +1,295 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.model.nipBCOnchainZaps + +import com.vitorpamplona.amethyst.Amethyst +import com.vitorpamplona.amethyst.commons.model.Note +import com.vitorpamplona.amethyst.commons.model.OnchainZapStatus +import com.vitorpamplona.amethyst.model.LocalCache +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nipBCOnchainZaps.verify.OnchainZapVerifier +import com.vitorpamplona.quartz.nipBCOnchainZaps.verify.VerifiedOnchainZap +import com.vitorpamplona.quartz.nipBCOnchainZaps.zap.OnchainZapEvent +import com.vitorpamplona.quartz.utils.Log +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.flow +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.util.concurrent.ConcurrentHashMap + +/** + * Coordinates NIP-BC on-chain zap verification on top of the chain backend. + * + * `LocalCache.consume(OnchainZapEvent)` owns the event dispatch and optimistic + * gallery attachment; this class owns the asynchronous chain-verification side: + * + * - Launching a verifier coroutine when a new event arrives (with per-event + * in-flight de-duplication so simultaneous relay echoes don't double-fetch). + * - Re-running verification for visible notes when the chain tip advances or + * when a screen first comes into view. + * - Owning the shared chain-tip poller flow that UI subscribes to. + * + * Stateless from the caller's perspective — the per-event resolution state lives + * on the source [Note] itself (`onchainZapResolved`), so it travels with the Note + * across cache eviction and doesn't accumulate here. + */ +class OnchainZapResolver( + private val cache: LocalCache, +) { + /** + * Caps the parallelism of [reverifyOnchainZapsForNote] so a thread with many + * pending entries doesn't blast public Esplora endpoints with a burst of + * simultaneous requests. + */ + private val reverifySemaphore = Semaphore(permits = 8) + + /** + * In-flight set of event ids currently being verified. Prevents two `consume()` + * calls (or a `consume` plus 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 = ConcurrentHashMap.newKeySet() + + /** + * In-flight set of note id strings currently being reverified. Lets the on-chain + * zap gallery driver be called from many visible composables without the same + * note's reverify pass running concurrently. The per-event gate above is the + * backstop; this one short-circuits earlier and avoids creating async coroutines + * that would just no-op. + */ + private val reverifyingNoteIds: MutableSet = ConcurrentHashMap.newKeySet() + + /** + * Shared poller for the current bitcoin chain tip height. Each gallery that + * holds non-CONFIRMED on-chain zaps subscribes to this flow and re-verifies its + * entries whenever the tip advances. Lazy + `WhileSubscribed` so the HTTP call + * only fires when at least one UI surface needs it. + * + * Lazy initialization is required because [Amethyst.instance] may not exist + * when [OnchainZapResolver] is first constructed. 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 by lazy { + val scope = + runCatching { Amethyst.instance.applicationIOScope }.getOrNull() + ?: return@lazy MutableStateFlow(null).asStateFlow() + + flow { + while (true) { + val tip = + cache.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(TIP_POLL_INTERVAL_MS) + } + }.stateIn( + scope, + SharingStarted.WhileSubscribed(stopTimeoutMillis = 5_000L), + initialValue = null, + ) + } + + /** + * Launches an asynchronous chain verification for [event] unless one is already + * in flight or [source] has already reached a terminal verdict. Returns + * immediately. Apply-to-Note updates happen on the application IO scope. + * + * [repliesTo] is the pre-computed list of notes the event references so consume() + * doesn't pay the cost of resolving it twice. + */ + fun launchVerification( + event: OnchainZapEvent, + source: Note, + repliesTo: List, + ) { + val backend = cache.onchainBackend ?: return + if (source.onchainZapResolved) return + if (!verifyingEventIds.add(event.id)) return + + val verifier = OnchainZapVerifier(backend) + + Amethyst.instance.applicationIOScope.launch { + try { + verifyAndUpgradeOnchainZap(event, source, repliesTo, verifier) + } finally { + verifyingEventIds.remove(event.id) + } + } + } + + /** + * Re-run on-chain zap verification for every non-CONFIRMED entry attached to + * [note]. 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. + * + * - 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 + * [launchVerification] 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 = cache.onchainBackend ?: 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) + 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 = cache.computeReplyTo(sourceEvent) + verifyAndUpgradeOnchainZap(sourceEvent, source, repliesTo, verifier) + } + } finally { + verifyingEventIds.remove(sourceEvent.id) + } + } + }.awaitAll() + } + } finally { + reverifyingNoteIds.remove(note.idHex) + } + } + + /** + * Run the chain verifier for a single on-chain zap event and apply the result to + * every target note. Designed to be safe to call repeatedly — the [Note] entries + * upgrade 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 the [verifyingEventIds] gate; this + * function does not itself protect against duplicate concurrent runs. + */ + private suspend fun verifyAndUpgradeOnchainZap( + event: OnchainZapEvent, + source: Note, + repliesTo: List, + verifier: OnchainZapVerifier, + ) { + // Clamp claimedSats to non-negative — `amount` tag parses via toLongOrNull() + // with no sign check, so a malicious sender could otherwise put "-1" in the + // gallery as a negative-sats badge. + val claimedSats = (event.claimedAmountInSats() ?: 0L).coerceAtLeast(0L) + try { + when (val result = verifier.verify(event)) { + is VerifiedOnchainZap.Confirmed -> { + 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 `launchVerification`. + 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. + } + + is VerifiedOnchainZap.Rejected -> { + if (result.reason == VerifiedOnchainZap.Rejected.Reason.TX_NOT_FOUND) { + // Transient — the tx may not have propagated to the backend's + // indexer yet. Leave the entry as UNVERIFIED so a later + // reverifyOnchainZapsForNote() call (e.g. on chain tip change) + // can promote it. + Log.d("OnchainZap") { "tx not yet indexed for ${event.id} (${result.txid}); will retry" } + } 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 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 + } + } + } + } catch (t: Throwable) { + // Never swallow cancellation — it must propagate so screen-scoped callers + // (the gallery's reverification driver) can tear down cleanly. + if (t is CancellationException) throw t + Log.w("OnchainZap", "verification failed for ${event.id}", t) + } + } + + companion object { + /** + * Polling interval for the shared on-chain chain-tip flow. Aligned with + * `CachingOnchainBackend.tipHeightTtlSeconds` (60s) — polling faster would + * just hit the cache and do no useful work. + */ + private const val TIP_POLL_INTERVAL_MS = 60_000L + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt index 98b67d0a32..5ae2b6c56e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/OnchainZapGallery.kt @@ -111,22 +111,24 @@ private fun DriveOnchainZapReverification( val pendingCount = remember(entries) { entries.count { it.status != OnchainZapStatus.CONFIRMED } } if (pendingCount == 0) return + val resolver = LocalCache.onchainZapResolver + // 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) + resolver.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() + val tip by resolver.onchainTipHeightFlow.collectAsStateWithLifecycle() LaunchedEffect(note.idHex, tip) { if (tip != null) { - LocalCache.reverifyOnchainZapsForNote(note) + resolver.reverifyOnchainZapsForNote(note) } } }