Adding SHA1 gadget #7
Conversation
|
I will make changes to this gadget to use the in-built |
|
For development purposes, you can use a dependency override to test the De Morgan OR gadget now merged in the master branch of bellpepper, before the upcoming release. |
|
Bellpepper 0.3.0 is released with the or gadget! |
|
Thanks @huitseeker for the heads up. It feels good to contribute to bellpepper. I will get the SHA1 PR done soon. |
There was a problem hiding this comment.
Very nice work! The code is clear and makes good use of lower-level primitives which abstracts away the risks of writing circuits. All allocations use proper constructors, and variables are always derived from previous inputs.
I left some comments suggesting the use of arrays instead of Vec for lists whose size are known at compile-time, and also suggestions for comments which could clarify the rounds. Let me know what you think.
crates/sha1/src/sha1.rs
Outdated
| fn get_sha1_iv() -> Vec<UInt32> { | ||
| IV.iter().map(|&v| UInt32::constant(v)).collect() | ||
| } |
There was a problem hiding this comment.
The length of the result has a size known at compile time. Have you considered returning an array instead?
There was a problem hiding this comment.
Thanks for pointing this out. I will try changing the output to an array
crates/sha1/src/sha1.rs
Outdated
| .collect()) | ||
| } | ||
|
|
||
| pub fn sha1<Scalar, CS>(mut cs: CS, input: &[Boolean]) -> Result<Vec<Boolean>, SynthesisError> |
There was a problem hiding this comment.
The function could return an array instead of a Vec since the size is known at compile-time.
There was a problem hiding this comment.
I will try changing the output type here as well
crates/sha1/src/sha1.rs
Outdated
| let a1 = and_uint32(cs.namespace(|| "1st and"), &b, &c)?; | ||
| let a2 = and_uint32(cs.namespace(|| "2nd and"), &b, &d)?; | ||
| let a3 = and_uint32(cs.namespace(|| "3rd and"), &c, &d)?; | ||
|
|
||
| let tmp = or_uint32(cs.namespace(|| "1st or"), &a1, &a2)?; | ||
| or_uint32(cs.namespace(|| "2nd or"), &tmp, &a3)? |
There was a problem hiding this comment.
The namespaces could describe the operations being performed, and a comment at the top of this block could state what the round computes.
There was a problem hiding this comment.
I will revisit the SHA1 spec and give some descriptive labels to the namespaces. You can review and let me know if those are fine
crates/sha1/src/sha1.rs
Outdated
| let f = if i < 20 { | ||
| // f = (b and c) or ((not b) and d) | ||
| UInt32::sha256_ch(cs.namespace(|| "ch"), &b, &c, &d)? | ||
| } else if !(40..60).contains(&i) { |
There was a problem hiding this comment.
A comment stating that this branch is for rounds 20 <= t <= 39 and 60 <= t <= 79 would make it easier to match the spec.
There was a problem hiding this comment.
I will add this comment
|
Thanks for replying to the comments, happy to review the updates! |
|
Hey @adr1anh, please check if I have addressed all your suggestions. I renamed some variables to better match the spec. No nitpick is too small. Send them all. |
A second attempt at the SHA1 gadget using a simpler OR gate. See previous attempt at #6