BEEFY: Define a BeefyVerify trait for signatures

[serban300]

Context:

Beefy introduces some cryptographic types here. The current underlying crypto scheme used is ECDSA, but the idea is to be able to use these types as beefy_primitives::crypto::Public, beefy_primitives::crypto::Signature, etc, without caring about the underlying crypto scheme ( ideally it should be possible for the underlying crypto scheme to be changed without impacting the code that uses these types).

The problem is that I couldn't find any generic way to check that a beefy authority signature is valid without calling sp_core::ecdsa::Pair::verify_prehashed() or sp_io::crypto::secp256k1_ecdsa_recover_compressed(), so we would need to use ECDSA specifically.

Description of changes:

This PR defines a BeefyVerify trait (for signatures) and implements it for beefy_primitives::crypto::AuthoritySignature.

This is a more customizable version of the Verify trait, that enables the caller to provide a msg hashing function and a conversion function for the signer.

With these new abstractions we can do something like:

	BeefyVerify::<Keccak256, _, Identity>::custom_verify(
		sig,
		message,
		public,
	)

instead of calling ECDSA logic specifically.

Related to: paritytech/parity-bridges-common#2469

[acatangiu]

💯

[Lederstrumpf]

lgtm - only suggestion's extending tests.
other aspects of beefy are still coupled to keccak/ecdsa, but this is a good start.

[Lederstrumpf]

This is still firmly coupled to keccak. Is BeefyKeystore envisioned to become hasher generic too, or is scope only to decouple from ecdsa?

[serban300]

The scope was only to decouple the verification from ECDSA, in order to be able to verify beefy signatures without hadrcoding the underlying crypto scheme in bridges. Something like this. Also in bridges in the current PoC (the same branch linked above) now we define a BridgedBeefyCommitmentHasher associated type for each bridged chain with beefy finality, so for bridges in a way the hashing is generic as well.

But I wasn't sure if this is the right approach for bridges and it would be worth discussing this in more detail anyway in order to understand what would be the right approach for the Beefy keystore as well and adapt BeefyVerify accordingly. How is the hashing algorithm used for signing the commitment meant to be selected ? Will it be configurable for each chain (e.g. one chain may use ECDSA + Keccak256 while other may use ECDSA + Blake_2_256) ? Or will it be specific to the crypto scheme (e.g. ECDSA will use for example Keccak256 for any chain, always; BLS will use for example Blake_2_256 for any chain always). Or will it be configurable by each client ?

cc: @acatangiu

[serban300]

I pushed some commits. I removed SignerToAccountId since I don't think we're going to use it. I don't know if BLS will support recovering the signer from the signature. And we'll probably send the entire list of validators anyway.

Regarding the hash I still don't know what would be the best option. It really depends on how it's going to be used. The current version is more customizable. But if the hashing algorithm will be tied to the crypto scheme for example I would define it as an associated type for BeefyAuthorityId and it would be easier to use.

[Lederstrumpf]

Will it be configurable for each chain (e.g. one chain may use ECDSA + Keccak256 while other may use ECDSA + Blake_2_256) ? Or will it be specific to the crypto scheme (e.g. ECDSA will use for example Keccak256 for any chain, always; BLS will use for example Blake_2_256 for any chain always)

Signature scheme & hash function aren't firmly coupled. For instance, in Bitcoin you also use ECDSA, but with SHA-256 for hashing, instead of Keccak256. Similarly for BLS scheme, you can use a variety of hash-to-curves. So let's keep it decoupled here too. I'd also make BeefyKeystore hasher generic then, but we can cross that line later.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1921662

[serban300]

@acatangiu @Lederstrumpf could you PTAL on the latest changes here ? If there are no preferences related to #12299 (comment) I would merge the PR as it is for the moment.

[acatangiu]

I'm happy with current PR, feel free to merge.

[Lederstrumpf]

lgtm, given scope of PR.

[Lederstrumpf]

bot merge