Fix ecdsa_bls verify in BEEFY primitives

[drskalman]

BEEFY ECDSA signatures are on keccak has of the messages. As such we can not simply call

EcdsaBlsPair::verify(signature.as_inner_ref(), msg, self.as_inner_ref())

because that invokes ecdsa default verification which perfoms blake2 hash which we don't want.

This bring up the second issue makes: This makes sign and verify function in pair_crypto useless, at least for BEEFY use case.
Moreover, there is no obvious clean way to generate the signature given that pair_crypto does not exposes sign_prehashed. You could in theory query the keystore for the pair (could you?), invoke to_raw and re-generate each sub-pair and sign using each. But that sounds extremely anticlimactic and will be frow upon by auditors . So I appreciate any alternative suggestion.

[davxy]

Moreover, there is no obvious clean way to generate the signature given that pair_crypto does not exposes sign_prehashed. You could in theory query the keystore for the pair (could you?),

No you can't.

Maybe a solution could be to add some context specific parameters to the sign keystore call? Should be an opaque set of options which can then be used at sign time (like prehashed bool).

Otherwise I don't see any other way other than continue to bloat the Keystore interface with special methods. This also allows to remove the ecdsa "prehashed sign" method already present.

Don't know... this is just an idea. I need to check better the interface

[drskalman]

I have a less intrusive proposal: We introduce a new trait PrehashedSingerPair which introduce PrehashedSingerPair::sign_prehashed function. We implement it for paired_crypto::Pair only if its elements implement it as well. Then we implement it for ecdsa and bls, so we have it for ecdsa_bls377. Then similar to current ecdsa_sign_prehashed function we add ecdsa_bls377_sign_prehashed to the keystore.

There is only one cosmetic caveat here: BLS signature needs hash-to-curve (as opposed to hash-to-field) and that makes it much more of an elaborate process. That is why w3f-bls library currently does not exposes the hasher choice (because nobody is going to use it as usually there is only one IETF standardized hasher for each curve). Nonetheless, we could just fake bls::Pair<T>::sign_prehashed to call bls:Pair<T>::sign if nobody gets deeply hurt and we are good to go.

@davxy: let me know what you think?

[drskalman]

we can call it PrehashedSingerPair::maybe_sign_prehashed it makes people happier :-)

[davxy]

I thinked a bit about this and indeed I like your idea.

So, for the keystore we need to follow a route similar to what has been done for ecdsa.
I.e. provide an ecdsa_bls377_sign_prehashed that in the implementation does something like ecdsa.

For what concerns the way the sign_prehashed is provided by ecdsa-bls12-377 well here we have two routes:

1. define this sign_prehashed only for the Pair specialized for ecdsa_bls12-377 (i.e. within its module

   polkadot-sdk/substrate/primitives/core/src/paired_crypto.rs

   Line 41 in ad5163b

   pub mod ecdsa_bls377 {

   ).

2. Be more generic and introduce this trait you are talking about. And implement it for ecdsa, bls and for any paired crypto where the "sub-schemes" implements it.

Solution 1 is more quick and dirty.
Solution 2 is more generic

Going with 1 doesn't preclude to implement 2 later... For me is the same.

In case you want to go with 2 please add the traits to crypto.rs close to

polkadot-sdk/substrate/primitives/core/src/crypto.rs

Lines 1116 to 1128 in ad5163b

	pub trait VrfSecret: VrfCrypto {
	/// Get VRF-specific output .
	fn vrf_output(&self, data: &Self::VrfInput) -> Self::VrfOutput;
	/// Sign VRF-specific data.
	fn vrf_sign(&self, input: &Self::VrfSignData) -> Self::VrfSignature;
	}
	/// VRF Public Key.
	pub trait VrfPublic: VrfCrypto {
	/// Verify input data signature.
	fn vrf_verify(&self, data: &Self::VrfSignData, signature: &Self::VrfSignature) -> bool;
	}

. Something like:

• PrehashedSigner
• PrehashedVerifier (yeah introducing the verifier has a more clean design)

[drskalman]

define this sign_prehashed only for the Pair specialized for ecdsa_bls12-377 (i.e. within its module

Your suggestion 1 makes sense, because it is a very special and unusual situation and maybe we don't need to address it in a wider context. However, I think this requires me to remove this generic implementation of struct Pair, otherwise, I assume rust would complain about multiple implementation of the struct? Is that OK?

[davxy]

Rust is happy with multiple impl blocks.

• The one that you have outside of ecdsa-bls12-377 module is generic.
• The one that you have inside would have non generic Left and Right pair.

The one that is outside ecdsa-bls12-377 module is empty BTW :-D so can be removed

[drskalman]

Rust is happy with multiple impl blocks.

You are right :-) maybe it is re-implementation after specifying an associated type which doesn't work.

The one that is outside ecdsa-bls12-377 module is empty BTW :-D so can be removed
I think you have already removed it. I was pointing to a wrong copy of the code :-)

[drskalman]

As such this is ready for review. cc: @davxy @Lederstrumpf

[davxy]

🟢 excluding the trivial nitpicks

[Lederstrumpf]

ditto, 🟢 excluding the trivial nitpicks

[Polkadot-Forum]

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/8