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 option to use pari for small_roots #39243

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

grhkm21
Copy link
Contributor

@grhkm21 grhkm21 commented Jan 2, 2025

Use pari.zncoppersmith as an alternative Coppersmith implementation for small_roots.

I decided to make pari the default because in my testings (in many practical cases, not really "edge cases") it outperforms Sage's default ("basic") implementation, both in speed and strength - see the added test for example.

@grhkm21 grhkm21 requested a review from yyyyx4 January 2, 2025 04:10
@grhkm21
Copy link
Contributor Author

grhkm21 commented Jan 2, 2025

Hmm, it seems that the pari zncoppersmith errors quite often for some reason (stack overflow) - I believe they keep trying to incrementing the constructed matrix size until the theoretical limit. Maybe changing the default to pari isn't a good idea? Any opinions?

@user202729
Copy link
Contributor

user202729 commented Jan 2, 2025

Actually this would be a breaking change: previously small_roots(algorithm="pari") would still work, the algorithm= is just passed directly to .LLL() which it accepts.

   AVAILABLE ALGORITHMS:

   * "'NTL:LLL'" -- NTL's LLL + choice of "fp"

   * "'fpLLL:heuristic'" -- fpLLL's heuristic + choice of "fp"

   * "'fpLLL:fast'" -- fpLLL's fast + choice of "fp"

   * "'fpLLL:proved'" -- fpLLL's proved + choice of "fp"

   * "'fpLLL:wrapper'" -- fpLLL's automatic choice (default)

   * "'pari'" -- pari's qflll

Maybe rename the new algorithm= to small_roots_algorithm=, the old algorithm= to lll_algorithm=, and deprecate the old algorithm=?

that is, if the zncoppersmith() is still faster than the old LLL(algorithm=pari).


Another alternative I can think of is algorithm="pari_zncoppersmith being treated as a special case.

Worst case (if default to pari isn't better in all cases) you can try to do something such as (pseudocode)

t = Timer(5, lambda: warning("looks like small_roots() is taking a long time, try using algorithm='pari'?")
B = B.LLL()
t.cancel()

then the user still get notification to possibly change, but only if it's slow.

Copy link

github-actions bot commented Jan 2, 2025

Documentation preview for this PR (built with commit 1b57621; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jan 2, 2025

I'll fix the algorithm keyword first. I think it'll be a bad idea to have lll_algorithm, we should just keep the old algorithm, but indeed rename the current algorithm to small_roots_algorithm.

Also, as the example suggests, pari is not only faster than Sage, but also stronger, but it errors sometimes. I don't think there's an objective "better", but maybe we should catch the PariError and tell the user to try Sage instead (though 99.999% Sage will just return [])

@user202729
Copy link
Contributor

If sage is likely to return [] anyway it seems more reasonable to just set the default to pari then.

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

Successfully merging this pull request may close these issues.

2 participants