MinaProtocol / mina

Mina is a cryptocurrency protocol with a constant size blockchain, improving scaling while maintaining decentralization and security.
https://minaprotocol.com
Apache License 2.0
1.98k stars 525 forks source link

Remove custom registry for stubs package #15812

Open martyall opened 1 month ago

martyall commented 1 month ago

I discovered this issue in the process of working on #15802

We are currently using this custom registry to manage the rust dependencies for the kimchi_bindings/stubs package. This design decision was made in a crunch during the lead up to the berkely release but has several problems:

  1. It is currently impossible to upgrade the proof-systems submodule as-is without also upgrading the vendored deps submodule -- there are transitive dependency versions from proof-systems that are missing from the vendored set. It was mentioned in slack that proof-systems should also use this custom registry, which would have prevent this problem but would cause more imo.

  2. AFAIK the only way programmatic way to get around (1) is to regenerate the vendored deps, which defeats the purpose of doing this in the first place. Using manual resolution to continuously fix these kinds of problems is extremely tedious.

  3. the stubs package is the only package using this registry, e.g. the mina wasm package is not, nor is proof-systems`. If the outcome of this discussions is to keep the current design, we should probably just commit these vendored deps to the mina repo itself.

  4. Maybe most importantly, the same effects can be achieved using a lock file and Cargo.toml with patch directives and exact version pinning.

There were some comments in the slack thread about supply side attacks, which can be address by (4). But you should also consider that the current solution makes it difficult to upgrade when security vulnerabilities are discovered in our existing dependencies.

Now that the pressure is off, we should take the time to do this right.

dannywillems commented 1 month ago

Additional comment: the custom registry must be by branch (develop/berkeley/master) as they do use different dependencies.

dannywillems commented 1 month ago

There is also proof-systems-vendor that proof-systems depends on, and may cause future issues. It is already causing issues for PR (https://github.com/o1-labs/proof-systems/pull/2394).