Conversation
Removes use of unsafe transmute
Lacks optimisations for anchors and carries
mp variable
ocdbytes
left a comment
There was a problem hiding this comment.
LGTM !
some nit picks and some personal doubts I had while reviewing.
Suggestion (Not necessary) : Also we can add edge case tests for 0 values, identity values etc. which may not come in proptest for simd_mul and simd_sqr.
| }; | ||
|
|
||
| /// Two parallel Montgomery squarings: `(v0², v1²)`. | ||
| /// input must fit in 2^255-1; no runtime checking |
There was a problem hiding this comment.
Doubt : why are we not checking the limits of the inputs here ?
There was a problem hiding this comment.
The main reason is that these are internal functions to a kernel that runs a hot loop. The kernel needs to be designed in a way that the precondition holds because checking in a hot loop is costly. This note is there as a reminder to let the kernel have this invariant.
I did add a debug assert to u256_to_u255 to check if the invariant is actually being uphold when running in debug mode.
There was a problem hiding this comment.
cool works. Thanks for clarifying
I try to not have overlap in tests. They are mathematically special but they are not edge cases. I would have used those if we didn't have a reference to test against. |
| } | ||
|
|
||
| #[test] | ||
| fn test_simd_sqr() { |
There was a problem hiding this comment.
Nit: test_simd_sqr validates against simd_mul(a, a, b, b) but not independently against ark_ff_reference. Since test_simd_mul already validates mul against ark_ff, the chain sqr → mul → ark_ff provides transitive coverage. But the sqr-specific code path (cross-terms doubled + diagonal terms added separately) differs from mul's schoolbook, so a direct ark_ff_reference(a, a) check would add confidence for free:
let a2_ref = ark_ff_reference(a, a);
prop_assert_eq!(Fr::new(BigInt(a2s)), a2_ref);There was a problem hiding this comment.
simd_sqr can be bitwise compared against simd_mul without a transformation (happens within ark_ff_reference and Fr::new). This makes sqr-specific code path easier to debug. Property-based tests with transitivity gives us adequate coverage.
batched bn254 51 bits RNE multiplier
Reviewing: Not all file movements and renames are tracked properly by Github's diff view. The important folder is
rnewhich contain the new Montgomery multiplication implementation. I've tried to stay true to the structure of the one inrtz.