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

Use latest proof-systems/master in Mina #16405

Open
wants to merge 24 commits into
base: compatible
Choose a base branch
from

Conversation

dannywillems
Copy link
Member

@dannywillems dannywillems commented Dec 9, 2024

Unifying the branches of proof-systems. proof-systems/master wasn't used in Mina, even though all the recent changes are happening there.

Other related PR:

o1js cannot be updated right now because mina/compatible is current incompatible with o1js/main, see o1-labs/o1js#1944. @Trivo25 is working on it.

Explain your changes:
From commit b6f5f579c78bd9b4b6ab44b666a9183f74a360fa

git submodule update --init --force --recursive
# First update proof-systems
cd src/lib/crypto/proof-systems
git checkout  8c0ed39e4797dafc6e5e3486dc4fcebec4543623
cd ../
git add proof-systems
git commit -m "Deps/proof-systems: bump up"
# removing the kimchi stubs library for now
# 1. edit .gitsubmodules by removing the kimchi-stubs dir
# 2. edit .git/config to remove kimchi stubs vendor
# 3. Remove git submodules
git rm --cached src/lib/crypto/kimchi_bindings/stubs/kimchi-stubs-vendors
rm -rf src/lib/crypto/kimchi_bindings/stubs/kimchi-stubs-vendors
# 4. Commit changes
# Check that git submodules work
git submodule update --init --force --recursive

From now, we will start editing Cargo files and update the files in Mina that have breaking changes. We will start by trying to compile libraries by libraries. First, we start with the libraries located in src/lib/crypto, which I guess would be the ones with the most breaking changes.

kimchi_stubs_vendors have been removed as some people were strongly against keeping it.

@dannywillems dannywillems requested a review from a team as a code owner December 9, 2024 10:40
@dannywillems dannywillems marked this pull request as draft December 9, 2024 10:40
@dannywillems dannywillems force-pushed the dw/use-latest-master-proof-systems branch 3 times, most recently from da66e68 to c279a30 Compare December 9, 2024 13:56
@dannywillems
Copy link
Member Author

!ci-build-me

1 similar comment
@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems dannywillems marked this pull request as ready for review December 9, 2024 16:10
@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems
Copy link
Member Author

dannywillems commented Dec 9, 2024

Mina seems all green and proof-systems too, but o1js is unhappy. Checking tomorrow.

To 8c0ed39e4797dafc6e5e3486dc4fcebec4543623
It seems that clippy is not run in this crate. We should activate it later.
Output given when simply executing `cargo build --release` in the directory
Use 25c111e2a35fb9e953e3b6378a3982d85bc6bac3, commit removing the unused gate
types related to Keccak, instead of propagating the changes in the whole
codebase.
Bringing back ocaml_str, which is used in gen_scalars.ml
The method is used in src/lib/pickles to generate scalars equations, and the
variables are used from the cache to generate the OCaml code.
Moving secp256k1 into dev-dependencies to avoid compiling it when generating wasm library
Removing secp256k1 as a dependency as it was simply used in testing in
proof-systems.
It has been previously added by mistake.
Removing secp256k1 as a dependency as it was simply used in testing in
proof-systems.
It has been previously added by mistake.
To have the same version than proof-systems
Use the commit than downgrades wasm-bindgen to 0.2.87
@dannywillems dannywillems force-pushed the dw/use-latest-master-proof-systems branch from a121afe to 2cc26fa Compare December 10, 2024 12:25
@dannywillems
Copy link
Member Author

!ci-nightly-me

@dannywillems
Copy link
Member Author

!ci-build-me

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.

lgtm

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.

Oh wait, I don't see the removal of the keccak constraints in this PR, is that expected?

@dannywillems
Copy link
Member Author

Oh wait, I don't see the removal of the keccak constraints in this PR, is that expected?

Should there be any in Mina?

@querolita
Copy link
Member

@dannywillems What I am saying is that it seems like the updated proof-systems submodule is not removing the keccak stuff, in the same way that the proof-systems counterpart PR did. Thus I am wondering if anything went wrong with the merge or if there's still another PR to be done once the proof-systems PR gets merged and the commit is updated.

@dannywillems
Copy link
Member Author

@dannywillems What I am saying is that it seems like the updated proof-systems submodule is not removing the keccak stuff, in the same way that the proof-systems counterpart PR did. Thus I am wondering if anything went wrong with the merge or if there's still another PR to be done once the proof-systems PR gets merged and the commit is updated.

The proof-systems commit is c54f985d5fe28dc5f34a275884bee486e0861521, which is the head of !2868, which includes the changes.

@querolita
Copy link
Member

querolita commented Dec 11, 2024

@dannywillems Okay, everything's ok, I was looking at the wrong commit. Reaffirming my accept.

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.

Great! Overall almost all the changes are renamings and simple refactorings. I didn't see anything that was significantly affecting the protocol logic, which is (I hope) good.

I'd suggest to run nightlies on this PR before merging. It's quite an important step and it'd be nice to test everything end-to-end!

( field
"0x0000000000000000000000000000000000000000000000000000000000000002"
, 4 )
* field
Copy link
Member

Choose a reason for hiding this comment

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

This ... seems a tiny bit suspicious. I see that 0x2^4 = 0x10, so it seems that the values are still the same. I guess I just don't understand why?

@martyall martyall mentioned this pull request Dec 13, 2024
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