Add sage script for generating scalar_split_lambda constants #852
Conversation
* Move curve parameters to separate file * Rename main prover script for clarity
|
I recommend trying Add/replace the BETA and LAMBDA assertions with an assertion that the value is a root of |
real-or-random
force-pushed
the
202011-sage-split-lambda
branch
2 times, most recently
from
7f2238f to
5b316a8
Compare
I think the current one is more instructive... I don't know, I don't want to spend a lot of time on this. I added root assumptions for BETA and LAMBDA. |
real-or-random
force-pushed
the
202011-sage-split-lambda
branch
from
5b316a8 to
0854198
Compare
|
I'm not really familiar with sage, and I haven't tested this PR, but it is okay by me. |
|
Another check to add is that (It will also be the case that all the assertions would pass if both LAMBDA and BETA were replaced with LAMBDA^2 and BETA^2, but there is no helping that because that would be a valid, but different, implementation.) |
real-or-random
force-pushed
the
202011-sage-split-lambda
branch
from
0854198 to
4e4158e
Compare
|
I addressed all of your comments. |
sage/gen_split_lambda_constants.sage
Outdated
| while True: | ||
| if inf_norm(v2) < inf_norm(v1): | ||
| v1, v2 = v2, v1 | ||
| m = round( (v1[0]*v2[0] + v1[1]*v2[1]) // inf_norm(v1)**2 ) |
There was a problem hiding this comment.
No need to round anymore with //.
There was a problem hiding this comment.
Oh, I thought / vs // is just a python2 compatibility thing but I think what you had in mind is to avoid floating-point arithmetic. But this changed the semantics.
The correct thing is to round (to the nearest int) and not floor (for a reason), so that's why I used / and round instead of //. (It just happens that the output is the same for our specific input.)
So I now changed this to a version which
- rounds to the nearest integer (what the textbooks do TM [1]),
- relies integer arithmetic instead of floating-point arithmetic, and
- should work in Python 2 and Python 3 because it does not use
/.
[1] https://en.wikipedia.org/wiki/Lattice_reduction#In_two_dimensions Ok not a book but you get the point.
There was a problem hiding this comment.
Just saying, you could delete the whole thing and call LLL where all this has been worked out.
|
Looks good, ACK 4e4158e |
real-or-random
force-pushed
the
202011-sage-split-lambda
branch
from
4e4158e to
329a2e0
Compare
|
ACK 329a2e0 CI failure is unrelated. |
…ants Summary: Backport of [[bitcoin-core/secp256k1#852 | secp256k1#852]] Test Plan: sage gen_split_lambda_constants.sage Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9377
…ants Summary: Backport of [[bitcoin-core/secp256k1#852 | secp256k1#852]] Test Plan: sage gen_split_lambda_constants.sage Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9377
cc @roconnor-blockstream