feat: general offline checker #164
Conversation
INT-1765 General Offline Checker
Create implementation of offline checker that has common parts from page r/w checker and vm memory checker Refactor page r/w checker and memory checker to use general offline checker |
| /// this bit indicates if (idx, clk) is strictly more than the one in the previous row | ||
| pub lt_bit: T, | ||
| /// a bit to indicate if this is a valid operation row | ||
| pub is_valid: T, |
There was a problem hiding this comment.
I prefer is_extra to indicate auxiliary rows. is_valid sounds like we're making a check for something 🤔
There was a problem hiding this comment.
@OsamaAlkhodairy I need it to be is_valid right now for the interactions for the VM memory chip (should send with count is_valid); we can change this once Jonathan's pr is merged
|
@bfan05 lmk when you finish addressing Osama's comments and I'll take a final pass. btw @bfan05 @OsamaAlkhodairy after we allow the interactions to have up to degree 2, were there any things to potentially change in OfflineChecker? |
There was a problem hiding this comment.
This can be merged after is_equal_data is fixed, and after @OsamaAlkhodairy gives the OK.
|
@OsamaAlkhodairy can you take a look |
Will do this tomorrow |
|
@bfan05 @jonathanpwang I ran a benchmark on one config on main~3 (main before InteractionBuilder was merged) vs this branch. Here are the results: main-results, general-offline-checker-results. It seems like the trace generation time went up by 5 seconds in this branch vs main~3. The time it took for the other parts of proving were similar but a little different, so the total proving time went up by only 2 seconds vs main~3. There are usually some fluctuations in each benchmark run, so I'm not totally sure about the significance of those results. I discussed with Ben some improvements for trace generation (basically to use borrowing instead of cloning index and data multiple times). Since the VM memory uses arrays of fixed length instead of vectors everywhere, we will have to clone only there (if we store the underlying idx and data as vectors to make trace generation for PageOfflineChecker faster), but I think that's okay since If we think the results of this benchmark are okay, we can merge this and add the mentioned optimizations in another PR (hopefully having the general OfflineChecker doesn't fundamentally make things slower). What do you think @jonathanpwang? |
|
@OsamaAlkhodairy I think we can merge -- let me handle the resolving of conflicts. We should then make some flamegraphs (turn off parallelism) to do a more detailed analysis. |
|
@bfan05 @OsamaAlkhodairy the unit test failed after I merged |
|
@jonathanpwang will see if i can fix |
|
@bfan05 it seems to be something about the OfflineChecker's receive, maybe the order of fields isn't right (although I tried to check it). Or I messed with the is valid? |
|
@jonathanpwang i sent you a message in slack; I found a fix but it increases number of interactions unnecessarily |
* first commit * chore: deduplicate is_equal indicator for IsEqualVec * wip * fix group by and other tests * chore: use aux instead of indexing into flatten for is_equal_data * feat: air and trace for general offline checker * wip * wip * wip * feat: general offline checker * feat: memory offline checker uses general offline checker * wip * feat: page r/w checker uses general offline checker * chore: remove rustc files * chore: address comments * chore: remove same_data and corresponding aux cols from general oc * chore: add file * chore: fix lints * chore: use p3-maybe-rayon * remove rayon * chore: fix MemoryChip * address comments * temp interaction fix * feat: is_receive column for offline checker * chore: remove is_internal column from PageOfflineChecker and use OfflineChecker is_receive instead --------- Co-authored-by: Jonathan Wang <31040440+jonathanpwang@users.noreply.github.com>
Resolves INT-1765
A bit bigger of a PR than I had anticipated but the overlapping logic for page r/w checker and vm memory checker have been combined.
Note: general offline checker does not constrain anything about data being equal across rows (only that the
same_databit is computed correctly). Each individual offline checker handles this according to its needs.