Proof batching for breakdown reveal aggregation #1323

andyleiserson · 2024-09-27T19:17:54Z

No description provided.

codecov · 2024-09-27T19:52:06Z

Codecov Report

Attention: Patch coverage is 99.09502% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.46%. Comparing base (726a549) to head (798061e).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
ipa-core/src/protocol/context/batcher.rs	98.24%	1 Missing ⚠️
ipa-core/src/protocol/ipa_prf/aggregation/mod.rs	92.85%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

andyleiserson · 2024-09-27T20:15:21Z

Draft run, 2M: https://draft-mpc.vercel.app/query/view/upset-poll2024-09-27T1918

akoshelev · 2024-09-27T20:34:44Z

ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs

@@ -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


Lets file an issue for that!

#1324. Thanks for filing.

akoshelev · 2024-09-27T20:37:45Z

ipa-core/src/protocol/context/batcher.rs

-            "validate_record called twice for record {record_id}",
-        );
+        if batch.pending_records.len() <= record_offset_in_batch {
+            batch


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)

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.

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

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.

andyleiserson · 2024-10-02T22:50:31Z

ipa-core/src/protocol/context/batcher.rs

@@ -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);


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.

andyleiserson · 2024-10-02T22:54:08Z

ipa-core/src/protocol/context/batcher.rs

-            "validate_record called twice for record {record_id}",
-        );
+        if batch.pending_records.len() <= record_offset_in_batch {
+            batch


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.

But avoid excessive memory allocations

andyleiserson · 2024-10-08T00:09:32Z

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.

andyleiserson · 2024-10-08T21:03:35Z

Rough accounting of added steps:

Aggregation
- Depth 4 for outer iteration (only need 2 for production, but 4 for tests with small TARGET_PROOF_SIZE)
- Depth 24 for inner iteration (aggregation chunk size must be <= 16M
- 101 steps: 32 each for add, sat_add/add, sat_add/select
- (4 - 1) × 24 × 101 = 7,272
Validation: 600 × 7 = 4,200

andyleiserson · 2024-10-09T21:25:21Z

vec_chunks is inefficient and ought to be improved
fix issue in draft

akoshelev · 2024-10-11T19:57:45Z

5M run: https://draft-mpc.vercel.app/query/view/tasty-talks2024-10-11T1822 (success)

akoshelev

I am going to merge this because it unblocks large inputs. We can address feedback in a follow up

akoshelev · 2024-10-08T16:43:56Z

ipa-core/src/protocol/basics/reveal.rs

@@ -1,3 +1,27 @@
+// Several of the reveal impls use distinct type parameters for the value being revealed


isn't it supposed to be a module description with //!?

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.

akoshelev · 2024-10-08T16:51:35Z

ipa-core/src/protocol/context/dzkp_validator.rs

+//
+// 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.


it seems that the code is doing the inverse - tests use power of two, everything else doesn't.

I don't understand -- I think the comment is explaining that tests user a power of two, protocols don't.

yea, you're right. I read that again and it does say tests need it to be a power of two

akoshelev · 2024-10-11T20:00:28Z

ipa-core/src/utils/vec_chunks.rs

+}
+
+impl<T: Clone> Iterator for VecChunks<T> {
+    type Item = Vec<T>;


I checked the usage and it seems that you could yield &[u8]. Maybe there are plans to use owned chunks later?

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.

akoshelev · 2024-10-11T20:01:59Z

ipa-core/src/utils/vec_chunks.rs

+}
+
+#[cfg(all(test, unit_test))]
+mod tests {


probably also want to test chunk_size = 0 to cause panic

akoshelev · 2024-10-11T20:03:37Z

ipa-core/src/protocol/context/dzkp_malicious.rs

@@ -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 {


we should explain usize::MAX usage too here

andyleiserson

Opened #1347 with the follow-up.

andyleiserson · 2024-10-14T20:11:30Z

ipa-core/src/protocol/context/dzkp_validator.rs

+//
+// 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.


I don't understand -- I think the comment is explaining that tests user a power of two, protocols don't.

andyleiserson · 2024-10-14T20:15:15Z

ipa-core/src/protocol/basics/reveal.rs

@@ -1,3 +1,27 @@
+// Several of the reveal impls use distinct type parameters for the value being revealed


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.

andyleiserson · 2024-10-14T20:23:29Z

ipa-core/src/utils/vec_chunks.rs

+}
+
+impl<T: Clone> Iterator for VecChunks<T> {
+    type Item = Vec<T>;


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

akoshelev reviewed Sep 27, 2024

View reviewed changes

akoshelev mentioned this pull request Sep 27, 2024

Add batching for breakdown reveal aggregation #1324

Closed

akoshelev reviewed Sep 27, 2024

View reviewed changes

andyleiserson commented Oct 2, 2024

View reviewed changes

andyleiserson added 5 commits October 7, 2024 16:56

Unlimited batch size for reveal aggregation

d5abd2d

But avoid excessive memory allocations

Batching for breakdown reveal aggregation

fcf5421

Reduce TARGET_PROOF_SIZE for tests

2fe5d41

Add a test for growing pending_records

3bba435

More large batch tests

6e9df6d

andyleiserson force-pushed the reveal-batch branch from 93c5537 to 6e9df6d Compare October 8, 2024 00:02

andyleiserson changed the title ~~Unlimited batch size for reveal aggregation~~ Proof batching for breakdown reveal aggregation Oct 8, 2024

andyleiserson added 2 commits October 7, 2024 18:17

Compact gate fixes

98c5d72

Fixing step count blow-up

6dc5db2

andyleiserson added 2 commits October 8, 2024 15:10

More test fixes

922b9f4

Merge remote-tracking branch 'ipa/main' into reveal-batch

ae99031

andyleiserson added 3 commits October 9, 2024 14:53

Fix a bug and adjust the test to catch it.

73d1e73

Keep semi-honest for shuttle, don't do shuttle for malicious

2d90d8f

Optimize vec_chunks

9779a02

andyleiserson force-pushed the reveal-batch branch from 772234b to 9779a02 Compare October 10, 2024 00:59

Merge from main

798061e

akoshelev approved these changes Oct 11, 2024

View reviewed changes

akoshelev merged commit 41b057c into private-attribution:main Oct 11, 2024
12 checks passed

andyleiserson added a commit to andyleiserson/ipa that referenced this pull request Oct 14, 2024

Review follow-up from private-attribution#1323

b86bf90

andyleiserson commented Oct 14, 2024

View reviewed changes

andyleiserson added a commit that referenced this pull request Oct 15, 2024

Merge pull request #1347 from andyleiserson/reveal-batch

08cc86d

Review follow-up from #1323

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof batching for breakdown reveal aggregation #1323

Proof batching for breakdown reveal aggregation #1323

andyleiserson commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

andyleiserson commented Sep 27, 2024

akoshelev Sep 27, 2024

andyleiserson Oct 2, 2024

akoshelev Sep 27, 2024

andyleiserson Oct 2, 2024

akoshelev Oct 2, 2024

andyleiserson Oct 8, 2024

andyleiserson Oct 2, 2024

andyleiserson Oct 2, 2024

andyleiserson commented Oct 8, 2024

andyleiserson commented Oct 8, 2024

andyleiserson commented Oct 9, 2024 •

edited

Loading

akoshelev commented Oct 11, 2024

akoshelev left a comment

akoshelev Oct 8, 2024

andyleiserson Oct 14, 2024

akoshelev Oct 8, 2024

andyleiserson Oct 14, 2024

akoshelev Oct 15, 2024

akoshelev Oct 11, 2024

andyleiserson Oct 14, 2024

akoshelev Oct 11, 2024

akoshelev Oct 11, 2024

andyleiserson left a comment

andyleiserson Oct 14, 2024

andyleiserson Oct 14, 2024

andyleiserson Oct 14, 2024

		@@ -1,3 +1,27 @@
		// Several of the reveal impls use distinct type parameters for the value being revealed

Proof batching for breakdown reveal aggregation #1323

Proof batching for breakdown reveal aggregation #1323

Conversation

andyleiserson commented Sep 27, 2024 • edited Loading

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

andyleiserson commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleiserson commented Oct 8, 2024

andyleiserson commented Oct 8, 2024

andyleiserson commented Oct 9, 2024 • edited Loading

akoshelev commented Oct 11, 2024

akoshelev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleiserson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleiserson commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

andyleiserson commented Oct 9, 2024 •

edited

Loading