From 1b2ff172b4838d772be21243a13ff115085275cc Mon Sep 17 00:00:00 2001 From: girazoki Date: Wed, 30 Oct 2024 16:02:49 +0100 Subject: [PATCH] a few fixes here and there --- pallets/external-validator-slashes/src/lib.rs | 64 +++++++++------ .../external-validator-slashes/src/mock.rs | 27 ++++++- .../external-validator-slashes/src/tests.rs | 77 ++++++++++++++++++- 3 files changed, 139 insertions(+), 29 deletions(-) diff --git a/pallets/external-validator-slashes/src/lib.rs b/pallets/external-validator-slashes/src/lib.rs index f658212b1..e20cefd7c 100644 --- a/pallets/external-validator-slashes/src/lib.rs +++ b/pallets/external-validator-slashes/src/lib.rs @@ -76,8 +76,6 @@ pub mod pallet { + TryFrom; /// A conversion from account ID to validator ID. - /// - /// Its cost must be at most one storage read. type ValidatorIdOf: Convert>; /// Number of eras that slashes are deferred by, after computation. @@ -91,6 +89,7 @@ pub mod pallet { #[pallet::constant] type BondingDuration: Get; + // SlashId type, used as a counter on the number of slashes type SlashId: Default + FullCodec + TypeInfo @@ -106,8 +105,10 @@ pub mod pallet { /// Interface for interacting with a session pallet. type SessionInterface: SessionInterface; + /// Era index provider, used to fetch the active era among other thins type EraIndexProvider: EraIndexProvider; + /// Invulnerable provider, used to get the invulnerables to know when not to slash type InvulnerablesProvider: InvulnerablesProvider; /// The weight information of this pallet. @@ -123,6 +124,7 @@ pub mod pallet { ProvidedNonSlashableEra, ActiveEraNotSet, DeferPeriodIsOver, + ErrorComputingSlash, } #[pallet::pallet] @@ -142,6 +144,7 @@ pub mod pallet { #[pallet::unbounded] pub type BondedEras = StorageValue<_, Vec<(EraIndex, SessionIndex)>, ValueQuery>; + /// A counter on the number of slashes we have performed #[pallet::storage] #[pallet::getter(fn next_slash_id)] pub type NextSlashId = StorageValue<_, T::SlashId, ValueQuery>; @@ -155,6 +158,7 @@ pub mod pallet { #[pallet::call] impl Pallet { + /// Cancel a slash that was deferred for a later era #[pallet::call_index(0)] #[pallet::weight(T::WeightInfo::cancel_deferred_slash(slash_indices.len() as u32))] pub fn cancel_deferred_slash( @@ -166,6 +170,7 @@ pub mod pallet { let active_era = T::EraIndexProvider::active_era().index; + // We need to be in the defer period ensure!( era <= active_era .saturating_add(T::SlashDeferDuration::get().saturating_add(One::one())) @@ -178,7 +183,7 @@ pub mod pallet { is_sorted_and_unique(&slash_indices), Error::::NotSortedAndUnique ); - + // fetch slashes for the era in which we want to defer let mut era_slashes = Slashes::::get(&era); let last_item = slash_indices[slash_indices.len() - 1]; @@ -192,6 +197,7 @@ pub mod pallet { era_slashes.remove(index); } + // insert back slashes Slashes::::insert(&era, &era_slashes); Ok(()) } @@ -211,26 +217,33 @@ pub mod pallet { let slash_defer_duration = T::SlashDeferDuration::get(); - let window_start = active_era.saturating_sub(T::BondingDuration::get()); - ensure!(era >= window_start, Error::::ProvidedNonSlashableEra); + let _ = T::EraIndexProvider::era_to_session_start(era) + .ok_or(Error::::ProvidedNonSlashableEra)?; let next_slash_id = NextSlashId::::get(); - let slash = Slash:: { - slash_id: next_slash_id, - reporters: vec![], - confirmed: false, - - validator, + let slash = compute_slash::( percentage, + next_slash_id, + era, + validator, + slash_defer_duration, + ) + .ok_or(Error::::ErrorComputingSlash)?; + + // If we defer duration is 0, we immediately apply and confirm + let era_to_consider = if slash_defer_duration == 0 { + era + } else { + era.saturating_add(slash_defer_duration) + .saturating_add(One::one()) }; - let mut era_slashes = Slashes::::get(&era); + + let mut era_slashes = Slashes::::get(&era_to_consider); era_slashes.push(slash); - Slashes::::insert( - &era.saturating_add(slash_defer_duration) - .saturating_add(One::one()), - &era_slashes, - ); + + Slashes::::insert(era_to_consider, &era_slashes); + NextSlashId::::put(next_slash_id.saturating_add(One::one())); Ok(()) } @@ -337,12 +350,17 @@ where slash_era + slash_defer_duration + 1, ); - Slashes::::mutate( - slash_era - .saturating_add(slash_defer_duration) - .saturating_add(One::one()), - move |for_later| for_later.push(slash), - ); + // Cover slash defer duration equal to 0 + if slash_defer_duration == 0 { + Slashes::::mutate(slash_era, move |for_now| for_now.push(slash)); + } else { + Slashes::::mutate( + slash_era + .saturating_add(slash_defer_duration) + .saturating_add(One::one()), + move |for_later| for_later.push(slash), + ); + } // Fix unwrap next_slash_id = next_slash_id.saturating_add(One::one()); diff --git a/pallets/external-validator-slashes/src/mock.rs b/pallets/external-validator-slashes/src/mock.rs index ef9305073..e5661ea33 100644 --- a/pallets/external-validator-slashes/src/mock.rs +++ b/pallets/external-validator-slashes/src/mock.rs @@ -18,7 +18,7 @@ use { crate as external_validator_slashes, frame_support::{ parameter_types, - traits::{ConstU16, ConstU64}, + traits::{ConstU16, ConstU64, Get}, }, frame_system as system, sp_core::H256, @@ -114,6 +114,7 @@ pub struct MockEraIndexProvider; thread_local! { pub static ERA_INDEX: RefCell = const { RefCell::new(0) }; + pub static DEFER_PERIOD: RefCell = const { RefCell::new(2) }; } impl MockEraIndexProvider { @@ -130,7 +131,13 @@ impl EraIndexProvider for MockEraIndexProvider { } } fn era_to_session_start(era_index: EraIndex) -> Option { - Some(era_index.into()) + let active_era = Self::active_era().index; + if era_index > active_era || era_index < active_era.saturating_sub(BondingDuration::get()) { + None + } else { + // Else we assume eras start at the same time as sessions + Some(era_index.into()) + } } } @@ -176,8 +183,20 @@ impl InvulnerablesProvider for MockInvulnerableProvider { } } +pub struct DeferPeriodGetter; +impl Get for DeferPeriodGetter { + fn get() -> EraIndex { + DEFER_PERIOD.with(|q| (*q.borrow()).clone()) + } +} + +impl DeferPeriodGetter { + pub fn with_defer_period(defer_period: EraIndex) { + DEFER_PERIOD.with(|r| *r.borrow_mut() = defer_period); + } +} + parameter_types! { - pub const DeferPeriod: u32 = 2u32; pub const BondingDuration: u32 = 5u32; } @@ -185,7 +204,7 @@ impl external_validator_slashes::Config for Test { type RuntimeEvent = RuntimeEvent; type ValidatorId = ::AccountId; type ValidatorIdOf = IdentityValidator; - type SlashDeferDuration = DeferPeriod; + type SlashDeferDuration = DeferPeriodGetter; type BondingDuration = BondingDuration; type SlashId = u32; type SessionInterface = (); diff --git a/pallets/external-validator-slashes/src/tests.rs b/pallets/external-validator-slashes/src/tests.rs index ef512daee..5609346e4 100644 --- a/pallets/external-validator-slashes/src/tests.rs +++ b/pallets/external-validator-slashes/src/tests.rs @@ -170,7 +170,7 @@ fn test_on_offence_injects_offences() { ); // current era (1) + defer period + 1 let slash_era = 0 - .saturating_add(crate::mock::DeferPeriod::get()) + .saturating_add(crate::mock::DeferPeriodGetter::get()) .saturating_add(One::one()); assert_eq!( @@ -202,13 +202,86 @@ fn test_on_offence_does_not_work_for_invulnerables() { ); // current era (1) + defer period + 1 let slash_era = 1 - .saturating_add(crate::mock::DeferPeriod::get()) + .saturating_add(crate::mock::DeferPeriodGetter::get()) .saturating_add(One::one()); assert_eq!(Slashes::::get(slash_era), vec![]); }); } +#[test] +fn defer_period_of_zero_confirms_immediately_slashes() { + new_test_ext().execute_with(|| { + crate::mock::DeferPeriodGetter::with_defer_period(0); + start_era(0, 0); + assert_ok!(ExternalValidatorSlashes::force_inject_slash( + RuntimeOrigin::root(), + 0, + 1u64, + Perbill::from_percent(75) + )); + assert_eq!( + Slashes::::get(0), + vec![Slash { + validator: 1, + percentage: Perbill::from_percent(75), + confirmed: true, + reporters: vec![], + slash_id: 0 + }] + ); + }); +} + +#[test] +fn we_cannot_cancel_anything_with_defer_period_zero() { + new_test_ext().execute_with(|| { + crate::mock::DeferPeriodGetter::with_defer_period(0); + start_era(0, 0); + assert_ok!(ExternalValidatorSlashes::force_inject_slash( + RuntimeOrigin::root(), + 0, + 1u64, + Perbill::from_percent(75) + )); + assert_noop!( + ExternalValidatorSlashes::cancel_deferred_slash(RuntimeOrigin::root(), 0, vec![0]), + Error::::DeferPeriodIsOver + ); + }); +} + +#[test] +fn test_on_offence_defer_period_0() { + new_test_ext().execute_with(|| { + crate::mock::DeferPeriodGetter::with_defer_period(0); + start_era(0, 0); + start_era(1, 1); + Pallet::::on_offence( + &[OffenceDetails { + // 1 and 2 are invulnerables + offender: (3, ()), + reporters: vec![], + }], + &[Perbill::from_percent(75)], + 0, + ); + + let slash_era = 0; + + assert_eq!( + Slashes::::get(slash_era), + vec![Slash { + validator: 3, + percentage: Perbill::from_percent(75), + confirmed: true, + reporters: vec![], + slash_id: 0 + }] + ); + }); +} + fn start_era(era_index: EraIndex, session_index: SessionIndex) { Pallet::::on_era_start(era_index, session_index); crate::mock::MockEraIndexProvider::with_era(era_index);