From c47d8dc1682c0be318a2838f3af4a6a7bc80d22c Mon Sep 17 00:00:00 2001 From: metalurgical <97008724+metalurgical@users.noreply.github.com> Date: Sun, 26 Apr 2026 13:20:32 +0200 Subject: [PATCH 1/6] fix: correct underflow in `random_updated_at` Fix underflow when subtracting large durations from Instant in random_updated_at. - Compute age as a percentage of max_age - Use saturating_mul to avoid overflow when calculating age - Clamp Instant subtraction underflow to now via checked_sub(...).unwrap_or(now) - Ensure percentage does not exceed 100 - Adds coverage tests --- Cargo.toml | 1 + crates/price-estimation/Cargo.toml | 1 + .../src/native_price_cache.rs | 72 +++++++++++++++++-- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7ad6cdcfab..3eaba96cdc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -147,6 +147,7 @@ tracing-subscriber = { version = "0.3.22", features = ["json"] } url = "2.5.0" vergen = "8" winner-selection = { path = "crates/winner-selection" } +rand_chacha = { version = "0.9.0" } [workspace.lints] clippy.cast_possible_wrap = "deny" diff --git a/crates/price-estimation/Cargo.toml b/crates/price-estimation/Cargo.toml index 42f08ca145..e117f5396e 100644 --- a/crates/price-estimation/Cargo.toml +++ b/crates/price-estimation/Cargo.toml @@ -61,6 +61,7 @@ mockall = { workspace = true } testlib = { workspace = true } token-info = { workspace = true, features = ["test-util"] } toml = { workspace = true } +rand_chacha = { workspace = true } [features] test-util = ["dep:mockall"] diff --git a/crates/price-estimation/src/native_price_cache.rs b/crates/price-estimation/src/native_price_cache.rs index 95768316e4..4c17663f23 100644 --- a/crates/price-estimation/src/native_price_cache.rs +++ b/crates/price-estimation/src/native_price_cache.rs @@ -193,13 +193,21 @@ impl Cache { self.0.max_age } - /// Returns a randomized `updated_at` timestamp that is 50-90% of max_age - /// in the past, to avoid spikes of expired prices all being fetched at - /// once. + /// Returns a timestamp that is a `percentage` of `max_age` + /// in the past. Panics if `percentage` > 100. Clamps to `now` if an + /// underflow occurs. + fn updated_at_percentage(max_age: Duration, now: Instant, percentage: u32) -> Instant { + assert!(percentage <= 100, "percentage > 100"); + let age = max_age.saturating_mul(percentage) / 100; + now.checked_sub(age).unwrap_or(now) + } + + /// Returns a randomized `updated_at_percentage` timestamp that is 50–90% of + /// `max_age` in the past, to avoid spikes of expired prices all being + /// fetched at once. fn random_updated_at(max_age: Duration, now: Instant, rng: &mut impl Rng) -> Instant { - let percent_expired = rng.random_range(50..=90); - let age = max_age.as_secs() * percent_expired / 100; - now - Duration::from_secs(age) + let percent_expired: u32 = rng.random_range(50..=90); + Self::updated_at_percentage(max_age, now, percent_expired) } fn len(&self) -> usize { @@ -544,6 +552,7 @@ mod tests { anyhow::anyhow, futures::FutureExt, num::ToPrimitive, + rand_chacha::{ChaCha20Rng, rand_core::SeedableRng}, }; fn token(u: u64) -> Address { @@ -1143,4 +1152,55 @@ mod tests { anyhow!("protocol") )))); } + + #[test] + fn random_updated_at_underflow_check() { + let now = Instant::now(); + let max_age = Duration::MAX; + let mut rng = ChaCha20Rng::from_seed(Default::default()); + + let updated_at = Cache::random_updated_at(max_age, now, &mut rng); + assert_eq!(updated_at, now); + } + + #[test] + #[should_panic(expected = "percentage > 100")] + fn updated_at_percent_panic() { + let now = Instant::now(); + let max_age = Duration::MAX; + Cache::updated_at_percentage(max_age, now, 101); + } + + #[test] + fn updated_at_percent_edges() { + let now = Instant::now(); + let max_age = Duration::from_secs(600); + let cases = [ + (0, now), // 0% + (50, now - Duration::from_secs(300)), // 50% + (90, now - Duration::from_secs(540)), // 90% + (100, now - Duration::from_secs(600)), // 100% + ]; + + for (percentage, expected) in cases { + assert_eq!( + Cache::updated_at_percentage(max_age, now, percentage), + expected, + ); + } + } + + #[test] + fn random_updated_at_range() { + let now = Instant::now(); + let max_age = Duration::from_secs(600); + let mut rng = ChaCha20Rng::from_seed(Default::default()); + let min = now - (max_age * 90 / 100); + let max = now - (max_age * 50 / 100); + + for _ in 0..100 { + let updated_at = Cache::random_updated_at(max_age, now, &mut rng); + assert!(updated_at >= min && updated_at <= max); + } + } } From eebb9ae60766e38225375e7a133291b3a1b89f60 Mon Sep 17 00:00:00 2001 From: metalurgical <97008724+metalurgical@users.noreply.github.com> Date: Mon, 27 Apr 2026 20:02:13 +0200 Subject: [PATCH 2/6] update: cargo.lock --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index 02a5c983e4..8af244f2c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6392,6 +6392,7 @@ dependencies = [ "prometheus", "prometheus-metric-storage", "rand 0.9.4", + "rand_chacha 0.9.0", "rate-limit", "request-sharing", "reqwest 0.13.2", From 2ce1231ab5429ee24a3694f8287ba8705cb86dd9 Mon Sep 17 00:00:00 2001 From: metalurgical <97008724+metalurgical@users.noreply.github.com> Date: Mon, 27 Apr 2026 21:29:59 +0200 Subject: [PATCH 3/6] refactor: remove additional dependency and percent assert - Switch to `StdRng::seed_from_u64` for deterministic tests - Remove `assert` on percentage <= 100 - Remove `rand_chacha` dependency. --- Cargo.lock | 1 - Cargo.toml | 1 - crates/price-estimation/Cargo.toml | 1 - .../price-estimation/src/native_price_cache.rs | 18 ++++-------------- 4 files changed, 4 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8af244f2c2..02a5c983e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6392,7 +6392,6 @@ dependencies = [ "prometheus", "prometheus-metric-storage", "rand 0.9.4", - "rand_chacha 0.9.0", "rate-limit", "request-sharing", "reqwest 0.13.2", diff --git a/Cargo.toml b/Cargo.toml index 3eaba96cdc..7ad6cdcfab 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -147,7 +147,6 @@ tracing-subscriber = { version = "0.3.22", features = ["json"] } url = "2.5.0" vergen = "8" winner-selection = { path = "crates/winner-selection" } -rand_chacha = { version = "0.9.0" } [workspace.lints] clippy.cast_possible_wrap = "deny" diff --git a/crates/price-estimation/Cargo.toml b/crates/price-estimation/Cargo.toml index e117f5396e..42f08ca145 100644 --- a/crates/price-estimation/Cargo.toml +++ b/crates/price-estimation/Cargo.toml @@ -61,7 +61,6 @@ mockall = { workspace = true } testlib = { workspace = true } token-info = { workspace = true, features = ["test-util"] } toml = { workspace = true } -rand_chacha = { workspace = true } [features] test-util = ["dep:mockall"] diff --git a/crates/price-estimation/src/native_price_cache.rs b/crates/price-estimation/src/native_price_cache.rs index 4c17663f23..a13cb8cdbb 100644 --- a/crates/price-estimation/src/native_price_cache.rs +++ b/crates/price-estimation/src/native_price_cache.rs @@ -194,10 +194,8 @@ impl Cache { } /// Returns a timestamp that is a `percentage` of `max_age` - /// in the past. Panics if `percentage` > 100. Clamps to `now` if an - /// underflow occurs. + /// in the past. Clamps to `now` if an underflow occurs. fn updated_at_percentage(max_age: Duration, now: Instant, percentage: u32) -> Instant { - assert!(percentage <= 100, "percentage > 100"); let age = max_age.saturating_mul(percentage) / 100; now.checked_sub(age).unwrap_or(now) } @@ -552,7 +550,7 @@ mod tests { anyhow::anyhow, futures::FutureExt, num::ToPrimitive, - rand_chacha::{ChaCha20Rng, rand_core::SeedableRng}, + rand::{SeedableRng, rngs::StdRng}, }; fn token(u: u64) -> Address { @@ -1157,20 +1155,12 @@ mod tests { fn random_updated_at_underflow_check() { let now = Instant::now(); let max_age = Duration::MAX; - let mut rng = ChaCha20Rng::from_seed(Default::default()); + let mut rng = StdRng::seed_from_u64(0); let updated_at = Cache::random_updated_at(max_age, now, &mut rng); assert_eq!(updated_at, now); } - #[test] - #[should_panic(expected = "percentage > 100")] - fn updated_at_percent_panic() { - let now = Instant::now(); - let max_age = Duration::MAX; - Cache::updated_at_percentage(max_age, now, 101); - } - #[test] fn updated_at_percent_edges() { let now = Instant::now(); @@ -1194,7 +1184,7 @@ mod tests { fn random_updated_at_range() { let now = Instant::now(); let max_age = Duration::from_secs(600); - let mut rng = ChaCha20Rng::from_seed(Default::default()); + let mut rng = StdRng::seed_from_u64(0); let min = now - (max_age * 90 / 100); let max = now - (max_age * 50 / 100); From 6851e9501fe3a722491197a446f35f0ed6c120d9 Mon Sep 17 00:00:00 2001 From: metalurgical <97008724+metalurgical@users.noreply.github.com> Date: Tue, 28 Apr 2026 11:35:31 +0200 Subject: [PATCH 4/6] refactor: percentage clamp If percentage > 100, clamp 100 --- crates/price-estimation/src/native_price_cache.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/price-estimation/src/native_price_cache.rs b/crates/price-estimation/src/native_price_cache.rs index a13cb8cdbb..4477619be9 100644 --- a/crates/price-estimation/src/native_price_cache.rs +++ b/crates/price-estimation/src/native_price_cache.rs @@ -196,6 +196,7 @@ impl Cache { /// Returns a timestamp that is a `percentage` of `max_age` /// in the past. Clamps to `now` if an underflow occurs. fn updated_at_percentage(max_age: Duration, now: Instant, percentage: u32) -> Instant { + let percentage = if percentage > 100 { 100 } else { percentage }; let age = max_age.saturating_mul(percentage) / 100; now.checked_sub(age).unwrap_or(now) } From ef8fe0f558cc93b2c4994d7ffbd1fc443238d70d Mon Sep 17 00:00:00 2001 From: metalurgical <97008724+metalurgical@users.noreply.github.com> Date: Tue, 28 Apr 2026 19:24:40 +0200 Subject: [PATCH 5/6] fix: update underflow check to exclude random_range() --- crates/price-estimation/src/native_price_cache.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/price-estimation/src/native_price_cache.rs b/crates/price-estimation/src/native_price_cache.rs index 4477619be9..e10e21dcfe 100644 --- a/crates/price-estimation/src/native_price_cache.rs +++ b/crates/price-estimation/src/native_price_cache.rs @@ -1153,12 +1153,15 @@ mod tests { } #[test] - fn random_updated_at_underflow_check() { + fn updated_at_percentage_underflow_check() { let now = Instant::now(); - let max_age = Duration::MAX; - let mut rng = StdRng::seed_from_u64(0); - - let updated_at = Cache::random_updated_at(max_age, now, &mut rng); + let age = Duration::MAX; + // Check against maximum values from random_updated_at(). + // Duration::MAX is max_age + // (max_age * 90) overflows, gets clamped to Duration::MAX + // producing (Duration::MAX / 100) as age + // (now - age) underflows, clamps to now + let updated_at = Cache::updated_at_percentage(age, now, 90); assert_eq!(updated_at, now); } From ac2cef5cbd2d6bbc44f67d1307568063f7e68d76 Mon Sep 17 00:00:00 2001 From: metalurgical <97008724+metalurgical@users.noreply.github.com> Date: Wed, 29 Apr 2026 09:41:42 +0200 Subject: [PATCH 6/6] update: check subtaction consistency Discard absurd values when representation is possible but not logically correct. --- crates/price-estimation/src/native_price_cache.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/price-estimation/src/native_price_cache.rs b/crates/price-estimation/src/native_price_cache.rs index e10e21dcfe..72419105c8 100644 --- a/crates/price-estimation/src/native_price_cache.rs +++ b/crates/price-estimation/src/native_price_cache.rs @@ -198,7 +198,17 @@ impl Cache { fn updated_at_percentage(max_age: Duration, now: Instant, percentage: u32) -> Instant { let percentage = if percentage > 100 { 100 } else { percentage }; let age = max_age.saturating_mul(percentage) / 100; - now.checked_sub(age).unwrap_or(now) + if let Some(updated_at) = now.checked_sub(age) { + // check subtraction is consistent, should get age back otherwise treat it as + // invalid and clamp to now + if now.duration_since(updated_at) == age { + updated_at + } else { + now + } + } else { + now + } } /// Returns a randomized `updated_at_percentage` timestamp that is 50–90% of