-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(notes): drop expired notes and match geohash tags case-insensitively #896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
293a751
26100f4
15af9bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,19 @@ class LocationNotesManager private constructor() { | |
| private const val TAG = "LocationNotesManager" | ||
| private const val MAX_NOTES_IN_MEMORY = 500 | ||
|
|
||
| /** How often displayed notes are re-checked against their NIP-40 expiry. */ | ||
| private const val EXPIRY_PRUNE_INTERVAL_MS = 60_000L | ||
|
|
||
| /** | ||
| * Drops notes whose NIP-40 expiry has passed. Their ids stay in | ||
| * `noteIDs`, so a relay replay cannot resurrect them. | ||
| */ | ||
| internal fun pruneExpired(notes: List<Note>, nowMillis: Long): List<Note> = | ||
| notes.filter { note -> | ||
| val expiresAt = note.expiresAtSeconds ?: return@filter true | ||
| expiresAt * 1000L > nowMillis | ||
| } | ||
|
|
||
| @Volatile | ||
| private var INSTANCE: LocationNotesManager? = null | ||
|
|
||
|
|
@@ -38,7 +51,9 @@ class LocationNotesManager private constructor() { | |
| val pubkey: String, | ||
| val content: String, | ||
| val createdAt: Int, | ||
| val nickname: String? | ||
| val nickname: String?, | ||
| /** NIP-40 expiry as unix seconds, when the note carries one. */ | ||
| val expiresAtSeconds: Long? = null | ||
| ) { | ||
| /** | ||
| * Display name for the note - matches iOS exactly | ||
|
|
@@ -99,6 +114,7 @@ class LocationNotesManager private constructor() { | |
| private var liveLocationToken: Long? = null | ||
| private var subscribeRetryJob: Job? = null | ||
| private var initialLoadJob: Job? = null | ||
| private var expiryPruneJob: Job? = null | ||
|
|
||
| init { | ||
| LiveLocationPrivacyGate.addRevocationListener(::stop) | ||
|
|
@@ -370,7 +386,18 @@ class LocationNotesManager private constructor() { | |
| } | ||
|
|
||
| _state.value = State.LOADING | ||
|
|
||
|
|
||
| // A note can expire while it is on screen (a 24h dead drop crossing its | ||
| // boundary). Filtering at ingest alone would keep it visible until the | ||
| // subscription is recreated, so re-check the displayed notes as well. | ||
| expiryPruneJob?.cancel() | ||
| expiryPruneJob = scope.launch { | ||
| while (isActive) { | ||
| delay(EXPIRY_PRUNE_INTERVAL_MS) | ||
| pruneExpiredNotes() | ||
| } | ||
| } | ||
|
|
||
| // Subscribe for each geohash in the ±1 set | ||
| subscribedGeohashes.forEach { gh -> | ||
| if (!LiveLocationPrivacyGate.accepts(token)) return | ||
|
|
@@ -417,23 +444,30 @@ class LocationNotesManager private constructor() { | |
| return | ||
| } | ||
|
|
||
| // Check for geohash tag | ||
| val geohashTag = event.tags.firstOrNull { it.size >= 2 && it[0] == "g" } | ||
| if (geohashTag == null) { | ||
| Log.v(TAG, "Ignoring event without geohash tag: ${event.id.take(16)}...") | ||
| return | ||
| // Check for geohash tag. Tag names and geohashes are case-insensitive, | ||
| // and iOS matches them lowercased, so a note tagged ["G", "U4PRUYD"] | ||
| // has to be the same note on both platforms. | ||
| val validGeohashes = subscribedGeohashes.map { it.lowercase() }.toSet() | ||
| val geohashTag = event.tags.firstOrNull { | ||
| it.size >= 2 && it[0].lowercase() == "g" && validGeohashes.contains(it[1].lowercase()) | ||
| } | ||
|
|
||
| // Check if matches current geohash | ||
| val eventGeohash = geohashTag[1] | ||
| if (!subscribedGeohashes.contains(eventGeohash)) { | ||
| if (geohashTag == null) { | ||
| Log.v(TAG, "Ignoring event without a matching geohash tag: ${event.id.take(16)}...") | ||
| return | ||
| } | ||
|
|
||
| // Deduplicate | ||
| if (noteIDs.contains(event.id)) { | ||
| return | ||
| } | ||
|
|
||
| // NIP-40: relays are not required to drop expired events, so enforce it | ||
| // here - otherwise a 24h dead drop stays visible past its expiry. | ||
| val expiresAt = expirationSeconds(event) | ||
| if (expiresAt != null && expiresAt * 1000L <= System.currentTimeMillis()) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a note arrives before its NIP-40 expiration time, this check admits it into AGENTS.md reference: AGENTS.md:L90-L91 Useful? React with 👍 / 👎. |
||
| Log.v(TAG, "Ignoring expired note: ${event.id.take(16)}...") | ||
| return | ||
| } | ||
|
|
||
| // Extract nickname from tags | ||
| val nicknameTag = event.tags.firstOrNull { it.size >= 2 && it[0] == "n" } | ||
|
|
@@ -445,7 +479,8 @@ class LocationNotesManager private constructor() { | |
| pubkey = event.pubkey, | ||
| content = event.content, | ||
| createdAt = event.createdAt, | ||
| nickname = nickname | ||
| nickname = nickname, | ||
| expiresAtSeconds = expiresAt | ||
| ) | ||
|
|
||
| // Add to collection | ||
|
|
@@ -465,6 +500,27 @@ class LocationNotesManager private constructor() { | |
| _state.value = State.READY | ||
| } | ||
|
|
||
| /** | ||
| * The NIP-40 `expiration` tag as unix seconds, when the event carries one. | ||
| */ | ||
| private fun expirationSeconds(event: NostrEvent): Long? { | ||
| val tag = event.tags.firstOrNull { it.size >= 2 && it[0].lowercase() == "expiration" } | ||
| ?: return null | ||
| return tag[1].toLongOrNull() | ||
| } | ||
|
|
||
| /** | ||
| * Drops displayed notes whose NIP-40 expiry has passed. | ||
| */ | ||
| private fun pruneExpiredNotes() { | ||
| val current = _notes.value ?: return | ||
| val remaining = pruneExpired(current, System.currentTimeMillis()) | ||
| // Ids stay in noteIDs so a relay replay cannot resurrect a dropped note. | ||
| if (remaining.size != current.size) { | ||
| _notes.value = remaining | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Trim oldest notes to stay within memory limit | ||
| */ | ||
|
|
@@ -497,6 +553,8 @@ class LocationNotesManager private constructor() { | |
| subscribeRetryJob = null | ||
| initialLoadJob?.cancel() | ||
| initialLoadJob = null | ||
| expiryPruneJob?.cancel() | ||
| expiryPruneJob = null | ||
|
|
||
| if (subscriptionIDs.isNotEmpty()) { | ||
| subscriptionIDs.values.forEach { subId -> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package com.bitchat.android.nostr | ||
|
|
||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertFalse | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Test | ||
|
|
||
| /** | ||
| * Two places Android disagreed with iOS on the same note event: the case of the | ||
| * geohash tag, and a NIP-40 expiry that passes while the note is displayed. | ||
| */ | ||
| class LocationNotesCaseAndExpiryTest { | ||
|
|
||
| private fun note(id: String, expiresAtSeconds: Long?) = LocationNotesManager.Note( | ||
| id = id, | ||
| pubkey = "a".repeat(64), | ||
| content = "hi", | ||
| createdAt = 1_700_000_000, | ||
| nickname = null, | ||
| expiresAtSeconds = expiresAtSeconds | ||
| ) | ||
|
|
||
| private fun event(tags: List<List<String>>) = NostrEvent( | ||
| id = "b".repeat(64), | ||
| pubkey = "a".repeat(64), | ||
| createdAt = 1_700_000_000, | ||
| kind = NostrKind.TEXT_NOTE, | ||
| tags = tags, | ||
| content = "hi" | ||
| ) | ||
|
|
||
| @Test | ||
| fun `an uppercase geohash tag passes the subscription filter`() { | ||
| val filter = NostrFilter.geohashNotes(geohash = "u4pruyd") | ||
|
|
||
| assertTrue(filter.matches(event(listOf(listOf("g", "u4pruyd"))))) | ||
| // iOS has no client-side filter and lowercases where it reads the tag, | ||
| // so this note is visible there; it has to reach the handler here too. | ||
| assertTrue(filter.matches(event(listOf(listOf("G", "U4PRUYD"))))) | ||
| } | ||
|
|
||
| @Test | ||
| fun `a different geohash is still rejected`() { | ||
| val filter = NostrFilter.geohashNotes(geohash = "u4pruyd") | ||
|
|
||
| assertFalse(filter.matches(event(listOf(listOf("g", "u4pruye"))))) | ||
| assertFalse(filter.matches(event(listOf(listOf("g"))))) | ||
| } | ||
|
|
||
| @Test | ||
| fun `a note is dropped once its expiry passes`() { | ||
| val now = 1_700_000_000_000L | ||
| val notes = listOf( | ||
| note("plain", expiresAtSeconds = null), | ||
| note("later", expiresAtSeconds = 1_700_000_060L), | ||
| note("gone", expiresAtSeconds = 1_699_999_940L) | ||
| ) | ||
|
|
||
| val remaining = LocationNotesManager.pruneExpired(notes, now) | ||
|
|
||
| assertEquals(listOf("plain", "later"), remaining.map { it.id }) | ||
| } | ||
|
|
||
| @Test | ||
| fun `pruning at the expiry instant drops the note`() { | ||
| val notes = listOf(note("edge", expiresAtSeconds = 1_700_000_000L)) | ||
|
|
||
| assertTrue(LocationNotesManager.pruneExpired(notes, 1_699_999_999_000L).isNotEmpty()) | ||
| assertTrue(LocationNotesManager.pruneExpired(notes, 1_700_000_000_000L).isEmpty()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For relay-delivered events with an uppercase
Gtag or uppercase geohash, this handler still will not run:LocationNotesInitializersubscribes with the originalNostrFilter, andNostrRelayManager.handleMessagecallssubInfo.filter.matches(response.event)before dispatch;NostrFilter.matchesstill compares tag names and values exactly (it[0] == tagName,eventValues.contains(requiredValue)). Because the lowercasing happens only here, those events are dropped in the relay layer and Android still diverges from the intended case-insensitive behavior unless the filter match/subscription path is normalized too.AGENTS.md reference: AGENTS.md:L90-L91
Useful? React with 👍 / 👎.