From f1192492d23375759c3107e5d00e1b13ddeb9887 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 11 Oct 2026 09:02:06 +0700 Subject: [PATCH] chore: track `previous` references and preserve foreign tags --- crates/nip29/src/group.rs | 13 ++ crates/nip29/src/protocol.rs | 198 ++++++++++++++++++--- crates/workspace/src/sidebar/group_view.rs | 10 +- docs/TODO.md | 24 ++- docs/nip29-comparison.md | 137 -------------- 5 files changed, 213 insertions(+), 169 deletions(-) delete mode 100644 docs/nip29-comparison.md diff --git a/crates/nip29/src/group.rs b/crates/nip29/src/group.rs index 15a02885..e880293f 100644 --- a/crates/nip29/src/group.rs +++ b/crates/nip29/src/group.rs @@ -580,6 +580,8 @@ impl Group { fn absorb(&mut self, event: &Event, me: PublicKey) -> Absorbed { if STATE_KINDS.contains(&event.kind) { + self.window.note(event); + return if self.absorb_state(event, me) { Absorbed::Updated } else { @@ -587,6 +589,17 @@ impl Group { }; } + // NIP-29: relays must reject events whose + // `previous` references are absent from their database. + let previous = event.tags.iter().find(|tag| tag.kind() == "previous"); + if self.window.references_seen(previous) == Some(false) { + log::warn!( + "nip29: event {} carries previous references not seen on {}", + event.id, + self.key.relay() + ); + } + self.track(event); let updated = self.absorb_membership(event, me); diff --git a/crates/nip29/src/protocol.rs b/crates/nip29/src/protocol.rs index d546f920..bad8c22d 100644 --- a/crates/nip29/src/protocol.rs +++ b/crates/nip29/src/protocol.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::collections::hash_map::DefaultHasher; use std::fmt; use std::hash::{Hash, Hasher}; @@ -665,9 +666,10 @@ pub(crate) struct GroupList { entries: Vec, /// The raw private section of the last seen list event. content: Option, - /// Whether the content was decrypted in this session; an opaque section - /// (another client's encryption, or a refused decrypt) is carried verbatim. + /// Whether the content was decrypted in this session. readable: bool, + /// Tags written by other clients. + foreign: Vec, } impl GroupList { @@ -677,27 +679,35 @@ impl GroupList { } let mut entries = Vec::new(); + let mut foreign = Vec::new(); - for tag in event.tags.iter().filter(|tag| tag.kind() == "group") { - let fields = tag.as_slice(); + for tag in event.tags.iter() { + match tag.kind() { + "group" => { + let fields = tag.as_slice(); - let (Some(id), Some(relay)) = (fields.get(1), fields.get(2)) else { - continue; - }; + let (Some(id), Some(relay)) = (fields.get(1), fields.get(2)) else { + continue; + }; - let Ok(id) = GroupId::new(id.as_str()) else { - continue; - }; - let Ok(relay) = RelayUrl::parse(relay.as_str()) else { - continue; - }; + let Ok(id) = GroupId::new(id.as_str()) else { + continue; + }; + let Ok(relay) = RelayUrl::parse(relay.as_str()) else { + continue; + }; - entries.push(GroupListEntry { - id, - relay, - name: fields.get(3).cloned(), - private: false, - }); + entries.push(GroupListEntry { + id, + relay, + name: fields.get(3).cloned(), + private: false, + }); + } + // Rebuilt from the entries on publish. + "r" => {} + _ => foreign.push(tag.clone()), + } } let content = if event.content.is_empty() { @@ -710,6 +720,7 @@ impl GroupList { entries, readable: content.is_none(), content, + foreign, }) } @@ -777,7 +788,8 @@ impl GroupList { /// The tags of the public section of the list event. pub(crate) fn public_tags(&self) -> Vec { - let mut tags = Vec::with_capacity(self.entries.len() * 2); + let mut tags = self.foreign.clone(); + tags.reserve(self.entries.len() * 2); for entry in &self.entries { if entry.private { @@ -967,28 +979,41 @@ impl Activity { } #[derive(Debug, Clone, Default)] -pub(crate) struct TimelineWindow(Vec); +pub(crate) struct TimelineWindow { + events: Vec, + /// First 4 bytes of every event seen from this relay, + /// for `previous` reference checks. + seen: HashSet, +} impl TimelineWindow { pub fn new() -> Self { - Self(Vec::new()) + Self::default() } pub fn push(&mut self, event: Event) { - self.0.retain(|existing| existing.id != event.id); - let position = self.0.partition_point(|e| e.created_at > event.created_at); + self.seen.insert(prefix(event.id)); + self.events.retain(|existing| existing.id != event.id); + let position = self + .events + .partition_point(|e| e.created_at > event.created_at); - self.0.insert(position, event); - self.0.truncate(TIMELINE_WINDOW); + self.events.insert(position, event); + self.events.truncate(TIMELINE_WINDOW); + } + + /// Note an event as seen without keeping it in the window. + pub fn note(&mut self, event: &Event) { + self.seen.insert(prefix(event.id)); } pub fn remove(&mut self, id: EventId) { - self.0.retain(|event| event.id != id); + self.events.retain(|event| event.id != id); } pub fn previous_tag(&self, me: PublicKey) -> Option { let references: Vec = self - .0 + .events .iter() .filter(|event| event.pubkey != me) .take(PREVIOUS_REFS) @@ -1001,4 +1026,121 @@ impl TimelineWindow { Some(Tag::custom("previous", references)) } } + + /// Whether every `previous` reference points at an event seen from this + /// relay. `None` when the event carries no `previous` tag. Mirrors the + /// relay-side rejection the spec expects, so a relay handing out + /// out-of-context events can be called out. + pub fn references_seen(&self, tag: Option<&Tag>) -> Option { + let values = tag?.as_slice().iter().skip(1); + + for value in values { + let Ok(prefix) = u32::from_str_radix(value, 16) else { + return Some(false); + }; + + if !self.seen.contains(&prefix) { + return Some(false); + } + } + + Some(true) + } +} + +/// The first 8 hex characters of an event id, as a `previous` reference. +fn prefix(id: EventId) -> u32 { + let bytes = id.as_bytes(); + u32::from_be_bytes([bytes[0], bytes[1], bytes[2], bytes[3]]) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::LOCAL_KEYS; + + fn event(kind: Kind, tags: Vec) -> Event { + futures::executor::block_on( + EventBuilder::new(kind, "") + .tags(tags) + .finalize_async(&*LOCAL_KEYS), + ) + .unwrap() + } + + fn relay(url: &str) -> RelayUrl { + RelayUrl::parse(url).unwrap() + } + + #[test] + fn public_tags_preserve_foreign_tags() { + let event = event( + Kind::SimpleGroups, + vec![ + Tag::custom("group", ["pizza", "wss://relay.example", "Pizza Lovers"]), + Tag::custom("d", ["favorite-groups"]), + Tag::custom("client", ["nostord"]), + Tag::custom("r", ["wss://other.example"]), + ], + ); + + let mut list = GroupList::parse(&event).unwrap(); + assert_eq!(list.entries().len(), 1); + + list.upsert(GroupListEntry { + id: GroupId::new("sushi").unwrap(), + relay: relay("wss://relay2.example"), + name: None, + private: false, + }); + + let tags = list.public_tags(); + + // Foreign tags survive a republish... + assert!(tags.iter().any(|tag| tag.kind() == "d" + && tag.as_slice().get(1).map(String::as_str) == Some("favorite-groups"))); + assert!(tags.iter().any(|tag| tag.kind() == "client")); + + // ...while `group` and `r` tags are rebuilt from the entries. + assert_eq!(tags.iter().filter(|tag| tag.kind() == "group").count(), 2); + assert_eq!(tags.iter().filter(|tag| tag.kind() == "r").count(), 2); + } + + #[test] + fn previous_references_validate_against_seen_events() { + let mut window = TimelineWindow::new(); + let seen = event(Kind::ChatMessage, vec![Tag::custom("t", ["one"])]); + let other = event(Kind::ChatMessage, vec![Tag::custom("t", ["two"])]); + + window.push(seen.clone()); + + let reference = |id: &EventId| { + let bytes = id.as_bytes(); + Tag::custom( + "previous", + [format!( + "{0:02x}{1:02x}{2:02x}{3:02x}", + bytes[0], bytes[1], bytes[2], bytes[3] + )], + ) + }; + + assert_eq!( + window.references_seen(Some(&reference(&seen.id))), + Some(true) + ); + assert_eq!( + window.references_seen(Some(&reference(&other.id))), + Some(false) + ); + assert_eq!(window.references_seen(None), None); + + // Any number of references is allowed, including zero. + let empty = Tag::custom("previous", Vec::::new()); + assert_eq!(window.references_seen(Some(&empty)), Some(true)); + + // A malformed reference can't be verified. + let malformed = Tag::custom("previous", ["nothex"]); + assert_eq!(window.references_seen(Some(&malformed)), Some(false)); + } } diff --git a/crates/workspace/src/sidebar/group_view.rs b/crates/workspace/src/sidebar/group_view.rs index 8e5f5a40..01ae3f2e 100644 --- a/crates/workspace/src/sidebar/group_view.rs +++ b/crates/workspace/src/sidebar/group_view.rs @@ -254,11 +254,17 @@ impl Sidebar { ) -> AnyElement { let hide_avatar = AppSettings::get_hide_avatar(cx); let persons = PersonRegistry::global(cx); + let person = persons.read(cx).get(public_key, cx); + let group = group.clone(); + let public_key = *public_key; let member_is_admin = !roles.is_empty(); - let group = group.clone(); + let is_self = NostrRegistry::global(cx) + .read(cx) + .current_user() + .is_some_and(|me| me == public_key); Nav::new(public_key.to_hex()) .label(person.name()) @@ -294,7 +300,7 @@ impl Sidebar { cx.write_to_clipboard(ClipboardItem::new_string(npub)); })); - if !admin { + if !admin || is_self { return menu; } diff --git a/docs/TODO.md b/docs/TODO.md index 435b3697..08b47183 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -1,7 +1,6 @@ # TODO -Open items from the search and lazy-loading work. Each entry notes the known -tradeoff and where the fix would land. +Open items. Each entry notes the known tradeoff and where the fix would land. ## nip29 @@ -17,3 +16,24 @@ tradeoff and where the fix would land. gossip relays, but the deeper fix is a per-relay scoped subscription using an `a` tag with `10009:` coordinates. The exact coordinate format each relay accepts needs to be tested first. +- [ ] **LiveKit AV.** The `/.well-known/nip29/livekit` probe gates the edit + toggle, but the token flow (`/.well-known/nip29/livekit/` with NIP-98 + kind 27235) and a `kind:39004` participant subscription are missing — both + only matter once an AV client is on the roadmap. +- [ ] **Threads pane.** Replies to kind 11 messages send and render + (composer target + reply-context line), but there is no dedicated threads + view like nostord's. +- [ ] **NIP-11 `nip29.subgroups` gating.** The subgroup UI (parent edit, + child reorder) is always shown; relays that don't support subgroups reject + the 9002. Reading the relay information document would gate it. +- [ ] **Invite sync window.** The moderation subscription replays the last + 100 moderation events, so invites older than that window appear in the + list only once they enter the local envelope cache. +- [ ] **`previous` mismatches are advisory.** Incoming events whose + `previous` references weren't seen from the relay are logged + (`Group::absorb`) but still rendered: partial history makes dropping + false-positive-prone. Late-publication rejection remains relay-side, per + the spec. +- [ ] **Download stall watchdog.** The auto-updater's streamed download + (`auto_update::download`) relies on the HTTP client's defaults; a + mid-transfer stall hangs the progress loop instead of erroring out. diff --git a/docs/nip29-comparison.md b/docs/nip29-comparison.md deleted file mode 100644 index fb23d890..00000000 --- a/docs/nip29-comparison.md +++ /dev/null @@ -1,137 +0,0 @@ -# NIP-29 alignment: coop vs nostord - -Comparison of our NIP-29 implementation (`crates/nip29`, `crates/nip29_ui`, -`crates/workspace`) against [nostord](https://github.com/nostrord/nostrord) -(`main` @ `bfd933e`), checked against -[NIP-29](https://github.com/nostr-protocol/nips/blob/master/29.md). - -The two clients are architecturally similar: relay-hosted groups keyed by -`(relay, id)`, relay-generated `3900x` state events, user moderation events -`9000-9022`, and a NIP-51 `kind:10009` group list. They made different -trade-offs; each has real gaps the other closed. - -## TL;DR - -- We follow the spec more literally where nostord deviates: `previous` - timeline refs, membership-from-9000/9001 (the spec's "implementation - quirks" section), and fork/migration detection via admins' `kind:10009`. - nostord implements none of those three. -- nostord is ahead on product surface: full LiveKit AV (token endpoint + - `kind:39004`) and a threads pane. Invite code management, subgroup - reparent/reorder and reply chips landed here since the comparison. -- Spec bugs in ours worth fixing: none open. The `duplicate:` join - rejection, 9008 delete-group, reaction aggregation and the `39002`/`39001` - membership fallback were implemented. - -## 1. Protocol compliance vs NIP-29 - -| Spec item | coop | nostord | -| --- | --- | --- | -| `h` tag on all user→group events | yes, every builder (`crates/nip29/src/protocol.rs`) | yes | -| Moderation kinds 9000/9001/9002/9005/9007/9009 | yes, all | yes, all | -| **9008 delete-group** | yes, plus confirm dialog | yes | -| **9010 update-pin-list + 39005** | send + parse + UI | stub ("disabled until the pinning backend exists") | -| `previous` tag (last 50 seen, excl. self, ≥3 × 8 chars) | `TimelineWindow` (`crates/nip29/src/protocol.rs`) | not built, not validated | -| Membership = latest of own 9000/9001 | `Membership::from_events`, rosters resolve `Unknown` as fallback | derived from 39001/39002 rosters + local markers instead | -| Join request `code` tag / `naddr1…?invite=` | `GroupReference::parse` | yes, plus legacy formats (`wss://host'id`, `?relay=&group=`) | -| `duplicate:` 9021 rejection = already member | treated as success (`mark_member`) | matched by wording, treated as success | -| 39000 metadata: all flags + `supported_kinds` + `parent`/`child` | yes, plus banner and content-JSON fallback | yes, minus banner; tags only | -| 39001/39002/39003 state | parsed, admins used for actions | richer use (roster as membership source) | -| **39004 livekit participants + token endpoint (NIP-98 kind 27235)** | flag parsed/editable only | full flow incl. `/.well-known` support probe | -| Kind 10009 list | NIP-44 private section, public-wins, opaque-carry | plus foreign-tag preservation and relay-consensus freshness | -| Fork/migration detection (admins' 10009) | `watch_forks` + "may have moved" banner | none | -| Metadata author check (relay-signed 3900x) | parsed from any author | same | - -Both clients ignore "clients should also check [`previous` refs] to keep -relays honest" — fine, it is a SHOULD. NIP-42 auth: we configure -`SignerAuthenticator` on the SDK client (`crates/state/src/lib.rs`), nostord -waits for AUTH explicitly before REQs; both work on auth-gating relays. - -## 2. Where coop is ahead (keep these) - -1. **Fork/migration detection.** The spec says clients SHOULD (MUST when the - primary relay is down) watch admins' `kind:10009`. - `GroupsRegistry::watch_forks` / `evaluate_forks` does this on a 15-minute - cycle with a "may have moved" banner and a rejoin button. nostord has - nothing here. Known limitation (see `docs/TODO.md`): the fork-detection - subscription is unscoped, so NIP-29 group relays refuse it by policy; it - currently works via generic and gossip relays only. -2. **Membership per the spec quirk.** Latest own 9000/9001 decides, with - `Refused` / `Removed` states nostord lacks entirely. -3. **Pinning.** Full 39005/9010 round-trip, pin chips, admin toggle. -4. **Spec-shaped `previous` refs** and a metadata parser that tolerates - content-JSON variants. -5. **Discovery primitives.** Contact-list `10009` sweep plus per-relay - browse (`Discovery`, `GroupsRegistry::browse`). nostord deliberately has - no relay browsing (join by address only). The browse core exists even - though the UI only wires contact-based discovery. - -## 3. Gaps, prioritized - -### P1 — spec correctness / interop - -1. ~~**`duplicate:` join rejection**~~ **done.** A `duplicate:`-prefixed - publish failure on 9021 now marks the group membership as `Member` - (`GroupsRegistry::join_key` → `Group::mark_member`). -2. ~~**9008 delete-group**~~ **done.** `GroupId::delete_group` builder, - `Group::delete_group`, `GroupsRegistry::delete_group` (forgets the group - on accept), and an admin-only "Delete group" button with a confirm - dialog in `nip29_ui::actions`. -3. ~~**Reactions render as message bubbles.**~~ **done.** Kind-7 events no - longer become rows: `Group::absorb` routes them to `push_reaction`, which - aggregates them per target (`Group::reactions`); `nip29_ui` renders a - grouped reaction bar under the target message. Deleting an event drops - its reactions. - -### P2 — client-parity features - -4. ~~**Invite management.**~~ **done.** The group subscription now includes a - moderation filter (`9009` + `9005`, `GroupId::moderation_filter`), - `Group::invites()` exposes active codes (revocations arrive as 9005 - deletions and via the publisher's own `Sent` path), and the invite dialog - lists codes with per-code copy-link and revoke. -5. ~~**Membership fallback to rosters.**~~ **done.** `Group:: - adopt_roster_membership`: while the latest own 9000/9001 leaves membership - `Unknown`, being listed in 39001/39002 counts as `Member`. `Pending`, - `Refused` and `Removed` are never overridden. -6. **LiveKit — partially done.** The edit dialog probes - `/.well-known/nip29/livekit` (expecting 204) and gates the toggle with a - description when the relay doesn't announce support. Remaining: the token - flow (`/.well-known/nip29/livekit/` with NIP-98 kind 27235) and a - `kind:39004` participant subscription — both only useful once an AV - client is on the roadmap. -7. ~~**Subgroup management UI.**~~ **done.** The edit dialog has a Parent - field (empty = root; reparenting) and a children list with Up/Down - reordering (the spec's 9002 child-list replace semantics). Self-parent and - parent-is-child are rejected client-side; NIP-11 `nip29.subgroups` is not - read yet. -8. **Reply UI — partially done.** The composer can target a reply - (context menu → Reply, chip with cancel), sends kind 11 with `e` tags, and - messages render a reply-context line (author + first line) resolved from - loaded rows. Remaining: a dedicated threads pane (nostord has one). - -### P3 — polish - -9. **10009 foreign tags.** We rebuild only `group`/`r` tags on publish - (`GroupList::public_tags`), dropping anything other clients stored in the - event; nostord preserves unknown tags verbatim. Cheap fix, avoids - clobbering. -10. **No `previous` validation on incoming events** (spec SHOULD) and no - client-side late-publication guard — mostly relay-side concerns. -11. **Self-demotion guard.** Member roster UI lives in the sidebar - (`crates/workspace/src/sidebar/group_view.rs`: promote/demote/remove) at - parity with nostord minus roles beyond `admin` (both clients only assign - one role). nostord additionally blocks self-demotion/self-removal — - worth copying to avoid locking yourself out. - -## 4. Suggested order of work - -1. ~~Fix `duplicate:`-prefix handling on 9021~~ done. -2. ~~Exclude/aggregate kind-7 reactions in group rows~~ done. -3. ~~Add 9008 delete-group + confirm dialog~~ done. -4. ~~Subscribe 9009 → invite list + revoke via 9005~~ done. -5. ~~39002/39001 membership fallback~~ done. -6. ~~Subgroup reparent/reorder in the edit dialog~~ done. -7. LiveKit probe (done, gates the toggle) + token flow (larger; only if AV is - in scope). -8. Reply sending + chips (done); a dedicated threads pane remains.