Skip to content

feat(blockchain): add chain-event bus (head/block/justified/finalized)#516

Open
MegaRedHand wants to merge 7 commits into
mainfrom
feat/chain-events-bus
Open

feat(blockchain): add chain-event bus (head/block/justified/finalized)#516
MegaRedHand wants to merge 7 commits into
mainfrom
feat/chain-events-bus

Conversation

@MegaRedHand

@MegaRedHand MegaRedHand commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Adds a chain-event pub-sub mechanism to the blockchain actor: a ChainEvent enum (head, block, justified_checkpoint, finalized_checkpoint) published through an EventBus facade over a bounded tokio broadcast channel. The actor is the sole publisher; subscribers attach their own receivers, and a slow consumer skips missed events instead of back-pressuring the actor. Near-zero cost with no subscribers (receiver_count guard). No HTTP surface in this PR: the RPC consumer arrives in the next PR of the series.

Emission is actor-layer only, not threaded through the store. Before each state-changing store call (store::on_tick, store::on_block) BlockChainServer snapshots (head, latest_justified, latest_finalized), runs the store function completely unchanged, then diffs the snapshot against the post-call state and emits in order blockheadjustified_checkpointfinalized_checkpoint. store.rs, both spec-test files, and test_driver.rs stay byte-identical to main: no Option<&EventBus> parameter spreads across on_tick/on_block/update_head or any of their test call sites, and the sole-publisher property is structural (only BlockChainServer ever holds a live EventBus) rather than something every call site has to preserve by convention.

Two consequences of diffing at this level, both accepted:

  • block needs a was-it-new guard: on_block returns Ok early for an already-imported root, so the actor checks block-known-ness (the same has_state check the store itself uses) before importing, and only emits block for a genuinely new root.
  • Several head moves inside a single tick or block import coalesce into one head event carrying the net move; fine for subscribers, since the beacon-API analog is also last-value-wins per slot.

The head event is additionally recency-gated: it only fires when the new head is within HEAD_EVENT_RECENCY_SLOTS (32) of the wall-clock slot, so catch-up/backfill never spams head events (adopted from Lighthouse's head-SSE recency filter); the other events stay ungated.

Payload serialization is untagged, so the JSON body stays flat and topic names live only on the transport's event: line.

Supersedes #460, re-cut as a stacked series for easier review:

  1. this PR: the event mechanism
  2. GET /lean/v0/events SSE endpoint (all events)
  3. ?topics= server-side filtering
  4. chain_reorg + safe_target events
  5. high-rate attestation/aggregate events

Has unit tests and passes clippy with -D warnings.

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. crates/blockchain/src/store.rs and crates/blockchain/src/lib.rs: the proposer prebuild path suppresses events by using EventBus::disabled() while it calls on_tick(..., true, ...) and accept_new_attestations(...). Those calls can legitimately advance fork choice and finalization before the block is built. The later interval-0 tick is explicitly skipped once the store clock has already been advanced, so subscribers can miss a real head / finalized_checkpoint transition entirely, or only see the later block event. For a consensus event stream, that is a correctness bug. The fix should be to use the real bus for that catch-up path, or buffer and flush those emissions in publication order instead of dropping them.

No other material correctness, security, or Rust-pattern issues stood out in the diff itself.

I could not run cargo test here because the sandbox prevents rustup from creating temporary files under its read-only home.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

Overall this is a clean, well-architected PR. The event bus design correctly prioritizes consensus liveness over delivery guarantees, and the integration points are surg


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds chain-event publishing to the blockchain actor. The main changes are:

  • A bounded broadcast bus for block, head, justified, and finalized events.
  • Snapshot-based event detection around tick and block processing.
  • Flat serializable event payloads with separate topic names.
  • Event-bus construction and actor wiring in the node binary.

Confidence Score: 5/5

No additional blocking issue was found in this follow-up.

  • The latest event-bus wiring and dependency changes are consistent.
  • No separate issue remained after applying the fixes described by the existing checkpoint-event findings.

Important Files Changed

Filename Overview
crates/blockchain/src/events.rs Adds event types, topic mapping, the broadcast bus, snapshot diffing, and focused unit tests.
crates/blockchain/src/lib.rs Publishes state changes around tick and block processing and exposes the event API.
bin/ethlambda/src/main.rs Constructs the event bus and passes it to the blockchain actor.
crates/blockchain/Cargo.toml Moves Serde into runtime dependencies for event serialization.

Reviews (4): Last reviewed commit: "Merge branch 'main' into feat/chain-even..." | Re-trigger Greptile

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review: PR 516 — chain-event bus

Overall this is clean, well-scoped work. Threading &EventBus explicitly (never Option) through on_tick/on_block/update_head is a good call — it keeps the sole-publisher invariant visible in signatures, and the receiver_count() == 0 guard genuinely makes this free until the SSE endpoint (next PR) attaches a subscriber. The untagged serialization with out-of-band topic correctly fixes the double-tagging bug referenced from #460. Event ordering (blockjustified_checkpointheadfinalized_checkpoint) matches what's implemented and is called out clearly in both the PR description and code comments. Test coverage for the bus itself (no-subscriber no-op, disabled-bus no-op, flat JSON shape, topic mapping) is solid.

Three points worth a look, none blocking:

  1. Minor DB-read duplication in update_head (crates/blockchain/src/store.rs:84, :117) — the header for new_head is now fetched once to build the Head event (to get slot/parent_root) and then fetched again a few lines later purely for the info! log's new_slot. Could reuse the Option<BlockHeader> already fetched for the event instead of re-querying storage on every reorg.

  2. get_proposal_head hard-codes EventBus::disabled() (crates/blockchain/src/store.rs:933) rather than accepting events from its caller. The comment's rationale (don't surface a head move before the block it's building even exists) is reasonable, and in the current single-threaded actor it's provably safe since no real time passes between the interval-4 tick and this catch-up call. But it means any future refactor that introduces a gap between those two ticks would silently drop head/finalized transitions with no signal that a drop occurred. Worth at least a debug!/metric if this path is ever expected to observe a real transition, or a stronger comment noting the invariant this relies on (no intervening attestation/tick between interval-4's own accept_new_attestations and this one).

  3. ChainEvent's untagged shape is structurally ambiguous across Block/JustifiedCheckpoint/FinalizedCheckpoint (crates/blockchain/src/events.rs:34) — all three are {slot, root}. Fine today since only Serialize is derived and the topic always travels alongside it, but worth a short comment directly on the enum (not just the module doc) warning future authors that Deserialize must never be derived here without either a discriminant or requiring the caller to consult Topic first — untagged deserialization would silently misresolve a finalized_checkpoint payload as a Block otherwise.

No fork-choice/justification/finalization logic changes here — the PR is additive observability around existing store semantics, and the diffs read faithfully against what's actually mutated (e.g., the justified_checkpoint emission site choice in on_block_core is correctly justified by update_head never advancing latest_justified itself).


Automated review by Claude (Anthropic) · sonnet · custom prompt

Comment thread crates/blockchain/src/store.rs
Comment thread crates/blockchain/src/store.rs Outdated
@MegaRedHand
MegaRedHand marked this pull request as draft July 15, 2026 14:05
@MegaRedHand
MegaRedHand marked this pull request as ready for review July 15, 2026 14:07
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

Overall this is a clean, well-documented addition that correctly implements a best-effort event bus without affecting consensus safety. A few specific notes:

Code correctness

  1. Misleading expect with defensive check (store.rs:84-96)
    You .expect("block header exists") on the query, but then wrap the success case in if let Some. If the header is truly expected to exist, the if let is unreachable; if it might be missing, the expect is too strong. Either change the message to expect("store query should succeed") or remove the expect and handle the Err case explicitly with a warning.

  2. Justification event coverage gap?
    The comment in on_block_core states it's "the only production site where latest_justified changes", but justification/finalization in the spec happens in process_slots (called from on_tick). If ethlambda advances justified checkpoints during empty slot processing, those events will be missed. Verify whether store.process_slots can update the justified checkpoint; if so, emit the event there as well.

Safety & performance

  1. Event emission in hot paths
    emit is non-blocking and ignores send errors, which is correct for a best-effort bus. The `receiver

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. High: proposer-side prebuild can advance canonical fork choice without emitting the new events, so the stream becomes incomplete on slots where this node proposes. propose_block() runs at end-of-slot and calls produce_block_with_signatures(), which reaches get_proposal_head(); that helper advances the store to the next slot and promotes pending attestations, but it does so with EventBus::disabled() in store.rs. Those calls can move head and latest_finalized via on_tick() / accept_new_attestations() at store.rs and store.rs. The real interval-0 tick is then intentionally skipped after prebuild in lib.rs and lib.rs, so those head / finalized_checkpoint changes are never emitted later. For an SSE consumer, that means missed fork-choice/finalization updates exactly on local proposer slots.

Assumptions / gaps

  • I could not run the crate tests in this environment: cargo needs to write under /home/runner/.cargo / fetch leanVM, which is blocked by the sandbox. This review is from static inspection only.

Summary

Aside from Point 1, the event plumbing looks disciplined: single-writer bus, non-blocking broadcast, and the block -> justified -> head -> finalized ordering in on_block_core() is sensible.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review: feat(blockchain): add chain-event bus (head/block/justified/finalized)

Overall this is a clean, well-scoped first PR in the series. The facade design (bounded broadcast channel, sole-publisher actor, receiver-count guard, &EventBus-never-Option threading, disabled() for tests) is sound and the emission ordering (block → justified_checkpoint → head → finalized_checkpoint) matches the PR description and the code. Tests cover the bus mechanics and the untagged-serialization invariant well.

1. Proposer nodes silently drop a Head event on their own proposing slot (crates/blockchain/src/store.rs:927-945, consumed from lib.rs:699-704)

get_proposal_head runs on_tick + accept_new_attestations against &mut self.store — the live chain state, not a scratch copy — but wires them to a local EventBus::disabled(). If promoting attestations at the target slot's interval 0 moves the fork-choice head (a real, persisted change), that Head/FinalizedCheckpoint transition is dropped. Because on_tick's advance loop is gated on store.time() < time (store.rs:368), the real interval-0 tick that runs later on the actor's own self.events bus finds the clock already caught up and never re-executes accept_new_attestations, so this event is gone for good — not just delayed.

Net effect: on any slot where this node is the proposer, subscribers miss the head-move event that an observing (non-proposing) node emits normally at the same point in its tick cycle. The final head event still fires when the new block is imported via on_block, so overall consistency at rest isn't broken — but consumers of the head stream (e.g., a future SSE-based head tracker) will see gaps correlated with local proposing activity, which is a subtle and easy-to-miss inconsistency. Worth a comment justifying it if it's intentional ("we don't want a transient head" doesn't quite hold since observer nodes emit the equivalent event) or wiring self.events through here.

2. Minor: redundant get_block_header fetch in update_head (crates/blockchain/src/store.rs:87-90 vs 121-125)

The new Head event branch fetches new_head's header, and a few lines later the pre-existing logging block fetches it again just for new_slot. Not a bug, but easy to collapse into a single fetch reused by both the event and the log line.

3. Design note: untagged ChainEvent variants are structurally ambiguous (crates/blockchain/src/events.rs:36-51)

Block, JustifiedCheckpoint, and FinalizedCheckpoint all serialize to the same shape ({"slot": u64, "root": H256}). That's fine today since only Serialize is derived and the topic travels out-of-band, but it's a trap waiting for whichever follow-up PR adds a Deserialize/replay path for these payloads — serde's untagged matcher would silently pick the first structurally-matching variant (JustifiedCheckpoint, declared before FinalizedCheckpoint) regardless of the real topic. Worth a short comment now so it isn't re-discovered as a confusing bug later.

No fork-choice, justification/finalization, or signature-verification logic was touched by this PR beyond threading &EventBus through — the actual consensus semantics are unchanged, and the diffs to on_block_core/update_head correctly emit events only after the corresponding state mutation has succeeded (post-insert_signed_block/insert_state, post-update_checkpoints), so there's no risk of emitting an event for a state change that never actually committed.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@MegaRedHand
MegaRedHand marked this pull request as draft July 15, 2026 14:16
@MegaRedHand
MegaRedHand force-pushed the feat/chain-events-bus branch from 23825f1 to 3a3c804 Compare July 15, 2026 14:34
@MegaRedHand
MegaRedHand marked this pull request as ready for review July 15, 2026 14:36
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

Overall this is a well-architected PR. The snapshot-diff pattern cleanly separates observability from consensus logic, and the best-effort broadcast design correctly prioritizes liveness over event delivery guarantees.

Correctness & Safety

  1. Event ordering consistency (crates/blockchain/src/lib.rs:968)
    The emission order in process_block is: BlockJustifiedHeadFinalized (because diff_and_emit emits justified→head→finalized). This matches the comment intent, but verify this order satisfies SSE consumer expectations (e.g., if clients expect head before justified).

  2. Missing header handling (crates/blockchain/src/lib.rs:314-322)
    The defensive check for missing head headers is good, but consider if this scenario indicates store corruption. If store.head() returns a root without a corresponding header, the store is inconsistent. Logging at warn is appropriate, but ensure monitoring alerts on this log.

  3. State check for block novelty (crates/blockchain/src/lib.rs:957-960)
    Using has_state(&block_root) to detect duplicate blocks is correct (state is stored by block root), but add a comment explaining why state presence implies block presence, as this is subtle for future maintainers.

Code Quality

  1. Serde untagged variant (crates/blockchain/src/events.rs:57)
    #[serde(untagged)] eliminates type discriminants in JSON. Since deserialization isn't implemented (and likely unnecessary for SSE), this is acceptable, but document that consumers must use the SSE event: line (Topic) to distinguish variants.

  2. Unused capacity in disabled bus (crates/blockchain/src/events.rs:128)
    EventBus::disabled() creates a channel with capacity 1. Consider using broadcast::channel(0) if the API permits, or document that the capacity is arbitrary since emits are no-ops anyway.

  3. Topic string mapping (crates/blockchain/src/events.rs:36)
    Consider deriving Display for Topic instead of only as_str, which is more idiomatic for string representation.

Testing

  1. Test coverage for lagged subscribers (crates/blockchain/src/events.rs)
    Add a test verifying that a slow subscriber receives TryRecvError::Lagged (or dropped messages) when the channel exceeds CHAIN_EVENT_CHANNEL_CAPACITY, confirming the back-pressure behavior.

Nits

  1. Slot type consistency (crates/blockchain/src/events.rs:64-77)
    The ChainEvent variants use u64 for slots. If the codebase uses a Slot type alias (e.g., type Slot = u64), consider using it here for consistency.

The PR is safe to merge. The snapshot/diff approach is robust and keeps the store code path clean of event plumbing.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Findings:

  1. get_proposal_head() mutates fork-choice state outside the new snapshot/diff windows, so head/justified/finalized events can be dropped permanently if block production or local import fails. The only event snapshots are around store::on_tick() and store::on_block() in lib.rs and lib.rs, but proposer block building still calls on_tick() and accept_new_attestations() directly inside get_proposal_head() in store.rs. If produce_block_with_signatures() returns an error after that catch-up, or process_block() later fails, the store has already advanced and no event is ever emitted because the next tick snapshots the new values as baseline. This breaks event-stream correctness for any future SSE consumer. The proposer catch-up path needs the same snapshot/diff handling, or it needs to be made side-effect-free.

  2. The block event’s meaning is inconsistent with the storage path for pending blocks. ChainEvent::Block is documented as “A block was imported into the store” in events.rs, but blocks with missing parents are already persisted via insert_pending_block() in lib.rs and store.rs. Later, process_block() emits Block based on !has_state(root) in lib.rs, so a previously stored pending block is reported as a fresh block event only when its state becomes available. If the intended semantics are “first seen by this node” or “persisted,” this is wrong; if the intended semantics are “became processable/live-chain-visible,” the docs and topic name should say that explicitly.

No consensus-state-transition, attestation-validation, SSZ, or XMSS handling regressions stood out in the touched code beyond the event-stream consistency issues above.

I couldn’t run cargo test here because the environment’s Rust toolchain path is read-only and cargo failed before build startup.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@MegaRedHand
MegaRedHand marked this pull request as draft July 15, 2026 14:40
@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review: feat(blockchain): add chain-event bus (head/block/justified/finalized)

Overall this is a clean, well-isolated design — pub-sub facade over a bounded broadcast channel, sole-publisher invariant enforced structurally rather than by convention, and store.rs/spec tests genuinely untouched as claimed. The untagged serialization and has_state-based new-block gate are both correct and well tested. One correctness gap stands out, though.

1. Justified/finalized checkpoint events can be silently dropped on the proposer's own slot (correctness)

BlockChainServer::on_tick (crates/blockchain/src/lib.rs:427-429) wraps the only store::on_tick call it makes with a ChainEventSnapshot:

let pre_tick = ChainEventSnapshot::capture(&self.store);
store::on_tick(&mut self.store, timestamp_ms, is_proposer);
pre_tick.diff_and_emit(&self.store, &self.events);

But at interval 4, when this node is the next slot's proposer, on_tick also calls propose_blockstore::produce_block_with_signatures (crates/blockchain/src/store.rs:883), which internally calls get_proposal_head (crates/blockchain/src/store.rs:865-877):

fn get_proposal_head(store: &mut Store, slot: u64) -> H256 {
    ...
    on_tick(store, slot_time_ms, true);        // <-- a SECOND, unwrapped on_tick
    accept_new_attestations(store, false);
    store.head().expect("store head exists")
}

This second on_tick/accept_new_attestations call mutates head/justified/finalized (that's exactly the "interval-0 attestation acceptance" mechanism the wrapped call in on_tick exists to observe) but runs completely outside any ChainEventSnapshot. By the time process_block takes its own snapshot (crates/blockchain/src/lib.rs:963), the mutation from get_proposal_head has already happened — so it becomes the baseline the diff starts from, not something the diff detects.

For head, this is (mostly) masked: the freshly-built block's own import in on_block almost always changes head again, so a head event still fires (the module doc at crates/blockchain/src/lib.rs:283-286 acknowledges this and treats it as intentional coalescing).

For justified_checkpoint/finalized_checkpoint, there's no such guarantee. If get_proposal_head's internal on_tick call advances justification/finalization to their final values for the slot, and the subsequent on_block call for the newly proposed block causes no further change (the common case — the block was built from that already-caught-up state), the corresponding JustifiedCheckpoint/FinalizedCheckpoint events are never emitted at all, not merely delayed or coalesced.

Failure scenario: A validator on this node proposes a block in a slot where its own attestation-acceptance pass (inside get_proposal_head) is what pushes justification or finalization forward. Any subscriber relying on this event bus (the upcoming SSE endpoint) to track finalization will simply never see that finalization event, even though lean_finalized_slot/store state did advance. This directly undermines the feature's stated purpose for exactly the case (finalization tracking) called out in the PR description.

Suggested fix: wrap get_proposal_head's mutation too — e.g. have propose_block take its own ChainEventSnapshot before calling produce_block_with_signatures and diff/emit immediately after, before building continues, or extend process_block's snapshot to be taken before propose_block is invoked rather than only around on_block.

Minor

  • EventBus::disabled() (crates/blockchain/src/events.rs:99-101) is only ever called from its own unit test (crates/blockchain/src/events.rs:154) — no production or test call site actually constructs a BlockChainConfig without a live bus (only main.rs builds one). Either wire it up somewhere real (e.g. have spec-test/test_driver.rs harnesses that do build a BlockChainServer use it) or drop it until a caller needs it.
  • EventBus::emit's receiver_count() == 0 guard followed by tx.send(...) is a harmless TOCTOU (a subscriber could arrive/leave between the two), but worth noting only as an FYI — send already handles the zero-receiver error path, so the guard is a pure optimization and not a correctness issue.

Nits

  • Tests are thorough for the diff/emit mechanism itself (ordering, no-op case, missing-header skip) — good coverage for what's there; the gap in Item 1 above is precisely the case that's not covered because it requires exercising the proposer path with real justification movement, which these unit tests (built on bare Store, no full BlockChainServer proposer flow) don't reach.

Automated review by Claude (Anthropic) · sonnet · custom prompt

@MegaRedHand
MegaRedHand force-pushed the feat/chain-events-bus branch from 3a3c804 to a966632 Compare July 15, 2026 14:52
First PR of the chain-events series (re-cut of #460), shipping only the
pub-sub mechanism so the real design decisions (event shape, publisher
model, back-pressure policy) are reviewable on their own. The SSE
transport and topic filtering follow in separate PRs; the bus stays
unconsumed until the endpoint lands.

Emission is actor-layer only, not threaded through the store. Before
each state-changing store call (store::on_tick, store::on_block) the
actor snapshots (head, latest_justified, latest_finalized), runs the
store function completely unchanged, then diffs the snapshot against
the post-call state and emits in order block -> justified_checkpoint
-> head -> finalized_checkpoint. This keeps store.rs, both spec-test
files, and test_driver.rs byte-identical to main: no Option<&EventBus>
parameter spreads across on_tick/on_block/update_head or any of their
test call sites, and the sole-publisher property is structural (only
BlockChainServer ever holds a live EventBus) rather than something
every call site has to preserve by convention.

Two consequences of diffing at this level, both accepted:

- `block` needs a was-it-new guard: on_block returns Ok early for an
  already-imported root, so the actor checks block-known-ness (the
  same has_state check the store itself uses) before importing, and
  only emits `block` for a genuinely new root.
- Several head moves inside a single tick or block import coalesce
  into one `head` event carrying the net move; fine for subscribers,
  since the beacon-API analog is also last-value-wins per slot.

The bus itself is a facade over a single bounded tokio broadcast
channel: emit() never blocks or fails, since a slow subscriber lags
and drops instead of back-pressuring consensus, and a receiver-count
guard makes emission free when nobody is listening. ChainEvent
serializes untagged so payloads stay flat: the topic name travels
out-of-band via ChainEvent::topic(), fixing #460's double-tagged SSE
payload before any transport exists.

The `head` event is additionally gated by HEAD_EVENT_RECENCY_SLOTS,
adopted from a review of Lighthouse's canonical-head recompute path:
a new head is only emitted when its slot is within that many slots of
the wall clock, so startup catch-up and backfill don't spam subscribers
with historical head moves on the way to the tip. `block`,
justified_checkpoint, and finalized_checkpoint stay ungated; consumers
tracking sync progress should watch `block` instead.
@MegaRedHand
MegaRedHand force-pushed the feat/chain-events-bus branch from a966632 to 60d9c2e Compare July 15, 2026 17:51
…nd_emit

Reorder chain-event emission so the head event fires before
justified_checkpoint, giving subscribers the head move ahead of the
checkpoint updates that follow it. Drop the ordering assertion test,
which only pinned the old sequence.
Colocate the pre-call snapshot type (and HEAD_EVENT_RECENCY_SLOTS) with
the ChainEvent/EventBus definitions it depends on, keeping all chain-event
machinery in events.rs. The actor references it via crate::events; the
snapshot's diff/gate tests move alongside it. No behavior change.
The recency gate is already covered by chain_event_diff_gates_stale_head
(gated) and chain_event_diff_emits_recent_head (within window); the
explicit boundary case was redundant.
Eventless call paths (spec tests, test_driver.rs) never construct an
EventBus at all, so the dormant-bus constructor had no production callers;
its only user was its own test.
@MegaRedHand
MegaRedHand marked this pull request as ready for review July 17, 2026 18:46
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

Overall Assessment: Good PR. The event bus is correctly designed as a best-effort, non-blocking observer that doesn't interfere with consensus logic. The snapshot/diff pattern cleanly separates event emission from store internals.

Issues & Suggestions:

1. ChainEvent deserialization hazard (crates/blockchain/src/events.rs:43)
The #[serde(untagged)] attribute combined with identical fields across variants (slot, root) makes deserialization into ChainEvent impossible for consumers (variants 2-4 are indistinguishable in JSON).

  • Suggestion: Document explicitly that this type is serialize-only. Consumers must deserialize into topic-specific structs (e.g., HeadEvent, BlockEvent) based on the SSE event: line, not into the ChainEvent enum.
  • Note: The test at line 249 only checks serialization, which is insufficient to catch this.

2. Panic risk in snapshot capture (crates/blockchain/src/events.rs:174-182)
ChainEventSnapshot::capture uses expect for store queries. If the store is corrupted or in an unexpected state, the blockchain actor will panic.

  • Suggestion: Replace expect with proper error handling. Return Option<Self> or Result<Self, _> and log an error / skip event emission if the store returns None. Consensus stability is more important than event emission.

3. Minor race in emit optimization (crates/blockchain/src/events.rs:85-88)
The receiver_count() check avoids an error on send when no subscribers exist, but a subscriber could drop between the check and the send. The code handles this by ignoring the send error, so the check is merely an optimization, not a bug.

4. is_new check consistency (crates/blockchain/src/lib.rs:882-885)
Using has_state to detect new blocks assumes on_block writes state atomically and that the actor is single-threaded (which it appears to be). This is correct but fragile if on_block implementation changes.

  • Suggestion: Add a comment explaining the assumption that BlockChainServer processes blocks sequentially, making the pre-check safe.

5. Redundant wall-clock calculation (crates/blockchain/src/lib.rs:903-904)
process_block recalculates wall_clock_slot manually while on_tick uses a pre-calculated slot. Consider extracting a helper fn wall_clock_slot(genesis_time_ms: u64) -> u64 to ensure consistency.

6. Serde dependency move (crates/blockchain/Cargo.toml)
Correctly moved serde from [dev-dependencies] to [dependencies] since ChainEvent derives Serialize for production use.

Positive Notes:

  • Best-effort design: The bounded broadcast channel (256) with lagged subscriber dropping prevents slow consumers from back-pressuring consensus. Correctly documented.
  • Ordering: Emitting Block before Head/Justified/Finalized in `

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

  1. Performance: the zero-subscriber case is not actually a no-op. on_tick and process_block always do ChainEventSnapshot::capture plus diff_and_emit, and those methods always read head, latest_justified, and latest_finalized from the store, plus get_block_header on head changes (events.rs, events.rs). The only receiver_count guard is inside EventBus::emit, so the comment in main.rs is misleading: emissions are skipped, but the hot-path store reads still happen on every tick and block import. Since this PR ships with no consumers yet, that is pure overhead in fork-choice/import paths. I would short-circuit before snapshot/diff when there are no subscribers.

Open questions / assumptions:

  • main.rs moves the only EventBus handle into BlockChain::spawn. If the follow-up SSE endpoint is meant to subscribe from main, it will need a cloned handle retained outside the actor.

Aside from that, I did not find a consensus-correctness or security regression in the new event ordering itself. I could not run Cargo tests in this environment because the leanVM git dependency is not cached locally, and Cargo cannot populate ~/.cargo here.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review: feat(blockchain): add chain-event bus (head/block/justified/finalized) (PR 516)

Overall this is a clean, well-isolated addition — store.rs, the spec tests, and test_driver.rs stay untouched as claimed, the actor remains the sole publisher, and the test coverage (flat serialization, recency gate, missing-header defensive path, no-subscriber no-op) is solid. No changes touch fork choice, attestation processing, justification/finalization, XMSS, or SSZ logic — this PR only adds read-only observation of already-computed store state, so consensus-correctness risk is low. A few points worth addressing:

1. "Near-zero cost with no subscribers" only applies to emit(), not the surrounding snapshot/diff machinery

EventBus::emit correctly no-ops when receiver_count() == 0 (crates/blockchain/src/events.rs:95-100), but ChainEventSnapshot::capture and diff_and_emit run unconditionally on every tick (lib.rs:350-354) and every processed block (lib.rs:884-904), regardless of subscriber count. Each capture/diff round-trips through the backend for head(), latest_justified(), latest_finalized() (and conditionally get_block_header()) — these are real Table::Metadata/Table::BlockHeaders reads (crates/storage/src/store.rs:742-749), not in-memory lookups. That's ~6-7 extra reads per tick (×5/slot) and per block import, even with zero consumers wired up (which is the case for the entire lifetime of this PR, since no endpoint exists yet).

This matters most during initial sync/backfill cascades, where process_or_pend_block can call process_block — and therefore a fresh snapshot+diff — once per block in a potentially large queue (lib.rs:935-936). Consider gating the whole snapshot+diff behind a single receiver_count() > 0 check (exposed from EventBus) so the "free when idle" property actually holds structurally rather than only at the final emit call.

2. ChainEvent untagged shape is ambiguous for future Deserialize

Block { slot, root }, JustifiedCheckpoint { slot, root }, and FinalizedCheckpoint { slot, root } are structurally identical under #[serde(untagged)] (events.rs:52-58). Harmless today since only Serialize is derived and the topic travels out-of-band via ChainEvent::topic(), but if a future PR (e.g., a test client replaying SSE events, or a Rust-side consumer) derives Deserialize, these three variants become indistinguishable. Worth a short comment on the enum noting that any future Deserialize impl must dispatch on the out-of-band topic rather than relying on serde's untagged matching.

3. Minor: redundant has_state check

process_block computes is_new via self.store.has_state(&block_root) (lib.rs:882-885) purely to know whether to emit a Block event, then store::on_blockon_block_core immediately re-derives the identical check internally (store.rs:580-583) to decide whether to short-circuit. Confirmed the two checks use the exact same predicate, so this is correct, just a duplicate DB read per block. Not worth restructuring given the explicit goal of keeping store.rs untouched, but flagging in case a future PR wants on_block to report "was this new" directly instead of re-deriving it at the call site.

4. Nit: wall-clock-slot computation duplicated again

The genesis_time_ms/slot-from-unix_now_ms() pattern at lib.rs:902-903 repeats a pattern already inlined at several other sites in this file (lines 276, 495, 564, 694). Not introduced by this PR's design — just continuing existing style — so not blocking, but a helper (fn wall_clock_slot(&self) -> u64) would be a nice follow-up if anyone touches this file again.

Nice-to-haves confirmed correct

  • has_state/duplicate-import guard on Block emission correctly runs before store::on_block consumes the block, and only after confirming parent-state presence in process_or_pend_block (pending blocks never get a spurious event).
  • Self-produced blocks (propose_blockprocess_and_publish_blockprocess_block) go through the same event path as gossiped/backfilled blocks — no separate/missed emission path.
  • Recency-gate arithmetic (header.slot + HEAD_EVENT_RECENCY_SLOTS >= wall_clock_slot) correctly avoids u64 underflow by adding rather than subtracting.
  • Emission order (blockheadjustified_checkpointfinalized_checkpoint) matches the PR description and the reorder commit.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Shape ChainEvent payloads after the Ethereum beacon-API eventstream
(v4.0.0): `block` is the block root, `state` the state root, and `slot`
stands in for the beacon `epoch`. Drop the `#[serde(untagged)]`
Serialize derive; the wire encoding (decimal-string ints, 0x-hex roots,
out-of-band topic) returns with the SSE endpoint in a follow-up PR. This
also removes the ambiguous untagged shape flagged in review. Since serde
is no longer used in the crate, drop the serde/serde_json deps.
`justified_checkpoint` has no beacon analog and is kept as an ethlambda
extension mirroring `finalized_checkpoint`.

Also fix a checkpoint event dropped on the proposer's own slots: the
interval-0 catch-up in `get_proposal_head` advances head/finalized
outside every snapshot window, so a finalization moved there was folded
into the later block-import diff's baseline and never emitted.
`propose_block` now snapshots around the build and emits those moves on
both build success and failure, matching what a non-proposing node emits
at its interval-0 tick.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant