Repository navigation
feat(relay): port the wire handlers, strategies and rate limits - #947
Conversation
The table stated one case as the level's property. The settings ship six kind-scoped 60 s windows plus the global hour one, and the key carries the selector set, so an author holds 1, 2 or up to 7 keys and the store fills at between ~9,400 and 65,536 authors. The closure follows the kinds too: 61 s for a single-kind flood, 1 h + 1 s for kinds that match no kind window. Every closure cell also gains the sweep interval: a lapsed key is removed only by a pass, so the bound is window + up to SWEEP_INTERVAL_MS, not the window alone.
The enumeration left out 446-9999 and 40000-65535, which matter because the probe result two lines below uses kind 9999 -- a kind the list did not cover. It is now the exact complement of the six shipped windows.
🦋 Changeset detectedLatest commit: 74ea36e The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
The changeset check requires a file whenever a package directory changes, and this branch changes only Rust crates, so there is nothing to release. Empty frontmatter declares exactly that.
[High impact] The PR changes the client-facing behavior of the Rust relay across its WebSocket connections. Fix subscription cancellation and outbound queue growth before merging. Findings
|
| pub fn end_read(&self, subscription_id: &str) { | ||
| self.in_flight | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()) | ||
| .remove(subscription_id); |
There was a problem hiding this comment.
When a client replaces an unfinished REQ with different filters under the same ID, begin_read installs the new sender and cancels the old read. The old read then calls end_read, which removes the new sender. The new read treats that dropped sender as cancellation and can stop without EOSE.
Give each read an ownership token and remove the entry only when that token still matches.
Knowledge Base Used: Nostr websocket protocol
| maybe_issue_invite_code(&subscription_id, &filters, context).await; | ||
|
|
||
| fetch_and_send(&subscription_id, &filters, context).await; |
There was a problem hiding this comment.
Closed subscriptions send more frames
REQ cancellation starts only after maybe_issue_invite_code finishes. If an authenticated invite request is waiting in invites.create when CLOSE arrives, abort_read finds no read to cancel. When the write finishes, the handler still sends the invite event and starts a stored read, producing frames after the subscription was closed.
Register cancellation before the first await and check it before sending or starting more work.
Knowledge Base Used: Nostr websocket protocol
| /// Creates the queue and its sink. The receiver is drained by the connection task. | ||
| pub fn channel() -> (Self, mpsc::UnboundedReceiver<String>) { | ||
| let (sender, receiver) = mpsc::unbounded_channel(); | ||
|
|
||
| (Self::new(sender), receiver) |
There was a problem hiding this comment.
FrameSink queues encoded frames without a size bound or any way to slow the producer. A valid stored read can enqueue thousands of large events while its client reads slowly. Once the socket write blocks, the connection loop also stops checking liveness, while the separately spawned read keeps filling this queue.
Bound queued bytes and either pause reads or disconnect clients that cannot keep up.
| // event"; it is asked per event, as the capture's | ||
| // `createReadAuthorizationGuard` is, so an AUTH that lands mid-read applies to | ||
| // the rest of the stream. | ||
| if expiration::is_expired_event(&event, now) |
There was a problem hiding this comment.
The stored read checks every event against the time captured before the loop. If an event expires while an earlier record is being fetched or sent, it still passes this check and is delivered after expiry. Read context.now_secs() for each event, as the TypeScript path does.
Knowledge Base Used: Nostr websocket protocol
| if let Some(max_id_length) = limits.and_then(|limits| limits.max_subscription_id_length) | ||
| && subscription_id.len() as u64 > max_id_length |
There was a problem hiding this comment.
Subscription IDs use three different length rules: decode_request counts Unicode characters, while REQ and COUNT limits count UTF-8 bytes. TypeScript counts UTF-16 units throughout. With the shipped 256 limit, an ID of 100 Chinese characters passes decoding but is rejected by both handlers because it occupies 300 bytes, although TypeScript accepts it.
Use encode_utf16().count() consistently and add non-ASCII cases.
| "test": "crates/nostream-relay/tests/handlers.rs::a_restricted_count_needs_a_scoped_authors_filter", | ||
| "titles": [ | ||
| "CountMessageHandler handleMessage() returns CLOSED when counting restricted kinds scoped to another pubkey", | ||
| "CountMessageHandler handleMessage() returns CLOSED with auth-required when counting restricted kinds unauthenticated", | ||
| "CountMessageHandler handleMessage() returns COUNT when restricted kinds are scoped to the authenticated client" |
There was a problem hiding this comment.
Census overstates test coverage
These mappings claim coverage that the named test does not provide. a_restricted_count_needs_a_scoped_authors_filter sends only an unauthenticated, unscoped COUNT; it never authenticates or tests another author's scope or an allowed scope. Yet all three cases count as mapped.
Add those inputs and assertions before counting them as carried. The same problem appears in the AUTH mappings for the exact timestamp boundary and invalid relay URL.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| }) | ||
| .unwrap_or_default(), | ||
| advance_ms: value["advance_ms"].as_u64().unwrap_or(0), | ||
| none: value["send"].is_null() && value["drain"].is_null() && value["connect"].is_null(), |
There was a problem hiding this comment.
Replay misses unexpected replies
Recorded silent steps have a send object and an empty expect, so this condition always sets none to false for them. collect then reads zero frames and returns immediately. In particular, the recorded CLOSE in req-close-abort is never checked for silence. The reader also stops at the expected frame count, leaving extra final frames unchecked.
Detect silent steps from their expectations and check for unexpected frames before ending a session.
What this delivers
nostream-relay's wire core: the relay now answers a real client end to end in Rust — connectioncontrol,
EVENTingestion through the strategies and rate limits,REQ/CLOSEsubscriptions withstreaming
EVENT/EOSE,AUTH(NIP-42) with restricted reads,COUNT,OK/NOTICE/CLOSEDmessaging, and the per-level limiter the capture implements.
Job 008 of the nostream 4.0 rewrite (umbrella #860, issue #865). It is the first slice of #865:
the wire core and its parity harness. The remaining fourteen strategies are job 009; NIP-11 is
nostream-admin's, NIP-66 isnostream-services'.Parity is measured, not asserted. Two oracles run as tests:
ts_case_parity— 557 cases from the TypeScript census, 515 mapped, 42 exempt by name; 462 ofthem execute against the Rust handlers in this job, the rest belong to job 009, admin, services
and broadcast.
wire_parity— 16 captured sessions, 78 steps replayed against a live relay, frames comparedbyte for byte, one declared tolerance.
Review history
Five rounds, five lenses in rounds 1–4 and architecture only in round 5. Round 1 found real
defects (a limiter key leak of 6,968 kB over 30k pubkeys, a nondeterministic oracle). Rounds 2–4
closed them and fixed a per-record
println!that stalled aREQat 4,000 frames. The last threerounds' remaining findings were prose only — the ADR's limiter table, two doc comments and
build.md's record — landed separately asd73abfband4f8ac67, each checked againstlimits.rsanddefault-settings.yamlrather than against the builder's report.Reviewed by
chappie-daemon.Test evidence at
4f8ac67cargo test --workspace --all-featurescargo fmt --checkcargo clippy(default and-D warningsprofiles)cargo build --releasewire_parityts_case_parityShape of the diff
The base is unchanged at
471d439and the branch is zero behind it, so this diff is the whole joband nothing else — no rebase noise, no unrelated commits.