Peer feedback

benches/oneshot/ipa.rs

@@ -9,7 +9,7 @@ use ipa::{
     ff::Fp32BitPrime,
     helpers::{query::IpaQueryConfig, GatewayConfig},
     test_fixture::{
-        ipa::{ipa_in_the_clear, test_ipa, test_oprf_ipa, IpaSecurityModel},
+        ipa::{ipa_in_the_clear, test_ipa, test_oprf_ipa, CappingOrder, IpaSecurityModel},
         EventGenerator, EventGeneratorConfig, TestWorld, TestWorldConfig,
     },
 };
@@ -123,11 +123,17 @@ async fn run(args: Args) -> Result<(), Error> {
     .take(args.query_size)
     .collect::<Vec<_>>();
 
+    let order = if args.oprf {
+        CappingOrder::CapOldestFirst
+    } else {
+        CappingOrder::CapMostRecentFirst
+    };
     let expected_results = ipa_in_the_clear(
         &raw_data,
         args.per_user_cap,
         args.attribution_window(),
         args.breakdown_keys,
+        &order,
     );
 
     let world = TestWorld::new_with(config.clone());

pre-commit

@@ -99,8 +99,12 @@ check "Concurrency tests" \
 check "IPA benchmark" \
     cargo bench --bench oneshot_ipa --no-default-features --features="enable-benches descriptive-gate" -- -n 62
 
+check "IPA OPRF benchmark" \
+    cargo bench --bench oneshot_ipa --no-default-features --features="enable-benches descriptive-gate" -- -n 62 --oprf
+
 check "Arithmetic circuit benchmark" \
     cargo bench --bench oneshot_arithmetic --no-default-features --features "enable-benches descriptive-gate"
 
 check "Sort benchmark" \
-    cargo bench --bench oneshot_sort --no-default-features --features="enable-benches descriptive-gate"
+    cargo bench --bench oneshot_sort --no-default-features --features="enable-benches descriptive-gate"
+

scripts/collect_steps.py

@@ -206,4 +206,5 @@ def oprf_steps(steps):
     sorted_steps = sorted(full_steps)
 
     for step in sorted_steps:
-        print(step)
+        print(step)
+

src/bin/report_collector.rs

@@ -247,6 +247,7 @@ async fn ipa(
             ipa_query_config.per_user_credit_cap,
             ipa_query_config.attribution_window_seconds,
             ipa_query_config.max_breakdown_key,
+            CappingOrder::CapOldestFirst,
         );
 
         // pad the output vector to the max breakdown key, to make sure it is aligned with the MPC results

src/protocol/ipa/mod.rs

@@ -464,7 +464,7 @@ pub mod tests {
         test_executor::{run, run_with},
         test_fixture::{
             input::GenericReportTestInput,
-            ipa::{ipa_in_the_clear, test_ipa, IpaSecurityModel},
+            ipa::{ipa_in_the_clear, test_ipa, CappingOrder, IpaSecurityModel},
             logging, EventGenerator, EventGeneratorConfig, Reconstruct, Runner, TestWorld,
             TestWorldConfig,
         },
@@ -815,6 +815,7 @@ pub mod tests {
                 per_user_cap,
                 ATTRIBUTION_WINDOW_SECONDS,
                 MAX_BREAKDOWN_KEY,
+                &CappingOrder::CapOldestFirst,
             );
 
             let config = TestWorldConfig {

src/protocol/prf_sharding/mod.rs

@@ -37,11 +37,11 @@ pub mod bucket;
 pub mod feature_label_dot_product;
 
 pub struct PrfShardedIpaInputRow<BK: GaloisField, TV: GaloisField, TS: GaloisField> {
-    prf_of_match_key: u64,
-    is_trigger_bit: Replicated<Gf2>,
-    breakdown_key: Replicated<BK>,
-    trigger_value: Replicated<TV>,
-    timestamp: Replicated<TS>,
+    pub prf_of_match_key: u64,
+    pub is_trigger_bit: Replicated<Gf2>,
+    pub breakdown_key: Replicated<BK>,
+    pub trigger_value: Replicated<TV>,
+    pub timestamp: Replicated<TS>,
 }
 
 impl<BK: GaloisField, TV: GaloisField, TS: GaloisField> PrfShardedIpaInputRow<BK, TV, TS> {

src/test_fixture/ipa.rs

@@ -48,6 +48,7 @@ pub fn ipa_in_the_clear(
     per_user_cap: u32,
     attribution_window: Option<NonZeroU32>,
     max_breakdown: u32,
+    order: &CappingOrder,
 ) -> Vec<u32> {
     // build a view that is convenient for attribution. match key -> events sorted by timestamp in reverse
     // that is more memory intensive, but should be faster to compute. We can always opt-out and
@@ -81,14 +82,14 @@ pub fn ipa_in_the_clear(
             &mut breakdowns,
             per_user_cap,
             attribution_window,
-            CappingOrder::CapOldestFirst,
+            order,
         );
     }
 
     breakdowns
 }
 
-enum CappingOrder {
+pub enum CappingOrder {
     CapOldestFirst,
     CapMostRecentFirst,
 }
@@ -101,7 +102,7 @@ fn update_expected_output_for_user<'a, I: IntoIterator<Item = &'a TestRawDataRec
     expected_results: &mut [u32],
     per_user_cap: u32,
     attribution_window_seconds: Option<NonZeroU32>,
-    order: CappingOrder,
+    order: &CappingOrder,
 ) {
     let within_window = |value: u64| -> bool {
         if let Some(window) = attribution_window_seconds {
@@ -119,7 +120,7 @@ fn update_expected_output_for_user<'a, I: IntoIterator<Item = &'a TestRawDataRec
         if record.is_trigger_report {
             pending_trigger_reports.push(record);
         } else if !pending_trigger_reports.is_empty() {
-            for trigger_report in pending_trigger_reports.into_iter() {
+            for trigger_report in pending_trigger_reports {
                 let time_delta_to_source_report = trigger_report.timestamp - record.timestamp;
 
                 // only count trigger reports that are within the attribution window
@@ -135,11 +136,9 @@ fn update_expected_output_for_user<'a, I: IntoIterator<Item = &'a TestRawDataRec
     }
 
     match order {
-        CappingOrder::CapOldestFirst => update_breakdowns(
-            attributed_triggers.into_iter(),
-            expected_results,
-            per_user_cap,
-        ),
+        CappingOrder::CapOldestFirst => {
+            update_breakdowns(attributed_triggers, expected_results, per_user_cap);
+        }
         CappingOrder::CapMostRecentFirst => update_breakdowns(
             attributed_triggers.into_iter().rev(),
             expected_results,
@@ -272,6 +271,7 @@ pub async fn test_oprf_ipa<F>(
                             is_trigger_bit: is_trigger_bit_share,
                             breakdown_key: single_row.breakdown_key,
                             trigger_value: single_row.trigger_value,
+                            timestamp: single_row.timestamp,
                         }
                     })
                     .collect::<Vec<_>>();
@@ -280,10 +280,16 @@ pub async fn test_oprf_ipa<F>(
                     _,
                     BreakdownKey,
                     TriggerValue,
+                    Timestamp,
                     F,
                     _,
                     Replicated<Gf2>,
-                >(ctx, sharded_input, user_cap.ilog2().try_into().unwrap())
+                >(
+                    ctx,
+                    sharded_input,
+                    user_cap.ilog2().try_into().unwrap(),
+                    config.attribution_window_seconds,
+                )
                 .await
                 .unwrap()
             },
@@ -295,6 +301,8 @@ pub async fn test_oprf_ipa<F>(
         .into_iter()
         .map(|v| u32::try_from(v.as_u128()).unwrap())
         .collect::<Vec<_>>();
+
+    //TODO(richaj): To be removed once the function supports non power of 2 breakdowns
     let _ = result.split_off(expected_results.len());
     assert_eq!(result, expected_results);
 }