-
Notifications
You must be signed in to change notification settings - Fork 10
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
WASM bindings #10
WASM bindings #10
Conversation
e1a025e
to
1555241
Compare
5573895
to
b8b2392
Compare
This PR is ready for review. |
Python bindings and benchmark
Benchmark WASM bindings
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.
Some preliminary comments
} | ||
|
||
pub fn from_bytes(bytes: &[u8]) -> Self { | ||
const NONCE_LEN: usize = 97; |
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.
Shouldn't we make these constants as class constants somehow? I don't know how this works in Rust.
Also, is there any reference why is this 97 bytes? This is a G1 element, so in principle it should be possible to fit in 48 bytes in compressed form and 96 bytes in uncompressed. Same for the auth_tag element, which is from G2 (96 bytes in compressed form, 192 bytes in uncompressed)
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 will replace those constants with a less-cryptic declaration.
The reason for these numbers is that arkworks implementation is also adding an "infinity sign" (bool
, one byte) to its groups.
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.
Can we used a compressed form for both G1 and G2 elements?
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.
In serialization, I will replace G1 and G2 usage with their compressed representation.
nonce_bytes.copy_from_slice(&bytes[..NONCE_LEN]); | ||
let nonce = E::G1Affine::read(&nonce_bytes[..]).unwrap(); | ||
|
||
const CIPHERTEXT_LEN: usize = 33; |
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.
Why is it 33 bytes? The ciphertext length shouldn't be constant as it varies with the message to be encrypted. Related to this, I'm thinking that the serialization order should be commitment, auth_tag, ciphertext
since the first 2 components have fixed size.
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.
Please see my previous comment about arkworks: https://github.com/nucypher/ferveo/pull/10/files/ca2e46e67637ce34d531da03124523fb567b7002#r1029195007
That being said, I expect to replace these serialization methods before the initial release #15
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.
But the ciphertext size depends on the message, not on arkworks stuff, right?
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.
Sorry, I misunderstood your question - I thought you were referring to struct Ciphertext<E: PairingEngine>
.
Yes, the ciphertext size depends on the message. const CIPHERTEXT_LEN: usize = 33;
is not actually used anywhere! It's a left-over artifact of some refactoring that I missed. For some reason, it wasn't picked up by the linter. I'm going to remove this line.
@@ -0,0 +1,79 @@ | |||
# WASM results |
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's the purpose of this file as part of the codebase? Is it expected to change regularly?
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 expect it to change at least once after we finalize changes to the DKG scheme (simple vs fast decryption). We also introduced other changes that I want to keep track of.
@@ -40,7 +40,6 @@ impl<E: PairingEngine> Ciphertext<E> { | |||
let mut bytes = Vec::new(); | |||
self.commitment.write(&mut bytes).unwrap(); | |||
self.auth_tag.write(&mut bytes).unwrap(); | |||
bytes.extend_from_slice(&self.ciphertext); |
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.
oops! it seems you removed here the ciphertext from the serialized output!
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 find it very worrisome that this wasn't caught by tests.
Closes #6
Note for reviewers: This PR should be reviewed and merged first: Add AAD to API #8serde
#15