fix: sponge pad panics on input

[vlopes11]

closes #44

[grjte]

Thanks @vlopes11! I left a couple of nits and a question inline

[grjte]

Is this also just testing that it won't panic? Can you add a comment that explains what this test is for?

[vlopes11]

Indeed, the number 111 is quite "magic" here. Comment added; I also changed the number to a prime so we have the guarantee it won't be divisible.

[grjte]

Why can't we use RATE_RANGE.start + i..RATE_RANGE.end here?

[vlopes11]

True, fixed!

[grjte]

I'm confused by this and the below comment. I thought the first element of the capacity range is supposed to be either 1 or 0, not the number of elements to be hashed anymore. @Al-Kindi-0?

[vlopes11]

We have indeed a couple of unanswered questions there. I was thinking of moving the PR to draft until we have these answered.

We indeed set the first element as the count

crypto/src/hash/rpo/mod.rs

Lines 109 to 110 in 6de7730

	let mut state = [ZERO; STATE_WIDTH];
	state[CAPACITY_RANGE.start] = Felt::new(num_elements as u64);

But during the first iteration, we override it with the input, as i is set as 0

crypto/src/hash/rpo/mod.rs

Lines 116 to 136 in 6de7730

	let mut i = 0;
	let mut buf = [0_u8; 8];
	for chunk in bytes.chunks(BINARY_CHUNK_SIZE) {
	if i < num_elements - 1 {
	buf[..BINARY_CHUNK_SIZE].copy_from_slice(chunk);
	} else {
	// if we are dealing with the last chunk, it may be smaller than BINARY_CHUNK_SIZE
	// bytes long, so we need to handle it slightly differently. We also append a byte
	// with value 1 to the end of the string; this pads the string in such a way that
	// adding trailing zeros results in different hash
	let chunk_len = chunk.len();
	buf = [0_u8; 8];
	buf[..chunk_len].copy_from_slice(chunk);
	buf[chunk_len] = 1;
	}
	// convert the bytes into a field element and absorb it into the rate portion of the
	// state; if the rate is filled up, apply the Rescue permutation and start absorbing
	// again from zero index.
	state[RATE_RANGE.start + i] = Felt::new(u64::from_le_bytes(buf));
	i += 1;

There is a correct statement below that we don't need to add an extra element with [1, 0, 0, 0, 0 ,0, 0] because the number of elements in the initial inputs indeed will differentiate cases with or without remainder chunks.

crypto/src/hash/rpo/mod.rs

Lines 143 to 149 in 6de7730

	// if we absorbed some elements but didn't apply a permutation to them (would happen when
	// the number of elements is not a multiple of RATE_WIDTH), apply the RPO permutation.
	// we don't need to apply any extra padding because we injected total number of elements
	// in the input list into the capacity portion of the state during initialization.
	if i > 0 {
	Self::apply_permutation(&mut state);
	}

But, if we change it to be either 0 or 1, then we have to apply an extra padding element for such cases.

[vlopes11]

Another question:

If we apply a montgomery reduction here (as opposed to Felt::from_mont that will just take the raw number)

crypto/src/hash/rpo/mod.rs

Lines 132 to 135 in 6de7730

	// convert the bytes into a field element and absorb it into the rate portion of the
	// state; if the rate is filled up, apply the Rescue permutation and start absorbing
	// again from zero index.
	state[RATE_RANGE.start + i] = Felt::new(u64::from_le_bytes(buf));

Why do we limit the chunk size to 7 bytes, increasing the permutation count?

crypto/src/hash/rpo/mod.rs

Lines 40 to 41 in 6de7730

	/// The number of byte chunks defining a field element when hashing a sequence of bytes
	const BINARY_CHUNK_SIZE: usize = 7;

I think we should either apply the mont reduce + take 8 bytes, or take 7 and use raw elements, saving a bit of overhead.

[Al-Kindi-0]

Thank you @vlopes11 and @grjte for raising this issue.
Indeed, for hash(&[u8]) the padding used is not the same as in the RPO paper but was left as is from the previous implementation. I think this a good time to revisit this.
@vlopes11: Regarding your first point, I think that during the first iteration, it is state[RATE_RANGE.start + 0] that is written to as opposed to state[CAPACITY_RANGE.start]. Regarding your second point, I think you are right, we should be able to apply mont_reduce(u64) to 8 byte-chunks or from_mont(u64) to 7 byte-chunks.

[vlopes11]

@Al-Kindi-0 thanks for the clarification. As for the second item, I think its best to go for 8 bytes with montgomery redcution as the reduction itself should be far cheaper than a permutation - and safer. wdyt?

Regarding the padding, can you link the latest paper we should use as reference? I'll reproduce in the code the definition we have there.

[Al-Kindi-0]

@Al-Kindi-0 thanks for the clarification. As for the second item, I think its best to go for 8 bytes with montgomery redcution as the reduction itself should be far cheaper than a permutation - and safer. wdyt?

I am leaning also towards this option but maybe we should benchmark the two approaches to get a better idea.

Regarding the padding, can you link the latest paper we should use as reference? I'll reproduce in the code the definition we have there.

https://eprint.iacr.org/2022/1577.pdf

[vlopes11]

@Al-Kindi-0 it's clear what needs to be done. As pointed by @grjte , capacity will be either one or zero.

The last question I have is about one of the rules in the paper: 2.5 The zero-length input is not allowed. Why is that? It's not immediately obvious because if we just permutate zeroes, we will also have a valid output (I think). And, considering sponge hashes are usually infallible, I guess we have to come to an output for this case.

[Al-Kindi-0]

I think this is was mainly done in order to simplify (thinking about) the new padding rule. Hashing of only zeros is still allowed but the length then is not zero.

[bobbinth]

A couple of comments on this:

1. We should use the same padding rule as for hash_elements() here. Specifically, if the number of elements to be hashed is a multiple of 8, we set capacity registers to ZERO's. If it is not a multiple of 8, then we set the first capacity register to ONE and the rest to ZERO's, and also append ONE to the input element string.
2. For binary string, we should keep splitting the input into 7-byte chunks. Splitting it into 8-byte chunks would cause collisions as 8-byte chunks don't map one-to-one to Field elements.
3. We may need to apply additional padding to the last chunk. Specifically, we need to append 1 to the string before converting the last chunk into a field element. I think this is already being done, but I'd probably add a test to make sure something like [0; 9] and [0; 10] hash to different values.

[bobbinth]

Looks good! Thank you! I left a couple small comments inline.

[bobbinth]

All looks good! Thank you!

[grjte]

Looks good, thank you!