Inaccurate comments
In some reviewed places, we noticed inaccurate comments. We collect them here.
Function KeccakPaddedCircuitInput::to_instance_values
In circuits/src/keccak/mod.rs, the function KeccakPaddedCircuitInput::to_instance_values
has the following documentation comment:
/// For fixed-len: Appends the vk hash to the application public inputs.
/// For variable-len: Appends the length and the vk hash to the application
/// public inputs.
pub fn to_instance_values(&self) -> Vec<F> {
/*
let mut result = if self.is_fixed {
vec![self.app_vk_hash]
} else {
vec![self.len, self.app_vk_hash]
};
*/
assert!(self.is_var(), "Fixed length keccak no longer supported");
let mut result = vec![self.len];
result.extend_from_slice(&self.app_vk.flatten());
result.push(self.has_commitment);
result.push(self.commitment_hash);
result.extend_from_slice(&self.commitment_point_limbs);
result.extend_from_slice(&self.app_public_inputs);
result
}
This comment is inaccurate in the following ways:
In sentences of the form "Appends X to Y", X and Y are switched. We also pointed this out in section 4.1 of the audit from May 20th, 2024.
There is a description for fixed-length case, but that case is actually disabled.
The variable-length description is not updated to use the verification key rather than its hash, as well as the inclusion of the commitments.
Function AssignedKeccakInput::from_keccak_padded_input
Also in circuits/src/keccak/mod.rs, the function AssignedKeccakInput::from_keccak_padded_input
includes the following snippet:
// Constrain `len + has_commitment < MAX_LEN`
let len_inputs_and_commitment =
range.gate.add(ctx, len, has_commitment);
range.check_less_than_safe(ctx, len_inputs_and_commitment, max_len + 1);
The check is for len + has_commitment <= max_len
, which is the correct check — len
is the number of public inputs without the hashed commitment, the number of hashed commitments is added to that, and the sum should be at most the total number of padded public inputs. The comment uses <
instead of <=
, however.