Skip to content

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

Merged
vbraun merged 15 commits intosagemath:developfrom
mkoeppe:needs_for_sage_crypto
Aug 27, 2023
Merged

sage.crypto: Update # needs, modularization fixes#36106
vbraun merged 15 commits intosagemath:developfrom
mkoeppe:needs_for_sage_crypto

Conversation

@mkoeppe
Copy link
Copy Markdown
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

dcoudert
dcoudert requested changes Aug 21, 2023
View reviewed changes
src/sage/crypto/classical_cipher.py Outdated
sage: S = AlphabeticStrings()
sage: E = HillCryptosystem(S,3)
sage: E
sage: E = HillCryptosystem(S,3); E # needs sage.modules
Copy link
Copy Markdown
Collaborator

@dcoudert dcoudert Aug 21, 2023

Choose a reason for hiding this comment

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

block scope

Copy link
Copy Markdown
Contributor Author

@mkoeppe mkoeppe Aug 21, 2023

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

- ``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
Copy Markdown
Collaborator

@dcoudert dcoudert Aug 21, 2023

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
Copy Markdown
Contributor Author

@mkoeppe mkoeppe Aug 21, 2023

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
True
"""
if isinstance(plaintext, (list, tuple, Vector_mod2_dense)):
if isinstance(plaintext, (list, tuple, Vector)):
Copy link
Copy Markdown
Collaborator

@dcoudert dcoudert Aug 21, 2023

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
Copy Markdown
Contributor Author

@mkoeppe mkoeppe Aug 21, 2023

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
Copy Markdown
Collaborator

dcoudert commented Aug 21, 2023

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
Copy Markdown
Contributor Author

mkoeppe commented Aug 21, 2023

done in 37a6b46

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

@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
Copy Markdown
Contributor Author

mkoeppe commented Aug 21, 2023

Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 21, 2023

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

@vbraun vbraun merged commit 300fd19 into sagemath:develop 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