Fix the saturated addition panic inside IPA #1286
Conversation
It required quite a bit of plumbing to make the compact gate stuff work
Otherwise, I can't make it work with compact gate `track_steps`.
It could be possible if we include stuff from `test_fixture` folder
conditionally, but `track_steps` does not support that currently.
|
draft run for 1M: https://draft-mpc.vercel.app/query/view/tubby-mix2024-09-19T0723 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1286 +/- ##
==========================================
+ Coverage 93.69% 93.70% +0.01%
==========================================
Files 203 202 -1
Lines 33270 33333 +63
==========================================
+ Hits 31171 31234 +63
Misses 2099 2099 ☔ View full report in Codecov by Sentry. |
| }, | ||
| ]; | ||
| let dp_params = DpMechanism::NoDp; | ||
| let padding_params = PaddingParameters::relaxed(); |
There was a problem hiding this comment.
doesn't this happen by default when testing?
There was a problem hiding this comment.
not sure what you mean by that. We have to supply this padding_params parameter for oprf_ipa, so we need to pick an explicit value (Rust does not support default values for function parameters)
There was a problem hiding this comment.
got it. I was thinking this was gated by feature flags, but I suppose that just means this is unavailable without that flag.
|
|
||
| #[test] | ||
| fn saturated_agg() { | ||
| const EXPECTED: &[u128] = &[0, 255, 255, 0, 0, 0, 0, 0]; |
There was a problem hiding this comment.
if we're trying to test going from 2 bytes to 4, shouldn't we test some much bigger numbers?
There was a problem hiding this comment.
the histogram value is 1 byte wide, so we definitely hit overflow with this test data which was our goal for this test. Any deviation from "standard" 8 bit trigger values requires transpose plumbing which seemed unnecessary here
| @@ -614,6 +614,8 @@ impl_transpose_shares_bool_to_ba!(BA32, 32, 256, test_transpose_shares_bool_to_b | |||
| impl_transpose_shares_bool_to_ba_small!(BA8, 8, 32, test_transpose_shares_bool_to_ba_8x32); | |||
| // added to support HV = BA32 to hold results when adding Binomial noise | |||
| impl_transpose_shares_bool_to_ba_small!(BA32, 32, 32, test_transpose_shares_bool_to_ba_32x32); | |||
| // // Usage: IPA test case for saturated additions | |||
| // impl_transpose_shares_bool_to_ba_small!(BA3, 3, 32, test_transpose_shares_bool_to_ba_3x32); | |||
There was a problem hiding this comment.
intentionally commented out?
There was a problem hiding this comment.
late night PR :( thanks for catching it!
|
https://draft-mpc.vercel.app/query/view/gory-waft2024-09-19T1639 - 1M rows with semi-honest security on this branch (passed) |
This fixes #1285. It was a silly error where the code used 32 bit step, but the compact gate declaration pointed to 16 bit. I spent the majority of the time actually reproducing this error in a unit test and making compact gate work with in-memory infra to allow people to write more of them.
So this PR essentially brings a test to reproduce the issue, runs more things with compact gate enabled and adds yet another approval step to the CI. It also fixes compile errors that we had for long time with compact gate and in memory infrastructure. Those errors stayed in the code because we never ran it with compact gate