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

Dev/benchmarks #45 #58

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

vhnatyk
Copy link

@vhnatyk vhnatyk commented Feb 18, 2019

Dummy PR - still may require many different changes, just a draft for @omershlo to review the change to gg18/keygen.rs that I referred to as Black Magik 😄. Also, I found a way to run everything on mobile since there is a branch in rust-crypto where aarch64 was fixed.

replaced cryptography_utils with curv, used latest version of
.phase1_verify_com_phase3_verify_correct_key_phase2_distribute

Squashed commits:
[21e0b57] re-enabled two party keygen bench
@vhnatyk vhnatyk mentioned this pull request Feb 18, 2019
@omershlo
Copy link
Contributor

@vhnatyk it seems you are using an old version of keygen. Can you use try and use the latest in master please?

@vhnatyk
Copy link
Author

vhnatyk commented Feb 18, 2019

@vhnatyk it seems you are using an old version of keygen. Can you use try and use the latest in master please?

Umm, sorry - seems it's latest gg18/keygen.rs, and has only one commit 7483f3880b2995ecf83db25ecafe860f0df29c59. Maybe you refer to 2p keygen bench - lindell_2017/keygen.rs - that one is latest

@omershlo
Copy link
Contributor

Oh I see the problem!
The keygen for gg18 in benches that you are referring to is not up to date - compare it with the test in master: https://github.com/KZen-networks/multi-party-ecdsa/blob/master/src/protocols/multi_party_ecdsa/gg_2018/test.rs#L56

If you want to get rid of the black magic - copy the code from test.rs instead of what we have now in benches. let me know if you prefer me to do it [its suppose to be straight forward]
It will also allow you to run with different t,n parameters easily.

@vhnatyk
Copy link
Author

vhnatyk commented Feb 18, 2019

If it's just simple overwriting update to gg18\keygen.rs - yes pls update, otherwise if some merging etc - probably I can do it and you'll review. Cool - let's get rid of Black Magik ! 🤘 - I will rerun benches with latest

@omershlo
Copy link
Contributor

I pushed the new GG18 keygen benchmarks to master.

  • I changed Cargo.toml to point benchmark to this file
  • write now I bench two random cases (t,n = 1,2 and 2,3), feel free to expend and add stuff according to Benchmarks! #45

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.

2 participants