Opened 5 years ago

Closed 5 years ago

#22986 closed enhancement (fixed)

Refactor SBox code (move from crypto.mq to crypto)

Reported by: Friedrich Wiemer Owned by: Friedrich Wiemer
Priority: major Milestone: sage-8.0
Component: cryptography Keywords: sbox, crypto
Cc: Merged in:
Authors: Friedrich Wiemer Reviewers: Martin Albrecht, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: f3e3c8b (Commits, GitHub, GitLab) Commit: f3e3c8bd016f2c03bd852ad8ec2d6dc6df99290e
Dependencies: Stopgaps:

Status badges

Description (last modified by Friedrich Wiemer)

As discussed in the google groups (https://groups.google.com/forum/#!topic/sage-devel/9389hm5VbBQ) and in issue 20336 (https://trac.sagemath.org/ticket/20336#comment:10), the code for SBoxes should be moved out of crypto.mq (because it is now much more generell than the code under mq).

Change History (31)

comment:1 Changed 5 years ago by Friedrich Wiemer

Authors: Friedrich Wiemer
Description: modified (diff)
Keywords: sbox added
Owner: set to Friedrich Wiemer

comment:2 Changed 5 years ago by Friedrich Wiemer

Component: PLEASE CHANGEcryptography
Keywords: crypto added
Type: PLEASE CHANGEenhancement

comment:3 Changed 5 years ago by Friedrich Wiemer

Branch: u/asante/refactor_sbox_code__move_from_crypto_mq_to_crypto_

comment:4 Changed 5 years ago by Friedrich Wiemer

Commit: 4f4fc59ec9d10cac01fab8cd9c3ad0927c3700bd
Status: newneeds_review

New commits:

041409erefactored SBox code by moving to crypto package
4f4fc59updated documentation on SBoxes to reflect new path

comment:5 Changed 5 years ago by Martin Albrecht

Reviewers: Martin Albrecht

Looks good, except for two caveats:

  • This adds SBox to the global namespace. I'm not sure this will be widely appreciated so I'd ask on [sage-devel] if there are any objects. Apologies if I simply missed this having been done already.
  • Why import all in __init__.py?

comment:6 Changed 5 years ago by Friedrich Wiemer

Regarding the import all: I was following this part of the docu files-and-directory-structure, in particular:

If you want to create a new directory in the Sage library SAGE_ROOT/src/sage (say, measure_theory), that directory should contain a file init.py that contains the single line import all in addition to whatever files you want to add (say, borel_measure.py and banach_tarski.py), and also a file all.py listing imports from that directory that are important enough to be in the Sage’s global namespace at startup. The file all.py might look like this:

But as the docu also says something about lazy_import, which is not used here, I'm confused, whats the correct way to do here.

comment:7 Changed 5 years ago by Martin Albrecht

Makes sense, thanks

comment:8 Changed 5 years ago by Dima Pasechnik

IMHO, it should not get into the global namespace.

perhaps for all crypto things a crypto catalog should be itroduced, and this can go there then.

I also would suggest that it is lazy_import'ed, too.

comment:9 Changed 5 years ago by git

Commit: 4f4fc59ec9d10cac01fab8cd9c3ad0927c3700bdb6fc04a0259f872c896475b00d27c29a1af6cee5

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

b6fc04aremoved SBox from sage.crypto.all and use lazy imports

comment:10 Changed 5 years ago by Travis Scrimshaw

Status: needs_reviewneeds_work

Needs a rebase on the latest develop.

comment:11 Changed 5 years ago by git

Commit: b6fc04a0259f872c896475b00d27c29a1af6cee5e42b8600054fcbfc725a6177cf26f3666969bf69

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

e42b860Merge branch 'develop' into t/22986/refactor_sbox_code__move_from_crypto_mq_to_crypto_

comment:12 Changed 5 years ago by Friedrich Wiemer

Status: needs_workneeds_review

Merged develop branch

comment:13 Changed 5 years ago by Travis Scrimshaw

In general, we do not do imports in __init__.py; mostly they are blank files that are Python boilerplate (at least with how we use them). I believe this has no effect.

Although AFAIK, your changes do not make anything worse for the global namespace, but having the lazy imports are good for startup time. You can do multiple imports by, e.g.,

lazy_import('sage.crypto.classical', ['AffineCryptosystem', 'HillCryptosystem'])

Once that is changed (as I think it is cleaner code), then I am willing to set a positive review.

comment:14 Changed 5 years ago by git

Commit: e42b8600054fcbfc725a6177cf26f3666969bf6987ea9ff1253bac8c7bd11e7a7c6f33e141bf7882

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

87ea9ffchanged absolute imports to lazy imports

comment:15 Changed 5 years ago by Friedrich Wiemer

tscrim: I changed the imports accordingly. Regarding the __init__.py, this means there should also be no import all? IDK how this global namespace is generated and if this import is necessary for this. If you vote for removing the crypto stuff from the global namespace or this is not needed for this, I can also remove these imports.

comment:16 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

Just commenting on this aspect: the import all in __init__.py should not be needed. I consider it a regression.

comment:17 Changed 5 years ago by git

Commit: 87ea9ff1253bac8c7bd11e7a7c6f33e141bf788266caf7f89d244e7f6253b05e3c484deb38230461

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

66caf7fremove from __init__.py

comment:18 Changed 5 years ago by Friedrich Wiemer

Status: needs_workneeds_review

Removed import all from all __init__.py's

comment:19 Changed 5 years ago by Travis Scrimshaw

Reviewers: Martin AlbrechtMartin Albrecht, Travis Scrimshaw
Status: needs_reviewpositive_review

I would say removing things from the global namespace is a more serious issue that deserves its own ticket as you have to deal with deprecations and creating the catalog (or whatever approach we end up doing, but a catalog seems natural to me). So now that you've taken care of the __init__, positive review.

comment:20 Changed 5 years ago by Volker Braun

Status: positive_reviewneeds_work

Tests fail

comment:21 Changed 5 years ago by git

Commit: 66caf7f89d244e7f6253b05e3c484deb382304611e0f09a3b131f4840cad36257ad70736961060a5

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

bc66479fixed doctests
1e0f09aregistered unpickle for old mq.SBox

comment:22 Changed 5 years ago by Friedrich Wiemer

Status: needs_workneeds_review

The SBox related tests now work -- sorry for not checking them :/

There are still errors:

----------------------------------------------------------------------
sage -t --warn-long 58.4 src/sage/repl/interpreter.py  # 3 doctests failed
sage -t --warn-long 58.4 src/sage/repl/ipython_tests.py  # 4 doctests failed
sage -t --warn-long 58.4 src/sage/repl/interface_magic.py  # 3 doctests failed
----------------------------------------------------------------------

but I think these are not due to my changes to the SBox module.. Is this correct?

comment:23 Changed 5 years ago by Travis Scrimshaw

Status: needs_reviewneeds_work

I've seen these pop up elsewhere. <edit>"These" being the doctest failures in comment:22.</edit>

Last two things. Stupid thing first:

@@ -1485,7 +1531,9 @@ class SBox(SageObject):
 
         EXAMPLES::
 
-            sage: S = mq.SBox([x**254 for x in sorted(GF(2**8))])
+            sage: from sage.crypto.sbox import SBox
+            sage: from sage.crypto.sbox import SBox
+            sage: S = SBox([x**254 for x in sorted(GF(2**8))])
             sage: S.is_involution()
             True
         """

Second thing is that you should deprecate mq.SBox, where you tell people to explicitly import SBox. Sorry I didn't catch this before!

Last edited 5 years ago by Travis Scrimshaw (previous) (diff)

comment:24 Changed 5 years ago by Dima Pasechnik

surely you do not need 2 lines like this in the previous comment, 1 is enough.

+            sage: from sage.crypto.sbox import SBox
+            sage: from sage.crypto.sbox import SBox

comment:25 Changed 5 years ago by git

Commit: 1e0f09a3b131f4840cad36257ad70736961060a5324f79c2c9c4484be5b843117aa02d0a75b19512

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

1c74908removed duplicate import sbox in example doc string
324f79cdeprecate sage.crypto.mq.sbox

comment:26 Changed 5 years ago by Friedrich Wiemer

Status: needs_workneeds_review

I removed the duplicate line in the doc string. @dimpase are you refering to the same line as Travis?

For the deprecation, I added deprecated_function_alias in sage.crypto.mq.sbox, which should be the correct way?

comment:27 in reply to:  26 ; Changed 5 years ago by Travis Scrimshaw

Replying to asante:

For the deprecation, I added deprecated_function_alias in sage.crypto.mq.sbox, which should be the correct way?

Not quite. There's actually a deprecation mechanism in lazy_import. See, e.g., misc/arith.py or rings/all.py.

comment:28 Changed 5 years ago by git

Commit: 324f79c2c9c4484be5b843117aa02d0a75b19512f3e3c8bd016f2c03bd852ad8ec2d6dc6df99290e

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

f3e3c8buse deprecation mechanism from lazy_import

comment:29 in reply to:  27 Changed 5 years ago by Friedrich Wiemer

Replying to tscrim:

Not quite. There's actually a deprecation mechanism in lazy_import. See, e.g., misc/arith.py or rings/all.py.

Thanks for the pointer, I adjusted the deprecation.

comment:30 Changed 5 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

I am pretty sure Dima's comment was an expansion of my comment. Everything else seems to be in order. Positive review.

comment:31 Changed 5 years ago by Volker Braun

Branch: u/asante/refactor_sbox_code__move_from_crypto_mq_to_crypto_f3e3c8bd016f2c03bd852ad8ec2d6dc6df99290e
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.