From d9b487b812f4155bcd3c2148181d9772d4f80d56 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Wed, 13 Nov 2024 11:56:01 -0500 Subject: [PATCH 1/3] Added Aggregator::is_agg_param_valid method, representing the is_valid method in the spec --- src/vdaf.rs | 5 +++++ src/vdaf/dummy.rs | 4 ++++ src/vdaf/poplar1.rs | 47 +++++++++++++++++++++++++++++++++++++++++++++ src/vdaf/prio2.rs | 5 +++++ src/vdaf/prio3.rs | 5 +++++ 5 files changed, 66 insertions(+) diff --git a/src/vdaf.rs b/src/vdaf.rs index 8dc8b2fe1..907dd6fc1 100644 --- a/src/vdaf.rs +++ b/src/vdaf.rs @@ -298,6 +298,11 @@ pub trait Aggregator: Vda agg_param: &Self::AggregationParam, output_shares: M, ) -> Result; + + /// Validates an aggregation parameter with respect to all previous aggregaiton parameters used + /// for the same input share + #[must_use] + fn is_agg_param_valid(cur: &Self::AggregationParam, prev: &[Self::AggregationParam]) -> bool; } /// Aggregator that implements differential privacy with Aggregator-side noise addition. diff --git a/src/vdaf/dummy.rs b/src/vdaf/dummy.rs index 6903cd547..5b969bc19 100644 --- a/src/vdaf/dummy.rs +++ b/src/vdaf/dummy.rs @@ -166,6 +166,10 @@ impl vdaf::Aggregator<0, 16> for Vdaf { } Ok(aggregate_share) } + + fn is_agg_param_valid(_cur: &Self::AggregationParam, _prev: &[Self::AggregationParam]) -> bool { + true + } } impl vdaf::Client<16> for Vdaf { diff --git a/src/vdaf/poplar1.rs b/src/vdaf/poplar1.rs index 514ed3906..ae5759b8f 100644 --- a/src/vdaf/poplar1.rs +++ b/src/vdaf/poplar1.rs @@ -17,6 +17,7 @@ use crate::{ use bitvec::{prelude::Lsb0, vec::BitVec}; use rand_core::RngCore; use std::{ + collections::BTreeSet, convert::TryFrom, fmt::Debug, io::{Cursor, Read}, @@ -1245,6 +1246,52 @@ impl, const SEED_SIZE: usize> Aggregator output_shares, ) } + + /// Validates that no aggregation parameter with the same level as `cur` has been used with the + /// same input share before. `prev` contains the aggregation parameters used for the same input. + /// + /// # Panics + /// Panics if `prev.len() > 1` + fn is_agg_param_valid(cur: &Poplar1AggregationParam, prev: &[Poplar1AggregationParam]) -> bool { + assert!( + prev.len() <= 1, + "list of previous aggregation parameters must be of size 0 or 1" + ); + + // Helper function to determine the prefix of `input` at `last_level`. + fn get_ancestor(input: &IdpfInput, this_level: u16, last_level: u16) -> IdpfInput { + input.prefix((this_level - last_level) as usize) + } + + // Exit early if this is the first time + if prev.is_empty() { + return true; + } + + // Unpack this agg param and the last one in the list + let Poplar1AggregationParam { level, prefixes } = cur; + let Poplar1AggregationParam { + level: last_level, + prefixes: last_prefixes, + } = prev.last().as_ref().unwrap(); + let last_prefixes_set = BTreeSet::from_iter(last_prefixes); + + // Check that the level increased. + if level <= last_level { + return false; + } + + // Check that prefixes are suffixes of the last level's prefixes. + for prefix in prefixes { + let last_prefix = get_ancestor(prefix, *level, *last_level); + if !last_prefixes_set.contains(&last_prefix) { + // Current prefix not a suffix of last level's prefixes. + return false; + } + } + + true + } } impl, const SEED_SIZE: usize> Collector for Poplar1 { diff --git a/src/vdaf/prio2.rs b/src/vdaf/prio2.rs index ba725d90d..6481a5b1e 100644 --- a/src/vdaf/prio2.rs +++ b/src/vdaf/prio2.rs @@ -325,6 +325,11 @@ impl Aggregator<32, 16> for Prio2 { Ok(agg_share) } + + fn is_agg_param_valid(_cur: &Self::AggregationParam, _prev: &[Self::AggregationParam]) -> bool { + // Nothing to do. There are no aggregation parameters (it's the unit type) + true + } } impl Collector for Prio2 { diff --git a/src/vdaf/prio3.rs b/src/vdaf/prio3.rs index f6e42d972..49faa0ff8 100644 --- a/src/vdaf/prio3.rs +++ b/src/vdaf/prio3.rs @@ -1441,6 +1441,11 @@ where Ok(agg_share) } + + fn is_agg_param_valid(_cur: &Self::AggregationParam, _prev: &[Self::AggregationParam]) -> bool { + // Nothing to do. There are no aggregation parameters (it's the unit type) + true + } } #[cfg(feature = "experimental")] From ab323711c6857652b52c236a29d177193b548821 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Thu, 14 Nov 2024 16:03:58 -0500 Subject: [PATCH 2/3] Added unit tests for Poplar1::is_agg_param_valid --- src/vdaf.rs | 2 +- src/vdaf/poplar1.rs | 90 ++++++++++++++++++++++++++++++++------------- src/vdaf/prio2.rs | 6 +-- src/vdaf/prio3.rs | 6 +-- 4 files changed, 71 insertions(+), 33 deletions(-) diff --git a/src/vdaf.rs b/src/vdaf.rs index 907dd6fc1..2c68f2e40 100644 --- a/src/vdaf.rs +++ b/src/vdaf.rs @@ -300,7 +300,7 @@ pub trait Aggregator: Vda ) -> Result; /// Validates an aggregation parameter with respect to all previous aggregaiton parameters used - /// for the same input share + /// for the same input share. `prev` MUST be sorted from least to most recently used. #[must_use] fn is_agg_param_valid(cur: &Self::AggregationParam, prev: &[Self::AggregationParam]) -> bool; } diff --git a/src/vdaf/poplar1.rs b/src/vdaf/poplar1.rs index ae5759b8f..3eda7d48b 100644 --- a/src/vdaf/poplar1.rs +++ b/src/vdaf/poplar1.rs @@ -1249,27 +1249,19 @@ impl, const SEED_SIZE: usize> Aggregator /// Validates that no aggregation parameter with the same level as `cur` has been used with the /// same input share before. `prev` contains the aggregation parameters used for the same input. - /// - /// # Panics - /// Panics if `prev.len() > 1` + /// `prev` MUST be sorted from least to most recently used. fn is_agg_param_valid(cur: &Poplar1AggregationParam, prev: &[Poplar1AggregationParam]) -> bool { - assert!( - prev.len() <= 1, - "list of previous aggregation parameters must be of size 0 or 1" - ); - - // Helper function to determine the prefix of `input` at `last_level`. - fn get_ancestor(input: &IdpfInput, this_level: u16, last_level: u16) -> IdpfInput { - input.prefix((this_level - last_level) as usize) - } - - // Exit early if this is the first time + // Exit early if there are no previous aggregation params to compare to, i.e., this is the + // first time the input share has been processed if prev.is_empty() { return true; } // Unpack this agg param and the last one in the list - let Poplar1AggregationParam { level, prefixes } = cur; + let Poplar1AggregationParam { + level: cur_level, + prefixes: cur_prefixes, + } = cur; let Poplar1AggregationParam { level: last_level, prefixes: last_prefixes, @@ -1277,20 +1269,15 @@ impl, const SEED_SIZE: usize> Aggregator let last_prefixes_set = BTreeSet::from_iter(last_prefixes); // Check that the level increased. - if level <= last_level { + if cur_level <= last_level { return false; } - // Check that prefixes are suffixes of the last level's prefixes. - for prefix in prefixes { - let last_prefix = get_ancestor(prefix, *level, *last_level); - if !last_prefixes_set.contains(&last_prefix) { - // Current prefix not a suffix of last level's prefixes. - return false; - } - } - - true + // Check that current prefixes are extensions of the last level's prefixes. + cur_prefixes.iter().all(|cur_prefix| { + let last_prefix = cur_prefix.prefix(*last_level as usize); + last_prefixes_set.contains(&last_prefix) + }) } } @@ -2026,6 +2013,57 @@ mod tests { assert_matches!(err, CodecError::Other(_)); } + // Tests Poplar1::is_valid() functionality. This unit tests is translated from + // https://github.com/cfrg/draft-irtf-cfrg-vdaf/blob/a4874547794818573acd8734874c9784043b1140/poc/tests/test_vdaf_poplar1.py#L187 + #[test] + fn agg_param_validity() { + // The actual Poplar instance doesn't matter for the parameter validity tests + type V = Poplar1; + + // Helper function for making aggregation params + fn make_agg_param(bitstrings: &[&[u8]]) -> Result { + Poplar1AggregationParam::try_from_prefixes( + bitstrings + .iter() + .map(|v| { + let bools = v.iter().map(|&b| b != 0).collect::>(); + IdpfInput::from_bools(&bools) + }) + .collect(), + ) + } + + // Test `is_valid` returns False on repeated levels, and True otherwise. + let agg_params = [ + make_agg_param(&[&[0], &[1]]).unwrap(), + make_agg_param(&[&[0, 0]]).unwrap(), + make_agg_param(&[&[0, 0], &[1, 0]]).unwrap(), + ]; + assert!(V::is_agg_param_valid(&agg_params[0], &[])); + assert!(V::is_agg_param_valid(&agg_params[1], &agg_params[..1])); + assert!(!V::is_agg_param_valid(&agg_params[2], &agg_params[..2])); + + // Test `is_valid` accepts level jumps. + let agg_params = [ + make_agg_param(&[&[0], &[1]]).unwrap(), + make_agg_param(&[&[0, 1, 0], &[0, 1, 1], &[1, 0, 1], &[1, 1, 1]]).unwrap(), + ]; + assert!(V::is_agg_param_valid(&agg_params[1], &agg_params[..1])); + + // Test `is_valid` rejects unconnected prefixes. + let agg_params = [ + make_agg_param(&[&[0]]).unwrap(), + make_agg_param(&[&[0, 1, 0], &[0, 1, 1], &[1, 0, 1], &[1, 1, 1]]).unwrap(), + ]; + assert!(!V::is_agg_param_valid(&agg_params[1], &agg_params[..1])); + + // Test that the `Poplar1AggregationParam` constructor rejects unsorted and duplicate + // prefixes. + assert!(make_agg_param(&[&[1], &[0]]).is_err()); + assert!(make_agg_param(&[&[1, 0, 0], &[0, 1, 1]]).is_err()); + assert!(make_agg_param(&[&[0, 0, 0], &[0, 1, 0], &[0, 1, 0]]).is_err()); + } + #[derive(Debug, Deserialize)] struct HexEncoded(#[serde(with = "hex")] Vec); diff --git a/src/vdaf/prio2.rs b/src/vdaf/prio2.rs index 6481a5b1e..680f09ea7 100644 --- a/src/vdaf/prio2.rs +++ b/src/vdaf/prio2.rs @@ -326,9 +326,9 @@ impl Aggregator<32, 16> for Prio2 { Ok(agg_share) } - fn is_agg_param_valid(_cur: &Self::AggregationParam, _prev: &[Self::AggregationParam]) -> bool { - // Nothing to do. There are no aggregation parameters (it's the unit type) - true + /// Returns `true` iff `prev.is_empty()` + fn is_agg_param_valid(_cur: &Self::AggregationParam, prev: &[Self::AggregationParam]) -> bool { + prev.is_empty() } } diff --git a/src/vdaf/prio3.rs b/src/vdaf/prio3.rs index 49faa0ff8..f0d482dea 100644 --- a/src/vdaf/prio3.rs +++ b/src/vdaf/prio3.rs @@ -1442,9 +1442,9 @@ where Ok(agg_share) } - fn is_agg_param_valid(_cur: &Self::AggregationParam, _prev: &[Self::AggregationParam]) -> bool { - // Nothing to do. There are no aggregation parameters (it's the unit type) - true + /// Returns `true` iff `prev.is_empty()` + fn is_agg_param_valid(_cur: &Self::AggregationParam, prev: &[Self::AggregationParam]) -> bool { + prev.is_empty() } } From 8c89cad6f4bc1ccf2448b675d90898341e1bcbd0 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Thu, 14 Nov 2024 19:33:11 -0500 Subject: [PATCH 3/3] Comment typo Co-authored-by: David Cook --- src/vdaf/poplar1.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vdaf/poplar1.rs b/src/vdaf/poplar1.rs index 3eda7d48b..1d396e4da 100644 --- a/src/vdaf/poplar1.rs +++ b/src/vdaf/poplar1.rs @@ -2013,7 +2013,7 @@ mod tests { assert_matches!(err, CodecError::Other(_)); } - // Tests Poplar1::is_valid() functionality. This unit tests is translated from + // Tests Poplar1::is_valid() functionality. This unit test is translated from // https://github.com/cfrg/draft-irtf-cfrg-vdaf/blob/a4874547794818573acd8734874c9784043b1140/poc/tests/test_vdaf_poplar1.py#L187 #[test] fn agg_param_validity() {