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

Move crypto implementations out of sp-core #1975

Open
1 of 2 tasks
kayabaNerve opened this issue Oct 22, 2023 · 4 comments
Open
1 of 2 tasks

Move crypto implementations out of sp-core #1975

kayabaNerve opened this issue Oct 22, 2023 · 4 comments
Assignees
Labels
I5-enhancement An additional feature request. T13-deprecation The current issue/pr is, or should be, part of a deprecation process.

Comments

@kayabaNerve
Copy link

kayabaNerve commented Oct 22, 2023

Motivation

Currently compiling sp-core with std will compile:

  • 2 secp256k1 libraries (one in C, one in Rust)
  • An Ed25519 lib
  • A Ristretto lib
  • The Arkworks suite
  • BLS12-381 (arkworks)
  • Bandersnatch (also arkworks)

And that's for elliptic curves alone. Several hash algorithms/transcripts also get pulled in.

Request

Decrease tree size, ideally not only via feature flags yet completely in order to ensure minimal Cargo.locks (making supply chain evaluations much easier).

Solution

Move all of these literals into their own primitives crates.

For secp256k1/Ed25519/Ristretto specifically, their use is almost fully abstracted behind traits. Accordingly, it should be feasible to have sp-schnorrkel, sp-ed25519, and sp-ecdsa preventing sp-core compilations from forcing inclusion of so many dependencies.

I'd hope to see this expanded to the Arkworks section, yet I know that's under active development and don't want to throw a wrench at it while it moves.

I'd be fine helping with this yet I'm unwilling and/or not-reasonably-able to sign the CLA at this time, sorry.

Are you willing to help with this request?

Maybe (please elaborate above)


Status

@kayabaNerve kayabaNerve added the I5-enhancement An additional feature request. label Oct 22, 2023
@github-actions github-actions bot added the I10-unconfirmed Issue might be valid, but it's not yet known. label Oct 22, 2023
@kayabaNerve
Copy link
Author

#133 relevant.

@davxy
Copy link
Member

davxy commented Oct 23, 2023

This is something I've been thinking about for a while.

Even though it doesn't help with the supply chain size I'd like to first identify and move all the crypto-related stuff in the substrate/primitives/crypto folder (is already present and for the moment it just contains EC related host functions).
This at least will separate it from non-crypto core stuff.

As a potential follow-up we can then consider "one crate per primitive" or "feature gated primitives"..

  • For sure it'd help reducing the not required deps and build times in the consumers
  • It'd be a bit more annoying as you have to pick the crate of interest instead of including sp-crypto in one shot

Maybe feature flags are better than separate crates here (don't know... just asking for opinions)?

Let's collect some opinions first (cc @paritytech/sdk-node)

@davxy davxy removed the I10-unconfirmed Issue might be valid, but it's not yet known. label Oct 23, 2023
@koute
Copy link
Contributor

koute commented Oct 23, 2023

Let's collect some opinions first

Personally I'd probably move all crypto related stuff into sp-crypto, make all of them disabled by default (so by default only the generic interfaces would be compiled, without any actual crypto code/dependencies) and add feature flags for each of them.

@michalkucharczyk
Copy link
Contributor

My two cents:

  • sounds good, sp-crypto makes sense.
  • I am thinking of explicit randomness feature. Now the functionality which requires randomness is implicitly hidden behind std feature.
  • would be also nice to have tests working for different subsets of features.

@davxy davxy self-assigned this Nov 14, 2023
@davxy davxy added the T13-deprecation The current issue/pr is, or should be, part of a deprecation process. label Jan 22, 2024
@davxy davxy changed the title [Request] Move crypto implementations out of sp-core Move crypto implementations out of sp-core Jan 22, 2024
github-merge-queue bot pushed a commit that referenced this issue Jan 23, 2024
Step towards #1975

As reported
#1975 (comment)
I'd like to encapsulate crypto related stuff in a dedicated folder.

Currently all cryptographic primitive wrappers are all sparsed in
`substrate/core` which contains "misc core" stuff.

To simplify the process, as the first step with this PR I propose to
move the cryptographic hashing there.

The `substrate/crypto` folder was already created to contains `ec-utils`
crate.

Notes:
- rename `sp-core-hashing` to `sp-crypto-hashing`
- rename `sp-core-hashing-proc-macro` to `sp-crypto-hashing-proc-macro`
- As the crates name is changed I took the freedom to restart fresh from
version 0.1.0 for both crates

---------

Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
)

Step towards paritytech#1975

As reported
paritytech#1975 (comment)
I'd like to encapsulate crypto related stuff in a dedicated folder.

Currently all cryptographic primitive wrappers are all sparsed in
`substrate/core` which contains "misc core" stuff.

To simplify the process, as the first step with this PR I propose to
move the cryptographic hashing there.

The `substrate/crypto` folder was already created to contains `ec-utils`
crate.

Notes:
- rename `sp-core-hashing` to `sp-crypto-hashing`
- rename `sp-core-hashing-proc-macro` to `sp-crypto-hashing-proc-macro`
- As the crates name is changed I took the freedom to restart fresh from
version 0.1.0 for both crates

---------

Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T13-deprecation The current issue/pr is, or should be, part of a deprecation process.
Projects
Status: in progress
Development

No branches or pull requests

4 participants