Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(boojum)!: Deleted the repetition in normalization #42

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(boojum)!: Deleted the repetition in normalization #42

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b03cc4f
Deleted the repetition
Fitznik Nov 18, 2024
3d5ccba
Merge branch 'main' into normalize_fix
popzxc Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 1 addition & 16 deletions crates/boojum/src/gadgets/non_native_field/implementations/implementation_u16.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,22 +136,7 @@ where
// well, we just mul by 1
let mut one: NonNativeFieldOverU16<F, T, N> =
Self::allocated_constant(cs, T::one(), &self.params);
let mut normalized = self.mul(cs, &mut one);

// assert that we only have "modulus limbs" moduluses in this element
assert_eq!(normalized.non_zero_limbs, normalized.params.modulus_limbs);

// sub modulus
let modulus = self
.params
.modulus
.map(|el| cs.allocate_constant(F::from_u64_unchecked(el as u64)));
Comment on lines -144 to -148
Copy link
Member

Choose a reason for hiding this comment

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

I'm not an expert, but since this (seemingly) changes the layout of the circuit, is it a breaking change (e.g. it requires to generate new keys)?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, a new key needs to be generated. However, this is mostly a performance improvement and unrelated to security, so it is not urgent to fix.

Copy link
Member

Choose a reason for hiding this comment

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

I've marked it as a breaking change though: note the ! in the fix! (it instructs the release bot that the change is breaking), and applied Protocol upgrade label (for general awareness -- so that this PR is not merged accidentally).
Please do so in the future PRs if you know that it requires new VKs.

Also cc @mm-zk --- IIRC you maintain a table of tasks/PRs that should be included for each upgrade -- please add this PR there.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for flagging this!

// for rare case when our modulus is exactly 16 * K bits, but we use larger representation
let els_to_skip = N - self.params.modulus_limbs;
let _ =
u16_long_subtraction_noborrow_must_borrow(cs, &normalized.limbs, &modulus, els_to_skip);
assert!(normalized.form == RepresentationForm::Normalized);
normalized.tracker.max_moduluses = 1;
let normalized = self.mul(cs, &mut one);

// update self to normalized one
*self = normalized;
Expand Down
Loading