arkworks-rs / std

A standard library wrapper for use in the `arkworks` ecosystem
https://www.arkworks.rs
Apache License 2.0
37 stars 33 forks source link

make `rand/getrandom` dependency explicit with a feature #37

Closed lightyear15 closed 2 years ago

lightyear15 commented 3 years ago

Description

In the effort to make the arkworks framework available for compilation in wasm environment, we need to make getrandom indirect dependency an opt-in feature. rand/std feature triggers the dependency over getrandom, so it can't be included in the std feature. It must be separated in an extra feature that we named getrandom

lightyear15 commented 3 years ago

Hello, let me give some more context about this PR: I am working on a block chain related project that aims at using the arkworks libraries (std, algebra, crypto-primitives...) on wasm environment. Wasm for block chain is an ugly beast, and it presents issues that cannot be resolved with crates like wasm-bindgen. rand and its dependency on getrandom is one of these. There is no possibility of using, nor creating, randomness and random variables, so getrandom is a crate, any smart contract developer cannot import in their project.

I am working my way through porting arkworks crates in an wasm environment where developers can leverage rust features to opt-in/opt-out the dependency on getrandom. This PR is the first of a list of changes I have ready to get pushed for review.

Pratyush commented 3 years ago

Thanks for the PR =)

weikengchen commented 3 years ago

You may need to do a cargo fmt in the stable channel. Let me think about how to handle the failure of the GitHub actions about check no_std.

The failure regarding "check no_std" is that it still tries to invoke getrandom. What should we do?

weikengchen commented 3 years ago

Let me have a try locally...

weikengchen commented 3 years ago

I used cargo tree -e features. It outputs the following, which seems to not make sense why the default rand_core would invoke the getrandom:

ark-std v0.3.0 (/Users/cusgadmin/CLionProjects/ark-std-getrandom)
├── num-traits v0.2.14
│   [build-dependencies]
│   └── autocfg feature "default"
│       └── autocfg v1.0.1
└── rand feature "std_rng"
    ├── rand v0.8.4
    │   ├── libc v0.2.105
    │   ├── rand_chacha v0.3.1
    │   │   ├── ppv-lite86 feature "simd"
    │   │   │   └── ppv-lite86 v0.2.15
    │   │   └── rand_core feature "default"
    │   │       └── rand_core v0.6.3
    │   │           └── getrandom feature "default"
    │   │               └── getrandom v0.2.3
    │   │                   ├── libc v0.2.105
    │   │                   └── cfg-if feature "default"
    │   │                       └── cfg-if v1.0.0
    │   └── rand_core feature "default" (*)
    ├── rand feature "rand_chacha"
    │   └── rand v0.8.4 (*)
    └── rand feature "rand_hc"
        └── rand v0.8.4 (*)
weikengchen commented 3 years ago

Another thing from our discussion is that maybe the cleanliest way is to just avoid std---although std is useful, but the many things that are likely useful in wasm, as you pointed out:

The bypass used in arkworks is to use things from core:: or alloc::, and ark-std is sort of created to help this. Note that HashMap has to be replaced by BTreeMap.

lightyear15 commented 3 years ago

Hi @weikengchen , no worries about the no-std failing, it's a known problem with Cargo. getrandom being a dev-dependency does not prevent Cargo from trying to include it even when doing a simple cargo check no matter what the target is. In order to fix this issue, you simply need to set resolver = "2" in the top Cargo.toml. check here for more info I have tested locally and it seems it passes the no-std test, I am fairly confident it will pass the ci test as well :crossed_fingers:

weikengchen commented 3 years ago

@Pratyush The resolver 2 is the default for edition=2021. I think we may need to prepare to migrate to 1.56 as well as edition 2021.

weikengchen commented 3 years ago

(we still need cargo fmt in the stable channel)