-
Notifications
You must be signed in to change notification settings - Fork 313
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
base: master
Are you sure you want to change the base?
Conversation
@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 |
There was a problem hiding this 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
@@ -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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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) |
Reworked pull request that covers all initial questions
all functions from 2-party ECDSA compile and run on iOS 👍
added wiki page covering whole repo
31.889s for Lindell 2017 bench_full_keygen_party_one_two on iPhone 6S (comparing to 4.0988s on Mac Mini i5 16GB RAM)