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

make rand/getrandom dependency explicit with a feature #37

Merged
merged 3 commits into from Jan 14, 2022

Conversation

lightyear15
Copy link
Contributor

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
Copy link
Contributor Author

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.

@lightyear15 lightyear15 changed the title make rand/getrandom dependency explicit with a feature make rand/getrandom dependency explicit with a feature Oct 25, 2021
[features]
default = [ "std" ]
std = [ "rand/std" ]
std = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to disable rand/std when std is on? That's not supported in wasm anyway, right? Or does wasm support std?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done any in-depth research, but, while working on this wasm contract, I can tell you that many things from std are compatible with wasm.

  • std::collections like Vec or Hashmap are ok
  • Option and Result work just fine
  • So far, never hit any issue working withString and str
  • array and slices are 👌

AFAIK there isn't any particular issue using std in wasm environment, except probably IO and multi-thread stuff.

The issue with using the current version of ark-std in wasm for smart contracts is getrandom crate: there is no way a blockchain EVM can provide you with functions that generate randomness.
This change aims at making the dependency over getrandom optional. So crates that sit above ark-std like all the algebra crates, crypto-primitives , crates in curve, etc..., they can still rely on re-exported traits from ark_std::rand, while gaining wasm compatibility for free.

Side Note: all the no-std gears are dedicated more to the embedded world, rather than to wasm.

@Pratyush
Copy link
Member

Thanks for the PR =)

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``
@weikengchen
Copy link
Member

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
Copy link
Member

Let me have a try locally...

@weikengchen
Copy link
Member

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
Copy link
Member

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:

  • std::collections like Vec or Hashmap
  • Option and Result
  • String and str
  • array and slices

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.

getrandom being a dev-dependency makes cargo trying to include it in the
build even when we are trying to build with no-std.
Setting the resolver = "2" fixes the issue as Cargo is forced to
recompute all the dependencies.
@lightyear15
Copy link
Contributor Author

lightyear15 commented Oct 27, 2021

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 🤞

@weikengchen
Copy link
Member

@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
Copy link
Member

(we still need cargo fmt in the stable channel)

@Pratyush Pratyush changed the title make rand/getrandom dependency explicit with a feature make rand/getrandom dependency explicit with a feature Jan 14, 2022
@Pratyush Pratyush merged commit 9501c25 into arkworks-rs:master Jan 14, 2022
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.

None yet

3 participants