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

Improve points being used in data structures #220

Closed
wants to merge 12 commits into from

Conversation

burdges
Copy link
Contributor

@burdges burdges commented Dec 26, 2018

In multi party protocols, there are situations where indexing some data structure with a public key comes in handy, so this adds various #[derive(..)] to CompressedRistretto.

In addition, this adds a RistrettoBoth both convenience type, which requires extensive boilerplate itself, but makes it easy to keep compressed and uncompressed forms together. If used properly, I believe RistrettoBoth should reduce implementation errors, improve readability, simplify indexing data structure, and maybe facilitate performance improvements like dalek-cryptography/ed25519-dalek#61

In essence, RistrettoBoth acts like a CompressedRistretto that drags around RistrettoPoint, which you access by calling .as_point(). I therefore did not implement constant time or arithmetic operation traits for it, but..

hdevalence and others added 12 commits December 26, 2018 22:03
Also removes the chi function since Ristretto elligator merges it with the square root.
There are enough things that require both together that this looks
convenient.  There is minimal cost to calling as_point when you want
a poiont form so I have not added arithmatic traits, but maybe they'd
help in places.

These should only exist for data being comitted to transcripts, like
compressed points, so I implemented many traits using only the
compressed form, not the point form with extra constant time assurances.
These should make serde and the optional multiscalar stuff play
nicely, but maybe another route would work better.
We could add TryFrom going the other way, except no error type is
defined.
@hdevalence hdevalence changed the base branch from master to develop December 27, 2018 04:36
@hdevalence
Copy link
Contributor

Thanks for this PR.

The additional derives for CompressedRistretto might be reasonable, and it would be good to have them in a separate PR based off of the current develop branch for discussion just for those. (I would like to think about what it means for us to advertise a point as being Ord, for instance, before merging it).

I'm not sure if we want to add the RistrettoBoth API. In the next few months, I'm planning to update and merge the code I wrote that provides a precomputation API (#125), which might supersede the precomputation use-case, and I'm not sure it's better to allow API clients to not think about the serialization/validation operation. For Ed25519, it makes sense to factor the public-key decompression into the PublicKey struct, but this might not be the case for other protocols, and in our Bulletproofs implementation, we arrange the code so that we don't need to maintain a pair of points.

@burdges
Copy link
Contributor Author

burdges commented Dec 27, 2018

Cool. Yeah cosmetic things like this should wait until any relevant developments get sorted out. :)

Yes, it's true protocol implementors could simply accept RistrettoBoth types instead of creating their own public key type, which should often have an error type with deserialization errors, like Ed25519, but the protocol implementor can reason about the deserialization failures better than users. In principle implementors can abuse RistrettoPoint similarly though, so at least RistrettoBoth gives a venue to explain this worry.

@hdevalence
Copy link
Contributor

It might be a good idea - if it ends up being useful in a bunch of libraries that use curve25519-dalek, I wouldn't be opposed to adding it, I would just like to wait on it.

@burdges
Copy link
Contributor Author

burdges commented Dec 31, 2018

There is a minor mistake in the PartialEq and Eq definition for RistrettoBoth here, probably either they should be derived, and thus compare the point too, or else the fields should be kept private to the module.

@burdges
Copy link
Contributor Author

burdges commented Mar 13, 2019

I increasingly find this "both" approach slightly distasteful. It might be best to provide a both type but actually use some trait so that deserialization can be delayed until optional_multiscalar_mul. I originally considered using a trait that worked for multiple stages of being decompressed to be too much magic but maybe it's fine.

pinkforest pushed a commit to pinkforest/curve25519-dalek that referenced this pull request Jun 27, 2023
* Fixed bench when `batch` feature is not present

* Added bench build regression test to CI

* Fixed batch build more generally

* Simplified batch cfg gates in benches

* Updated criterion

* Made CI batch-nondeterministic test use nostd

* Fix batch_deterministic build

* Removed bad compile error when batch and batch_deterministic are selected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants