ristretto crypto module

[kianenigma]

a step toward solving #1685. remarks:

A general dilemma in implementing this so far has been choosing the degree to which this module is going to be similar to ed25519.

• the module is currently not used anywhere and solely tested with a similar pattern like the ed25519.rs.
• a Signature (which is now aliased via: pub type Signature = schnorrkel::Signature;) is still implementing pub trait Verifiable but this slightly counter-intuitive now since schnorrkel does NOT provide a verify object via its Signature but instead via its KeyPair/PublicKey. This still works but comparing the current implementation of Verifiable to the one for ed25519, it seems slightly hacky.
• a plausible enhancement to the PR is using untrusted crate again.
• LocalizedSignature in ed25519 derives two additional traits (Encode, Decode) which are not possible atm since both the trait and the struct for which the trait should exist (Signature) are external to the crate. I guess this might be an issue?

[parity-cla-bot]

It looks like @kianenigma hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

[parity-cla-bot]

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

Many thanks,

Parity Technologies CLA Bot

[rphmeier]

@kianenigma I would recommend making a PR to w3f/schnorrkel that adds an optional feature for parity-codec implementation on relevant types.

[rphmeier]

that doesn't seem right, since we're in the substrate code.

Also, the way that schnorrkel is meant to be used is that signatures for each purpose will have their own signing-context (to create domain separation, prevent length-extension attacks).

Using it with a single global context lowers the security of the library.

[burdges]

We should eventually be able to write roughly this whenever const fn gets stabilized and becomes popular enough for merlin to use:

const POLKADOT_PAYMENT_CTX: SigningContext = signing_context(b"PolkadotPayment");

but right now this would require a lazy_static I think.

It's entirely possible that I overthought this interface though. ;) In fact, one could add convenience methods like

fn sign_bytes(&self, context: &'static [u8], bytes: &[u8]) {
    self.sign(signing_context(contect).bytes(bytes))
}

but there are places in polkadot where you want the hash256 builder method instead of the bytes builder method because Sha2 or Shake128 sound faster than merlin's Keccek variant. And these builder help keep the interface simple in this case.

Also, there are cases where you sign very small things, like certificates and the VRF in block production, where you could build the merlin transcript manually, maybe using an extension trait like merlin recommends.

[gavofyork]

b"substrate transaction" or even b"transaction" is fine. This is the fundamental "context" and here is not the place to be trying to shovel data concerning e.g. the genesis block or chain ID into. The signature payload already depends on this stuff which is added at the sensible place further down the stack where this information is more readily available.

[burdges]

Anyways @rphmeier is correct that using the same context for everything breaks the domain separation that signing_context helps provide.

We could do domain separation in other ways like by substrate and polkadot having their own versions of the signing_context function that do "substrate" or "polkadot" before whatever context the users provides, so

pub fn polkadot_context(context : &'static [u8]) -> SigningContext {
    let mut t = SigningContext::new("polkadot")
    t.commit_bytes(b"",context);
    SigningContext(t)
}

We should however do some domain separation though because the alternatives are either adding more key types, which wastes space on chain, or everyone reinventing domain separation locally, probably by doing more careful serialization.

[kianenigma]

Based on what I am reading so far, adding a set of sign() / verify() methods that accept the context seems to be a good way to go. API-wise it also looks clean. we will end up with two groups of exposed functions:

• default context: pub fn verify_strong(sig, message, pubkey) -> bool / pub fn sign(message)
• custom context: pub fn verify_strong_ctx(sig, message, pubkey) -> bool / pub fn sign_ctx(message)

as already suggested by @burdges

[gavofyork]

Happy to make it more general to e.g. "Substrate family" or some such so it nominally covers more than transaction signing. But Polkadot and Substrate domain separation are managed at a place further down in the stack where it makes more sense for that data to be integrated into the signing data.

If there's any use at all to this context parameter it's to distinguish between using the key for Substrate-family signing and the key being used in other non-Substrate family contexts (which are probably nil, but then I'm unconvinced of the utility of exposing context at this low-level).

[kianenigma]

@kianenigma I would recommend making a PR to w3f/schnorrkel that adds an optional feature for parity-codec implementation on relevant types.

Indeed possible and since I am already quite entangled with this crypto library it's a good way to go. Though, I should take some time and inform myself properly of the context and probably ask around a lot to get an exact view of what each component (e.g parity-codec) is and how they will fit. Important to do so given the fact that I am new to all of this.

[burdges]

Yeah I think adding parity-codec seems fine. I haven't used parity-codec yet myself, but I homogenized the serde code quite a bit in https://github.com/w3f/schnorrkel/blob/master/src/serdey.rs#L16 so maybe parity codec would be similar. And parity-codec can be feature gated like serde for outside users.

[burdges]

I'd think derive works here

[kianenigma]

true. Fixed

[gavofyork]

Unless schnorrkel::Signature and friends are bare type defs, then you can probably get away with prefixing with #[derive(Encode, Decode)] and using parity_codec_derive.

[gavofyork]

All good apart from maybe switching "polkadot transaction" to "substrate transaction".

[rphmeier]

@gavofyork I think you misunderstand the signing context a little bit. While it could be used for Chain ID replay protection and things like that, it's also the case that we might use this library for signatures which are not on transactions.

Maybe we should have a general_substrate signing cotext.

[burdges]

Just. curious: Is it possible for polkadot to pass these Transcripts to schorrkel code running native in substrate. Would this require wrapping the sign calls in some trait and avoiding the T: SigningTranscript type parameter?

[kianenigma]

Unless schnorrkel::Signature and friends are bare type defs, then you can probably get away with prefixing with #[derive(Encode, Decode)] and using parity_codec_derive.

I think the way to circumvent this is to have sth like

#[derive(Encode, Decode)]
pub struct Signature(schnorrkel::Signature);

Instead of the current pub type Signature = schnorrkel::Signature;.

I also don't like it from the perspective of being uniform. schnorrkel provides types for KeyPair, Public and Private key as well but we have wrapper structs for them (here, here). Why not have one for Signature as well? at the end of the day, all of them should be treated the same way. Either all type aliases or all wrapped instructs of our own so we can implement all needed traits on them.

[not 100% sure and have not tested this approach out so it might not work/ have other consequences. Just an idea]

Or go with the PR to the original repo with a built-in implementation of parity-codec types so they can be derived here.

[kianenigma]

Another thing not to be forgotten:

a new pub struct Sr25519AuthorityId(pub [u8; 32]); should be added here with probably the same group of trait implelentations.

[gavofyork]

Maybe we should have a general_substrate signing cotext.

Makes sense to me.

[kianenigma]

This PR will most likely be closed/ignored in favor of #1730.