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.

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

Add GLV for simple curves: BW6-767, Grumpkin, secp256k1, secq256k1 #778

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

weikengchen
Copy link
Member

@weikengchen weikengchen commented Feb 13, 2024

Description

This PR adds GLV curves for those simple cases where a = 0 and the curve is a SW curve.

The parameters are generated as follows. First of all, you get beta. Another beta would be beta^2.

beta  = mod(1, q).nth_root(3)

Then, you get lambda. There are two candidates. One can just test it out.

root_of_minus_three = mod(-3, r).sqrt()
mod(-1 + root_of_minus_three, r) / mod(2, r)
# or
mod(-1 - root_of_minus_three, r) / mod(2, r)

Use an algorithm to find out the scalars

def find_basis(a, b): 
    res = []
    x1 = 1
    y1 = 0
    u = a
    x2 = 0
    y2 = 1
    v = b
    res.append((x2, y2, v, v * v > b))
    res.append((x1, y1, u, u * u > b))
    while u != 0:
        q = v // u
        r = v - q * u
        x = x2 - q * x1
        y = y2 - q * y1
        v = u
        u = r
        x2 = x1
        x1 = x
        y2 = y1
        y1 = y
        res.append((x1, y1, u, u * u > b))
    return res

res = find_basis(793479390729215512621379701633421447060886740281060493010456487427281649075476305620758731620350, r)

# find the first l
for i in range(len(res)):
    if res[i][3] == False: 
        l = i - 1
        break
l
a1 = res[l + 1][2]
a2 = -res[l + 1][0]
rtl_size = res[l][2] ^ 2 + res[l][0] ^ 2
rtl2_size = res[l + 2][2] ^ 2 + res[l + 2][0] ^ 2
if rtl_size < rtl2_size:
    a2 = res[l][2]
    b2 = -res[l][0]
else:
    a2 = res[l + 2][2]
    b2 = -res[l + 2][0]

The implementation here follows this Hackmd https://hackmd.io/@drouyang/glv by @drouyang

This PR is related to #718 but does not solve it.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

do not apply:

  • Updated relevant documentation in the code

@weikengchen weikengchen requested review from a team as code owners February 13, 2024 14:00
@weikengchen weikengchen requested review from z-tech, Pratyush and mmagician and removed request for a team February 13, 2024 14:00
@Pratyush
Copy link
Member

Can we reuse standard settings of beta and lambda that are also used by other implementations?

Also, can we add the relevant sage scripts to the curve folders?

@weikengchen
Copy link
Member Author

Let me ask a clarifying question. What do you mean by standard settings?

So, I cross check with BW6-761 and I can reproduce their parameters, although the scalar decomposition is a little bit different---BW6-761's scalar decomposition coeffs are true, false, true, true. I, locally, have false, true, false, false, which would be equivalent but just not sure why BW6-761's coeffs prefer negations.

(If you have any info about why previous implementation tends to prefer negations, or whether we should ping @simonmasson for help...)

For the rest, there is an issue. BLS12-381, BLS12-377, BN254 don't use the standard GLV parameters. They use a special trick that is tailored for them (as you can see, their scalar decomposition is fairly simple). The curves that we are implementing here, however, does not have those special parameters. So the references for those would be a specific paper.

@weikengchen
Copy link
Member Author

Let me organize my script and can include one. My current script is mostly doing a trial-and-error to figure out which lambda should be used, which is clearly not ideal.

Another thing pending for this PR is that I may want to run the benchmark to see the improvement.

@Pratyush
Copy link
Member

@weikengchen I assume that secp256k1 uses particular parameters, especially in libsecp256, right? Similarly, for Grumpkin, other implementations might have a particular choice. Best to be compatible with them. That's all I meant

@weikengchen
Copy link
Member Author

Got it. Let me look around.

By the way, the GLV implementation is already different from the standard GLV. The standard GLV rounds numbers. We floor. Per the original proof, this may imply that, for a 256-bit number, the decomposed scalar could be 129-bit instead of 128-bit.

@weikengchen weikengchen marked this pull request as draft February 17, 2024 17:19
@weikengchen
Copy link
Member Author

allow me to do additional review about the parameters. The problem is that the positive sign and negative sign are crucial, and I think the current test template is insufficient in that it does not detect "correct but not short vector GLV".

@mmagician
Copy link
Member

@weikengchen indeed, the tests are only checking correctness.
I would bench each individual curve with the proposed parameters before making it the default. See e.g. the PR and description here.

@mratsim
Copy link

mratsim commented Jun 30, 2024

@weikengchen I assume that secp256k1 uses particular parameters, especially in libsecp256, right? Similarly, for Grumpkin, other implementations might have a particular choice. Best to be compatible with them. That's all I meant

GLV are purely internal, being compatible helps for debugging but it wouldn't change the result of the scalar multiplication.

Can we reuse standard settings of beta and lambda that are also used by other implementations?

Also, can we add the relevant sage scripts to the curve folders?

Here is my script for deriving GLV on G1 and G2 fo BN254, BLS12-381, BLS12-377, BW6-761 (though hardcoded because too slow in sage).

I use LLL though instead.

https://github.com/mratsim/constantine/blob/dcc93104a9b40746e1233d5c11df4731c516adce/sage/derive_endomorphisms.sage#L61-L67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants