Implement Sinsemilla primitives #21
Conversation
therealyingtong
force-pushed
the
sinsemilla
branch
from
ee4b4a8
to
f9a089d
Compare
src/primitives/sinsemilla.rs
Outdated
| pub(crate) fn hash_to_point( | ||
| domain_prefix: &str, | ||
| msg: impl Iterator<Item = bool> + ExactSizeIterator, | ||
| ) -> pallas::Point { |
There was a problem hiding this comment.
should this be pallas::Affine instead?
There was a problem hiding this comment.
Agreed, the spec currently defines P as the group of points (x,y) on the short Weierstrass form of the curve.
There was a problem hiding this comment.
I do not view this as a type error. pallas::Point and pallas::Affine are equivalent group elements by construction and have 1:1 conversion (enforced by the group::Curve and group::prime::{PrimeCurve, PrimeCurveAffine} traits); the exact in-memory representation of pallas::Point is irrelevant to the user.
(halo2::arithmetic::CurveExt currently exposes the internal representation's Jacobian coordinates, but I disagree with this being part of the main efficient API; it should be contained within a trait specific to implementing IETF's hash-to-curve. Similarly I would ideally like to not expose the affine coordinates in normal APIs, and instead contain that to the context of implementing circuits.)
However, pallas::Point: group::Curve is the format that is efficient to perform operations on, and is intended to be what is passed around in-memory. pallas::Affine is only intended for encoding/decoding and accessing affine coordinates. Returning pallas::Affine from this API would force an unnecessary projective -> affine -> projective round-trip on every call to sinsemilla::commit or sinsemilla::short_commit.
There was a problem hiding this comment.
I basically retract my comment, as I did basically the same thing in https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/sapling/commitment/pedersen_hashes.rs, even though the Jubjub group element definitions in the spec are also 'just' (u, v) etc. 🙈
I would love if the type was named pallas::ProjectivePoint or something. 😅
There was a problem hiding this comment.
What @str4d said. Basically you're always allowed to use isomorphic representations.
(Strictly speaking, \mathbb{P} denotes the type just of an element of the "represented group" of Pallas curve points.)
| /// Hash extractor for Pallas, from [§ 5.4.8.7]. | ||
| /// | ||
| /// [§ 5.4.8.7]: https://zips.z.cash/protocol/orchard.pdf#concreteextractorpallas | ||
| fn extract(point: &pallas::Point) -> pallas::Base { |
There was a problem hiding this comment.
Same here, should probably be pallas::Affine; Extract_P is defined as having a P element input which is an affine point:
Style opinion, as this is an open PR vs needing to open a new thing in halo2: naming all point types as explicitly *Point, such as AffinePoint, ProjectivePoint, etc, which can improve readability and human distinguishability of types. The jubjub crate basically does this.
I know in halo2 a lot of these are generated by macros but the names are chosen on macro invocation and renamed and exported in the pallas module. This is of course not limited to or blocking on this PR but is a kind of naming / typing style thing we zcash engineers can discuss, if desired.
There was a problem hiding this comment.
Having this take pallas::Affine pushes a point.to_affine() call onto every caller, instead of locating them all inside this function. There are only a couple of callers though, so I don't overly mind.
Re: naming of pallas::Point, I opted in the halo2 crate to leave the point representation out of the re-exported name for overall readability (and also because as above, it's irrelevant to the consumer). What mattered more to me was that I could write pallas::Point everywhere, so that it was clear that this is a Pallas point (rather than Vesta or some other curve); pallas::ProjectivePoint is almost twice as long.
jubjub is a slightly different case, because we need to represent both the curve type and whether the point is constrained to the prime-order subgroup, so we already need to include more information in the name (in hindsight jubjub::FullPoint would have been a better name for this purpose than jubjub::ExtendedPoint). Pallas and Vesta are prime-order curves and don't have this problem.
There was a problem hiding this comment.
pallas::ProjectivePoint may be longer but it's not super long, and since there are multiple point types, vs a 'Point' and an 'Affine', it would be nice in terms of having the code document itself. Luckily Rust compiles away token names so we need not be precious on name lengths, as long we we don't turn into Java (😅 )
There was a problem hiding this comment.
I'm only weakly against a rename (I like Dodger's "weakly" vs "strongly" for indicating preference), so I'll defer to others on this.
There was a problem hiding this comment.
I like the current short names. In practice it's not hard to remember that Point is projective (and for most purposes it doesn't matter, since the representations are isomorphic).
| /// | ||
| /// [§ 5.4.7.4]: https://zips.z.cash/protocol/orchard.pdf#concretesinsemillacommit | ||
| #[allow(non_snake_case)] | ||
| pub(crate) fn commit( |
There was a problem hiding this comment.
SinsemillaCommit as an instance of SinsemillaHashToPoint also returns an affine point as an element of P
There was a problem hiding this comment.
yup, @str4d i've made a few changes to primitives/sinsemilla.rs to get it working with my WIP chip here: https://github.com/zcash/orchard/pull/25/files
There was a problem hiding this comment.
i've reverted these changes; now calling to_affine() at the caller instead. 👍
therealyingtong
force-pushed
the
sinsemilla
branch
from
f9a089d
to
a03ee87
Compare
Codecov Report
@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 0.00% 44.30% +44.30%
==========================================
Files 4 5 +1
Lines 32 79 +47
==========================================
+ Hits 0 35 +35
- Misses 32 44 +12
Continue to review full report at Codecov.
|
Test vectors will be added in a subsequent PR.