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

Optional dependencies #391

Closed
tomtau opened this issue Jun 29, 2020 · 6 comments · Fixed by #441
Closed

Optional dependencies #391

tomtau opened this issue Jun 29, 2020 · 6 comments · Fixed by #441
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request question Further information is requested
Milestone

Comments

@tomtau
Copy link
Contributor

tomtau commented Jun 29, 2020

Not sure if this will be resolved with #387
but could some tendermint-rs dependencies be made optional (with a feature-guard) -- e.g. if one's TM genesis.json is configured with:

    "validator": {
      "pub_key_types": [
        "ed25519"
      ]
    }

It's unnecessary to have signatory with ecdsa-feature, signatory-secp256k1 and ripemd160?

@xla xla added dependencies Pull requests that update a dependency file enhancement New feature or request question Further information is requested labels Jun 29, 2020
@xla
Copy link
Contributor

xla commented Jul 7, 2020

This can only work if the genesis information is known at compile time. Alternatively different releases with different features could be evaluated.

@yihuang
Copy link
Contributor

yihuang commented Jul 10, 2020

One of the motivation to make signatory-secp256k1 optional: currently secp256k1-sys has dependency cc >=1.0.28, <=1.0.41, and ring has dependency cc ^1.0.52, they are conflicted.
So trying to upgrade to current tendermint-rs master version cause our project to downgrade some dependencies.

EDIT: It seems cargo resolve version not considering feature, so make it feature guarded don't help with this.

@ebuchman
Copy link
Member

This can only work if the genesis information is known at compile time

Sure, but we know virtually everyone is only using ed25519, so we should make this optional, and even default off.

@ebuchman ebuchman added this to the v0.15.0 milestone Jul 12, 2020
yihuang added a commit to yihuang/tendermint-rs that referenced this issue Jul 13, 2020
yihuang added a commit to yihuang/tendermint-rs that referenced this issue Jul 13, 2020
@tarcieri
Copy link
Contributor

FYI, I just shipped a pure Rust alternative to secp256k1-sys - the k256 crate (already a dependency of tendermint-rs) now natively supports ECDSA signing and verification:

https://www.reddit.com/r/rust/comments/i7lca3/ann_rustcrypto_k256_and_p256_v020_pure_rust/

It uses a similar implementation strategy (complete projective formulas, constant time scalar multiplication, field arithmetic based on "lazy reductions", optional endomorphism-based optimizations).

@xla
Copy link
Contributor

xla commented Aug 11, 2020

Do we need to follow-up on @tarcieri announcement @liamsi ?

@tarcieri
Copy link
Contributor

@xla I plan on opening a PR later today which upgrades everything.

It can remove both the signatory-secp256k1 and signatory-dalek dependencies, as the k256 crate now natively implements ECDSA, and ed25519-dalek now natively implements the Signer and Verifier APIs.

tarcieri pushed a commit that referenced this issue Aug 13, 2020
Native support for the `signature` traits used by the `signatory*`
was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256`
crate recently added a native pure Rust implementation of
ECDSA/secp256k1.

Together these changes eliminate the need for the `signatory*` wrapper
crates. Additionally, `k256` eliminates the need for the rust-secp256k1
C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all `signatory` dependencies and uses
`ed25519-dalek` and `k256` directly instead. Both of these were already
dependencies, so it's not adding any additional dependencies, but
instead removes several of them.
tarcieri pushed a commit that referenced this issue Aug 17, 2020
Native support for the `signature` traits used by the `signatory*`
was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256`
crate recently added a native pure Rust implementation of
ECDSA/secp256k1.

Together these changes eliminate the need for the `signatory*` wrapper
crates. Additionally, `k256` eliminates the need for the rust-secp256k1
C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all `signatory` dependencies and uses
`ed25519-dalek` and `k256` directly instead. Both of these were already
dependencies, so it's not adding any additional dependencies, but
instead removes several of them.
tarcieri pushed a commit that referenced this issue Aug 17, 2020
Native support for the `signature` traits used by the `signatory*`
was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256`
crate recently added a native pure Rust implementation of
ECDSA/secp256k1.

Together these changes eliminate the need for the `signatory*` wrapper
crates. Additionally, `k256` eliminates the need for the rust-secp256k1
C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all `signatory` dependencies and uses
`ed25519-dalek` and `k256` directly instead. Both of these were already
dependencies, so it's not adding any additional dependencies, but
instead removes several of them.
tarcieri pushed a commit that referenced this issue Aug 17, 2020
Native support for the `signature` traits used by the `signatory*`
was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256`
crate recently added a native pure Rust implementation of
ECDSA/secp256k1.

Together these changes eliminate the need for the `signatory*` wrapper
crates. Additionally, `k256` eliminates the need for the rust-secp256k1
C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all `signatory` dependencies and uses
`ed25519-dalek` and `k256` directly instead. Both of these were already
dependencies, so it's not adding any additional dependencies, but
instead removes several of them.
tarcieri pushed a commit that referenced this issue Aug 18, 2020
Native support for the `signature` traits used by the `signatory*`
was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256`
crate recently added a native pure Rust implementation of
ECDSA/secp256k1.

Together these changes eliminate the need for the `signatory*` wrapper
crates. Additionally, `k256` eliminates the need for the rust-secp256k1
C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all `signatory` dependencies and uses
`ed25519-dalek` and `k256` directly instead. Both of these were already
dependencies, so it's not adding any additional dependencies, but
instead removes several of them.
tarcieri pushed a commit that referenced this issue Aug 18, 2020
Native support for the `signature` traits used by the `signatory*`
was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256`
crate recently added a native pure Rust implementation of
ECDSA/secp256k1.

Together these changes eliminate the need for the `signatory*` wrapper
crates. Additionally, `k256` eliminates the need for the rust-secp256k1
C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all `signatory` dependencies and uses
`ed25519-dalek` and `k256` directly instead. Both of these were already
dependencies, so it's not adding any additional dependencies, but
instead removes several of them.
brapse pushed a commit that referenced this issue Aug 18, 2020
Native support for the `signature` traits used by the `signatory*`
was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256`
crate recently added a native pure Rust implementation of
ECDSA/secp256k1.

Together these changes eliminate the need for the `signatory*` wrapper
crates. Additionally, `k256` eliminates the need for the rust-secp256k1
C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all `signatory` dependencies and uses
`ed25519-dalek` and `k256` directly instead. Both of these were already
dependencies, so it's not adding any additional dependencies, but
instead removes several of them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants