Hybrid padding #1381
Hybrid padding #1381
Conversation
| matchkey_cardinality_cap, | ||
| oprf_padding_sensitivity, | ||
| } => { | ||
| let oprf_padding = |
There was a problem hiding this comment.
do we still plan to use OPRFPaddingDp or it will be replicated as well for hybrid?
There was a problem hiding this comment.
This gets somewhat confusing, because we use the term "OPRF" to name both the entire ipa v2 protocol as well as the pseudo random value that the match key is converted into.
In this instance, we are padding values so that the process of converting match keys into OPRF values is differentially private (same as previously), as opposed to padding values for aggregation or breakdown key reveal. In that sense, I think this naming still makes sense, and actually "HybridPadding" would be less clear (at least once the old protocol is purged.)
Happy for input on this - but that was my thinking. I can add a comment here to make that more clear as well.
There was a problem hiding this comment.
Looking at this more closely, this is actually just poor naming of the existing OPRFPaddingDp struct, which itself isn't tied specifically to the OPRF, and is used for aggregation padding as well. I'll open a new PR to make that naming more generic. It still makes sense to me to use the OPRFPadding struct here since we're still using it to provide DP for the match_key to OPRF conversion.
| @@ -130,6 +131,71 @@ pub trait Paddable { | |||
| Self: Sized; | |||
| } | |||
|
|
|||
| impl<BK, V> Paddable for IndistinguishableHybridReport<BK, V> | |||
There was a problem hiding this comment.
This trait could be made better...
There was a problem hiding this comment.
can you take a look and see if this impl is cleaner?
| @@ -451,6 +518,31 @@ mod tests { | |||
| Ok(input) | |||
| } | |||
|
|
|||
| pub async fn set_up_apply_dp_padding_pass_for_indistinguishable_reports< | |||
There was a problem hiding this comment.
The formatting on this function is crazy. Is it because the name of the function is kinda long? Does it make sense to move this to a separate module? Maybe that way you can reduce the size of the function name since it will have a more specific context in which is used.
There was a problem hiding this comment.
just the standard formatter. I don't think it makes sense to move to a separate module, all the padding functions live in this module.
| } => { | ||
| let oprf_padding = | ||
| OPRFPaddingDp::new(oprf_epsilon, oprf_delta, oprf_padding_sensitivity)?; | ||
| for cardinality in 1..=matchkey_cardinality_cap { |
There was a problem hiding this comment.
I attempted to get this all into a single padding_input_rows.extend but I ran into a conflict between needing to use move so that cardinality didn't outlive the closure and a mutable rng not working with the move.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1381 +/- ##
==========================================
+ Coverage 93.55% 93.63% +0.07%
==========================================
Files 223 223
Lines 37003 37351 +348
==========================================
+ Hits 34620 34973 +353
+ Misses 2383 2378 -5 ☔ View full report in Codecov by Sentry. |
This implements the
Paddabletrait forIndistinguishableHybridReportso that it can work withapply_dp_padding. It also implementsTestIndistinguishableHybridReportandReconstructto support the tests.Note that this is fairly repetitive to the OPRF implementation. If we wanted to keep that around, we'd want to make this more generic, but given our intention is to delete that, it's copied and modified to work directly with
IndistinguishableHybridReport.