[MegaRedHand]

Summary

Opt-in per-block telemetry: with --telemetry.clickhouse-url (or ETHLAMBDA_TELEMETRY_CLICKHOUSE_URL), a node writes one ClickHouse row per point in each block's life, and per data column arrival, in the block_events table layout Silver uses. Both clients' timings can then be compared from one table and one set of queries, block by block, which the lean_block_import_phase_seconds histogram cannot do (it aggregates the blocks away).

Off by default: without the flag, emit builds nothing.

Piece	Where
Row encoding, batching HTTP writer, global install/emit sink	new crate crates/common/telemetry
Per-block rows from the existing import report; arrival rows and beacon attestable after the per-arrival head recomputation	crates/blockchain (import_timing.rs, lib.rs)
column_recv / column_validated rows	crates/net/p2p (beacon/column_checks.rs and callers)
Flags, install before P2P::spawn	bin/ethlambda
Docs	docs/telemetry.md (new), docs/cli.md, docs/metrics.md, CLAUDE.md

Design choices

• Silver's table, verbatim. Same DDL, created on first use. Silver's stage names where the meaning matches (received, da_available, el_sent/el_verdict, stf_done, attestable), <section>_start/_end for ethlambda's other import sections. source and verdict keep ethlambda's own values.
• In process, never blocking. Emit sites try_send into a bounded channel; a tokio task batches and POSTs JSONEachRow to ClickHouse's HTTP interface with reqwest (already a dependency). One insert per tick (1 s) unless a full request is waiting, since ClickHouse writes a part per insert.
• Bounded failure. A failed request is retried on the next tick from a capped backlog (100k rows, oldest dropped first). A batch ClickHouse rejects as malformed (HTTP 400) is dropped and counted rather than retried forever. Delivery is at least once.
• attestable means what Silver's does: the new head including the block is computed. Beacon recomputes the head once per arrival, after the cascade, so every block the cascade imported gets that time (only if the head computation succeeded).
• el_sent/el_verdict carry their own instants (EngineAnswer: sent, answered, verdict), not the import's engine section, which is the time the import waited. The two only coincide while nothing else runs during the request; an import that overlaps the state transition with engine_newPayload keeps correct rows.
• da_available comes from an exhaustive match on the availability evidence, so a new kind of evidence is a compile error here rather than a silent "nothing required".
• Credentials stay out of output: the URL is not logged, its Debug shows scheme, host and port only, a rejected value is never echoed in the CLI error, and request errors drop the URL. A startup warning fires when the URL sends credentials over plain http to a host that is not loopback.

Facts checked against a real ClickHouse 25.8 over HTTP: a malformed or wrongly typed row is a 400, an unknown table a 404, bad credentials a 403; an integer of nanoseconds parses into DateTime64(9); a DateTime above u32::MAX seconds is silently wrapped, so the encoder writes NULL there.

Known gaps (also in docs/telemetry.md)

• A beacon node's own proposals (from a validator client) enter as gossip with near-zero arrival latency; filter them by their missing decode_end row. A dedicated source is a possible follow-up.
• Gloas blocks have no el_* rows (the payload goes to the execution client with the envelope).
• Rows still pending when the node exits are lost.

Pilot (mainnet, ethlambda-4)

A mainnet follower on nightly (this PR plus the open perf stack) has written to a local ClickHouse 25.8 since 2026-11-03 18:38Z, with --telemetry.client-name beacon-eth4. The node's own DDL created the table; inserts run at one per second with 0 failures and 0 dropped rows.

Check	Result
Stages per block	every imported block has received, stf_done, el_verdict and attestable
Duplicates	none per (block, stage, column)
Columns	every column_validated has its column_recv (8 custody columns per block with blobs)
Same blocks as Silver (its own rows, eth-5, 30 min)	98 shared roots, 0 slot and 0 slot_start_date_time mismatches
Typical block	about 60 ms from received to attestable

Merging into nightly surfaced the two robustness issues fixed in f2eb961: nightly's #755 overlaps engine_newPayload with the state transition (the el_* rows had been read from the wait section), and #760 adds a ColumnsVerified evidence variant that the earlier matches! counted as nothing required.

Test plan

• make fmt, make lint
• cargo test -p ethlambda-telemetry --profile release-fast (19, including a mock ClickHouse: batching, retry, cap, refusal, basic auth, slow requests)
• cargo test -p ethlambda-blockchain --lib --profile release-fast (310)
• cargo test -p ethlambda-p2p --lib --profile release-fast (365)
• cargo test -p ethlambda --bins --profile release-fast (272)
• Pilot: a mainnet follower writing to a real ClickHouse, rows checked against Silver's for the same blocks (above)

[github-actions]

🤖 Codex Code Review

⚠️ The review job did not complete (result: failure). See the job log

---

Automated review by OpenAI Codex

[github-actions]

🤖 Claude Code Review

Review: PR 765, ClickHouse block telemetry

I read the full diff, writer.rs, event.rs, the lib.rs hooks and the CLI and credential handling. I did not build the code or run any tests. I also did not read the p2p column hooks (column_checks.rs, verdict.rs, handler.rs, handlers.rs) or the import_timing.rs cascade code beyond the first 600 diff lines, so those are unreviewed. I found no blocking correctness or security problems.

What looks good

• Off by default. emit takes a closure and returns before building anything when the sink isn't installed, so an unconfigured node pays nothing.
• Never blocks the actors. Emit sites use try_send into a bounded channel and count drops. The writer keeps at most one request in flight, and its backlog is capped at 100k rows, oldest dropped first.
• Failure handling.
  • An HTTP 400 on insert drops the batch instead of retrying forever.
  • Failed batches go back to the front of the buffer in order.
  • A panicked insert task is counted as dropped rows.
  • needs_schema re-runs the idempotent DDL after any non-400 failure.
• Credentials.
  • ClickHouseUrl's Debug omits the userinfo and query.
  • TypedValueParser keeps the raw value out of clap's error text.
  • reqwest::Error::without_url strips the URL from request errors.
  • hide_env_values is set, and the tests cover these paths.
• Cascade bookkeeping. cascade_telemetry is reset in on_block, the only caller of run_import_cascade. imported is appended only for blocks with a source label, so it can't grow without bound.
• imported_here(). Requiring stf_start.is_some() stops a re-delivered stored block from producing a second set of rows.
• Timestamp encoding. The DateTime overflow is handled (NULL above u32::MAX seconds), and Instant is converted to wall clock against an anchor with saturating arithmetic.

Minor points, none blocking

1. writer.rs, run: rows are timestamped by Anchor::now() when the writer task dequeues them, not when they are emitted. Each Instant is converted relative to that anchor, so timestamps stay correct. If the writer task is starved or the channel lags, the anchor's wall-clock reading is taken later, which is only a small clock-sampling skew. This is fine, but the doc comment on Anchor could say the conversion assumes the wall clock does not step between the instant and the anchor.
2. Credentials go as basic auth. With an http:// URL, a password crosses the network in cleartext. Consider a one-time startup warn! when the URL has userinfo and the scheme is http and the host isn't loopback.
3. Delivery is at least once. A request that times out client-side (10 s) after ClickHouse committed it gets retried and duplicates its rows. This is documented. A one-line mention that time-out ⇒ duplicate is possible would help people writing queries. Duplicate-tolerant queries (for example DISTINCT on (stage, block_root, event_date_time)) are the practical answer.
4. telemetry::install ignores a second call with a warn!, but the SINK static makes the crate hard to test end to end. The "no test calls install" comment is a fragile invariant. A #[cfg(test)] guard would be sturdier.
5. hostname() uses a raw libc::gethostname call. The SAFETY comment is accurate, but a rustix or hostname crate call would avoid new unsafe. Low priority, since libc is already a dependency.
6. As the PR states, beacon blocks proposed locally enter as gossip with near-zero latency. The stated workaround is filtering on a missing decode_end row. A dedicated source label would make that cleaner, so I'd file the follow-up.

The unchecked test-plan item (pilot against a real ClickHouse and comparison with Silver's rows) is the main remaining validation. The writer is fully tested against a mock server, so I'd treat that pilot as a post-merge check.

Verdict: approve, with the optional cleartext-credentials warning in point 2 as the most useful follow-up.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR for the telemetry system integration. Let me analyze the code across correctness, security, performance, and Rust best practices.

Security

Credential Exposure in Debug — bin/ethlambda/src/cli.rs:22-31

The ClickHouseUrl Debug implementation properly redacts credentials, but there's a subtle issue: reqwest::Url::host_str() returns Option<&str> and port_or_known_default() returns Option<u16>. The Debug derive for Option will print Some("host") and Some(8123), which leaks the Some wrapper. This is minor but inconsistent with the clean redaction intent.

// Current:
.field("host", &self.0.host_str())        // prints Some("host")
.field("port", &self.0.port_or_known_default())  // prints Some(8123)
// Better:
.field("host", &self.0.host_str().unwrap_or("none"))
.field("port", &self.0.port_or_known_default().unwrap_or(0))

URL Password in Display or Error Paths — bin/ethlambda/src/cli.rs:56-68

The reject closure in ClickHouseUrlParser correctly avoids echoing the raw value. However, cmd.clone().error(...) clones the entire Command, which could be expensive. More importantly, the error message format string uses {flag} which will call Arg::to_string() — verify this doesn't leak the argument's value.

Actually, arg.to_string() on an Arg returns the flag name (e.g., --telemetry.clickhouse-url), not the value. This is correct.

hostname() Buffer Safety — bin/ethlambda/src/main.rs:1000-1014

The unsafe block uses libc::gethostname with a 256-byte buffer. The safety comment is accurate but incomplete: it doesn't mention that gethostname may truncate without NUL on Linux if the name exceeds 255 bytes. The position fallback handles this, but the SAFETY comment should mention truncation behavior explicitly.

Critical: String::from_utf8_lossy can produce replacement characters for invalid UTF-8 sequences from the hostname. This is fine for telemetry, but the comment should note this.

Correctness & Consensus Safety

imported_here() Logic — crates/blockchain/src/import_timing.rs:470-473

pub fn imported_here(&self) -> bool {
    self.outcome == "imported" && self.timings.stf_start.is_some()
}

This relies on stf_start being Some only when the state transition actually ran. The test at line 1557 verifies the re-delivery case. However, this is fragile: if stf_start is set but the STF panics or returns early, outcome might still be "imported" from a prior path. The comment says "Not merely outcome == 'imported'" but doesn't explain why stf_start is the reliable discriminator.

Suggestion: Add a dedicated boolean stf_ran rather than overloading stf_start. The current approach works but couples timing collection to semantic state.

da_columns_required Flag — crates/blockchain/src/lib.rs:1787

timings.da_columns_required =
    matches!(evidence, Some(fork_choice::DataAvailability::Columns(_)));

This correctly distinguishes "check ran and found columns" from "check ran and found nothing." But note: DataAvailability::Columns(_) means columns were required, not that they were available. The naming is slightly confusing — da_columns_required is accurate, but the stage name da_available (line 503) claims availability. The check passes (returns Some) only when availability is satisfied, so this is correct in practice.

Cascade Telemetry arrival Race — crates/blockchain/src/lib.rs:2710

self.cascade_telemetry
    .arrival
    .get_or_insert((slot, block_root));

get_or_insert only sets on first call per cascade. The test at line 5554 verifies the first block is preserved. However, process_or_pend_block is called recursively via run_import_cascade — the arrival is reset in on_block at line 2096, but what about process_and_publish_block (local block production)?

Looking at log_import_report (line 2230), local blocks have source: None and thus source_label() returns None, so they're filtered from telemetry. The comment at line 2252 confirms this. Good.

head_ok and attestable Timing — crates/blockchain/src/import_timing.rs:815-825

if cascade.head_ok
    && let Some(head_end) = cascade.head_end
{
    events.extend(
        self.imported
            .iter()
            .map(|&(slot, root, source)| row("attestable", head_end, slot, root, source)),
    );
}

All imported blocks get the same head_end instant. This is correct per the design doc: "every block that cascade imported gets the same time." But this means the attestable timestamp doesn't reflect per-block head computation time — it's the cascade's head computation end. The docs acknowledge this; ensure consumers understand.

Performance

Vec Allocations in telemetry_events() — crates/blockchain/src/import_timing.rs:496-620

The marks vector has 29 fixed elements, then filtered to events. This allocates twice (marks + events) for every block import. Since this is only called when telemetry is installed, it's acceptable. But consider:

// Could use a small array on stack, but filter_map still needs dynamic output

The emit call uses || report.telemetry_events() which builds the vec only when SINK.get() is Some. This is correct per the lazy design.

Anchor::now() in encode — crates/common/telemetry/src/event.rs:175

Anchor::now() is called per event in writer.rs:167:

pending.push_back(encode(&event, Anchor::now(), &config));

This means each event in a batch gets a slightly different anchor instant/wall-clock pair. Events are converted relative to their own anchor, which introduces jitter. For a batch of 10,000 events, the last event's anchor could be milliseconds later than the first.

Impact: time_into_slot_ms could vary by a few ms across a batch for the same logical instant. This is minor for telemetry but worth documenting.

Fix: Use one Anchor per batch flush, not per event.

Channel Capacity — crates/common/telemetry/src/lib.rs:19

const CHANNEL_CAPACITY: usize = 16_384;

With 29+ events per block plus column events, this is ~500 blocks of backlog. At 12s slots, that's ~100 minutes. Reasonable.

drop_past_cap Drops Oldest — crates/common/telemetry/src/writer.rs:244-249

fn drop_past_cap(pending: &mut VecDeque<String>, max_rows: usize) {
    let excess = pending.len().saturating_sub(max_rows);
    if excess > 0 {
        pending.drain(..excess);  // drops FRONT = oldest
        metrics::inc_rows_dropped(excess as u64);
    }
}

Correct: oldest first. But drain(..excess) on a VecDeque may reallocate or shift. With max_pending_rows: 100_000, this is acceptable.

Rust Best Practices

std::time::Instant Serialization — crates/common/telemetry/src/event.rs:30

BlockEvent stores Instant directly, which is only meaningful for subtraction. The encode function converts via Anchor. This is clean separation.

serde_json::to_string in Hot Path — crates/common/telemetry/src/event.rs:178

serde_json::to_string(&row).expect("a row of plain fields always serializes")

This expect is sound — all fields are basic types. But to_string allocates per row. With 10K rows per batch, this is 10K allocations. Consider a buffered writer or serde_json::to_vec into a pre-allocated buffer.

Actually, the rows are joined with "\n" later (writer.rs:316), so individual strings are needed. Acceptable.

tokio::spawn Without Abort Handle — crates/common/telemetry/src/lib.rs:37

tokio::spawn(writer::run(config, rx, writer::Limits::DEFAULT));

No handle stored, no graceful shutdown. The docs say "rows still pending when the node exits are lost." This is acknowledged, but for a blockchain client, consider if we should flush on SIGTERM.

LazyLock vs OnceLock — crates/common/telemetry/src/metrics.rs:10

static ROWS: LazyLock<IntCounterVec> = LazyLock::new(|| { ... });

LazyLock is Rust 1.80+; this is fine if MSRV permits. The init() function forces initialization by touching labels. Good practice for Prometheus.

reqwest::Client Clone — crates/common/telemetry/src/writer.rs:255

let client = client.clone();

reqwest::Client is cheap to clone (Arc internally). Correct.

Error Handling

execute Error Chaining — crates/common/telemetry/src/writer.rs:328-340

let mut message = err.to_string();
let mut source = err.source();
while let Some(cause) = source {
    message.push_str(": ");
    message.push_str(&cause.to_string());
    source = cause.source();
}

Manual error chaining instead of anyhow or eyre. This is fine for a focused crate, but reqwest::Error::without_url() is used correctly.

HTTP 400 Handling — crates/common/telemetry/src/writer.rs:347-351

rejected: is_insert && status == reqwest::StatusCode::BAD_REQUEST,

ClickHouse returns 400 for malformed rows. This is correct per ClickHouse HTTP interface docs. But what about 401/403 (auth failures)? These would retry indefinitely. The needs_schema flag would recreate the table, but auth failures would persist.

Suggestion: Add special handling for 401/403 to log clearly and maybe back off faster.

Memory Safety

VecDeque Growth — crates/common/telemetry/src/writer.rs

pending can grow to max_pending_rows (100K). Each row is a String of ~200-300 bytes. That's ~30MB max. Bounded and acceptable.

InFlight Panic Safety — crates/common/telemetry/src/writer.rs:220-230

async fn join(in_flight: &mut Option<InFlight>) -> Outcome {
    match in_flight {
        Some(InFlight { rows, handle }) => handle.await.unwrap_or_else(|err| {
            metrics::inc_rows_dropped(*rows as u64);
            ...
        }),
        None => std::future::pending().await,
    }
}

If the task panics, handle.await returns Err(JoinError). The unwrap_or_else handles this and counts rows as dropped. Correct.

Code Clarity

verdict_label Function Location — crates/blockchain/src/lib.rs:4209-4217

This is a free function at module level, only used in one place. Consider making it a method on PayloadValidity or moving closer to use site. Minor.

source_label Duplication — crates/blockchain/src/import_timing.rs:700-708

Extracted to shared function. Good refactor.

at() Helper in Tests — crates/blockchain/src/import_timing.rs:1270

fn at(base: Instant, ms: u64) -> Option<Instant> {
    Some(base + Duration::from_millis(ms))
}

Clean test helper. Good.

Documentation

The docs/telemetry.md is comprehensive. One gap: it doesn't specify the block_events table engine. The DDL uses MergeTree without TTL or partitioning. For high-volume telemetry, consider documenting partitioning recommendations.

Minor Issues

1. Typo in comment: crates/blockchain/src/import_timing.rs:175 — "gloas" should be "gloas" (appears multiple times, seems intentional for "gossip + das" or similar? Actually appears to be a term of art in this codebase.)

2. SlotClock::slot_start_ms saturation: crates/common/telemetry/src/event.rs:64

   fn slot_start_ms(&self, slot: u64) -> u64 {
       self.genesis_ms
           .saturating_add(slot.saturating_mul(self.slot_ms))
   }

   With slot_ms: 1 (clamped from 0) and large slot, this saturates at u64::MAX. The offset_ms function then computes start_ns as u128::MAX * 1_000_000, which doesn't saturate. But slot_start_date_time filters to u32::MAX seconds, so the overflow is contained.

3. truncate on char boundary: crates/common/telemetry/src/writer.rs:359-365

   fn truncate(text: &str, max_bytes: usize) -> &str {
       let mut end = max_bytes.min(text.len());
       while !text.is_char_boundary(end) {
           end -= 1;
       }
       &text[..end]
   }

   If max_bytes is 0 and text starts with a multi-byte char, this underflows end since usize wraps. But max_bytes is 512 in practice. Add assert!(max_bytes > 0) or handle end == 0.

Summary

Severity	Count	Description
Critical	0	No consensus or security critical issues
High	0	No memory safety or DoS vectors
Medium	2	Anchor per-event jitter; truncate underflow
Low	4	Debug formatting; hostname comment; auth error handling; typo

The code is well-structured, security-conscious with credential redaction, and thoroughly tested. The telemetry system correctly avoids impacting performance when disabled and handles backpressure gracefully.

---

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