diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/PollResultsScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/PollResultsScreen.kt index a6baf003df..f491491f77 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/PollResultsScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/PollResultsScreen.kt @@ -76,6 +76,7 @@ import com.vitorpamplona.amethyst.commons.viewmodels.nip88Polls.PollOptionResult import com.vitorpamplona.amethyst.commons.viewmodels.nip88Polls.PollResultsUiState import com.vitorpamplona.amethyst.commons.viewmodels.nip88Polls.PollResultsViewModel import com.vitorpamplona.amethyst.commons.viewmodels.nip88Polls.PollVoterRow +import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.EventFinderFilterAssemblerSubscription import com.vitorpamplona.amethyst.ui.components.LoadNote import com.vitorpamplona.amethyst.ui.navigation.navs.INav import com.vitorpamplona.amethyst.ui.navigation.routes.routeFor @@ -140,6 +141,12 @@ private fun PollResults( ) { val account = accountViewModel.account + // Keeps a REQ open for this poll while the screen is on top. Without it the only votes ever + // shown are the ones the one-shot backfill happened to catch: a vote cast while you are reading + // would never arrive, because the feed card that used to hold this subscription is disposed + // behind us. It also loads the kind-1068 event itself when we arrived by deep link. + EventFinderFilterAssemblerSubscription(note, accountViewModel) + // Opening this screen is the opt-in, exactly like the card's "View results" link — so the feed // card stops hiding the tally behind a tap once you have been here. LaunchedEffect(note.idHex) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/RelayPollResponseLoader.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/RelayPollResponseLoader.kt index 7ac32fcaac..d24599e1d7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/RelayPollResponseLoader.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/polls/results/RelayPollResponseLoader.kt @@ -29,7 +29,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.count import com.vitorpamplona.quartz.nip01Core.relay.client.accessories.fetchAllPagesFromPool import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl -import com.vitorpamplona.quartz.nip45Count.HyperLogLog +import com.vitorpamplona.quartz.nip45Count.mergeCountResults import com.vitorpamplona.quartz.nip88Polls.poll.PollEvent import com.vitorpamplona.quartz.nip88Polls.response.PollResponseEvent @@ -44,7 +44,8 @@ import com.vitorpamplona.quartz.nip88Polls.response.PollResponseEvent * partially and nothing says so. [fetchAllPagesFromPool] walks `until` backwards per relay * until each is exhausted. * - * 2. **Knowing what "complete" is.** See [mergeCounts] — a COUNT fan-out cannot simply be summed. + * 2. **Knowing what "complete" is.** See [mergeCountResults] — a COUNT fan-out cannot be summed, + * because relays mirror each other and the same vote would be counted once per relay. */ class RelayPollResponseLoader( private val client: INostrClient, @@ -54,33 +55,6 @@ class RelayPollResponseLoader( companion object { /** Per-relay idle window for both the drain and the COUNT. */ const val TIMEOUT_MS = 20_000L - - /** - * Combines per-relay COUNT results into one figure that is never inflated. - * - * Summing is wrong: relays mirror each other, so the same vote is counted once per relay - * that holds it. NIP-45's optional `hll` field exists precisely for this — HyperLogLog - * registers merge by taking the per-register maximum, and the union's cardinality is - * re-estimated from the merged registers, so shared events collapse instead of stacking. - * - * When no relay ships HLL (most don't), the best available answer is the **largest single - * relay's count**: every relay's count is a lower bound on the union, so the max is the - * tightest lower bound we can justify. Relays that don't implement COUNT never answer and - * are simply absent — they make the figure smaller, never larger, which is the safe - * direction for a number the UI presents as "at least this many". - */ - fun mergeCounts(counts: List>): Pair? { - if (counts.isEmpty()) return null - - val hlls = counts.mapNotNull { it.second } - if (hlls.isNotEmpty()) { - val merged = HyperLogLog.merge(hlls) - return HyperLogLog.estimate(merged).toInt() to true - } - - // No registers to union — fall back to the tightest lower bound, never the sum. - return (counts.maxOf { it.first }) to false - } } override suspend fun load(poll: PollEvent): PollLoadReport { @@ -103,12 +77,13 @@ class RelayPollResponseLoader( } val results = client.count(relays.associateWith { listOf(filter) }, idleTimeoutMs = TIMEOUT_MS) - val merged = mergeCounts(results.values.map { it.count to it.hll }) + // Combination rules (never a sum) live in quartz — see mergeCountResults. + val merged = mergeCountResults(results.values) return PollLoadReport( - reported = merged?.first, + reported = merged?.count, // An estimate is approximate; so is a figure from only some of the relays we asked. - approximate = (merged?.second ?: false) || results.size < relays.size, + approximate = (merged?.approximate ?: false) || results.size < relays.size, relaysAsked = relays.size, relaysAnswered = results.size, ) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModel.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModel.kt index 723c37e67c..0cd76de132 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModel.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModel.kt @@ -42,9 +42,11 @@ import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch +import kotlin.coroutines.CoroutineContext import kotlin.reflect.KClass /** One option's share of the poll, ready to draw. */ @@ -96,9 +98,11 @@ class PollResultsUiState( /** * Screen state for the poll results page. * - * Reads the live tally off the poll [Note] — no new subscription, no second cache — and turns it - * into rows the UI can render without doing set arithmetic in composition. The only interactive - * state is [selectedOption], which scopes the voter list without touching the summary above it. + * Reads the live tally off the poll [Note] — no second cache — and turns it into rows the UI can + * render without doing set arithmetic in composition. Every rebuild after the first runs on + * [computeContext] (Default): a backfill emits one tally per consumed vote, and re-sorting thousands of + * voters that many times on the UI thread would drop frames for the whole drain. The only + * interactive state is [selectedOption], which scopes the voter list without touching the summary. * * Muting is applied here rather than in the tally: a muted voter still counts toward the totals * (they did vote, and hiding them from the maths would misreport the poll), they are just not @@ -112,6 +116,9 @@ class PollResultsViewModel( follows: Flow>, hiddenChanges: Flow, private val loader: PollResponseLoader? = null, + // Injected so tests can drive both on the test scheduler; production never passes them. + private val computeContext: CoroutineContext = Dispatchers.Default, + private val loadContext: CoroutineContext = Dispatchers.IO, ) : ViewModel() { companion object { /** Faces shown per option before collapsing into a "+N" chip, matching UserGallery. */ @@ -132,7 +139,7 @@ class PollResultsViewModel( // Page past the subscription cap once, on open. Without this a poll with more responses // than the live filter's limit shows a confidently wrong tally. if (loader != null) { - viewModelScope.launch(Dispatchers.IO) { + viewModelScope.launch(loadContext) { val poll = awaitPollEvent() backfill.value = BackfillState(running = true) val report = runCatching { loader.load(poll) }.getOrNull() @@ -160,11 +167,16 @@ class PollResultsViewModel( backfill, ) { tally, followSet, selected, _, load -> build(tally, followSet, selected, load) - }.stateIn( - scope = viewModelScope, - started = SharingStarted.WhileSubscribed(5_000), - initialValue = build(pollNote.pollState().responses.value, emptySet(), null, BackfillState()), - ) + }.flowOn(computeContext) + .stateIn( + scope = viewModelScope, + started = SharingStarted.WhileSubscribed(5_000), + // Built eagerly so the first frame already has the tally instead of flashing an + // empty poll. Only this one pass is on the caller's thread; every recomputation + // after it — including the flood of them during a backfill, one per consumed vote — + // runs on Default via the flowOn above. + initialValue = build(pollNote.pollState().responses.value, emptySet(), null, BackfillState()), + ) fun selectOption(code: String?) { _selectedOption.value = if (_selectedOption.value == code) null else code diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModelTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModelTest.kt index bf49356049..3292ee828d 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModelTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/nip88Polls/PollResultsViewModelTest.kt @@ -124,7 +124,7 @@ class PollResultsViewModelTest { note.pollState().addResponse(responseNote) } - private fun viewModel( + private fun TestScope.viewModel( note: Note, hidden: Set = emptySet(), follows: Set = setOf(followed), @@ -136,6 +136,10 @@ class PollResultsViewModelTest { follows = MutableStateFlow(follows), hiddenChanges = MutableStateFlow(Unit), loader = loader, + // Keep the state build and the backfill on the test scheduler; in production these are + // Default and IO so a big poll's re-sorts never land on the UI thread. + computeContext = UnconfinedTestDispatcher(testScheduler), + loadContext = UnconfinedTestDispatcher(testScheduler), ) @Test diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt index 0f19d9d75c..08d7db6b67 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt @@ -29,7 +29,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.commands.toClient.CountResult import com.vitorpamplona.quartz.nip01Core.relay.commands.toClient.Message import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl -import com.vitorpamplona.quartz.nip45Count.HyperLogLog +import com.vitorpamplona.quartz.nip45Count.mergeCountResults import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.channels.Channel.Factory.UNLIMITED import kotlinx.coroutines.ensureActive @@ -179,15 +179,9 @@ suspend fun INostrClient.count( } /** - * Queries multiple relays for a COUNT and merges the HyperLogLog - * registers from all responses to produce a single merged estimate. + * Queries multiple relays for a COUNT and combines the answers into one figure. * - * If any relay returns HLL data, the results are merged by taking - * the maximum register value across all relays, and the cardinality - * is re-estimated from the merged registers. - * - * If no relay returns HLL data, falls back to the maximum count - * reported by any relay. + * The combination rules — and why summing is never one of them — live in [mergeCountResults]. * * @param relays List of relays to query. * @param filter The filter to count against. @@ -207,20 +201,5 @@ suspend fun INostrClient.countMerged( idleTimeoutMs = idleTimeoutMs, ) - if (results.isEmpty()) return null - - val hlls = results.values.mapNotNull { it.hll } - - return if (hlls.isNotEmpty()) { - val merged = HyperLogLog.merge(hlls) - val estimate = HyperLogLog.estimate(merged) - CountResult( - count = estimate.toInt(), - approximate = true, - hll = merged, - ) - } else { - // No HLL data - use the maximum count from any relay - results.values.maxByOrNull { it.count } - } + return mergeCountResults(results.values) } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip45Count/CountMerge.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip45Count/CountMerge.kt new file mode 100644 index 0000000000..076f893589 --- /dev/null +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip45Count/CountMerge.kt @@ -0,0 +1,67 @@ +/* + * 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.quartz.nip45Count + +import com.vitorpamplona.quartz.nip01Core.relay.commands.toClient.CountResult + +/** + * Combines NIP-45 COUNT answers from several relays into one figure. + * + * **Never sum.** Relays mirror each other, so the same event is normally held by several of them + * and adding their counts multiplies the result by an unknown factor. Every honest combination is + * therefore either a set union or a lower bound: + * + * - **HyperLogLog union.** Relays that ship the optional `hll` field give 256 registers that merge + * by per-register maximum; the union's cardinality is re-estimated from the merged registers, so + * events held by several relays collapse into one instead of stacking. + * - **Largest plain count.** A relay's own count is a lower bound on the union, so the biggest one + * is the tightest bound available without registers to union. + * + * When both kinds arrive, neither wins by default: the HLL union covers only the relays that + * supplied registers, and a plain count from a relay outside that set can easily exceed it — so the + * answer is the larger of the two. Taking the estimate alone would silently discard the better + * bound. + * + * Relays that don't implement COUNT simply never answer and are absent here. That can only make the + * result too small, which is the safe direction for a number presented as "at least this many". + * + * @return the merged result, or null when nothing to merge. + */ +fun mergeCountResults(results: Collection): CountResult? { + if (results.isEmpty()) return null + + val hlls = results.mapNotNull { it.hll } + if (hlls.isEmpty()) { + return results.maxByOrNull { it.count } + } + + val merged = HyperLogLog.merge(hlls) + val union = HyperLogLog.estimate(merged).toInt() + + // Relays that answered without registers are not represented in the union above. + val bestPlain = results.filter { it.hll == null }.maxOfOrNull { it.count } ?: 0 + + return CountResult( + count = maxOf(union, bestPlain), + approximate = true, + hll = merged, + ) +} diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip45Count/CountMergeTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip45Count/CountMergeTest.kt new file mode 100644 index 0000000000..1b673e5bfc --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip45Count/CountMergeTest.kt @@ -0,0 +1,156 @@ +/* + * 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.quartz.nip45Count + +import com.vitorpamplona.quartz.nip01Core.relay.commands.toClient.CountResult +import kotlin.random.Random +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +class CountMergeTest { + /** Register-index offset into the key, as a relay counting by `#e` would use. */ + private val offset = 8 + + /** A relay that answered with a plain count and no registers. */ + private fun plain(count: Int) = CountResult(count = count, approximate = false, hll = null) + + /** + * A relay that answered with HLL registers built from [ids] distinct 32-byte keys. + * + * Real registers rather than hand-written bytes, so the union assertions exercise the same + * add/merge/estimate path a relay's answer would. + */ + private fun hll(ids: Iterable): CountResult { + val builder = HllBuilder(offset = offset) + ids.forEach { builder.add(key(it)) } + return builder.toCountResult() + } + + /** + * Deterministic distinct 32-byte key. + * + * Pseudo-random bytes rather than a counter written into the array: HyperLogLog reads a + * register index and a leading-zero run straight out of the key and assumes those bits are + * uniformly distributed, which is true of real event ids and false of counters — structured + * keys pile identical values into the registers and the estimator goes wild. + */ + private fun key(n: Int): ByteArray = Random(n).nextBytes(32) + + @Test + fun noAnswersMergeToNull() { + assertNull(mergeCountResults(emptyList())) + } + + @Test + fun plainCountsTakeTheLargestNeverTheSum() { + val merged = mergeCountResults(listOf(plain(10), plain(500), plain(120))) + + assertNotNull(merged) + // 630 would be the sum. Relays mirror each other, so the union is at least the biggest + // single relay and at most the sum — only the lower bound is defensible. + assertEquals(500, merged.count) + assertEquals(null, merged.hll) + } + + @Test + fun aSingleRelayPassesItsOwnAnswerThrough() { + val merged = mergeCountResults(listOf(plain(42))) + + assertNotNull(merged) + assertEquals(42, merged.count) + } + + @Test + fun overlappingHllRegistersCollapseInsteadOfStacking() { + // Two relays holding heavily overlapping sets: 0..199 and 100..299. + val a = hll(0 until 200) + val b = hll(100 until 300) + val merged = mergeCountResults(listOf(a, b)) + + assertNotNull(merged) + assertTrue(merged.approximate) + assertNotNull(merged.hll) + + // The claim under test is the merge property, not the estimator's accuracy: merging two + // relays' registers must give exactly what one relay holding the union would have reported. + // (HLL at m=256 has no bias correction and reads high around n≈m, so asserting a band + // around the true 300 would be testing the estimator, not this function.) + assertEquals(hll(0 until 300).count, merged.count) + + // And it must land nowhere near the 400 a naive sum would claim. + assertTrue(merged.count < a.count + b.count, "merging must collapse the overlap, not stack it") + } + + @Test + fun disjointHllRegistersMergeToTheUnionToo() { + val merged = mergeCountResults(listOf(hll(0 until 150), hll(500 until 650))) + + assertNotNull(merged) + assertEquals(hll((0 until 150) + (500 until 650)).count, merged.count) + } + + @Test + fun aPlainCountLargerThanTheHllUnionIsNotDiscarded() { + // The regression this test exists for: a relay outside the register set can hold far more + // than the relays that supplied registers. Estimating from the registers alone threw that + // relay's answer away and reported ~50 instead of 5000. + val merged = mergeCountResults(listOf(hll(0 until 50), plain(5000))) + + assertNotNull(merged) + assertEquals(5000, merged.count) + // Still approximate, and the registers are still carried so a caller can merge again. + assertTrue(merged.approximate) + assertNotNull(merged.hll) + } + + @Test + fun aPlainCountSmallerThanTheHllUnionDoesNotDragItDown() { + val union = hll(0 until 300) + val merged = mergeCountResults(listOf(union, plain(5))) + + assertNotNull(merged) + assertEquals(union.count, merged.count) + } + + @Test + fun mergedRegistersAreTheUnionOfTheInputs() { + val a = hll(0 until 100) + val b = hll(100 until 200) + val merged = mergeCountResults(listOf(a, b)) + + assertNotNull(merged) + val expected = HyperLogLog.merge(listOf(a.hll!!, b.hll!!)) + assertEquals(expected.toList(), merged.hll!!.toList()) + } + + @Test + fun theResultIsAlwaysAtLeastEveryIndividualAnswer() { + // The property that makes this safe to show as "at least this many". + val answers = listOf(plain(7), plain(31), hll(0 until 20), plain(12)) + val merged = mergeCountResults(answers) + + assertNotNull(merged) + assertTrue(merged.count >= answers.filter { it.hll == null }.maxOf { it.count }) + } +}