Also regarding DataType::G1Data, wrapping ark_bn254::G1Affine
This type is used by some functions (e.g. chunk_precompute_p) to hold the precomputed values -x/y,y, which is a pair of Fq-Elements, but not coordinates of a valid point on G1 in the coordinates G1Affine expects. This is prone to coding errors, as the correctness of functions taking G1Affine-values as arguments might depend on the argument being valid.
These functions use G1Affine::new_unchecked to be able to create these invalid values. This function should only be used the coordinates are known to represent a G1Affine-Point to skip redundant checks, or when the validity is checked in the same scope.
As in the current code, the assumption that all G1Affine-elements returned are valid does not hold, it must be carefully checked that functions that assume their argument to be a valid G1 point (e.g.chunk_hash_p) are always called with valid elements
It would be safer to have a separate type holding the (-x/y,y)pairs. With that change, the type checking of rust would validate that the two separate types for Fq-pairs are only used in their intended way.
General issue:
Throughout the codebase, the types ark_bn254::G1Affine and ark_bn254::G2Affine are used to store coordinate of points. These types are intended for points in certain subgroups of certain curves.
However, these types are used in various invalid ways:
- Often unsafe constructors like new_unchecked are used to construct points. This skips the check that the coordinates are valid non-zero points on the curve in the correct subgroup, and is used also in cases where this is not the case. In particular with coordinates (0,0), which is intended to be the point at infinity, but G1Affine::new_unchecked(0,0) will not be treated as the point at infinity by the ark code (e.g. arithmetic functions).
- Sometimes arithmetic functions etc. are called on G1Affine or G2Affine points that might store invalid coordinates that might not be points in the correct subgroup. Intended results are not guaranteed in this case.
- Going back from the ark types to coordinates, sometimes it is not adequately checked whether the point is infinity.
- In some places the projective coordinate transformation (x,y) -> (-x/y,1/y) is applied to coordiantes, and the coordinates (-x/y,1/y) stored in the G1Affine type even though these coordinates are generally not on the curve.
This can have various problems. To illustrate this, here is one particular example and its impact:
#[test]
fn zellic_test_public_input_zero() {
println!("Preparing Input");
let public_input_int : u64 = 0;
let public_input : ark_bn254::Fr = ark_bn254::Fr::from(public_input_int);
let vk: ark_groth16::VerifyingKey<Bn254> = ark_groth16::VerifyingKey {
alpha_g1: G1Affine::generator(),
beta_g2: G2Affine::generator(),
gamma_g2: G2Affine::generator(),
delta_g2: G2Affine::generator(),
gamma_abc_g1: vec![G1Affine::generator(), G1Affine::generator()],
};
let proof: ark_groth16::Proof<Bn254> = ark_groth16::Proof {
a: G1Affine::generator().mul_bigint([1*1 + 1*1 + public_input_int*1 + 1*1]).into_affine(),
b: G2Affine::generator(),
c: G1Affine::generator(),
};
let scalars = [public_input];
println!("public input: {:?}", public_input);
// generate segments
println!("get_segments_from_groth16_proof");
let (success, segments) = get_segments_from_groth16_proof(proof, scalars.to_vec(), &vk);
println!("Finished generating segments, success={}", success);
assert!(success);
// segments to assertion
println!("get_assertion_from_segments");
let assts = get_assertion_from_segments(&segments);
println!("execute_script_from_assertion");
let res = execute_script_from_assertion(&segments, assts);
println!("Result is none: {}", res.is_none());
if res.is_some() {
println!("Result id: {}", res.unwrap().0);
}
}
Also regarding DataType::G1Data, wrapping ark_bn254::G1Affine
This type is used by some functions (e.g. chunk_precompute_p) to hold the precomputed values -x/y,y, which is a pair of Fq-Elements, but not coordinates of a valid point on G1 in the coordinates G1Affine expects. This is prone to coding errors, as the correctness of functions taking G1Affine-values as arguments might depend on the argument being valid.
These functions use G1Affine::new_unchecked to be able to create these invalid values. This function should only be used the coordinates are known to represent a G1Affine-Point to skip redundant checks, or when the validity is checked in the same scope.
As in the current code, the assumption that all G1Affine-elements returned are valid does not hold, it must be carefully checked that functions that assume their argument to be a valid G1 point (e.g.chunk_hash_p) are always called with valid elements
It would be safer to have a separate type holding the (-x/y,y)pairs. With that change, the type checking of rust would validate that the two separate types for Fq-pairs are only used in their intended way.
General issue:
Throughout the codebase, the types ark_bn254::G1Affine and ark_bn254::G2Affine are used to store coordinate of points. These types are intended for points in certain subgroups of certain curves.
However, these types are used in various invalid ways:
This can have various problems. To illustrate this, here is one particular example and its impact: