Moving to reveal based aggregation #1302
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1302 +/- ##
==========================================
- Coverage 93.42% 93.32% -0.10%
==========================================
Files 209 208 -1
Lines 33856 33513 -343
==========================================
- Hits 31629 31276 -353
- Misses 2227 2237 +10 ☔ View full report in Codecov by Sentry. |
|
Ran a draft successfully with 100,000 rows: https://draft-mpc.vercel.app/query/view/slow-room2024-09-21T0455 |
|
Still seems to stall out on 1 million: https://draft-mpc.vercel.app/query/view/sole-scar2024-09-21T0501 |
|
stalls for 500k: https://draft-mpc.vercel.app/query/view/iron-hop2024-09-21T1917 |
|
I am testing a fix in #1306 - was able to get past 500k records, running 1M now: https://draft-mpc.vercel.app/query/view/frank-snow2024-09-23T0241 UPD: it passed, testing on larger inputs |
| chunk_size, | ||
| // The size of a single batch should not exceed the active work limit, | ||
| // otherwise it will stall | ||
| std::cmp::min(sh_ctx.active_work().get(), chunk_size), |
There was a problem hiding this comment.
@andyleiserson does this change make sense to you? I had to change it because otherwise I was getting stalls even on very small inputs, when active_work is larger than chunk_size.
There was a problem hiding this comment.
Is this expected to interact with #1307? I would expect active work (even at 32k) to be quite a bit less than the target proof size. I believe there are only ~50 multiplications per record in attribution? Which would mean a chunk size of 1M for a proof size of 50M.
There was a problem hiding this comment.
Sort of. If the chunk size is larger than active work in this case(I think it may only apply to validated_seq_join though), then we are going to stall, because we won't schedule enough futures to fill up the batch and validate_record will prevent seq_join from scheduling more futures.
so it is crucial that validated_seq_join populates the whole batch and validates it inside one active work window
There was a problem hiding this comment.
Oof yeah. I feel like we talked about this in July and then I clearly forgot about it.
I just looked at the original malicious share conversion code (before all the validate_record stuff) and it has two seq_joins, one before the reveal, and one after the reveal.
benjaminsavage
merged commit 1521f8b
into
private-attribution:main
No description provided.