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.

Sign up for GitHub

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

Jump to bottom

Group trait overhaul #52

Merged
merged 9 commits into from
Jan 18, 2022
Merged

Group trait overhaul #52

merged 9 commits into from
Jan 18, 2022

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jan 9, 2022 •
edited
Loading

Replaces #49.

  • Move Group constraints to associated type: Elem.
  • Implement Group for NistP256 instead of ProjectivePoint.
  • Implement Group for Ristretto255, our own type, instead of RistrettoPoint. This type is re-exported.
  • Change SUITE_ID to u16.
  • Reworked scalar de-serialization, now scalars are actually checked for validity and not reduced or clamped into validity. Zero-checking is now not a separate function.
  • Reworked hash_to_scalar and hash_to_curve not take iterators anymore in preparation of Improve API for hash2curve functions RustCrypto/traits#876. This included reverting a lot of convenience utilities. Maybe we will come up with alternatives in the future.
  • Reworked expand_message_xmd in with some additional constraints according to the spec.
  • Simplify P-256 HashToScalar implementation.

@daxpedda daxpedda marked this pull request as draft January 9, 2022 19:58
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 9, 2022
@daxpedda daxpedda force-pushed the group-trait-2 branch 2 times, most recently from 92e47e3 to 2c19262 Compare January 9, 2022 20:18
@daxpedda
Rework scalar de-serialization
f8cd7a1
@daxpedda
Rename Group methods
0efb3d9 
- `random_nonzero_scalar` -> `random_scalar`
- `scalar_as_bytes` -> `serialize_scalar`
- `scalar_invert` -> `invert_scalar`
@daxpedda
Rework element de-serialization
95810fb
@daxpedda
Rename and remove Group methods
c255d64 
`to_arr` -> `serialize_elem`
`base_point` -> `base_elem`
`is_identity` -> removed
`identity` -> `identity_elem`
`zero_scalar` -> hidden behind `cfg(test)`
@daxpedda daxpedda force-pushed the group-trait-2 branch 4 times, most recently from 13379ad to 23950cb Compare January 10, 2022 06:04
@daxpedda daxpedda marked this pull request as ready for review January 17, 2022 15:55
kevinlewi
kevinlewi reviewed Jan 18, 2022
View reviewed changes
Copy link
Contributor

@kevinlewi kevinlewi left a comment

Choose a reason for hiding this comment

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

This is really great! A couple of minor comments.

src/util.rs
($var:ident) => {
&$var
};
pub(crate) fn i2osp_2(input: usize) -> Result<GenericArray<u8, U2>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this named i2osp_2? Do you perhaps just mean i2osp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named this i2osp_2 because i2osp was still alive and this one wasn't generic, it simply always used a size of 2. Later on it just happened that all instances of i2osp were replaced with i2osp_2 so it was left like this.

I think the naming is still appropriate as the official i2osp function takes a length, that one doesn't, it always uses a length of 2, which is the only thing needed in VOPRF.

src/util.rs
let $var = $var.chain(chain_skip!(__temp$(, $feed2)?));
)+
};
pub(crate) fn i2osp_2_array<L: ArrayLength<u8> + IsLess<U256>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this named i2osp_2_array? Do you perhaps just mean i2osp_array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above.

src/group/mod.rs
use crate::voprf::Mode;
use crate::Result;

pub(crate) const STR_HASH_TO_SCALAR: [u8; 13] = *b"HashToScalar-";
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if neither the p256 or ristretto255 feature is enabled, then these strings won't get used. Perhaps we could either just make them pub, or cfg-if on the features?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna handle this in a follow-up when we actually remove the P-256 implementation and replace it with a generic one over traits of elliptic-curve, then they are always used.

@daxpedda daxpedda requested a review from kevinlewi January 18, 2022 09:03
@kevinlewi
Copy link
Contributor

Awesome, thanks!

@kevinlewi kevinlewi merged commit 652fd1d into facebook:main Jan 18, 2022
@daxpedda daxpedda mentioned this pull request Jan 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinlewi kevinlewi kevinlewi approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daxpedda @kevinlewi @facebook-github-bot