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

unsatisified no_std #84

Closed
3 tasks done
alxiong opened this issue Jul 25, 2022 · 1 comment · Fixed by #87
Closed
3 tasks done

unsatisified no_std #84

alxiong opened this issue Jul 25, 2022 · 1 comment · Fixed by #87
Assignees

Comments

@alxiong
Copy link
Contributor

alxiong commented Jul 25, 2022

Currently we thought we had proper no_std support since we use ark_std by default and only use std library with features(std).

I run into a bug today on cap-rollup branch that suggests otherwise.

How to reproduce

Check out to cap-rollup branch, run cargo check the code should compile successfully, but when running cargo check --features std, then you should see this error:

    Checking jf-plonk v0.1.2 (/Users/alex/work/jellyfish/plonk)
error[E0119]: conflicting implementations of trait `std::error::Error` for type `errors::PlonkError`
  --> plonk/src/errors.rs:50:1
   |
47 | impl ark_std::error::Error for PlonkError {}
   | ----------------------------------------- first implementation here
...
50 | impl std::error::Error for PlonkError {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `errors::PlonkError`

This is unexpected since ark_std::error::Error should be a different trait than std::error::Error.

Analysis

@zhenfeizhang and I spent some time digging around.

  • We first eliminate the possibility of leaky no_std from upsteam ark_std library by creating a minimal reproducible example.
  • We also double checked that our syntax #![cfg_attr(not(feature = "std"), no_std)] is a correct no_std declaration
  • we come to temporary conclusion that we could have accidentally imported a version of ark_std with features = ["std"], this could be through parallel features of ark-ff, ark-ec, ark-bls or any curve impl crates.
    • this suspicion gets confirmed by removing the parallel feature flag on ark-ff and ark-ec in our utilities crate, and we get the following error when only trying to compile jf-utils:
         --> utilities/src/serialize.rs:119:17
          |
      119 | #[derive(Debug, Snafu)]
          |                 ^^^^^ method cannot be called on `&SerializationError` due to unsatisfied trait bounds
          |
         ::: /Users/alex/.cargo/registry/src/github.com-1ecc6299db9ec823/ark-serialize-0.3.0/src/error.rs:5:1
          |
      5   | pub enum SerializationError {
          | ---------------------------
          | |
          | doesn't satisfy `SerializationError: AsErrorSource`
          | doesn't satisfy `SerializationError: StdError`
          |
          = note: the following trait bounds were not satisfied:
                  `SerializationError: StdError`
                  which is required by `SerializationError: AsErrorSource`
                  `&SerializationError: StdError`
                  which is required by `&SerializationError: AsErrorSource`
          = note: this error originates in the derive macro `Snafu` (in Nightly builds, run with -Z macro-backtrace for more info)
      

Root Cause

  • WIP: find out the real root cause
  • WIP: evaluate other repos for similar problem
  • Q: how to setup code to automatically detect accidental reliance on std in the future?
@alxiong
Copy link
Contributor Author

alxiong commented Jul 26, 2022

So there are two places that triggers our jellyfish to use std

Solution and Changes

  • remove Snafu dependency (maybe implement Display for TaggedBlobError for the downstream (cc @jbearer)
  • add parallel flag to our jf-* crates
  • replace usage of HashSet, HashMap with BTreeMap Vec and BTreeSet since the former are NOT no_std (see stackoverflow answer)

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 a pull request may close this issue.

2 participants