Skip to content

Conversation

@kurotych
Copy link
Member

@kurotych kurotych commented Mar 31, 2025

Migration info: It has taken 1m48s to run all migrations in this PR on the local DB setup.
UPD migration info: It takes 44s to run all migrations in this PR on the local labnet DB setup (04.22.25)

Mobile verifier:

  • Remove verify_legacy function and grandfathered_radio_threshold TABLE.
  • DELETE cbsd_id from radio_threshold table (migration) and radio_threshold.rs module
  • Remove selecting cbrs_heartbeats from valid_radios.sql
  • Remove cell_type from struct HeartbeatReward
  • Make distances_to_asserted not optional in HeartbeatReward
  • DROP old_hex_coverage table
  • Add migration DELETE FROM seniority WHERE radio_type = 'cbrs' which deletes around 500k rows (75%) in seniority table.
  • Add migration DELETE FROM coverage_objects WHERE radio_type = 'cbrs'; which deletes around 35k rows (55%) in coverage_objects table.

@kurotych kurotych force-pushed the remove-cbrs-db-part-2 branch 2 times, most recently from 5d4a8ea to 08338a3 Compare March 31, 2025 18:10
@kurotych kurotych changed the base branch from main to upd-radio-hashmap March 31, 2025 18:27
@kurotych kurotych changed the base branch from upd-radio-hashmap to main March 31, 2025 18:27
@kurotych kurotych force-pushed the remove-cbrs-db-part-2 branch 2 times, most recently from 08338a3 to a3c8d4f Compare March 31, 2025 18:30
@kurotych kurotych requested a review from Copilot April 2, 2025 09:31
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes CBRS-related functionality from the database layer and updates associated tests and data structures accordingly.

  • Removed legacy CBRS verification and associated database tables/columns.
  • Updated HeartbeatReward to remove the cell_type field and to require non-optional distances_to_asserted.
  • Adjusted SQL queries and test cases to reflect these changes.

Reviewed Changes

Copilot reviewed 4 out of 11 changed files in this pull request and generated 1 comment.

File Description
mobile_verifier/tests/integrations/heartbeats.rs Removed cell_type from test cases and updated distances usage.
mobile_verifier/src/reward_shares.rs Updated iterator usage to handle errors from the new API.
mobile_verifier/src/radio_threshold.rs Removed legacy CBRS verification and adjusted insert/delete queries.
mobile_verifier/src/heartbeats/mod.rs Removed cell_type from HeartbeatReward and added error checking for distances vs. multipliers.
Files not reviewed (7)
  • mobile_verifier/migrations/42_drop_grandfathered_radio_threshold.sql: Language not supported
  • mobile_verifier/migrations/43_radio_threshold_rework_unique_idx.sql: Language not supported
  • mobile_verifier/migrations/44_drop_cbrs_heartbeats.sql: Language not supported
  • mobile_verifier/migrations/45_drop_old_hex_coverage.sql: Language not supported
  • mobile_verifier/migrations/46_delete_cbrs_from_seniority.sql: Language not supported
  • mobile_verifier/migrations/47_delete_cbrs_from_coverage_objects.sql: Language not supported
  • mobile_verifier/src/heartbeats/valid_radios.sql: Language not supported
Comments suppressed due to low confidence (2)

mobile_verifier/src/radio_threshold.rs:257

  • Ensure that the complete removal of legacy handling does not inadvertently affect other report verifications, especially in contexts that might still expect legacy behavior.
let is_legacy = self.verify_legacy(&report.hotspot_pubkey, &None).await?;

mobile_verifier/src/heartbeats/mod.rs:237

  • [nitpick] Consider adding logging or additional context when a length mismatch is detected in iter_distances_and_scores to simplify debugging if data inconsistencies occur.
if self.trust_score_multipliers.len() != self.distances_to_asserted.len() {

VALUES ($1, $2, $3, $4, true, $5)
ON CONFLICT (hotspot_pubkey)
Copy link

Copilot AI Apr 2, 2025

Choose a reason for hiding this comment

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

Verify that using only hotspot_pubkey as the unique constraint does not allow duplicate entries, given that cbsd_id has been removed from the conflict target.

Suggested change
VALUES ($1, $2, $3, $4, true, $5)
ON CONFLICT (hotspot_pubkey)
VALUES ($1, $2, $3, $4, $5, true, $6)
ON CONFLICT (hotspot_pubkey, cbsd_id)

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

Verify that using only hotspot_pubkey as the unique constraint does not allow duplicate entries

In migration 43_radio_threshold_rework_unique_idx.sql unique index reworked to only hotspot_pubkey field

...
CREATE UNIQUE INDEX radio_threshold_hotspot_pubkey_idx ON radio_threshold (hotspot_pubkey);

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know that this unique constraint holds in the current mainnet db?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tested migrations on the one of the mobile verifier backups and it is worked.

But I will check it again before merge on the latest mainnet backup.

@kurotych kurotych marked this pull request as ready for review April 2, 2025 09:39
@helium helium deleted a comment from michaeldjeffrey Apr 3, 2025
@kurotych
Copy link
Member Author

kurotych commented Apr 3, 2025

The comment I accidentally deleted:

@michaeldjeffrey commented on this pull request.

In mobile_verifier/src/radio_threshold.rs:

@@ -374,8 +345,7 @@ pub async fn verified_radio_thresholds(
reward_period: &Range<DateTime>,
) -> Result<VerifiedRadioThresholds, sqlx::Error> {
let mut rows = sqlx::query_as::<_, RadioThreshold>(

We're only grabbing a single field, this query can be simplified

let gateways = sqlx::query_scalar::<, PublicKeyBinary>(
"SELECT hotspot_pubkey
FROM radio_threshold WHERE threshold_timestamp < $1 and cbsd_id IS NULL",
)
.bind(reward_period.end)
.fetch_all(pool)
.await?
.into_iter()
.collect::<HashSet<
>>();

Ok(VerifiedRadioThresholds { gateways })

And we can get rid of RadioThreshold.

@kurotych kurotych force-pushed the remove-cbrs-db-part-2 branch from 565069c to 4f07901 Compare April 3, 2025 10:01
@kurotych
Copy link
Member Author

kurotych commented Apr 3, 2025

The comment I accidentally deleted:

@michaeldjeffrey commented on this pull request.

In mobile_verifier/src/radio_threshold.rs:

@@ -374,8 +345,7 @@ pub async fn verified_radio_thresholds(
reward_period: &Range,
) -> Result<VerifiedRadioThresholds, sqlx::Error> {
let mut rows = sqlx::query_as::<_, RadioThreshold>(

We're only grabbing a single field, this query can be simplified

let gateways = sqlx::query_scalar::<, PublicKeyBinary>( "SELECT hotspot_pubkey FROM radio_threshold WHERE threshold_timestamp < $1 and cbsd_id IS NULL", ) .bind(reward_period.end) .fetch_all(pool) .await? .into_iter() .collect::<HashSet<>>();

Ok(VerifiedRadioThresholds { gateways })

And we can get rid of RadioThreshold.

@michaeldjeffrey done

@bbalser
Copy link
Collaborator

bbalser commented Apr 21, 2025

Looks good, need to renumber the migrations

@kurotych kurotych force-pushed the remove-cbrs-db-part-2 branch from 6d50d88 to e6ae2fc Compare April 21, 2025 15:32
@kurotych
Copy link
Member Author

Looks good, need to renumber the migrations

@bbalser Done

@kurotych kurotych merged commit 52f8e53 into main Apr 22, 2025
53 checks passed
@kurotych kurotych deleted the remove-cbrs-db-part-2 branch April 22, 2025 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants