Make some BEEFY keystore logic more generic (#10763)

This PR:
1. makes some BEEFY keystore methods more generic:
- `sign()`
- `public_keys()`
This is done by implementing the specific logic in the
`BeefyAuthorityId`.
2. Removes the `BeefyAuthorityId::SignatureHasher` since for some
algorithms it doesn't make sense to have a hasher.

Also since now the `BeefyAuthorityId` implements both the signing and
the verification logic, we should have better consistency.

Related to
#8707 (comment)

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: serban300

bridges/modules/beefy/src/lib.rs

@@ -58,8 +58,6 @@ pub type BridgedBlockHash<T, I> = bp_runtime::HashOf<BridgedChain<T, I>>;
 /// Pallet initialization data.
 pub type InitializationDataOf<T, I> =
 	InitializationData<BridgedBlockNumber<T, I>, bp_beefy::MmrHashOf<BridgedChain<T, I>>>;
-/// BEEFY commitment hasher, used by configured bridged chain.
-pub type BridgedBeefyCommitmentHasher<T, I> = bp_beefy::BeefyCommitmentHasher<BridgedChain<T, I>>;
 /// BEEFY validator id, used by configured bridged chain.
 pub type BridgedBeefyAuthorityId<T, I> = bp_beefy::BeefyAuthorityIdOf<BridgedChain<T, I>>;
 /// BEEFY validator set, used by configured bridged chain.

bridges/modules/beefy/src/mock.rs

@@ -17,8 +17,8 @@
 use crate as beefy;
 use crate::{
 	utils::get_authorities_mmr_root, BridgedBeefyAuthoritySet, BridgedBeefyAuthoritySetInfo,
-	BridgedBeefyCommitmentHasher, BridgedBeefyMmrLeafExtra, BridgedBeefySignedCommitment,
-	BridgedMmrHash, BridgedMmrHashing, BridgedMmrProof,
+	BridgedBeefyMmrLeafExtra, BridgedBeefySignedCommitment, BridgedMmrHash, BridgedMmrHashing,
+	BridgedMmrProof,
 };
 
 use bp_beefy::{BeefyValidatorSignatureOf, ChainWithBeefy, Commitment, MmrDataOrHash};
@@ -33,7 +33,7 @@ use sp_runtime::{
 };
 
 pub use sp_consensus_beefy::ecdsa_crypto::{AuthorityId as BeefyId, Pair as BeefyPair};
-use sp_core::crypto::Wraps;
+use sp_consensus_beefy::test_utils::BeefySignerAuthority;
 use sp_runtime::traits::Keccak256;
 
 pub type TestAccountId = u64;
@@ -44,7 +44,6 @@ pub type TestBridgedAuthoritySetInfo = BridgedBeefyAuthoritySetInfo<TestRuntime,
 pub type TestBridgedValidatorSet = BridgedBeefyAuthoritySet<TestRuntime, ()>;
 pub type TestBridgedCommitment = BridgedBeefySignedCommitment<TestRuntime, ()>;
 pub type TestBridgedValidatorSignature = BeefyValidatorSignatureOf<TestBridgedChain>;
-pub type TestBridgedCommitmentHasher = BridgedBeefyCommitmentHasher<TestRuntime, ()>;
 pub type TestBridgedMmrHashing = BridgedMmrHashing<TestRuntime, ()>;
 pub type TestBridgedMmrHash = BridgedMmrHash<TestRuntime, ()>;
 pub type TestBridgedBeefyMmrLeafExtra = BridgedBeefyMmrLeafExtra<TestRuntime, ()>;
@@ -105,7 +104,6 @@ impl Chain for TestBridgedChain {
 }
 
 impl ChainWithBeefy for TestBridgedChain {
-	type CommitmentHasher = Keccak256;
 	type MmrHashing = Keccak256;
 	type MmrHash = <Keccak256 as Hash>::Output;
 	type BeefyMmrLeafExtra = ();
@@ -184,12 +182,11 @@ pub fn sign_commitment(
 	let random_validators =
 		rand::seq::index::sample(&mut rand::thread_rng(), total_validators, signature_count);
 
-	let commitment_hash = TestBridgedCommitmentHasher::hash(&commitment.encode());
 	let mut signatures = vec![None; total_validators];
 	for validator_idx in random_validators.iter() {
 		let validator = &validator_pairs[validator_idx];
 		signatures[validator_idx] =
-			Some(validator.as_inner_ref().sign_prehashed(commitment_hash.as_fixed_bytes()).into());
+			Some(BeefySignerAuthority::sign(validator, &commitment.encode()));
 	}
 
 	TestBridgedCommitment { commitment, signatures }

bridges/primitives/beefy/src/lib.rs

@@ -52,11 +52,6 @@ use sp_std::prelude::*;
 /// primitives. Some of types can be configured in low-level pallets, but are constrained
 /// when BEEFY+MMR bundle is used.
 pub trait ChainWithBeefy: Chain {
-	/// The hashing algorithm used to compute the digest of the BEEFY commitment.
-	///
-	/// Corresponds to the hashing algorithm, used by `sc_consensus_beefy::BeefyKeystore`.
-	type CommitmentHasher: sp_runtime::traits::Hash;
-
 	/// The hashing algorithm used to build the MMR.
 	///
 	/// The same algorithm is also used to compute merkle roots in BEEFY
@@ -84,7 +79,7 @@ pub trait ChainWithBeefy: Chain {
 	/// A way to identify a BEEFY validator.
 	///
 	/// Corresponds to the `BeefyId` field of the `pallet-beefy` configuration.
-	type AuthorityId: BeefyAuthorityId<Self::CommitmentHasher> + Parameter;
+	type AuthorityId: BeefyAuthorityId + Parameter;
 
 	/// A way to convert validator id to its raw representation in the BEEFY merkle tree.
 	///
@@ -105,8 +100,6 @@ pub type BeefyValidatorSignatureOf<C> =
 /// Signed BEEFY commitment used by given Substrate chain.
 pub type BeefySignedCommitmentOf<C> =
 	SignedCommitment<BlockNumberOf<C>, BeefyValidatorSignatureOf<C>>;
-/// Hash algorithm, used to compute the digest of the BEEFY commitment before signing it.
-pub type BeefyCommitmentHasher<C> = <C as ChainWithBeefy>::CommitmentHasher;
 /// Hash algorithm used in Beefy MMR construction by given Substrate chain.
 pub type MmrHashingOf<C> = <C as ChainWithBeefy>::MmrHashing;
 /// Hash type, used in MMR construction by given Substrate chain.

prdoc/pr_10763.prdoc

@@ -0,0 +1,21 @@
+title: Make some BEEFY keystore logic more generic
+doc:
+- audience: Node Dev
+  description: |-
+    This PR:
+    1. makes some BEEFY keystore methods more generic:
+    - `sign()`
+    - `public_keys()`
+       This is done by implementing the specific logic in the `BeefyAuthorityId`.
+    2. Removes the `BeefyAuthorityId::SignatureHasher` since for some algorithms it doesn't make sense to have a hasher.
+
+    Also since now the `BeefyAuthorityId` implements both the signing and the verification logic, we should have better consistency.
+
+    Related to https://github.com/paritytech/polkadot-sdk/pull/8707#discussion_r2673377834
+crates:
+- name: sp-consensus-beefy
+  bump: major
+- name: sc-consensus-beefy
+  bump: patch
+- name: pallet-beefy
+  bump: patch

substrate/client/consensus/beefy/src/fisherman.rs

@@ -23,7 +23,7 @@ use sp_api::ProvideRuntimeApi;
 use sp_application_crypto::RuntimeAppPublic;
 use sp_blockchain::HeaderBackend;
 use sp_consensus_beefy::{
-	check_double_voting_proof, AuthorityIdBound, BeefyApi, BeefySignatureHasher, DoubleVotingProof,
+	check_double_voting_proof, AuthorityIdBound, BeefyApi, DoubleVotingProof,
 	OpaqueKeyOwnershipProof, ValidatorSetId,
 };
 use sp_runtime::{
@@ -131,7 +131,7 @@ where
 			(active_rounds.validators(), active_rounds.validator_set_id());
 		let offender_id = proof.offender_id();
 
-		if !check_double_voting_proof::<_, _, BeefySignatureHasher>(&proof) {
+		if !check_double_voting_proof::<_, _>(&proof) {
 			debug!(target: LOG_TARGET, "🥩 Skipping report for bad equivocation {:?}", proof);
 			return Ok(());
 		}

substrate/client/consensus/beefy/src/justification.rs

@@ -20,8 +20,7 @@ use codec::DecodeAll;
 use sp_application_crypto::RuntimeAppPublic;
 use sp_consensus::Error as ConsensusError;
 use sp_consensus_beefy::{
-	AuthorityIdBound, BeefySignatureHasher, KnownSignature, ValidatorSet, ValidatorSetId,
-	VersionedFinalityProof,
+	AuthorityIdBound, KnownSignature, ValidatorSet, ValidatorSetId, VersionedFinalityProof,
 };
 use sp_runtime::traits::{Block as BlockT, NumberFor};
 
@@ -61,11 +60,10 @@ pub(crate) fn verify_with_validator_set<'a, Block: BlockT, AuthorityId: Authorit
 > {
 	match proof {
 		VersionedFinalityProof::V1(signed_commitment) => {
-			let signatories = signed_commitment
-				.verify_signatures::<_, BeefySignatureHasher>(target_number, validator_set)
-				.map_err(|checked_signatures| {
-					(ConsensusError::InvalidJustification, checked_signatures)
-				})?;
+			let signatories =
+				signed_commitment.verify_signatures::<_>(target_number, validator_set).map_err(
+					|checked_signatures| (ConsensusError::InvalidJustification, checked_signatures),
+				)?;
 
 			if signatories.len() >= crate::round::threshold(validator_set.len()) {
 				Ok(signatories)