Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Dev/2p ecdsa ios #27 #60
Changes from 8 commits
bcfcbf5
23b28fb
0b7b63a
f952bfb
f5f27c1
516e6bf
1006629
1c4face
3401d0d
126b481
f733db9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 originalrust-crypto
master is that itAdds 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