support crypto primitives for no_std introducing full_crypto feature

[brenzi]

This adds a new compiler feature full_crypto to

• substrate-primitives
• substrate-application-crypto

Why

The upstream logic knows only two use cases:

• within a std environment -> use crypto primitives like sr25519::Pair
• within wasm, a no_std environment with the goal of yielding lightweight binaries -> use host calls for crypto primitives

As we use substrate-primitives within an SGX enclave to sign extrinsics, we're actually adding a third use case:

• within a no_std environment but needing crypto primitives -> no solution so far.

So far, we just patched cargo dependencies to our own fork of these substrate crates, but this is hard to maintain and we think others might benefit from our generalization.

CI

In order to make sure we can test this new feature automatically, we wrote a minimal crate:
https://github.com/scs/test-no-std/tree/substrate-primitives

unfortunately, the linker still fails, but as long as there is no duplicate lang itemerror, the build is to be considered a success.

[parity-cla-bot]

It looks like @brenzi signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[burdges]

Is rand appearing here? We've reached rand 0.7 by now?

As I've noted before w3f/schnorrkel#31 (comment) I think rand_os activates std before 0.2.1, so before rand 0.7. We avoid OsRng, ThreadRng, etc. on chain, but require roughly OsRng for substraTEE, etc.

We'll want RngCore5As4 from w3f/schnorrkel@8150ef6 if we want both ed25519-dalek and rand_core 0.5 (rand 0.7). Are cargo features an issue? I think not..

[brenzi]

@burdges: What we need for substraTEE are: signing & verifying and hashing. For random we use Intel SGX features.
This also means we're not actually using Schnorrkel ourselves within the enclave, because - in contrast to ed25519 - sr25519 seems to require random for deriving a keypair from a seed. So we want to verify sr25519 signatures, but for signing we use an ed25519 pair.

I'm not sure I understand your remarks. no_std works for us in the enclave with sr25519 and ed25519. However, because of a cargo bug, we still need to patch to avoid duplicate lang item std:

[patch.crates-io]
ed25519-dalek = { git = "https://github.com/scs/ed25519-dalek.git", branch = "no_std_sgx"}

but that doesn't affect this codebase here.

[brenzi]

added CI comments ^^^^

[burdges]

sr25519 seems to require random for deriving a keypair from a seed.

We do not require randomness when expanding a keypair from a seed:
https://docs.rs/schnorrkel/0.1.1/src/schnorrkel/keys.rs.html#114

We do use randomness when doing HDKD in https://docs.rs/schnorrkel/0.1.1/src/schnorrkel/derive.rs.html#177 which I should probably replace with a t.witness_bytes call so that you can replace thread_rng using attach_rng. I tried to write the code more semantically once upon a time, but this'll make more sense now I think. Thanks!

We previously used randomness when doing HDKD in https://docs.rs/schnorrkel/0.1.1/src/schnorrkel/derive.rs.html#177 but any recent schnorrkel fixes this https://docs.rs/schnorrkel/0.8.5/src/schnorrkel/derive.rs.html#185 so use attach_rng if you must control the randomness source.

[brenzi]

@burdges: thanks for your helpful hints. This means we actually might use sr25519 keys for our enclave.

However: I consider this off-topic for this PR.

[brenzi]

@bkchr, @kianenigma: This PR just got flagged "in progress". While it's true that it is not ready for merging/review, I'd like to ask you for a statement if you are willing to accept our added use case and the approach of this PR

[gavofyork]

yeah; looks reasonable to me.

[bkchr]

Sorry for the delay!

Generally looking good. Please remove these duplicate implementations, just import the trait from rstd.

And as I said, I would call the feature full_crypto.

[bkchr]

@brenzi you need to update the Cargo.lock and commit that. (Just run cargo build for a moment)

[brenzi]

It appears to me that cargo doesn't forward our new feature full_crypto correctly:

error[E0412]: cannot find type `Pair` in module `app`
  --> core/finality-grandpa/primitives/src/lib.rs:38:31
   |
38 | pub type AuthorityPair = app::Pair;
   |                               ^^^^ not found in `app`
help: possible candidates are found in other modules, you can import them into scope
   |
25 | use app_crypto::Pair;
   |
25 | use app_crypto::ed25519::Pair;
   |
25 | use app_crypto::sr25519::Pair;
   |
25 | use sr_primitives::testing::sr25519::Pair;
   |

error[E0277]: the trait bound `app::Signature: std::hash::Hash` is not satisfied
  --> core/finality-grandpa/primitives/src/lib.rs:33:2
   |
33 |     app_crypto!(ed25519, GRANDPA);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::hash::Hash` is not implemented for `app::Signature`
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

@bkchr: do you have a clue?

[brenzi]

ahh, got it. need to add the feature for the finality crate as well - because it's a macro I guess.

[bkchr]

Please don't build it in release, that just waste resources^^

[brenzi]

ok.

[brenzi]

@bkchr It seems I broke the CI script:

Running with gitlab-runner 12.4.1 (05161b14)
  on ci2-docker 8a60f15c
Using Docker executor with image parity/rust-builder:latest ...
ERROR: Preparation failed: Error response from daemon: Conflict. The container name "/runner-8a60f15c-project-145-concurrent-0-cache-c33bcaa1fd2c77edfc3893b41966cea8" is already in use by container "81f69a0237714f11351d20325b818d3400aa19476e4057b1d43bcaefcfbe0075". You have to remove (or rename) that container to be able to reuse that name. (cache_container.go:97:0s)
Will be retried in 3s ...

but why and how?

[brenzi]

@bkchr: It seems that my new CI test is not run:

substrate/.gitlab-ci.yml

Line 274 in 536d6c3

build-full-crypto-feature:

[brenzi]

I'll have to put this on wait because it seem's the latest version causes no_std collisions when testing against https://github.com/scs/test-no-std/tree/substrate-primitives.
I'll revert once that's fixed

[brenzi]

the last commit fixes it. My only remaining question is if my new CI test is actually running.

[brenzi]

could we include building https://github.com/scs/test-no-std/tree/substrate-primitives in CI as well? It's a minimal crate that checks for no_std collisions. Maybe you already have means to test such things for wasm? Better ways?

[bkchr]

One last nitpick, otherwise it looks good.

Ty for your patience :)

[bkchr]

Ty!

[brenzi]

@bkchr thanks for your patience and support too.