Make active_work at least records_per_batch #1316
Conversation
| } | ||
|
|
||
| impl<'a> DZKPUpgraded<'a> { | ||
| pub(super) fn new( | ||
| validator_inner: &Arc<MaliciousDZKPValidatorInner<'a>>, | ||
| base_ctx: MaliciousContext<'a>, | ||
| ) -> Self { | ||
| // Adjust active_work to be at least records_per_batch. If it is less, we will |
There was a problem hiding this comment.
we should probably log the fact that we've adjusted it - maybe at the debug level
|
|
||
| #[tokio::test] | ||
| async fn select_semi_honest() { | ||
| test_select_semi_honest::<BA8>().await; |
There was a problem hiding this comment.
is it worth testing it for for weird types like BA3 and BA7 as well?
There was a problem hiding this comment.
Yes, I think so. I had BA4 but took it out because it's not boolean_vector! (not for any good reason other than we haven't needed it). But I think it is worth having a less-than-one-byte case, and maybe even adding a new BA type so we can cover the between-one-and-two-bytes case.
| (1usize<<log_count, 1usize<<log_multiplication_amount) | ||
| } | ||
| fn record_count_strategy() -> impl Strategy<Value = usize> { | ||
| prop_oneof![1usize..=512, (1usize..=9).prop_map(|i| 1usize << i)] |
There was a problem hiding this comment.
I am being stupid and it is 12pm now - is it really testing
There was a problem hiding this comment.
What it does is:
- 50% of the time it picks a random power of two between 2 and 512 (9 possible values)
- 50% of the time it picks a random integer between 1 and 512 (512 possible values)
Two reasons to focus on powers of two, although I didn't put a huge amount of thought into this (e.g. it just occurred to me to add
- Because we plan to restrict batch sizes to powers of two.
- As the count increases, it makes sense to me to sample the space more sparsely, there's relatively less interesting about testing batch sizes 399, 400, and 401 than about testing batch sizes 7, 8, and 9.
I think it is clearer to have the two prop_oneof cases on different lines, but rustfmt insisted on a single line.
| // Adjust active_work to be at least records_per_batch. The MAC validator | ||
| // currently configures the batcher with records_per_batch = active_work, which | ||
| // makes this adjustment a no-op, but we do it this way to match the DZKP validator. | ||
| let records_per_batch = batch.lock().unwrap().records_per_batch(); |
There was a problem hiding this comment.
would it be better to assert this to make sure we don't miss the misalignment between MAC and ZKP?
There was a problem hiding this comment.
Sure, I'm fine with that.
| @@ -217,7 +217,7 @@ impl<'a, F: ExtendableField> BatchValidator<'a, F> { | |||
|
|
|||
| // TODO: Right now we set the batch work to be equal to active_work, | |||
| // but it does not need to be. We can make this configurable if needed. | |||
| let records_per_batch = ctx.active_work().get().min(total_records.get()); | |||
| let records_per_batch = ctx.active_work().get(); | |||
There was a problem hiding this comment.
Reducing if larger than total_records may have been necessary with an earlier version of the batcher, but the current version should take care of that internally, so I removed it here. No relation to the rest of these changes though.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1316 +/- ##
==========================================
+ Coverage 93.45% 93.46% +0.01%
==========================================
Files 207 207
Lines 33451 33553 +102
==========================================
+ Hits 31260 31361 +101
- Misses 2191 2192 +1 ☔ View full report in Codecov by Sentry. |
Adjust active_work to be at least records_per_batch. If it is less, we will stall, since every record in the batch remains incomplete until the batch is validated.
This change also modifies an existing DZKP proptest to be more extensive. I believe the test is capable of exposing both the hang when active_work is less than batch size (when run without the fix in this PR), and the hang due to read_size not dividing active work (not yet fixed).
I moved a couple tests that are doing DZKPs with various BA types from
secret_sharing::vector::implsto the DZKP module. My intent was to adapt them to cover the interaction with record size, but I haven't done that yet. (The move is in a separate commit, although in the current state I don't think there's interesting diff on top of the move anyways.)