chore: `field_conversion` clean-up/int audit by iakovenkos · Pull Request #16898 · AztecProtocol/aztec-packages

iakovenkos · 2025-09-09T13:17:29Z

🧾 Audit Context

stdlib::field_conversion provides the serialization/deserialization methods for stdlib::Transcript.

🛠️ Changes Made

Simplified the logic by making it more uniform and getting rid of redundant arguments
Enabled on_curve checks for Ultra deserialization of group elements (BN254 and Grumpkin)
Removed redundant range constraints in fr --> bigfield conversion and optimized the constraint ensuring that fr = lo + hi * shift. Note that limbs were constrained to be (136, 118) bits, although such range constraints are enforced by bigfield(low, hi) constructor.
Optimized point_at_infinity check for Grumpkin points.
Expanded docs and made them Doxygen-friendly
Added tests and an issue that will be resolved in a follow-up
The vks and circuit sizes have changed due to enabled checks and the small optimizations described above

📌 Notes for Reviewers

Agreed with @suyash67 that existing point-at-infinity issue AztecProtocol/barretenberg#1527 should be handled by ensuring the consistent representation of points at infinity in all groups. Added a test that will fail once the issue is resolved.

…i/field-conversion-audit

iakovenkos · 2025-09-09T16:54:51Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp

 * @tparam Builder
 */
-template <typename Builder> void cycle_group<Builder>::validate_is_on_curve() const
+template <typename Builder> void cycle_group<Builder>::validate_on_curve() const


to match the naming pattern in other groups

iakovenkos · 2025-09-09T16:56:36Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.hpp

 template <typename Builder> class cycle_group {
  public:
    using field_t = stdlib::field_t<Builder>;
+    using BaseField = field_t;


biggroup and goblin have this alias, seems useful for uniform group element handling in field_conversion

iakovenkos · 2025-09-09T16:57:32Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp

+        T out(x, y, check_point_at_infinity<Builder, T>(fr_vec));
+        // Note that in the case of bn254 with Mega arithmetization, the check is delegated to ECCVM, see
+        // `on_curve_check` in `ECCVMTranscriptRelationImpl`.
+        out.validate_on_curve();


Added in-circuit on_curve checks

iakovenkos · 2025-09-10T15:56:41Z

barretenberg/cpp/src/barretenberg/dsl/acir_format/civc_recursion_constraints.cpp


    // Recursively verify CIVC proof
-    auto mega_vk = std::make_shared<VerificationKey>(builder, key_fields);
+    auto mega_vk = std::make_shared<VerificationKey>(key_fields);


Here and in several places below, builder is redundant as field_conversion can extract from the fields being passed while ensuring that those are in-circuit.

iakovenkos · 2025-09-10T15:57:24Z

barretenberg/cpp/src/barretenberg/stdlib/honk_verifier/ultra_recursive_verifier.test.cpp

        // Check the size of the recursive verifier
        if constexpr (std::same_as<RecursiveFlavor, MegaZKRecursiveFlavor_<UltraCircuitBuilder>>) {
-            uint32_t NUM_GATES_EXPECTED = 796978;
+            uint32_t NUM_GATES_EXPECTED = 803143;


More gates due to enabled on-curve checks

iakovenkos · 2025-09-10T15:57:59Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

    element();
    element(const typename NativeGroup::affine_element& input);
    element(const Fq& x, const Fq& y);
+    element(const Fq& x, const Fq& y, const bool_ct& is_infinity);


feels like a very natural constructor, here and in other groups

iakovenkos · 2025-09-10T16:00:31Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.cpp

-    auto sum = low + hi * shift;
-    builder.assert_equal(f.witness_index, sum.witness_index, "assert_equal");
+    const fr<Builder> zero = fr<Builder>::from_witness_index(&builder, 0);
+    fr<Builder>::evaluate_linear_identity(hi * shift, low, -fr_element, zero);


minus 1 gate thanks to combining the addition with constraining to be 0

iakovenkos · 2025-09-10T16:01:18Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.cpp


-    std::vector<fr<Builder>> fr_vec{ low, hi };
-    return convert_from_bn254_frs<Builder, fq<Builder>>(builder, fr_vec);
+    return fq<Builder>(low, hi);


not calling convert_from_bn254_frs cause it's not needed/seems convoluted

iakovenkos · 2025-09-10T16:02:16Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp

+ *
+ * @return
+ */
+template <typename Builder, typename T> bool_t<Builder> check_point_at_infinity(std::span<const fr<Builder>> fr_vec)


Isolated the check to handle grumpkin and bn254 uniformly in deserialization

…i/field-conversion-audit

iakovenkos · 2025-09-16T12:57:56Z

barretenberg/cpp/src/barretenberg/stdlib/goblin_verifier/goblin_recursive_verifier.test.cpp

-                }
-            }
-        }
+        tamper_with_libra_eval(tampered_proof.translator_proof);


the same here, the tampering before would simply invalidate on_curve check. switched to modifying one of the evals

iakovenkos · 2025-09-16T12:58:26Z

barretenberg/cpp/src/barretenberg/stdlib/honk_verifier/ultra_recursive_verifier.test.cpp

        // Check the size of the recursive verifier
        if constexpr (std::same_as<RecursiveFlavor, MegaZKRecursiveFlavor_<UltraCircuitBuilder>>) {
-            uint32_t NUM_GATES_EXPECTED = 796978;
+            uint32_t NUM_GATES_EXPECTED = 801953;


as a result of enabled on_curve checks

…i/field-conversion-audit

suyash67 · 2025-09-16T23:50:41Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp

+        // Efficiently compute ((x^2 + 5 y^2) == 0)
+        const fr<Builder> x_sqr = fr_vec[0].sqr();
+        const fr<Builder> y = fr_vec[1];
+        return (y.madd(y, x_sqr * bb::fr(5)).is_zero());


Should this be:

const fr<Builder> five_y = fr_vec[1] * bb::fr(5); return (y.madd(five_y, x_sqr)).is_zero());

as the comment says:

$$x^2 + 5y^2 = 0 \textsf{ mod } p$$

suyash67 · 2025-09-17T00:04:02Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.cpp

+    BB_ASSERT_EQ(static_cast<uint256_t>(low_val) + (static_cast<uint256_t>(hi_val) << NUM_BITS_IN_TWO_LIMBS),
+                 value,
+                 "field_conversion: limb decomposition");
    // checks this decomposition low + hi * 2^64 = value with an assert_equal


nit: the comment needs to be corrected to: low + hi * 2^68

oops, stale comment, fixed

suyash67 · 2025-09-17T00:39:53Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp

+    using bigfield_ct = fq<Builder>;
+
+    if constexpr (IsAnyOf<T, field_ct>) {
+        ;


extra semicolon?

suyash67 · 2025-09-17T01:04:22Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.test.cpp

+{
+    using Builder = TypeParam;
+
+    { // Serialize and deserialize the Grumpkin generator


I think it might be worth adding serialize and deserializing random point(s) on curve for testing along with the edge-cases you've already covered.

thanks, added loops testing 50 random points for both curves

iakovenkos · 2025-09-17T22:06:16Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.cpp

 {
-    constexpr uint64_t NUM_BITS_IN_TWO_LIMBS = 2 * NUM_LIMB_BITS;                // 136
-    constexpr uint64_t UPPER_TWO_LIMB_BITS = TOTAL_BITS - NUM_BITS_IN_TWO_LIMBS; // 118
+    ASSERT(!fr_element.is_constant());


@ledwards2225 after our discussions, i think this should use split_unique from your pr

actually merged your changes from cycle-group-5 to my follow-up and nuked the entire method, see stdlib::field_conversion::convert_challenge in #17034

…i/field-conversion-audit

ledwards2225 · 2025-09-18T16:47:49Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.test.cpp

+        if constexpr (IsAnyOf<Builder, UltraCircuitBuilder>) {
+            EXPECT_THROW_OR_ABORT(group_element = bn254_element<Builder>::from_witness(&builder, group_element_val),
+                                  "");
+        } else {


no failure because this is checked in eccvm right?

ledwards2225 · 2025-09-18T16:50:12Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.test.cpp

 {
    using Builder = TypeParam;
    Builder builder;
+    const fr<Builder> zero(fr<Builder>::from_witness_index(&builder, 0));


I think we should try to avoid this pattern of assuming index 0 corresponds to value 0

ledwards2225 · 2025-09-18T16:52:45Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.test.cpp

+        auto y = bb::stdlib::field_conversion::convert_from_bn254_frs<Builder, T>(frs);
+        bool expected = std::is_same_v<Builder, UltraCircuitBuilder> ? !point_at_infinity : true;
+
+        EXPECT_EQ(x.get_value() == y.get_value(), expected);


nano-nit: in/out seems clearer than x/y in a context where x/y can be plausibly confused with point coordinates

ledwards2225

Looks good to me - nice simplifications

suyash67

cool stuff, thanks for the amazing documentation!

BEGIN_COMMIT_OVERRIDE chore: Tube-related clean up (#17064) chore: cycle group cleanup #5 (#17026) chore: `field_conversion` clean-up/int audit (#16898) fix: bigfield veridise audit fixes (#16842) END_COMMIT_OVERRIDE

@suyash67

### 🧾 Audit Context `stdlib::field_conversion` provides the serialization/deserialization methods for `stdlib::Transcript`. ### 🛠️ Changes Made - Simplified the logic by making it more uniform and getting rid of redundant arguments - Enabled `on_curve` checks for Ultra deserialization of group elements (BN254 and Grumpkin) - Removed redundant range constraints in `fr --> bigfield` conversion and optimized the constraint ensuring that `fr = lo + hi * shift`. Note that limbs were constrained to be (136, 118) bits, although such range constraints are enforced by `bigfield(low, hi)` constructor. - Optimized `point_at_infinity` check for Grumpkin points. - Expanded docs and made them Doxygen-friendly - Added tests and [an issue](AztecProtocol/barretenberg#1541) that will be resolved in a follow-up - The vks and circuit sizes have changed due to enabled checks and the small optimizations described above ### 📌 Notes for Reviewers Agreed with @suyash67 that existing point-at-infinity issue AztecProtocol/barretenberg#1527 should be handled by ensuring the consistent representation of points at infinity in all groups. Added a test that will fail once the issue is resolved. --------- Co-authored-by: ledwards2225 <l.edwards.d@gmail.com>

@suyash67

`stdlib::field_conversion` provides the serialization/deserialization methods for `stdlib::Transcript`. - Simplified the logic by making it more uniform and getting rid of redundant arguments - Enabled `on_curve` checks for Ultra deserialization of group elements (BN254 and Grumpkin) - Removed redundant range constraints in `fr --> bigfield` conversion and optimized the constraint ensuring that `fr = lo + hi * shift`. Note that limbs were constrained to be (136, 118) bits, although such range constraints are enforced by `bigfield(low, hi)` constructor. - Optimized `point_at_infinity` check for Grumpkin points. - Expanded docs and made them Doxygen-friendly - Added tests and [an issue](AztecProtocol/barretenberg#1541) that will be resolved in a follow-up - The vks and circuit sizes have changed due to enabled checks and the small optimizations described above Agreed with @suyash67 that existing point-at-infinity issue AztecProtocol/barretenberg#1527 should be handled by ensuring the consistent representation of points at infinity in all groups. Added a test that will fail once the issue is resolved. --------- Co-authored-by: ledwards2225 <l.edwards.d@gmail.com>

@suyash67

### 🧾 Audit Context `stdlib::field_conversion` provides the serialization/deserialization methods for `stdlib::Transcript`. ### 🛠️ Changes Made - Simplified the logic by making it more uniform and getting rid of redundant arguments - Enabled `on_curve` checks for Ultra deserialization of group elements (BN254 and Grumpkin) - Removed redundant range constraints in `fr --> bigfield` conversion and optimized the constraint ensuring that `fr = lo + hi * shift`. Note that limbs were constrained to be (136, 118) bits, although such range constraints are enforced by `bigfield(low, hi)` constructor. - Optimized `point_at_infinity` check for Grumpkin points. - Expanded docs and made them Doxygen-friendly - Added tests and [an issue](AztecProtocol/barretenberg#1541) that will be resolved in a follow-up - The vks and circuit sizes have changed due to enabled checks and the small optimizations described above ### 📌 Notes for Reviewers Agreed with @suyash67 that existing point-at-infinity issue AztecProtocol/barretenberg#1527 should be handled by ensuring the consistent representation of points at infinity in all groups. Added a test that will fail once the issue is resolved. --------- Co-authored-by: ledwards2225 <l.edwards.d@gmail.com>

iakovenkos added 8 commits September 8, 2025 18:10

initial clean-up

9206325

move asserts

7d1760a

simplify further

abe1b42

convert to grumpkin more efficiently

0c8f5e6

enable on_curve validation for points received in the transcript

d2e6bc8

Merge remote-tracking branch 'origin/merge-train/barretenberg' into s…

c377bdd

…i/field-conversion-audit

docs + question

acdaa51

add missing range constraints

a63559d

iakovenkos commented Sep 9, 2025

View reviewed changes

iakovenkos added 2 commits September 10, 2025 11:40

expand convert_from_bn254_frs docs

af8117b

convert_to_grumpkin_fr docs

5bfb6f6

iakovenkos commented Sep 10, 2025

View reviewed changes

iakovenkos added 8 commits September 15, 2025 13:30

fix and add tests

0ca0024

fix avm build

f21bb97

TestBasicDoubleHonkRecursionConstraints debugging

3e792a5

fix acir test

74fcd3d

fix tests

3d4efb4

remove redundant range constraints

883d484

Merge remote-tracking branch 'origin/merge-train/barretenberg' into s…

f7e2154

…i/field-conversion-audit

clean-up

3c37128

iakovenkos self-assigned this Sep 16, 2025

iakovenkos marked this pull request as ready for review September 16, 2025 12:45

iakovenkos requested a review from jeanmon as a code owner September 16, 2025 12:45

iakovenkos commented Sep 16, 2025

View reviewed changes

iakovenkos added 3 commits September 16, 2025 13:37

Merge remote-tracking branch 'origin/merge-train/barretenberg' into s…

a0dfb28

…i/field-conversion-audit

fix test

95c68fd

fix silly bugs in field conversion tests

6e9b48b

iakovenkos requested review from ledwards2225 and suyash67 September 16, 2025 16:19

suyash67 reviewed Sep 16, 2025

View reviewed changes

suyash67 reviewed Sep 17, 2025

View reviewed changes

address review comments

0ed5343

iakovenkos commented Sep 17, 2025

View reviewed changes

Merge remote-tracking branch 'origin/merge-train/barretenberg' into s…

e0b240c

…i/field-conversion-audit

ledwards2225 reviewed Sep 18, 2025

View reviewed changes

ledwards2225 approved these changes Sep 18, 2025

View reviewed changes

suyash67 approved these changes Sep 18, 2025

View reviewed changes

ledwards2225 added 4 commits September 19, 2025 18:47

field conversion test cleanup

27c18bb

Merge branch 'merge-train/barretenberg' into si/field-conversion-audit

5436aca

vk hash

a84c115

fix build

599e0fc

ledwards2225 merged commit b31eec9 into merge-train/barretenberg Sep 19, 2025
6 checks passed

ledwards2225 deleted the si/field-conversion-audit branch September 19, 2025 19:44

AztecBot mentioned this pull request Sep 19, 2025

feat: merge-train/barretenberg #17157

Merged

Conversation

iakovenkos commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧾 Audit Context

🛠️ Changes Made

📌 Notes for Reviewers

Uh oh!

iakovenkos Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iakovenkos Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iakovenkos Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ledwards2225 left a comment

Choose a reason for hiding this comment

Uh oh!

suyash67 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iakovenkos commented Sep 9, 2025 •

edited

Loading

iakovenkos Sep 9, 2025 •

edited

Loading

iakovenkos Sep 10, 2025 •

edited

Loading

iakovenkos Sep 10, 2025 •

edited

Loading