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

Add discrete logarithm for anomalous elliptic curves #34253

Closed
sylvainpelissier opened this issue Jul 31, 2022 · 23 comments
Closed

Add discrete logarithm for anomalous elliptic curves #34253

sylvainpelissier opened this issue Jul 31, 2022 · 23 comments

Comments

@sylvainpelissier
Copy link
Contributor

A fast method to solve the discrete logarithm in anomalous elliptic curves uses the p-adic logarithm as described in: https://link.springer.com/article/10.1007/s001459900052. I implemented the algorithm based on https://crypto.stackexchange.com/questions/70454/why-smarts-attack-doesnt-work-on-this-ecdlp/70508#70508 for EllipticCurvePoint_finite_field

Component: number theory

Keywords: p-adic discrete logarithm

Author: Sylvain Pelissier

Branch/Commit: 86ad465

Reviewer: Lorenz Panny

Issue created by migration from https://trac.sagemath.org/ticket/34253

@sylvainpelissier
Copy link
Contributor Author

Commit: f33b250

@sylvainpelissier
Copy link
Contributor Author

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 2, 2022

Reviewer: Lorenz Panny

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 2, 2022

comment:2

Thanks for writing this code, it looks very nice! A few comments:

  • The test
+        if E.order() == E.base().order():

can be very expensive if the order of the curve hasn't been computed yet. (Not a very common scenario, but possible by using .set_order() on points.)
Suggestion: Replace by

n == E.base_field().cardinality()

or something like that.

  • If I remember correctly, E.formal_group().log() is another way to construct the power series underlying this mapping. Perhaps we should add SEEALSO cross-references, and/or reuse that method (assuming it doesn't make things slower or otherwise worse)?

  • Here

+        For anomalous curves with `#E = p`, the `padic_elliptic_logarithm`

you can use the syntax

:meth:`padic_elliptic_logarithm`

to turn the padic_elliptic_logarithm reference into a link in the HTML documentation.

  • This change
-        EXAMPLES::
+        EXAMPLES:

             sage: F = GF((3,6),'a')

is incorrect; the documentation format requires a double colon before an indented block.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

726112aCorrections on padic_elliptic_logarithm based on remarks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2022

Changed commit from f33b250 to 726112a

@sylvainpelissier
Copy link
Contributor Author

comment:4

Thank you for your remarks they are helpful. I corrected everything expect the one related to E.formal_group().log(). I would need more time to test and understand if it works the same way or bring improvements.

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 3, 2022

comment:5

Looks good.

One last thing: Much of the new code is a slightly modified version of https://crypto.stackexchange.com/a/70508, so we must give proper attribution in the docstring (e.g., in the AUTHORS or the ALGORITHM part).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

5069aacAdd proper attribution

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Changed commit from 726112a to 5069aac

@sylvainpelissier
Copy link
Contributor Author

comment:7

Yes you are right. This should be fine now.

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 4, 2022

comment:8

Here's an example for which this branch fails to compute .discrete_log():

sage: R.<x> = ZZ[]
sage: F.<i> = GF(787^2, modulus=x^2+1)
sage: E = EllipticCurve([268*i + 507, 42*i + 772])

You could either add an .is_prime_field() check, or (better) generalize the code to non-prime finite fields.

(By the way, you should set the ticket to "needs review" once it's ready from your perspective. I looked at this anyway because I didn't notice it was still "new", but in general you will only attract reviewers by setting the correct status.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2022

Changed commit from 5069aac to 3c0c8a7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

3c0c8a7Add primiality test

@sylvainpelissier
Copy link
Contributor Author

comment:10

I've added the primality test. I have to figure out how to handle the non prime field case. Do you want me to open another ticket for that ?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2022

Changed commit from 3c0c8a7 to 538f50e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

538f50eRearrange checks

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 5, 2022

comment:13

I think this is incorrect for non-anomalous curves over prime fields: In that case, the first if holds (so the elif branches are skipped), but for a non-cyclic group we do still have to run the pairing check to ensure there's a solution.

Given that the prime case is already working and useful on its own, I think the non-prime case should go into a new ticket.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2022

Changed commit from 538f50e to 86ad465

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

86ad465Correct check for anomalous curves

@sylvainpelissier
Copy link
Contributor Author

comment:15

Good catch thank you. It is corrected.

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 6, 2022

comment:16

Thanks!

@vbraun
Copy link
Member

vbraun commented Aug 7, 2022

Changed branch from u/gh-sylvainpelissier/padic_elliptic_logarithm to 86ad465

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants