search

zkcrypto / jubjub

Implementation of the Jubjub elliptic curve group
Other
119 stars 47 forks source link

Fix ZIP 216 bug #46

Closed str4d closed 3 years ago

str4d commented 3 years ago

See ZIP 216 for more details.

A new API jubjub::AffinePoint::from_bytes_pre_zip216_compatibility has been added for consensus compatibility. We will use this API in Zcash code until NU5 activates.

str4d commented 3 years ago

Spotted a typo in ZIP 216 while implementing this: https://github.com/zcash/zips/pull/518

daira commented 3 years ago

I don't view the non-canonical encoding issue as a bug — or rather, it was a bug when originally identified, but that was fixed for Zcash by changing the spec. I think it's confusing to refer to it as a bug without explaining the context. The purpose of this change is to reduce attack surface.

I think that the API should be called from_bytes_zcash_pre_nu5_compatibility.

str4d commented 3 years ago

I think that the API should be called from_bytes_zcash_pre_nu5_compatibility.

I disagree with having zcash in the name, since it's not solely Zcash that we are exposing this for; anyone who depends on Jubjub in a consensus-critical context will need this. Hence my referring to ZIP 216 specifically in the current name (being a consensus change that others could also choose to apply).

How about from_bytes_pre_zip216_compatibility?

str4d commented 3 years ago

I don't view the non-canonical encoding issue as a bug

I do view it as a bug, because AffinePoint::from_bytes was documented as rejecting non-canonical points, and it did not do so. This PR fixes a bug in the API, not a bug in Zcash per-se.

daira commented 3 years ago

from_bytes_pre_zip216_compatibility is a fine name.