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

Update EIP-2537: Editorial #8274

Merged
merged 8 commits into from
Mar 12, 2024
Merged

Update EIP-2537: Editorial #8274

merged 8 commits into from
Mar 12, 2024

Conversation

khovratovich
Copy link
Contributor

Edited a few sentences that were ambiguous.

Added the Fp, Fp2, G1, G2 definitions.

@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Feb 29, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Feb 29, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Feb 29, 2024
@eth-bot eth-bot changed the title Editorial Update EIP-2537: Editorial Feb 29, 2024
@@ -67,10 +69,10 @@ Twist type: M
B coefficient for twist c0 = 0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004
B coefficient for twist c1 = 0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004
Generators:
G1:
H1:
Copy link
Contributor

Choose a reason for hiding this comment

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

why H and not G?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to resolve ambiguity. Because G1 is used for a group of points.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Feb 29, 2024
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

generally looks good

EIPS/eip-2537.md Outdated Show resolved Hide resolved
@shamatar
Copy link
Contributor

shamatar commented Feb 29, 2024

If clarifications are in the process, then we should double check (as I no longer remember if it was well covered in the original documents, it was too long ago):

  • subgroup checks for non-pairing operations: I think they should be on the caller's responsibility, so implementation should only do "on curve" check
  • number of input-pairs into pairing operation and multiexp operation: empty input must return an error and not zero-point/identity

I will check a the original document, but second confirmation will be great

Note: if we require(!) and not suggest all implementations to do fast subgroup checks, then actually it will be fine to require inputs to be in the main subgroups (G1/G2) and not just "on curve". Otherwise slow subgroup check via multiplication will ruin gas schedule and make additions expensive

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Mar 1, 2024
@khovratovich
Copy link
Contributor Author

  • subgroup checks for non-pairing operations: I think they should be on the caller's responsibility, so implementation should only do "on curve" check
  • number of input-pairs into pairing operation and multiexp operation: empty input must return an error and not zero-point/identity

Both are true in the current document.

Co-authored-by: Alex Stokes <r.alex.stokes@gmail.com>
EIPS/eip-2537.md Outdated
@@ -320,7 +333,8 @@ There are no backward compatibility questions.

### Subgroup checks

A subgroup check **is mandatory** during the pairing call. Implementations *should* use fast subgroup checks: at the time of writing, multiplication gas cost is based on the `double-and-add` multiplication method that has a clear "worst case" (all bits are equal to one). For pairing operations, it is expected that implementations use faster subgroup checks, e.g. by using the wNAF multiplication method for elliptic curves that is ~ `40%` cheaper with windows size equal to 4. (Tested empirically. Savings are due to lower hamming weight of the group order and even lower hamming weight for wNAF. Concretely, subgroup check for both G1 and G2 points in a pair are around `35000` combined).
The subgroup check for point `H` verifies that `H` multiplied by scalar `q` yields the infinity point on the respective curve.
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than use this naive expensive subgroup check (in particular for G2) I'd rather suggest to point to https://eprint.iacr.org/2021/1130.pdf

Copy link
Contributor Author

@khovratovich khovratovich Mar 1, 2024

Choose a reason for hiding this comment

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

Can we provide complexity estimates for this method? What is the value of u?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's all in the paper. 'u' is the seed for generating BLS12-381 and pretty short ( -0xd201000000010000)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you should not require "check by multiplication" only. Whether we should require fast subgroup checks or not is a separate question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reformulated the text with your link @asanso

@asanso
Copy link
Contributor

asanso commented Mar 1, 2024

  • subgroup checks for non-pairing operations: I think they should be on the caller's responsibility, so implementation should only do "on curve" check
  • number of input-pairs into pairing operation and multiexp operation: empty input must return an error and not zero-point/identity

Both are true in the current document.

I agree with @khovratovich that the current situation is ambiguous. Moreover, it is even more convoluted than that, as leaving the responsibility to callers to check the subgroup for non-pairing operations opens the door to subtle issues. For example, in order to expedite scalar multiplication, implementers might be tempted (within their rights) to apply GLV endomorphism optimization. However, the problem arises because the GLV is defined and valid only within the 'magic subgroup'.

@shamatar
Copy link
Contributor

shamatar commented Mar 1, 2024

If no one knows a good reasonable case for explicit wanting to mix-in not-main-subgroup point for non-pairing cases, I can perform some basic gas measurements in case of fast subgroup check. Hopefully costs should not change too much for multiplication and addition since they are dominated by one final inversion

Copy link

github-actions bot commented Mar 1, 2024

The commit 21f396f (as a parent of ba051b3) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Mar 1, 2024
@khovratovich
Copy link
Contributor Author

The CI validator does not like a link to ePrint. Probably we can ignore this error.

@asanso
Copy link
Contributor

asanso commented Mar 4, 2024

@khovratovich IMHO it would be good to separate the really good editorial changes and typos you corrected from the subgroup check discussion. How about slit this PR in 2 so we could merge the editorial changes/typos first ?

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Mar 12, 2024
@eth-bot eth-bot enabled auto-merge (squash) March 12, 2024 15:57
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 6eaa523 into ethereum:master Mar 12, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants