Skip to content
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 207 additions & 9 deletions crates/cli/src/commands/test/validator.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,26 @@
//! Validator client connectivity tests.

use super::{TestCategoryResult, TestConfigArgs};
use crate::error::Result;
use std::{collections::HashMap, io::Write, sync::mpsc, time::Duration};

use clap::Args;
use std::{io::Write, time::Duration};
use rand::Rng;
use tokio::{
net::TcpStream,
time::{Instant, timeout},
};

use super::{
AllCategoriesResult, TestCaseName, TestCategory, TestCategoryResult, TestConfigArgs,
TestResult, TestVerdict, calculate_score, evaluate_highest_rtt, evaluate_rtt, filter_tests,
publish_result_to_obol_api, sort_tests, write_result_to_file, write_result_to_writer,
};
use crate::{duration::Duration as CliDuration, error::Result};

// Thresholds (from Go implementation)
const THRESHOLD_MEASURE_AVG: Duration = Duration::from_millis(50);
const THRESHOLD_MEASURE_POOR: Duration = Duration::from_millis(240);
const THRESHOLD_LOAD_AVG: Duration = Duration::from_millis(50);
const THRESHOLD_LOAD_POOR: Duration = Duration::from_millis(240);

/// Arguments for the validator test command.
#[derive(Args, Clone, Debug)]
Expand All @@ -30,10 +47,191 @@ pub struct TestValidatorArgs {
}

/// Runs the validator client tests.
pub async fn run(_args: TestValidatorArgs, _writer: &mut dyn Write) -> Result<TestCategoryResult> {
// TODO: Implement validator tests
// - Ping
// - PingMeasure
// - PingLoad
unimplemented!("validator test not yet implemented")
pub async fn run(args: TestValidatorArgs, writer: &mut dyn Write) -> Result<TestCategoryResult> {
tracing::info!("Starting validator client test");

let start_time = Instant::now();

// Get and filter test cases
let all_test_cases = HashMap::from([
(TestCaseName::new("Ping", 1), ()),
(TestCaseName::new("PingMeasure", 2), ()),
(TestCaseName::new("PingLoad", 3), ()),
]);
let mut queued_tests = filter_tests(&all_test_cases, args.test_config.test_cases.as_deref());

if queued_tests.is_empty() {
return Err(crate::error::CliError::Other(
"test case not supported".into(),
));
}

sort_tests(&mut queued_tests);

// Run tests with timeout
let test_results = tokio::time::timeout(args.test_config.timeout, async {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the finished tests, only show timeout on real timeout tests

let mut results = Vec::new();
for test in queued_tests.iter() {
let result = match test.name.as_str() {
"Ping" => ping_test(&args).await,
"PingMeasure" => ping_measure_test(&args).await,
"PingLoad" => ping_load_test(&args).await,
_ => TestResult::new(&test.name).fail(std::io::Error::other("unknown test")),
};
results.push(result);
}
results
})
.await
.unwrap_or_else(|_| {
vec![TestResult::new("Timeout").fail(std::io::Error::other("timeout interrupted"))]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timeout is not a test name, in charon implementation, they are using the real test name

});
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the timeout occurs at line 86, a single "Timeout" test result is returned for all tests. This masks which specific test timed out and loses information about any tests that completed before the timeout. Consider either returning partial results with a timeout error for the incomplete test, or including information about which test was running when the timeout occurred.

Suggested change
// Run tests with timeout
let test_results = tokio::time::timeout(args.test_config.timeout, async {
let mut results = Vec::new();
for test in queued_tests.iter() {
let result = match test.name.as_str() {
"Ping" => ping_test(&args).await,
"PingMeasure" => ping_measure_test(&args).await,
"PingLoad" => ping_load_test(&args).await,
_ => TestResult::new(&test.name).fail(std::io::Error::other("unknown test")),
};
results.push(result);
}
results
})
.await
.unwrap_or_else(|_| {
vec![TestResult::new("Timeout").fail(std::io::Error::other("timeout interrupted"))]
});
// Run tests with overall timeout budget, tracking partial results and test-level timeouts.
let mut test_results = Vec::new();
for test in queued_tests.iter() {
// Compute remaining time for the overall timeout budget.
let elapsed = start_time.elapsed();
let Some(remaining) = args.test_config.timeout.checked_sub(elapsed) else {
// No time left before starting this test: mark this specific test as timed out.
test_results.push(
TestResult::new(&test.name).fail(std::io::Error::other("timeout interrupted")),
);
break;
};
// Run the individual test with the remaining timeout budget.
let fut = async {
match test.name.as_str() {
"Ping" => ping_test(&args).await,
"PingMeasure" => ping_measure_test(&args).await,
"PingLoad" => ping_load_test(&args).await,
_ => {
TestResult::new(&test.name)
.fail(std::io::Error::other("unknown test"))
}
}
};
match timeout(remaining, fut).await {
Ok(result) => test_results.push(result),
Err(_) => {
// This specific test exceeded the remaining timeout budget.
test_results.push(
TestResult::new(&test.name)
.fail(std::io::Error::other("timeout interrupted")),
);
break;
}
}
}

Copilot uses AI. Check for mistakes.
let score = calculate_score(&test_results);

let mut res = TestCategoryResult::new(TestCategory::Validator);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was a bit confused as this was the first time writing this, so I took AI's suggestion to write the run function. Do take a look if any changes are needed here.

res.targets.insert(args.api_address.clone(), test_results);
res.execution_time = Some(CliDuration::new(start_time.elapsed()));
res.score = Some(score);

if !args.test_config.quiet {
write_result_to_writer(&res, writer)?;
}

if !args.test_config.output_json.is_empty() {
write_result_to_file(&res, args.test_config.output_json.as_ref()).await?;
}

if args.test_config.publish {
let all = AllCategoriesResult {
validator: Some(res.clone()),
..Default::default()
};
publish_result_to_obol_api(
all,
&args.test_config.publish_addr,
&args.test_config.publish_private_key_file,
)
.await?;
}

Ok(res)
}

async fn ping_test(args: &TestValidatorArgs) -> TestResult {
let mut result = TestResult::new("Ping");

match timeout(
Duration::from_secs(1),
TcpStream::connect(&args.api_address),
)
.await
{
Ok(Ok(_conn)) => {
result.verdict = TestVerdict::Ok;
}
Ok(Err(e)) => {
return result.fail(e);
}
Err(_) => {
return result.fail(std::io::Error::new(
std::io::ErrorKind::TimedOut,
"connection timeout",
));
}
}

result
}

async fn ping_measure_test(args: &TestValidatorArgs) -> TestResult {
let mut result = TestResult::new("PingMeasure");
let before = Instant::now();

match timeout(
Duration::from_secs(1),
TcpStream::connect(&args.api_address),
)
.await
{
Ok(Ok(_conn)) => {
let rtt = before.elapsed();
result = evaluate_rtt(rtt, result, THRESHOLD_MEASURE_AVG, THRESHOLD_MEASURE_POOR);
}
Ok(Err(e)) => {
return result.fail(e);
}
Err(_) => {
return result.fail(std::io::Error::new(
std::io::ErrorKind::TimedOut,
"connection timeout",
));
}
}

result
}

async fn ping_load_test(args: &TestValidatorArgs) -> TestResult {
tracing::info!(
duration = ?args.load_test_duration,
target = %args.api_address,
"Running ping load tests..."
);

let mut result = TestResult::new("PingLoad");

let (tx, rx) = mpsc::channel::<Duration>();
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using std::sync::mpsc with async code is not recommended. The standard library's mpsc channel is designed for sync code and can cause blocking in async contexts. Since this is async code using tokio, consider using tokio::sync::mpsc instead, which is specifically designed for async/await and won't block the async runtime.

Copilot uses AI. Check for mistakes.
let address = args.api_address.clone();
let duration = args.load_test_duration;

let handle = tokio::spawn(async move {
let start = Instant::now();
let mut interval = tokio::time::interval(Duration::from_secs(1));

Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interval tick at line 192 causes the first iteration to fire immediately, but then subsequent iterations wait 1 second. This creates inconsistent timing in the load test. The first ping_continuously task starts immediately, but subsequent tasks start at 1-second intervals. Consider using interval.tick().await before the loop or skip the first tick to ensure consistent spacing between spawned tasks.

Suggested change
// Discard the first immediate tick so that all subsequent ticks are spaced by 1 second.
interval.tick().await;

Copilot uses AI. Check for mistakes.
while start.elapsed() < duration {
interval.tick().await;

let tx = tx.clone();
let addr = address.clone();
let remaining = duration.saturating_sub(start.elapsed());

tokio::spawn(async move {
ping_continuously(addr, tx, remaining).await;
});
}
Comment on lines +250 to +265
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The load test can spawn an unbounded number of concurrent tasks. For a 5-second test, up to 5 tasks are spawned (one per second), each running ping_continuously. However, for longer durations, this could spawn many concurrent tasks. Additionally, each ping_continuously task continuously creates connections in a loop with only a small random sleep between attempts (0-100ms). This could create hundreds or thousands of concurrent connections, potentially overwhelming the system or the target validator. Consider limiting the maximum number of concurrent tasks or the connection rate per task.

Copilot uses AI. Check for mistakes.
});

let _ = handle.await;

let mut rtts = Vec::new();
while let Ok(rtt) = rx.try_recv() {
rtts.push(rtt);
Comment on lines +271 to +272
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a race condition in the ping_load_test function. After the spawned tasks complete (line 204), the code immediately tries to collect RTT results using try_recv. However, the spawned tasks in lines 198-200 may not have finished sending their results yet because they're not awaited. This means some RTT measurements could be lost, leading to incomplete or inaccurate test results.

The issue is that when handle.await completes, it only means the spawning loop finished, not that all the spawned ping_continuously tasks have completed and sent their RTTs. Consider using a bounded channel or waiting for all tasks to complete before collecting results.

Suggested change
while let Ok(rtt) = rx.try_recv() {
rtts.push(rtt);
loop {
match rx.recv() {
Ok(rtt) => rtts.push(rtt),
Err(_) => break,
}

Copilot uses AI. Check for mistakes.
}

tracing::info!(target = %args.api_address, "Ping load tests finished");

Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no RTT measurements are collected (rtts is empty), evaluate_highest_rtt returns a zero duration which gets marked as "Poor" by evaluate_rtt. This could be misleading - if no measurements were collected due to connection failures, the test should probably be marked as "Fail" rather than "Poor". Consider explicitly checking if rtts is empty and returning an appropriate failure result.

Suggested change
if rtts.is_empty() {
return result.fail(std::io::Error::new(
std::io::ErrorKind::Other,
"no RTT measurements collected during ping load test",
));
}

Copilot uses AI. Check for mistakes.
result = evaluate_highest_rtt(rtts, result, THRESHOLD_LOAD_AVG, THRESHOLD_LOAD_POOR);

result
}

async fn ping_continuously(address: String, tx: mpsc::Sender<Duration>, max_duration: Duration) {
let start = Instant::now();

while start.elapsed() < max_duration {
let before = Instant::now();

match timeout(Duration::from_secs(1), TcpStream::connect(&address)).await {
Ok(Ok(_conn)) => {
let rtt = before.elapsed();
if tx.send(rtt).is_err() {
return;
}
}
Comment on lines +288 to +295
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TCP connections established during tests are not being explicitly closed. While Rust will drop them when the variable goes out of scope, it's better practice to explicitly close connections in test scenarios to avoid exhausting file descriptors during load tests. Consider explicitly dropping the connection or storing it in a variable that's immediately dropped after measuring the RTT.

Copilot uses AI. Check for mistakes.
_ => return,
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling in ping_continuously is too aggressive. When any connection error or timeout occurs (line 231 matches all non-Ok(Ok(_)) cases), the function immediately returns. This means a single connection failure terminates the entire load test for that task, even though load tests are typically designed to measure performance under sustained load and should tolerate occasional failures. Consider logging the error and continuing the test instead of returning early.

Suggested change
return;
}
}
_ => return,
// Receiver has been dropped; no point in continuing.
return;
}
}
Ok(Err(e)) => {
tracing::warn!(target = %address, error = ?e, "Ping connection attempt failed during load test");
// Continue attempting pings until max_duration elapses.
}
Err(e) => {
tracing::warn!(target = %address, error = ?e, "Ping connection attempt timed out during load test");
// Continue attempting pings until max_duration elapses.
}

Copilot uses AI. Check for mistakes.
}

let sleep_ms = rand::thread_rng().gen_range(0..100);
tokio::time::sleep(Duration::from_millis(sleep_ms)).await;
}
}