From c40ce009558b8b9943e8dff84c66d486d454c63b Mon Sep 17 00:00:00 2001 From: akildemir <34187742+akildemir@users.noreply.github.com> Date: Sun, 17 Dec 2023 01:44:08 +0300 Subject: [PATCH] Slash bad validators (#468) * implement general design * add slashing * bug fixes * fix pr comments * misc fixes * fix grandpa abi call type * Correct rebase artifacts I introduced * Cleanups and corrections 1) Uses vec![] for the OpaqueKeyProof as there's no value to passing it around 2) Remove usage of Babe/Grandpa Offences for tracking if an offence is known for checking if can slash. If can slash, no prior offence must have been known. 3) Rename DisabledIndices to SeraiDisabledIndices, drop historical data for current session only. 4) Doesn't remove from the pre-declared upcoming Serai set upon slash due to breaking light clients. 5) Into/From instead of AsRef for KeyOwnerProofSystem's generic to ensure safety of the conversion. * Correct deduction from TotalAllocatedStake on slash It should only be done if in set and only with allocations contributing to TotalAllocatedStake (Allocation + latest session's PendingDeallocation). * Changes meant for prior commit --------- Co-authored-by: Luke Parker --- Cargo.lock | 2 + substrate/abi/src/babe.rs | 4 +- substrate/abi/src/grandpa.rs | 2 +- substrate/runtime/Cargo.toml | 2 + substrate/runtime/src/lib.rs | 78 +++-- substrate/validator-sets/pallet/Cargo.toml | 2 + substrate/validator-sets/pallet/src/lib.rs | 321 +++++++++++++++++---- 7 files changed, 334 insertions(+), 77 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 731031ef..c9fadb16 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7671,6 +7671,7 @@ dependencies = [ "frame-support", "frame-system", "frame-system-rpc-runtime-api", + "pallet-authorship", "pallet-babe", "pallet-grandpa", "pallet-timestamp", @@ -7748,6 +7749,7 @@ dependencies = [ "sp-io", "sp-runtime", "sp-session", + "sp-staking", "sp-std", ] diff --git a/substrate/abi/src/babe.rs b/substrate/abi/src/babe.rs index b5fe89cb..29bbee9c 100644 --- a/substrate/abi/src/babe.rs +++ b/substrate/abi/src/babe.rs @@ -1,11 +1,11 @@ use sp_consensus_babe::EquivocationProof; -use serai_primitives::Header; +use serai_primitives::{Header, SeraiAddress}; #[derive(Clone, PartialEq, Eq, Debug, scale::Encode, scale::Decode, scale_info::TypeInfo)] pub struct ReportEquivocation { pub equivocation_proof: Box>, - pub key_owner_proof: (), + pub key_owner_proof: SeraiAddress, } // We could define a Babe Config here and use the literal pallet_babe::Call diff --git a/substrate/abi/src/grandpa.rs b/substrate/abi/src/grandpa.rs index 33c01dac..54de8182 100644 --- a/substrate/abi/src/grandpa.rs +++ b/substrate/abi/src/grandpa.rs @@ -5,7 +5,7 @@ use serai_primitives::{BlockNumber, SeraiAddress}; #[derive(Clone, PartialEq, Eq, Debug, scale::Encode, scale::Decode, scale_info::TypeInfo)] pub struct ReportEquivocation { pub equivocation_proof: Box>, - pub key_owner_proof: (), + pub key_owner_proof: SeraiAddress, } #[derive(Clone, PartialEq, Eq, Debug, scale::Encode, scale::Decode, scale_info::TypeInfo)] diff --git a/substrate/runtime/Cargo.toml b/substrate/runtime/Cargo.toml index a5992522..c1666ba9 100644 --- a/substrate/runtime/Cargo.toml +++ b/substrate/runtime/Cargo.toml @@ -46,6 +46,7 @@ frame-benchmarking = { git = "https://github.com/serai-dex/substrate", default-f serai-primitives = { path = "../primitives", default-features = false } pallet-timestamp = { git = "https://github.com/serai-dex/substrate", default-features = false } +pallet-authorship = { git = "https://github.com/serai-dex/substrate", default-features = false } pallet-transaction-payment = { git = "https://github.com/serai-dex/substrate", default-features = false } @@ -98,6 +99,7 @@ std = [ "serai-primitives/std", "pallet-timestamp/std", + "pallet-authorship/std", "pallet-transaction-payment/std", diff --git a/substrate/runtime/src/lib.rs b/substrate/runtime/src/lib.rs index b4828cde..ac13d293 100644 --- a/substrate/runtime/src/lib.rs +++ b/substrate/runtime/src/lib.rs @@ -6,6 +6,8 @@ #[cfg(feature = "std")] include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs")); +use core::marker::PhantomData; + // Re-export all components pub use serai_primitives as primitives; pub use primitives::{BlockNumber, Header}; @@ -55,9 +57,11 @@ use support::{ parameter_types, construct_runtime, }; +use validator_sets::MembershipProof; + +use sp_authority_discovery::AuthorityId as AuthorityDiscoveryId; use babe::AuthorityId as BabeId; use grandpa::AuthorityId as GrandpaId; -use sp_authority_discovery::AuthorityId as AuthorityDiscoveryId; /// Nonce of a transaction in the chain, for a given account. pub type Nonce = u32; @@ -141,8 +145,6 @@ parameter_types! { Weight::from_parts(2u64 * WEIGHT_REF_TIME_PER_SECOND, u64::MAX), NORMAL_DISPATCH_RATIO, ); - - pub const MaxAuthorities: u32 = validator_sets::primitives::MAX_KEY_SHARES_PER_SET; } pub struct CallFilter; @@ -275,20 +277,43 @@ impl in_instructions::Config for Runtime { type RuntimeEvent = RuntimeEvent; } +// for publishing equivocation evidences. +impl frame_system::offchain::SendTransactionTypes for Runtime +where + RuntimeCall: From, +{ + type Extrinsic = Transaction; + type OverarchingCall = RuntimeCall; +} + +// for validating equivocation evidences. +// The following runtime construction doesn't actually implement the pallet as doing so is +// unnecessary +// TODO: Replace the requirement on Config for a requirement on FindAuthor directly +impl pallet_authorship::Config for Runtime { + type FindAuthor = ValidatorSets; + type EventHandler = (); +} + +// Maximum number of authorities per session. +pub type MaxAuthorities = ConstU32<{ validator_sets::primitives::MAX_KEY_SHARES_PER_SET }>; + +/// Longevity of an offence report. +pub type ReportLongevity = ::EpochDuration; + impl babe::Config for Runtime { #[allow(clippy::identity_op)] type EpochDuration = ConstU64<{ 1 * DAYS }>; type ExpectedBlockTime = ConstU64<{ TARGET_BLOCK_TIME * 1000 }>; - type EpochChangeTrigger = pallet_babe::ExternalTrigger; + type EpochChangeTrigger = babe::ExternalTrigger; type DisabledValidators = ValidatorSets; type WeightInfo = (); - type MaxAuthorities = MaxAuthorities; - // TODO: Handle equivocation reports - type KeyOwnerProof = sp_core::Void; - type EquivocationReportSystem = (); + type KeyOwnerProof = MembershipProof; + type EquivocationReportSystem = + babe::EquivocationReportSystem; } impl grandpa::Config for Runtime { @@ -297,10 +322,10 @@ impl grandpa::Config for Runtime { type WeightInfo = (); type MaxAuthorities = MaxAuthorities; - // TODO: Handle equivocation reports type MaxSetIdSessionEntries = ConstU64<0>; - type KeyOwnerProof = sp_core::Void; - type EquivocationReportSystem = (); + type KeyOwnerProof = MembershipProof; + type EquivocationReportSystem = + grandpa::EquivocationReportSystem; } pub type Executive = frame_executive::Executive< @@ -459,18 +484,22 @@ sp_api::impl_runtime_apis! { Babe::next_epoch() } + // This refers to a key being 'owned' by an authority in a system with multiple keys per + // validator + // Since we do not have such an infrastructure, we do not need this fn generate_key_ownership_proof( - _: sp_consensus_babe::Slot, - _: BabeId, + _slot: sp_consensus_babe::Slot, + _authority_id: BabeId, ) -> Option { - None + Some(sp_consensus_babe::OpaqueKeyOwnershipProof::new(vec![])) } fn submit_report_equivocation_unsigned_extrinsic( - _: sp_consensus_babe::EquivocationProof
, + equivocation_proof: sp_consensus_babe::EquivocationProof
, _: sp_consensus_babe::OpaqueKeyOwnershipProof, ) -> Option<()> { - None + let proof = MembershipProof(equivocation_proof.offender.clone().into(), PhantomData); + Babe::submit_unsigned_equivocation_report(equivocation_proof, proof) } } @@ -483,18 +512,19 @@ sp_api::impl_runtime_apis! { Grandpa::current_set_id() } - fn submit_report_equivocation_unsigned_extrinsic( - _: sp_consensus_grandpa::EquivocationProof<::Hash, u64>, - _: sp_consensus_grandpa::OpaqueKeyOwnershipProof, - ) -> Option<()> { - None - } - fn generate_key_ownership_proof( _set_id: sp_consensus_grandpa::SetId, _authority_id: GrandpaId, ) -> Option { - None + Some(sp_consensus_grandpa::OpaqueKeyOwnershipProof::new(vec![])) + } + + fn submit_report_equivocation_unsigned_extrinsic( + equivocation_proof: sp_consensus_grandpa::EquivocationProof<::Hash, u64>, + _: sp_consensus_grandpa::OpaqueKeyOwnershipProof, + ) -> Option<()> { + let proof = MembershipProof(equivocation_proof.offender().clone().into(), PhantomData); + Grandpa::submit_unsigned_equivocation_report(equivocation_proof, proof) } } diff --git a/substrate/validator-sets/pallet/Cargo.toml b/substrate/validator-sets/pallet/Cargo.toml index c678edec..06365875 100644 --- a/substrate/validator-sets/pallet/Cargo.toml +++ b/substrate/validator-sets/pallet/Cargo.toml @@ -27,6 +27,7 @@ sp-std = { git = "https://github.com/serai-dex/substrate", default-features = fa sp-application-crypto = { git = "https://github.com/serai-dex/substrate", default-features = false } sp-runtime = { git = "https://github.com/serai-dex/substrate", default-features = false } sp-session = { git = "https://github.com/serai-dex/substrate", default-features = false } +sp-staking = { git = "https://github.com/serai-dex/substrate", default-features = false } frame-system = { git = "https://github.com/serai-dex/substrate", default-features = false } frame-support = { git = "https://github.com/serai-dex/substrate", default-features = false } @@ -51,6 +52,7 @@ std = [ "sp-application-crypto/std", "sp-runtime/std", "sp-session/std", + "sp-staking/std", "frame-system/std", "frame-support/std", diff --git a/substrate/validator-sets/pallet/src/lib.rs b/substrate/validator-sets/pallet/src/lib.rs index be125ebb..132b4364 100644 --- a/substrate/validator-sets/pallet/src/lib.rs +++ b/substrate/validator-sets/pallet/src/lib.rs @@ -1,33 +1,67 @@ #![cfg_attr(not(feature = "std"), no_std)] +use core::marker::PhantomData; + +use scale::{Encode, Decode}; +use scale_info::TypeInfo; + +use sp_std::{vec, vec::Vec}; +use sp_core::sr25519::{Public, Signature}; +use sp_application_crypto::RuntimePublic; +use sp_session::{ShouldEndSession, GetSessionNumber, GetValidatorCount}; +use sp_runtime::{KeyTypeId, ConsensusEngineId, traits::IsMember}; +use sp_staking::offence::{ReportOffence, Offence, OffenceError}; + +use frame_system::{pallet_prelude::*, RawOrigin}; +use frame_support::{ + pallet_prelude::*, + traits::{DisabledValidators, KeyOwnerProofSystem, FindAuthor}, + BoundedVec, WeakBoundedVec, StoragePrefixedMap, +}; + +use serai_primitives::*; +pub use validator_sets_primitives as primitives; +use primitives::*; + +use coins_pallet::{Pallet as Coins, AllowMint}; +use dex_pallet::Pallet as Dex; + +use pallet_babe::{ + Pallet as Babe, AuthorityId as BabeAuthorityId, EquivocationOffence as BabeEquivocationOffence, +}; +use pallet_grandpa::{ + Pallet as Grandpa, AuthorityId as GrandpaAuthorityId, + EquivocationOffence as GrandpaEquivocationOffence, +}; + +#[derive(Debug, Encode, Decode, TypeInfo, PartialEq, Eq, Clone)] +pub struct MembershipProof(pub Public, pub PhantomData); +impl GetSessionNumber for MembershipProof { + fn session(&self) -> u32 { + let current = Pallet::::session(NetworkId::Serai).unwrap().0; + if Babe::::is_member(&BabeAuthorityId::from(self.0)) { + current + } else { + // if it isn't in the current session, it should have been in the previous one. + current - 1 + } + } +} +impl GetValidatorCount for MembershipProof { + // We only implement and this interface to satisfy trait requirements + // Although this might return the wrong count if the offender was in the previous set, we don't + // rely on it and Substrate only relies on it to offer economic calculations we also don't rely + // on + fn validator_count(&self) -> u32 { + Babe::::authorities().len() as u32 + } +} + #[allow(deprecated, clippy::let_unit_value)] // TODO #[frame_support::pallet] pub mod pallet { use super::*; - use scale_info::TypeInfo; - - use sp_core::sr25519::{Public, Signature}; - use sp_std::{vec, vec::Vec}; - use sp_application_crypto::RuntimePublic; - use sp_session::ShouldEndSession; - use sp_runtime::traits::IsMember; - - use frame_system::pallet_prelude::*; - use frame_support::{ - pallet_prelude::*, traits::DisabledValidators, BoundedVec, WeakBoundedVec, StoragePrefixedMap, - }; - - use serai_primitives::*; - pub use validator_sets_primitives as primitives; - use primitives::*; - - use coins_pallet::{Pallet as Coins, AllowMint}; - use dex_pallet::Pallet as Dex; - - use pallet_babe::{Pallet as Babe, AuthorityId as BabeAuthorityId}; - use pallet_grandpa::{Pallet as Grandpa, AuthorityId as GrandpaAuthorityId}; - #[pallet::config] pub trait Config: frame_system::Config @@ -45,8 +79,6 @@ pub mod pallet { #[pallet::genesis_config] #[derive(Clone, PartialEq, Eq, Debug, Encode, Decode)] pub struct GenesisConfig { - /// Stake requirement to join the initial validator sets. - /// Networks to spawn Serai with, and the stake requirement per key share. /// /// Every participant at genesis will automatically be assumed to have this much stake. @@ -254,14 +286,25 @@ pub mod pallet { /// Pending deallocations, keyed by the Session they become unlocked on. #[pallet::storage] - type PendingDeallocations = - StorageMap<_, Blake2_128Concat, (NetworkId, Session, Public), Amount, OptionQuery>; + type PendingDeallocations = StorageDoubleMap< + _, + Blake2_128Concat, + (NetworkId, Public), + Identity, + Session, + Amount, + OptionQuery, + >; /// The generated key pair for a given validator set instance. #[pallet::storage] #[pallet::getter(fn keys)] pub type Keys = StorageMap<_, Twox64Concat, ValidatorSet, KeyPair, OptionQuery>; + /// Disabled validators. + #[pallet::storage] + pub type SeraiDisabledIndices = StorageMap<_, Identity, u32, Public, OptionQuery>; + #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { @@ -483,6 +526,20 @@ pub mod pallet { Ok(()) } + fn session_to_unlock_on_for_current_set(network: NetworkId) -> Option { + let mut to_unlock_on = Self::session(network)?; + // Move to the next session, as deallocating currently in-use stake is obviously invalid + to_unlock_on.0 += 1; + if network == NetworkId::Serai { + // Since the next Serai set will already have been decided, we can only deallocate one + // session later + to_unlock_on.0 += 1; + } + // Increase the session by one, creating a cooldown period + to_unlock_on.0 += 1; + Some(to_unlock_on) + } + /// Decreases a validator's allocation to a set. /// /// Errors if the capacity provided by this allocation is in use. @@ -557,20 +614,12 @@ pub mod pallet { // Set it to PendingDeallocations, letting it be released upon a future session // This unwrap should be fine as this account is active, meaning a session has occurred - let mut to_unlock_on = Self::session(network).unwrap(); - if network == NetworkId::Serai { - // Since the next Serai set will already have been decided, we can only deallocate once the - // next set ends - to_unlock_on.0 += 2; - } else { - to_unlock_on.0 += 1; - } - // Increase the session by one, creating a cooldown period - to_unlock_on.0 += 1; + let to_unlock_on = Self::session_to_unlock_on_for_current_set(network).unwrap(); let existing = - PendingDeallocations::::get((network, to_unlock_on, account)).unwrap_or(Amount(0)); + PendingDeallocations::::get((network, account), to_unlock_on).unwrap_or(Amount(0)); PendingDeallocations::::set( - (network, to_unlock_on, account), + (network, account), + to_unlock_on, Some(Amount(existing.0 + amount.0)), ); @@ -643,11 +692,12 @@ pub mod pallet { if !Self::handover_completed(network, session) { return None; } - PendingDeallocations::::take((network, session, key)) + PendingDeallocations::::take((network, key), session) } fn rotate_session() { - let prior_serai_participants = Participants::::get(NetworkId::Serai) + // next serai validators that is in the queue. + let now_validators = Participants::::get(NetworkId::Serai) .expect("no Serai participants upon rotate_session"); let prior_serai_session = Self::session(NetworkId::Serai).unwrap(); @@ -660,16 +710,14 @@ pub mod pallet { // Update Babe and Grandpa let session = prior_serai_session.0 + 1; - let validators = prior_serai_participants; - let next_validators = - Participants::::get(NetworkId::Serai).expect("no Serai participants after new_session"); + let next_validators = Participants::::get(NetworkId::Serai).unwrap(); Babe::::enact_epoch_change( WeakBoundedVec::force_from( - validators.iter().copied().map(|(id, w)| (BabeAuthorityId::from(id), w)).collect(), + now_validators.iter().copied().map(|(id, w)| (BabeAuthorityId::from(id), w)).collect(), None, ), WeakBoundedVec::force_from( - next_validators.into_iter().map(|(id, w)| (BabeAuthorityId::from(id), w)).collect(), + next_validators.iter().cloned().map(|(id, w)| (BabeAuthorityId::from(id), w)).collect(), None, ), Some(session), @@ -677,8 +725,18 @@ pub mod pallet { Grandpa::::new_session( true, session, - validators.into_iter().map(|(id, w)| (GrandpaAuthorityId::from(id), w)).collect(), + next_validators.into_iter().map(|(id, w)| (GrandpaAuthorityId::from(id), w)).collect(), ); + + // Clear SeraiDisabledIndices, only preserving keys still present in the new session + // First drain so we don't mutate as we iterate + let mut disabled = vec![]; + for (_, validator) in SeraiDisabledIndices::::drain() { + disabled.push(validator); + } + for disabled in disabled { + Self::disable_serai_validator(disabled); + } } /// Returns the required stake in terms SRI for a given `Balance`. @@ -713,6 +771,75 @@ pub mod pallet { } total_required } + + fn can_slash_serai_validator(validator: Public) -> bool { + // Checks if they're active or actively deallocating (letting us still slash them) + // We could check if they're upcoming/still allocating, yet that'd mean the equivocation is + // invalid (as they aren't actively signing anything) or severely dated + // It's not an edge case worth being comprehensive to due to the complexity of being so + Babe::::is_member(&BabeAuthorityId::from(validator)) || + PendingDeallocations::::iter_prefix((NetworkId::Serai, validator)).next().is_some() + } + + fn slash_serai_validator(validator: Public) { + let network = NetworkId::Serai; + + let mut allocation = Self::allocation((network, validator)).unwrap_or(Amount(0)); + // reduce the current allocation to 0. + Self::set_allocation(network, validator, Amount(0)); + + // Take the pending deallocation from the current session + allocation.0 += PendingDeallocations::::take( + (network, validator), + Self::session_to_unlock_on_for_current_set(network).unwrap(), + ) + .unwrap_or(Amount(0)) + .0; + + // Reduce the TotalAllocatedStake for the network, if in set + // TotalAllocatedStake is the sum of allocations and pending deallocations from the current + // session, since pending deallocations can still be slashed and therefore still contribute + // to economic security, hence the allocation calculations above being above and the ones + // below being below + if InSet::::contains_key(NetworkId::Serai, validator) { + let current_staked = Self::total_allocated_stake(network).unwrap(); + TotalAllocatedStake::::set(network, Some(current_staked - allocation)); + } + + // Clear any other pending deallocations. + for (_, pending) in PendingDeallocations::::drain_prefix((network, validator)) { + allocation.0 += pending.0; + } + + // burn the allocation from the stake account + Coins::::burn( + RawOrigin::Signed(Self::account()).into(), + Balance { coin: Coin::Serai, amount: allocation }, + ) + .unwrap(); + } + + /// Disable a Serai validator, preventing them from further authoring blocks. + /// + /// Returns true if the validator-to-disable was actually a validator. + /// Returns false if they weren't. + fn disable_serai_validator(validator: Public) -> bool { + if let Some(index) = + Babe::::authorities().into_iter().position(|(id, _)| id.into_inner() == validator) + { + SeraiDisabledIndices::::set(u32::try_from(index).unwrap(), Some(validator)); + + let session = Self::session(NetworkId::Serai).unwrap(); + Self::deposit_event(Event::ParticipantRemoved { + set: ValidatorSet { network: NetworkId::Serai, session }, + removed: validator, + }); + + true + } else { + false + } + } } #[pallet::call] @@ -910,10 +1037,104 @@ pub mod pallet { } } + #[rustfmt::skip] + impl + From> KeyOwnerProofSystem<(KeyTypeId, V)> for Pallet { + type Proof = MembershipProof; + type IdentificationTuple = Public; + + fn prove(key: (KeyTypeId, V)) -> Option { + Some(MembershipProof(key.1.into(), PhantomData)) + } + + fn check_proof(key: (KeyTypeId, V), proof: Self::Proof) -> Option { + let validator = key.1.into(); + + // check the offender and the proof offender are the same. + if validator != proof.0 { + return None; + } + + // check validator is valid + if !Self::can_slash_serai_validator(validator) { + return None; + } + + Some(validator) + } + } + + impl ReportOffence> for Pallet { + /// Report an `offence` and reward given `reporters`. + fn report_offence( + _: Vec, + offence: BabeEquivocationOffence, + ) -> Result<(), OffenceError> { + // slash the offender + let offender = offence.offender; + Self::slash_serai_validator(offender); + + // disable it + Self::disable_serai_validator(offender); + + Ok(()) + } + + fn is_known_offence( + offenders: &[Public], + _: & as Offence>::TimeSlot, + ) -> bool { + for offender in offenders { + // It's not a known offence if we can still slash them + if Self::can_slash_serai_validator(*offender) { + return false; + } + } + true + } + } + + impl ReportOffence> for Pallet { + /// Report an `offence` and reward given `reporters`. + fn report_offence( + _: Vec, + offence: GrandpaEquivocationOffence, + ) -> Result<(), OffenceError> { + // slash the offender + let offender = offence.offender; + Self::slash_serai_validator(offender); + + // disable it + Self::disable_serai_validator(offender); + + Ok(()) + } + + fn is_known_offence( + offenders: &[Public], + _slot: & as Offence>::TimeSlot, + ) -> bool { + for offender in offenders { + if Self::can_slash_serai_validator(*offender) { + return false; + } + } + true + } + } + + impl FindAuthor for Pallet { + fn find_author<'a, I>(digests: I) -> Option + where + I: 'a + IntoIterator, + { + let i = Babe::::find_author(digests)?; + Some(Babe::::authorities()[i as usize].0.clone().into()) + } + } + impl DisabledValidators for Pallet { - fn is_disabled(_: u32) -> bool { - // TODO - false + fn is_disabled(index: u32) -> bool { + SeraiDisabledIndices::::get(index).is_some() } } }