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: rename PAIRING to PAIRING_CHECK; introduce PAIRING_PRODUCT precomiple #8309

Merged
merged 5 commits into from
Jun 19, 2024

Conversation

zhenfeizhang
Copy link
Contributor

@zhenfeizhang zhenfeizhang commented Mar 13, 2024

this PR does the following

  • rename BLS12_PAIRING to BLS12_PAIRING_CHECK.

The function does not return the actual pairing result but instead checks if the result is identity or not. This renaming reflects the actual behavior. EIP for BN256 also used pairing check terminology.

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

eth-bot commented Mar 13, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Mar 13, 2024
@eth-bot eth-bot changed the title Update eip-2537: rename PAIRING to PAIRING_CHECK; introduce PAIRING_PRODUCT precomiple Update EIP-2537: rename PAIRING to PAIRING_CHECK; introduce PAIRING_PRODUCT precomiple Mar 13, 2024
EIPS/eip-2537.md Outdated Show resolved Hide resolved
@ralexstokes
Copy link
Member

@zhenfeizhang thanks for this! I'm open to updating the name from "pairing" to "pairing check"

I intend to get some feedback on tomorrow's ACDE and the next roll call around the necessity for the "pairing result" precompile

so until then let's just let this sit and see what we decide to do

@ralexstokes
Copy link
Member

hi @zhenfeizhang !

we discussed on a recent ACD call and there was some pushback that:

  1. while there are cryptographic applications that could use the pairing product, they are somewhat niche
  2. exposing the product precompile commits us to a particular pairing implementation, while only exposing the "check" would let us upgrade to a better/faster implementation in the future with much less coordination/change
  3. there is some ongoing work on "EVMMAX" which would strike a middle ground for doing arithmetic with arbitrary modulus sizes, which would likely support the use case of doing a pairing in the EVM interpreter like you suggest above

please let me know if you see it differently, but at the moment I'd lean towards keeping the EIP as is, and waiting for something like EVMMAX to land to unlock these other use cases

@Marchhill
Copy link
Contributor

Hey @ralexstokes I have a few responses to your points:

  1. One application which doesn't seem too niche is verification of aggregated SNARKs, these generally require taking the pairing result and performing further arithmetic
  2. If we expose the pairing result the pairing check operation could still easily be upgraded. Additionally if we wanted upgradability for the pairing operation itself, some bits of the input could be used to specify the pairing version to use. Also this means less fragmentation as all contracts use a standard pairing version rather than all reimplementing their own potentially incompatible versions.
  3. EVMMAX would be useful for doing operations in the target group, however this would require pairing to be reimplemented. If we're happy with then you could argue we don't need the precompile in the first place, as we could do everything in the interpreter

I'm in support of changing the name to "pairing check"

@asanso
Copy link
Contributor

asanso commented Apr 9, 2024

I'm in support of changing the name to "pairing check"

+1

@mratsim
Copy link
Contributor

mratsim commented Apr 20, 2024

exposing the product precompile commits us to a particular pairing implementation,

To clarify, it would require us to settle on a specific ABI for Fp12/GT, and would be useless without Fp12/GT multiplication at minimum and ideally endomorphism accelerated GT exponentiation as well.

If this is the way we want to go, the ABI encoding should be canonical and should not be the default dump from Gnark and Kilic which are dependent on the Fp2 -> Fp6 -> Fp12 towering, see supranational/blst#101 (comment)

@ralexstokes
Copy link
Member

given that it seems it would require additional standardization work to settle on something for the pairing product (see prior comment from mratsim), I think it best to leave that for future work.

moving the name to PAIRING_CHECK makes sense -- @zhenfeizhang do you mind updating this PR w/ just that change? then im happy to merge

@zhenfeizhang
Copy link
Contributor Author

given that it seems it would require additional standardization work to settle on something for the pairing product (see prior comment from mratsim), I think it best to leave that for future work.

moving the name to PAIRING_CHECK makes sense -- @zhenfeizhang do you mind updating this PR w/ just that change? then im happy to merge

Updated as proposed.

Copy link

github-actions bot commented Jun 9, 2024

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Jun 9, 2024
@mratsim
Copy link
Contributor

mratsim commented Jun 9, 2024

not stale. cc @asanso @ralexstokes for review/merge

@github-actions github-actions bot removed the w-stale Waiting on activity label Jun 10, 2024
@ralexstokes
Copy link
Member

@zhenfeizhang thanks for updating this PR and sorry for the latency :)

if you have a few minutes to update the original comment above to remove the bits about the pairing product, that would be helpful as we reference this in the future, thanks!

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.

lgtm thanks!

@zhenfeizhang
Copy link
Contributor Author

@zhenfeizhang thanks for updating this PR and sorry for the latency :)

if you have a few minutes to update the original comment above to remove the bits about the pairing product, that would be helpful as we reference this in the future, thanks!

@ralexstokes done. Repost the removed section here for future reference.

===============================

  • introduce the BLS12_PAIRING_PRODUCT precompile.

In certain cases there may be a need to compute the actual pairing product, instead of checking the product is 1 or not. Without precompile, we cannot compute a pairing product in solidity. For future proof, I suggest to add this precompile.

Note that for BN256, such a precompile does not exist, but the developers can still code this function in solidity (and pay a high gas fee) as the base field fits in U256 and there exists precompiles for MULMOD. This is almost impossible for BLS12-381. Here the base field is of 381 bits. If one were to implement pairing product, they have to implement the base field operation first, and then implement the actual pairing operation. This base field operation is already very expensive as we don't have MULMOD for 381 bits objects. The overall gas cost will be prohibitively high.

@SamWilsn
Copy link
Contributor

@zhenfeizhang you'll need to resolve the conflicts in this pull request before this can be merged.

@zhenfeizhang
Copy link
Contributor Author

@zhenfeizhang you'll need to resolve the conflicts in this pull request before this can be merged.

done

@eth-bot eth-bot enabled auto-merge (squash) June 19, 2024 19:12
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...

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 9ccf12c into ethereum:master Jun 19, 2024
12 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.

7 participants