Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adopt prio 0.14.0 #1673

Merged
merged 11 commits into from
Aug 16, 2023
Merged

Adopt prio 0.14.0 #1673

merged 11 commits into from
Aug 16, 2023

Conversation

tgeoghegan
Copy link
Contributor

The bulk of the changes here deal with the change to the representation of Prio3Histogram. Since prio 0.14.x implements VDAF-06, taking this change will break compatibility with DAP-04.

Part of #1669

@tgeoghegan tgeoghegan requested a review from a team as a code owner August 4, 2023 01:16
@tgeoghegan
Copy link
Contributor Author

tgeoghegan commented Aug 4, 2023 •
edited
Loading

I'm pretty sure this will also break divviup-api, which will have to update its Prio3Histogram representation (or will have to adapt how it deals with Prio3Histogram based on the Janus aggregator API version).

@tgeoghegan tgeoghegan mentioned this pull request Aug 2, 2023
Closed
11 tasks
@tgeoghegan tgeoghegan force-pushed the timg/prio-0.14 branch 2 times, most recently from ac66f60 to 00898ea Compare August 8, 2023 16:18
divergentdave
divergentdave reviewed Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I have one parameter naming comment on three different APIs

aggregator_core/src/datastore/tests.rs Outdated Show resolved Hide resolved
core/src/task.rs Outdated
#[derivative(Debug(format_with = "bucket_count"))]
buckets: Vec<u64>,
},
Prio3Histogram { buckets: usize },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for naming the parameter length instead of buckets here, in order to match with what VDAF uses.

integration_tests/src/divviup_api_client.rs Outdated
@@ -21,7 +21,7 @@ pub enum ApiVdaf {
/// Corresponds to Prio3Count
Count,
Histogram {
buckets: Vec<u64>,
buckets: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we rename the parameter to length in the aggregator API, we'll want to do the same in the control plane API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't get the end to end Prio3Histogram test working until we have a divviup-api that knows about the VDAF-06 representation of histograms (divviup/divviup-api#410), which opens up a can of worms of how divviup-api should deal with aggregators running different Janus versions. Anyway, for now, I disabled the relevant test.

interop_binaries/src/lib.rs Outdated
@@ -117,7 +117,7 @@ pub enum VdafObject {
length: NumberAsString<usize>,
},
Prio3Histogram {
buckets: Vec<NumberAsString<u64>>,
buckets: NumberAsString<usize>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used length instead of buckets in divergentdave/draft-dcook-ppm-dap-interop-test-design#41, again to align with https://cfrg.github.io/draft-irtf-cfrg-vdaf/draft-irtf-cfrg-vdaf.html#section-7.4.3-2.

tools/src/bin/collect.rs Outdated
help_heading = "VDAF Algorithm and Parameters"
)]
buckets: Option<Buckets>,
buckets: Option<usize>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we replace buckets with length elsewhere, we could eliminate this flag and simplify a bit.

@tgeoghegan
Adopt prio 0.14.0
7029084 
The bulk of the changes here deal with the change to the representation
of `Prio3Histogram`. Since `prio` 0.14.x implements VDAF-06, taking this
change will break compatibility with DAP-04.

Part of #1669
@tgeoghegan
Disable Daphne integration test
6c34b59
@tgeoghegan
disable divviup-ts tests
40421ca
@tgeoghegan
Fix histogram collect test
a16d04d
@tgeoghegan
fix collect tool args test
db6b438
@tgeoghegan
review feedback
81fbab5
@tgeoghegan
buckets->length
f8d4920
@tgeoghegan
disable divviup-api histogram test
4a01cc8
@tgeoghegan
Copy link
Contributor Author

I've renamed buckets to length everywhere and simplified the collect tool by collapsing --buckets into --length.

@tgeoghegan tgeoghegan requested a review from a team August 15, 2023 22:16
divergentdave
divergentdave approved these changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one more arithmetic issue in some transitional code.

integration_tests/src/divviup_api_client.rs Outdated Show resolved Hide resolved
integration_tests/tests/common/mod.rs Outdated Show resolved Hide resolved
@tgeoghegan tgeoghegan enabled auto-merge (squash) August 16, 2023 19:39
@tgeoghegan tgeoghegan merged commit 74aa957 into main Aug 16, 2023
@tgeoghegan tgeoghegan deleted the timg/prio-0.14 branch August 16, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divergentdave divergentdave divergentdave approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tgeoghegan @divergentdave