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

[20241209] Merge develop into master #2868

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

dannywillems
Copy link
Member

@dannywillems dannywillems commented Dec 9, 2024

master commit: cdbc90c275dba3b70aff5fc2ad5a29ebee10c2e5
developer commit: 872c8f2d1cae6bc0e46d1cf79655eb4592792b52

The code related to Keccak constraints in Kimchi has been removed as it is not used in Berkeley/Kimchi.

Commands:

git checkout master
git checkout -b dw/merge-back-develop-in-master
git merge develop
git rm -rf circuit-construction/
git merge --continue
git push origin dw/merge-back-develop-in-master

In addition to that, I added some commits to make Mina happy.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 76.78%. Comparing base (2136d6c) to head (0c64141).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
kimchi/src/circuits/expr.rs 0.00% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2868      +/-   ##
==========================================
+ Coverage   76.61%   76.78%   +0.17%     
==========================================
  Files         255      253       -2     
  Lines       60855    60614     -241     
==========================================
- Hits        46622    46543      -79     
+ Misses      14233    14071     -162     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Keccak has been rewritten in o1js instead of being exported from Kimchi.
Therefore, the code has never been used.
The code related to witness generation has been kept to be used in the o1vm.
@dannywillems dannywillems marked this pull request as draft December 9, 2024 15:22
@dannywillems dannywillems marked this pull request as ready for review December 9, 2024 16:10
To have the same version used by o1js/mina.
Copy link
Member

@querolita querolita left a comment

Choose a reason for hiding this comment

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

After checking with Danny, I confirm that the Keccak parts that are being removed in this PR are not in use right now. Those constraints and gadgets were a first version of the ones that were then introduced for the o1vm project, and they were only being used inside an old-ish draft PR to check correctness of constraints and witness generation (which required a generic number of witness columns, as well as a full integration of that circuit gate in the proof system, also closed PRs). Other than that, we must not remove the keccak witness generation code inside kimchi, because that's the one being used in o1vm. Perhaps now it makes sense to move that inside the o1vm module.

I also checked that the changes I made to the bound on the zk rows for chunking has been correctly updated in this merge.

Copy link
Member

@volhovm volhovm left a comment

Choose a reason for hiding this comment

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

This seems about right -- I understand the only last 3 commits are "on top of master for compatibility with mina". They seem to be very benign.

This was referenced Dec 13, 2024
@dannywillems dannywillems force-pushed the dw/merge-back-develop-in-master branch 2 times, most recently from 6e6eadb to 335feeb Compare January 12, 2025 08:07
@dannywillems dannywillems force-pushed the dw/merge-back-develop-in-master branch from 2326314 to 0c64141 Compare January 12, 2025 08:47
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.

3 participants