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: Remove MUL precompiles #8945

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chfast
Copy link
Member

@chfast chfast commented Oct 9, 2024

This removes BLS12_G1MUL and BLS12_G2MUL precompiles because they are trivially replaceable by corresponding MSM precompiles.

This reduces the number of precompile's addresses defined in this EIP from 9 to 7. The addresses of remaining 7 precompiles are changed to be continues.

The Rationale entry describes why this change make sense. Additionally, the cost of MSM for single input (k==1) has been corrected to match the original MUL cost. The specification now suggests how this case should be implemented.
Morever, because of the ABI compatibility between MUL and MSM all existing tests for MULs can be easily converted to tests for MSMs.

The PoC of MUL and MSM precompiles equivalence is provided in evmone PR#1042.

The adjustment of the MSM implementation for single input in evmone PR#1046.

@chfast chfast requested a review from eth-bot as a code owner October 9, 2024 13:12
@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Oct 9, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 9, 2024

File EIPS/eip-2537.md

Requires 1 more reviewers from @asanso, @ineffectualproperty, @ralexstokes, @shamatar

@eth-bot eth-bot added the a-review Waiting on author to review label Oct 9, 2024
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

One comment, some line got deleted which likely should not be deleted

EIPS/eip-2537.md Show resolved Hide resolved
This removes `BLS12_G1MUL` and `BLS12_G2MUL` precompiles because
they are trivially replaceable by corresponding MSM precompiles.

This reduces the number of precompile's addresses defined in this EIP
from 9 to 7. The addresses of remaining 7 precompiles are changed
to be continues.

The Rationale entry describes why this change make sense.
Additionally, the cost of MSM for single input (`k==1`) has been
corrected to match the original MUL cost. The specification now
suggests how this case should be implemented.
Morever, because of the ABI compatibility between MUL and MSM
all existing tests for MULs can be easily converted to tests for MSMs.

The PoC of MUL and MSM precompiles equivalence is provided in
[evmone PR#1042](ethereum/evmone#1042).
@ralexstokes
Copy link
Member

I think we have historically preferred to not overload precompiles like this, but I'd love to hear what others think

Copy link
Contributor

@cygnusv cygnusv left a comment

Choose a reason for hiding this comment

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

seven

| BLS12_G2MSM | 0x0e | precompile address |
| BLS12_PAIRING_CHECK | 0x0f | precompile address |
| BLS12_MAP_FP_TO_G1 | 0x10 | precompile address |
| BLS12_MAP_FP2_TO_G2 | 0x11 | precompile address |

If `block.timestamp >= FORK_TIMESTAMP` we introduce *nine* separate precompiles to perform the following operations:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If `block.timestamp >= FORK_TIMESTAMP` we introduce *nine* separate precompiles to perform the following operations:
If `block.timestamp >= FORK_TIMESTAMP` we introduce *seven* separate precompiles to perform the following operations:

@asanso
Copy link
Contributor

asanso commented Oct 12, 2024

not sure as well, it's kind of matter of taste here. At this point we could even remove BLS12_G1ADD and BLS12_G2ADD because they are trivially replaceable by corresponding MSM precompiles......

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.

6 participants