-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Prio3 dispatch logic, remove unneeded variants #749
Conversation
@@ -208,113 +198,6 @@ mod test { | |||
|
|||
test_versions! { roundtrip_report } | |||
|
|||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT the only difference between this test and the previous is that the prio_draft09
crate is used instead of prio
. This was necessary because we needed the right version of Prio3Sum. We avoid this by using a VDAF for testing that is supported by both versions (Prio2).
#[test] | ||
fn finish_agg_job_vdaf_draft09() { | ||
let version = DapVersion::Draft09; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let t = | ||
AggregationJobTest::new(TEST_VDAF, HpkeKemId::X25519HkdfSha256, DapVersion::Draft09); | ||
let mut reports = t.produce_reports(vec![DapMeasurement::U64(1), DapMeasurement::U64(1)]); | ||
fn agg_job_init_req_skip_vdaf_prep_error(version: DapVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test isn't specific to a DAP version.
Self::Prio3Draft09(Prio3Config::SumVecField64MultiproofHmacSha256Aes128 { .. }) | ||
| Self::Prio2 { .. } | ||
| Self::Prio3(..) => VdafVerifyKey::L32([0; 32]), | ||
Self::Prio3Draft09(..) => VdafVerifyKey::L16([0; 16]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we dropped the old variants of Prio3, the seed size is 32 bytes for every Prio3 variant we support :)
83fde97
to
adcf494
Compare
/// max_measurement]`. | ||
Sum { max_measurement: u64 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that parameter change from VDAF-08 to VDAF-13.
f36a7b6
to
d290a79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me, in conjunction with the stacked PRs. Closing mine
Move sharding, preparation, and unsharding dispatchers to `Prio3Config`. Not all variants are supported by all DAP versions, so pass the version to each dispatcher in order to resolve DAP and Prio3 version compatibility. (Note Prio3 is not API-compatible across versions.) The following variants are supported in DAP-13: * Prio3Count * Prio3Sum * Prio3SumVec * Prio3Histogram The following variants are supported in DAP-09: * Prio3SumVecField64MultiproofHmacSha256Aes128 Modify the taskprov provisioning logic accordingly, by removing support for the following VDAFs in DAP-13: * Prio3SumVecField64MultiproofHmacSha256Aes128 * Pine32HmacSha256Aes128 * Pine64HmacSha256Aes128 These VDAFs need to be upgraded for VDAF-13 compatibility.
Yes indeed, nice catch :)
|
d290a79
to
4d3dd54
Compare
Partially addresses #698.
Alternative to #748.
Move sharding, preparation, and unsharding dispatchers to
Prio3Config
.Not all variants are supported by all DAP versions, so pass the version
to each dispatcher in order to resolve DAP and Prio3 version
compatibility. (Note Prio3 is not API-compatible across versions.)
The following variants are supported in DAP-13:
The following variants are supported in DAP-09:
Modify the taskprov provisioning logic accordingly, by removing support
for the following VDAFs in DAP-13:
These VDAFs need to be upgraded for VDAF-13 compatibility.