Implement generic FF arithmetic

[cjpatton]

Partially solves #10.

This change replaces the existing finite field (FF) arithmetic with a generic, constant time implementation that is suitable for any modulus p < 2^126. The code was designed by @armfazh and ported from Go into Rust by me.

The goal of this PR is to make no API changes. In particular, the same FF parameters are used for Field. The next step will be to rework the code to be compatible with any field, which may or may not involve breaking the API.

On a personal note, I would appreciate all levels of feedback, as this is the first time I've ever tried to contribute to a real Rust project. Feel free to tear it apart :)

[acmiyaguchi]

For what it's worth, I merged these changes into a wasm-compatible fork (link, rebasing was ugly...) and tried out a test web-app using the library (link). For an input of [1,1,0,1], I used to get [1,1,0,1] out. After switching to the new branch, I get [2564090464,6858009185,4293918721,2564090464]. I'm guessing that this might be caused by the new field representation of u128 vs u32 and wasm-bindgen only supporting 64-bit integers.

[cjpatton]

For what it's worth, I merged these changes into a wasm-compatible fork (link, rebasing was ugly...) and tried out a test web-app using the library (link). For an input of [1,1,0,1], I used to get [1,1,0,1] out. After switching to the new branch, I get [2564090464,6858009185,4293918721,2564090464]. I'm guessing that this might be caused by the new field representation of u128 vs u32 and wasm-bindgen only supporting 64-bit integers.

This likely has to do with the way the new code represents field elements internally. You need to cast field elements to their integer representation by doing SMALL_FP.from_elem(x).

UPDATE: I can confirm this is what's happening. The following:

println!("{}", SMALL_FP.from_elem(2564090464));
println!("{}", SMALL_FP.from_elem(6858009185));
println!("{}", SMALL_FP.from_elem(4293918721));
println!("{}", SMALL_FP.from_elem(2564090464));         

prints:

1
1
0
1

Not sure why you're seeing the internal representations in your context. The Into and From traits do the conversion properly...

[armfazh]

Not sure why you're seeing the internal representations in your context. The Into and From traits do the conversion properly...

This need to be enforced with the type system, thus, the external u128 is not mapped to the u128 used internally.

[acmiyaguchi]

Just to clarify, I'm using the default Serialize/Deserialize traits on the VerificationMessage struct.

https://github.com/abetterinternet/libprio-rs/blob/e0247d036716f3bdbe8702147568a7e107dc5d28/src/server.rs#L164-L172

The above is serialized in a wrapper:

    pub fn generate_verification_message(&mut self, eval_at: u32, share: &[u8]) -> JsValue {
        set_panic_hook();
        let verification = self
            .this
            .generate_verification_message(eval_at.into(), share)
            .unwrap();
        JsValue::from_serde(&verification).unwrap()
    }

Serde doesn't use the Into and From traits afaik, so I may need to write a custom implementation of Serialize and Deserialize if I can't rely on serializing the Field elements directly. It sounds like I can just use ::from to go from Field to u32 and vice-versa during that process?

[cjpatton]

To get from Field to u32, do SMALL_FP.from_elem(x) as u32.
To get from u32 to Field, do SMALL_FP.elem(x as u128).
This is what the Into/From traits do. I'm newish to Rust, so I'm not sure how ::from is going to work. I believe that Field::from(x) will let you get a Field from a u32.

[acmiyaguchi]

Aha, I see now. I'm using reconstruct_shares which returns a vector of Field elements:

https://github.com/abetterinternet/libprio-rs/blob/e0247d036716f3bdbe8702147568a7e107dc5d28/src/util.rs#L129

I was just showing the serialized form of this directly, but casting the elements into integers was what I needed.

    let reconstructed: Vec<u32> = util::reconstruct_shares(share1.as_ref(), share2.as_ref())
        .unwrap()
        .iter()
        .map(|el| u32::from(*el))
        .collect();

Would this be considered an API compatible change from the perspective of https://github.com/abetterinternet/prio-server? I don't have much context from that perspective.

[cjpatton]

Would this be considered an API compatible change from the perspective of https://github.com/abetterinternet/prio-server? I don't have much context from that perspective.

Golly, I hope so. That's a question for @tgeoghegan maybe?

[tgeoghegan]

re: from and API compatibility: I'm not really worried about breaking prio-server, for a couple reasons. First, we would have to explicitly adopt a new libprio-rs version which would give us a chance to fix tests that break, and I assume we will eventually make breaking API changes to this crate. Second, I don't believe the usage of reconstruct_shares in prio-server should be a problem. We only ever do comparisons on Vec<Field> and I don't think we ever convert them to primitives.

Now, on the subject of primitive->Field conversions: I'm worried about unsafe conversions from Field(u128) to u32. Right now, we have impl From<Field> for u32, which can't be safe because u128 is bigger than u32. As implemented, it'll silently truncate values larger than u32::MAX. I think what we should do is instead implement TryFrom<Field> for u32 (and also u16 and u64 and all the integer sizes smaller than u128; see for example the implementations of From and TryFrom on u32 itself). I know @cjpatton is aiming not to break the API with this particular PR, so for now, rather than removing impl From<Field> for u32, I think it should use u32::try_from<u128> and panic! if it fails. This won't break clients who are converting back and forth from u32 to Field, but it also won't silently truncate if someone is doing something weird. Providing all the TryFrom impls is a bit of a chore so feel free to punt that to a future PR (please record an issue if you do).

[cjpatton]

I know @cjpatton is aiming not to break the API with this particular PR, so for now, rather than removing impl From<Field> for u32, I think it should use u32::try_from<u128> and panic! if it fails. This won't break clients who are converting back and forth from u32 to Field, but it also won't silently truncate if someone is doing something weird. Providing all the TryFrom impls is a bit of a chore so feel free to punt that to a future PR (please record an issue if you do).

My main concern about breaking the API is its impact on prio-server, If you're not concerned about this, then I'm fine breaking the API.

However, I think we should punt to a future PR for the following reason. The conversion between Field and u32 is currently safe because the modulus is hard-coded to a 32-bit prime. This will change once we allow larger fields in a future PR. In that PR we'll need to carefully consider how to safely convert between field elements and primitive types. For example, we may define conversion only to the primitive type that contains the modulus. For example, for a 64-bit prime field, we may only define conversion to u64s.

[tgeoghegan]

nit: this is fine for now since the only caller of this method is only going to print any error they encounter, but if we end up with more errors from fp, it'd be best to define an fp::Error enum with variants for specific error cases (like finite_field does).

[cjpatton]

Ack. Yeah, this seems like it will be handy. I'll keep this in mind for the future.

[tgeoghegan]

I agree with your point about u32 <-> Field being safe, even if Field wraps u128, because everything is modulo 4293918721, and that we can make this safer when we expand the Field object model. LGTM, thank you for this contribution!

[cjpatton]

Squashed.