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.

Sign up for GitHub

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

Jump to bottom

sage.crypto: Update # needs, modularization fixes #36106

Merged
merged 15 commits into from
Aug 27, 2023
Merged

sage.crypto: Update # needs, modularization fixes #36106

merged 15 commits into from
Aug 27, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Aug 19, 2023

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe self-assigned this Aug 19, 2023
dcoudert
dcoudert suggested changes Aug 21, 2023
View reviewed changes
src/sage/crypto/classical_cipher.py Outdated
@@ -165,25 +170,23 @@ def __init__(self, parent, key):
EXAMPLES::

sage: S = AlphabeticStrings()
sage: E = HillCryptosystem(S,3)
sage: E
sage: E = HillCryptosystem(S,3); E # needs sage.modules
Copy link
Contributor

Choose a reason for hiding this comment

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

block scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done in 826f72d

src/sage/crypto/lattice.py
- ``'cyclotomic'`` -- Special case of ideal. Allows for
efficient processing proposed by [LM2006]_.

- ``n`` -- Determinant size, primal: `det(L) = q^n`, dual: `det(L) = q^{m-n}`.
For ideal lattices this is also the degree of the quotient polynomial.
- ``m`` -- Lattice dimension, `L \subseteq Z^m`.
Copy link
Contributor

Choose a reason for hiding this comment

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

you inserted empty lines lines 39 and 50 to separate items but not here. Shouldn't it be the same for all items of the list ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done in 2f0f2f6

src/sage/crypto/block_cipher/des.py
@@ -511,7 +512,7 @@ def encrypt(self, plaintext, key):
sage: des.encrypt(P, K56) == C
True
"""
if isinstance(plaintext, (list, tuple, Vector_mod2_dense)):
if isinstance(plaintext, (list, tuple, Vector)):
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure that this change is safe for users ? I'm not sure it's currently tested...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this just distinguishes scalar input from list/vector input. In fact, when neither of the two conditions holds, inputType remains uninitialized, leading to an error in the last line of the function

@dcoudert
Copy link
Contributor

could you fix the following warnings:

sage -t --long --warn-long 33.3 --random-seed=250269248297457194959005008327880188134 src/sage/crypto/public_key/blum_goldwasser.py
**********************************************************************
File "src/sage/crypto/public_key/blum_goldwasser.py", line 133, in sage.crypto.public_key.blum_goldwasser.BlumGoldwasser
Warning: Variable 'q' referenced here was set only in doctest marked '# needs sage.combinat sage.symbolic'
    gcd(p, q) == a*p + b*q == 1
**********************************************************************
File "src/sage/crypto/public_key/blum_goldwasser.py", line 133, in sage.crypto.public_key.blum_goldwasser.BlumGoldwasser
Warning: Variable 'p' referenced here was set only in doctest marked '# needs sage.combinat sage.symbolic'
    gcd(p, q) == a*p + b*q == 1
**********************************************************************
File "src/sage/crypto/public_key/blum_goldwasser.py", line 133, in sage.crypto.public_key.blum_goldwasser.BlumGoldwasser
Warning: Variable 'a' referenced here was set only in doctest marked '# needs sage.combinat sage.symbolic'
    gcd(p, q) == a*p + b*q == 1
    [125 tests, 0.07 s]
sage -t --long --warn-long 33.3 --random-seed=250269248297457194959005008327880188134 src/sage/crypto/mq/sr.py
**********************************************************************
File "src/sage/crypto/mq/sr.py", line 2073, in sage.crypto.mq.sr.SR_generic.polynomial_system
Warning: Variable 's' referenced here was set only in doctest marked '# needs sage.modules sage.rings.finite_rings sage.rings.polynomial.pbori'
    F.subs(s).groebner_basis() # long time
**********************************************************************
File "src/sage/crypto/mq/sr.py", line 2073, in sage.crypto.mq.sr.SR_generic.polynomial_system
Warning: Variable 'F' referenced here was set only in doctest marked '# needs sage.modules sage.rings.finite_rings sage.rings.polynomial.pbori'
    F.subs(s).groebner_basis() # long time
    [368 tests, 10.43 s]

We have also several compilation warnings that should be fixed in the future

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 21, 2023

done in 37a6b46

dcoudert
dcoudert approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 21, 2023

Thank you!

@github-actions
Copy link

Documentation preview for this PR (built with commit 37a6b46; changes) is ready! 🎉

@vbraun vbraun merged commit 300fd19 into sagemath:develop Aug 27, 2023
11 of 12 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoudert dcoudert dcoudert approved these changes

Assignees

@mkoeppe mkoeppe

Labels
c: cryptography t: refactoring
Projects
None yet
Milestone
sage-10.2
Development

Successfully merging this pull request may close these issues.
3 participants
@mkoeppe @dcoudert @vbraun