Robust + Move breakdown to bucket generic to take any size even if not a power of 2 #794
Conversation
richajaindce
changed the title
src/protocol/prf_sharding/mod.rs
Outdated
| let result: Vec<_> = world | ||
| .semi_honest( | ||
| ( | ||
| get_bits::<Fp32BitPrime>(breakdown_key.try_into().unwrap(), Gf5Bit::BITS), |
There was a problem hiding this comment.
This is very hard to read. Can you make this easier to understand?
There was a problem hiding this comment.
extracted to a separate variable
src/protocol/prf_sharding/mod.rs
Outdated
| fn semi_honest_move_value_to_single_bucket_not_power_two() { | ||
| const BREAKDOWN_COUNT: usize = 50; |
There was a problem hiding this comment.
There is too much duplication between semi_honest_move_value_to_single_bucket and semi_honest_move_value_to_single_bucket_not_power_two.
I recommend just merging them into a single test that loops many times, trying many combinations of "num_breakdowns" (some of which are powers of two, and some of which are not), and value.
We do not have good test coverage as it is, since you're only testing 50, and only for a single value.
There was a problem hiding this comment.
Running a loop 10 times and combined them
richajaindce
changed the title
| let mut row_contribution = vec![value; 1 << BK::BITS]; | ||
|
|
||
| assert!(breakdown_count <= 1 << BK::BITS); | ||
| let mut row_contribution = vec![value; breakdown_count]; |
There was a problem hiding this comment.
It might be more efficient to initialize the vector with zero values instead, but that means putting "value" at index 0 as an extra step.
| fn semi_honest_move_value_to_single_bucket_out_of_range() { | ||
| const MAX_BREAKDOWN_COUNT: usize = 127; | ||
| for robust_for_breakdown_key_gt_count in [true, false] { | ||
| for _ in 1..10 { |
There was a problem hiding this comment.
We should not be running multiple iterations of a test like this.
adding tests for aggregation + move bucket