Confusing implementation of InCircuitHash
and FieldElementRepresentation
for AssignedVerificationKey
While AssignedVerificationKey
has been changed to include new fields h1
and h2
for verification of the proof of knowledge for the Pedersen commitment, the implementations of the InCircuitHash
, FieldElementRepresentation
, and InCircuitPartialHash
have not been updated. The way these traits are used in the Poseidon implementation and the universal batch verifier chip results in behavior consistent with the specification, as handling of hashing of h1
and h2
is done via the extra terms that var_len_poseidon_no_len_check_with_extra_terms
now takes.
However, these unchanged implementations of InCircuitHash
and FieldElementRepresentation
for AssignedVerificationKey
are locally unexpected; for example, FieldElementRepresentation
is documented as follows:
/// Interface for types that can be represented as a vector of field elements
pub trait FieldElementRepresentation<F: EccPrimeField> {
/// Returns the representation of `self` as assigned field elements.
fn representation(&self) -> Vec<AssignedValue<F>>;
/// Returns the total number of field elements in `self`.
fn num_elements(&self) -> usize;
}
The comments here would suggest that representation
returns a vector of field elements that completely determines self
, while in fact h1
and h2
are left out.
Given the narrow usage of AssignedVerificationKey
in the codebase, it appears unlikely that mistakes will arise from confusion about what these two traits ensure in the near future. For best practice, we would nevertheless recommend to document that not all parts of self
might be represented or hashed and/or possibly rename the trait.