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

Small fix of zk rows for chunking #16057

Merged
merged 20 commits into from
Oct 8, 2024
Merged

Small fix of zk rows for chunking #16057

merged 20 commits into from
Oct 8, 2024

Conversation

querolita
Copy link
Member

@querolita querolita commented Sep 16, 2024

This is part of the investigation of the chunking bug in Pickles and o1js (RFCs here and here).

This branch is based on this branch by @Trivo25.

Explain your changes:

  • Discussions taking place in the o1js RFC PR suggest that the number of zero knowledge rows was not being correctly propagated to o1js. I started inspecting where the zk rows parameter was coming from and found a few places were it was not being correctly used.
  • in kimchi_bindings/wasm/src/plonk_verifier_index.rs there were two places where a hardcoded 3 was still being used to initialize w and the permutation polynomial. Instead, the updated zk_rows should be used:
    • res.set(zk_w(domain, index.zk_rows)).unwrap();
    • res.set(permutation_vanishing_polynomial(domain, index.zk_rows)).unwrap();
  • in src/lib/pickles/compile.ml and src/lib/pickles/step_branch_data.ml, the number of rows was not computed correctly.
    • I modified the calculation to become ((2 * (permuts + 1) * num_chunks) - 2 + permuts) / permuts instead.
    • According to the chunking RFC and Kimchi,
   zk_rows = (16 * (domain_size / max_poly_size) + 5) / 7
  • Substituted hardcoded occurrences of num_chunks and zk_rows with a constant default value for ease of debug (merged from this PR)

Explain how you tested your changes:

  • Before these changes were performed, the chunk4.ml used to fail with the error (Pickles.verify dlog_check).
    • The fix in the verifier index solves that error.
    • Nonetheless, the o1js tests still complain that there are not enough random rows to achieve zero-knowledge (expected: 4, got: 3). This suggests some miscalculation in the bindings that I still haven't found.
  • The second fix does not have an impact in all scenarios, but only in somewhat specific situations where the old formula and the new one return different values. An example is when num_chunks = 11, but this si not a power of 2. Another one is when num_chunks = 4 (basically anytime (16*c+6)/7 gives an integer number). There, the old computation gives 10 rows whereas this one gives 9. Having more rows is not a problem for zero knowledge-ness itself (it would be the other way round though). But this mismatch between the Kimchi side and the Pickles side can be a source of problems. Thus, it is better if we use the same formula on both sides. This, prevents us from getting another (Pickles.verify dlog_check) error.

TODO:

  • Find further places in the codebase where a hardcoded 3 might be used to instantiate variables instead of a proper variable default_zk_rows or similar.
  • Look into o1js-bindings and learn about the dependencies between libraries across our stack.

Partially addresses o1-labs/o1js#1806

Checklist:

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them
  • Closes #0000

@querolita querolita requested a review from a team as a code owner September 16, 2024 16:12
@dannywillems dannywillems self-requested a review September 16, 2024 17:37
@dannywillems
Copy link
Member

!ci-build-me

1 similar comment
@querolita
Copy link
Member Author

!ci-build-me

@querolita
Copy link
Member Author

!ci-build-me

1 similar comment
@querolita
Copy link
Member Author

!ci-build-me

@mrmr1993
Copy link
Member

Can you redirect this PR against compatible? I think these changes shouldn't change our existing circuits, so these can go in the next soft-fork upgrade (and can thus also be included in an upcoming o1js release).

@querolita querolita changed the base branch from develop to compatible September 23, 2024 15:17
@querolita querolita requested review from a team as code owners September 23, 2024 15:17
@querolita querolita changed the base branch from compatible to develop September 23, 2024 15:17
@querolita querolita changed the base branch from develop to compatible September 23, 2024 15:24
@querolita
Copy link
Member Author

!ci-build-me

1 similar comment
@querolita
Copy link
Member Author

!ci-build-me

@querolita
Copy link
Member Author

!ci-build-me

1 similar comment
@querolita
Copy link
Member Author

!ci-build-me

@mrmr1993 mrmr1993 requested a review from a team as a code owner September 25, 2024 07:54
@querolita
Copy link
Member Author

!ci-build-me

@querolita
Copy link
Member Author

!ci-build-me

@querolita
Copy link
Member Author

!ci-build-me

@querolita querolita merged commit ea896cc into compatible Oct 8, 2024
46 checks passed
@querolita querolita deleted the chunking/zkrows_fix branch October 8, 2024 16:54
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.

4 participants