review feedback

divviup · Aug 14, 2023 · 81fbab5 · 81fbab5
1 parent db6b438
commit 81fbab5
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 86 deletions.
diff --git a/core/src/task.rs b/core/src/task.rs
@@ -30,8 +30,8 @@ pub enum VdafInstance {
     Prio3Sum { bits: usize },
     /// A vector of `Prio3` sums.
     Prio3SumVec { bits: usize, length: usize },
-    /// A `Prio3` histogram.
-    Prio3Histogram { buckets: usize },
+    /// A `Prio3` histogram with `length` buckets in it.
+    Prio3Histogram { length: usize },
     /// A `Prio3` 16-bit fixed point vector sum with bounded L2 norm.
     #[cfg(feature = "fpvec_bounded_l2")]
     Prio3FixedPoint16BitBoundedL2VecSum { length: usize },
@@ -84,7 +84,10 @@ impl TryFrom<&taskprov::VdafType> for VdafInstance {
                 bits: *bits as usize,
             }),
             taskprov::VdafType::Prio3Histogram { buckets } => Ok(Self::Prio3Histogram {
-                buckets: buckets.clone(),
+                // taskprov does not yet deal with the VDAF-06 representation of histograms. In the
+                // meantime, we translate the bucket boundaries to a length that Janus understands.
+                // https://github.com/wangshan/draft-wang-ppm-dap-taskprov/issues/33
+                length: buckets.len() + 1, // +1 to account for the top bucket extending to infinity
             }),
             taskprov::VdafType::Poplar1 { bits } => Ok(Self::Poplar1 {
                 bits: *bits as usize,
@@ -94,10 +97,6 @@ impl TryFrom<&taskprov::VdafType> for VdafInstance {
     }
 }
 
-fn bucket_count(buckets: &Vec<u64>, f: &mut fmt::Formatter) -> fmt::Result {
-    write!(f, "[{} buckets]", buckets.len() + 1)
-}
-
 /// Internal implementation details of [`vdaf_dispatch`](crate::vdaf_dispatch).
 #[macro_export]
 macro_rules! vdaf_dispatch_impl_base {
@@ -171,8 +170,8 @@ macro_rules! vdaf_dispatch_impl_base {
                 $body
             }
 
-            ::janus_core::task::VdafInstance::Prio3Histogram { buckets } => {
-                let $vdaf = ::prio::vdaf::prio3::Prio3::new_histogram(2, *buckets)?;
+            ::janus_core::task::VdafInstance::Prio3Histogram { length } => {
+                let $vdaf = ::prio::vdaf::prio3::Prio3::new_histogram(2, *length)?;
                 type $Vdaf = ::prio::vdaf::prio3::Prio3Histogram;
                 const $VERIFY_KEY_LENGTH: usize = ::janus_core::task::PRIO3_VERIFY_KEY_LENGTH;
                 $body
@@ -765,14 +764,14 @@ mod tests {
             ],
         );
         assert_tokens(
-            &VdafInstance::Prio3Histogram { buckets: 6 },
+            &VdafInstance::Prio3Histogram { length: 6 },
             &[
                 Token::StructVariant {
                     name: "VdafInstance",
                     variant: "Prio3Histogram",
                     len: 1,
                 },
-                Token::Str("buckets"),
+                Token::Str("length"),
                 Token::U64(6),
                 Token::StructVariantEnd,
             ],

diff --git a/integration_tests/src/divviup_api_client.rs b/integration_tests/src/divviup_api_client.rs
@@ -21,7 +21,7 @@ pub enum ApiVdaf {
     /// Corresponds to Prio3Count
     Count,
     Histogram {
-        buckets: usize,
+        buckets: Vec<u64>,
     },
     Sum {
         bits: usize,
@@ -35,8 +35,14 @@ impl TryFrom<&VdafInstance> for ApiVdaf {
         match vdaf {
             VdafInstance::Prio3Count => Ok(ApiVdaf::Count),
             VdafInstance::Prio3Sum { bits } => Ok(ApiVdaf::Sum { bits: *bits }),
-            VdafInstance::Prio3Histogram { buckets } => {
-                Ok(ApiVdaf::Histogram { buckets: *buckets })
+            VdafInstance::Prio3Histogram { length } => {
+                // divviup-api does not yet support the new Prio3Histogram representation. Until it
+                // does, we synthesize fake bucket boundaries that will yield the number of buckets
+                // we want.
+                // https://github.com/divviup/divviup-api/issues/410
+                Ok(ApiVdaf::Histogram {
+                    buckets: (0..=length).collect(),
+                })
             }
             _ => Err(anyhow!("unsupported VDAF: {vdaf:?}")),
         }

diff --git a/interop_binaries/src/lib.rs b/interop_binaries/src/lib.rs
@@ -117,7 +117,7 @@ pub enum VdafObject {
         length: NumberAsString<usize>,
     },
     Prio3Histogram {
-        buckets: NumberAsString<usize>,
+        length: NumberAsString<usize>,
     },
     #[cfg(feature = "fpvec_bounded_l2")]
     Prio3FixedPoint16BitBoundedL2VecSum {
@@ -151,8 +151,8 @@ impl From<VdafInstance> for VdafObject {
                 length: NumberAsString(length),
             },
 
-            VdafInstance::Prio3Histogram { buckets } => VdafObject::Prio3Histogram {
-                buckets: NumberAsString(buckets),
+            VdafInstance::Prio3Histogram { length } => VdafObject::Prio3Histogram {
+                length: NumberAsString(length),
             },
 
             #[cfg(feature = "fpvec_bounded_l2")]
@@ -196,8 +196,8 @@ impl From<VdafObject> for VdafInstance {
                 length: length.0,
             },
 
-            VdafObject::Prio3Histogram { buckets } => {
-                VdafInstance::Prio3Histogram { buckets: buckets.0 }
+            VdafObject::Prio3Histogram { length } => {
+                VdafInstance::Prio3Histogram { length: length.0 }
             }
 
             #[cfg(feature = "fpvec_bounded_l2")]

diff --git a/messages/src/taskprov.rs b/messages/src/taskprov.rs
@@ -9,10 +9,7 @@ use prio::codec::{
     decode_u16_items, decode_u24_items, decode_u8_items, encode_u16_items, encode_u24_items,
     encode_u8_items, CodecError, Decode, Encode,
 };
-use std::{
-    fmt::{self, Debug, Formatter},
-    io::Cursor,
-};
+use std::{fmt::Debug, io::Cursor};
 
 /// Defines all parameters necessary to configure an aggregator with a new task.
 /// Provided by taskprov participants in all requests incident to task execution.
@@ -325,8 +322,9 @@ pub enum VdafType {
         bits: u8,
     },
     Prio3Histogram {
-        /// List of buckets.
-        #[derivative(Debug(format_with = "fmt_histogram"))]
+        /// Number of buckets in the histogram
+        // This may change as the taskprov draft adapts to VDAF-06
+        // https://github.com/wangshan/draft-wang-ppm-dap-taskprov/issues/33
         buckets: Vec<u64>,
     },
     Poplar1 {
@@ -342,10 +340,6 @@ impl VdafType {
     const POPLAR1: u32 = 0x00001000;
 }
 
-fn fmt_histogram(buckets: &Vec<u64>, f: &mut Formatter) -> Result<(), fmt::Error> {
-    write!(f, "num_buckets: {}", buckets.len())
-}
-
 impl Encode for VdafType {
     fn encode(&self, bytes: &mut Vec<u8>) {
         match self {

diff --git a/tools/src/bin/collect.rs b/tools/src/bin/collect.rs
@@ -320,21 +320,13 @@ struct Options {
         display_order = 0
     )]
     vdaf: VdafType,
-    /// Number of vector elements, for use with --vdaf=countvec and --vdaf=sumvec
+    /// Number of vector elements, when used with --vdaf=countvec and --vdaf=sumvec or number of
+    /// histogram buckets, when used with --vdaf=histogram
     #[clap(long, help_heading = "VDAF Algorithm and Parameters")]
     length: Option<usize>,
     /// Bit length of measurements, for use with --vdaf=sum and --vdaf=sumvec
     #[clap(long, help_heading = "VDAF Algorithm and Parameters")]
     bits: Option<usize>,
-    /// Comma-separated list of bucket boundaries, for use with --vdaf=histogram
-    #[clap(
-        long,
-        required = false,
-        num_args = 1,
-        action = ArgAction::Set,
-        help_heading = "VDAF Algorithm and Parameters"
-    )]
-    buckets: Option<usize>,
 
     #[clap(flatten)]
     query: QueryOptions,
@@ -412,40 +404,40 @@ where
         options.hpke_private_key.clone(),
     );
     let http_client = default_http_client().map_err(|err| Error::Anyhow(err.into()))?;
-    match (options.vdaf, options.length, options.bits, options.buckets) {
-        (VdafType::Count, None, None, None) => {
+    match (options.vdaf, options.length, options.bits) {
+        (VdafType::Count, None, None) => {
             let vdaf = Prio3::new_count(2).map_err(|err| Error::Anyhow(err.into()))?;
             run_collection_generic(parameters, vdaf, http_client, query, &())
                 .await
                 .map_err(|err| Error::Anyhow(err.into()))
         }
-        (VdafType::CountVec, Some(length), None, None) => {
+        (VdafType::CountVec, Some(length), None) => {
             let vdaf = Prio3::new_sum_vec(2, 1, length).map_err(|err| Error::Anyhow(err.into()))?;
             run_collection_generic(parameters, vdaf, http_client, query, &())
                 .await
                 .map_err(|err| Error::Anyhow(err.into()))
         }
-        (VdafType::Sum, None, Some(bits), None) => {
+        (VdafType::Sum, None, Some(bits)) => {
             let vdaf = Prio3::new_sum(2, bits).map_err(|err| Error::Anyhow(err.into()))?;
             run_collection_generic(parameters, vdaf, http_client, query, &())
                 .await
                 .map_err(|err| Error::Anyhow(err.into()))
         }
-        (VdafType::SumVec, Some(length), Some(bits), None) => {
+        (VdafType::SumVec, Some(length), Some(bits)) => {
             let vdaf =
                 Prio3::new_sum_vec(2, bits, length).map_err(|err| Error::Anyhow(err.into()))?;
             run_collection_generic(parameters, vdaf, http_client, query, &())
                 .await
                 .map_err(|err| Error::Anyhow(err.into()))
         }
-        (VdafType::Histogram, None, None, Some(buckets)) => {
-            let vdaf = Prio3::new_histogram(2, buckets).map_err(|err| Error::Anyhow(err.into()))?;
+        (VdafType::Histogram, Some(length), None) => {
+            let vdaf = Prio3::new_histogram(2, length).map_err(|err| Error::Anyhow(err.into()))?;
             run_collection_generic(parameters, vdaf, http_client, query, &())
                 .await
                 .map_err(|err| Error::Anyhow(err.into()))
         }
         #[cfg(feature = "fpvec_bounded_l2")]
-        (VdafType::FixedPoint16BitBoundedL2VecSum, Some(length), None, None) => {
+        (VdafType::FixedPoint16BitBoundedL2VecSum, Some(length), None) => {
             let vdaf: Prio3FixedPointBoundedL2VecSumMultithreaded<FixedI16<U15>> =
                 Prio3::new_fixedpoint_boundedl2_vec_sum_multithreaded(2, length)
                     .map_err(|err| Error::Anyhow(err.into()))?;
@@ -454,7 +446,7 @@ where
                 .map_err(|err| Error::Anyhow(err.into()))
         }
         #[cfg(feature = "fpvec_bounded_l2")]
-        (VdafType::FixedPoint32BitBoundedL2VecSum, Some(length), None, None) => {
+        (VdafType::FixedPoint32BitBoundedL2VecSum, Some(length), None) => {
             let vdaf: Prio3FixedPointBoundedL2VecSumMultithreaded<FixedI32<U31>> =
                 Prio3::new_fixedpoint_boundedl2_vec_sum_multithreaded(2, length)
                     .map_err(|err| Error::Anyhow(err.into()))?;
@@ -463,7 +455,7 @@ where
                 .map_err(|err| Error::Anyhow(err.into()))
         }
         #[cfg(feature = "fpvec_bounded_l2")]
-        (VdafType::FixedPoint64BitBoundedL2VecSum, Some(length), None, None) => {
+        (VdafType::FixedPoint64BitBoundedL2VecSum, Some(length), None) => {
             let vdaf: Prio3FixedPointBoundedL2VecSumMultithreaded<FixedI64<U63>> =
                 Prio3::new_fixedpoint_boundedl2_vec_sum_multithreaded(2, length)
                     .map_err(|err| Error::Anyhow(err.into()))?;
@@ -597,7 +589,6 @@ mod tests {
             vdaf: VdafType::Count,
             length: None,
             bits: None,
-            buckets: None,
             query: QueryOptions {
                 batch_interval_start: Some(1_000_000),
                 batch_interval_duration: Some(1_000),
@@ -682,30 +673,6 @@ mod tests {
             "1000".to_string(),
         ]);
 
-        let mut bad_arguments = base_arguments.clone();
-        bad_arguments.extend(["--vdaf=count".to_string(), "--buckets=4".to_string()]);
-        let bad_options = Options::try_parse_from(bad_arguments).unwrap();
-        assert_matches!(
-            run(bad_options).await.unwrap_err(),
-            Error::Clap(err) => assert_eq!(err.kind(), ErrorKind::ArgumentConflict)
-        );
-
-        let mut bad_arguments = base_arguments.clone();
-        bad_arguments.extend(["--vdaf=sum".to_string(), "--buckets=4".to_string()]);
-        let bad_options = Options::try_parse_from(bad_arguments).unwrap();
-        assert_matches!(
-            run(bad_options).await.unwrap_err(),
-            Error::Clap(err) => assert_eq!(err.kind(), ErrorKind::ArgumentConflict)
-        );
-
-        let mut bad_arguments = base_arguments.clone();
-        bad_arguments.extend(["--vdaf=countvec".to_string(), "--buckets=4".to_string()]);
-        let bad_options = Options::try_parse_from(bad_arguments).unwrap();
-        assert_matches!(
-            run(bad_options).await.unwrap_err(),
-            Error::Clap(err) => assert_eq!(err.kind(), ErrorKind::ArgumentConflict)
-        );
-
         let mut bad_arguments = base_arguments.clone();
         bad_arguments.extend(["--vdaf=countvec".to_string(), "--bits=3".to_string()]);
         let bad_options = Options::try_parse_from(bad_arguments).unwrap();
@@ -751,10 +718,7 @@ mod tests {
         }
 
         let mut bad_arguments = base_arguments.clone();
-        bad_arguments.extend([
-            "--vdaf=histogram".to_string(),
-            "--buckets=apple".to_string(),
-        ]);
+        bad_arguments.extend(["--vdaf=histogram".to_string(), "--length=apple".to_string()]);
         assert_eq!(
             Options::try_parse_from(bad_arguments).unwrap_err().kind(),
             ErrorKind::ValueValidation
@@ -793,7 +757,7 @@ mod tests {
         Options::try_parse_from(good_arguments).unwrap();
 
         let mut good_arguments = base_arguments.clone();
-        good_arguments.extend(["--vdaf=histogram".to_string(), "--buckets=4".to_string()]);
+        good_arguments.extend(["--vdaf=histogram".to_string(), "--length=4".to_string()]);
         Options::try_parse_from(good_arguments).unwrap();
 
         #[cfg(feature = "fpvec_bounded_l2")]
@@ -846,7 +810,6 @@ mod tests {
             vdaf: VdafType::Count,
             length: None,
             bits: None,
-            buckets: None,
             query: QueryOptions {
                 batch_interval_start: None,
                 batch_interval_duration: None,
@@ -886,7 +849,6 @@ mod tests {
             vdaf: VdafType::Count,
             length: None,
             bits: None,
-            buckets: None,
             query: QueryOptions {
                 batch_interval_start: None,
                 batch_interval_duration: None,

diff --git a/tools/tests/cmd/collect_fpvec_bounded_l2.trycmd b/tools/tests/cmd/collect_fpvec_bounded_l2.trycmd
@@ -33,7 +33,7 @@ Authorization:
           [env: DAP_AUTH_TOKEN=]
 
       --authorization-bearer-token <AUTHORIZATION_BEARER_TOKEN>
-          Authentication token for the "Authorization: Bearer ..." HTTP header, in base64
+          Authentication token for the "Authorization: Bearer ..." HTTP header
 
           [env: AUTHORIZATION_BEARER_TOKEN=]
 
@@ -52,14 +52,11 @@ VDAF Algorithm and Parameters:
           - fixedpoint64bitboundedl2vecsum: Prio3FixedPoint64BitBoundedL2VecSum
 
       --length <LENGTH>
-          Number of vector elements, for use with --vdaf=countvec and --vdaf=sumvec
+          Number of vector elements, when used with --vdaf=countvec and --vdaf=sumvec or number of histogram buckets, when used with --vdaf=histogram
 
       --bits <BITS>
           Bit length of measurements, for use with --vdaf=sum and --vdaf=sumvec
 
-      --buckets <BUCKETS>
-          Comma-separated list of bucket boundaries, for use with --vdaf=histogram
-
 Collect Request Parameters (Time Interval):
       --batch-interval-start <BATCH_INTERVAL_START>
           Start of the collection batch interval, as the number of seconds since the Unix epoch