WASM bindings #10
WASM bindings #10
Conversation
piotr-roslaniec
force-pushed
the
wasm-bindings
branch
2 times, most recently
from
e1a025e to
1555241
Compare
piotr-roslaniec
force-pushed
the
wasm-bindings
branch
from
5573895 to
b8b2392
Compare
|
This PR is ready for review. |
Python bindings and benchmark
Benchmark WASM bindings
| } | ||
|
|
||
| pub fn from_bytes(bytes: &[u8]) -> Self { | ||
| const NONCE_LEN: usize = 97; |
There was a problem hiding this comment.
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.
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.
Can we used a compressed form for both G1 and G2 elements?
There was a problem hiding this comment.
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.
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.
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.
But the ciphertext size depends on the message, not on arkworks stuff, right?
There was a problem hiding this comment.
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.
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.
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.
oops! it seems you removed here the ciphertext from the serialized output!
There was a problem hiding this comment.
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