fix(boojum)!: Deleted the repetition in normalization #42
Conversation
popzxc
changed the title
| // sub modulus | ||
| let modulus = self | ||
| .params | ||
| .modulus | ||
| .map(|el| cs.allocate_constant(F::from_u64_unchecked(el as u64))); |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Not sure that multiplication guarantees full normalization. It can still give the same number of bits, but not < modulus canonical representation |
|
In our implementation, the enforce_reduced function (at the end of multiplication) ensures that the final result is strictly within [0,P) by explicitly performing modular reduction and constraining the canonical form. The reason for this commit is that the stuffing of normalize and enforce_reduce is identical. |
popzxc
changed the title
popzxc
changed the title
When we perform normalization, which is the multiplication of a number by 1, then in the same multiplication we prove that the result is greater than the module and then in normalization we repeat this process, which duplicates the constraints