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 #16464

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

Conversation

dannywillems
Copy link
Member

@dannywillems dannywillems commented Jan 13, 2025

TODO:


Revamping #16405. Note that this PR targets fix/compatibility, waiting for a fix in o1js.

This pull request has as a first goal to update Mina to use the master branch of proof-systems, instead of develop (which is going to be deleted when the related o1-labs/proof-systems#2868 is merged).

Reviewers can follow commits by commits. The commits are ordered in a way to understand the thought process.
Run these commands to check the build works locally:

git clone https://github.com/MinaProtocol/mina/ /tmp/test-mina
cd /tmp/test-mina
git submodule update --init --recursive
opam switch create ./ 4.14.0
eval $(opam env)
opam switch import opam.export
./scripts/pin-external-packages.sh
dune build src/lib/crypto
dune runtest src/lib/crypto
dune runtest src/lib/pickles
# cleaning and rebuilding.
dune clean
cargo clean
dune build src/lib/crypto

In addition to making the code in kimchi-bindings/stubs and kimchi-bindings/wasm compatible with the latest version of proof-systems, this work re-organizes the management of the Rust dependencies between the different Rust crates. Subsequent works will move all the Rust code into proof-systems (see the WIP o1-labs/proof-systems#2875 and o1-labs/proof-systems#2876).

Currently, the Rust dependencies of proof-systems/develop and the Rust dependencies involved in the crates kimchi-bindings/stubs and kimchi-bindings/wasm are separated. When doing this work, I encountered multiple conflicting version dependencies, which made the work more complicated to deliver. Therefore, I decided:

  • to keep proof-systems as a submodule (even though in the history there is an attempt to use it as a direct dependency).

  • to use Cargo workspaces to manage the dependencies of kimchi-bindings/stubs and kimchi-bindings/wasm together, in addition to proof-systems. It is done by defining once and for all the versions of the dependencies in a top-level Cargo.toml. The Cargo.toml in kimchi-bindings/stubs and kimchi-bindings/wasm. As proof-systems is a submodule, and defines also a workspace, we have to ignore it in the top-level Cargo.toml, and copying the dependencies required by proof-systems at the top-level Cargo.toml is required. A script checking that the versions are the same is in progress here.

  • to get rid of nightly for wasm. When trying to vendor the dependencies, I realized that it wasn't possible to vendor the std (required as we do build-std=panic_abort,std). I end up realizing that using nightly wasn't required, as already noticed in Rust 1.74 o1-labs/proof-systems#2875, and we could simply use the same Rust version).

  • to vendor all dependencies to keep a consistent Rust environment, coined "mina-rust-dependencies", managed as a submodule this time. It is mostly proof-systems-vendor renamed, and updated. A related PR exists here for compatible. We might need to create multiple branch later.

  • to add a Makefile target to (re-)vendor the Rust dependencies, update-rust-dependencies.

The cargo workspace will probably disappear soon as we're moving the Rust code in proof-systems.

@dannywillems dannywillems requested a review from a team as a code owner January 13, 2025 18:57
@dannywillems dannywillems changed the title Dw/revamp use latest master proof systems Use latest proof-systems/master in Mina Jan 13, 2025
@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 draft January 13, 2025 20:00
@dannywillems
Copy link
Member Author

Converting to draft while fixing nix.

@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems dannywillems force-pushed the dw/revamp-use-latest-master-proof-systems branch from 57a8bc4 to 109ec75 Compare January 23, 2025 13:00
@dannywillems dannywillems force-pushed the dw/revamp-use-latest-master-proof-systems branch from 109ec75 to 554c22e Compare January 23, 2025 13:08
@dannywillems dannywillems force-pushed the dw/revamp-use-latest-master-proof-systems branch from 554c22e to 5272287 Compare January 23, 2025 13:10
@dannywillems dannywillems force-pushed the dw/revamp-use-latest-master-proof-systems branch from e70f245 to fa6f100 Compare January 24, 2025 19:22
@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems dannywillems requested a review from volhovm January 24, 2025 19:34
@dannywillems dannywillems force-pushed the dw/revamp-use-latest-master-proof-systems branch from b7562b2 to cb8f9a9 Compare January 27, 2025 08:01
@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems dannywillems marked this pull request as ready for review January 27, 2025 13:32
Base automatically changed from fix/compatibility to compatible January 27, 2025 14:10
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.

Compiles well now 🎉

Only has some minor warnings: the config key profile.release.target one and the one on the screenshot:
screen_2025-01-27_001

@dannywillems
Copy link
Member Author

Do not merge, please. Fixing o1js/o1js-bindings first.

@dannywillems
Copy link
Member Author

I updated the o1js/bindings PR dep:

Build o1js + o1js-bindings (o1-labs/o1js#1990 and o1-labs/o1js-bindings#330)

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.

5 participants