Use any FieldElement and clean up field API #22

cjpatton · 2021-04-05T20:19:08Z

Closes #10.

This PR includes the following changes:

Generalizes the code for generating and validating proofs to work any implementation of the FieldElement trait.
Renames Field to Field32 in order to signify that it implements a 32-bit field.
Renames the finite_field module to simply field.
Renames enum FiniteFieldError to FieldError.

tgeoghegan

Looks great! While it's a lot of lines changed, I'm pleased at how gracefully F: FieldElement replaces Field all over the place. All I have is some obtuse gripes about error handling and interfaces on FieldElement.

src/field.rs

tgeoghegan · 2021-04-06T22:38:02Z

src/client.rs

    /// Construct a new Prio client
    pub fn new(dimension: usize, public_key1: PublicKey, public_key2: PublicKey) -> Option<Self> {
        let n = (dimension + 1).next_power_of_two();

-        if 2 * n > Field::generator_order() as usize {
+        if F::Integer::try_from(2 * n).unwrap() > F::generator_order() {


Since we're already breaking API compat and now that there's two failure modes in this function (2n can either be too large for F::Integer or bigger than generator_order) I think this should return Result<Self, ClientError> and provide a friendlier error message that whatever the panic from unwrap() would be.

Done. (Please take a look at the diff ... I had to refactor things a bit.)

tgeoghegan · 2021-04-06T22:49:14Z

src/util.rs

-        let integer = <Field as FieldElement>::Integer::from_le_bytes(chunk.try_into().unwrap());
-        vec.push(Field::from(integer));
+pub fn deserialize<F: FieldElement>(data: &[u8]) -> Result<Vec<F>, FieldError> {
+    let mut vec = Vec::<F>::with_capacity(data.len() / F::BYTES);


Should this check that data.len() is a multiple of F::BYTES?

Done. (Needed to refactor the code to propagate this error.)

src/util.rs

tgeoghegan · 2021-04-06T22:59:00Z

src/server.rs

@@ -189,12 +190,13 @@ pub fn generate_verification_message(
    mem.points_h[0] = *unpacked.h0;

    // set points_f and points_g
+    let one = F::root(0).unwrap();


It seems like we now have quite a few places where we call F::root(0).unwrap(). That bugs me a bit because at each callsite, the reader has to go "wait, if this method can ever fail, why it is safe and cool to .unwrap() here?" Your answer is going to be that for any correct implementation of FieldElement, F::root(0) cannot fail. So what if we introduce a method fn FieldElement::zeroth_root(&self) -> Self, which internally does F::root(0).unwrap()?

I'll go with F::one(), since this is always the multiplicative identity.

tgeoghegan · 2021-04-06T23:03:23Z

src/server.rs

        loop {
-            let eval_at = Field::from(rand::random::<u32>());
+            let eval_at = F::rand(&mut rng);


It seems like everywhere F::rand is used, the caller has to provide a rand::Rng, which is a bit of a hassle. I see the value in exposing a variant that lets the caller provide an RNG, but could we perhaps have two variants, one that takes no arguments and defaults to rand::thread_rng() and another that lets the caller provide a rand::Rng?

Hmm, well already have random_field_from_seed which maps a seed to a field element. The client and aggregators need to agree on how to do this procedure. This is basically doing the same thing as the current fn rand<R: Rng + ?Sized>(rng: &mut R) -> Self API, which is a little messy. I think we should just nix fn rand<R: Rng + ?Sized>(rng: &mut R) -> Self; call and just have fn rand() -> Self.

tgeoghegan · 2021-04-06T23:13:27Z

src/field.rs

+            fn append_to(&self, bytes: &mut Vec<u8>) {
+                let int = $fp.from_elem(self.0);
+                let mut slice = [0; Self::BYTES];
+                for i in 0..Self::BYTES {
+                    slice[i] = ((int >> (i << 3)) & 0xff) as u8;
+                }
+                bytes.extend_from_slice(&slice);
+            }
+
+            fn read_from(bytes: &[u8]) -> Result<Self, FieldError> {
+                if Self::BYTES > bytes.len() {
+                    return Err(FieldError::FromBytesShortRead);
+                }
+
+                let mut int = 0;
+                for i in 0..Self::BYTES {
+                    int |= (bytes[i] as u128) << (i << 3);
+                }
+
+                if int >= $fp.p {
+                    return Err(FieldError::FromBytesModulusOverflow);
+                }
+                Ok(Self($fp.elem(int)))
+            }


It bugs me slightly that these methods aren't symmetric. It also seems unfortunate that append_to is only capable of appending to a Vec<u8>. It would be more generally useful to provide routines that allow transforming a FieldElement to and from [u8], and then you could do anything you want with the byte array, whether it's appending to a Vec or writing to an std::io::Write.

I wonder: what if instead of these two methods on FieldElement, you had FieldElement require TryFrom<[u8]>, and provided From $elem for [u8]?

As discussed on the design call, @acmiyaguchi will look into replacing these functions with implementations of whatever traits are needed by serde. See #24. I've added TODOs in the code to remind us.

tgeoghegan

A couple nits about the ? operator but this is essentially good to go. Please address those, then squash as you see fit, and I'll merge. Thanks!

src/client.rs

src/server.rs

This change generalizes the code for generating and validating proofs to work with any implementation of the FieldElement trait. It also makes the following API-breaking changes: * Rename Field to Field32 in order to signify that it implements a 32-bit field * Rename FiniteFieldError to FieldError * Move finite_field to field * Simplify the FieldElement API a bit

cjpatton · 2021-04-07T20:54:29Z

Squashed and ready to merge.

tgeoghegan requested changes Apr 6, 2021

View reviewed changes

cjpatton mentioned this pull request Apr 7, 2021

Unify and clean up serialization logic #24

Closed

cjpatton requested a review from tgeoghegan April 7, 2021 19:24

tgeoghegan requested changes Apr 7, 2021

View reviewed changes

src/client.rs Outdated Show resolved Hide resolved

src/server.rs Outdated Show resolved Hide resolved

cjpatton force-pushed the more-fields-2 branch from ae8cc97 to 4533590 Compare April 7, 2021 20:53

tgeoghegan approved these changes Apr 7, 2021

View reviewed changes

tgeoghegan merged commit 85feeb8 into divviup:main Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use any FieldElement and clean up field API #22

Use any FieldElement and clean up field API #22

cjpatton commented Apr 5, 2021

tgeoghegan left a comment

tgeoghegan Apr 6, 2021

cjpatton Apr 7, 2021 •

edited

Loading

tgeoghegan Apr 6, 2021

cjpatton Apr 7, 2021

tgeoghegan Apr 6, 2021

cjpatton Apr 7, 2021

tgeoghegan Apr 6, 2021

cjpatton Apr 7, 2021

tgeoghegan Apr 6, 2021

cjpatton Apr 7, 2021

tgeoghegan left a comment

cjpatton commented Apr 7, 2021

Use any FieldElement and clean up field API #22

Use any FieldElement and clean up field API #22

Conversation

cjpatton commented Apr 5, 2021

tgeoghegan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpatton Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgeoghegan left a comment

Choose a reason for hiding this comment

cjpatton commented Apr 7, 2021

cjpatton Apr 7, 2021 •

edited

Loading