-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proof batching for breakdown reveal aggregation #1323
Proof batching for breakdown reveal aggregation #1323
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
==========================================
+ Coverage 93.44% 93.46% +0.02%
==========================================
Files 209 210 +1
Lines 34512 34681 +169
==========================================
+ Hits 32249 32414 +165
- Misses 2263 2267 +4 ☔ View full report in Codecov by Sentry. |
@@ -541,7 +540,9 @@ where | |||
protocol: &Step::Aggregate, | |||
validate: &Step::AggregateValidate, | |||
}, | |||
aggregate_values_proof_chunk(B, usize::try_from(TV::BITS).unwrap()), | |||
// TODO: add batching for breakdown reveal aggregation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets file an issue for that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1324. Thanks for filing.
"validate_record called twice for record {record_id}", | ||
); | ||
if batch.pending_records.len() <= record_offset_in_batch { | ||
batch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth writing a test for this - it is important that we can resize this vec if we get over 50M (we definitely do in PRF)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it, maybe we don't want to take this change at all? I don't think it's much better than 1 << 30
, even with a test. What we really want is for the protocols to specify an appropriate batch size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. Protocols should be able to correctly estimate the number of records, compared to number of multiplications. It could be also true that we are ok giving them some slack.
We could start without resizing it and bring it back if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test. I added a smaller value of TARGET_PROOF_SIZE
for cfg(test)
. This test case would be fine with the default, but I think it's valuable to cover the multi-batch case for batched aggregation and for other protocols.
@@ -111,13 +116,14 @@ impl<'a, B> Batcher<'a, B> { | |||
fn get_batch_by_offset(&mut self, batch_offset: usize) -> &mut BatchState<B> { | |||
if self.batches.len() <= batch_offset { | |||
self.batches.reserve(batch_offset - self.batches.len() + 1); | |||
let pending_records_capacity = self.records_per_batch.min(TARGET_PROOF_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that TARGET_PROOF_SIZE
is not really valid here -- what we really want is TARGET_PROOF_SIZE / steps_in_this_protocol
. But it doesn't seem worth going down that road, because what we really really want is for records_per_batch
to have a useful value.
"validate_record called twice for record {record_id}", | ||
); | ||
if batch.pending_records.len() <= record_offset_in_batch { | ||
batch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it, maybe we don't want to take this change at all? I don't think it's much better than 1 << 30
, even with a test. What we really want is for the protocols to specify an appropriate batch size.
93c5537
to
6e9df6d
Compare
The first commit in this PR is the same as before, but I've also now included the changes to do proofs in batches for breakdown reveal aggregation. I think that I could split the commits into two different PRs if needed, but it's not too bad. |
Rough accounting of added steps:
|
|
772234b
to
9779a02
Compare
5M run: https://draft-mpc.vercel.app/query/view/tasty-talks2024-10-11T1822 (success) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to merge this because it unblocks large inputs. We can address feedback in a follow up
@@ -1,3 +1,27 @@ | |||
// Several of the reveal impls use distinct type parameters for the value being revealed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it supposed to be a module description with //!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a note that applies to several of the implementations here, but it doesn't seem sufficient to document the module. That said, if you prefer to make it a module doc comment even though it's incomplete, I'm fine doing that.
// | ||
// A smaller value is used for tests, to enable covering some corner cases with a | ||
// reasonable runtime. Some of these tests use TARGET_PROOF_SIZE directly, so for tests | ||
// it does need to be a power of two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that the code is doing the inverse - tests use power of two, everything else doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand -- I think the comment is explaining that tests user a power of two, protocols don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, you're right. I read that again and it does say tests need it to be a power of two
} | ||
|
||
impl<T: Clone> Iterator for VecChunks<T> { | ||
type Item = Vec<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the usage and it seems that you could yield &[u8]
. Maybe there are plans to use owned chunks later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started from "aggregate_values takes a stream of owned values" and ended up with this, but the reasoning wasn't sound, getting &[T]
from slice::chunks
is sufficient.
} | ||
|
||
#[cfg(all(test, unit_test))] | ||
mod tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably also want to test chunk_size = 0 to cause panic
@@ -38,7 +38,7 @@ impl<'a, B: ShardBinding> DZKPUpgraded<'a, B> { | |||
base_ctx: MaliciousContext<'a, B>, | |||
) -> Self { | |||
let records_per_batch = validator_inner.batcher.lock().unwrap().records_per_batch(); | |||
let active_work = if records_per_batch == 1 { | |||
let active_work = if records_per_batch == 1 || records_per_batch == usize::MAX { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should explain usize::MAX
usage too here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #1347 with the follow-up.
// | ||
// A smaller value is used for tests, to enable covering some corner cases with a | ||
// reasonable runtime. Some of these tests use TARGET_PROOF_SIZE directly, so for tests | ||
// it does need to be a power of two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand -- I think the comment is explaining that tests user a power of two, protocols don't.
@@ -1,3 +1,27 @@ | |||
// Several of the reveal impls use distinct type parameters for the value being revealed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a note that applies to several of the implementations here, but it doesn't seem sufficient to document the module. That said, if you prefer to make it a module doc comment even though it's incomplete, I'm fine doing that.
} | ||
|
||
impl<T: Clone> Iterator for VecChunks<T> { | ||
type Item = Vec<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started from "aggregate_values takes a stream of owned values" and ended up with this, but the reasoning wasn't sound, getting &[T]
from slice::chunks
is sufficient.
Review follow-up from #1323
No description provided.