From 2c6382f04406c24e95b465e302c73d01e3335726 Mon Sep 17 00:00:00 2001 From: Quang Le Date: Fri, 28 Aug 2026 17:45:37 +0700 Subject: [PATCH] perf: precompile health regexes, share HTTP client, cut hot-path clones --- crates/app/src/health/checks.rs | 39 ++++++------- crates/app/src/health/select.rs | 57 ++++++++++--------- crates/cli/src/commands/test/beacon.rs | 17 +----- crates/cli/src/commands/test/helpers.rs | 24 ++++++-- crates/cli/src/commands/test/mev.rs | 15 +---- crates/cli/src/commands/test/peers.rs | 3 +- crates/core/src/types.rs | 74 +++++++++++++++++++++++-- crates/p2p/src/conn_logger.rs | 4 +- crates/parsigex/src/behaviour.rs | 24 ++++---- crates/parsigex/src/handler.rs | 2 +- 10 files changed, 158 insertions(+), 101 deletions(-) diff --git a/crates/app/src/health/checks.rs b/crates/app/src/health/checks.rs index 78f47d94..82ebe4db 100644 --- a/crates/app/src/health/checks.rs +++ b/crates/app/src/health/checks.rs @@ -1,12 +1,13 @@ //! Health checks: severity, cluster metadata, the check type, the fixed list of -//! 9 checks, and the label-pair helper. +//! 9 checks, and their precompiled label matchers. + +use std::sync::LazyLock; use super::{ checker::QueryFunc, error::Result, - model::LabelPair, reducers::{gauge_max, increase}, - select::{count_labels, count_non_zero_labels, no_labels, sum_labels}, + select::{LabelMatcher, count_labels, count_non_zero_labels, no_labels, sum_labels}, }; /// Severity of a health check. @@ -59,13 +60,13 @@ pub(crate) struct Check { pub(crate) func: fn(&QueryFunc<'_>, &Metadata) -> Result, } -/// Convenience constructor for a label pair. -fn label(name: &str, value: &str) -> LabelPair { - LabelPair { - name: name.to_owned(), - value: value.to_owned(), - } -} +/// Label matchers for [`pending_validators`], compiled once. +static PENDING_STATUS_LABELS: LazyLock<[LabelMatcher; 1]> = + LazyLock::new(|| [LabelMatcher::new("status", "pending")]); + +/// Label matchers for [`proposal_failures`], compiled once. +static PROPOSAL_DUTY_LABELS: LazyLock<[LabelMatcher; 1]> = + LazyLock::new(|| [LabelMatcher::new("duty", ".*proposal")]); /// Lossy `i64` → `f64` conversion used only for threshold comparisons. #[allow( @@ -78,7 +79,7 @@ fn to_f64(n: i64) -> f64 { fn high_error_log_rate(q: &QueryFunc<'_>, m: &Metadata) -> Result { // Allow 2 errors per validator. - let value = q.query("app_log_error_total", sum_labels(Vec::new()), increase)?; + let value = q.query("app_log_error_total", sum_labels(&[]), increase)?; Ok(value > 2.0 * to_f64(m.num_validators)) } @@ -87,7 +88,7 @@ fn high_warning_log_rate(q: &QueryFunc<'_>, m: &Metadata) -> Result { // but the warn counter is emitted as `app_log_warn_total`, so Charon's own // check never matches it. We query the emitted name so this check fires. // Allow 2 warnings per validator. - let value = q.query("app_log_warn_total", sum_labels(Vec::new()), increase)?; + let value = q.query("app_log_warn_total", sum_labels(&[]), increase)?; Ok(value > 2.0 * to_f64(m.num_validators)) } @@ -105,7 +106,7 @@ fn insufficient_connected_peers(q: &QueryFunc<'_>, m: &Metadata) -> Result fn pending_validators(q: &QueryFunc<'_>, _m: &Metadata) -> Result { let max_val = q.query( "core_scheduler_validator_status", - count_labels(vec![label("status", "pending")]), + count_labels(&*PENDING_STATUS_LABELS), gauge_max, )?; Ok(max_val > 0.0) @@ -114,32 +115,28 @@ fn pending_validators(q: &QueryFunc<'_>, _m: &Metadata) -> Result { fn proposal_failures(q: &QueryFunc<'_>, _m: &Metadata) -> Result { let value = q.query( "core_tracker_failed_duties_total", - sum_labels(vec![label("duty", ".*proposal")]), + sum_labels(&*PROPOSAL_DUTY_LABELS), increase, )?; Ok(value > 0.0) } fn high_registration_failures_rate(q: &QueryFunc<'_>, _m: &Metadata) -> Result { - let value = q.query( - "core_bcast_recast_errors_total", - sum_labels(Vec::new()), - increase, - )?; + let value = q.query("core_bcast_recast_errors_total", sum_labels(&[]), increase)?; Ok(value > 0.0) } fn metrics_high_cardinality(q: &QueryFunc<'_>, _m: &Metadata) -> Result { let max_val = q.query( "app_health_metrics_high_cardinality", - sum_labels(Vec::new()), + sum_labels(&[]), gauge_max, )?; Ok(max_val > 0.0) } fn using_fallback_beacon_nodes(q: &QueryFunc<'_>, _m: &Metadata) -> Result { - let max_val = q.query("app_eth2_using_fallback", sum_labels(Vec::new()), gauge_max)?; + let max_val = q.query("app_eth2_using_fallback", sum_labels(&[]), gauge_max)?; Ok(max_val > 0.0) } diff --git a/crates/app/src/health/select.rs b/crates/app/src/health/select.rs index 885566d2..88200b29 100644 --- a/crates/app/src/health/select.rs +++ b/crates/app/src/health/select.rs @@ -45,12 +45,29 @@ pub(crate) fn no_labels() -> Selector { }) } +/// A label name paired with the compiled regex its value must match, built +/// once per selector rather than once per metric series. +pub(crate) struct LabelMatcher { + name: &'static str, + /// [`None`] if the pattern failed to compile, which never matches. + regex: Option, +} + +impl LabelMatcher { + pub(crate) fn new(name: &'static str, pattern: &str) -> Self { + Self { + name, + regex: Regex::new(pattern).ok(), + } + } +} + /// Sums the values of series matching all of `labels`. -pub(crate) fn count_labels(labels: Vec) -> Selector { +pub(crate) fn count_labels(labels: &'static [LabelMatcher]) -> Selector { Box::new(move |fam: &MetricFamily| { let mut sum = 0.0_f64; for metric in &fam.metrics { - if labels_contain(&metric.labels, &labels) { + if labels_contain(&metric.labels, labels) { sum += metric.value_or_zero(); } } @@ -60,14 +77,14 @@ pub(crate) fn count_labels(labels: Vec) -> Selector { /// Sums the values of series matching all of `labels`; errors on non /// gauge/counter families. -pub(crate) fn sum_labels(labels: Vec) -> Selector { +pub(crate) fn sum_labels(labels: &'static [LabelMatcher]) -> Selector { Box::new(move |fam: &MetricFamily| { if fam.metric_type != MetricType::Gauge && fam.metric_type != MetricType::Counter { return Err(Error::UnsupportedMetricType); } let mut sum = 0.0_f64; for metric in &fam.metrics { - if labels_contain(&metric.labels, &labels) { + if labels_contain(&metric.labels, labels) { sum += metric.value_or_zero(); } } @@ -75,27 +92,13 @@ pub(crate) fn sum_labels(labels: Vec) -> Selector { }) } -/// Returns true if every pair in `contain` matches some label in `labels`: -/// names must be equal and the `contain` value is matched as a regex against -/// the label value. A regex that fails to compile is treated as no match. -pub(crate) fn labels_contain(labels: &[LabelPair], contain: &[LabelPair]) -> bool { - for c in contain { - let mut found = false; - for l in labels { - if l.name != c.name { - continue; - } - if Regex::new(&c.value) - .map(|re| re.is_match(&l.value)) - .unwrap_or(false) - { - found = true; - break; - } - } - if !found { - return false; - } - } - true +/// Returns true if every matcher in `contain` matches some label in `labels`: +/// names must be equal and the matcher's regex must match the label value. A +/// matcher whose regex failed to compile is treated as no match. +pub(crate) fn labels_contain(labels: &[LabelPair], contain: &[LabelMatcher]) -> bool { + contain.iter().all(|c| { + labels + .iter() + .any(|l| l.name == c.name && c.regex.as_ref().is_some_and(|re| re.is_match(&l.value))) + }) } diff --git a/crates/cli/src/commands/test/beacon.rs b/crates/cli/src/commands/test/beacon.rs index 2faf166f..54a61525 100644 --- a/crates/cli/src/commands/test/beacon.rs +++ b/crates/cli/src/commands/test/beacon.rs @@ -12,8 +12,9 @@ use super::{ helpers::{ AllCategoriesResult, CategoryScore, TestCaseName, TestCategory, TestCategoryResult, TestResult, TestResultError, TestVerdict, calculate_score, evaluate_highest_rtt, - evaluate_rtt, filter_tests, must_output_to_file_on_quiet, publish_result_to_obol_api, - request_rtt, sort_tests, write_result_to_file, write_result_to_writer, + evaluate_rtt, filter_tests, http_client, must_output_to_file_on_quiet, + publish_result_to_obol_api, request_rtt, sort_tests, write_result_to_file, + write_result_to_writer, }, }; use crate::{duration::Duration, error::Result as CliResult}; @@ -29,18 +30,6 @@ use tokio::{ }; use tokio_util::sync::CancellationToken; -/// Per-request timeout for beacon-node diagnostic HTTP calls, so a hostile or -/// slow endpoint cannot stall a diagnostic indefinitely. -const BEACON_HTTP_TIMEOUT: StdDuration = StdDuration::from_secs(10); - -/// Builds a diagnostic HTTP client with a request timeout. -fn http_client() -> reqwest::Client { - reqwest::Client::builder() - .timeout(BEACON_HTTP_TIMEOUT) - .build() - .unwrap_or_default() -} - const THRESHOLD_BEACON_MEASURE_AVG: StdDuration = StdDuration::from_millis(40); const THRESHOLD_BEACON_MEASURE_POOR: StdDuration = StdDuration::from_millis(100); const THRESHOLD_BEACON_LOAD_AVG: StdDuration = StdDuration::from_millis(40); diff --git a/crates/cli/src/commands/test/helpers.rs b/crates/cli/src/commands/test/helpers.rs index d307d8c2..d341b199 100644 --- a/crates/cli/src/commands/test/helpers.rs +++ b/crates/cli/src/commands/test/helpers.rs @@ -1,7 +1,9 @@ //! Shared types and helper functions for all test categories. use serde::{Deserialize, Serialize}; -use std::{collections::HashMap, fmt, io::Write, path::Path, time::Duration as StdDuration}; +use std::{ + collections::HashMap, fmt, io::Write, path::Path, sync::LazyLock, time::Duration as StdDuration, +}; use crate::{ ascii::{append_score, get_category_ascii, get_score_ascii}, @@ -581,6 +583,22 @@ pub(crate) fn hash_ssz(data: &[u8]) -> CliResult { Ok(hasher.hash_root()?) } +/// Per-request timeout for diagnostic HTTP calls, so a hostile or slow endpoint +/// cannot stall a diagnostic indefinitely. +const DIAG_HTTP_TIMEOUT: StdDuration = StdDuration::from_secs(10); + +/// Returns the shared diagnostic HTTP client. One client means one connection +/// pool, so probes keep alive instead of handshaking inside their own RTT. +pub(crate) fn http_client() -> &'static reqwest::Client { + static CLIENT: LazyLock = LazyLock::new(|| { + reqwest::Client::builder() + .timeout(DIAG_HTTP_TIMEOUT) + .build() + .unwrap_or_default() + }); + &CLIENT +} + /// Measures the round-trip time (RTT) for an HTTP request and logs a warning if /// the response status code doesn't match the expected status. pub(crate) async fn request_rtt( @@ -589,9 +607,7 @@ pub(crate) async fn request_rtt( body: Option>, expected_status: StatusCode, ) -> CliResult { - let client = reqwest::Client::new(); - - let mut request_builder = client.request(method, url.as_ref()); + let mut request_builder = http_client().request(method, url.as_ref()); if let Some(body_bytes) = body { request_builder = request_builder diff --git a/crates/cli/src/commands/test/mev.rs b/crates/cli/src/commands/test/mev.rs index 329d3b53..cff74b0e 100644 --- a/crates/cli/src/commands/test/mev.rs +++ b/crates/cli/src/commands/test/mev.rs @@ -11,8 +11,8 @@ use super::{ AllCategoriesResult, TestCategory, TestCategoryResult, TestConfigArgs, TestResult, TestVerdict, calculate_score, constants::{SLOT_TIME, SLOTS_IN_EPOCH}, - evaluate_rtt, must_output_to_file_on_quiet, publish_result_to_obol_api, request_rtt, - write_result_to_file, write_result_to_writer, + evaluate_rtt, http_client, must_output_to_file_on_quiet, publish_result_to_obol_api, + request_rtt, write_result_to_file, write_result_to_writer, }; use crate::{ commands::test::TestCaseName, @@ -21,20 +21,9 @@ use crate::{ }; use clap::Args; -/// Per-request timeout for MEV/beacon diagnostic HTTP calls. -const MEV_HTTP_TIMEOUT: Duration = Duration::from_secs(10); /// Maximum diagnostic response body read from a beacon/relay endpoint (16 MB). const BN_MAX_BODY: usize = 16 * 1024 * 1024; -/// Builds a diagnostic HTTP client with a request timeout so a hostile/slow -/// endpoint cannot stall a diagnostic indefinitely. -fn http_client() -> reqwest::Client { - reqwest::Client::builder() - .timeout(MEV_HTTP_TIMEOUT) - .build() - .unwrap_or_default() -} - /// Reads a response body, rejecting bodies that exceed [`BN_MAX_BODY`]. Uses /// the advertised `Content-Length` for the fast-path reject; the client timeout /// bounds a slow/absent-length body. diff --git a/crates/cli/src/commands/test/peers.rs b/crates/cli/src/commands/test/peers.rs index 1eb982ae..1bdd3f49 100644 --- a/crates/cli/src/commands/test/peers.rs +++ b/crates/cli/src/commands/test/peers.rs @@ -502,9 +502,8 @@ async fn run_relay_http_tests( async fn relay_ping_test(url: &str, ct: &CancellationToken) -> TestResult { let result = TestResult::new("PingRelay"); - let client = reqwest::Client::new(); tokio::select! { - res = client.get(url).send() => match res { + res = super::http_client().get(url).send() => match res { Ok(resp) if resp.status().is_success() => result.ok(), Ok(resp) => result.fail(TestResultError::from_string(format!("HTTP status {}", resp.status()))), Err(e) => result.fail(e), diff --git a/crates/core/src/types.rs b/crates/core/src/types.rs index 7965950b..2b69d038 100644 --- a/crates/core/src/types.rs +++ b/crates/core/src/types.rs @@ -54,18 +54,43 @@ pub enum DutyType { impl Display for DutyType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if let Some(s) = self.as_str() { + return f.write_str(s); + } + // DutySentinel renders as a JSON object, so it keeps the serde path. // safe to unwrap because we know the duty type is valid let v = serde_json::to_value(self).expect("failed to serialize duty type"); - if let Some(s) = v.as_str() { - write!(f, "{}", s) - } else { - // fallback for non-string variants (structs, numbers, etc.) - write!(f, "{}", v) - } + write!(f, "{}", v) } } impl DutyType { + /// Returns the rendered name for this duty type, or [`None`] for + /// [`DutyType::DutySentinel`], which has no flat string form. + /// + /// The strings MUST match the `snake_case` serde encoding: they are used as + /// metric label values and in [`Display`]. + pub fn as_str(&self) -> Option<&'static str> { + let s = match self { + DutyType::Unknown => "unknown", + DutyType::Proposer => "proposer", + DutyType::Attester => "attester", + DutyType::Signature => "signature", + DutyType::Exit => "exit", + DutyType::BuilderProposer => "builder_proposer", + DutyType::BuilderRegistration => "builder_registration", + DutyType::Randao => "randao", + DutyType::PrepareAggregator => "prepare_aggregator", + DutyType::Aggregator => "aggregator", + DutyType::SyncMessage => "sync_message", + DutyType::PrepareSyncContribution => "prepare_sync_contribution", + DutyType::SyncContribution => "sync_contribution", + DutyType::InfoSync => "info_sync", + DutyType::DutySentinel(_) => return None, + }; + Some(s) + } + /// Returns true if the duty type is valid. pub fn is_valid(&self) -> bool { !matches!(self, DutyType::Unknown | DutyType::DutySentinel(_)) @@ -1053,6 +1078,43 @@ mod tests { assert!(!DutyType::DutySentinel(Box::new(DutyType::Attester)).is_valid()); } + /// `Display` renders duty types into metric label values, so it must stay + /// byte-identical to the serde encoding it used to round-trip through. + #[test] + fn duty_type_as_str_matches_serde() { + let all = [ + DutyType::Unknown, + DutyType::Proposer, + DutyType::Attester, + DutyType::Signature, + DutyType::Exit, + DutyType::BuilderProposer, + DutyType::BuilderRegistration, + DutyType::Randao, + DutyType::PrepareAggregator, + DutyType::Aggregator, + DutyType::SyncMessage, + DutyType::PrepareSyncContribution, + DutyType::SyncContribution, + DutyType::InfoSync, + ]; + for dt in &all { + let json = serde_json::to_value(dt).expect("serialize"); + let expected = json.as_str().expect("unit variants encode as strings"); + assert_eq!(dt.as_str(), Some(expected), "as_str for {dt:?}"); + assert_eq!(dt.to_string(), expected, "Display for {dt:?}"); + } + } + + /// `DutySentinel` has no flat string form; it keeps the JSON-object + /// rendering the previous serde round-trip produced. + #[test] + fn duty_type_sentinel_display_keeps_json_form() { + let sentinel = DutyType::DutySentinel(Box::new(DutyType::Attester)); + assert_eq!(sentinel.as_str(), None); + assert_eq!(sentinel.to_string(), r#"{"duty_sentinel":"attester"}"#); + } + #[test] fn duty_type_to_i32_literal_charon_numbers() { // Numbers are the canonical Charon core/types.go @ v1.7.1 enum values diff --git a/crates/p2p/src/conn_logger.rs b/crates/p2p/src/conn_logger.rs index 088467d3..f1ff21b9 100644 --- a/crates/p2p/src/conn_logger.rs +++ b/crates/p2p/src/conn_logger.rs @@ -263,9 +263,7 @@ impl NetworkBehaviour for ConnectionLogger // Drop cached identify addresses once the peer has no active // connections and is not a known cluster peer, to bound // `peer_addresses` growth. - if store.connections_to_peer(&event.peer_id).is_empty() - && !known.contains(&event.peer_id) - { + if !store.has_connection(&event.peer_id) && !known.contains(&event.peer_id) { store.remove_peer_addresses(&event.peer_id); } } diff --git a/crates/parsigex/src/behaviour.rs b/crates/parsigex/src/behaviour.rs index 4505150e..caa9590a 100644 --- a/crates/parsigex/src/behaviour.rs +++ b/crates/parsigex/src/behaviour.rs @@ -37,13 +37,17 @@ use crate::{ handler::{FromHandler, ToHandler}, }; -/// Future returned by verifier callbacks. -pub type VerifyFuture = - Pin> + Send + 'static>>; +/// Future returned by verifier callbacks. May borrow the data it verifies. +pub type VerifyFuture<'a> = + Pin> + Send + 'a>>; /// Verifier callback type. -pub type Verifier = - Arc VerifyFuture + Send + Sync + 'static>; +/// +/// The partial signature is borrowed: cloning it deep-copies a boxed +/// `SignedData`, up to a whole beacon block, once per entry per message. +pub type Verifier = Arc< + dyn for<'a> Fn(Duty, PubKey, &'a ParSignedData) -> VerifyFuture<'a> + Send + Sync + 'static, +>; /// Returns a [`Verifier`] that verifies each inbound partial signature against /// the sending peer's public share, looked up by the partial signature's share @@ -64,7 +68,7 @@ pub fn new_eth2_verifier( pub_shares_by_key: HashMap>, ) -> Verifier { let pub_shares_by_key = Arc::new(pub_shares_by_key); - Arc::new(move |duty, pubkey, par_signed_data| { + Arc::new(move |duty, pubkey, par_signed_data: &ParSignedData| { let eth2_cl = eth2_cl.clone(); let pub_shares_by_key = pub_shares_by_key.clone(); Box::pin(async move { @@ -694,7 +698,7 @@ mod eth2_verifier_tests { pub_shares_by_key.insert(group_pubkey, pub_shares); let verifier = new_eth2_verifier(client.clone(), pub_shares_by_key); - verifier(attester_duty(), group_pubkey, par) + verifier(attester_duty(), group_pubkey, &par) .await .expect("partial signature against the correct public share verifies"); } @@ -718,7 +722,7 @@ mod eth2_verifier_tests { pub_shares_by_key.insert(group_pubkey, pub_shares); let verifier = new_eth2_verifier(client.clone(), pub_shares_by_key); - let err = verifier(attester_duty(), group_pubkey, par) + let err = verifier(attester_duty(), group_pubkey, &par) .await .expect_err("partial signature against the wrong public share is rejected"); @@ -742,7 +746,7 @@ mod eth2_verifier_tests { let pub_shares_by_key = HashMap::new(); let verifier = new_eth2_verifier(client.clone(), pub_shares_by_key); - let err = verifier(attester_duty(), group_pubkey, par) + let err = verifier(attester_duty(), group_pubkey, &par) .await .expect_err("partial signature for an unknown pubkey is rejected"); @@ -767,7 +771,7 @@ mod eth2_verifier_tests { pub_shares_by_key.insert(group_pubkey, pub_shares); let verifier = new_eth2_verifier(client.clone(), pub_shares_by_key); - let err = verifier(attester_duty(), group_pubkey, par) + let err = verifier(attester_duty(), group_pubkey, &par) .await .expect_err("partial signature with an unknown share index is rejected"); diff --git a/crates/parsigex/src/handler.rs b/crates/parsigex/src/handler.rs index 963ecc60..6df08844 100644 --- a/crates/parsigex/src/handler.rs +++ b/crates/parsigex/src/handler.rs @@ -262,7 +262,7 @@ async fn do_recv( return Err(Failure::InvalidDuty); } for (pub_key, par_sig) in data_set.inner() { - verifier(duty.clone(), *pub_key, par_sig.clone()) + verifier(duty.clone(), *pub_key, par_sig) .await .map_err(|e| Failure::InvalidPartialSignature(e.to_string()))?; }