Confusing behavior of native compute_challenge_points

In circuits/src/batch_verify/universal/native.rs, the function compute_challenge_points takes a UniversalBatchVerifierInput as input.

The UniversalBatchVerifierInput type is usually used for unpadded inputs where the public inputs do not contain the hash of the commitment yet. Typically, the function BatchEntry::from_ubv_input_and_config in circuits/src/batch_verify/universal/types.rs is used to convert from UniversalBatchVerifierInput to BatchEntry, handling computation of the hash of the possible commitment and appending it to the public inputs, as well as padding.

The native function compute_challenge_points is intended to compute the challenge points, which are obtained as a hash that includes the public input in its padded form (with possibly the hash of the commitment). It would be consistent with other usage if compute_challenge_points were to use BatchEntry::from_ubv_input_and_config to handle the commitment hash and padding and then use the inputs field from the resulting BatchEntry for the hash.

Instead, the function's behavior differs from usage in tests and normally (via #[cfg(test)]), and it functions the following way:

1. During tests, it appends padding but does not append the hash of the commitment, even if a commitment is present.

2. Outside of tests, it neither appends the hash of a commitment nor adds padding.

Even though UniversalBatchVerifierInput is elsewhere (outside of circuits/src/batch_verify/universal/native.rs) used for unpadded inputs without the commitment hash, this behavior requires the caller to provide a UniversalBatchVerifierInput for which inputs, contrary to normal usage,

1. already has the commitment hash appended when in a test.

2. outside of tests, already has the commitment hash appended and is padded.

This is confusing when reading the code.

In the current codebase, it appears that compute_challenge_points is only called by component, which is a test, and get_pairs. The latter function is in turn only called by component again and by verify_universal_groth16_batch, and verify_universal_groth16_batch is only called by test_universal_verifier_same_vk, test_universal_verifier_distinct_vks, and test_universal_verifier_general_case, all of which are tests.

Thus, compute_challenge_points is only used in tests in practice, so part of the confusing duplicate behavior could be reduced by only handling the test case.

In the test case, it is still confusing why the function expects a type that usually does not contain the hash of the commitment in the public inputs to nevertheless contain it.

The reason the code produces the correct challenge points is that in both component and get_pairs, the update_batch function is used to add the hash of the commitment, if present, to the public inputs.

We recommend to convert the original UniversalBatchVerifierInput to a BatchEntry with BatchEntry::from_ubv_input_and_config instead and then use that directly in compute_challenge_points without adding padding. Alternatively, if there are reasons to separate appending the hash of the commitment and the padding steps, consider creating a new type for this intermediate data, and use it as input for compute_challenge_points. If no code changes along those lines are made, we recommend documenting the behavior and the reason why this works in comments for compute_challenge_points and the other relevant places.