Add 300Mbps speedtest validation with bounds checking #1031
Conversation
Implements SpeedtestValueOutOfBounds validation to reject speedtests exceeding 300MB upload/download limits.
@bbalser and I were reviewing this earlier, and believe we have a confusion between We should be checking these values against Megabits/ps. Probably this constant in the coverage point calculator should be renamed. The value is correct, but the name is incorrect. Should be I think all the tests should be using the |
kurotych
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements validation for speedtest data to prevent unrealistic values from affecting reward calculations. It adds bounds checking to reject speedtests exceeding 300 Mbps upload/download limits.
Key changes:
- Adds SpeedtestValueOutOfBounds validation logic with 300 Mbps threshold
- Extracts BYTES_PER_MEGABIT constant for reuse across modules
- Updates proto dependency to include new verification result enum
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| mobile_verifier/src/speedtests.rs | Implements bounds checking logic in validate_speedtest method |
| mobile_verifier/tests/integrations/speedtests.rs | Adds comprehensive test coverage for bounds validation scenarios |
| coverage_point_calculator/src/speedtest.rs | Extracts BYTES_PER_MEGABIT as public constant |
| Cargo.toml | Updates helium-proto dependency to branch with SpeedtestValueOutOfBounds enum |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| pubkey: hotspot.clone(), | ||
| serial: "test-serial".to_string(), | ||
| timestamp: Utc::now(), | ||
| upload_speed: mbps(400), // exceeds limit |
There was a problem hiding this comment.
[nitpick] Comment contains inconsistent unit reference. The limit and description are for '300Mbps' but the comment says 'exceeds limit' without specifying the unit consistently with the PR description.
| upload_speed: mbps(400), // exceeds limit | |
| upload_speed: mbps(400), // exceeds 300Mbps limit |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| #[sqlx::test] | ||
| async fn speedtest_download_exceeds_300mb_limit(pool: Pool<Postgres>) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Function name uses 'mb' but should use 'mbps' to be consistent with the speed measurement unit. The limit is 300 Mbps (megabits per second), not 300 MB (megabytes).
| async fn speedtest_download_exceeds_300mb_limit(pool: Pool<Postgres>) -> anyhow::Result<()> { | |
| async fn speedtest_download_exceeds_300mbps_limit(pool: Pool<Postgres>) -> anyhow::Result<()> { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| #[sqlx::test] | ||
| async fn speedtest_both_speeds_exceed_300mb_limit(pool: Pool<Postgres>) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Function name uses 'mb' but should use 'mbps' to be consistent with the speed measurement unit. The limit is 300 Mbps (megabits per second), not 300 MB (megabytes).
| async fn speedtest_both_speeds_exceed_300mb_limit(pool: Pool<Postgres>) -> anyhow::Result<()> { | |
| async fn speedtest_both_speeds_exceed_300mbps_limit(pool: Pool<Postgres>) -> anyhow::Result<()> { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| #[sqlx::test] | ||
| async fn speedtest_within_300mb_limit_should_be_valid(pool: Pool<Postgres>) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Function name uses 'mb' but should use 'mbps' to be consistent with the speed measurement unit. The limit is 300 Mbps (megabits per second), not 300 MB (megabytes).
| async fn speedtest_within_300mb_limit_should_be_valid(pool: Pool<Postgres>) -> anyhow::Result<()> { | |
| async fn speedtest_within_300mbps_limit_should_be_valid(pool: Pool<Postgres>) -> anyhow::Result<()> { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| #[sqlx::test] | ||
| async fn speedtest_exactly_300mb_limit_should_be_valid(pool: Pool<Postgres>) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Function name uses 'mb' but should use 'mbps' to be consistent with the speed measurement unit. The limit is 300 Mbps (megabits per second), not 300 MB (megabytes).
| async fn speedtest_exactly_300mb_limit_should_be_valid(pool: Pool<Postgres>) -> anyhow::Result<()> { | |
| async fn speedtest_exactly_300mbps_limit_should_be_valid(pool: Pool<Postgres>) -> anyhow::Result<()> { |
Copilot uses AI. Check for mistakes.
| latency: 80, // Poor tier | ||
| }, | ||
| }, | ||
| // Invalid speedtest - upload exceeds 300MB, should NOT be included |
There was a problem hiding this comment.
Comment uses 'MB' but should use 'Mbps' to be consistent with the speed measurement unit. The limit is 300 Mbps (megabits per second), not 300 MB (megabytes).
| // Invalid speedtest - upload exceeds 300MB, should NOT be included | |
| // Invalid speedtest - upload exceeds 300 Mbps, should NOT be included |
Copilot uses AI. Check for mistakes.
| latency: 90, // Poor tier | ||
| }, | ||
| }, | ||
| // Invalid speedtest - download exceeds 300MB, should NOT be included |
There was a problem hiding this comment.
Comment uses 'MB' but should use 'Mbps' to be consistent with the speed measurement unit. The limit is 300 Mbps (megabits per second), not 300 MB (megabytes).
| // Invalid speedtest - download exceeds 300MB, should NOT be included | |
| // Invalid speedtest - download exceeds 300 Mbps, should NOT be included |
Copilot uses AI. Check for mistakes.
| let avgs = speedtest_avg_receiver.finish().await?; | ||
|
|
||
| // Should have 2 average entries (one for each valid speedtest) | ||
| // Invalid speedtests with speeds > 300MB should NOT generate averages |
There was a problem hiding this comment.
Comment uses 'MB' but should use 'Mbps' to be consistent with the speed measurement unit. The limit is 300 Mbps (megabits per second), not 300 MB (megabytes).
| // Invalid speedtests with speeds > 300MB should NOT generate averages | |
| // Invalid speedtests with speeds > 300 Mbps should NOT generate averages |
Copilot uses AI. Check for mistakes.
Good catch, thank you! |
Implements SpeedtestValueOutOfBounds validation to not count speedtests exceeding 300Mbps upload/download limits.