Deduplicate VdafConfig and VDAF constructor arguments (#4711) - #4793
Conversation
As David pointed out (see #4711), building a client or collector required stating the VDAF's parameters twice: once to `Prio3::new_histogram(2, 12, 4)` and again as `VdafConfig::Prio3Histogram { length: 12, chunk_length: 4 }`. And a mismatch wasn't not a compile error, just a silent HPKE AAD byte-identity bug that surfaces as a decryption failure at the aggregator. This adds `ConfiguredVdaf<V>` -- it holds a concrete VDAF alongside the `VdafConfig` describing it, with `prio3_*` constructors that build both from one set of parameters. It also adds `Client::builder_from_configured_vdaf`, `Collector::builder_from_configured_vdaf`, and `CollectorBuilder::from_configured_vdaf`. `with_vdaf_config` remains for the configs with no concrete VDAF to derive from. Note this uncovered two existing mismatches: the client's `setup_client` and the collector's `setup_collector` test helpers both hardcoded `VdafConfig::Prio3Count` while callers passed Prio3Sum, Prio3Histogram, and `dummy::Vdaf`. Oops.
…-and-new-histogram
Avoid the need for clients to import janus_core
|
|
||
| /// Creates a [`ClientBuilder`] from the required set of DAP task parameters and a | ||
| /// [`ConfiguredVdaf`], which supplies both the VDAF and its [`VdafConfig`]. | ||
| pub fn builder_from_configured_vdaf( |
There was a problem hiding this comment.
This should be the main entry point that people use, so I suggest renaming methods so that this is Client::builder(), and the one above as Client::builder_with_custom_vdaf() or similar.
There was a problem hiding this comment.
Oh, I should have thought of that -- but I am so glad you did!
There was a problem hiding this comment.
We could add a VdafConfig to the arguments here, since it is always required, and it should be tightly coupled to the vdaf: V argument. Then, we could get rid of the with_vdaf_config() method on the builder.
There was a problem hiding this comment.
Similarly, I suggest adding a VdafConfig argument to this, renaming builder() to builder_with_custom_vdaf() or similar, and builder_from_configured_vdaf() to builder().
3 participants
As David pointed out (see #4711), building a client or collector required stating the VDAF's parameters twice: once to
Prio3::new_histogram(2, 12, 4)and again asVdafConfig::Prio3Histogram { length: 12, chunk_length: 4 }. And a mismatch wasn't not a compile error, just a silent HPKE AAD byte-identity bug that surfaces as a decryption failure at the aggregator.This adds
ConfiguredVdaf<V>-- it holds a concrete VDAF alongside theVdafConfigdescribing it, withprio3_*constructors that build both from one set of parameters.It also adds
Client::builder_from_configured_vdaf,Collector::builder_from_configured_vdaf, andCollectorBuilder::from_configured_vdaf.with_vdaf_configremains for the configs with no concrete VDAF to derive from.Note this uncovered two existing mismatches: the client's
setup_clientand the collector'ssetup_collectortest helpers both hardcodedVdafConfig::Prio3Countwhile callers passed Prio3Sum, Prio3Histogram, anddummy::Vdaf. Oops.