Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIP 109 boost by device type #798

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

michaeldjeffrey
Copy link
Contributor

@michaeldjeffrey michaeldjeffrey commented Apr 26, 2024

helium/proto#401
helium/helium-program-library#634


BoostedHexInfo.device_type added

Because legacy boosts exists (NULL in the database mapping to BoostedHexDeviceType::All), a single hex can now be considered boosted more than a single time. BoostedHexes carries n number of boosts per cell.

When asking for the current multiplier of a hex, the device type in question must be provided. All boosts that apply are combined.

parse_boosted_hex_info_from_database test

Because this project does not control the metadata db, I wanted to make sure BoostedHexDeviceType would correctly serialize as a part of BoostedHexInfo.

Looking at the helium-program-library implementation it wasn't clear how DeviceTypeV0 would make it's way into the database. There is a similar field DeviceType in mobile_hotspot_infos that has also uses a rust enum that is present as a jsonb column.

If that assumption turns out to be false, updating the test should lead us to the glory of correctly serializing device_type.

BoostedHexes and "expired boosts"

@bbalser and I talked about BoostedHexes only ever containing non-expired boosts.

Ideally, that would occur at the level of mobile-config when boosted hexes are being streamed out of the db. However, boosted hexes do not have a end_ts field that is easily queryable, and I'm not comfortable at this point in time putting that much math into a sql query.

I tried at each next level. In the end, I decided to give up on that goal for this PR. Tests using BoostedHexes directly would not have a way to enforce the contract of only holding actively boosted hexes, and now didn't feel like the correct time to dive into those tests to find a way they could.

@michaeldjeffrey michaeldjeffrey force-pushed the mj/hip-109-boost-by-device-type branch 3 times, most recently from cb43abb to 8904841 Compare April 30, 2024 22:31
@michaeldjeffrey
Copy link
Contributor Author

let (_, rewards) = tokio::join!(
    // run rewards for poc and dc
    rewarder::reward_poc_and_dc(
        &pool,
        &hex_boosting_client,
        &mobile_rewards_client,
        &speedtest_avg_client,
        &epoch,
        dec!(0.0001)
    ),
    receive_expected_rewards(&mut mobile_rewards)
);

When I tried to run rewarder::reward_poc_and_dc by itself, then receive the rewards, the test would never finish. I do not understand why.

@andymck
Copy link
Contributor

andymck commented May 2, 2024

let (_, rewards) = tokio::join!(
    // run rewards for poc and dc
    rewarder::reward_poc_and_dc(
        &pool,
        &hex_boosting_client,
        &mobile_rewards_client,
        &speedtest_avg_client,
        &epoch,
        dec!(0.0001)
    ),
    receive_expected_rewards(&mut mobile_rewards)
);

When I tried to run rewarder::reward_poc_and_dc by itself, then receive the rewards, the test would never finish. I do not understand why.

This will be because the filesink writer awaits a oneshot response to indicate the file/s have been written to disk. If you do not call receive_expected_rewards then this oneshot never gets returned and thus the rewarder's writer will wait indefinitely. See link below for an example of the oneshot response being sent from the tests receive handler: https://github.com/helium/oracles/blob/main/mobile_verifier/tests/common/mod.rs#L35

@andymck
Copy link
Contributor

andymck commented May 2, 2024

Other than clippy not being happy, this all looks good to me.

@michaeldjeffrey michaeldjeffrey force-pushed the mj/hip-109-boost-by-device-type branch 4 times, most recently from cbf911e to af92443 Compare May 6, 2024 21:10
Making the internal member hexes private will make it easier to change
the implementation when device type is introduced.
A cell can be boosted more than once.
Old boosts will retain device_type of ::All.

Any boost matching the device_type accumulates into the multiplier.
ex: 3 boost with a multiplier of 10, will multiply by 30.
The device_type column is assumed to be a jsonb, because the device_type
column for the `mobile_hotspot_infos` table is an enum, and a jsonb
field.

It's nullable, which will represent boosting a hex for ALL device types.

Otherwise, a camelcase value mapping to the DeviceTypeV0 in
helium-program-library.
https://github.com/helium/helium-program-library/blob/master/programs/hexboosting/src/state.rs

This is different from the snake_case values used with protobufs.
With type hints enabled, `hex` shows up as a `BoostedHex` going into a
function that expects `BoostedHex`. Renaming to `hex_proto` will
hopefully clear up the need for the type casting.
This test uses a single type of radio to reduce adding noise to rewards
by factors other than hex boosting.

The first pass of the test calculates rewards for 2 radios with no
boosted hexes to prove they are in equal standing.

The second pass boosts only the hex for single radio, but with multiple
BoostedHexDeviceType's to ensure they accumulate.
There was not enough custom functionality to warrant owning a
BoostedHexDeviceType enum of our own.
* Refactor to use BoostedHexes methods

Making the internal member hexes private will make it easier to change
the implementation when device type is introduced.

* Add device type to boosted hex info

* refactor metadata_db tests to make test clearer

also makes it easier to add new tests

* remove expired boosted hexes when streaming from db

* ensure no tests are written with expired boosted hexes

* optimize by computing end_ts in db query

Thanks for the query help Brian!

By precomputing the end timestamp of a boosted hex, we can not have to
stream all the hexes out of the db just to throw them away.

* fixup after rebase

- remove unused imports
- remove old refactor return types

* make boosted hex test function more explicit

If the expired check had been made a global check, the ability to use
BoostedHexes for modified hexes would have broken at runtime.

The attempt here is to make very explicit during testing how to meet the
same contract as the database queries for boosted hexes. I think there
are still some cracks, but we can narrow in on those as we find them.
For now, I think naming test constructor functions is a good start.
There is another test for same device types across hexes.
This test is for different types in the same hex.

The 2nd unboosted hex serves the purpose of giving us a relative
baseline so we don't need to hardcode values into the test.
It becomes more difficult to compare ratios with radio_reward_v2 because
boosted rewards are scaled. In many of the test we were hoping to have
nice ratios (1:10) of poc_rewards. However, this was mostly because a
radio would have received 0 boosted_rewards. Checking that ratio is
impossible.

We can however check the coverage_points (now that they mean points, and
not rewards) for the expected ratio, and rely on other tests to ensure
we're calculating poc_reward amount correctly.
@michaeldjeffrey michaeldjeffrey marked this pull request as ready for review September 6, 2024 18:45
coverage_map/src/indoor.rs Show resolved Hide resolved
coverage_map/src/indoor.rs Show resolved Hide resolved
mobile_config/src/boosted_hex_info.rs Show resolved Hide resolved
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.

5 participants