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

Add JNI code for ed25519-zebra #37

Merged
merged 10 commits into from
Feb 26, 2021
Merged

Add JNI code for ed25519-zebra #37

merged 10 commits into from
Feb 26, 2021

Conversation

droark
Copy link
Contributor

@droark droark commented Jan 7, 2021

Add some code allowing other languages, via JNI, to interact with ed25519-zebra. The initial commit:

  • Allows users to obtain a random 32 byte signing key seed.
  • Allows users to obtain a 32 byte verification key from a signing key seed.
  • Allows users to sign arbitrary data.
  • Allows users to verify an Ed25519 signature.
  • Includes a Java file that can be used.
  • Includes some Scala-based JNI tests.

Add some code allowing other languages, via JNI, to interact with ed25519-zebra. The initial commit:

- Allows users to obtain a random 32 byte signing key seed.
- Allows users to obtain a 32 byte verification key from a signing key seed.
- Allows users to sign arbitrary data.
- Allows users to verify an Ed25519 signature.
- Includes a Java file that can be used.
- Includes some Scala-based JNI tests.
ed25519jni/.cargo/config Outdated Show resolved Hide resolved
ed25519jni/Cargo.toml Outdated Show resolved Hide resolved
ed25519jni/src/main/rust/lib.rs Outdated Show resolved Hide resolved
ed25519jni/src/main/rust/utils/exception.rs Outdated Show resolved Hide resolved
ed25519jni/src/main/rust/lib.rs Outdated Show resolved Hide resolved
- Minor Rust code optimizations.
- Rust build optimizations.
- Tweak the JNI JAR prereq script to match the new outputs.
@droark droark requested a review from yaahc January 27, 2021 21:03
@droark
Copy link
Contributor Author

droark commented Jan 27, 2021

@yaahc - Folded in your requested changes. The build system might not be fully optimized, though. I noticed that libed25519jni.a and libed25519jni.{so/dylib} appear in both target/{mode} and target/{mode}/deps. Is there a way to get those files to appear only once? I'm assuming it's something subtle that I missed.

Thanks.

@droark
Copy link
Contributor Author

droark commented Jan 28, 2021

@yaahc - Pushed some more build system changes. They partially undid your requested changes, such as removing the workspace. If you think it's worthwhile to put that back, I'm happy to discuss it. Things are fine on my end but I don't mind moving things around if they'll improve things or fit standard Rust patterns.

- More build system tidying. The primary goal is to try to firewall the JNI code from everything else.
- README tidying.
Douglas Roark added 2 commits January 31, 2021 22:24
- Clean up the wrapper classes (streamlining, make constructors private, more mutability safety).
- private -> public for a static variable intended for public usage.
- Minor comment & build system cleanup.
Decided to bump the version to reflect earlier changes.
ed25519jni/rust/Cargo.toml Outdated Show resolved Hide resolved
@yaahc
Copy link

yaahc commented Feb 2, 2021

@yaahc - Pushed some more build system changes. They partially undid your requested changes, such as removing the workspace. If you think it's worthwhile to put that back, I'm happy to discuss it. Things are fine on my end but I don't mind moving things around if they'll improve things or fit standard Rust patterns.

oh, so you removed the workspace but didn't add back the .cargo/config. That's probably fine, just need to be careful to make sure our CI still runs the tests in the subcrate. Out of curiosity what was the issue with the workspace that led you to remove it?

@droark
Copy link
Contributor Author

droark commented Feb 4, 2021

@yaahc - I'm a little confused. I was under the impression that .cargo/config wasn't necessary. It also seemed to be messing with the path within the target directory.

Regarding why it was added in the first place, it's a bit of cruft. At first, the Java and the Rust code were basically sitting on top of each other. The config file got added in order to have a place where target files could go. Once everything got separated and worked out on my end, the file didn't seem to be necessary anymore. If it's causing CI problems, I can definitely reassess.

Douglas Roark added 2 commits February 4, 2021 12:49
Also add "-JNI" to assist with tagging and otherwise distinguish the JNI code from the main library version/code.
@yaahc
Copy link

yaahc commented Feb 4, 2021

@yaahc - I'm a little confused. I was under the impression that .cargo/config wasn't necessary. It also seemed to be messing with the path within the target directory.

Oh, you're right, it isn't necessary. I thought you'd used it to deduplicate the target/ directory between the inner and the outer crate so they can share compilation artifacts. This is why I suggested switching to a workspace, because one of the features of a workspace is that all crates in the workspace share a single target directory. So when you removed both I was confused because I assumed this would cause issues with target/ directories for you that had been a problem before, but it seems that is not the case, so it's fine.

Regarding why it was added in the first place, it's a bit of cruft. At first, the Java and the Rust code were basically sitting on top of each other. The config file got added in order to have a place where target files could go. Once everything got separated and worked out on my end, the file didn't seem to be necessary anymore. If it's causing CI problems, I can definitely reassess.

👍

Douglas Roark added 3 commits February 22, 2021 14:24
Also add a test suite for VerificationKeyBytes.
- Fix hashCode() override.
- Add a test.
- Remove unneecessary semicolons.
Mirror the Signature struct from Rust and add some basic tests. Also do a bit of Scala test cleanup.
@droark droark requested a review from yaahc February 26, 2021 18:54
@droark
Copy link
Contributor Author

droark commented Feb 26, 2021

@yaahc - Okay, I think we're finally golden. :) The last 2-3 commits should be all that you need to review but I understand if you want to review the whole thing again. My apologies for always adding stuff. This should be it for awhile.

@yaahc yaahc merged commit 2abe8b9 into ZcashFoundation:main Feb 26, 2021
@teor2345 teor2345 mentioned this pull request Apr 15, 2021
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