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

p521: impl FieldElement::{sqrt, invert} and add basic field tests #946

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Nov 1, 2023

See also: #947

Also relevant (sage calculations):

@MasterAwesome
Copy link
Contributor

MasterAwesome commented Nov 1, 2023

The arithmetic itself seems to work selectively, like mul, it needs the invert and square function implementation, but for the two_inverse_const, I can pass it if I input the correct FieldElement limbs:

two = FieldElement([2, 0, 0, 0, 0, 0, 0, 0, 0])
invert = FieldElement([0, 0, 0, 0, 0, 0, 0, 0, 72057594037927936])
mul = FieldElement([1, 0, 0, 0, 0, 0, 0, 0, 0])

After handwriting the inverse values field elements, running the tests:

test arithmetic::field::tests::one_is_multiplicative_identity ... ok
test arithmetic::field::tests::delta_constant ... FAILED
test arithmetic::field::tests::root_of_unity_constant ... FAILED
test arithmetic::field::tests::root_of_unity_inv_constant ... FAILED
test arithmetic::field::tests::zero_is_additive_identity ... ok
test arithmetic::field::tests::two_inv_constant ... ok
test arithmetic::scalar::tests::one_is_multiplicative_identity ... ok
test arithmetic::scalar::tests::delta_constant ... ok
test arithmetic::scalar::tests::root_of_unity_inv_constant ... ok
test arithmetic::scalar::tests::two_inv_constant ... ok
test arithmetic::scalar::tests::zero_is_additive_identity ... ok
test arithmetic::scalar::tests::root_of_unity_constant ... ok
test arithmetic::scalar::tests::invert ... ok

@tarcieri
Copy link
Member Author

tarcieri commented Nov 1, 2023

Aah yes, PrimeField::{TWO_INV, ROOT_OF_UNITY_ENV} are still stubbed out due to a missing inversion implementation.

That doesn't explain why the root_of_unity_constant and delta_constant tests are failing, though. That would speak to an issue with the constant definitions, pow_vartime, or mul

@tarcieri
Copy link
Member Author

tarcieri commented Nov 1, 2023

Regarding square root, it should be possible to use the Tonelli–Shanks algorithm (https://eprint.iacr.org/2012/685.pdf) for the case where q ≡ 3 (mod 4).

This is a special case that can be solved using a simpler variant which is just the Shanks algorithm, computed as a single exponentiation, raising the input to (q + 1) / 4 (see p.2).

I'm computing q as:

0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff

...and (q + 1) / 4 as:

0x4000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

...although if I attempt to implement square root that way it's failing the tests.

It computes sqrt(4 mod p) as:

FieldElement([0, 0, 0, 0, 536870912, 0, 0, 0, 0])

...which, when squared, yields:

FieldElement([2, 0, 0, 0, 0, 0, 0, 0, 0])

...instead of the expected 4.

Edit: pushed up sqrt experiments as 142d5f5

@MasterAwesome
Copy link
Contributor

MasterAwesome commented Nov 1, 2023

Interesting, I implemented sqrt by squaring self 519 times but realized it fails for 9,25,36,49. The fe.sqrt().square() had an interesting property top word is always input-1. So for 9 the tight_field[0] = 8. Same for 25,36,49. I'm unsure why the other words are incorrect. I'll take a look into it soon.

EDIT: Something is weird with my arithmetic. If I take FieldElement::from(9).sqrt(), take that and square it it generates FieldElement ([8, 0, ....]), and if I take that and do fiat_p521_from_bytes(fiat_p521_to_bytes(out)). I get the correct output :o. I suspect this is also a consequence of unsaturated math. I'm able to pass all sqrt tests.

@MasterAwesome
Copy link
Contributor

MasterAwesome commented Nov 1, 2023

Maybe the strategy is to reimplement PartialEq to compare as bytes instead of words, checking if this works now.

EDIT: Works, seems to be because of the way the FieldElement can be represented since we are unsaturated. Your implementation of sqrt has incorrect math, the value of w needs to have an 8 instead of 4. But just squaring 519 times is possibly faster? Benchmarked a few samples and it does seem faster to sqn(519) instead of pow_vartime

fn eq(&self, rhs: &Self) -> bool {
    let lhs = fiat_p521_to_bytes(&self.0);
    let rhs = fiat_p521_to_bytes(&rhs.0);

    lhs.ct_eq(&rhs).into()
}

0x00000000_00000000,
0x00000000_00000000,
0x00000000_00000000,
0x00000000_00000040,
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to end with 80 because it's 2^519.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, interesting, substituting:

Suggested change
0x00000000_00000040,
0x00000000_00000080,

(not sure I follow why, but)

...passes sqrt for 1 and 4 but fails on 9:

---- arithmetic::field::tests::sqrt stdout ----
thread 'arithmetic::field::tests::sqrt' panicked at 'assertion failed: `(left == right)`
  left: `FieldElement([8, 0, 288230376151711744, 288230376151711743, 288230376151711743, 288230376151711743, 288230376151711743, 288230376151711743, 144115188075855871])`,
 right: `FieldElement([9, 0, 0, 0, 0, 0, 0, 0, 0])`', p521/src/arithmetic/field.rs:574:5

Copy link
Contributor

@MasterAwesome MasterAwesome Nov 2, 2023

Choose a reason for hiding this comment

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

9 fails due to #946 (comment)

This will fix it. The field element output you see there is the correct representation of 9. Just that their PartialEq comparison is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

As for the why, (p+1)/4 = (2^521 - 1 + 1)/4 = (2^521)/2^2 = 2^519 = 0x80000...

Copy link
Member Author

Choose a reason for hiding this comment

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

Where did you get that formula?

I'm looking at p.2 of: https://eprint.iacr.org/2012/685.pdf

...which says:

In the case that q ≡ 3 (mod 4), one can simply use a specialized version of the Tonelli-Shanks
procedure, the Shanks algorithm, where the square root of a quadratic residue a ∈ Fq, can be
computed via one single exponentiation as, b = a * (q+1)/4

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, indeed it all works, the sqrt tests are passing, although I'm still not sure why it's (p+1)/4 instead of (q+1)/4

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming our number is a quadratic residue => x^((p-1)/2)) = 1 due to Euler's criterion. Therefore x^((p+1)/2)) = x.

There's a simplification that can be made when p=3mod4 in which case p can be expressed as 4k +3 which guarantees (p+1)/4 to be an integer in which case (x^((p+1)/4))^2 = x. This means x^((p+1)/4) is the square root.

@tarcieri
Copy link
Member Author

tarcieri commented Nov 2, 2023

Looks like we just need inversions now. I found this paper which specifies an addition chain for P-521 inversions, although I'm not sure it's the most efficient option: https://eprint.iacr.org/2014/852.pdf

Edit: I remembered I also had #787 open for this

@tarcieri tarcieri changed the title [WIP] p521: add basic field tests p521: impl FieldElement::{sqrt, invert} and add basic field tests Nov 2, 2023
@tarcieri
Copy link
Member Author

tarcieri commented Nov 2, 2023

With 82d6608 added all of the basic field tests are now passing! 🎉

@tarcieri tarcieri marked this pull request as ready for review November 2, 2023 16:29
@tarcieri tarcieri merged commit 6fd4817 into master Nov 2, 2023
11 checks passed
@tarcieri tarcieri deleted the p521/basic-field-tests branch November 2, 2023 16:46
@MasterAwesome
Copy link
Contributor

With 82d6608 added all of the basic field tests are now passing! 🎉

You should be able to simplify the math that's happening in pow by just squaring 519 times. Also thanks for working with me on this, I appreciate the quick replies and everything else!

@tarcieri
Copy link
Member Author

tarcieri commented Nov 2, 2023

Thanks for helping to get it working!

tarcieri pushed a commit that referenced this pull request Nov 2, 2023
When using p521, the limbs can be populated differently which when
deriving [`Debug`] shows up as different values for equal FieldElements,
this can lead to confusion.

See:
-
#946 (comment),
equal field elements were considered as unequal.
-
#946 (comment),
assert_eq! wasn't meaningful.

Signed-off-by: Arvind Mukund <armu30@gmail.com>
tarcieri added a commit that referenced this pull request Nov 2, 2023
Now that #946, #950, and #951 have landed it seems prudent to add a real
`arithmetic` feature.

This adds a feature similar to the other crates in this repo which
exposes the following types which provide a curve arithmetic
implementation:

- `AffinePoint`
- `ProjectivePoint`
- `Scalar`

The `wip-arithmetic-do-not-use` feature is now removed as well. While
technically SemVer breaking, it wasn't supposed to be used in the first
place!

Closes #947
@tarcieri tarcieri mentioned this pull request Nov 2, 2023
tarcieri added a commit that referenced this pull request Nov 2, 2023
Now that #946, #950, and #951 have landed it seems prudent to add a real
`arithmetic` feature.

This adds a feature similar to the other crates in this repo which
exposes the following types which provide a curve arithmetic
implementation:

- `AffinePoint`
- `ProjectivePoint`
- `Scalar`

The `wip-arithmetic-do-not-use` feature is now removed as well. While
technically SemVer breaking, it wasn't supposed to be used in the first
place!

Closes #947
tarcieri added a commit that referenced this pull request Nov 3, 2023
Now that #946, #950, and #951 have landed it seems prudent to add a real
`arithmetic` feature.

This adds a feature similar to the other crates in this repo which
exposes the following types which provide a curve arithmetic
implementation:

- `AffinePoint`
- `ProjectivePoint`
- `Scalar`

The `wip-arithmetic-do-not-use` feature is now removed as well. While
technically SemVer breaking, it wasn't supposed to be used in the first
place!

Closes #947
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