Skip to content

Point at infinity not handled in field_conversion::convert_to_bn254_frs() #1527

@ledwards2225

Description

@ledwards2225

field_conversion::convert_from_bn254_frs() has special handling for the point at infinity but its counterpart field_conversion::convert_to_bn254_frs() does not. This is likely because this method does not historically get used for points at infinity so it simply hasn't come up. I ran into this when making a change that led to certain VK commitments being the point at infinity (changes that I ultimately didn't merge). It arises there because we serialize the VK commitments to buffer in order to compute an in-circuit VK hash.

It's not clear that this poses any issue for soundness but it's worth thinking through when we audit this code.

The existing code at the time of writing:

    } else if constexpr (IsAnyOf<T, bn254_element<Builder>>) {
        using BaseField = bn254_element<Builder>::BaseField;
        auto fr_vec_x = convert_to_bn254_frs<Builder, BaseField>(val.x);
        auto fr_vec_y = convert_to_bn254_frs<Builder, BaseField>(val.y);
        std::vector<fr<Builder>> fr_vec(fr_vec_x.begin(), fr_vec_x.end());
        fr_vec.insert(fr_vec.end(), fr_vec_y.begin(), fr_vec_y.end());
        return fr_vec;
    }

A possible update to properly handle the point at infinity. (The $(0,0)$ representation was chosen to match the native serialization so the a natively computed VK hash matches the corresponding in-circuit hash):

    } else if constexpr (IsAnyOf<T, bn254_element<Builder>>) {
        using BaseField = bn254_element<Builder>::BaseField;

        Builder* ctx = val.get_context();
        fr<Builder> zero_limb = fr<Builder>::from_witness_index(ctx, ctx->zero_idx);
        BaseField zero_coordinate(zero_limb, zero_limb);

        // For the point at infinity both coordinates are serialized as zero.
        BaseField x_to_serialize = BaseField::conditional_assign(val.is_point_at_infinity(), zero_coordinate, val.x);
        BaseField y_to_serialize = BaseField::conditional_assign(val.is_point_at_infinity(), zero_coordinate, val.y);

        auto fr_vec_x = convert_to_bn254_frs<Builder, BaseField>(x_to_serialize);
        auto fr_vec_y = convert_to_bn254_frs<Builder, BaseField>(y_to_serialize);
        std::vector<fr<Builder>> fr_vec(fr_vec_x.begin(), fr_vec_x.end());
        fr_vec.insert(fr_vec.end(), fr_vec_y.begin(), fr_vec_y.end());
        return fr_vec;
    } else if constexpr (IsAnyOf<T, grumpkin_element<Builder>>) {

Note: The proposed update sometimes introduces a new "witness appearing in only one gate". This is due to variable field_ct prime_limb in bigfield::conditional_select() which may not be used if the result of the conditional assign is only used in serialization (say to a VK hashing buffer). This is fine and can just be marked as an exception along the lines of other such witnesses.

Note 2: A possible alternative would be to handle this in biggroup directly. Currently the point at infinity representation in biggroup is (in some places) a generator point. If we used a consistent representation there we may not need any special handling in field_conversion

Metadata

Metadata

Assignees

Labels

auditThings to do during the next auditnice-to-have

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions