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

Precompiled contracts for addition and scalar multiplication on the elliptic curve alt_bn128 #213

Merged
merged 12 commits into from Dec 1, 2017

Conversation

Projects
None yet
@chriseth
Contributor

chriseth commented Feb 13, 2017

Precompiled contracts for elliptic curve operations are required in order to perform zkSNARK verification within the block gas limit.

Replaces #196

Show outdated Hide outdated EIPS/ecopts.md
Show outdated Hide outdated EIPS/ecopts.md

@pirapira pirapira referenced this pull request Feb 13, 2017

Closed

Byzantium changes #229

12 of 12 tasks complete
Show outdated Hide outdated EIPS/ecopts.md
Show outdated Hide outdated EIPS/ecopts.md
Show outdated Hide outdated EIPS/ecopts.md
Show outdated Hide outdated EIPS/ecopts.md
Show outdated Hide outdated EIPS/ecopts.md

@chriseth chriseth referenced this pull request Feb 22, 2017

Merged

Integrate libsnark #3587

8 of 8 tasks complete
Test cases and scalars larger than field order.
Scalars larger than the field characteristic are allowed because it only makes sense to restrict them to the group order, not the field characteristic and adding the group order as another magic constant here would complicated the specification.
@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 9, 2017

Contributor

TODO: change semantics to throw an exception both for too short and too long input.

Contributor

chriseth commented Mar 9, 2017

TODO: change semantics to throw an exception both for too short and too long input.

@arkpar arkpar referenced this pull request Mar 9, 2017

Closed

Byzantium release #4833

12 of 12 tasks complete
@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast Mar 21, 2017

Contributor

A comment from my perspective. @chriseth and me are in the process of "implementing" this and #212 EIPs. "Implementing" means to integrate the external library: libff which was a part of bigger libsnark just a month ago. The libff supports only Linux at the moment and nothing indicates that it is activelly developed. After the split the libff does not have tests properly integrated. It also depends on GMP what is another issue.

My personal opinion is this piece of code if to be integrated to any Ethereum client needs full code review and good set of unit tests.

I don't know why alt_bn128 curve is so special, but if we plan to stick with only this one for a long time, we can modify the code base of libff to be less generic and support only this single curve. Such fork can be joint effort of multiple teams if others are interested.

I wander what other libraries do you use. And what is used by ZCash?

Contributor

chfast commented Mar 21, 2017

A comment from my perspective. @chriseth and me are in the process of "implementing" this and #212 EIPs. "Implementing" means to integrate the external library: libff which was a part of bigger libsnark just a month ago. The libff supports only Linux at the moment and nothing indicates that it is activelly developed. After the split the libff does not have tests properly integrated. It also depends on GMP what is another issue.

My personal opinion is this piece of code if to be integrated to any Ethereum client needs full code review and good set of unit tests.

I don't know why alt_bn128 curve is so special, but if we plan to stick with only this one for a long time, we can modify the code base of libff to be less generic and support only this single curve. Such fork can be joint effort of multiple teams if others are interested.

I wander what other libraries do you use. And what is used by ZCash?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 21, 2017

Contributor

@chfast at least in the latest version, zcash uses libsnark (or some fork of it). Note that geth will use a different library, I think it might be this one here: https://github.com/golang/crypto/tree/master/bn256 and there is yet another implementation in rust: https://github.com/zcash/bn

So I think this is a C++-only discussion. We might take libff and swap out GMP for something else.

Contributor

chriseth commented Mar 21, 2017

@chfast at least in the latest version, zcash uses libsnark (or some fork of it). Note that geth will use a different library, I think it might be this one here: https://github.com/golang/crypto/tree/master/bn256 and there is yet another implementation in rust: https://github.com/zcash/bn

So I think this is a C++-only discussion. We might take libff and swap out GMP for something else.

Show outdated Hide outdated EIPS/ecopts.md
Show outdated Hide outdated EIPS/ecopts.md
Show outdated Hide outdated EIPS/ecopts.md
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Sep 8, 2017

Member

I am 100% too late to the party with this, but it could be actually useful to have the inputs ABI encoded and reside on the same address for this reason: in WASM this precompile can be implemented natively as a contract, but with the current API it would be present in two copies.

This of course assumes WASM would come to life in a reasonable timeframe, otherwise the current API is more useful from the implementation perspective.

From in-contract usage perspective having it ABI encoded would enable using them without any complicated bindings, a simple interface contract would be enough in Solidity without the need of touching assembly.

Member

axic commented Sep 8, 2017

I am 100% too late to the party with this, but it could be actually useful to have the inputs ABI encoded and reside on the same address for this reason: in WASM this precompile can be implemented natively as a contract, but with the current API it would be present in two copies.

This of course assumes WASM would come to life in a reasonable timeframe, otherwise the current API is more useful from the implementation perspective.

From in-contract usage perspective having it ABI encoded would enable using them without any complicated bindings, a simple interface contract would be enough in Solidity without the need of touching assembly.

@pipermerriam pipermerriam referenced this pull request Oct 20, 2017

Merged

Byzantium Fork Rules #123

9 of 9 tasks complete

@pirapira pirapira referenced this pull request Oct 23, 2017

Open

Apply Byzantium changes #459

0 of 10 tasks complete

pirapira added some commits Nov 17, 2017

- Truncated input that results in a valid curve point.
- Points not on curve (but valid otherwise).
- Multiply point with scalar that lies between the order of the group and the field (should succeed).
- Multiply point with scalar that is larger than the field order (should succeed).

This comment has been minimized.

@cdetrio

cdetrio Nov 17, 2017

Member
@cdetrio

cdetrio Nov 17, 2017

Member

This comment has been minimized.

@pirapira

pirapira Dec 1, 2017

Member

@cdetrio do you think a link to the test filler should be added to the EIP?

@pirapira

pirapira Dec 1, 2017

Member

@cdetrio do you think a link to the test filler should be added to the EIP?

This comment has been minimized.

@cdetrio

cdetrio Dec 1, 2017

Member

@pirapira I was intending to link the test cases. But on second thought, we don't do that for other EIPs so I guess having the links in the PR/discussion is enough.

@cdetrio

cdetrio Dec 1, 2017

Member

@pirapira I was intending to link the test cases. But on second thought, we don't do that for other EIPs so I guess having the links in the PR/discussion is enough.

- Both contracts should succeed on empty input.
- Truncated input that results in a valid curve point.
- Points not on curve (but valid otherwise).
- Multiply point with scalar that lies between the order of the group and the field (should succeed).

This comment has been minimized.

@cdetrio

cdetrio Nov 17, 2017

Member
  • Multiply point with scalar that lies between the order of the group and the field (should succeed).
    • YES
    • order of the group: curve_order = 21888242871839275222246405745257275088548364400416034343698204186575808495617 (in hex: 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001)
    • order of the field (same as modulus of the field): field_modulus = 21888242871839275222246405745257275088696311157297823662689037894645226208583 (in hex: 0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47)
    • https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/stZeroKnowledge/pointMulAdd2Filler.json#L692-L693
      • uses ECMUL with scalars -1 mod field_order and -2 mod field_order (0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd46 and 0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd45)
@cdetrio

cdetrio Nov 17, 2017

Member
  • Multiply point with scalar that lies between the order of the group and the field (should succeed).
    • YES
    • order of the group: curve_order = 21888242871839275222246405745257275088548364400416034343698204186575808495617 (in hex: 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001)
    • order of the field (same as modulus of the field): field_modulus = 21888242871839275222246405745257275088696311157297823662689037894645226208583 (in hex: 0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47)
    • https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/stZeroKnowledge/pointMulAdd2Filler.json#L692-L693
      • uses ECMUL with scalars -1 mod field_order and -2 mod field_order (0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd46 and 0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd45)
- Curve points which would be valid if the numbers were taken mod p (should fail).
- Both contracts should succeed on empty input.
- Truncated input that results in a valid curve point.
- Points not on curve (but valid otherwise).
- Curve points which would be valid if the numbers were taken mod p (should fail).
- Both contracts should succeed on empty input.
- Truncated input that results in a valid curve point.

This comment has been minimized.

@cdetrio

cdetrio Nov 17, 2017

Member
@cdetrio

cdetrio Nov 17, 2017

Member

This comment has been minimized.

@pirapira

pirapira Nov 20, 2017

Member

Added such cases in the test google spreadsheet

@pirapira

pirapira Nov 20, 2017

Member

Added such cases in the test google spreadsheet

Inputs to test:
- Curve points which would be valid if the numbers were taken mod p (should fail).
- Both contracts should succeed on empty input.

pirapira added some commits Dec 1, 2017

@pirapira

Looks good to me.

@pirapira pirapira merged commit ac33ce6 into master Dec 1, 2017

@pirapira pirapira deleted the ecopts branch Dec 1, 2017

@shamatar

This comment has been minimized.

Show comment
Hide comment
@shamatar

shamatar May 11, 2018

Hello @pirapira

As important optimizations were merged in 1.8.x and actual processor time for every operation (especially MUL) was significantly reduced, what are the plans to reduce a precompile costs?

Sincerely, Alex

shamatar commented May 11, 2018

Hello @pirapira

As important optimizations were merged in 1.8.x and actual processor time for every operation (especially MUL) was significantly reduced, what are the plans to reduce a precompile costs?

Sincerely, Alex

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira May 14, 2018

Member

@shamatar thanks for the news. Many people would be happier with the lower gas amounts, so my suggestion is to file an EIP about the newer gas costs (if there is none yet).

Member

pirapira commented May 14, 2018

@shamatar thanks for the news. Many people would be happier with the lower gas amounts, so my suggestion is to file an EIP about the newer gas costs (if there is none yet).

@Shadowfiend

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend May 17, 2018

Contributor

I've opened #1088 to propose this, as I didn't see any other EIPs on the matter.

Contributor

Shadowfiend commented May 17, 2018

I've opened #1088 to propose this, as I didn't see any other EIPs on the matter.

@aerth aerth referenced this pull request Jun 5, 2018

Merged

HF7 @ 36050 #24

@mratsim mratsim referenced this pull request Aug 29, 2018

Open

Implement all precompiled contracts #122

0 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment