Skip to content

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

Draft
MegaRedHand wants to merge 1 commit into
mainfrom
feat/chain-events-bus
Draft

feat(blockchain): add chain-event bus (head/block/justified/finalized)#516
MegaRedHand wants to merge 1 commit 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 blockjustified_checkpointheadfinalized_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 head, block, justified, and finalized events.
  • Actor-side state snapshots that emit events after ticks and block imports.
  • Flat JSON event payloads with transport-level topic names.
  • Event-bus setup in the node binary.

Confidence Score: 5/5

No additional blocking issue qualifies for this follow-up review.

  • No distinct unresolved failure was found beyond the issues already reported.
  • The updated imports, dependencies, and configuration construction are consistent across the inspected build surfaces.

Important Files Changed

Filename Overview
crates/blockchain/src/events.rs Adds chain-event types, topic mapping, serialization, and the broadcast bus.
crates/blockchain/src/lib.rs Threads the event bus into the actor and emits state changes around ticks and block imports.
bin/ethlambda/src/main.rs Creates the production event bus and adds it to the blockchain configuration.
crates/blockchain/Cargo.toml Moves serde into production dependencies for event serialization.

Reviews (3): Last reviewed commit: "feat(blockchain): add chain-event bus (h..." | 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
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