Skip to content

Comments

chore: preliminary clean up and reorg of cycle_group#16645

Merged
ledwards2225 merged 5 commits intomerge-train/barretenbergfrom
lde/cycle-group-0
Aug 29, 2025
Merged

chore: preliminary clean up and reorg of cycle_group#16645
ledwards2225 merged 5 commits intomerge-train/barretenbergfrom
lde/cycle-group-0

Conversation

@ledwards2225
Copy link
Contributor

@ledwards2225 ledwards2225 commented Aug 28, 2025

Preliminary cycle_group cleanup (no logic changes):

  • Extract subclasses cycle_scalar, straus_lookup_table and straus_scalar_slice and move to their own files
  • Remove methods marked requires IsNotUltraArithmetic<Builder> since no no-Ultra builders exist

Note: some branching based on "is Ultra" remains within certain methods and will be removed in subsequent PRs

@ledwards2225 ledwards2225 changed the base branch from next to merge-train/barretenberg August 28, 2025 20:06
return result;
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-Ultra method deleted entirely

@ledwards2225 ledwards2225 self-assigned this Aug 29, 2025
@ledwards2225 ledwards2225 marked this pull request as ready for review August 29, 2025 16:13
@@ -20,8 +23,6 @@ namespace bb::stdlib {

template <typename Builder>
concept IsUltraArithmetic = (Builder::CIRCUIT_TYPE == CircuitType::ULTRA);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls remove this concept

field_t borrow = lo.is_constant() ? need_borrow : field_t::from_witness(get_context(), need_borrow);

// directly call `create_new_range_constraint` to avoid creating an arithmetic gate
if (!lo.is_constant()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

field_t has a method split_at, I think it can be re-used here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh split_at actually assumes that there's no wrapping, i.e. num_bits <= grumpkin::MAX_NO_WRAP_INTEGER_BIT_LENGTH (=252). @suyash67 shouldn't this constant actually be 253?

base_point.is_point_at_infinity(), offset_generator, point_table[i]);
}
}
if constexpr (IS_ULTRA) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls remove here and below

Copy link
Contributor

@iakovenkos iakovenkos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left a couple of comments re the future work

@ledwards2225 ledwards2225 merged commit bf936e7 into merge-train/barretenberg Aug 29, 2025
6 checks passed
@ledwards2225 ledwards2225 deleted the lde/cycle-group-0 branch August 29, 2025 20:37
github-merge-queue bot pushed a commit that referenced this pull request Aug 30, 2025
BEGIN_COMMIT_OVERRIDE
chore: preliminary clean up and reorg of cycle_group (#16645)
END_COMMIT_OVERRIDE
mralj pushed a commit that referenced this pull request Oct 13, 2025
Preliminary `cycle_group` cleanup (no logic changes):
- Extract subclasses `cycle_scalar`, `straus_lookup_table` and
`straus_scalar_slice` and move to their own files
- Remove methods marked `requires IsNotUltraArithmetic<Builder>` since
no no-Ultra builders exist

Note: some branching based on "is Ultra" remains _within_ certain
methods and will be removed in subsequent PRs
ludamad pushed a commit that referenced this pull request Dec 16, 2025
Preliminary `cycle_group` cleanup (no logic changes):
- Extract subclasses `cycle_scalar`, `straus_lookup_table` and
`straus_scalar_slice` and move to their own files
- Remove methods marked `requires IsNotUltraArithmetic<Builder>` since
no no-Ultra builders exist

Note: some branching based on "is Ultra" remains _within_ certain
methods and will be removed in subsequent PRs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants