feat: remove redundant additions #6
Conversation
reduced benchmarks to only test public methods improved performance by ~5-10%
src/lib.nr
Outdated
| @@ -89,9 +273,10 @@ pub mod sha512 { | |||
|
|
|||
| } | |||
|
|
|||
| // methods here are interface-compatible with stdlib | |||
There was a problem hiding this comment.
sha256 and sha512 have been removed from the stdlib so we don't need to maintain compatibility btw.
There was a problem hiding this comment.
what about the sha256 blackbox interface? that's the one I want to maintain compatibility with tbh
There was a problem hiding this comment.
sha256 blackbox interface
To be clear, do you mean std::hash::sha256_compression here? That's the only blackbox function related to sha256 and the only sha256 logic in the stdlib.
I doubt this is the case based on the code this comment is on.
There was a problem hiding this comment.
Ah, I see you have a similar comment on the implementation of the compression function here. I've commented on a different part of the code.
There was a problem hiding this comment.
I think this is just about the black-box compression function
TomAFrench
changed the title
|
@kashbrti most of my comments have been addressed (just the thing about stdlib compatibility unresolved) so it's ready for a review from you. |
| @@ -0,0 +1,248 @@ | |||
| use crate::tables::{ | |||
There was a problem hiding this comment.
this file is just replacing the table computation code in the tables.nr file I guess
|
@zac-williamson the diff for this PR is quite massive. there's lots of refactoring happening, there are added comments and optimizations. I believe we should test this implementation against a rust SHA-512 lib using noir-runner (I can submit a PR for this later). Thanks for the extensive comments and the refactoring :) |
|
@kashbrti you could split out some of the less critical changes out into separate PR(s). Pulling out the table computation + benchmarks would remove a good amount of the diff. |
| let temp1 = state.h.raw + S1 + choose_output + (SHA512_ROUND_CONSTANTS[i] as Field) + w[i]; | ||
| // temp1 is a linar sum of ~50 inputs. Without std::as_witness the addition gates required to eval are duplicated | ||
| // for both `raw_e` and `raw_a` | ||
| std::as_witness(temp1); // temp1 = 106 gates |
| // 34 30 | ||
| // 39 25 | ||
| EncodedChoose { raw: e, e: e_ror0, ror14, ror18, ror41 } | ||
| let mut e_ror0: Field = base4_encoded_slices[0]; |
There was a problem hiding this comment.
see encode_message_extension comment. same reasoning
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
| EncodedWitness { raw: w, w: ror0, ror1, ror8, ror19, ror61, rshift6, rshift7 } | ||
| // W2_POWER_SUM represents scalars that each base4_encoded_slices member must be multiplied by, in order to produce | ||
| // `encode(raw.rotate19) + encode(raw.rotate61) + encode(raw >> 6)` | ||
| let W2_POWER_SUM: [Field; 12] = [ |
|
|
||
| // W15_POWER_SUM represents scalars that each base4_encoded_slices member must be multiplied by, in order to produce | ||
| // `encode(raw.rotate1) + encode(raw.rotate8) + encode(raw >> 7)` | ||
| let W15_POWER_SUM: [Field; 12] = [ |
src/lib.nr
Outdated
| @@ -89,9 +273,10 @@ pub mod sha512 { | |||
|
|
|||
| } | |||
|
|
|||
| // methods here are interface-compatible with stdlib | |||
There was a problem hiding this comment.
I think this is just about the black-box compression function
reduced benchmarks to only test public methods
improved performance by ~5-10%
Description
Old version of lib did not have much documentation about the algorithm - this PR adds extensive comments.
In addition, some redundant addition gates were removed from the SHA512 permutation which reduces the cost of the permutation by ~10%
PR Checklist*
cargo fmton default settings.