divviup / libprio-rs

Implementation of Prio in Rust.
Mozilla Public License 2.0
103 stars 31 forks source link

Implement draft-irtf-cfrg-vdaf-13 #1122

Open divergentdave opened 1 month ago

divergentdave commented 1 month ago

Here's a summary of what we'll need to implement, based on the changelog.

This will also be a good opportunity for other breaking changes we want to make.

rozbb commented 3 weeks ago

@divergentdave looking at the entry:

FLP circuit output may be a vector. If so, it is reduced using query randomness. (draft-10)

Is this already reflected in the code? Specifically in trait Type: https://github.com/divviup/libprio-rs/blob/d2fe428cadaf690a56a74f4c682f62326f959f06/src/flp.rs#L184-L190 The output is a single field element, rather than a vector, so this is already done. Am I missing something?

divergentdave commented 3 weeks ago

The fundamental difference is that we want to, for example, combine the range check and sum check values in the Histogram circuit via a random linear combination with query randomness values, rather joint randomness values. The way we've divided this up between abstraction layers in the spec is that we changed the circuit interface to return a vector of field elements, instead of a single field element, and then added the optional compression with query randomness values in the query algorithm of the FLP we define. (note that query randomness is passed to query(), but not passed into the validity circuit) Thus I think we should change the signature of Type::valid() to return a vector of field elements.

rozbb commented 3 weeks ago

Ah ok, will do

rozbb commented 2 weeks ago

Is there a nontrivial relation between VERIFIER_LEN and EVAL_OUTPUT_LEN? Here, there is 1 element added to the verifier list https://github.com/divviup/libprio-rs/blob/d2fe428cadaf690a56a74f4c682f62326f959f06/src/flp.rs#L446-L447 And later it is assumed that the number of eval elements is 1: https://github.com/divviup/libprio-rs/blob/d2fe428cadaf690a56a74f4c682f62326f959f06/src/flp.rs#L480-L491 This means that self.verifier_len() should probably be self.eval_output_len() + A where A is the sum of the gadget arities, rather than just 1 + A, right?

divergentdave commented 2 weeks ago

Here's the definition of verifier length in draft-10: https://www.ietf.org/archive/id/draft-irtf-cfrg-vdaf-10.html#figure-22. EVAL_OUTPUT_LEN does not affect the verifier length, because the query algorithm only uses one verifier field element to do a probabilistic check that the validity circuit output is the zero vector. See https://www.ietf.org/archive/id/draft-irtf-cfrg-vdaf-10.html#section-7.3.3.2-2.4.1 and https://www.ietf.org/archive/id/draft-irtf-cfrg-vdaf-10.html#section-7.3.3.2-3.

The overall breakdown of the verifier is one field element for the whole output vector, then for each gadget, as many field elements as the arity of the gadget for evaluations of the wire polynomials at a query randomness point, plus one more field element for the evaluation of the gadget polynomial at the same point. (so $1 + \sum_i{(A_i+1)}$)

rozbb commented 2 weeks ago

Ah, so to be clear: the thing to do here is to replace the verifier.push(validity) line with something that does a random dot product, and then pushes that to verifier, right?

rozbb commented 1 week ago

Poplar1: change handling of an edge case in the first round preparation message. (it's possible we may not need to do anything here) (draft-10)

I think you're right that this is moot. The prepare_next() function in the draft permits prep_msg to be None, but the Rust does not: https://github.com/divviup/libprio-rs/blob/a8e48e7548b4653ed85fab7bc4e51127611dc6ee/src/vdaf/poplar1.rs#L1167-L1172 where Poplar1PrepareMessage is: https://github.com/divviup/libprio-rs/blob/a8e48e7548b4653ed85fab7bc4e51127611dc6ee/src/vdaf/poplar1.rs#L512-L521

rozbb commented 1 week ago

Separate question: does the current codebase validate aggregation parameters at all? I'm trying to impl the draft-09 changes, but can't even find where the basic level replay check is done.

divergentdave commented 1 week ago

No, that isn't implemented at all currently. See #1133.

rozbb commented 1 week ago

Prio3: Soundness improvement in places where we use the Schwartz-Zippel lemma, both reducing vectors inside various circuits and reducing the vector output of the circuit. (draft-12)

Is there any change here outside of the FLP query specification?

divergentdave commented 1 week ago

Yes, we also changed circuits using the ParallelSum gadget to use more joint randomness values. They now use one new joint randomness value with each gadget call. This affected the SumVec, Histogram, and MultihotCountVec circuits. See cfrg/draft-irtf-cfrg-vdaf#436.

rozbb commented 1 week ago

Ok, in that case I think that subtask is completed. This is the valid() compression, and this is the parallel sum update

rozbb commented 1 week ago

Design question. I'm adding ctx strings to the codebase, and there are two big options here.

  1. I could modify all the methods in Aggregator to accept ctx: &[u8] as an input. This would allow users to call different methods with different contexts. However, I suspect that that's really not the intended use case. So...
  2. I could simply add a ctx field to everything that impls Aggregator, e.g., Prio3. So the trait would make no mention of ctx, but the constructors of aggregators would all require a ctx field.

If that's a fair description of the scenario, then I'm pro option 2. What do you think?

branlwyd commented 1 week ago

The only objection to #2 that I can think of: a DAP implementation might currently cache a VDAF instantiation keyed on the VDAF & parameters for cross-task use. Since the application context string used by DAP is task-specific, this implementation strategy would be disallowed if we took implementation strategy #2. That said, none of the DAP implementations I'm aware of do this, and all the VDAF instantiations I'm aware of are cheap enough to instantiate that I don't think we necessarily need to support this use-case.

Barring other objections, I like #2.

rozbb commented 1 week ago

Understood, that makes sense. Okay I’ll go with 2 for now.

branlwyd commented 1 week ago

Hmm, the collector's operation (roughly, calling Vdaf.unshard) does not use a context string; but if we moved the context string to the constructor, collector implementations would need to care about this parameter.

rozbb commented 1 week ago

Ah, yeah. And also thinking about it more, it seems that this ctx would be the only state that these structs carry, aside from num_shares. Maybe this isn’t ideal