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.

Sign up for GitHub

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

Jump to bottom

Add repair share functionality #281

Merged
merged 22 commits into from
Apr 24, 2023
Merged

Add repair share functionality #281

merged 22 commits into from
Apr 24, 2023

Conversation

natalieesk
Copy link
Contributor

This closes #41

natalieesk and others added 9 commits March 13, 2023 17:30
@natalieesk
Add compute random value function for repair share functionality (#41)
f1f52ff 
This is step 1 of 3
@natalieesk
Add compute random value function for repair share functionality for …
00ad274 
…each ciphersuite (#41)
@natalieesk
Add compute_sum_of_random_values function for repair share functional…
3aa30d6 
…ity (#41)

This is step 2 of 3
@natalieesk
Add recover_share function for repair share functionality (#41)
70eb71e 
This is step 3 of 3
@natalieesk
Add communication rounds functions for repair share functionality for…
84134cd 
… each ciphersuite (#41)

Add compute_sum_of_random_variables function
Add recover_share function
@natalieesk
Fix recover_share tests so they test the right thing
9a99472 
Fix secp256 recover share test values
Fix ristretto255 recover share test values
Fix ristretto255 compute sum of random values test values
@natalieesk
Rewrite compute_random_values to generate_random_values for repair sh…
298520d 
…are functionality (#41)

Test generate_random_values directly
End to end test to be added in another commit
Updated gendoc to use original file values to fix clippy complaints
@natalieesk
Rename functions and update documentation for repair (#41)
6a6ab0a
@natalieesk @conradoplg
Add end to end test for repair share (#41)
35ed98d 
Fix lagrange coefficient calculation

Co-authored-by: conrado <conradoplg@gmail.com>
@conradoplg conradoplg mentioned this pull request Mar 13, 2023
Closed
@codecov
Copy link

codecov bot commented Mar 13, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.96 🎉

Comparison is base (6554f2b) 70.12% compared to head (da1907a) 71.09%.

❗ Current head da1907a differs from pull request most recent head 6b80689. Consider uploading reports for the commit 6b80689 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #281      +/-   ##
==========================================
+ Coverage   70.12%   71.09%   +0.96%     
==========================================
  Files          24       25       +1     
  Lines        2186     2259      +73     
==========================================
+ Hits         1533     1606      +73     
  Misses        653      653
Impacted Files Coverage Δ
frost-core/src/frost/keys.rs 80.57% <ø> (ø)
frost-core/src/frost/keys/repairable.rs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

conradoplg
conradoplg requested changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

I left some comments to improve the documentation of the functions. The suggestions are just suggestions to give a sense of the details needed, feel free to change the wordings.

frost-core/src/frost/keys/repairable.rs Show resolved Hide resolved
frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/tests/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/tests/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
frost-ed25519/src/keys/repairable.rs Show resolved Hide resolved
conradoplg
conradoplg requested changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! There is one last part that I'm not sure about

frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
frost-core/src/frost/keys/repairable.rs Outdated Show resolved Hide resolved
@natalieesk @conradoplg
Update repairable documentation
03e99db 
Co-authored-by: Conrado Gouvea <conrado@zfnd.org>
@conradoplg
Copy link
Contributor

@natalieesk I left an additional comment. Could you please also solve the .gitignore conflict, and update the branch to make it pass CI? (git pull origin main)

@conradoplg
Copy link
Contributor

@natalieesk I think now it's just missing syncing with main (git fetch && git merge origin/main) and solving the build issues (i.e. fix the test so that it works with the new keygen_with_dealer which now returns a HashMap)

@conradoplg conradoplg mentioned this pull request Apr 24, 2023
Closed
conradoplg
conradoplg approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! 🎉

@mergify mergify bot merged commit 0b98161 into main Apr 24, 2023
@mergify mergify bot deleted the repair_share_41 branch April 24, 2023 15:27
@mpguerra mpguerra linked an issue May 23, 2023 that may be closed by this pull request
Feature: remove signers or refresh shares with trusted dealer #245
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dconnolly dconnolly dconnolly left review comments

@conradoplg conradoplg conradoplg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@natalieesk @conradoplg @dconnolly