feat(rpc): add ?topics= filtering to the events stream#518
Conversation
🤖 Kimi Code ReviewThe PR introduces topic-filtered SSE subscriptions with a bitmask-based filter ( Critical: Potential overflow in
self.0 & (1 << topic as u16) != 0If the Suggestion: Add a compile-time assertion or document the invariant: impl TopicSet {
// ...
pub fn contains(self, topic: Topic) -> bool {
let discriminant = topic as u16;
debug_assert!(discriminant < 16, "Topic discriminant overflow");
self.0 & (1 << discriminant) != 0
}
}Alternatively, use Minor: Whitespace handling in topic parsing
list.split(',').map(Topic::from_str).collect()This does not handle whitespace around commas. A request to Suggestion: Trim whitespace: list.split(',').map(str::trim).map(Topic::from_str).collect()Minor: Unbounded string in error variant
Code quality observations
Documentation The divergence from the Beacon API (optional vs mandatory Summary Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo findings. The PR is narrowly scoped to SSE event delivery and topic parsing in crates/blockchain/src/events.rs and crates/net/rpc/src/events.rs. I did not see correctness, security, or performance regressions in the touched paths. The filtering remains outside consensus execution, so fork choice, attestation validation, justification/finalization, state transition, XMSS, and SSZ logic are unaffected by this change. Verification gap: I could not run the Rust tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR adds topic filtering to the chain-event SSE endpoint. The main changes are:
Confidence Score: 5/5The latest changes look safe to merge.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/events.rs | Adds topic parsing, topic bitsets, and filtered event subscriptions while preserving lag errors. |
| crates/net/rpc/src/events.rs | Adds query parsing, validation, filtered SSE streaming, and handler coverage. |
| crates/blockchain/src/lib.rs | Exports the new subscription types and updates existing tests to request all topics. |
| crates/net/rpc/Cargo.toml | Replaces the previous stream dependencies with futures-util. |
| docs/rpc.md | Documents accepted topics, response statuses, and unfiltered defaults. |
Reviews (3): Last reviewed commit: "feat(rpc): add ?topics= filtering to the..." | Re-trigger Greptile
🤖 Claude Code ReviewNot related — that's gossipsub topics, a separate concept. I have enough to write the review now. Review:
|
🤖 Kimi Code ReviewThe PR introduces topic-filtered SSE subscriptions with a bitmask-based filter ( Issues & Suggestions:
Security: No concerns. Input validation correctly rejects unknown topics with 400. Performance: The design trades buffer space for simplicity (filtered events still occupy broadcast slots). This is acceptable per the documented "best-effort" contract, but ensure Consensus Safety: No consensus logic touched; this is purely RPC/telemetry. Verdict: Approvable once the 16-topic limit is documented or guarded (Item 1). The rest are minor nits. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
Otherwise this looks reasonable. The PR stays out of fork choice / STF / attestation / SSZ paths, and I did not see any immediate correctness or security regressions in the SSE filtering flow. I couldn’t run the Rust tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview:
|
bb2acdd to
8bc49c0
Compare
c139bc2 to
0b53690
Compare
🤖 Codex Code ReviewFindings:
No consensus, fork-choice, attestation, STF, XMSS, or SSZ logic is touched in this PR. Aside from the two points above, the SSE filtering change looks reasonable and the error-path/tests are in decent shape. I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewOverall Assessment: Solid PR. Clean implementation of server-side topic filtering for SSE events with proper error handling and good test coverage. No consensus-critical bugs (this is RPC-layer only). Issues & Suggestions: 1. Potential overflow in Recommendation: Add a compile-time assertion or document the constraint: // In TopicSet impl:
const _: () = assert!(std::mem::variant_count::<Topic>() <= 16, "TopicSet cannot hold more than 16 topics");2. Silent event dropping on serialization failure (crates/net/rpc/src/events.rs:65-71) 3. 4. Allocation on unknown topic error (crates/blockchain/src/events.rs:58) 5. Dependency version consistency (crates/net/rpc/Cargo.toml:29) Code Quality Positives:
Consensus Safety Note: Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewI have enough context to complete the review without building. Here's my analysis: Review:
|
0b53690 to
6a11257
Compare
50050d2 to
03118fc
Compare
65c367f to
252b760
Compare
Adopt #517's beacon-aligned ChainEvent ({slot, block, state}, Serialize derived on the type) while keeping #518's TopicSet/EventSubscription filtering. The snapshot tests moved from lib.rs into events.rs upstream; take that version and add TopicSet::ALL to their subscribe calls. Update the topic-filter tests to the new field names, and switch the "block filtered out" assertion to check the SSE event: line since every payload now carries a `block` field.
Adds server-side `?topics=` filtering to `GET /lean/v0/events`. The comma-separated list of event names is required (Beacon-API-aligned): a missing, empty, or unknown topic is a 400 that names the offending value. Filtering lives in the SSE consumer, not the bus: the handler parses the selection into a `Vec<Topic>` and skips events outside it. The stream is driven by `futures_util::unfold` over the broadcast receiver directly (dropping the tokio-stream wrapper), and a lagged subscriber now receives a Lighthouse-compatible `: error - dropped N messages` comment frame before the stream continues. `Topic` gains a `FromStr` inverse of `as_str` with a `thiserror` `UnknownTopic` error. Documented in docs/rpc.md.
fd6602c to
585b553
Compare
🤖 Kimi Code ReviewThe PR correctly implements mandatory topic filtering for the SSE events endpoint, matching Beacon API semantics. The switch from Minor notes:
Correctness verification:
Approval: LGTM. The code is idiomatic, well-tested, and handles edge cases (slow clients, bad inputs) appropriately. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
Other than that, the patch is localized to the RPC SSE surface; I did not see consensus/fork-choice/state-transition regressions in the touched code. I could not run the Rust tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have enough context for a thorough review. Review: PR 518 —
|
Adds Beacon-API-style server-side filtering to
GET /lean/v0/events:?topics=takes a required comma-separated list of topic names. As in the Beacon APIeventstreamendpoint,topicsis mandatory: a missing, empty, or unknown value returns 400 (the body names the offending topic). There is no "subscribe to everything" default. Seedocs/rpc.md.Filtering lives in the SSE handler, the sole consumer that needs it, rather than in the bus.
EventBus::subscribe()stays a plain fan-out (broadcast::Receiver); the handler parses the requested topics into aVec<Topic>and skips unmatched events in its ownrecvloop. Lag is surfaced to the handler, never swallowed. Topic-name parsing (FromStr for Topic,UnknownTopic) stays in the blockchain crate, where the orphan rule requires it. The handler bridgesrecvto a stream viafutures_util::stream::unfold, replacing thetokio-stream/futures-corepair withfutures-util(already in the lock transitively).On
Lagged(n)the stream also yields an: error - dropped n messagesSSE comment frame (wire-compatible with Lighthouse's lagged-client marker) instead of only logging, so clients get an explicit re-sync trigger.Stacked on #517.
Has unit and handler tests (including the missing/empty and unknown-topic 400 paths) and passes clippy with
-D warnings.