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.

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

k256: implement Scalar::sqrt #400

Merged
merged 1 commit into from
Aug 31, 2021
Merged

k256: implement Scalar::sqrt #400

merged 1 commit into from
Aug 31, 2021

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Aug 31, 2021

Implements Tonelli-Shank's algorithm for q mod 16 = 1 as synthesized using the ff_derive crate, similar to #392 which implements it for the p256.

Like in p256, as part of implementing this it was discovered that root_of_unity() was incorrect. Here it is (re)calculated with sage:

sage: n = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141
sage: GF(n).primitive_element()
7
sage: s = 6
sage: t = (n - 1) >> s
sage: power_mod(7,t,n)
5480320495727936603795231718619559942670027629901634955707709633242980176626

Note that the value was computed correctly originally, but the hex digits were shifted such that the resulting value was left shifted by 4-bits. This has now been corrected.

@tarcieri tarcieri requested review from fjarri and str4d August 31, 2021 17:05
@tarcieri tarcieri force-pushed the k256/scalar-sqrt branch 2 times, most recently from 0d89dc9 to 57a729f Compare August 31, 2021 17:30
@tarcieri tarcieri mentioned this pull request Aug 31, 2021
2 tasks
Comment on lines +120 to +126
let w = {
let t0 = self;
let t1 = t0.square();
let t2 = t1 * t0;
let t3 = t1.square();
let t4 = t3.square();
Copy link
Member Author

Choose a reason for hiding this comment

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

Curiously I wasn't able to replace this with pow_vartime as copied/pasted from p256, which is how sqrt is implemented there.

I tried the following:

        let w = self.pow_vartime(&[
            0x77fa4bd19a06c82,
            0xd755db9cd5e91407,
            0xffffffffffffffff,
            0x1fffffffffffffff,
        ]);

It's possible I miscomputed the exponent. Here's my Sage:

sage: n = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141
sage: n
115792089237316195423570985008687907852837564279074904382605163141518161494337
sage: s = 6
sage: t = (n - 1) >> s
sage: t
1809251394333065553493296640760748560200586941860545380978205674086221273349
sage: (t - 1) / 2
904625697166532776746648320380374280100293470930272690489102837043110636674
sage: hex((t - 1) / 2)
'0x1fffffffffffffffffffffffffffffffd755db9cd5e9140777fa4bd19a06c82'

@tarcieri tarcieri changed the title [WIP] k256: implement Scalar::sqrt k256: implement Scalar::sqrt Aug 31, 2021
@tarcieri tarcieri marked this pull request as ready for review August 31, 2021 18:13
Implements Tonelli-Shank's algorithm for q mod 16 = 1 as synthesized
using the `ff_derive` crate, similar to #392 which implements it for the
`p256`.

Like `p256`, as part of implementing this it was discovered that
`root_of_unity()` was incorrect. Here it is (re)calculated with sage:

    sage: n = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141
    sage: GF(n).primitive_element()
    7
    sage: s = 6
    sage: t = (n - 1) >> s
    sage: power_mod(7,t,n)
    5480320495727936603795231718619559942670027629901634955707709633242980176626

Note that the value was computed correctly originally, but the hex
digits were shifted such that the resulting value was left shifted by
4-bits. This has now been corrected.
@codecov-commenter
Copy link

Codecov Report

Merging #400 (d333555) into master (b9d0904) will increase coverage by 3.71%.
The diff coverage is 98.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
+ Coverage   54.88%   58.59%   +3.71%     
==========================================
  Files          29       29              
  Lines        4207     4565     +358     
==========================================
+ Hits         2309     2675     +366     
+ Misses       1898     1890       -8     
Impacted Files Coverage Δ
k256/src/arithmetic/scalar.rs 91.63% <98.89%> (+13.60%) ⬆️
k256/src/arithmetic/scalar/scalar_4x64.rs 92.03% <0.00%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9d0904...d333555. Read the comment docs.

@tarcieri tarcieri merged commit eda5854 into master Aug 31, 2021
@tarcieri tarcieri deleted the k256/scalar-sqrt branch August 31, 2021 20:31
tarcieri added a commit that referenced this pull request Sep 2, 2021
I had previously attempted to do this in #400 but had trouble tracking
down why it wasn't working. It seems the lower two limbs were shifted
shifted 8-bits, i.e. a copy-paste error from Sage.

I computed the correct limbs using `ff_derive`, and the tests now pass.
tarcieri added a commit that referenced this pull request Sep 2, 2021
I had previously attempted to do this in #400 but had trouble tracking
down why it wasn't working. It seems the lower two limbs were shifted
shifted 8-bits, i.e. a copy-paste error from Sage.

I computed the correct limbs using `ff_derive`, and the tests now pass.
This was referenced Dec 14, 2021
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.

None yet

2 participants