babe, grandpa: cleanup stale equivocation reports (paritytech#8041)

* grandpa: check equivocation report staleness on `validate_unsigned` * babe: check equivocation report staleness on `validate_unsigned` * node: bump spec_version * babe, grandpa: remove duplicate call destructuring
PolymeshAssociation · Feb 11, 2021 · d615b47 · d615b47
1 parent cf4f97d
commit d615b47
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 49 deletions.
diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
@@ -109,7 +109,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to 0. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 259,
+	spec_version: 263,
 	impl_version: 1,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,

diff --git a/frame/babe/src/equivocation.rs b/frame/babe/src/equivocation.rs
@@ -167,7 +167,7 @@ where
 impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 	type Call = Call<T>;
 	fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity {
-		if let Call::report_equivocation_unsigned(equivocation_proof, _) = call {
+		if let Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof) = call {
 			// discard equivocation report not coming from the local node
 			match source {
 				TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }
@@ -181,6 +181,9 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 				}
 			}
 
+			// check report staleness
+			is_known_offence::<T>(equivocation_proof, key_owner_proof)?;
+
 			ValidTransaction::with_tag_prefix("BabeEquivocation")
 				// We assign the maximum priority for any equivocation report.
 				.priority(TransactionPriority::max_value())
@@ -199,33 +202,35 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 
 	fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
 		if let Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof) = call {
-			// check the membership proof to extract the offender's id
-			let key = (
-				sp_consensus_babe::KEY_TYPE,
-				equivocation_proof.offender.clone(),
-			);
-
-			let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof.clone())
-				.ok_or(InvalidTransaction::BadProof)?;
-
-			// check if the offence has already been reported,
-			// and if so then we can discard the report.
-			let is_known_offence = T::HandleEquivocation::is_known_offence(
-				&[offender],
-				&equivocation_proof.slot_number,
-			);
-
-			if is_known_offence {
-				Err(InvalidTransaction::Stale.into())
-			} else {
-				Ok(())
-			}
+			is_known_offence::<T>(equivocation_proof, key_owner_proof)
 		} else {
 			Err(InvalidTransaction::Call.into())
 		}
 	}
 }
 
+fn is_known_offence<T: Trait>(
+	equivocation_proof: &EquivocationProof<T::Header>,
+	key_owner_proof: &T::KeyOwnerProof,
+) -> Result<(), TransactionValidityError> {
+	// check the membership proof to extract the offender's id
+	let key = (
+		sp_consensus_babe::KEY_TYPE,
+		equivocation_proof.offender.clone(),
+	);
+
+	let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof.clone())
+		.ok_or(InvalidTransaction::BadProof)?;
+
+	// check if the offence has already been reported,
+	// and if so then we can discard the report.
+	if T::HandleEquivocation::is_known_offence(&[offender], &equivocation_proof.slot_number) {
+		Err(InvalidTransaction::Stale.into())
+	} else {
+		Ok(())
+	}
+}
+
 /// A BABE equivocation offence report.
 ///
 /// When a validator released two or more blocks at the same slot.

diff --git a/frame/babe/src/tests.rs b/frame/babe/src/tests.rs
@@ -579,7 +579,16 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() {
 		Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
 			.unwrap();
 
-		// the report should now be considered stale and the transaction is invalid
+		// the report should now be considered stale and the transaction is invalid.
+		// the check for staleness should be done on both `validate_unsigned` and on `pre_dispatch`
+		assert_err!(
+			<Babe as sp_runtime::traits::ValidateUnsigned>::validate_unsigned(
+				TransactionSource::Local,
+				&inner,
+			),
+			InvalidTransaction::Stale,
+		);
+
 		assert_err!(
 			<Babe as sp_runtime::traits::ValidateUnsigned>::pre_dispatch(&inner),
 			InvalidTransaction::Stale,

diff --git a/frame/grandpa/src/equivocation.rs b/frame/grandpa/src/equivocation.rs
@@ -190,7 +190,7 @@ pub struct GrandpaTimeSlot {
 impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 	type Call = Call<T>;
 	fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity {
-		if let Call::report_equivocation_unsigned(equivocation_proof, _) = call {
+		if let Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof) = call {
 			// discard equivocation report not coming from the local node
 			match source {
 				TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }
@@ -204,6 +204,9 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 				}
 			}
 
+			// check report staleness
+			is_known_offence::<T>(equivocation_proof, key_owner_proof)?;
+
 			ValidTransaction::with_tag_prefix("GrandpaEquivocation")
 				// We assign the maximum priority for any equivocation report.
 				.priority(TransactionPriority::max_value())
@@ -223,36 +226,42 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 
 	fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
 		if let Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof) = call {
-			// check the membership proof to extract the offender's id
-			let key = (
-				sp_finality_grandpa::KEY_TYPE,
-				equivocation_proof.offender().clone(),
-			);
-
-			let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof.clone())
-				.ok_or(InvalidTransaction::BadProof)?;
-
-			// check if the offence has already been reported,
-			// and if so then we can discard the report.
-			let time_slot =
-				<T::HandleEquivocation as HandleEquivocation<T>>::Offence::new_time_slot(
-					equivocation_proof.set_id(),
-					equivocation_proof.round(),
-				);
-
-			let is_known_offence = T::HandleEquivocation::is_known_offence(&[offender], &time_slot);
-
-			if is_known_offence {
-				Err(InvalidTransaction::Stale.into())
-			} else {
-				Ok(())
-			}
+			is_known_offence::<T>(equivocation_proof, key_owner_proof)
 		} else {
 			Err(InvalidTransaction::Call.into())
 		}
 	}
 }
 
+fn is_known_offence<T: Trait>(
+	equivocation_proof: &EquivocationProof<T::Hash, T::BlockNumber>,
+	key_owner_proof: &T::KeyOwnerProof,
+) -> Result<(), TransactionValidityError> {
+	// check the membership proof to extract the offender's id
+	let key = (
+		sp_finality_grandpa::KEY_TYPE,
+		equivocation_proof.offender().clone(),
+	);
+
+	let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof.clone())
+		.ok_or(InvalidTransaction::BadProof)?;
+
+	// check if the offence has already been reported,
+	// and if so then we can discard the report.
+	let time_slot = <T::HandleEquivocation as HandleEquivocation<T>>::Offence::new_time_slot(
+		equivocation_proof.set_id(),
+		equivocation_proof.round(),
+	);
+
+	let is_known_offence = T::HandleEquivocation::is_known_offence(&[offender], &time_slot);
+
+	if is_known_offence {
+		Err(InvalidTransaction::Stale.into())
+	} else {
+		Ok(())
+	}
+}
+
 /// A grandpa equivocation offence report.
 #[allow(dead_code)]
 pub struct GrandpaEquivocationOffence<FullIdentification> {

diff --git a/frame/grandpa/src/tests.rs b/frame/grandpa/src/tests.rs
@@ -776,6 +776,15 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() {
 			.unwrap();
 
 		// the report should now be considered stale and the transaction is invalid
+		// the check for staleness should be done on both `validate_unsigned` and on `pre_dispatch`
+		assert_err!(
+			<Grandpa as sp_runtime::traits::ValidateUnsigned>::validate_unsigned(
+				TransactionSource::Local,
+				&call,
+			),
+			InvalidTransaction::Stale,
+		);
+
 		assert_err!(
 			<Grandpa as sp_runtime::traits::ValidateUnsigned>::pre_dispatch(&call),
 			InvalidTransaction::Stale,