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-665: Move to Draft #7515

Closed
wants to merge 12 commits into from

Conversation

rdubois-crypto
Copy link

Removing incorrect statement (recovery is possible on schnorr) .

Provide benching argument using openssl.
Adding extra motivations related to MPC and WebAuthn
Adding an extra precompile for zk versions ED255MMA
Adding solidity implementation reference for ED225MMA (mulmuladd)

Removing incorrect statement (recovery is possible on schnorr)
Adding extra motivations related to MPC and WebAuthn
Adding an extra precompile for zk versions
Adding solidity implementation reference
@github-actions github-actions bot added c-status Changes a proposal's status s-draft This EIP is a Draft t-core labels Aug 23, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 23, 2023

File EIPS/eip-665.md

Requires 1 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @Pandapip1, @SamWilsn

@eth-bot eth-bot changed the title update EIP665 Update EIP-665: Move to Draft Aug 23, 2023
@eth-bot eth-bot added the e-review Waiting on editor to review label Aug 23, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Aug 23, 2023
@g11tech
Copy link
Contributor

g11tech commented Aug 23, 2023

Some linter validations need addressing else pr looks fine

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Aug 24, 2023
EIPS/eip-665.md Outdated
@@ -20,34 +19,59 @@ The addition of a native compiled function, in a precompiled contract, to the EV

## Motivation

Ed25519 and Ed448 (that is, EdDSA using Curve25519 or Curve448) are IETF recommendations ([RFC7748](https://tools.ietf.org/html/rfc7748)) with some attractive properties:
Ed25519 and Ed448 (that is, EdDSA using Curve25519 or Curve448) are IETF recommendations ([RFC7748]`https://tools.ietf.org/html/rfc7748`) with some attractive properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

links should be in link formats (everywwhere in document)

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Aug 25, 2023
@github-actions
Copy link

The commit 9c7b936 (as a parent of 1509652) contains errors.
Please inspect the Run Summary for details.

@g11tech
Copy link
Contributor

g11tech commented Aug 28, 2023

@oberstet could you review this PR pls

@oberstet
Copy link
Contributor

@g11tech why should I review this, the EIP is dead? what's the status of the EIP among core developers? is there a new motivation to add new precompiles for ed25519? why is ED25519MMA needed for ed25519 as it seems unneeded as my old PRs show?

@oberstet
Copy link
Contributor

Adding an extra precompile for zk versions ED255MMA

ah, ok

fwiw, I think adding yet another precompile and piggybacking that on this PR will make it even more unlikely that it ever will be accepted

Adding solidity implementation reference for ED225MMA (mulmuladd)

nice!

do you have PRs for all the projects for which I did had working PRs adding the new precompile (or the 2 in this case now with zk support)?

do you have test cases that show that all implementation behave exactly the same wrt to all newly added precompiles?

@oberstet
Copy link
Contributor

why did you remove the discussion/references to HACL https://github.com/hacl-star/hacl-star - the only formally verified implementation of ed25519 crypto primitives I am aware of. which is in contrast to libsodium, which practically might still be preferred, but isn't formally verified

@rdubois-crypto
Copy link
Author

rdubois-crypto commented Aug 29, 2023

do you have PRs for all the projects for which I did had working PRs adding the new precompile (or the 2 in this case now with zk support)?

do you have test cases that show that all implementation behave exactly the same wrt to all newly added precompiles?

Actually the precompile is a subsequence of the whole EDDSA signature, to resume EDDSA computations are:
S.B-H(R||A||M).A == R, where B and A are points. The expansive part lies in computing this sequence of two elliptic scalar multiplication.
So MMA needs to extract the value s and H(R||A||M) as input scalars and A and B as input points.

The code doesn't need to be touched, only extracting the intermediate values for test vectors.

@rdubois-crypto
Copy link
Author

rdubois-crypto commented Aug 29, 2023

fwiw, I think adding yet another precompile and piggybacking that on this PR will make it even more unlikely that it ever will be accepted

That's sad. At first i planned to push only this EDMMA precompile, before realizing EIP-665 existed. I think that adding one more Ed25519 precompile would add more confusion as well. There are two expansive part in EDDSA:

  • sha512, which is not available in solidity
  • algebraic operations over non native curve
    I did not find an implementation for SHA512, but having a quick estimation (64 iterations of less than 40 operations), it shall be implemented in less than 30K gas, which is low compared to the elliptic computations.

Regarding this what do you think should be the political choice ?

  • adding EDMMA in a separate EIP
  • adding EDMMA here ?
  • keeping EDMMA instead of EDVERIFY ?

@rdubois-crypto
Copy link
Author

why did you remove the discussion/references to HACL https://github.com/hacl-star/hacl-star - the only formally verified implementation of ed25519 crypto primitives I am aware of. which is in contrast to libsodium, which practically might still be preferred, but isn't formally verified

EIP politic regarding external link has changed and every single link was rejected by the EIP-Walidator. References should be kept minimal. The argument is right and HACL shall be kept and libsodium removed then.

@rdubois-crypto
Copy link
Author

Maybe the discussion could be held here:
https://ethereum-magicians.org/t/eip-665-ed25519-signature-verification/15535

@g11tech
Copy link
Contributor

g11tech commented Aug 29, 2023

@g11tech why should I review this, the EIP is dead? what's the status of the EIP among core developers? is there a new motivation to add new precompiles for ed25519? why is ED25519MMA needed for ed25519 as it seems unneeded as my old PRs show?

well we are just editors and this PR updates your EIP as well as the author list. whether its included or not is outside our purview and is to be decided by the ACD and needs to be championed by the authors May be @rdubois-crypto feels upto it.

but anyway thanks for giving it a look and your comments, much appreciated 🙏

@oberstet
Copy link
Contributor

oberstet commented Aug 29, 2023

Regarding this what do you think should be the political choice ?

mmh, I don't see any political dimension? I'm pretty oldschool: the best technical solution, and with the users being priority should be chosen.

the best technical solution would (in my eyes .. all of this is just my 2cts) would be if Ethereum would move to WASM for its (standard) VM definition.

this would remove the necessity / reason for all precompiles, since the contract language would compile natively anyways

further, in my eyes, what would remain worth investigating: WASM lacks a 256 bit native integer, and that could be added .. to WASM. this would require working with the WASM guys in the first place, not Ethereum, which would simply use that and build a VM (an Ethereum one, not the base WASM) on

the reality is: eWASM is still not here in 2023, and Ethereum will stay on current EVM? not sure, I haven't been following things closely, and I have no clue where Ethereum core developers want to take it

regarding

adding EDMMA in a separate EIP
adding EDMMA here ?
keeping EDMMA instead of EDVERIFY ?

the one argument that would change my views within the constraints/context of not having WASM, and above: could I implement EDVERIFY in Solidity with only EDMMA, and if so, what about gas?

The argument is right and HACL shall be kept and libsodium removed then.

no, both (at least the references and discussion of both) should be kept. and implementation diversity is maybe a good goal anyways. as a developer, libsodium seems good enough and less hassle or work or risk (as in long-term maintenance, not functional correctness of course). as a math guy, HACL seems more attractive obviously. and WASM would render both void - if the proofs of HACL could be ported to a WASM implementation which should be possible. "best of both worlds". anyways, details;)

well we are just editors and this PR updates your EIP as well as the author list.

this - or any new EIP - is completely pointless and a waste of time (mine and yours) if there isn't at least the goal of getting it merged and implemented in real world EVM implementations, specifically all the important ones

this is a matter of organizing and getting enough support among core developers

for that, PRs for EVMs would be important - which I spent quite some time creating (attached with my EIP), but it was a waste of time, as the EIP seems dead.


in summary, if you want to give it a new try, I'd be interested in joining an eth core developer call, and give an argument one more try. lemme know if so! maybe things have changed after 5 years.

Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

We'll need the original author's approval for this PR, unless the majority editors agree to override.

+1 to overriding

Also, library links can't be used. You must directly pass the URL.

@rdubois-crypto
Copy link
Author

rdubois-crypto commented Sep 1, 2023

further, in my eyes, what would remain worth investigating: WASM lacks a 256 bit native integer, and that could be added .. to WASM. this would require #working with the WASM guys in the first place, not Ethereum, which would simply use that and build a VM (an Ethereum one, not the base WASM) on

Not gonna happen. 256 bit native integer doesn't exist on real world machine, so you need to build a multiprecision integer library on top of existing i32/i64. What is lacking tho to be efficient is a multiplier i64xi64 returning the high part of the product, which is something existing on all main architectures. Then it is classical math.

the one argument that would change my views within the constraints/context of not having WASM, and above: could I implement EDVERIFY in Solidity with only EDMMA, and if so, what about gas?

You also need SHA512, because unfortunately eddsa uses overkill hash size, and tis not available as precompile. As stated above the cost of SHA512 is far lesser than scalar multiplication. I began implemented it in solidity, to have a full eddsa implementation, but would prefer to have the precompile.

the reality is: eWASM is still not here in 2023, and Ethereum will stay on current EVM? not sure, I haven't been following things closely, and I have no clue where Ethereum core developers want to take it

I also regret that point and think that this would be optimal. However it is a bigger change than adding a single call and i'm a just a grain of sand here.

in summary, if you want to give it a new try, I'd be interested in joining an eth core developer call, and give an argument one more try. lemme know if so! maybe things have changed after 5 years.

I'm open to any discussion. My team just aims to achieve the lowest gas costs possible for the 4337 UserOp using ed25519 we want to build.

@oberstet
Copy link
Contributor

oberstet commented Sep 1, 2023

Not gonna happen. 256 bit native integer doesn't exist on real world machine

indeed, there doesn't seem to be a 256 bit word sized cpu. WASM is a virtual machine, and could define (it's not there at this time of course). since 64 bit are enough for any virtual memory space in practice, a real 256 bit word sized cpu seems 2 exponentiations away and unlikey, I agree.

I'm open to any discussion.

did you reach out? 5 years ago, I seem to remember I had to file an issue describing what I want to present/discuss on some special repo to get on a developer call with the topic .. after issue was accepted, I presented on dev call and joined couple of subsequent calls .. I don't know how it works today. do you?

We'll need the original author's approval for this PR, unless the majority editors agree to override.

right now, I am indeed opposed to this:

I don't think merging this PR to add yet another precompile (ED255MMA) into this EIP (which only adds EDVERIFY) makes sense. the main reason being: I think it's a waste of time unless core developers would consider it (the whole Ed25519 thing) at all

but: you can of course just file a new EIP! only adding ED255MMA, and no EDVERIFY

@Pandapip1
Copy link
Member

Moving an EIP into Draft (or even Review) is not an endorsement by anybody, and except in extremely rare circumstances, EIPs are not blocked because of the ideas present in them.

@g11tech
Copy link
Contributor

g11tech commented Sep 13, 2023

@ rdubois-crypto it would be better to file a new EIP

Copy link

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 Dec 23, 2023
Copy link

github-actions bot commented Feb 4, 2024

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-status Changes a proposal's status e-review Waiting on editor to review s-draft This EIP is a Draft t-core w-ci Waiting on CI to pass w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants