Modeled Coverage - Phase 3 #559

maplant · 2023-06-28T19:16:33Z

This is mostly complete, there are two remaining tasks that should be relatively quick to complete:

Convert unit tests in the reward_share module over
Add a cache to the heartbeat deamon to fetch coverage objects so we can invalidate heartbeats that use incorrect coverage objects

andymck · 2023-06-29T10:21:20Z

file_store/src/coverage.rs

+
+    fn try_from(v: CoverageObjectIngestReportV1) -> Result<Self> {
+        Ok(Self {
+            received_timestamp: v.received_timestamp.to_timestamp_millis()?,


any reason not to implement the MsgTimestamp trait to handle these timestamps here and in subsequent msgs below ?

Eh, just didn't see a reason to implement it to only be used one other place

I think its a matter of convention and ensuring consistency.

Cargo.toml

mobile_verifier/migrations/13_modeled_coverage.sql

mobile_verifier/src/coverage.rs

file_store/src/coverage.rs

andymck · 2023-06-29T11:35:03Z

mobile_verifier/src/reward_shares.rs

 }

-impl PocShares {
-    pub async fn aggregate(
+impl CoveragePoints {


nit, but points are really just shares. i had the same issue with subscriber rewards. personally i think its better to stick to the shares naming convention

mobile_verifier/src/coverage.rs

andymck · 2023-06-29T12:15:44Z

mobile_verifier/src/reward_shares.rs

+            // Guaranteed that points contains the given hotspot.
+            *coverage_points
+                .get_mut(&hotspot)
+                .unwrap()


the olde unwrap usage

andymck

this looks to be doing the right thing, will give it another pass when conflicts and tests are fixed

andymck · 2023-06-30T10:54:31Z

mobile_verifier/src/coverage.rs

+                        indoor,
+                        // It's pretty weird that we have to convert these back and forth, but it
+                        // shouldn't be too inefficient
+                        coverage: coverage.clone().into_iter().map(Into::into).collect(),


Following on from the above point, i think this is where the usefulness of having a full suite of conversions in the filestore comes into play ( allowing you to go from internal struct to proto and any child structs such as in this case the coverage struct)

for these coverage objects do we actually need to have an internal type that mirrors the proto type of can we just use the proto type? it looks to me like the only reason we have the internal type is to convert the hex string to an h3o::CellIndex and that's only used to cast to a u64 here for inserting into the db. i think you can avoid this conversion by just having the internal coverage object contain a vec of proto::RadioSignalLevel, do the full string -> CellIndex -> u64 here where you need it and avoid this iter/map/collect

my take is that we should have an internal type and especially where we are deriving the source data from the DB and then subsequently manipulating that data. I would then argue if we then have to convert the internal type to the proto type that should be handled in the filestore where we can have conversions centralised and have a standard set of conventions ( such as timestamp traits ). Generating protos on the fly leaves more opportunity for mishaps such as mis-setting a timestamp ( secs insteads of millis for example )

jeffgrunewald · 2023-07-05T17:54:42Z

Cargo.toml

@@ -97,6 +97,7 @@ itertools = "*"
 data-credits = {git = "https://github.com/helium/helium-program-library.git", tag = "v0.1.0"}
 helium-sub-daos = {git = "https://github.com/helium/helium-program-library.git", tag = "v0.1.0"}
 price-oracle = {git = "https://github.com/helium/helium-program-library.git", tag = "v0.1.0"}
+uuid = {version = "1.3.4", features = ["v4", "serde"]}


maybe not a concern depending on the expected volume of coverage objects, but ulid-rs may be good to keep in mind for "lexicographically sortable" IDs that are UUIDv4 compatible to prevent too much fragmentation in the database

I would absolutely prefer ulids, but we are given the uuids. we don't decide them

jeffgrunewald · 2023-07-07T14:33:45Z

file_store/src/coverage.rs

+
+    fn try_from(v: RadioHexSignalLevelProto) -> Result<Self> {
+        Ok(Self {
+            signal_level: SignalLevel::from_i32(v.signal_level).ok_or_else(|| {


can you use v.signal_level() or are you explicitly avoiding the proto behavior of using the default (lowest/first) element of the signal level enum if the oracle doing the try_from() has an older version of the proto code that doesn't account for the variant in a newer message?

This just happens to be the pattern I see everywhere else in the file store

file_store/src/error.rs

ingest/src/settings.rs

mobile_verifier/migrations/13_modeled_coverage.sql

mobile_verifier/src/coverage.rs

jeffgrunewald · 2023-07-07T16:35:22Z

mobile_verifier/src/coverage.rs

+#[derive(Clone, FromRow)]
+pub struct HexCoverage {
+    pub uuid: Uuid,
+    pub hex: i64,


is this i64 useful anywhere or would it be better to custom impl FromRow and just do the conversion into an h3o::CellIndex directly out of the db?

It is not, but having a custom FromRow impl seems excessive

jeffgrunewald · 2023-07-07T16:37:24Z

mobile_verifier/src/coverage.rs

+        }) = covered_hexes.next().await.transpose()?
+        {
+            self.hexes
+                .entry(CellIndex::try_from(hex as u64).unwrap())


this isn't a guaranteed safe operation unless we're sure we only inserted valid values into the db; h3ron didn't do validation so we've been bitten by trusting registered hexes before once we switched over to h3o

jeffgrunewald · 2023-07-07T16:59:36Z

mobile_verifier/src/heartbeats.rs

+              FROM heartbeats t1
+              WHERE t1.latest_timestamp = (
+                SELECT MAX(t2.latest_timestamp)
+                FROM heartbeats t2


can this be done with a group-by on cbsd_id to get the latest timestamp per radio instead of having a triple-nested select statement? or have the single sub-select order results by timestamp and only take the first (highest) one?

I am really not sure. We need to fetch the max latest timestamp and the min first timestamp of a group of at least 12 of cbsd_id

mobile_verifier/src/heartbeats.rs

Co-authored-by: Jeff Grunewald <jeff.grunewald@gmail.com>

mobile_verifier/src/coverage.rs

jeffgrunewald · 2023-07-08T19:08:52Z

mobile_verifier/src/coverage.rs

+}
+
+impl CachedCoverage {
+    pub fn max_distance_km(&self, latlng: LatLng) -> f64 {


would it be more accurate or more efficient to do this distance calculation with the native h3 grid_distance functions (grid_disk_distances_fast/safe())? could we confidently say due to RF propagation that coverage has to be within N range of the k-ring from the "source", get the (CellIndex, u32) iterator and return the furthest of those that's also a member of the coverage vec?

Co-authored-by: Jeff Grunewald <jeff.grunewald@gmail.com>

bbalser · 2023-07-25T14:03:29Z

mobile_verifier/src/heartbeats.rs

@@ -136,11 +187,31 @@ impl HeartbeatReward {
    ) -> impl Stream<Item = Result<HeartbeatReward, sqlx::Error>> + 'a {
        sqlx::query_as::<_, HeartbeatKey>(
            r#"
-            SELECT hotspot_key, cbsd_id, cell_type
+            WITH coverage_objs AS (


Is it possible for this query to pull a new coverage_object that is attached to a heartbeat that came after the rewardable period?

bbalser · 2023-07-25T14:12:14Z

mobile_verifier/src/coverage.rs

+            .await?;
+
+        // Is this a valid delete?
+        sqlx::query("DELETE FROM hex_coverage WHERE cbsd_id = $1 AND uuid != $2 AND coverage_claim_time < $3")


Can't this query delete a new coverage_object where the coverage_claim_time is in the past?

bbalser · 2023-07-25T18:25:28Z

mobile_verifier/src/reward_shares.rs

+            .map(|poc_rewards_per_share| {
+                let start_period = epoch.start.encode_timestamp();
+                let end_period = epoch.end.encode_timestamp();
+                self.coverage_points


There has to be a way to make this more readable

jeffgrunewald · 2023-07-29T14:29:16Z

file_store/src/heartbeat.rs

+}
+
+impl CellHeartbeat {
+    pub fn coverage_object(&self) -> Option<Uuid> {


Suggested change

pub fn coverage_object(&self) -> Option<Uuid> {

pub fn coverage_object_id(&self) -> Option<Uuid> {

jeffgrunewald · 2023-07-31T17:02:57Z

mobile_verifier/src/coverage.rs

+        cbsd_id: &'a str,
+        coverage_obj: &'a Uuid,
+        period_end: DateTime<Utc>,
+    ) -> Result<BoxStream<'a, Result<HexCoverage, sqlx::Error>>, sqlx::Error>;


are these errors overly prescriptive for a trait? why have the trait at all if it always assumes a postgres/sql source?

jeffgrunewald · 2023-07-31T17:11:37Z

mobile_verifier/src/coverage.rs

+        )
+        .bind(cbsd_id)
+        .bind(period_end)
+        .fetch_one(self)


is always guaranteed to be one? what happens when we first activate the feature and it's populating the new record types?

bbalser · 2023-08-21T12:37:31Z

mobile_verifier/migrations/15_modeled_coverage.sql

@@ -21,6 +21,7 @@ CREATE TABLE seniority (
       seniority_ts TIMESTAMPTZ NOT NULL,
       last_heartbeat TIMESTAMPTZ NOT NULL,
       uuid UUID NOT NULL,
+       update_reason INT NOT NULL,


why is update_reason an int and not an enum?

Because then we can have it be the same type as the proto enum

maplant added 6 commits June 21, 2023 15:11

Add ingest service for coverage object

0479b5f

implement basic algorithm for computing coverage points

498685e

Fix bug where multiple signal levels would get mixed

bab457f

Implement logic for modeled coverage

60814f7

Fmt

e52dc60

Convert rewarding to take coverage into account

e35ee73

maplant requested review from andymck, bbalser and jeffgrunewald June 28, 2023 19:16

andymck reviewed Jun 29, 2023

View reviewed changes

andymck reviewed Jun 30, 2023

View reviewed changes

maplant added 2 commits July 3, 2023 14:22

Convert over most of the reward share tests, add fixes

7e04c77

Port over one more test

54bcaed

jeffgrunewald reviewed Jul 7, 2023

View reviewed changes

maplant and others added 2 commits July 7, 2023 17:50

Address remaining issues

ca0434d

Update file_store/src/error.rs

da1ccb2

Co-authored-by: Jeff Grunewald <jeff.grunewald@gmail.com>

jeffgrunewald reviewed Jul 8, 2023

View reviewed changes

maplant and others added 11 commits July 10, 2023 10:27

Update mobile_verifier/src/coverage.rs

8a1e2a6

Co-authored-by: Jeff Grunewald <jeff.grunewald@gmail.com>

Fix tests, add setting, improve some of the code

57da60b

Add coverage_object column to heartbeats

ba97a7e

s/no/none/g

74d6eb8

Correct hex_coverage table

64fa0b3

Merge remote-tracking branch 'origin/main' into map/modeled-coverage

8c52ceb

Fmt and Fix

a1c254d

Move coverage object around to satisfy OCD

6619bfd

Clippy

99505a3

Fix CellHeartbeat to allow backwards compat w/ empty coverage_object

6b5c69e

Fix coverage claim time with first_timestamp

035e4a9

maplant added 9 commits July 18, 2023 13:30

Merge remote-tracking branch 'origin/main' into map/modeled-coverage

d886af8

Fix coverage claim time adjustment

5cd1b75

Fix seniority

fee3eb3

CMove to insert or update model for seniority

e35c82b

Refactor coverage and heartbeats a little bit

985c990

Merge remote-tracking branch 'origin/main' into map/modeled-coverage

d066c29

Refactor into_rewards a bit

a71a601

Clippy

1deea42

Delete old hex coverages

9c88a27

bbalser reviewed Jul 25, 2023

View reviewed changes

Fix deletion and validated heartbeats stream

45638d5

bbalser reviewed Jul 25, 2023

View reviewed changes

maplant added 2 commits July 26, 2023 14:41

Unnet into_rewards

8201e1b

Clippy

9301ad7

maplant changed the title ~~Modeled Coverage~~ Modeled Coverage - Phase 3 Jul 31, 2023

jeffgrunewald approved these changes Jul 31, 2023

View reviewed changes

maplant added 3 commits August 2, 2023 15:05

Embed heartbeat object

b8b719b

Add cbsd_id to seniority deletion

eccb3f5

Address issues with seniority

c2827a8

bbalser reviewed Aug 21, 2023

View reviewed changes

maplant added 8 commits August 22, 2023 10:13

Merge remote-tracking branch 'origin/main' into map/modeled-coverage

83f0ce6

Use timestamp instead of datetime

12b9c96

Refactor seniority updates to make them more easily testable

e288a10

First seniority unit test

cf76f30

Add tests for seniority updates

25d787a

Merge remote-tracking branch 'origin/main' into map/modeled-coverage

34057e7

Add initial coverage object tests

9ec0f69

Add modeled coverage integration tests

9b38ab4

maplant closed this Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modeled Coverage - Phase 3 #559

Modeled Coverage - Phase 3 #559

maplant commented Jun 28, 2023 •

edited

Loading

andymck Jun 29, 2023

maplant Jul 13, 2023

andymck Jul 17, 2023

andymck Jun 29, 2023

andymck Jun 29, 2023

andymck left a comment

andymck Jun 30, 2023

jeffgrunewald Jul 7, 2023

andymck Jul 17, 2023

jeffgrunewald Jul 5, 2023

maplant Jul 13, 2023

jeffgrunewald Jul 7, 2023

maplant Jul 13, 2023

jeffgrunewald Jul 7, 2023

maplant Jul 13, 2023

jeffgrunewald Jul 7, 2023

jeffgrunewald Jul 7, 2023

maplant Jul 13, 2023

jeffgrunewald Jul 8, 2023

bbalser Jul 25, 2023

bbalser Jul 25, 2023

bbalser Jul 25, 2023

jeffgrunewald Jul 29, 2023

jeffgrunewald Jul 31, 2023

jeffgrunewald Jul 31, 2023

bbalser Aug 21, 2023

maplant Aug 28, 2023

	pub fn coverage_object(&self) -> Option<Uuid> {
	pub fn coverage_object_id(&self) -> Option<Uuid> {

Modeled Coverage - Phase 3 #559

Modeled Coverage - Phase 3 #559

Conversation

maplant commented Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andymck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maplant commented Jun 28, 2023 •

edited

Loading