Skip to content

feat: wire recent blocks tracker into event subscriber#3616

Open
kevindeforth wants to merge 2 commits into
mainfrom
kd/3608-wire-RecentBlocksTracker-into-event-subscriber
Open

feat: wire recent blocks tracker into event subscriber#3616
kevindeforth wants to merge 2 commits into
mainfrom
kd/3608-wire-RecentBlocksTracker-into-event-subscriber

Conversation

@kevindeforth

@kevindeforth kevindeforth commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

resolves #3608

Uses the RecentBlocksTracker inside the chain gateway crate, such that block events are accompanied by a block status handle (indicating their finality status).

Because RecentBlocksTracker now lives in the chain gateway, we can avoid sending empty block updates to the consumer, sending only in case a matching block event was found.

This PR adds two integration tests: one for checking back pressure works as expected, the other for verifying that finality status is eventually updated.

@kevindeforth kevindeforth force-pushed the kd/3608-wire-RecentBlocksTracker-into-event-subscriber branch from 7be74e6 to d0a6a4d Compare June 17, 2026 16:25
@kevindeforth kevindeforth marked this pull request as ready for review June 18, 2026 07:15
@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

PR Review for 3616. See findings below.

@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

Pull request overview — This PR wires RecentBlocksTracker into the chain-gateway's block_processor::listen_blocks so each emitted BlockUpdate carries a BlockStatusHandle whose finality can be polled by consumers. The streamer no longer forwards empty (non-matching) blocks to consumers, and it changes its full-channel strategy from awaiting (true backpressure that propagates up to the nearcore stream) to non-blocking try_reserve (drop-newest). Two integration tests are added: one exercising the drop-on-full behavior, one confirming that a held handle eventually reports is_final() == Some(true).

@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

Changes: (1) BlockUpdate gains a status: BlockStatusHandle field. (2) listen_blocks instantiates RecentBlocksTracker::new(window), calls add_block for every streamer message, computes matched events, and only sends to the consumer channel when there is at least one match. (3) Send semantics flipped: was block_update_sender.send(...).await (awaits when full); now try_reserve with warn-log + drop on Full and error return on Closed. (4) process_block was split into make_context + match_block_events. (5) New consts.rs exposing DEFAULT_NUMBER_OF_TRACKED_BLOCKS = 200; window is max(DEFAULT, buffer_size). (6) Tests refactored to use must_recv_block_update; failure test now relies on try_recv after shutdown; two new integration tests added.

@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

Blocking — must fix before merge. crates/chain-gateway/src/event_subscriber/streamer/block_processor.rs:48-66 — Semantic regression: the previous implementation applied true backpressure via send().await, which propagates upstream to the nearcore indexer (whose stream channel is bounded at 100). The new try_reserve silently drops matched updates on a full channel. For chain-gateway's main consumer (mpc-node), dropping a sign request event is data loss the protocol cannot recover from — the contract has the request, but the node never sees it. At minimum: (a) add a metric counter for dropped updates, and (b) make the choice between drop-newest and backpressure explicit in the PR/design. If drop-newest is not the intent, restore send().await — the tracker can still ingest every block via add_block before the send. Same file, lines 50-55: the dropped events include BlockEventIds the callers subscribed to; consider logging matched_events.iter().map(|e| e.id) so on-call can correlate missed work.

@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

Non-blocking nits and follow-ups. (1) crates/chain-gateway/src/event_subscriber/streamer.rs:28-32.expect("usize is expected to fit into u64") panics on a path that is dead on every supported target (usize <= 64 bits). Per docs/engineering-standards.md (Don't panic) this is allowed as a guaranteed-dead path, but u64::try_from(buffer_size).unwrap_or(u64::MAX) or buffer_size as u64 avoids the panic entirely. (2) Same line — rationale for max(DEFAULT, buffer_size) is non-obvious: buffer_size measures matched updates while window_size measures raw blocks, so the units are not directly comparable. A one-line Why comment would help. (3) crates/chain-gateway/tests/event_subscriber_integration.rs:349 — Typo: blcok -> block. (4) crates/chain-gateway/tests/event_subscriber_integration.rs:355 — Test name test_event_subscriber_channel_buffer_handles_backpressure is misleading: it verifies that backpressure is not applied. Consider ..._drops_updates_when_channel_full.

@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

Non-blocking nits (cont.) (5) crates/chain-gateway/tests/event_subscriber_integration.rs:404-411 — After the state viewer confirms the second value, the streamer task and the state viewer are on independent paths; there is no guarantee the streamer has finished pushing into the channel by the time try_recv runs. Localnet timing makes this likely fine in practice, but the test is vulnerable to flakiness; consider draining with a short recv timeout for the expected ones, then asserting try_recv is empty. (6) Same file, lines 188-191 — After localnet.shutdown().await the sender side is dropped, so try_recv will return Disconnected, not Empty. The expect_err("expected channel to be empty") message is misleading. (7) block_processor.rs (general) — the removed comment explaining why this loop is sequential (vs. buffer_unordered in mpc-node) was a useful 'why' per engineering-standards.md (Write helpful code comments). Consider preserving a one-liner so a future reader does not reintroduce concurrency here. (8) consts.rs — A whole module for a single constant feels over-organized; inlining at the top of streamer.rs would reduce surface area. Subjective. (9) New tests do not follow the <sut>__should_<assertion> naming or strict Given/When/Then structure prescribed by CLAUDE.md / engineering-standards.md. The pre-existing tests in this file already deviate, so this is at most a follow-up cleanup. ⚠️ Issues found.

@kevindeforth

Copy link
Copy Markdown
Contributor Author

There is so much wrong with what Claude is spewing here, I don't even know where to start.

One thing that seems worth addressing is the blocking item:
I agree that we would like some sort of metric for dropped messages. In this PR, we log a a warning. Will add this as an acceptance criteria to #3229

As for some other pearls of wisdom:
u64::try_from(buffer_size).unwrap_or(u64::MAX) this might be some new stanrdard I am not aware of, as Claude already tried to sneak that into the code before opening the PR. I'd rather panic than have a buffer of sixe u64::MAX, but maybe I am not up-to-date on the latest trends here...

Localnet timing makes this likely fine in practice, but the test is vulnerable to flakiness; consider draining with a short recv timeout for the expected ones, then asserting try_recv is empty.

The event stream tracks optimistic events, whereas the viewer tracks finalized events. If this test is flaky due to that, then it means the finalized state view updates before we see the optimistic event, which would be pretty bad and force us to re-think our architecture.
If we observe flakyness, it's a signal for bigger issues.

the removed comment explaining why this loop is sequential (vs. buffer_unordered in mpc-node) was a useful 'why' per engineering-standards.md (Write helpful code comments). Consider preserving a one-liner so a future reader does not reintroduce concurrency here.

Nah, thie usefulness of this comment kind of died when we moved the RecentBlocksTracker to the chain-gateway crate. It will become truly obsolete once we integrate the chain-gateway with the node.

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.

feat(chain-gateway): wire RecentBlocksTracker into the event subscriber and omit empty block updates

1 participant