Skip to content
This repository has been archived by the owner on Mar 5, 2024. It is now read-only.

Add support for hip17 #677

Merged
merged 20 commits into from
Dec 10, 2020
Merged

Add support for hip17 #677

merged 20 commits into from
Dec 10, 2020

Conversation

vihu
Copy link
Member

@vihu vihu commented Nov 12, 2020

Refer: https://github.com/helium/HIP/blob/master/0017-hex-density-based-transmit-reward-scaling.md

TODO:

  • Switch h3 to master branch
  • Cross verify reward calculcations
  • Run performance checks
  • Add support for interactivity based on last_poc_challenge

@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #677 (9032342) into master (e434162) will increase coverage by 0.24%.
The diff coverage is 76.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
+ Coverage   68.69%   68.93%   +0.24%     
==========================================
  Files          94       95       +1     
  Lines       12409    12665     +256     
==========================================
+ Hits         8524     8731     +207     
- Misses       3885     3934      +49     
Impacted Files Coverage Δ
...transactions/v1/blockchain_txn_poc_receipts_v1.erl 29.62% <0.00%> (ø)
src/ledger/v1/blockchain_ledger_snapshot_v1.erl 39.00% <31.03%> (-1.60%) ⬇️
...nsactions/v1/blockchain_txn_assert_location_v1.erl 87.16% <40.00%> (-0.88%) ⬇️
src/transactions/v1/blockchain_txn_vars_v1.erl 62.60% <67.44%> (+0.48%) ⬆️
src/ledger/v1/blockchain_ledger_v1.erl 76.48% <69.09%> (-0.39%) ⬇️
src/blockchain_hex.erl 85.14% <85.14%> (ø)
src/transactions/v1/blockchain_txn_rewards_v1.erl 92.73% <92.40%> (+0.66%) ⬆️
src/blockchain.erl 63.03% <100.00%> (+0.20%) ⬆️
src/transactions/blockchain_txn_mgr.erl 54.12% <0.00%> (-0.92%) ⬇️
src/blockchain_worker.erl 38.99% <0.00%> (-0.27%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e434162...9032342. Read the comment docs.

@vihu vihu force-pushed the rg/hip0017 branch 4 times, most recently from 7542e58 to 5049ba0 Compare November 15, 2020 19:32
src/blockchain_hex.erl Outdated Show resolved Hide resolved
src/blockchain_hex.erl Outdated Show resolved Hide resolved
src/blockchain_hex.erl Outdated Show resolved Hide resolved
@Vagabond
Copy link
Contributor

I think we should merge the h3dex branch into this and then rewrite this code to use that rather than the active_gateways thing.

src/blockchain_hex.erl Outdated Show resolved Hide resolved
@vihu vihu force-pushed the rg/hip0017 branch 2 times, most recently from baafda2 to db4d126 Compare November 30, 2020 19:15
@vihu vihu marked this pull request as ready for review December 4, 2020 21:31
vihu and others added 9 commits December 4, 2020 14:06
- Calculate hex densities
- Add test for calculated hex densities on ledger pinned at 587624
- Add scale_test and specs
- Add scale function for density
- Add scale_test
- Remove unnecessary hip17 chain vars
- Plug HIP17 calculations to reward txn
- Introduce yet another chain var for density_target_res
- Add scale for rx and tx rewards
- Calculate k-nearest hex; plumb assert update
- Correct some misunderstandings
- Add h3 to key EQC test
- Don't use active_gateways, use a fold
- Use tested h3 keying scheme
- Fix some other minor issues
- Fix scale test
- Switch to using h3dex for density map
- More efficent initialization of densities
- Remove nested loops
- Fix eunits to use updated reward fun
- Fix one more broken eunit and specs
- Fix seek tuple and allow +1 upper bound
- Add a simple known h3dex test
- Review comment: Verbose spec for densities
- Fix broken hip17 var validation
- Case ladder for hip17 vars
- Bootstrap hexes for tests
- Fix init_per_testcase for comparison test
- Enhance tests, fix some more reward txn bugs
- Add export scale test
- Add basic real-world perf test for rewards
- Dont build the full density map
- Add a temporary full cross verification test
- Fix scaling
- Refactor hex API, make dialyzer happy
- Adhere to update hex API in tests
- Start at k-ring of 2
- Fix exporting scale test data
- Cleanup now unneeded debugging
- Update limit function
- Define lower resolution bound for rewards
- Use chain var for lower bound res
- Update scale_test to actually do cross checking
- Update hip17 reward suite to run again
- Fix hex test suite
- Fix interactive filtering
- Fix log msg
- Add another crosscheck
src/blockchain_hex.erl Outdated Show resolved Hide resolved
src/blockchain_hex.erl Outdated Show resolved Hide resolved
%% Calculate clipped and unclipped densities
densities(H3Index, VarMap, Interactive, Ledger).

-spec densities(
Copy link
Contributor

@evanmcc evanmcc Dec 7, 2020

Choose a reason for hiding this comment

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

how hard would it be to make it so there's a precalc densities vs. on demand runtime switch so we could bench them against each other?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @vihu's original code did build a totally pre-calculated map but we kind of ripped it out when @Vagabond pointed out that most of it wouldn't be worthwhile and we'd have to keep rebuilding/refreshing it

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern was the total memory needed for a global map. Even this local map building, I think, can throw away more than it does right now by discarding higher resolution hexes once we've calculated the lower resolution densities.

case blockchain_hex:var_map(Ledger) of
{error, _} ->
%% do the old thing
get_rewards_for_epoch(Current+1, End, Chain, Vars, Ledger,
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just one of these with

VarMap = case blockchain_hex:var_map(Ledger) of {ok, VM} -> VM; _ -> #{} end,

above it? Are we actually doing anything different?

Copy link
Contributor

Choose a reason for hiding this comment

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

Planning to go through this today and see what we can do to make this suck less.

Copy link
Contributor

@evanmcc evanmcc left a comment

Choose a reason for hiding this comment

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

yay

@evanmcc evanmcc merged commit 7258701 into master Dec 10, 2020
@evanmcc evanmcc deleted the rg/hip0017 branch December 10, 2020 00:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants