Ripemd160 implementation

[Sh0g0-1758]

We intend to add the ZK implementation of the ripemd160 hash function to this library. Though some tests are failing, a rough structure for the same is ready.

[avras]

I have included comments in some places. Significant reworking required.

[Sh0g0-1758]

@avras, thanks a lot for the constructive feedback. I have modified the code accordingly. I will add exhaustive tests tomorrow and then mark the PR as ready for review.

[Sh0g0-1758]

@avras, we have added 3 tests that are passing. Please review the code once.

[Sh0g0-1758]

gentle ping.

[Sh0g0-1758]

@avras, thanks a lot for the code cleanup and logic correction for the ripemd_d1 and ripemd_d2 functions.

Are any further changes needed from my end?

[avras]

Are any further changes needed from my end?

@Sh0g0-1758 I forgot to check for warnings from cargo xclippy -D warnings. A post-commit check is failing. Please pull this commit into your branch.
EDIT: Please pull this this subsequent commit instead which removes some unwraps.

[Sh0g0-1758]

done @avras.

[avras]

@huitseeker I have reviewed this PR and contributed changes. The other contributors Shourya and Himanshu are students who are working (remotely) under my guidance. Some fresh eyes could find missed issues. Please assign appropriate reviewers.

The first 3 pages of this doc have a succinct spec of RIPEMD160. The canonical reference is here.

[huitseeker]

The use of into_bits will force a clone, so you're better off asking for a, b, c by value here and let the caller manage the cloning.
See also C-CALLER-CONTROL

[avras]

Thanks for the suggestion. I have made this change.

[huitseeker]

Nit: this avoids the array index arithmetic

    bits.chunks(8) // Process 8 bits (1 byte) at a time
        .flat_map(|byte| {
            byte.iter()
                .rev()
                .cloned()
        })
        .collect()

[avras]

Thanks. This is much nicer.

[huitseeker]

Does this need to be pub ?

[avras]

Changed it to pub(crate)

[huitseeker]

Does this need to be pub ?

[avras]

Changed it to pub(crate)

[huitseeker]

Does this need to be pub ?

[avras]

Changed it to pub(crate)

[huitseeker]

f3, f4, f5 should be #[inline]

[huitseeker]

This should probably be #[inline].

[huitseeker]

Not super important, but you can probably compute how the size of the final vector in advance and allocate that size earlier, which avoids stressing the allocator with adding elements one-by-one:

let zero_bytes = 512 - (padded.len() + 64) % 512;
padded.extend(iter::repeat(Boolean::Constant(false)).take(zero_bytes));

[avras]

Thanks for the suggestion. The while loop idea is from the original sha256 gadet. I have made a slight change to your suggestion. If the message after appending the single 1 bit and the 64-bit message length has a length which is a multiple of 512, we don't need to append zero bits.

    let num_zero_bits = 512 - (padded.len() + 64) % 512;
    // If num_zero_bits == 512, then (padded.len() + 64) is already a multiple of 512
    // No zero bits need to be appended in this case
    if num_zero_bits != 512 {
        padded.extend(iter::repeat(Boolean::Constant(false)).take(num_zero_bits));
    }

[huitseeker]

This assumes the endianness of the input argument, therefore I would expect a doc comment on this function about that expectation.

[avras]

There is no assumption about the endianness of the input in RIPEMD160. The swap_byte_endianness function was to work around the way RIPEMD160 converts a 32-bit chunk into a integer (big-endian bytes with least significant byte first). I have removed this function to avoid confusion and did the endianness adjustment inline. I also added the below comments at the appropriate places

    // Given a chunk of 32 bits b0, b1,..., b31, RIPEMD160 converts them into
    // a 32-bit integer as follows:
    // - Bits b0 to b7 represent the least significant byte in big-endian order
    // - Bits b8 to b15 represent the next most significant byte in big-endian order
    // - Bits b16 to b23 represent the next most significant byte in big-endian order
    // - Bits b24 to b31 represent the most significant byte in big-endian order
    //
    // But the UInt32::from_bits function expects the bit sequence corresponding to
    // a 32-bit integer to be in little-endian order. So we need to feed it the sequence
    // b7, b6, ..., b0, b15, b14, ..., b8, b23, b22, ..., b16, b31, b30, ..., b24.
    // The below code performs this conversion
    let msg_words = msg_block
        .chunks(8)
        .flat_map(|byte| byte.iter().rev().cloned())
        .collect::<Vec<_>>()
        .chunks(32)
        .map(UInt32::from_bits)
        .collect::<Vec<_>>();

    // The UInt32::into_bits function outputs the bit sequence corresponding to
    // a 32-bit integer in little-endian order b0, b1, ..., b31
    // But RIPEMD160 outputs each 8 bit chunk in big-endian order.
    // So we need to output the sequence
    // b7, b6, ..., b0, b15, b14, ..., b8, b23, b22, ..., b16, b31, b30, ..., b24.
    // The below code performs this conversion
    let result = msg_digest
        .into_iter()
        .flat_map(|e| e.into_bits())
        .collect::<Vec<_>>()
        .chunks(8)
        .flat_map(|byte| byte.iter().rev().cloned())
        .collect::<Vec<_>>();

[huitseeker]

It would be great to see a randomized test here using an out-of-circuit implementation of ripemd160, and checking your gadget returns the same. You have the scaffolding for randomized tests already, and you can use the https://github.com/RustCrypto/hashes/tree/master/ripemd crate in rust to compute a reference value.

See e.g. https://github.com/lurk-lab/bellpepper-gadgets/blob/main/crates/keccak/tests/gadget.rs#L51-L83 for this sort of test (using different tooling for the RNG portion, i.e. proptest - though what you have here is fine, the important part is comparison to a reference implementation).

[avras]

I have added a randomized test based on the one in the sha256 gadget

[huitseeker]

Thanks for the PR! It's a good contribution, and welcome. The top-level comment is we need a test that compares the values of the gadget with a reference implementation of the ripemd hash. The rest of the comments, inline, is less important.

[avras]

I have made the changes suggested by @huitseeker. Please review.

[huitseeker]

Thanks a lot for this work @Sh0g0-1758 and @avras, this is excellent and thorough!