fix: correct underflow in random_updated_at#4363
fix: correct underflow in random_updated_at#4363metalurgical wants to merge 4 commits intocowprotocol:mainfrom
random_updated_at#4363Conversation
Fix unchecked subtraction on `Instant`. Since `Duration` is not bounded by `Instant`, large inputs can cause underflow. Clamp to a valid past timestamp on underflow.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the random_updated_at function to use floating-point multiplication for age calculation and introduces checked_sub to prevent underflow panics when the calculated age exceeds the current time. A unit test was also added to verify this behavior. Feedback was provided regarding the fallback logic, which still contains an unchecked subtraction that could potentially panic if the system uptime is less than one second.
add additional tests
metalurgical
force-pushed
the
fix/random-updated-at-underflow
branch
from
4c187fa to
b66e558
Compare
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
after the saturaring_* changes, please test the edges of the rng (50% and 90%) explicitly too
| let age = max_age | ||
| .checked_mul(percent_expired) | ||
| .map(|age| age / 100) | ||
| .unwrap_or(Duration::MAX); |
There was a problem hiding this comment.
use saturating_mul instead
| now.checked_sub(age) | ||
| .or_else(|| now.checked_sub(Duration::from_secs(1))) | ||
| .unwrap_or(now) |
There was a problem hiding this comment.
use saturating_sub instead
| fn random_updated_at_underflow_check() { | ||
| let now = Instant::now(); | ||
| let max_age = Duration::MAX; | ||
| let mut rng = rand::rng(); |
There was a problem hiding this comment.
seed the rng for determinism
2 participants
Description
Fix unchecked subtraction on
Instant. SinceDurationis not bounded byInstant, large inputs can cause underflow. Clamp to a valid past timestamp on underflow, ornowif that could fail.Changes
How to test
cargo nextest run -p price-estimation random_updated_at_underflow_check
cargo nextest run -p price-estimation random_updated_at_range