diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index c2a2542b55..4b0998c122 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -112,7 +112,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: 262, + spec_version: 263, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 2, diff --git a/substrate/frame/babe/src/equivocation.rs b/substrate/frame/babe/src/equivocation.rs index 9e487769ab..a0a1ff4fa0 100644 --- a/substrate/frame/babe/src/equivocation.rs +++ b/substrate/frame/babe/src/equivocation.rs @@ -167,7 +167,7 @@ where impl frame_support::unsigned::ValidateUnsigned for Module { type Call = Call; 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 frame_support::unsigned::ValidateUnsigned for Module { } } + // check report staleness + is_known_offence::(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 frame_support::unsigned::ValidateUnsigned for Module { 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, - ); - - if is_known_offence { - Err(InvalidTransaction::Stale.into()) - } else { - Ok(()) - } + is_known_offence::(equivocation_proof, key_owner_proof) } else { Err(InvalidTransaction::Call.into()) } } } +fn is_known_offence( + equivocation_proof: &EquivocationProof, + 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) { + 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/substrate/frame/babe/src/tests.rs b/substrate/frame/babe/src/tests.rs index 62b3889680..e4649d253c 100644 --- a/substrate/frame/babe/src/tests.rs +++ b/substrate/frame/babe/src/tests.rs @@ -676,7 +676,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!( + ::validate_unsigned( + TransactionSource::Local, + &inner, + ), + InvalidTransaction::Stale, + ); + assert_err!( ::pre_dispatch(&inner), InvalidTransaction::Stale, diff --git a/substrate/frame/grandpa/src/equivocation.rs b/substrate/frame/grandpa/src/equivocation.rs index 593ebf6ba6..bf05868481 100644 --- a/substrate/frame/grandpa/src/equivocation.rs +++ b/substrate/frame/grandpa/src/equivocation.rs @@ -190,7 +190,7 @@ pub struct GrandpaTimeSlot { impl frame_support::unsigned::ValidateUnsigned for Module { type Call = Call; 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 frame_support::unsigned::ValidateUnsigned for Module { } } + // check report staleness + is_known_offence::(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 frame_support::unsigned::ValidateUnsigned for Module { 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 = - >::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::(equivocation_proof, key_owner_proof) } else { Err(InvalidTransaction::Call.into()) } } } +fn is_known_offence( + equivocation_proof: &EquivocationProof, + 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 = >::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 { diff --git a/substrate/frame/grandpa/src/tests.rs b/substrate/frame/grandpa/src/tests.rs index 4870bf6062..0964be5993 100644 --- a/substrate/frame/grandpa/src/tests.rs +++ b/substrate/frame/grandpa/src/tests.rs @@ -775,6 +775,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!( + ::validate_unsigned( + TransactionSource::Local, + &call, + ), + InvalidTransaction::Stale, + ); + assert_err!( ::pre_dispatch(&call), InvalidTransaction::Stale,