Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

[WIP] add ed25519 signature verification precompile #8330

Closed

Conversation

oberstet
Copy link

@oberstet oberstet commented Apr 6, 2018

See also:

TODO: fill in test vectors in unit test

@parity-cla-bot
Copy link

It looks like @oberstet hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@oberstet oberstet changed the title add ed25519 signature verification precompile [WIP] add ed25519 signature verification precompile Apr 6, 2018
@oberstet
Copy link
Author

oberstet commented Apr 6, 2018

[clabot:check]

@parity-cla-bot
Copy link

It looks like @oberstet signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Apr 9, 2018
@5chdn 5chdn added this to the 1.11 milestone Apr 9, 2018
@5chdn 5chdn added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Apr 9, 2018
@5chdn 5chdn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 16, 2018
@5chdn
Copy link
Contributor

5chdn commented Apr 16, 2018

@oberstet Hey, is this still WIP?

@oberstet
Copy link
Author

@5chdn yeah, definitely! well, I am willing to do more work, but I am stuck on getting some guidance / decision. as described here https://gitter.im/ethereum/AllCoreDevs?at=5ad2e7587c3a01610de35a31, the main options for the ed25519 library we could use are

  • libhacl-c
  • libsodium

I think the latter would be a robust, conservative and practical choice, and I would rewrite/expand the PRs (not only this here) - if there is support. If not, this is just a waste of time ..

@sorpaas
Copy link
Collaborator

sorpaas commented Apr 16, 2018

Just a note, as we can always do this later once the actual spec settled down: Do we consider using the dalek version (https://github.com/dalek-cryptography/ed25519-dalek)? It indeed has less peer reviews but is pure Rust. If we can at least provide a feature flag to enable that version, that would be cool!

@oberstet
Copy link
Author

@sorpaas I am a Rust newbie, don't know the community/ecosystem much, so take this with a grain of salt:

The library you link: https://github.com/dalek-cryptography/ed25519-dalek#warnings => is using https://github.com/isislovecruft/curve25519-dalek for curce 25519 math - and that lib is quite new. and while Isis (https://github.com/isislovecruft) is certainly super cool dev, mmmh.

If you ask me right away, what to go for, I would only consider libsodium and HACL. Eg if we fuck this up, and it is because we have chosen some "random code from the Web", users will be angry. Righly so. In that light, I could only continue sleeping well choosing sth that can be defended - the choice (if there is an issue .. which always can happen).

@oberstet
Copy link
Author

Rgd compile switch to swap the actual implementation being used: could do, question is why? for "regular" use, implementations will behave identical .. the problem is the corner cases: does the elliptic curve math implementation fall apart for certain corner cases? testing this using a handful of test vectors is not enough. and because that is tricky, I'd rather rely on ideally one underlying implementation for all the Eth clients. From how I understood the discussion that happened on Gitter channel, this is also what others hinted at (to address the "consensus failure" catastrophic scenario).

@5chdn 5chdn modified the milestones: 1.11, 1.12 Apr 24, 2018
@5chdn
Copy link
Contributor

5chdn commented May 7, 2018

Hi @oberstet - are you still working on this?

@5chdn 5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label May 16, 2018
@5chdn
Copy link
Contributor

5chdn commented May 16, 2018

Marking this as stale. Happy to reopen this whenever you get back to it. 🤗

@5chdn 5chdn closed this May 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants