Skip to content

Remove aes_gcm module#144

Merged
ordian merged 6 commits into
paritytech:masterfrom
tomaka:ring-4
May 11, 2019
Merged

Remove aes_gcm module#144
ordian merged 6 commits into
paritytech:masterfrom
tomaka:ring-4

Conversation

@tomaka
Copy link
Copy Markdown
Contributor

@tomaka tomaka commented May 10, 2019

Based on top of #139
Closes #133.

Removes aes_gcm module.

@dvdplm
Copy link
Copy Markdown
Contributor

dvdplm commented May 10, 2019

I'm fine closing #133 after this, but would like a trace to remain about the rest of the work. Normally I'd reject something like this because it's a bit half-baked, but I suspect that you need this in Substrate urgently enough to just fork if we don't merge as-is.
I would greatly appreciate it if you could make a brain dump of what you know about ripping out ring entirely, i.e. the stuff left behind the aead feature, in a ticket either here or on Whisper in parity-ethereum.
Thanks!

@tomaka
Copy link
Copy Markdown
Contributor Author

tomaka commented May 10, 2019

I don't need this urgently, unless the definition of "urgent" is less than 2 months.

I want to get out of ring because it doesn't compile for WASM, and I went for a feature gate because ring is the only crate that provides an implementation of GCM (with the exception of rust-crypto, which we also got rid of).

It's not that I put a feature gate because I was too lazy, it's that there's no other way at the moment except by writing our own GCM crate, which I'm not qualified to do.

Comment thread .travis.yml Outdated
@dvdplm
Copy link
Copy Markdown
Contributor

dvdplm commented May 10, 2019

I don't need this urgently, unless the definition of "urgent" is less than 2 months.

Oh, ok, sorry got the wrong impression then.

How about we extract the GCM stuff into its own crate (or even, if Whisper is the only thing using it, maybe it can be just a module there)? That way we can move forward with parity-crypto being truly shared code.

@tomaka
Copy link
Copy Markdown
Contributor Author

tomaka commented May 10, 2019

So I had written features = "aead" instead of feature = "aead", which means it was always disabled.

I put back a SymmetricCrypto variant in the error, but that doesn't expose any type from ring (ring's error is an opaque type that doesn't provide any information anyway).

I think it's a good idea to always have the variant (even if the feature is disabled), in order to avoid breaking crates that match on the error when you enable the feature.

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 10, 2019

I took a look at reverse dependencies of parity-crypto,
on crates.io GCM is not used, for crates not published to crates.io, I've only searched for github and (apart from parity-ethereum copy-paste) found only parity-ethereum, substrate and https://github.com/Conflux-Chain/conflux-rust/blob/99a899bf129d66373890c2c5a3fe0e01938801a4/core/Cargo.toml#L36 usages, GCM is only used for whisper. Of course it doesn't take into account not published crates with private source, on gitlab, etc. But since we're making a breaking change, I would go for removing GCM from parity-crypto and putting it into whisper or its own crate as David suggested.

@tomaka
Copy link
Copy Markdown
Contributor Author

tomaka commented May 10, 2019

Pushed another commit that removes aes_gcm altogether.

@ordian ordian changed the title Add an aead feature Remove aes_gcm module May 10, 2019
@ordian ordian merged commit 0737ec3 into paritytech:master May 11, 2019
@tomaka tomaka deleted the ring-4 branch May 11, 2019 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[parity-crypto] Remove dependency on ring if sensible

3 participants