search

celo-org / celo-bls-snark-rs

Implements SNARK-friendly BLS signatures
https://celo.org
Apache License 2.0
84 stars 21 forks source link

Update the codebase to depend on arkworks-rs release 0.3.0 #229

Open huitseeker opened 2 years ago

huitseeker commented 2 years ago

Description

Makes the code base depend on a fixed release of arkworks, rather than a git pointer w.r.t. which the code is obsolete[^1]

Tested

Existing tests run + using bls-crypto as a dependency (as described in #228 ) works.

Reviewers : please check entropy updates (due to changing the RNG used in tests), separated in the last commit.

Other changes

Some cosmetic Option / Result pattern simplifications, split in their own commit.

Related issues

/cc @kobigurk @nategraf

[^1]: any project that depends on a celo-bls-snark-rs crate will update those pointers in a way that's different than (and incompatible with) the celo-bls-snark-rs project itself, which has a checked in Cargo.lock. And pinning the dependencies to a git revision does not solve this, since revision-pinning is not transitive.

nategraf commented 2 years ago

@mstraka100 or @gtank it would be great to get your eyes on this

gtank commented 2 years ago

We can't currently do this upgrade because our setup uses circuit behavior from the (troublesome, interstitial) version of zexe/arkworks that existed when we first built Plumo. This was discussed a bit in https://github.com/celo-org/celo-bls-snark-rs/pull/225#issuecomment-920201504

huitseeker commented 2 years ago

Since #228 makes any code-level integration with this package difficult, would there be an approach that would help? For instance:

  1. Pinning on a fixed version of arkworks which transitively defines a deterministic set of dependencies rather than a git dependency (e.g. v0.2.0, v0.3.0, ...i.e. the current approach of this PR), but for the bls-crypto crate, only?
  2. vendoring the arkworks dependency crates at the required versions? Here's a gist of the versions the repo (under the current Cargo.lock) depends on.[^1]
  3. Overriding all dependencies to the above set of revisions using a workspace-level set of patches
  4. is there a method towards reviewing the circuit changes and eventually upgrading the library we could help with?

[^1]: sadly, just pinning revisions to this set in Cargo.tomls does not work, as the revisions themselves do not transitively fix the same set of pinned versions.

gtank commented 2 years ago

Thanks for raising this!

The core problem is that we committed to a version of the dependencies that weren't really meant to exist. arkworks 0.1.0 isn't even publicly tagged on GitHub, much less as a stable crate target. We're basically digging the old zexe monorepo plus an arbitrary number of patches out of its descendant libraries and of course that's kind of wacky! I agree it makes this library unsustainable to maintain or use -- we've spent way too much effort working around this problem (https://github.com/celo-org/celo-bls-snark-rs/pull/223 https://github.com/celo-org/celo-bls-snark-rs/pull/225 https://github.com/celo-org/celo-bls-snark-rs/pull/226 https://github.com/celo-org/celo-bls-snark-rs/pull/230 https://github.com/celo-org/plumo-prover/pull/2).

Unfortunately, we have the Groth16 setup we have. Before the recent holidays in the US I'd been thinking about vendoring the old arkworks (or even zexe) versions, and I still believe that's the best immediate option. Just pinning revisions doesn't work, as you say, and IIRC we tried a set of workspace patches but it didn't work for some reason I'm having trouble recalling at the moment. Maybe it's worth trying again? I would absolutely welcome a PR that does either successfully. I may be able to get to the vendoring myself soon otherwise, but there's a different project that I need to finish first.

I do think option 4 is the correct answer long term, but how to help make it happen is question for @kobigurk more than me.

huitseeker commented 2 years ago

See #231 (and comment) for why we can rule out patching for this purpose.