Support for multiple signature scheme for BEEFY primitves

[drskalman]

This is a subset of the changes introduced by Pull request #13311 which tends to enable BEEFY to support more than one signature scheme at the time in order to accommodate various bridged chains. This is only the changes needed in primitives/consensus/beefy while keeping the current client backward compatible in order to facilitate reviewing and merging.

polkadot companion: paritytech/polkadot#7572

cumulus companion: paritytech/cumulus#2962

[acatangiu]

Everything looks good, except single question below.

[davxy]

Probably at some point you'll need to embed this key type in the keys used by some node (in SessionKeys here).

This is used to generate new keys "on demand" via the sp_session::SessionKeys interface or in the genesis build for the testnet node.

BTW, to add a key type there you need to implement the RuntimePublic trait for your application specific public key.
Is not required to implement the full set of functions (as you don't need all of them) you can implement only the generate_key.

As a reference see the bandersnatch-vrf PR here:

substrate/primitives/application-crypto/src/bandersnatch.rs

Lines 32 to 57 in bdd3df4

	impl RuntimePublic for Public {
	type Signature = Signature;
	/// Dummy implementation. Returns an empty vector.
	fn all(_key_type: KeyTypeId) -> Vec<Self> {
	Vec::new()
	}
	fn generate_pair(key_type: KeyTypeId, seed: Option<Vec<u8>>) -> Self {
	sp_io::crypto::bandersnatch_generate(key_type, seed)
	}
	/// Dummy implementation. Returns `None`.
	fn sign<M: AsRef<[u8]>>(&self, _key_type: KeyTypeId, _msg: &M) -> Option<Self::Signature> {
	None
	}
	/// Dummy implementation. Returns `false`.
	fn verify<M: AsRef<[u8]>>(&self, _msg: &M, _signature: &Self::Signature) -> bool {
	false
	}
	fn to_raw_vec(&self) -> Vec<u8> {
	sp_core::crypto::ByteArray::to_raw_vec(self)
	}
	}

[drskalman]

BTW, to add a key type there you need to implement the RuntimePublic trait for your application specific public key.

Hmm, I did but you removed them for some reason. I can bring them back if you think they are OK.

[davxy]

BTW, to add a key type there you need to implement the RuntimePublic trait for your application specific public key.

Hmm, I did but you removed them for some reason. I can bring them back if you think they are OK.

@drskalman we don't require to implement the full set of methods because most of them require host functions support (sp_io::...). Is preferable to add new host functions only if are really - really- required.

Unfortunately, working on analoguous code for introducing bandersnatch-vrf, I noticed that if you want to use the key as part of the session keys set then you require to implement at least the generate_pair because it is used to do a couple of things.

Now, as you can see from the snip I've shared, I just implemented the generate_pair and provided a dummy implementation for the others (makes no sense to introduce a HF that is never used).

Before adding it, just let me check a couple of things to see if we can instead fix this requirement in other way
@drskalman looks like -as is - there is no workaround. Explicit session keys set generation pass thorough the runtime and then access the keystore via externalities to save the key there.
In short, I think you need to add a "partially dummy" implementation to at least provide the ability to generate the key via host func as in the bandersnatch-vrf snip I shared

[drskalman]

@davxy : It seems that runtime_interface is failing to derive PassBy using your custom implementation of PassByInner:

    |   the trait `PassBy` is not implemented for `sp_core::bls::Public<w3f_bls::engine::TinyBLS<ark_ec::models::bls12::Bls12<ark_bls12_377::curves::Config>,
 ark_bls12_377::curves::Config>>`                                                                                                                     

Should we also manually implement PassBy for Public And Signature in core/src/bls.rs?

[drskalman]

@drskalman I added a suggestion that allows to remove direct dep from w3f-bls.

Some more nits

I think I addressed all the new ones.

[andresilva]

lgtm. just minor nits

[Lederstrumpf]

lgtm - one comment re. consistency on path forward wrt. host functions.

[davxy]

bot merge

[paritytech-processbot]

Error: Github API says "Allow edits from maintainers" is not enabled for paritytech/polkadot#7562. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.

[davxy]

@drskalman apparently you have to enable "allow edits from maintainers" in your polkadot PR

[ironoa]

@drskalman apparently you have to enable "allow edits from maintainers" in your polkadot PR

there seems to be a known GH issue that still hasn't been solved, which prevents "allow edits from maintainers" to be engaged on our end: https://github.com/orgs/community/discussions/5634

Have you ever managed to bypass this problem ?

[davxy]

@drskalman apparently you have to enable "allow edits from maintainers" in your polkadot PR

there seems to be a known GH issue that still hasn't been solved, which prevents "allow edits from maintainers" to be engaged on our end: https://github.com/orgs/community/discussions/5634

Have you ever managed to bypass this problem ?

cc @paritytech/ci

[davxy]

bot merge

[paritytech-processbot]

Error: "Check reviews" status is not passing for paritytech/polkadot#7572

[davxy]

bot merge