[BEEFY] Return valid signatures when verifying commitment

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

@@ -16,12 +16,11 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use crate::keystore::BeefyKeystore;
-use codec::{DecodeAll, Encode};
+use codec::DecodeAll;
 use sp_consensus::Error as ConsensusError;
 use sp_consensus_beefy::{
 	ecdsa_crypto::{AuthorityId, Signature},
-	ValidatorSet, ValidatorSetId, VersionedFinalityProof,
+	BeefySignatureHasher, KnownSignature, ValidatorSet, ValidatorSetId, VersionedFinalityProof,
 };
 use sp_runtime::traits::{Block as BlockT, NumberFor};
 
@@ -45,53 +44,39 @@ pub(crate) fn decode_and_verify_finality_proof<Block: BlockT>(
 ) -> Result<BeefyVersionedFinalityProof<Block>, (ConsensusError, u32)> {
 	let proof = <BeefyVersionedFinalityProof<Block>>::decode_all(&mut &*encoded)
 		.map_err(|_| (ConsensusError::InvalidJustification, 0))?;
-	verify_with_validator_set::<Block>(target_number, validator_set, &proof).map(|_| proof)
+	verify_with_validator_set::<Block>(target_number, validator_set, &proof)?;
+	Ok(proof)
 }
 
 /// Verify the Beefy finality proof against the validator set at the block it was generated.
-pub(crate) fn verify_with_validator_set<Block: BlockT>(
+pub(crate) fn verify_with_validator_set<'a, Block: BlockT>(
 	target_number: NumberFor<Block>,
-	validator_set: &ValidatorSet<AuthorityId>,
-	proof: &BeefyVersionedFinalityProof<Block>,
-) -> Result<(), (ConsensusError, u32)> {
-	let mut signatures_checked = 0u32;
+	validator_set: &'a ValidatorSet<AuthorityId>,
+	proof: &'a BeefyVersionedFinalityProof<Block>,
+) -> Result<Vec<KnownSignature<&'a AuthorityId, &'a Signature>>, (ConsensusError, u32)> {
 	match proof {
 		VersionedFinalityProof::V1(signed_commitment) => {
-			if signed_commitment.signatures.len() != validator_set.len() ||
-				signed_commitment.commitment.validator_set_id != validator_set.id() ||
-				signed_commitment.commitment.block_number != target_number
-			{
-				return Err((ConsensusError::InvalidJustification, 0))
-			}
-
-			// Arrangement of signatures in the commitment should be in the same order
-			// as validators for that set.
-			let message = signed_commitment.commitment.encode();
-			let valid_signatures = validator_set
-				.validators()
-				.into_iter()
-				.zip(signed_commitment.signatures.iter())
-				.filter(|(id, signature)| {
-					signature
-						.as_ref()
-						.map(|sig| {
-							signatures_checked += 1;
-							BeefyKeystore::verify(*id, sig, &message[..])
-						})
-						.unwrap_or(false)
-				})
-				.count();
-			if valid_signatures >= crate::round::threshold(validator_set.len()) {
-				Ok(())
+			let signatories = signed_commitment
+				.verify_signatures::<_, BeefySignatureHasher>(target_number, validator_set)
+				.map_err(|checked_signatures| {
+					(ConsensusError::InvalidJustification, checked_signatures)
+				})?;
+
+			if signatories.len() >= crate::round::threshold(validator_set.len()) {
+				Ok(signatories)
 			} else {
-				Err((ConsensusError::InvalidJustification, signatures_checked))
+				Err((
+					ConsensusError::InvalidJustification,
+					signed_commitment.signature_count() as u32,
+				))
 			}
 		},
 	}
 }
 
 #[cfg(test)]
 pub(crate) mod tests {
+	use codec::Encode;
 	use sp_consensus_beefy::{
 		known_payloads, test_utils::Keyring, Commitment, Payload, SignedCommitment,
 		VersionedFinalityProof,

substrate/primitives/consensus/beefy/src/commitment.rs

@@ -19,8 +19,30 @@ use alloc::{vec, vec::Vec};
 use codec::{Decode, Encode, Error, Input};
 use core::cmp;
 use scale_info::TypeInfo;
+use sp_application_crypto::RuntimeAppPublic;
+use sp_runtime::traits::Hash;
+
+use crate::{BeefyAuthorityId, Payload, ValidatorSet, ValidatorSetId};
+
+/// A commitment signature, accompanied by the id of the validator that it belongs to.
+#[derive(Debug)]
+pub struct KnownSignature<TAuthorityId, TSignature> {
+	/// The signing validator.
+	pub validator_id: TAuthorityId,
+	/// The signature.
+	pub signature: TSignature,
+}
 
-use crate::{Payload, ValidatorSetId};
+impl<TAuthorityId: Clone, TSignature: Clone> KnownSignature<&TAuthorityId, &TSignature> {
+	/// Creates a `KnownSignature<TAuthorityId, TSignature>` from an
+	/// `KnownSignature<&TAuthorityId, &TSignature>`.
+	pub fn to_owned(&self) -> KnownSignature<TAuthorityId, TSignature> {
+		KnownSignature {
+			validator_id: self.validator_id.clone(),
+			signature: self.signature.clone(),
+		}
+	}
+}
 
 /// A commitment signed by GRANDPA validators as part of BEEFY protocol.
 ///
@@ -113,9 +135,49 @@ impl<TBlockNumber: core::fmt::Debug, TSignature> core::fmt::Display
 
 impl<TBlockNumber, TSignature> SignedCommitment<TBlockNumber, TSignature> {
 	/// Return the number of collected signatures.
-	pub fn no_of_signatures(&self) -> usize {
+	pub fn signature_count(&self) -> usize {
 		self.signatures.iter().filter(|x| x.is_some()).count()
 	}
+
+	/// Verify all the commitment signatures against the validator set that was active
+	/// at the block where the commitment was generated.
+	///
+	/// Returns the valid validator-signature pairs if the commitment can be verified.
+	pub fn verify_signatures<'a, TAuthorityId, MsgHash>(
+		&'a self,
+		target_number: TBlockNumber,
+		validator_set: &'a ValidatorSet<TAuthorityId>,
+	) -> Result<Vec<KnownSignature<&'a TAuthorityId, &'a TSignature>>, u32>
+	where
+		TBlockNumber: Clone + Encode + PartialEq,
+		TAuthorityId: RuntimeAppPublic<Signature = TSignature> + BeefyAuthorityId<MsgHash>,
+		MsgHash: Hash,
+	{
+		if self.signatures.len() != validator_set.len() ||
+			self.commitment.validator_set_id != validator_set.id() ||
+			self.commitment.block_number != target_number
+		{
+			return Err(0)
+		}
+
+		// Arrangement of signatures in the commitment should be in the same order
+		// as validators for that set.
+		let encoded_commitment = self.commitment.encode();
+		let signatories: Vec<_> = validator_set
+			.validators()
+			.into_iter()
+			.zip(self.signatures.iter())
+			.filter_map(|(id, maybe_signature)| {
+				let signature = maybe_signature.as_ref()?;
+				match BeefyAuthorityId::verify(id, signature, &encoded_commitment) {
+					true => Some(KnownSignature { validator_id: id, signature }),
+					false => None,
+				}
+			})
+			.collect();
+
+		Ok(signatories)
+	}
 }
 
 /// Type to be used to denote placement of signatures
@@ -439,13 +501,13 @@ mod tests {
 			commitment,
 			signatures: vec![None, None, Some(sigs.0), Some(sigs.1)],
 		};
-		assert_eq!(signed.no_of_signatures(), 2);
+		assert_eq!(signed.signature_count(), 2);
 
 		// when
 		signed.signatures[2] = None;
 
 		// then
-		assert_eq!(signed.no_of_signatures(), 1);
+		assert_eq!(signed.signature_count(), 1);
 	}
 
 	#[test]

substrate/primitives/consensus/beefy/src/lib.rs

@@ -43,7 +43,7 @@ pub mod witness;
 #[cfg(feature = "std")]
 pub mod test_utils;
 
-pub use commitment::{Commitment, SignedCommitment, VersionedFinalityProof};
+pub use commitment::{Commitment, KnownSignature, SignedCommitment, VersionedFinalityProof};
 pub use payload::{known_payloads, BeefyPayloadId, Payload, PayloadProvider};
 
 use alloc::vec::Vec;