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

Can't compile on latest master commit #57

Closed
jasl opened this issue Aug 31, 2023 · 12 comments
Closed

Can't compile on latest master commit #57

jasl opened this issue Aug 31, 2023 · 12 comments

Comments

@jasl
Copy link

jasl commented Aug 31, 2023

Because Substrate is dependent on this repo via git, Cargo update dependencies often touch git commits unexpectedly.
So I found the issue on my Substrate project, and then I found it's reproducible on the repo as well.

Rust version 1.71.1

warning: unused import: `Fr`
  --> bandersnatch_vrfs/src/ring.rs:17:44
   |
17 | use ark_ed_on_bls12_381_bandersnatch::{Fq, Fr, SWConfig, SWAffine};
   |                                            ^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused imports: `HashToCurveError`, `curve_maps`, `map_to_curve_hasher::MapToCurveBasedHasher`
  --> bandersnatch_vrfs/src/lib.rs:14:15
   |
14 |     hashing::{HashToCurveError, curve_maps, map_to_curve_hasher::MapToCurveBasedHasher}, // HashToCurve
   |               ^^^^^^^^^^^^^^^^  ^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unused imports: `Zero`, `borrow::BorrowMut`
  --> bandersnatch_vrfs/src/lib.rs:16:15
   |
16 | use ark_std::{borrow::BorrowMut, Zero, vec::Vec, rand::RngCore};   // io::{Read, Write}
   |               ^^^^^^^^^^^^^^^^^  ^^^^

error[E0061]: this function takes 3 arguments but 2 arguments were supplied
  --> bandersnatch_vrfs/src/ring.rs:34:5
   |
34 |     PiopParams::setup(domain, &mut rng)
   |     ^^^^^^^^^^^^^^^^^------------------ an argument of type `ark_ec::short_weierstrass::Affine<BandersnatchConfig>` is missing
   |
note: expected `Affine<BandersnatchConfig>`, found `&mut ChaCha20Rng`
  --> bandersnatch_vrfs/src/ring.rs:34:31
   |
34 |     PiopParams::setup(domain, &mut rng)
   |                               ^^^^^^^^
   = note:         expected struct `ark_ec::short_weierstrass::Affine<BandersnatchConfig>`
           found mutable reference `&mut ChaCha20Rng`
note: associated function defined here
  --> /Users/jasl/.cargo/git/checkouts/ring-proof-e9e49c3c86c409a2/8657210/ring/src/piop/params.rs:31:12
   |
31 |     pub fn setup(domain: Domain<F>, h: Affine<Curve>, seed: Affine<Curve>) -> Self {
   |            ^^^^^
help: provide the argument
   |
34 |     PiopParams::setup(domain, /* ark_ec::short_weierstrass::Affine<BandersnatchConfig> */, /* ark_ec::short_weierstrass::Affine<BandersnatchConfig> */)
   |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

For more information about this error, try `rustc --explain E0061`.
warning: `bandersnatch_vrfs` (lib) generated 3 warnings
error: could not compile `bandersnatch_vrfs` (lib) due to previous error; 3 warnings emitted
warning: build failed, waiting for other jobs to finish...
@burdges
Copy link
Collaborator

burdges commented Aug 31, 2023

Can we point substrate to c86ebd4 for now?

I'll have flavors I could publish to crates.io this week, but then substrate would need to be updated to them.

@burdges
Copy link
Collaborator

burdges commented Aug 31, 2023

Or we can force push this repo back to that commit and branch off everything else?

@burdges
Copy link
Collaborator

burdges commented Aug 31, 2023

I've fixed it in 48b8507

Arkworks inherited zcash's usage of std::io for serialization, but they wanted to be no_std, so they created ark_std::io. This was necessary because rust-lang avoided doing a core::io for silly reasons. This strategic mistake by rust-lang is also why scale exists.

It's likely these errors were caused by the tests using one flavor when run from the repo root, but another flavor when run from the crate itself. We need a proper CI that catches this stuff, but still a lot of flux here.

Anyways please reopen if this does not fix the problem.

@burdges burdges closed this as completed Aug 31, 2023
@burdges
Copy link
Collaborator

burdges commented Aug 31, 2023

Actually your error looks completely different from the one I just fixed. lol

We should maybe git pin your dependencies @swasilyev ?

@burdges burdges reopened this Aug 31, 2023
@swasilyev
Copy link
Collaborator

Ups, sorry! Just merged w3f/ring-proof#10 blindly

@jasl
Copy link
Author

jasl commented Aug 31, 2023

Ups, sorry! Just merged w3f/ring-proof#10 blindly

Yeah, It seems this is the root cause

@swasilyev
Copy link
Collaborator

3119f51

@jasl
Copy link
Author

jasl commented Aug 31, 2023

@swasilyev @burdges

Currently, Substrate pinned an earlier version, which doesn't pin the ring-proof commit, and when I try to update Substrate, Cargo always updates ring-proof to the latest commit.

I tried to update the Substrate side, but it made massive dependencies conflicts.

To workaround, I manually edit ring-proof rev in Cargo.lock, I think the better way is to release this repo to crates.io

@jasl
Copy link
Author

jasl commented Aug 31, 2023

BTW, The problem is https://github.com/paritytech/ark-substrate/blob/main/curves/bls12_377/Cargo.toml#L21
sp-ark-bls12-377 depends on ark-scale 0.0.3 but here depends on 0.0.10

@davxy
Copy link
Collaborator

davxy commented Aug 31, 2023

I updated the ref in our repo.
As @jasl said sp-ark-... and co. depends on ark-scale 0.0.3 but the fix is enough to unlock the situation...
BTW I'll update the dependencies there as well (https://github.com/paritytech/ark-substrate/)

@burdges
Copy link
Collaborator

burdges commented Aug 31, 2023

ark-scale 0.0.10 should be fairly stable I think.

Afaik we've not yet started using ark-substrate yet internally, but we'll need to do so soon, so it's great to sort this out.

@davxy
Copy link
Collaborator

davxy commented Sep 1, 2023

Fixed by paritytech/polkadot-sdk#1342

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

No branches or pull requests

4 participants