Skip to content
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

Fix default check combinations #123

Merged
merged 8 commits into from
Sep 29, 2023
Merged

Conversation

mmagician
Copy link
Member

@mmagician mmagician commented Sep 28, 2023

Description

The trait-default implementation of check_combinations was incorrect. This was not detected because all of the implemented schemes have a custom implementation of this method. We are going to use the default method in Ligero though.

Description of the bug(s):

  1. The key in poly_evals: BTreeMap was of type (query_label, point), whereas the first element in the key tuple should have been a poly_label. This was causing map access here to fail with MissingEvaluation.
  • poly_query_set was keyed only with poly_label, but held multiple query values with the same poly_label. Iterating over its keys was be done in an arbitrary order.
  • The array we zip it with, evals, is returned calling values() on BTreeMap, the values are returned sorted by key of type (poly_label, point).
  • So the two constructed arrays were potentially in non-matching orders. To address this, we map the iterated poly labels as (poly_label, point), and collect first, so before it gets combined with a sorted vector of evaluations, it will itself get sorted on the correct key (poly_label, point).
    (2 might seem obvious in retrospect)

As mentioned, this is impossible to validate against the current codebase where all PCS have a custom implementation. To showcase the bug, I've sandwiched the actual fix in between 2 commits, where I first remove the custom impl from marlin_pc and later add it back after the fix. To verify the fix, you can check out 57ac1ab, and run cargo test marlin::marlin_pc::tests::full_end_to_end_equation_test -> should fail.
Then checkout the fix commit dcffaec, run the same command, and verify the tests pass now.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@mmagician mmagician marked this pull request as draft September 28, 2023 14:14
@mmagician
Copy link
Member Author

Hmm I might have messed something up, I converted to draft for now

@mmagician mmagician marked this pull request as ready for review September 28, 2023 14:37
@mmagician
Copy link
Member Author

mmagician commented Sep 28, 2023

Ok, I was a bit rash on squashing two loops into one - it turns out we need to first collect before zipping to ensure correct ordering by the new key. Ready now ✅

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the nits.

mmagician and others added 2 commits September 29, 2023 10:24
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
@mmagician mmagician requested a review from a team as a code owner September 29, 2023 14:37
@Pratyush
Copy link
Member

By the way @mmagician, in a separate PR, do you mind adding comments explaining what this code is doing? (Since you touched this code most recently you have the best idea.)

@Pratyush Pratyush added this pull request to the merge queue Sep 29, 2023
Merged via the queue into master with commit 0e9d907 Sep 29, 2023
4 checks passed
@Pratyush Pratyush deleted the fix-default-check-combinations branch September 29, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants