optimize FixedLengthSum of hash with minLen

[ggq89]

Description

Use minLen to reduce the times of selects and optimize the FixedLengthSum of hash.

In practice, the length of a piece of data to be hashed is variable, but its minimum value is rarely 0. In this case, if minLen is passed to FixedLengthSum, a lot of constraints can be saved. For example, in the project we are currently working on, one of the data lengths to be hashed ranges from 1680 to 1710. Using the new FixedLengthSum can reduce about 200,000 constraints, as shown in the results of Test_SHA3FixedLengthSum_WithMinLen_VS_Zero:

=== RUN   Test_SHA3FixedLengthSum_WithMinLen_VS_Zero
maxLen=1710, minLen=1680, hash=Keccak-512, nbConstraints: 6696856 vs 6555681(withMinLen)
maxLen=1710, minLen=1680, hash=SHA3-256, nbConstraints: 4040628 vs 3796970(withMinLen)
maxLen=1710, minLen=1680, hash=SHA3-384, nbConstraints: 4992172 vs 4800206(withMinLen)
maxLen=1710, minLen=1680, hash=SHA3-512, nbConstraints: 6696856 vs 6555681(withMinLen)
maxLen=1710, minLen=1680, hash=Keccak-256, nbConstraints: 4040628 vs 3796970(withMinLen)
--- PASS: Test_SHA3FixedLengthSum_WithMinLen_VS_Zero (47.33s)

Type of change

• New feature (non-breaking change which adds functionality)

How has this been tested?

• TestSHA2FixedLengthSum
• TestSHA2FixedLengthSumWithMinLen
• TestSHA3FixedLengthSum
• TestSHA3FixedLengthSumWithMinLen
• Test_SHA3FixedLengthSum_WithMinLen_VS_Zero

How has this been benchmarked?

• Benchmark A, on Macbook pro M1, 32GB RAM
• Benchmark B, on x86 Intel xxx, 16GB RAM

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I did not modify files generated from templates
• golangci-lint does not output errors locally
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

Pull Request Overview

This pull request optimizes the FixedLengthSum methods for SHA2 and SHA3 by adding a new minLen parameter to reduce unnecessary constraints during the hashing process. Key changes include:

• Updating the FixedLengthSum function signatures and internal logic in both SHA2 and SHA3 implementations to incorporate minLen.
• Adjusting and adding new test cases (including TestSHA3FixedLengthSumWithMinLen and updates in SHA2 tests) to validate the new functionality.
• Updating the hash interface documentation to reflect the new parameter.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
std/hash/sha3/sha3_test.go	Updated test cases to call FixedLengthSum with appropriate minLen values.
std/hash/sha2/sha2_test.go	Updated tests to loop over variable lengths using defined minLen and maxLen.
std/hash/sha2/sha2.go	Revised padding and summing logic in FixedLengthSum to use minLen.
std/hash/sha3/sha3.go	Modified FixedLengthSum, paddingFixedWidth, and absorbingFixedWidth to incorporate minLen.
std/hash/hash.go	Updated the BinaryFixedLengthHasher interface documentation.

Comments suppressed due to low confidence (3)

std/hash/sha3/sha3_test.go:115

• [nitpick] Using a literal '0' for the minLen parameter may be unclear; consider defining a descriptive constant or adding a comment to clarify that this case is intended as a baseline comparison.

res := h.FixedLengthSum(0, c.Length)

std/hash/sha2/sha2.go:115

• Review the loop boundary here: using 'i <= maxLen' assumes that the appended padding data aligns correctly with the original input length. Verify that this condition correctly accounts for the extended slice length and does not lead to off-by-one errors.

for i := minLen; i <= maxLen; i++ {

std/hash/sha3/sha3.go:82

• Ensure that the modified boundary condition in the paddingFixedWidth function correctly handles the range of indices after appending additional padding. A review of the off-by-one behavior here is advised.

for i := minLen; i <= maxLen; i++ {

[Copilot]

Pull Request Overview

This PR optimizes the FixedLengthSum implementations for SHA2 and SHA3 hashes by introducing a minimalLength parameter that allows skipping unnecessary constraints when the input is guaranteed to meet a lower bound.

• Introduces a new option WithMinimalLength in the hasher configuration for both SHA2 and SHA3.
• Updates padding, block composition, and absorbing loops in both hash implementations to take advantage of the minimal length.
• Adjusts test circuits and hash constructor helpers to pass and validate the minimalLength parameter.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
std/hash/sha2/sha2_test.go	Updated tests to incorporate minimalLength variations.
std/hash/sha2/sha2.go	Modified hash construction and FixedLengthSum to use minimalLength.
std/hash/hash.go	Added option WithMinimalLength and updated documentation.
std/hash/sha3/sha3.go	Integrated minimalLength in padding and absorbing functions.
std/hash/sha3/sha3_test.go	Adjusted tests to validate new minimalLength parameter.
std/hash/sha3/hashes.go	Updated SHA3 hash constructors to support minimalLength option.

Comments suppressed due to low confidence (1)

std/hash/sha3/sha3.go:133

• Consider verifying if integer division for minNbOfBlocks correctly handles cases where minimalLength is not a multiple of rate. If partial blocks should also trigger initialization, adjust the calculation accordingly.

minNbOfBlocks := d.minimalLength / d.rate

[ivokub]

Thanks for the contribution! I really like it and it indeed makes the implementation more efficient. I only change the interfaces to keep the current FixedBinaryHasher interface intact to ensure that anyone using it downstream wouldn't be broken. Instead, now the bound can be provided as an optional option when initializing the hash function. It is a bit more inconvenient to use when the circuits want to have different bounds, as we need to initialize the hasher for every different lower bound separately, but imo it is better to keep API backwards compatibility. I think sha2/sha3 is already used a lot.

Congrats btw for this change, it is super good catch!

@ggq89, @weijiguo have a look how it seems now. It is good to merge from my side but I'll accept review+merge when you confirm that my changes are good.

[ggq89]

That's good, it would be even better to move the MinimalLength option to the hash constructor

[weijiguo]

Looks good to me. And thank you for stepping in @ivokub