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

CSUB-347 weighted probabilistic threshold #814

Merged
merged 7 commits into from
Jan 6, 2023
Merged

Conversation

CAGS295
Copy link
Contributor

@CAGS295 CAGS295 commented Dec 13, 2022

Description of proposed changes:

Generate a fraction weighted by stake and a sample size fraction parameter.

Practical tips for PR review & merge:

  • All GitHub Actions report PASS
  • Newly added code/functions have unit tests
    • Coverage tools report all newly added lines as covered
    • The positive scenario is exercised
    • Negative scenarios are exercised, e.g. assert on all possible errors
    • Assert on events triggered if applicable
    • Assert on changes made to storage if applicable
  • Modified behavior/functions - try to make sure above test items are covered
  • Integration tests are added if applicable/needed

@github-actions
Copy link

For full LLVM coverage report click here!

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022

Codecov Report

Merging #814 (a6926b3) into dev (aa81365) will increase coverage by 0.08%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##              dev     #814      +/-   ##
==========================================
+ Coverage   78.06%   78.15%   +0.08%     
==========================================
  Files          66       67       +1     
  Lines       10601    10649      +48     
==========================================
+ Hits         8276     8323      +47     
- Misses       2325     2326       +1     
Impacted Files Coverage Δ
...allets/offchain-task-scheduler/src/mock/runtime.rs 100.00% <ø> (ø)
primitives/src/lib.rs 93.33% <ø> (ø)
runtime/src/lib.rs 24.25% <ø> (+0.61%) ⬆️
primitives/src/vrf.rs 93.87% <93.87%> (ø)
pallets/offchain-task-scheduler/src/ocw/tests.rs 100.00% <100.00%> (ø)
node/src/main.rs 16.66% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@CAGS295
Copy link
Contributor Author

CAGS295 commented Dec 19, 2022

blocked by #813

Bumps [jest](https://github.com/facebook/jest/tree/HEAD/packages/jest) from 29.2.1 to 29.3.1.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md)
- [Commits](https://github.com/facebook/jest/commits/v29.3.1/packages/jest)

---
updated-dependencies:
- dependency-name: jest
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@CAGS295 CAGS295 marked this pull request as ready for review January 3, 2023 20:53
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Added some questions around tests.

primitives/src/vrf.rs Outdated Show resolved Hide resolved
primitives/src/vrf.rs Show resolved Hide resolved
primitives/src/vrf.rs Show resolved Hide resolved
primitives/src/vrf.rs Outdated Show resolved Hide resolved
@CAGS295
Copy link
Contributor Author

CAGS295 commented Jan 5, 2023

Regarding the floating points...
The use of FP is fairly limited and is only used by worker threads to decide whether submitting transactions is appropriate.
The FLOPs don't look parallelizable. There may be accuracy loss but it is precise. Given that the hardware target supports standard FP64, I don't see why it could yield different results.
Doesn't risk balances directly, on average. The corner case is missing on rewards.

Nonetheless, I tried to get rid of it. I looked into parity's PerThings implementors and rust's fixed fixed point crate but I haven't found a way yet to implement 'fixed_point.pow(fixed_point)' with logs and exponents.

Not a strong argument but, Algorand uses FP in their main consensus in the same 'sortition' use case and Algorand has a strong peer-reviewed background. Maybe the use of FLOPS in this context is justified.

Help is welcomed, this is how far I've got.

Copy link
Contributor

@nathanwhit nathanwhit left a comment

Choose a reason for hiding this comment

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

A few comments, still looking at the floating point stuff so I'll leave comments on that later

primitives/Cargo.toml Outdated Show resolved Hide resolved
primitives/Cargo.toml Outdated Show resolved Hide resolved
primitives/src/vrf.rs Outdated Show resolved Hide resolved
@CAGS295 CAGS295 requested a review from atodorov January 6, 2023 04:56
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

LGTM for now. I will follow up with test related changes.

Copy link
Contributor

@nathanwhit nathanwhit left a comment

Choose a reason for hiding this comment

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

LGTM

@nathanwhit nathanwhit merged commit e1738cc into dev Jan 6, 2023
@nathanwhit nathanwhit deleted the feature/CSUB-347 branch January 6, 2023 17:43
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