Assessment reports>UPA circuits circuitId hash function change>Discussion>Improvements possible to make KeccakCircuit::new more robust

Improvements possible to make KeccakCircuit::new more robust

In circuits/src/keccak/mod.rs, the function KeccakCircuit::new contains the following code:

for input in inputs.0 {
    let assigned_input = AssignedKeccakInput::from_keccak_padded_input(
        ctx, &range, input,
    );
    let circuit_id = Self::compute_circuit_id(
        ctx,
        &range,
        &mut keccak,
        &assigned_input,
    );
    if is_fixed {
        Self::compute_proof_id_fixed_length(
            ctx,
            &range,
            &mut keccak,
            &assigned_input,
        );
        panic!("Keccak fixed length no longer supported");
    } else {
        // Specification: Proof ID Computation
        Self::compute_proof_id(
            ctx,
            &range,
            &mut keccak,
            &circuit_id,
            &assigned_input,
        );
    }
    // Specification: Curve-to-Field Hash
    Self::commitment_point_hash_query(
        ctx,
        &range,
        &mut keccak,
        &assigned_input,
    );
    public_inputs.push(assigned_input);
}

// Specification: Final Digest Computation.
// Here we select only the even var_len_queries because those
// contain the proofIds. The odd ones contain the circuitIds
// which are not hashes into the final digest.
let intermediate_outputs = keccak
    .var_len_queries()
    .iter()
    .skip(1)
    .step_by(2) // we skip the circuitId computations
    .flat_map(|query| query.output_bytes_assigned().to_vec())
    .collect::<Vec<_>>();

The logic to extract the proofIds towards the end of this snippet, involving skipping the first item and then skipping every second item, relies on the following:

  • The function compute_circuit_id creates exactly one var len query each.

  • We are in the variable-length case (as the fixed-length case panics), so compute_proof_id is used instead of compute_proof_id_fixed_length.

  • The function compute_proof_id creates exactly one var len query, and the result of that query is the proofId we would like to extract.

  • The function commitment_point_hash_query does not create any var len query (only a fixed len one).

While the outlined logic is correct, it would be more robust to handle the proofIds the same way as the circuitIds. In the case of KeccakMultiVarHasher::finalize, the result of the query is extracted and returned immediately,

keccak_chip.keccak_var_len(ctx, range, preimage, len);
return keccak_chip
    .var_len_queries()
    .last()
    .expect("no queries")
    .output_bytes_assigned()
    .to_vec();

and the functions KeccakChip::multi_var_query and KeccakCircuit::compute_circuit_id forward this return value.

One could do something analogous in compute_proof_id (that is, extract the result immediately after creating the query and returning it), and then in KeccakCircuit::new, one could push the return value to intermediate_outputs instead of needing to rely on the precise query order to obtain it at the end. This would make the code less likely to break and require changes should some of the called functions be changed to create more or less var len queries. It would also make it easier to see that the code is correct, not requiring to check the above four bullet points.

Zellic © 2025Back to top ↑