integrating bounded l2 norm fixed point vector sum types. #893

ooovi · 2023-01-08T13:56:38Z

As discussed here, this PR enables using our prio3 type for aggregating vectors of fixed-point numbers with janus. It mainly adds the enum cases and match branches required to dispatch on the 16-, 32- and 64-bit variants of fixed-point vector aggregation.

One notable thing I did is adding a helper enum to enable tagging the JSON from the upload request with the fixedpoint type in the end-to-end interop test only for our vectors, as the standard string representation of fixedpoint numbers does not carry that information.

Looking forward to your comments :)

tgeoghegan

This change looks sound to me, but the Janus team is debating how we want to handle this. For one thing, we may want to gate this VDAF behind a feature (much as the fixed point implementation in libprio-rs is gated behind feature experimental).

branlwyd

As Tim mentioned, let's gate this with crate features, disabled by default. (Tests can enable the features in order to allow testing this functionality by default.)

branlwyd · 2023-01-11T00:28:55Z

collector/src/lib.rs

@@ -822,7 +823,7 @@ mod tests {
        let collector = setup_collector(vdaf);

        let batch_interval = Interval::new(
-            Time::from_seconds_since_epoch(1_000_000),
+            Time::from_seconds_since_epoch(2_000_000), // TODO why???


Not sure offhand -- if you don't know why this change is necessary, I think it's OK to just drop the TODO.

I saw a test flake in this module yesterday, so it was probably random chance, unconnected to this timestamp. (I suspect we may be running into a backoff timer we need to tweak, but I haven't had a chance to dig into it yet)

ooovi · 2023-01-13T20:36:18Z

we are now hiding behind a feature.

Tests can enable the features in order to allow testing this functionality by default

we had to adjust the Dockerfile.interop and Dockerfile.interop_aggregator to enable the feature during tests. is that what you meant, or should we do something more?

branlwyd

we had to adjust the Dockerfile.interop and Dockerfile.interop_aggregator to enable the feature during tests. is that what you meant, or should we do something more?

I meant to modify the dependency in the dev-dependencies so that it enables the new feature, allowing the relevant tests to not be gated by the feature & thus tested by cargo test without additional flags. (My goal is to make a release binary not include these "experimental"/non-standardized VDAFs by default, but I think it is wise to test them by default.)

See

janus/aggregator/Cargo.toml

Line 98 in f8ad519

    
           janus_aggregator = { workspace = true, features = ["kube-openssl", "test-util"] }

for an example of the change to Cargo.toml -- there are a number of dev-dependencies throughout the workspace that enable the test-util feature.

collector/examples/collect.rs

branlwyd · 2023-01-13T23:45:36Z

collector/src/lib.rs

@@ -998,6 +1000,73 @@ mod tests {
        mocked_collect_complete.assert();
    }

+    #[tokio::test]
+    #[cfg(feature = "fpvec_bounded_l2")]
+    async fn successful_collect_prio3_fixedpoint_boundedl2_vec_sum() {


This test, for example, doesn't need to be gated by the feature if we enable the feature by default in tests.

branlwyd · 2023-01-13T23:57:25Z

interop_binaries/tests/end_to_end.rs

@@ -656,6 +659,228 @@ async fn e2e_prio3_count_vec() {
    }
 }

+#[tokio::test]
+#[cfg(feature = "fpvec_bounded_l2")]


These tests also don't need to be gated by the feature if we enable the feature by default in tests.

ooovi · 2023-01-17T16:54:27Z

I meant to modify the dependency in the dev-dependencies so that it enables the new feature, allowing the relevant tests to not be gated by the feature & thus tested by cargo test without additional flags. (My goal is to make a release binary not include these "experimental"/non-standardized VDAFs by default, but I think it is wise to test them by default.)

I did this (and the other changes you suggested) in the latest commit. Is there another way of enabling the feature on the containers used in the interop end-to-end test except passing the feature flag to the build command in the Dockerfile.interop and Dockerfile.interop_aggregator?

divergentdave · 2023-01-17T18:30:44Z

collector/src/lib.rs

@@ -822,7 +824,7 @@ mod tests {
        let collector = setup_collector(vdaf);

        let batch_interval = Interval::new(
-            Time::from_seconds_since_epoch(1_000_000),
+            Time::from_seconds_since_epoch(2_000_000),


This change can be backed out, #916 properly fixes this test.

branlwyd · 2023-01-17T22:10:31Z

I did this (and the other changes you suggested) in the latest commit. Is there another way of enabling the feature on the containers used in the interop end-to-end test except passing the feature flag to the build command in the Dockerfile.interop and Dockerfile.interop_aggregator?

Enabling the features via flags in Dockerfile.interop and Dockerfile.interop_aggregator is fine.

ooovi · 2023-01-24T10:42:44Z

I merged the current main branch, as there were conflicting changes. Also squashed into one commit. Could you run the workflow again? :)

ooovi · 2023-01-24T17:01:03Z

the workflow cannot be run on forks because they have no access to your github secrets. there seems to be an event that gives them access:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target
but i don't know if that's what you want to be doing in general:
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

branlwyd · 2023-01-25T00:36:55Z

I ran the tests locally; I am OK merging with that level of testing. I would prefer not to enable running with secrets against forks; perhaps if you will be creating many PRs, we can flip the appropriate bits so that you can create a branch on the main repository, and then we can enable you to run CI directly.

ooovi requested a review from a team as a code owner January 8, 2023 13:56

ooovi mentioned this pull request Jan 8, 2023

Integrating our custom prio type into janus #464

Closed

tgeoghegan reviewed Jan 9, 2023

View reviewed changes

branlwyd suggested changes Jan 11, 2023

View reviewed changes

branlwyd reviewed Jan 14, 2023

View reviewed changes

divergentdave reviewed Jan 17, 2023

View reviewed changes

branlwyd approved these changes Jan 17, 2023

View reviewed changes

ooovi force-pushed the fixedvec-pr branch from f641937 to d8c0dbf Compare January 24, 2023 10:40

integrating bounded l2 norm fixed point vector sum types.

d8c0dbf

branlwyd merged commit 3dbc0af into divviup:main Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrating bounded l2 norm fixed point vector sum types. #893

integrating bounded l2 norm fixed point vector sum types. #893

ooovi commented Jan 8, 2023 •

edited

Loading

tgeoghegan left a comment

branlwyd left a comment

branlwyd Jan 11, 2023

divergentdave Jan 11, 2023

ooovi commented Jan 13, 2023 •

edited

Loading

branlwyd left a comment

branlwyd Jan 13, 2023

branlwyd Jan 13, 2023

ooovi commented Jan 17, 2023

divergentdave Jan 17, 2023

branlwyd commented Jan 17, 2023

ooovi commented Jan 24, 2023 •

edited

Loading

ooovi commented Jan 24, 2023

branlwyd commented Jan 25, 2023

integrating bounded l2 norm fixed point vector sum types. #893

integrating bounded l2 norm fixed point vector sum types. #893

Conversation

ooovi commented Jan 8, 2023 • edited Loading

tgeoghegan left a comment

Choose a reason for hiding this comment

branlwyd left a comment

Choose a reason for hiding this comment

branlwyd Jan 11, 2023

Choose a reason for hiding this comment

divergentdave Jan 11, 2023

Choose a reason for hiding this comment

ooovi commented Jan 13, 2023 • edited Loading

branlwyd left a comment

Choose a reason for hiding this comment

branlwyd Jan 13, 2023

Choose a reason for hiding this comment

branlwyd Jan 13, 2023

Choose a reason for hiding this comment

ooovi commented Jan 17, 2023

divergentdave Jan 17, 2023

Choose a reason for hiding this comment

branlwyd commented Jan 17, 2023

ooovi commented Jan 24, 2023 • edited Loading

ooovi commented Jan 24, 2023

branlwyd commented Jan 25, 2023

ooovi commented Jan 8, 2023 •

edited

Loading

ooovi commented Jan 13, 2023 •

edited

Loading

ooovi commented Jan 24, 2023 •

edited

Loading