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/2p ecdsa ios #27 #60

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

vhnatyk
Copy link

@vhnatyk vhnatyk commented Feb 23, 2019

Reworked pull request that covers all initial questions

  • Ability to call locally at least one function of 2-party ECDSA on iOS:
    all functions from 2-party ECDSA compile and run on iOS 👍
  • Documentation on how to run:
    added wiki page covering whole repo
  • Simple benches (execution time for functions of your choice):
    31.889s for Lindell 2017 bench_full_keygen_party_one_two on iPhone 6S (comparing to 4.0988s on Mac Mini i5 16GB RAM)

@omershlo
Copy link
Contributor

@vhnatyk as usual great work. really interested to what functions/operations take most of the running time. Maybe we can learn from it and improve

Copy link
Contributor

@omershlo omershlo left a comment

Choose a reason for hiding this comment

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

is this really necessary to include bin/gmp_ios.zip? what is being used for
same question but about the .rtf files in doc.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
@@ -34,7 +34,7 @@ reqwest = "0.9.5"
rocket = "0.4.0"
rocket_contrib = "0.4.0"
uuid = { version = "0.7", features = ["v4"] }
rust-crypto = "^0.2"
rust-crypto = { git = "https://github.com/vhnatyk/rust-crypto", branch="aarch64"} #"^0.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the change between the "official" rust-crypto and yours?
lets talk about this point because it is problematic in terms of upgradability

Copy link
Author

@vhnatyk vhnatyk Feb 25, 2019

Choose a reason for hiding this comment

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

I forked where author implemented method rust_crypto_util_fixed_time_eq_asm that was missing in original rust-crypto for arm64 - as rust-crypto is not actively developed (perhaps due to being quite feature-complete?), those changes were not merged to master. The method is not used neither in core of gg18, not in core of lindell2017, but in benches for lindell2017 - so it's an open question is it required ❓ I had to fork it, but maybe it's a good idea for KZen to fork it as rust-crypto isn't actively changing anymore anyway (just like you forked some other libs). The only chages from original rust-crypto master is that it Adds a definition of rust_crypto_util_fixed_time_eq_asm for ARMv8. It's exactly the same as for ARMv7, except that it uses ARMv8 names for registers (wN instead of rN) Looks to me like there are no good or obvious reasons why it wasn't merged to master of rust-crypto. @omershlo What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I use rust_crypto only for GG18 with network (in the bin folder). It is suppose to be possible to run the test without this library...
having said that - did you send a PR for rust_crypto?
If we couldn't make it go away we will probably fork it as you suggest.

Copy link
Author

Choose a reason for hiding this comment

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

Original PR from the branch that I forked is hanging there for quite a while(few years) - no point to do another PR with same changes I think, as rust_crypto seems not maintained anymore (looks like it was split into separate libraries that got abandoned too). Let me rethink a bit with this, I'm working with Android right now, and the changes don't compile there ok with GCC so I'm trying clang - but I found another repo, where author fixed for both iOS and Android for aarch64 and it compiles ok for both platforms. At the moment I'm analyzing those changes

Copy link
Author

Choose a reason for hiding this comment

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

Imho fork by KZen is the best option - changes for compatibility with aarch64 to rust_crypto_util_fixed_time_eq_asm are relatively minor and seem to look solid, hmm

@vhnatyk
Copy link
Author

vhnatyk commented Feb 25, 2019

is this really necessary to include bin/gmp_ios.zip? what is being used for
same question but about the .rtf files in doc.

it's not necessary for sure, but makes it much much easier to run on mobile for somebody new to Rust, GMP and cross-compilation. I added it for the sole purpose of making it easier to recreate. In terms of licensing, GMP can be distributed either in GPL or LGPL - both licensing models fit licensing model of KZen repo. But it's purely optional. Same applies to big rtf log file - probably can be replaced with cool fancy gifs 👍 that are pretty common (Dinghy also has demo gif btw)

@vhnatyk
Copy link
Author

vhnatyk commented Mar 13, 2019

is this really necessary to include bin/gmp_ios.zip? what is being used for
same question but about the .rtf files in doc.

it's not necessary for sure, but makes it much much easier to run on mobile for somebody new to Rust, GMP and cross-compilation. I added it for the sole purpose of making it easier to recreate. In terms of licensing, GMP can be distributed either in GPL or LGPL - both licensing models fit licensing model of KZen repo. But it's purely optional. Same applies to big rtf log file - probably can be replaced with cool fancy gifs 👍 that are pretty common (Dinghy also has demo gif btw)

removed the files (and cleaned up wiki)

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.

None yet

2 participants