make `vdaf::verify_finish` take a slice #85

tgeoghegan · 2021-09-22T23:40:23Z

verify_finish wants to consume a collection of VerifierMessage.
Accomplishing this with Vec<VerifierMessage> forces allocation and
copying to the heap. We now instead use const generics and take M: TryInto<[VerifierMessage; const N: usize]>, so that callers that hold
either a [VerifierMessage] on the stack or a Vec<VerifierMessage> on
the heap, or anything else that can be converted to a slice of
VerifierMessage, can use verify_finish.

cjpatton

Dope! While at it, do you think we should do the same thing for dist_input() and dist_init(), i.e., make these output a vector of constant size?

cjpatton · 2021-09-22T23:42:54Z

src/vdaf.rs

+    M: TryInto<[VerifierMessage<V::Field>; N], Error = E>,
+    E: std::fmt::Debug,


It would be nice to reduce the number of type parameters. Is this possible?

Suggested change

M: TryInto<[VerifierMessage<V::Field>; N], Error = E>,

E: std::fmt::Debug,

M: TryInto<[VerifierMessage<V::Field>; N], Error: std::fmt::Debug>,

It would avoid the additional type parameter E. (Also, I think Debug is imported by default these days.)

Alternatively, I would be comfortable with a panic!() here instead of propagating the error. This is less of an input validation issue than it is a programmer error.

I tried doing that but you can't do it yet. There's a Rust RFC, it's implemented, but it's not yet stabilized.

In that case my preference would be to panic instead of propagating the error.

Sure, I struggle to imagine this failing for any reason beyond whatever collection msg is having the wrong number of elements, so I think we can just panic! and get rid of the Debug trait bound.

cjpatton and I chatted about this and got extremely galaxy brained about it. I'm going to try changing this so that verify_finish takes IntoIterator<Item = VerifierMessage<V::Field>>, which ✅ allows callers to provide any kind of slice or a vec or whatever and ✅ is infallible so we don't have to deal with any type bounds on errors.

cjpatton · 2021-09-22T23:43:32Z

src/vdaf.rs

+{
+    let msgs: [VerifierMessage<V::Field>; N] = msgs.try_into().map_err(|e| {
+        VdafError::Uncategorized(format!(
+            "input could not be converted to VerifierMessage slice: {:?}",


Suggested change

"input could not be converted to VerifierMessage slice: {:?}",

"verify_finish(): input could not be converted to VerifierMessage slice: {:?}",

For consistency

cjpatton · 2021-09-22T23:48:19Z

src/vdaf.rs

@@ -468,7 +480,8 @@ mod tests {
        // Aggregators decide whether the input is valid based on the verifier messages.
        let mut output = vec![Field64::zero(); input.as_slice().len()];
        for state in states {
-            let output_share = verify_finish(state, verifiers.clone()).unwrap();
+            let output_share =
+                verify_finish::<_, _, _, NUM_SHARES>(state, verifiers.clone()).unwrap();


I'm curious if all of the type parameters, including NUM_SHARES, could be inferred if verifiers had type [VerifierMessage<Boolean<Field64>>; NUM_SHARES].

Yes, you could instead do:

let verifiers_slice: [VerifierMessage<Field64>; NUM_SHARES] = verifiers.try_into().unwrap(); for state in states { let output_share = verify_finish(state, verifiers_slice.clone()).unwrap(); <...>

I wanted to prove that passing in a Vec<VerifierMessage<F>> works, though. What would be cool is if Rust had named trait bounds, so that instead of specifying NUM_SHARES positionally, I could do let output_share = verify_finish::<N = NUM_SHARES>(...);, since the _, _, _, is rather ugly.

I'm not concerened about the ugliness here, I was just curious.

cjpatton · 2021-09-22T23:54:06Z

src/vdaf.rs

@@ -374,10 +374,22 @@ pub fn verify_start<V: Value>(
 /// [`VerifierMessage`] messages broadcast by all of the aggregators and produces the aggregator's
 /// input share.
 // TODO(cjpatton) Check for ciphersuite mismatch between `state` and `msgs` and among `msgs`.
-pub fn verify_finish<V: Value>(
+pub fn verify_finish<M, E, V, const N: usize>(


How about SHARES instead of N? This would align nicely with the spec.

tgeoghegan · 2021-09-23T00:07:32Z

Dope! While at it, do you think we should do the same thing for dist_input() and dist_init(), i.e., make these output a vector of constant size?

Yeah, that is a good idea, if we're prepared to commit to making the number of shares a parameter that's known at compile time. I think that's fine because num_shares will be 2 in all realistic deployments of libprio-rs for as far out as I can see. I'm going to refactor dist_input and dist_init as you suggest and update ppm-prototype to use the resulting interfaces. It will take me until Friday to update this PR with those changes because I'm flying tomorrow, though.

cjpatton · 2021-09-23T00:18:22Z

Dope! While at it, do you think we should do the same thing for dist_input() and dist_init(), i.e., make these output a vector of constant size?

Yeah, that is a good idea, if we're prepared to commit to making the number of shares a parameter that's known at compile time. I think that's fine because num_shares will be 2 in all realistic deployments of libprio-rs for as far out as I can see. I'm going to refactor dist_input and dist_init as you suggest and update ppm-prototype to use the resulting interfaces. It will take me until Friday to update this PR with those changes because I'm flying tomorrow, though.

If we're going to constrain the number of inputs to verify_finish, then it makes sense to constrain the other functions the same way, since they would be effectively constrained at the protocol level anyway.

I think SHARES > 2 is going to be needed eventually. I think the more important underlying assumption s that a PPM task is always going to have a constant number of aggregators, so hard-coding this at compile time is fine.

tgeoghegan · 2021-09-24T18:27:19Z

I tried using const generics to make dist_input and dist_init return constant size vectors, but I found that it ends up being quite awkward. Right now we do something like:

let mut out: Vec<Value> = Vec::with_capacity(num_shares);
for foo in foos {
    out.push(Value { ... });
}
out

The equivalent construction with an array would be something like:

let mut out = <[Value; NUM_SHARES]>::default();
for (i, foo) in foos.enumerate() {
    out[i] = Value { ... }
}
out

Because you can't have an uninitialized array in Rust. The problem is that we need to provide an std::default::Default implementation for types like InputShareMessage and VerifierMessage, and in turn for Key, which isn't obvious. Since @cjpatton wants to change the VDAF syntax early next week, I think I'd rather defer that change until other stuff stabilizes some more.

tgeoghegan · 2021-09-24T18:29:14Z

Tagging @branlwyd for visibility -- don't worry if none of this makes sense, I just want you to be in the loop on libprio-rs changes.

`verify_finish` wants to consume a collection of `VerifierMessage`. Accomplishing this with `Vec<VerifierMessage>` forces allocation and copying to the heap. We instead take `IntoIterator<Item = VerifierMessage>`, which allows callers to pass either a slice of `VerifierMessage` on the stack or a `Vec` on the heap, or anything else that implements `IntoIterator`. Besides the flexibility, `IntoIterator.into_iter` is infallible, so this doesn't introduce any new error cases to handle.

tgeoghegan requested a review from cjpatton September 22, 2021 23:40

cjpatton requested changes Sep 22, 2021

View reviewed changes

tgeoghegan requested review from cjpatton and branlwyd September 24, 2021 18:27

cjpatton approved these changes Sep 28, 2021

View reviewed changes

tgeoghegan force-pushed the timg/verify-finish-slice branch from 534d9cf to dd4c7b8 Compare September 28, 2021 14:44

tgeoghegan mentioned this pull request Sep 28, 2021

Use const generics where feasible #18

Closed

tgeoghegan merged commit 627ae5a into main Sep 28, 2021

tgeoghegan deleted the timg/verify-finish-slice branch October 12, 2022 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make `vdaf::verify_finish` take a slice #85

make `vdaf::verify_finish` take a slice #85

tgeoghegan commented Sep 22, 2021

cjpatton left a comment

cjpatton Sep 22, 2021

tgeoghegan Sep 23, 2021

cjpatton Sep 23, 2021

tgeoghegan Sep 23, 2021

tgeoghegan Sep 23, 2021

cjpatton Sep 22, 2021

cjpatton Sep 22, 2021

tgeoghegan Sep 23, 2021 •

edited

Loading

cjpatton Sep 23, 2021

cjpatton Sep 22, 2021

tgeoghegan commented Sep 23, 2021

cjpatton commented Sep 23, 2021

tgeoghegan commented Sep 24, 2021 •

edited

Loading

tgeoghegan commented Sep 24, 2021

		M: TryInto<[VerifierMessage<V::Field>; N], Error = E>,
		E: std::fmt::Debug,

	"input could not be converted to VerifierMessage slice: {:?}",
	"verify_finish(): input could not be converted to VerifierMessage slice: {:?}",

make vdaf::verify_finish take a slice #85

make vdaf::verify_finish take a slice #85

Conversation

tgeoghegan commented Sep 22, 2021

cjpatton left a 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 Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgeoghegan commented Sep 23, 2021

cjpatton commented Sep 23, 2021

tgeoghegan commented Sep 24, 2021 • edited Loading

tgeoghegan commented Sep 24, 2021

make `vdaf::verify_finish` take a slice #85

make `vdaf::verify_finish` take a slice #85

tgeoghegan Sep 23, 2021 •

edited

Loading

tgeoghegan commented Sep 24, 2021 •

edited

Loading