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 —
|
Continues the chain-event pub-sub series (#516 / #517 / #518, all merged). Adds the beacon `block_gossip` analog. ## Event added | Topic | Payload | Emitted when | |-------|---------|--------------| | `block_gossip` | `{ slot, block }` | A block is seen on the network, before import | ## Notes - Emitted from the blockchain actor's `NewBlock` handler, **not** a second P2P-side publisher, so the actor stays the sole event publisher and the write flow stays one-directional (a core requirement of the series' design). - Fires for both gossip'd and by-root-fetched blocks, since both re-enter through `NewBlock`. Import may pend the block while its parent chain is fetched, so this precedes import. - Ungated like `block` (not recency-gated), so subscribers can watch sync progress. Low-rate (one per block), so the payload is built unconditionally behind `emit`'s no-subscriber guard. ## Testing `cargo fmt`, `cargo clippy -p ethlambda-blockchain -p ethlambda-rpc` (clean), blockchain lib + rpc events tests pass. --- **Independent PR, based off `main`.** One of three sibling PRs continuing the series — alongside #533 (`chain_reorg`/`safe_target`) and #534 (`attestation`/`aggregate`), each independently based off `main`. They touch overlapping regions of `events.rs` / `docs/rpc.md`, so whichever two merge later will each need a small conflict rebase. Opened as draft.
Continues the chain-event pub-sub series (#516 / #517 / #518, all merged). Adds the two high-rate topics. ## Events added | Topic | Payload | Emitted when | |-------|---------|--------------| | `attestation` | `{ validator_id, data: AttestationData }` | A single validator vote passes gossip validation (signature omitted) | | `aggregate` | `{ participants: [u64], data: AttestationData }` | A committee-signature aggregate is produced locally or accepted from gossip (proof omitted) | Both fire only for messages the store **accepted** (data + signature validation), so subscribers see the same votes fork choice does. A node never receives its own aggregate back over gossip, so `aggregate` fires at two sites: the locally produced `AggregateProduced` path and the gossip-received path. ## Design points - The ~3 KB XMSS signature and the SNARK proof bytes are deliberately **omitted** — too heavy for a high-rate stream. - The shared broadcast channel capacity is bumped **256 → 8192**. All topics share one ring buffer, so a subscriber's tolerable stall is `capacity / total_event_rate` (dominated by the attestation rate), not per-topic. A per-topic split behind the `EventBus` facade remains the escape hatch if real devnet rates show the shared window biting — see the note in `docs/rpc.md`. - Emission uses the plain, no-subscriber-guarded `emit`. Consequence: the per-vote `AttestationData` is cloned even when nobody is subscribed (the guard only skips the send). If that actor-hot-path cost proves material, a payload-deferral helper can be added in a follow-up. ## Testing `cargo fmt`, `cargo clippy -p ethlambda-blockchain -p ethlambda-rpc` (clean), blockchain + rpc lib tests pass, including `events_streams_attestation_with_nested_data` (verifies the untagged `data:` nesting over the wire). --- **Independent PR, based off `main`.** One of three sibling PRs continuing the series — alongside #533 (`chain_reorg`/`safe_target`) and #535 (`block_gossip`), each independently based off `main`. They touch overlapping regions of `events.rs` / `docs/rpc.md`, so whichever two merge later will each need a small conflict rebase.
## Summary Adds `tooling/event-monitor/`, a standalone live arrival-time dashboard for lean-consensus (ethlambda) nodes. It dials the `GET /lean/v0/events` SSE stream of several nodes, timestamps each event on a single collector clock, and serves a browser dashboard visualizing: - a **rolling beeswarm** of each event's arrival offset *within the slot*, one lane per node, and - a **propagation-delta beeswarm**: for a given block / aggregate, how long after the *first* node each other node saw it. The rolling window is adjustable live from the header, and a fresh page load backfills recent history from the collector so it is never blank. ## Design - **Standalone Cargo workspace** under `tooling/event-monitor/` (empty `[workspace]` table); it is not a member of the parent `ethlambda` workspace and does not affect the main build. - **Zero dependency on any ethlambda crate** — it only speaks the documented SSE/HTTP wire shape. `CONTRACT.md` is the frozen interface between the Rust/axum backend (`src/`) and the vanilla-JS frontend (`web/`, no build step). - A single collector clock makes propagation deltas skew-free across nodes. - `?demo=1` runs the frontend fully offline with synthetic data. This PR contains the tooling plus the CI wiring needed to actually check it. The RPC / event-bus changes that make a node emit these events shipped separately in the chain-events series (below). ## Required ethlambda API functionality All dependencies are **satisfied on `main`** — the default dashboard view works against a current node with no extra PRs. | Capability | Endpoint / topic(s) | Status | |---|---|---| | SSE events stream | `GET /lean/v0/events` | ✅ #517 | | Topic filtering | `?topics=<csv>` | ✅ #518 | | Slot-geometry bootstrap | `GET /lean/v0/genesis`, `GET /lean/v0/config/spec` | ✅ | | Base topics | `block`, `head`, `justified_checkpoint`, `finalized_checkpoint` | ✅ #516 | | Attestation / aggregate topics | `attestation`, `aggregate` | ✅ #534 | | Block-gossip topic | `block_gossip` | ✅ #535 | The collector accepts `block`, `block_gossip`, `head`, `justified_checkpoint`, `finalized_checkpoint`, `attestation` and `aggregate`; any other topic is logged and skipped. `safe_target` / `chain_reorg` (#533, closed) were removed in 4eeafff — both panels' topic filters discarded them, and `chain_reorg` is a point-in-time event rather than a per-node arrival race, so it does not belong on a beeswarm. Both panels default to `block` / `attestation` / `aggregate`. ## Review fixes Three defects found in review, each with a regression test: - **Reconnect backoff never reset after a healthy session.** The reset was keyed on a clean end-of-stream, but a node restart surfaces as a stream *error*, so every restart ratcheted the delay one step permanently; after ~7 of them a healthy node reported `down` with 10s reconnects. Now keyed on session duration (survived ≥1 heartbeat). This also closes the inverse case: a peer that accepts the request and instantly closes the stream used to be retried at `INITIAL_BACKOFF` forever. - **One bogus slot blanked the dashboard.** Both the history ring and the rolling window key retention off the highest slot seen, and that watermark only moves up, so a single event from a node on a different genesis aged out every real event until a restart. Events more than `MAX_FUTURE_SLOTS` ahead of the collector's own slot are now dropped, warning once per node per connection. Only the future side is bounded; old slots are legitimate (`finalized_checkpoint` trails head) and cannot move the watermark. - **Stale status clobbered fresh status.** The frontend applies `status` immediately but buffers `chain` during backfill, so the `/api/history` snapshot could overwrite a newer live status. Live status now wins. Also from that review: - **Slot geometry is re-resolved every 60s** and republished when it changes, dropping the retained history and slot watermark, so a regenerated genesis no longer silently corrupts every subsequent `offset_ms`. Collectors re-read per frame and `/api/meta` per request; an already-open tab needs a reload to pick up new geometry. - **The arrival axis now spans two slots at two scales**: the first at full resolution (90.9% of the width at 5 intervals), the next compressed into a tinted band half a first-slot interval wide, saturating beyond that. A block spilling past its slot boundary is the failure mode worth seeing, and it used to be indistinguishable from one landing exactly on the boundary. - **`offset_ms` includes the collector↔node round trip.** One clock is what makes propagation deltas skew-free, but nodes reached over different links carry a systematic offset that reads as lag. Now documented in `README.md` and `CONTRACT.md §2`. - History events are `Arc`-shared, so `publish_chain` no longer deep-copies per event and `/api/history` no longer clones up to `HISTORY_MAX_EVENTS` events while holding the mutex. - `topics = []` / `nodes = []` are rejected at load rather than becoming an opaque retry loop against a 400. Known and deliberate, left as follow-ups: the collector cannot see upstream `: error - dropped N messages` comments (`eventsource-stream` swallows them), so a lagging subscription silently under-samples; both canvases still repaint unconditionally at 60fps; and `bootstrap` has no retry, so the monitor exits if started before its nodes are listening. ## CI `tooling/event-monitor` declares its own `[workspace]`, so `cargo fmt --all`, `cargo check --workspace`, `cargo clippy --workspace`, `make lint` and `make test` all stop at the root workspace members and never reached it — it landed with nothing verifying it. The `Lint` job now also runs `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` and `cargo test` under `working-directory: tooling/event-monitor`, and `rust-cache` lists it as a second workspace so its target dir is cached and its `Cargo.lock` feeds the cache key. ## Testing - `cargo test` in `tooling/event-monitor/`: **41 passed** (39 lib + 2 integration), 0 failed. - `cargo fmt --check` and `cargo clippy --all-targets -- -D warnings`: clean. - `?demo=1` offline synthetic mode renders both panels with no live nodes. - Verified end-to-end against a live local devnet (blocks land ~0.1–0.2 s into the slot, attestations ~0.8 s = interval 1). - Two-slot axis geometry checked numerically: the overflow band is exactly 0.5 × one first-slot interval, the mapping is monotonic, and the endpoints land on the margins. - Geometry refresh checked against a stub node: rewriting its `genesis_time` mid-run is picked up within one refresh interval (`WARN slot geometry changed`), `/api/meta` then serves the new epoch, and the history ring holds the restarted chain's low slots with in-slot offsets — which only works because the watermark is reset, otherwise they would prune as older than the previous high-water mark. ## Run ```bash cd tooling/event-monitor cp config.example.toml config.toml $EDITOR config.toml # list your nodes' RPC URLs + topics cargo run --release -- --config config.toml # open the `listen` address (default http://127.0.0.1:8080) ```
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.