chore: cycle group cleanup #5 by ledwards2225 · Pull Request #17026 · AztecProtocol/aztec-packages

ledwards2225 · 2025-09-12T23:37:24Z

Cleaning up and restricting the interface to cycle_scalar. The main purpose of this thread of work is to make it easier to identify when certain constraints need to be applied (e.g. primality) and to make applying them less error prone:

Add method field_t::split_at_unrestricted() (better name TBD!) to implement hi/lo splitting logic previously housed in cycle_scalar
Remove unused constructors cycle_scalar(const field_t&) and cycle_scalar(const ScalarField&)
remove option to skip_primality_test in method create_from_bn254_scalar()

ledwards2225 · 2025-09-14T21:32:10Z

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

        // 1: add entry where point, scalar are witnesses
        expected += (element * scalar);
        points.emplace_back(cycle_group_ct::from_witness(&builder, element));
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));


cycle_scalar::from_witness is not used anywhere in production so it seems better to test the main production use case instead which is cycle_scalars constructed from bigfield elements (i.e. in the ECCVM recursive verifier). I've updated all of the tests accordingly. Eventually I may expand them to use cycle_scalars constructed through all of the main production entrypoints

ledwards2225 · 2025-09-14T21:33:32Z

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

    , hi(_hi)
 {}

-template <typename Builder> cycle_scalar<Builder>::cycle_scalar(const field_t& in)


previously only used in the transcript split_challenge which now uses an analogous field_utils method

ledwards2225 · 2025-09-14T21:33:47Z

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

-    hi.set_origin_tag(in.get_origin_tag());
-}
-
-template <typename Builder> cycle_scalar<Builder>::cycle_scalar(const ScalarField& in)


This was not used anywhere

ledwards2225 · 2025-09-14T21:35:11Z

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

 * @return cycle_scalar<Builder>
 */
-template <typename Builder>
-cycle_scalar<Builder> cycle_scalar<Builder>::create_from_bn254_scalar(const field_t& in, const bool skip_primality_test)


this method should never skip the bn254 scalar field primality test (and indeed this path was never used) so I've removed skip_primality_test from the signature.

ledwards2225 · 2025-09-14T21:37:11Z

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


-    cycle_scalar result{ lo, hi, NUM_BITS, skip_primality_test, true };
-    return result;
+    return cycle_scalar{ lo, hi, NUM_BITS, skip_primality_test, use_bn254_scalar_field_for_primality_test };


I think eventually I can just get rid of skip_primality_test and use_bn254_scalar_field_for_primality_test altogether and simply perform the right primality test right in the appropriate constructor / factory method. One step at a time.

ledwards2225 · 2025-09-15T03:30:12Z

barretenberg/cpp/src/barretenberg/stdlib/transcript/transcript.hpp

+        // WORKTODO: now that we're not using cycle_scalar, should we split differently?
+        const size_t low_bits = 128;
+        const size_t high_bits = 126;
+        const auto [lo, high] = challenge.split_at_unrestricted(low_bits);


This change has the effect of removing a validate_scalar_is_in_field(); call which was present in the constructor cycle_scalar(const field_t&) and therefore previously applied here. I don't see why such a check would be needed - maybe I'm missing something.

I suppose in the recursive verifier setting we can have challenges of one scaler field represented in another, e.g. in the eccvm.

The primality check is not needed here, cause the challenge is computed in-circuit, and it's fr by default, which is then converted in-circuit to bigfield in the case of eccvm

ledwards2225 · 2025-09-15T18:18:40Z

barretenberg/cpp/src/barretenberg/stdlib/eccvm_verifier/eccvm_recursive_verifier.test.cpp


        // Check that the size of the recursive verifier is consistent with historical expectation
-        uint32_t NUM_GATES_EXPECTED = 213923;
+        uint32_t NUM_GATES_EXPECTED = 213307;


Gate count reduction here and in UH rec verifier is due to no longer calling validate_scalar_is_in_field() within transcript method split_challenge(). This used to happen under the hood by virtue of using cycle_scalar(const field_t&) to perform the hi/lo splitting. We now use field_t::split_at() which performs identical splitting but does not perform an "in field" check.

ledwards2225 · 2025-09-15T19:38:08Z

barretenberg/cpp/src/barretenberg/stdlib/transcript/transcript.hpp

-        scalar.lo.create_range_constraint(cycle_scalar::LO_BITS);
-        scalar.hi.create_range_constraint(cycle_scalar::HI_BITS);
-        return std::array<DataType, 2>{ scalar.lo, scalar.hi };
+        // Note: Current choice of bit splitting is somewhat arbitrary and based on historic use of cycle_scalar.


I'm leaving this in a bit of an in-progress state as I'm hoping the details will be resolved by Sergei in his transcript / field _conversion investigations. In particular, (1) it would be nice to set lo_bits/hi_bits more systematically (e.g. should both be 127?) and (2) I don't see why we need the range constraints.

since you're using split_at the range constraints afterwards should be removed

ledwards2225 · 2025-09-15T19:58:55Z

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

+                                                                                 const size_t num_bits) const
 {
    ASSERT(lsb_index < num_bits);
    ASSERT(num_bits <= grumpkin::MAX_NO_WRAP_INTEGER_BIT_LENGTH);


I'm not sure why this constant is 252 - would be nice to add some documentation

iakovenkos · 2025-09-16T11:09:19Z

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

    Builder* get_context() const { return context; }

-    std::pair<field_t<Builder>, field_t<Builder>> split_at(
+    std::pair<field_t<Builder>, field_t<Builder>> split_at(const size_t lsb_index) const;


makes sense to me

iakovenkos

pls remove extra constraints after split_at, otherwise looks good!

iakovenkos · 2025-09-18T12:46:17Z

barretenberg/cpp/src/barretenberg/stdlib/transcript/transcript.hpp

-        using cycle_scalar = typename stdlib::cycle_group<Builder>::cycle_scalar;
-        const cycle_scalar scalar = cycle_scalar(challenge);
-        scalar.lo.create_range_constraint(cycle_scalar::LO_BITS);
-        scalar.hi.create_range_constraint(cycle_scalar::HI_BITS);


I think the vks have changed bc you removed the range constraints here. The gate count is likely the same because of the range constraint batching

I didn't remove them though, they just moved to split_unique. but its possible some ordering has changed

ledwards2225 · 2025-09-19T18:11:12Z

barretenberg/cpp/scripts/test_civc_standalone_vks_havent_changed.sh

 # - Upload the compressed results: aws s3 cp bb-civc-inputs.tar.gz s3://aztec-ci-artifacts/protocol/bb-civc-inputs-[hash(0:8)].tar.gz
 # Note: In case of the "Test suite failed to run ... Unexpected token 'with' " error, need to run: docker pull aztecprotocol/build:3.0
-pinned_short_hash="5aa55c9f"
+pinned_short_hash="04f38201"


No change to number of gates, only the details of constant values due to the use of changing the form of operations like a.assert_equal(b) in a couple places (e.g. using evaluate_linear_identity instead)

ledwards2225 · 2025-09-19T18:13:02Z

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

    (void)a;
    EXPECT_FALSE(builder.failed());
-    EXPECT_TRUE(CircuitChecker::check(builder));
+    check_circuit_and_gates(builder, 1);


adding gate pinning logic to all of these tests to avoid unexplained changes

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

Cleaning up and restricting the interface to cycle_scalar. The main purpose of this thread of work is to make it easier to identify when certain constraints need to be applied (e.g. primality) and to make applying them less error prone: - Add method `field_t::split_at_unrestricted()` (better name TBD!) to implement hi/lo splitting logic previously housed in `cycle_scalar` - Remove unused constructors `cycle_scalar(const field_t&)` and `cycle_scalar(const ScalarField&)` - remove option to `skip_primality_test` in method `create_from_bn254_scalar()`

remove unused cycle_scalar interfaces and use bigfield in test suite

6b23fd2

ledwards2225 changed the base branch from next to merge-train/barretenberg September 12, 2025 23:37

ledwards2225 marked this pull request as ready for review September 13, 2025 21:18

ledwards2225 added 4 commits September 13, 2025 21:41

remove option to skip primality in construct from bn254

4832bd6

use from_witness for consistency

bf3eacb

add split_at_unrestricted (bad name!)

c6292e0

add split method to field and bring back from witness for fuzzer build

14c1400

ledwards2225 commented Sep 14, 2025

View reviewed changes

bring back constructor for fuzzer

fb17bdf

ledwards2225 commented Sep 15, 2025

View reviewed changes

update plsit at naming and fix tests

ce5e3da

ledwards2225 commented Sep 15, 2025

View reviewed changes

ledwards2225 added 2 commits September 15, 2025 19:15

use split_at in create_from_bn254_scalar

dd1401e

cleanup

3035add

ledwards2225 commented Sep 15, 2025

View reviewed changes

Merge branch 'merge-train/barretenberg' into lde/cycle-group-5

984cb44

ledwards2225 commented Sep 15, 2025

View reviewed changes

Merge branch 'next' into lde/cycle-group-5

d1d440e

ledwards2225 requested review from IlyasRidhuan, charlielye, fcarreiro and jeanmon as code owners September 16, 2025 03:24

ledwards2225 changed the base branch from merge-train/barretenberg to next September 16, 2025 03:29

ledwards2225 changed the base branch from next to merge-train/barretenberg September 16, 2025 03:29

fcarreiro removed their request for review September 16, 2025 09:21

iakovenkos mentioned this pull request Sep 16, 2025

Transcript/field_conversion: remove redundant constraints in fr splitting and conversion to bigfield AztecProtocol/barretenberg#1541

Closed

iakovenkos reviewed Sep 16, 2025

View reviewed changes

iakovenkos self-requested a review September 16, 2025 13:15

iakovenkos approved these changes Sep 16, 2025

View reviewed changes

ledwards2225 added 3 commits September 17, 2025 20:09

implement split unique in field utils

d9eb676

Merge branch 'merge-train/barretenberg' into lde/cycle-group-5

7c563b0

fix wasm build

5da71f1

iakovenkos reviewed Sep 18, 2025

View reviewed changes

ledwards2225 added 9 commits September 18, 2025 14:41

Merge branch 'merge-train/barretenberg' into lde/cycle-group-5

6001747

revert test changes for now

e690fd2

add num gates pinning to cycle group tests

83be28b

use evaluate linear identity; no gate change

1de5c17

Merge branch 'merge-train/barretenberg' into lde/cycle-group-5

50808c0

vk hash

12c79de

Merge branch 'merge-train/barretenberg' into lde/cycle-group-5

1b5047e

vk hash

739ad72

try again

99c1cd1

ledwards2225 commented Sep 19, 2025

View reviewed changes

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

ledwards2225 deleted the lde/cycle-group-5 branch September 19, 2025 18:14

AztecBot mentioned this pull request Sep 19, 2025

feat: merge-train/barretenberg #17157

Merged

Conversation

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

Uh oh!

Uh oh!

ledwards2225 Sep 14, 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!

ledwards2225 Sep 14, 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!

iakovenkos Sep 16, 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!

ledwards2225 Sep 15, 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 left a comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ledwards2225 commented Sep 12, 2025 •

edited

Loading

ledwards2225 Sep 14, 2025 •

edited

Loading

ledwards2225 Sep 14, 2025 •

edited

Loading

iakovenkos Sep 16, 2025 •

edited

Loading

ledwards2225 Sep 15, 2025 •

edited

Loading