[WAR-535] Sylow review comments #26
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Cargo.toml
Outdated
| @@ -4,6 +4,7 @@ categories = ["cryptography", "mathematics"] | |||
| description = "Implementation of the BLS signature scheme using the alt-bn128 curve." | |||
| homepage = "https://github.com/warlock-labs/sylow" | |||
| keywords = ["alt-bn128", "bls", "cryptography", "elliptic-curve", "pairing"] | |||
| # BN254, any Ethereum/crypto-specific categories/keywords? | |||
There was a problem hiding this comment.
I'll modify "bls" to "zero-knowledge" for now since its closer to the current functionality, but there's a hard limit of 5 keywords :/
Cargo.toml
Outdated
| name = "key" | ||
| path = "examples/key.rs" | ||
| # do we need/want to add all examples here | ||
| # also, some examples demonstrating groups and fields |
There was a problem hiding this comment.
added the example for pairing explicitly as well
| @@ -5,6 +5,7 @@ | |||
| [](https://docs.rs/sylow/) | |||
|  | |||
|
|
|||
| <!-- Generally seems to be pronounced SEE-low at least in American English, and perhaps note that it's being named after Ludwig. --> | |||
There was a problem hiding this comment.
I just copied the ipa spelling from Wikipedia for this so I assume it reads SEE-low but I'm not sure! either way, ill let wikipedia be the decider
| }, | ||
| Err(e) => println!("Signing error: {:?}", e), | ||
| } | ||
| // Generate a new key pair |
There was a problem hiding this comment.
great catch rustfmt-ing this!
benches/field.rs
Outdated
| @@ -1,3 +1,5 @@ | |||
| // Would testing for operations running in const time, e.g. through sampling, | |||
| // be feasible as a part of benchmark or other testing? | |||
There was a problem hiding this comment.
yes it is, and they will have been added on main soon
src/fields/extensions.rs
Outdated
| @@ -27,6 +29,7 @@ impl<const D: usize, const N: usize, F: FieldExtensionTrait<D, N>> From<u64> | |||
| fn from(value: u64) -> Self { | |||
| let mut retval = [F::zero(); N]; | |||
| retval[0] = F::from(value); | |||
| // And we're sure F is always >= u64 | |||
There was a problem hiding this comment.
yes, since crypto_bigint won't let you instantiate a ConstMontyForm with a bit precision less than that
src/fields/extensions.rs
Outdated
| @@ -44,12 +47,14 @@ impl<const D: usize, const N: usize, F: FieldExtensionTrait<D, N>> FieldExtensio | |||
| /// <https://eprint.iacr.org/2010/354.pdf> | |||
| /// # Arguments | |||
| /// * `factor` - a field element that is used to scale the extension element | |||
| // This has a clear meaning in context, as opposed to multiplication? | |||
There was a problem hiding this comment.
note added clarifying this
src/fields/extensions.rs
Outdated
| pub(crate) fn scale(&self, factor: F) -> Self { | ||
| let mut i = 0; | ||
| let mut retval = [F::zero(); N]; | ||
| while i < N { | ||
| retval[i] = self.0[i] * factor; | ||
| i += 1; | ||
| // There's no concern about carry/overflow here? |
There was a problem hiding this comment.
in what way? overflow of the accumulator i or of the multiplication? the multiplication will always be handled with the Montgomery modular arithmetic and therefore handles overflow over the modulus
src/fields/extensions.rs
Outdated
| @@ -156,6 +162,7 @@ impl<const D: usize, const N: usize, F: FieldExtensionTrait<D, N>> Neg for Field | |||
| while i < N { | |||
| retval[i] = -self.0[i]; | |||
| i += 1; | |||
| // This element-wise negation just works? | |||
There was a problem hiding this comment.
yep, since this just determines A' such that A + A' = \vec{0} element-wise, which is achieved with each element of the extension being A'_i = -A_i.
src/fields/extensions.rs
Outdated
| @@ -156,6 +162,7 @@ impl<const D: usize, const N: usize, F: FieldExtensionTrait<D, N>> Neg for Field | |||
| while i < N { | |||
| retval[i] = -self.0[i]; | |||
| i += 1; | |||
| // This element-wise negation just works? | |||
There was a problem hiding this comment.
yep, since this just determines A' such that A + A' = \vec{0} element-wise, which is achieved with each element of the extension being A'_i = -A_i.
No description provided.