-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Sage script for constants calculation for banderwagon and bandersnatch #369
Open
rupam-04
wants to merge
20
commits into
mratsim:master
Choose a base branch
from
rupam-04:issue-359
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… bandersnatch in sage
For arrays, you can get inspiration from my lattice pretty printer here: constantine/sage/derive_endomorphisms.sage Lines 45 to 51 in 976c8bb
which generates this: constantine/constantine/math/constants/bls12_381_endomorphisms.nim Lines 21 to 27 in 976c8bb
or my Frobenius constants generator constantine/sage/derive_frobenius.sage Lines 139 to 149 in 976c8bb
which generates this constantine/constantine/math/constants/bls12_381_frobenius.nim Lines 51 to 129 in 976c8bb
After the Dyadic roots, we also need: the Precomputed Blocks and the table: constantine/constantine/math/constants/banderwagon_sqrt.nim Lines 210 to 1507 in 976c8bb
|
I tried using a dictionary instead of an array. That shouldn't be a problem right? |
advaita-saha
reviewed
May 3, 2024
advaita-saha
reviewed
May 3, 2024
…locks``` Co-authored-by: Advaita Saha <[email protected]>
advaita-saha
reviewed
May 3, 2024
advaita-saha
requested changes
May 3, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation for LUT is wrong
advaita-saha
requested changes
May 3, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please match your output with expected results. It's not matching
Using a table is fine at the moment for proof-of-concept purposes. However, they cannot be constant-time and so will need to be changed later: #358 |
I thought the sage script is just for generating the constants. So is it necessary to convert the sage code to constant time? |
advaita-saha
requested changes
May 4, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in terms of accuracy of the constants generated.
Should this be modularised such that constants for other curves can also be generated by plugging in the curve params ? @mratsim
Co-authored-by: Advaita Saha <[email protected]>
Not yet, but it will need to be refactored for #358.
Yes, this can be done in a subsequent PR, if this one is too big, atm it's only 47 lines but missing all the code that format things correctly so that we can just copy-paste the constants into the Nim codebase. One thing though that I think is important is this:
# - When lb is small enough, this is a matter of recognizing the value z
# among the known 2^lb-th roots of 1, with a "reverse lookup". This can
# be done by using only a small number of bits from the value (see the
# minhc() function in the code below, to find a minimal-sized bit
# pattern that is enough to distinguish all roots). We can thus stop
# the recursion before reaching lb == 1.and the function minhc: https://github.com/pornin/modsqrt/blob/0a9c69a0ce8b4a031e7f4a728a3bd93870088ecf/modsqrt.sage#L535-L559 Ergo, we are using a LUT of window 8 (see also https://hackmd.io/@jsign/bandersnatch-optimized-sqrt-notes#Detailed-comparison): constantine/constantine/math/constants/banderwagon_sqrt.nim Lines 167 to 172 in d7871d7
but we're lucky because 8-bit patterns can uniquely identify the root-of-unity we need. For constant-time implementation because we can't use branches, we have to scan through a whole array that maps bit pattern to root of unities, i.e. no sparse hash-table. And the mapping size grows exponentially with the number of bits, so we likely would want to use 6-bit windows for example if the pattern is small enough to uniquely identify the root of unity we need, that would be 4x less memory consumption. This means we need to check the absence of pattern collisions in our precomputed small windows. |
|
@mratsim things are pretty much completed it seems to me. Are we ready to merge this then? |
advaita-saha
approved these changes
Jul 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
None yet
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #359 .
Implemented the sqrtPrecomp_PrimitiveDyadicRoots in Sage. However I would need some help on defining the ret array in sage as I didn't come across any helpful SageMath docs.