Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

RFC: adopt zip 215 #144

Merged
merged 9 commits into from
Nov 5, 2020
Merged

RFC: adopt zip 215 #144

merged 9 commits into from
Nov 5, 2020

Conversation

tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Aug 20, 2020

Short RFC to adopt zip215 for Tendermint use cases.

Read ZIP215 here: https://zips.z.cash/zip-0215

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

  1. what library tendermint-rs is using right now?
  2. was https://github.com/hdevalence/ed25519consensus verified by someone outside of Zcash? Do we want to perform any additional testing before adopting it?

@tac0turtle
Copy link
Contributor Author

1. what library tendermint-rs is using right now?

ed22519-dalek

2. was [hdevalence/ed25519consensus](https://github.com/hdevalence/ed25519consensus) verified by someone outside of Zcash? Do we want to perform any additional testing before adopting it?

we can perform additional testing. Filippo did some work on the library as well. The part of the library that we will use is only the verify function, everything else is still under the go std library.

@erikgrinaker
Copy link
Contributor

Makes sense to adopt some standard, don't have an informed opinion as to which one. Will this have to be a breaking change, or can we support both the old and new schemes in a transition period?

@tac0turtle
Copy link
Contributor Author

Makes sense to adopt some standard, don't have an informed opinion as to which one. Will this have to be a breaking change, or can we support both the old and new schemes in a transition period?

It is not breaking but it is best to have it be handled on a new chain instead of an upgrade of version

@zmanian
Copy link
Contributor

zmanian commented Aug 25, 2020

👍

  1. what library tendermint-rs is using right now?
  2. was https://github.com/hdevalence/ed25519consensus verified by someone outside of Zcash? Do we want to perform any additional testing before adopting it?

Fillipo who is the maintainer for x/crypto in the Go standard library also looked at it.

Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Seems like a good idea to push for this standardization

rfc/003-ed25519-verification.md Show resolved Hide resolved
@tessr
Copy link
Contributor

tessr commented Sep 11, 2020

I'm generally in favor of adopting ZIP 215. I'd love to see this RFC address a few more things before we merge it:

  1. Concerns around breaking-ness. Although technically not a breaking change, it is a change that needs to be coordinated so that all validators on a network can verify signatures consistently with each other.*
  2. Anything we know about proposed plans regarding the future of this new library (e.g., is Filippo and/or Google going to take it over? I couldn't quite track the conversation here).
  3. If we want to do any security review on this.
  4. If it impacts other requested work, like signature aggregation or batch verification.

*To expand on the "breaking-ness" of this change:

  1. Old signatures (e.g. signatures created with the non-ZIP-215 ed5519 library) can be validated by new verification functions (e.g. the verification function from the ZIP-215 library).
  2. New signatures can also be validated with old verification functions; however, signatures can be maliciously crafted to validate with the new verification functions but not the old verification functions. This means that someone could deliberately create a malicious signature which would split (and halt) the network.
  3. Therefore, it’s not “breaking” to adopt ZIP 215, but it is important to coordinate upgrades all together. I raised this over in the Zcash Discord, and Henry agreed that it would be good to treat this change as a breaking one.

@robert-zaremba
Copy link
Contributor

robert-zaremba commented Sep 16, 2020

In SDK, I was mentioning BIP-340.

Are you going to use ed25519 with ZIP-215? @marbar3778 mentioned that you are working on sr25519 (W3F researched it for Polkadot), is it different than ZIP-215? Seams like sr25519 could be a safe bet.

@tac0turtle
Copy link
Contributor Author

Are you going to use ed25519 with ZIP-215? @marbar3778 mentioned that you are working on sr25519 (W3F researched it for Polkadot), is it different than ZIP-215? Seams like sr25519 could be a safe bet.

Sorry for the confusion. I meant that it is merely supported in the crypto package. We do not utilize the key type in Tendermint.

@tac0turtle tac0turtle requested review from tessr and removed request for tessr October 5, 2020 12:08
@hdevalence
Copy link

hdevalence commented Oct 6, 2020

Hi all, I wrote up a more detailed post on the background behind the ZIP215 rules:

https://hdevalence.ca/blog/2020-10-04-its-25519am

Hopefully it fills in any missing detail about why the rules are created the way they are.

@hdevalence
Copy link

I left some review comments -- I realize that this is somewhat of a "drive-by review" since I don't have a lot of context about Tendermint, so my apologies if I missed something because of that, and I hope the comments are helpful anyways.


- Third_party dependency
- library has not gone through a security review.
- unclear maintenance schedule

Choose a reason for hiding this comment

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

In re: this line and the comment made by @tessr here: #144 (comment) ,

Anything we know about proposed plans regarding the future of this new library (e.g., is Filippo and/or Google going to take it over? I couldn't quite track the conversation here).

as far as I know there are no current plans to upstream the library to Filippo or Google, so definitely don't count on that.

The maintenance aspect of that library is definitely a weak point. I'm not a Go expert and I'm only casually familiar with its ecosystem (e.g., I had no idea how to publish a package and I'm still not sure I did it right), so ideally someone who has those skills could help with that (or take it over).

That said, once the library is complete I think that it should require very little maintenance, because it has a tiny API surface, and it solves a problem with a fixed specification (ZIP215).

What has to happen for it to be complete? Basically there are two changes. First, it should be changed to use https://pkg.go.dev/filippo.io/edwards25519 internally, now that that library exists. Second, I'd like it to support batch verification with an init-update-finalize style API like ed25519-zebra. Currently this isn't scoped out on the repo's roadmap because I've been pretty busy, and I'm not sure when I would have time to make those changes, but I'd be happy to mentor someone else who'd like to have a go at making them.

@tessr tessr mentioned this pull request Oct 29, 2020
7 tasks
@tac0turtle tac0turtle merged commit d31a4a4 into master Nov 5, 2020
@melekes melekes deleted the marko/rfc-003 branch November 5, 2020 09:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants