diff --git a/runtime/parachains/src/builder.rs b/runtime/parachains/src/builder.rs index f2c40924edf5..9fa954066c1c 100644 --- a/runtime/parachains/src/builder.rs +++ b/runtime/parachains/src/builder.rs @@ -595,6 +595,7 @@ impl BenchBuilder { let (para_id, core_idx, group_idx) = self.create_indexes(seed); let candidate_hash = CandidateHash(H256::from(byte32_slice_from(seed))); + let relay_parent = H256::from(byte32_slice_from(seed)); Self::add_availability( para_id, @@ -614,9 +615,12 @@ impl BenchBuilder { // so we make sure that we have a super majority with valid statements. let dispute_statement = if validator_index % 4 == 0 { DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) + } else if validator_index < 3 { + // Set two votes as backing for the dispute set to be accepted + DisputeStatement::Valid( + ValidDisputeStatementKind::BackingValid(relay_parent) + ) } else { - // Note that in the future we could use some availability votes as an - // implicit valid kind. DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) }; let data = dispute_statement.payload_data(candidate_hash.clone(), session); diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 5e6a257344a5..bf918c61724f 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -33,7 +33,7 @@ use sp_runtime::{ traits::{AppVerify, One, Saturating, Zero}, DispatchError, RuntimeDebug, SaturatedConversion, }; -use sp_std::{cmp::Ordering, prelude::*}; +use sp_std::{cmp::Ordering, collections::btree_set::BTreeSet, prelude::*}; #[cfg(test)] #[allow(unused_imports)] @@ -85,6 +85,7 @@ pub trait SlashingHandler { session: SessionIndex, candidate_hash: CandidateHash, losers: impl IntoIterator, + backers: impl IntoIterator, ); /// Punish a series of validators who were against a valid parablock. This @@ -93,6 +94,7 @@ pub trait SlashingHandler { session: SessionIndex, candidate_hash: CandidateHash, losers: impl IntoIterator, + backers: impl IntoIterator, ); /// Called by the initializer to initialize the slashing pallet. @@ -110,6 +112,7 @@ impl SlashingHandler for () { _: SessionIndex, _: CandidateHash, _: impl IntoIterator, + _: impl IntoIterator, ) { } @@ -117,6 +120,7 @@ impl SlashingHandler for () { _: SessionIndex, _: CandidateHash, _: impl IntoIterator, + _: impl IntoIterator, ) { } @@ -459,6 +463,18 @@ pub mod pallet { DisputeState, >; + /// Backing votes stored for each dispute. + /// This storage is used for slashing. + #[pallet::storage] + pub(super) type BackersOnDisputes = StorageDoubleMap< + _, + Twox64Concat, + SessionIndex, + Blake2_128Concat, + CandidateHash, + BTreeSet, + >; + /// All included blocks on the chain, as well as the block number in this chain that /// should be reverted back to if the candidate is disputed and determined to be invalid. #[pallet::storage] @@ -511,7 +527,11 @@ pub mod pallet { DuplicateStatement, /// A dispute where there are only votes on one side. SingleSidedDispute, - /// Unconfirmed dispute statement sets provided + /// A dispute vote from a malicious backer. + MaliciousBacker, + /// No backing votes were provides along dispute statements. + MissingBackingVotes, + /// Unconfirmed dispute statement sets provided. UnconfirmedDispute, } @@ -569,6 +589,8 @@ impl DisputeStateFlags { struct ImportSummary { /// The new state, with all votes imported. state: DisputeState, + /// List of validators who backed the candidate being disputed. + backers: BTreeSet, /// Validators to slash for being (wrongly) on the AGAINST side. slash_against: Vec, /// Validators to slash for being (wrongly) on the FOR side. @@ -585,6 +607,48 @@ enum VoteImportError { ValidatorIndexOutOfBounds, /// Found a duplicate statement in the dispute statement set. DuplicateStatement, + /// Found an explicit valid statement after backing statement. + /// Backers should not participate in explicit voting so this is + /// only possible on malicious backers. + MaliciousBacker, +} + +#[derive(RuntimeDebug, Copy, Clone, PartialEq, Eq)] +enum VoteKind { + /// A backing vote that is counted as "for" vote in dispute resolution. + Backing, + /// Either an approval vote or and explicit dispute "for" vote. + ExplicitValid, + /// An explicit dispute "against" vote. + Invalid, +} + +impl From<&DisputeStatement> for VoteKind { + fn from(statement: &DisputeStatement) -> Self { + if statement.is_backing() { + Self::Backing + } else if statement.indicates_validity() { + Self::ExplicitValid + } else { + Self::Invalid + } + } +} + +impl VoteKind { + fn is_valid(&self) -> bool { + match self { + Self::Backing | Self::ExplicitValid => true, + Self::Invalid => false, + } + } + + fn is_backing(&self) -> bool { + match self { + Self::Backing => true, + Self::Invalid | Self::ExplicitValid => false, + } + } } impl From for Error { @@ -592,6 +656,7 @@ impl From for Error { match e { VoteImportError::ValidatorIndexOutOfBounds => Error::::ValidatorIndexOutOfBounds, VoteImportError::DuplicateStatement => Error::::DuplicateStatement, + VoteImportError::MaliciousBacker => Error::::MaliciousBacker, } } } @@ -601,8 +666,8 @@ impl From for Error { struct ImportUndo { /// The validator index to which to associate the statement import. validator_index: ValidatorIndex, - /// The direction of the vote, for block validity (`true`) or invalidity (`false`). - valid: bool, + /// The kind and direction of the vote. + vote_kind: VoteKind, /// Has the validator participated before, i.e. in backing or /// with an opposing vote. new_participant: bool, @@ -610,25 +675,53 @@ struct ImportUndo { struct DisputeStateImporter { state: DisputeState, + backers: BTreeSet, now: BlockNumber, new_participants: bitvec::vec::BitVec, pre_flags: DisputeStateFlags, + pre_state: DisputeState, + // The list of backing votes before importing the batch of votes. This field should be + // initialized as empty on the first import of the dispute votes and should remain non-empty + // afterwards. + // + // If a dispute has concluded and the candidate was found invalid, we may want to slash as many + // backers as possible. This list allows us to slash these backers once their votes have been + // imported post dispute conclusion. + pre_backers: BTreeSet, } impl DisputeStateImporter { - fn new(state: DisputeState, now: BlockNumber) -> Self { + fn new( + state: DisputeState, + backers: BTreeSet, + now: BlockNumber, + ) -> Self { let pre_flags = DisputeStateFlags::from_state(&state); let new_participants = bitvec::bitvec![u8, BitOrderLsb0; 0; state.validators_for.len()]; - - DisputeStateImporter { state, now, new_participants, pre_flags } + // consistency checks + for i in backers.iter() { + debug_assert_eq!(state.validators_for.get(i.0 as usize).map(|b| *b), Some(true)); + } + let pre_state = state.clone(); + let pre_backers = backers.clone(); + + DisputeStateImporter { + state, + backers, + now, + new_participants, + pre_flags, + pre_state, + pre_backers, + } } fn import( &mut self, validator: ValidatorIndex, - valid: bool, + kind: VoteKind, ) -> Result { - let (bits, other_bits) = if valid { + let (bits, other_bits) = if kind.is_valid() { (&mut self.state.validators_for, &mut self.state.validators_against) } else { (&mut self.state.validators_against, &mut self.state.validators_for) @@ -637,18 +730,31 @@ impl DisputeStateImporter { // out of bounds or already participated match bits.get(validator.0 as usize).map(|b| *b) { None => return Err(VoteImportError::ValidatorIndexOutOfBounds), - Some(true) => return Err(VoteImportError::DuplicateStatement), + Some(true) => { + // We allow backing statements to be imported after an + // explicit "for" vote, but not the other way around. + match (kind.is_backing(), self.backers.contains(&validator)) { + (true, true) | (false, false) => + return Err(VoteImportError::DuplicateStatement), + (false, true) => return Err(VoteImportError::MaliciousBacker), + (true, false) => {}, + } + }, Some(false) => {}, } - // inefficient, and just for extra sanity. - if validator.0 as usize >= self.new_participants.len() { - return Err(VoteImportError::ValidatorIndexOutOfBounds) - } + // consistency check + debug_assert!((validator.0 as usize) < self.new_participants.len()); - let mut undo = ImportUndo { validator_index: validator, valid, new_participant: false }; + let mut undo = + ImportUndo { validator_index: validator, vote_kind: kind, new_participant: false }; bits.set(validator.0 as usize, true); + if kind.is_backing() { + let is_new = self.backers.insert(validator); + // invariant check + debug_assert!(is_new); + } // New participants tracks those validators by index, which didn't appear on either // side of the dispute until now (so they make a first appearance). @@ -663,12 +769,16 @@ impl DisputeStateImporter { /// Revert a done transaction. fn undo(&mut self, undo: ImportUndo) { - if undo.valid { + if undo.vote_kind.is_valid() { self.state.validators_for.set(undo.validator_index.0 as usize, false); } else { self.state.validators_against.set(undo.validator_index.0 as usize, false); } + if undo.vote_kind.is_backing() { + self.backers.remove(&undo.validator_index); + } + if undo.new_participant { self.new_participants.set(undo.validator_index.0 as usize, false); } @@ -681,9 +791,9 @@ impl DisputeStateImporter { let pre_post_contains = |flags| (pre_flags.contains(flags), post_flags.contains(flags)); - // 1. Check for fresh FOR supermajority. Only if not already concluded. - let slash_against = - if let (false, true) = pre_post_contains(DisputeStateFlags::FOR_SUPERMAJORITY) { + // 1. Check for FOR supermajority. + let slash_against = match pre_post_contains(DisputeStateFlags::FOR_SUPERMAJORITY) { + (false, true) => { if self.state.concluded_at.is_none() { self.state.concluded_at = Some(self.now.clone()); } @@ -694,25 +804,59 @@ impl DisputeStateImporter { .iter_ones() .map(|i| ValidatorIndex(i as _)) .collect() - } else { + }, + (true, true) => { + // provide new AGAINST voters to slash. + self.state + .validators_against + .iter_ones() + .filter(|i| self.pre_state.validators_against.get(*i).map_or(false, |b| !*b)) + .map(|i| ValidatorIndex(i as _)) + .collect() + }, + (true, false) => { + log::error!("Dispute statements are never removed. This is a bug"); Vec::new() - }; + }, + (false, false) => Vec::new(), + }; - // 2. Check for fresh AGAINST supermajority. - let slash_for = - if let (false, true) = pre_post_contains(DisputeStateFlags::AGAINST_SUPERMAJORITY) { + // 2. Check for AGAINST supermajority. + let slash_for = match pre_post_contains(DisputeStateFlags::AGAINST_SUPERMAJORITY) { + (false, true) => { if self.state.concluded_at.is_none() { self.state.concluded_at = Some(self.now.clone()); } // provide FOR voters to slash. self.state.validators_for.iter_ones().map(|i| ValidatorIndex(i as _)).collect() - } else { + }, + (true, true) => { + // provide new FOR voters to slash including new backers + // who might have voted FOR before + let new_backing_vote = |i: &ValidatorIndex| -> bool { + !self.pre_backers.contains(i) && self.backers.contains(i) + }; + self.state + .validators_for + .iter_ones() + .filter(|i| { + self.pre_state.validators_for.get(*i).map_or(false, |b| !*b) || + new_backing_vote(&ValidatorIndex(*i as _)) + }) + .map(|i| ValidatorIndex(i as _)) + .collect() + }, + (true, false) => { + log::error!("Dispute statements are never removed. This is a bug"); Vec::new() - }; + }, + (false, false) => Vec::new(), + }; ImportSummary { state: self.state, + backers: self.backers, slash_against, slash_for, new_participants: self.new_participants, @@ -815,6 +959,8 @@ impl Pallet { // This should be small, as disputes are rare, so `None` is fine. #[allow(deprecated)] >::remove_prefix(to_prune, None); + #[allow(deprecated)] + >::remove_prefix(to_prune, None); // This is larger, and will be extracted to the `shared` pallet for more proper pruning. // TODO: https://github.com/paritytech/polkadot/issues/3469 @@ -903,11 +1049,15 @@ impl Pallet { } }; + let backers = + >::get(&set.session, &set.candidate_hash).unwrap_or_default(); + // Check and import all votes. let summary = { - let mut importer = DisputeStateImporter::new(dispute_state, now); + let mut importer = DisputeStateImporter::new(dispute_state, backers, now); for (i, (statement, validator_index, signature)) in set.statements.iter().enumerate() { - // assure the validator index and is present in the session info + // ensure the validator index is present in the session info + // and the signature is valid let validator_public = match session_info.validators.get(*validator_index) { None => { filter.remove_index(i); @@ -916,9 +1066,9 @@ impl Pallet { Some(v) => v, }; - let valid = statement.indicates_validity(); + let kind = VoteKind::from(statement); - let undo = match importer.import(*validator_index, valid) { + let undo = match importer.import(*validator_index, kind) { Ok(u) => u, Err(_) => { filter.remove_index(i); @@ -1016,13 +1166,16 @@ impl Pallet { } }; + let backers = + >::get(&set.session, &set.candidate_hash).unwrap_or_default(); + // Import all votes. They were pre-checked. let summary = { - let mut importer = DisputeStateImporter::new(dispute_state, now); + let mut importer = DisputeStateImporter::new(dispute_state, backers, now); for (statement, validator_index, _signature) in &set.statements { - let valid = statement.indicates_validity(); + let kind = VoteKind::from(statement); - importer.import(*validator_index, valid).map_err(Error::::from)?; + importer.import(*validator_index, kind).map_err(Error::::from)?; } importer.finish() @@ -1041,6 +1194,11 @@ impl Pallet { byzantine_threshold(summary.state.validators_for.len()), Error::::UnconfirmedDispute, ); + let backers = summary.backers; + // Reject statements with no accompanying backing votes. + ensure!(!backers.is_empty(), Error::::MissingBackingVotes); + >::insert(&set.session, &set.candidate_hash, backers.clone()); + // AUDIT: from now on, no error should be returned. let DisputeStatementSet { ref session, ref candidate_hash, .. } = set; let session = *session; @@ -1085,10 +1243,16 @@ impl Pallet { session, candidate_hash, summary.slash_against, + backers.clone(), ); // an invalid candidate, according to 2/3. Punish those on the 'for' side. - T::SlashingHandler::punish_for_invalid(session, candidate_hash, summary.slash_for); + T::SlashingHandler::punish_for_invalid( + session, + candidate_hash, + summary.slash_for, + backers, + ); } >::insert(&session, &candidate_hash, &summary.state); diff --git a/runtime/parachains/src/disputes/slashing.rs b/runtime/parachains/src/disputes/slashing.rs index bde4dec614a5..fd127d7f0789 100644 --- a/runtime/parachains/src/disputes/slashing.rs +++ b/runtime/parachains/src/disputes/slashing.rs @@ -16,32 +16,31 @@ //! Dispute slashing pallet. //! -//! Once a dispute is concluded, we want to slash validators -//! who were on the wrong side of the dispute. The slashing amount -//! depends on whether the candidate was valid (small) or invalid (big). -//! In addition to that, we might want to kick out the validators from the -//! active set. +//! Once a dispute is concluded, we want to slash validators who were on the +//! wrong side of the dispute. The slashing amount depends on whether the +//! candidate was valid (none at the moment) or invalid (big). In addition to +//! that, we might want to kick out the validators from the active set. +//! Currently, we limit slashing to the backing group for invalid disputes. //! //! The `offences` pallet from Substrate provides us with a way to do both. -//! Currently, the interface expects us to provide staking information -//! including nominator exposure in order to submit an offence. +//! Currently, the interface expects us to provide staking information including +//! nominator exposure in order to submit an offence. //! //! Normally, we'd able to fetch this information from the runtime as soon as //! the dispute is concluded. This is also what `im-online` pallet does. //! However, since a dispute can conclude several sessions after the candidate //! was backed (see `dispute_period` in `HostConfiguration`), we can't rely on -//! this information be available in the context of the current block. The -//! `babe` and `grandpa` equivocation handlers also have to deal -//! with this problem. +//! this information being available in the context of the current block. The +//! `babe` and `grandpa` equivocation handlers also have to deal with this +//! problem. //! //! Our implementation looks like a hybrid of `im-online` and `grandpa` //! equivocation handlers. Meaning, we submit an `offence` for the concluded -//! disputes about the current session candidate directly from the runtime. -//! If, however, the dispute is about a past session, we record unapplied -//! slashes on chain, without `FullIdentification` of the offenders. -//! Later on, a block producer can submit an unsigned transaction with -//! `KeyOwnershipProof` of an offender and submit it to the runtime -//! to produce an offence. +//! disputes about the current session candidate directly from the runtime. If, +//! however, the dispute is about a past session, we record unapplied slashes on +//! chain, without `FullIdentification` of the offenders. Later on, a block +//! producer can submit an unsigned transaction with `KeyOwnershipProof` of an +//! offender and submit it to the runtime to produce an offence. use crate::{disputes, initializer::ValidatorSetCount, session_info::IdentificationTuple}; use frame_support::{ @@ -64,7 +63,10 @@ use sp_runtime::{ use sp_session::{GetSessionNumber, GetValidatorCount}; use sp_staking::offence::{DisableStrategy, Kind, Offence, OffenceError, ReportOffence}; use sp_std::{ - collections::btree_map::{BTreeMap, Entry}, + collections::{ + btree_map::{BTreeMap, Entry}, + btree_set::BTreeSet, + }, prelude::*, }; @@ -73,7 +75,7 @@ const LOG_TARGET: &str = "runtime::parachains::slashing"; // These are constants, but we want to make them configurable // via `HostConfiguration` in the future. const SLASH_FOR_INVALID: Perbill = Perbill::from_percent(100); -const SLASH_AGAINST_VALID: Perbill = Perbill::from_perthousand(1); +const SLASH_AGAINST_VALID: Perbill = Perbill::zero(); const DEFENSIVE_PROOF: &'static str = "disputes module should bail on old session"; #[cfg(feature = "runtime-benchmarks")] @@ -228,18 +230,30 @@ where candidate_hash: CandidateHash, kind: SlashingOffenceKind, losers: impl IntoIterator, + backers: impl IntoIterator, ) { - let losers: Vec = losers.into_iter().collect(); + // sanity check for the current implementation + if kind == SlashingOffenceKind::AgainstValid { + debug_assert!(false, "should only slash ForInvalid disputes"); + return + } + let losers: BTreeSet<_> = losers.into_iter().collect(); if losers.is_empty() { - // Nothing to do return } + let backers: BTreeSet<_> = backers.into_iter().collect(); + let to_punish: Vec = losers.intersection(&backers).cloned().collect(); + if to_punish.is_empty() { + return + } + let session_info = crate::session_info::Pallet::::session_info(session_index); let session_info = match session_info.defensive_proof(DEFENSIVE_PROOF) { Some(info) => info, None => return, }; - let maybe = Self::maybe_identify_validators(session_index, losers.iter().cloned()); + + let maybe = Self::maybe_identify_validators(session_index, to_punish.iter().cloned()); if let Some(offenders) = maybe { let validator_set_count = session_info.discovery_keys.len() as ValidatorSetCount; let offence = SlashingOffence::new( @@ -255,12 +269,20 @@ where return } - let keys = losers + let keys = to_punish .into_iter() .filter_map(|i| session_info.validators.get(i).cloned().map(|id| (i, id))) .collect(); let unapplied = PendingSlashes { keys, kind }; - >::insert(session_index, candidate_hash, unapplied); + + let append = |old: &mut Option| { + let old = old + .get_or_insert(PendingSlashes { keys: Default::default(), kind: unapplied.kind }); + debug_assert_eq!(old.kind, unapplied.kind); + + old.keys.extend(unapplied.keys) + }; + >::mutate(session_index, candidate_hash, append); } } @@ -272,18 +294,20 @@ where session_index: SessionIndex, candidate_hash: CandidateHash, losers: impl IntoIterator, + backers: impl IntoIterator, ) { let kind = SlashingOffenceKind::ForInvalid; - Self::do_punish(session_index, candidate_hash, kind, losers); + Self::do_punish(session_index, candidate_hash, kind, losers, backers); } fn punish_against_valid( - session_index: SessionIndex, - candidate_hash: CandidateHash, - losers: impl IntoIterator, + _session_index: SessionIndex, + _candidate_hash: CandidateHash, + _losers: impl IntoIterator, + _backers: impl IntoIterator, ) { - let kind = SlashingOffenceKind::AgainstValid; - Self::do_punish(session_index, candidate_hash, kind, losers); + // do nothing for now + // NOTE: changing that requires modifying `do_punish` implementation } fn initializer_initialize(now: T::BlockNumber) -> Weight { diff --git a/runtime/parachains/src/disputes/slashing/benchmarking.rs b/runtime/parachains/src/disputes/slashing/benchmarking.rs index e053d48ca3f6..0b328e3e5797 100644 --- a/runtime/parachains/src/disputes/slashing/benchmarking.rs +++ b/runtime/parachains/src/disputes/slashing/benchmarking.rs @@ -109,8 +109,9 @@ where let validator_index = ValidatorIndex(0); let losers = [validator_index].into_iter(); + let backers = losers.clone(); - T::SlashingHandler::punish_against_valid(session_index, CANDIDATE_HASH, losers); + T::SlashingHandler::punish_for_invalid(session_index, CANDIDATE_HASH, losers, backers); let unapplied = >::get(session_index, CANDIDATE_HASH); assert_eq!(unapplied.unwrap().keys.len(), 1); @@ -123,7 +124,7 @@ fn dispute_proof( validator_id: ValidatorId, validator_index: ValidatorIndex, ) -> DisputeProof { - let kind = SlashingOffenceKind::AgainstValid; + let kind = SlashingOffenceKind::ForInvalid; let time_slot = DisputesTimeSlot::new(session_index, CANDIDATE_HASH); DisputeProof { time_slot, kind, validator_index, validator_id } @@ -134,7 +135,7 @@ benchmarks! { where T: Config, } - // in this setup we have a single `AgainstValid` dispute + // in this setup we have a single `ForInvalid` dispute // submitted for a past session report_dispute_lost { let n in 4..<::BenchmarkingConfig as BenchmarkingConfiguration>::MAX_VALIDATORS; diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index a6abb3cab1b7..8e9e0097f703 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -20,7 +20,8 @@ use crate::{ disputes::DisputesHandler, mock::{ new_test_ext, AccountId, AllPalletsWithSystem, Initializer, MockGenesisConfig, System, - Test, PUNISH_VALIDATORS_AGAINST, PUNISH_VALIDATORS_FOR, REWARD_VALIDATORS, + Test, PUNISH_BACKERS_FOR, PUNISH_VALIDATORS_AGAINST, PUNISH_VALIDATORS_FOR, + REWARD_VALIDATORS, }, }; use frame_support::{ @@ -30,6 +31,10 @@ use frame_support::{ use primitives::BlockNumber; use sp_core::{crypto::CryptoType, Pair}; +const VOTE_FOR: VoteKind = VoteKind::ExplicitValid; +const VOTE_AGAINST: VoteKind = VoteKind::Invalid; +const VOTE_BACKING: VoteKind = VoteKind::Backing; + fn filter_dispute_set(stmts: MultiDisputeStatementSet) -> CheckedMultiDisputeStatementSet { let config = >::config(); let post_conclusion_acceptance_period = config.dispute_post_conclusion_acceptance_period; @@ -138,22 +143,26 @@ fn test_import_new_participant() { start: 0, concluded_at: None, }, + BTreeSet::new(), 0, ); assert_err!( - importer.import(ValidatorIndex(9), true), + importer.import(ValidatorIndex(9), VOTE_FOR), VoteImportError::ValidatorIndexOutOfBounds, ); - assert_err!(importer.import(ValidatorIndex(0), true), VoteImportError::DuplicateStatement); - assert_ok!(importer.import(ValidatorIndex(0), false)); + assert_err!(importer.import(ValidatorIndex(0), VOTE_FOR), VoteImportError::DuplicateStatement); + assert_ok!(importer.import(ValidatorIndex(0), VOTE_AGAINST)); - assert_ok!(importer.import(ValidatorIndex(2), true)); - assert_err!(importer.import(ValidatorIndex(2), true), VoteImportError::DuplicateStatement); + assert_ok!(importer.import(ValidatorIndex(2), VOTE_FOR)); + assert_err!(importer.import(ValidatorIndex(2), VOTE_FOR), VoteImportError::DuplicateStatement); - assert_ok!(importer.import(ValidatorIndex(2), false)); - assert_err!(importer.import(ValidatorIndex(2), false), VoteImportError::DuplicateStatement); + assert_ok!(importer.import(ValidatorIndex(2), VOTE_AGAINST)); + assert_err!( + importer.import(ValidatorIndex(2), VOTE_AGAINST), + VoteImportError::DuplicateStatement + ); let summary = importer.finish(); assert_eq!(summary.new_flags, DisputeStateFlags::default()); @@ -180,10 +189,11 @@ fn test_import_prev_participant_confirmed() { start: 0, concluded_at: None, }, + BTreeSet::new(), 0, ); - assert_ok!(importer.import(ValidatorIndex(2), true)); + assert_ok!(importer.import(ValidatorIndex(2), VOTE_FOR)); let summary = importer.finish(); assert_eq!( @@ -211,15 +221,16 @@ fn test_import_prev_participant_confirmed_slash_for() { start: 0, concluded_at: None, }, + BTreeSet::new(), 0, ); - assert_ok!(importer.import(ValidatorIndex(2), true)); - assert_ok!(importer.import(ValidatorIndex(2), false)); - assert_ok!(importer.import(ValidatorIndex(3), false)); - assert_ok!(importer.import(ValidatorIndex(4), false)); - assert_ok!(importer.import(ValidatorIndex(5), false)); - assert_ok!(importer.import(ValidatorIndex(6), false)); + assert_ok!(importer.import(ValidatorIndex(2), VOTE_FOR)); + assert_ok!(importer.import(ValidatorIndex(2), VOTE_AGAINST)); + assert_ok!(importer.import(ValidatorIndex(3), VOTE_AGAINST)); + assert_ok!(importer.import(ValidatorIndex(4), VOTE_AGAINST)); + assert_ok!(importer.import(ValidatorIndex(5), VOTE_AGAINST)); + assert_ok!(importer.import(ValidatorIndex(6), VOTE_AGAINST)); let summary = importer.finish(); assert_eq!( @@ -250,14 +261,15 @@ fn test_import_slash_against() { start: 0, concluded_at: None, }, + BTreeSet::new(), 0, ); - assert_ok!(importer.import(ValidatorIndex(3), true)); - assert_ok!(importer.import(ValidatorIndex(4), true)); - assert_ok!(importer.import(ValidatorIndex(5), false)); - assert_ok!(importer.import(ValidatorIndex(6), true)); - assert_ok!(importer.import(ValidatorIndex(7), true)); + assert_ok!(importer.import(ValidatorIndex(3), VOTE_FOR)); + assert_ok!(importer.import(ValidatorIndex(4), VOTE_FOR)); + assert_ok!(importer.import(ValidatorIndex(5), VOTE_AGAINST)); + assert_ok!(importer.import(ValidatorIndex(6), VOTE_FOR)); + assert_ok!(importer.import(ValidatorIndex(7), VOTE_FOR)); let summary = importer.finish(); assert_eq!( @@ -275,6 +287,48 @@ fn test_import_slash_against() { assert_eq!(summary.new_flags, DisputeStateFlags::FOR_SUPERMAJORITY); } +#[test] +fn test_import_backing_votes() { + let mut importer = DisputeStateImporter::new( + DisputeState { + validators_for: bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], + validators_against: bitvec![u8, BitOrderLsb0; 0, 1, 0, 0, 0, 0, 0, 0], + start: 0, + concluded_at: None, + }, + BTreeSet::from_iter([ValidatorIndex(0)]), + 0, + ); + + assert_ok!(importer.import(ValidatorIndex(3), VOTE_FOR)); + assert_ok!(importer.import(ValidatorIndex(3), VOTE_BACKING)); + assert_ok!(importer.import(ValidatorIndex(3), VOTE_AGAINST)); + assert_ok!(importer.import(ValidatorIndex(6), VOTE_FOR)); + assert_ok!(importer.import(ValidatorIndex(7), VOTE_BACKING)); + // Don't import backing vote twice + assert_err!( + importer.import(ValidatorIndex(0), VOTE_BACKING), + VoteImportError::DuplicateStatement, + ); + // Don't import explicit votes after backing + assert_err!(importer.import(ValidatorIndex(7), VOTE_FOR), VoteImportError::MaliciousBacker,); + + let summary = importer.finish(); + assert_eq!( + summary.state, + DisputeState { + validators_for: bitvec![u8, BitOrderLsb0; 1, 0, 1, 1, 0, 0, 1, 1], + validators_against: bitvec![u8, BitOrderLsb0; 0, 1, 0, 1, 0, 0, 0, 0], + start: 0, + concluded_at: None, + }, + ); + assert_eq!( + summary.backers, + BTreeSet::from_iter([ValidatorIndex(0), ValidatorIndex(3), ValidatorIndex(7),]), + ); +} + // Test that dispute timeout is handled correctly. #[test] fn test_dispute_timeout() { @@ -329,6 +383,7 @@ fn test_dispute_timeout() { }); let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + let inclusion_parent = sp_core::H256::repeat_byte(0xff); // v0 and v1 vote for 3, v2 against. We need f+1 votes (3) so that the dispute is // confirmed. Otherwise It will be filtered out. @@ -338,16 +393,13 @@ fn test_dispute_timeout() { session, statements: vec![ ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), ValidatorIndex(0), - v0.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: start - 1, - } - .signing_payload(), - ), + v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: start - 1, parent_hash: inclusion_parent }, + )), ), ( DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), @@ -495,21 +547,20 @@ fn test_provide_multi_dispute_is_providing() { }); let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + let inclusion_parent = sp_core::H256::repeat_byte(0xff); + let session = 1; let stmts = vec![DisputeStatementSet { candidate_hash: candidate_hash.clone(), - session: 1, + session, statements: vec![ ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), ValidatorIndex(0), - v0.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 1, - } - .signing_payload(), - ), + v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: session, parent_hash: inclusion_parent }, + )), ), ( DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), @@ -518,7 +569,7 @@ fn test_provide_multi_dispute_is_providing() { &ExplicitDisputeStatement { valid: false, candidate_hash: candidate_hash.clone(), - session: 1, + session, } .signing_payload(), ), @@ -538,6 +589,70 @@ fn test_provide_multi_dispute_is_providing() { }) } +#[test] +fn test_disputes_with_missing_backing_votes_are_rejected() { + new_test_ext(Default::default()).execute_with(|| { + let v0 = ::Pair::generate().0; + let v1 = ::Pair::generate().0; + + run_to_block(3, |b| { + // a new session at each block + if b == 1 { + Some(( + true, + b, + vec![(&0, v0.public()), (&1, v1.public())], + Some(vec![(&0, v0.public()), (&1, v1.public())]), + )) + } else { + Some((true, b, vec![(&1, v1.public())], Some(vec![(&1, v1.public())]))) + } + }); + + let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + let session = 1; + + let stmts = vec![DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + v0.sign( + &ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(1), + v1.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ], + }]; + + assert!(Pallet::::process_checked_multi_dispute_data( + stmts + .into_iter() + .map(CheckedDisputeStatementSet::unchecked_from_unchecked) + .collect() + ) + .is_err(),); + }) +} + #[test] fn test_freeze_on_note_included() { new_test_ext(Default::default()).execute_with(|| { @@ -555,6 +670,8 @@ fn test_freeze_on_note_included() { }); let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + let inclusion_parent = sp_core::H256::repeat_byte(0xff); + let session = 3; // v0 votes for 3 let stmts = vec![DisputeStatementSet { @@ -586,16 +703,13 @@ fn test_freeze_on_note_included() { ), ), ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), ValidatorIndex(1), - v1.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 3, - } - .signing_payload(), - ), + v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: session, parent_hash: inclusion_parent }, + )), ), ], }]; @@ -629,11 +743,13 @@ fn test_freeze_provided_against_supermajority_for_included() { }); let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + let inclusion_parent = sp_core::H256::repeat_byte(0xff); + let session = 3; // v0 votes for 3 let stmts = vec![DisputeStatementSet { candidate_hash: candidate_hash.clone(), - session: 3, + session, statements: vec![ ( DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), @@ -642,7 +758,7 @@ fn test_freeze_provided_against_supermajority_for_included() { &ExplicitDisputeStatement { valid: false, candidate_hash: candidate_hash.clone(), - session: 3, + session, } .signing_payload(), ), @@ -654,22 +770,19 @@ fn test_freeze_provided_against_supermajority_for_included() { &ExplicitDisputeStatement { valid: false, candidate_hash: candidate_hash.clone(), - session: 3, + session, } .signing_payload(), ), ), ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), ValidatorIndex(1), - v1.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 3, - } - .signing_payload(), - ), + v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: session, parent_hash: inclusion_parent }, + )), ), ], }]; @@ -851,23 +964,22 @@ fn test_provide_multi_dispute_success_and_other() { }); let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + let inclusion_parent = sp_core::H256::repeat_byte(0xff); + let session = 3; // v0 and v1 vote for 3, v6 votes against let stmts = vec![DisputeStatementSet { candidate_hash: candidate_hash.clone(), - session: 3, + session, statements: vec![ ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), ValidatorIndex(0), - v0.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 3, - } - .signing_payload(), - ), + v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: session, parent_hash: inclusion_parent }, + )), ), ( DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), @@ -876,7 +988,7 @@ fn test_provide_multi_dispute_success_and_other() { &ExplicitDisputeStatement { valid: false, candidate_hash: candidate_hash.clone(), - session: 3, + session, } .signing_payload(), ), @@ -926,16 +1038,13 @@ fn test_provide_multi_dispute_success_and_other() { session: 5, statements: vec![ ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), ValidatorIndex(5), - v3.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 5, - } - .signing_payload(), - ), + v3.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: 5, parent_hash: inclusion_parent }, + )), ), ( DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), @@ -1141,6 +1250,211 @@ fn test_provide_multi_dispute_success_and_other() { }) } +/// In this setup we have only one dispute concluding AGAINST. +/// There are some votes imported post dispute conclusion. +/// We make sure these votes are accounted for in punishment. +#[test] +fn test_punish_post_conclusion() { + new_test_ext(Default::default()).execute_with(|| { + // supermajority threshold is 5 + let v0 = ::Pair::generate().0; + let v1 = ::Pair::generate().0; + let v2 = ::Pair::generate().0; + let v3 = ::Pair::generate().0; + let v4 = ::Pair::generate().0; + let v5 = ::Pair::generate().0; + let v6 = ::Pair::generate().0; + // Mapping between key pair and `ValidatorIndex` + // v0 -> 0 + // v1 -> 3 + // v2 -> 6 + // v3 -> 5 + // v4 -> 1 + // v5 -> 4 + // v6 -> 2 + + run_to_block(6, |b| { + // a new session at each block + Some(( + true, + b, + vec![ + (&0, v0.public()), + (&1, v1.public()), + (&2, v2.public()), + (&3, v3.public()), + (&4, v4.public()), + (&5, v5.public()), + (&6, v6.public()), + ], + Some(vec![ + (&0, v0.public()), + (&1, v1.public()), + (&2, v2.public()), + (&3, v3.public()), + (&4, v4.public()), + (&5, v5.public()), + (&6, v6.public()), + ]), + )) + }); + + let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + let inclusion_parent = sp_core::H256::repeat_byte(0xff); + let session = 3; + + let stmts = vec![DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), + ValidatorIndex(0), + v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: session, parent_hash: inclusion_parent }, + )), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(1), + v4.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(2), + v6.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(6), + v2.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(4), + v5.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(5), + v3.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ( + DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking), + ValidatorIndex(3), + v1.sign(&ApprovalVote(candidate_hash).signing_payload(session)), + ), + ], + }]; + + let stmts = filter_dispute_set(stmts); + assert_ok!( + Pallet::::process_checked_multi_dispute_data(stmts), + vec![(session, candidate_hash)], + ); + + assert_eq!( + PUNISH_VALIDATORS_FOR.with(|r| r.borrow().clone()), + vec![(session, vec![ValidatorIndex(0), ValidatorIndex(3)]),], + ); + assert_eq!( + PUNISH_BACKERS_FOR.with(|r| r.borrow().clone()), + vec![(session, vec![ValidatorIndex(0)]),], + ); + + // someone reveals 3 backing vote, 6 votes against + let stmts = vec![DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( + inclusion_parent, + )), + ValidatorIndex(3), + v1.sign(&CompactStatement::Valid(candidate_hash).signing_payload( + &SigningContext { session_index: session, parent_hash: inclusion_parent }, + )), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(6), + v2.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session, + } + .signing_payload(), + ), + ), + ], + }]; + + let stmts = filter_dispute_set(stmts); + assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![],); + + // Ensure punishment for is called + assert_eq!( + PUNISH_VALIDATORS_FOR.with(|r| r.borrow().clone()), + vec![ + (session, vec![ValidatorIndex(0), ValidatorIndex(3)]), + (session, vec![ValidatorIndex(3)]), + ], + ); + + assert_eq!( + PUNISH_BACKERS_FOR.with(|r| r.borrow().clone()), + vec![ + (session, vec![ValidatorIndex(0)]), + (session, vec![ValidatorIndex(0), ValidatorIndex(3)]) + ], + ); + + assert_eq!( + PUNISH_VALIDATORS_AGAINST.with(|r| r.borrow().clone()), + vec![(session, vec![]), (session, vec![]),], + ); + }) +} + #[test] fn test_revert_and_freeze() { new_test_ext(Default::default()).execute_with(|| { diff --git a/runtime/parachains/src/mock.rs b/runtime/parachains/src/mock.rs index 08d0b05c3d99..f62ca4a0d1c1 100644 --- a/runtime/parachains/src/mock.rs +++ b/runtime/parachains/src/mock.rs @@ -253,6 +253,7 @@ thread_local! { pub static REWARD_VALIDATORS: RefCell)>> = RefCell::new(Vec::new()); pub static PUNISH_VALIDATORS_FOR: RefCell)>> = RefCell::new(Vec::new()); pub static PUNISH_VALIDATORS_AGAINST: RefCell)>> = RefCell::new(Vec::new()); + pub static PUNISH_BACKERS_FOR: RefCell)>> = RefCell::new(Vec::new()); } impl crate::disputes::RewardValidators for Test { @@ -269,14 +270,18 @@ impl crate::disputes::SlashingHandler for Test { session: SessionIndex, _: CandidateHash, losers: impl IntoIterator, + backers: impl IntoIterator, ) { - PUNISH_VALIDATORS_FOR.with(|r| r.borrow_mut().push((session, losers.into_iter().collect()))) + PUNISH_VALIDATORS_FOR + .with(|r| r.borrow_mut().push((session, losers.into_iter().collect()))); + PUNISH_BACKERS_FOR.with(|r| r.borrow_mut().push((session, backers.into_iter().collect()))); } fn punish_against_valid( session: SessionIndex, _: CandidateHash, losers: impl IntoIterator, + _backers: impl IntoIterator, ) { PUNISH_VALIDATORS_AGAINST .with(|r| r.borrow_mut().push((session, losers.into_iter().collect()))) diff --git a/zombienet_tests/functional/0002-parachains-disputes.zndsl b/zombienet_tests/functional/0002-parachains-disputes.zndsl index 9386e07e209a..357a380e1949 100644 --- a/zombienet_tests/functional/0002-parachains-disputes.zndsl +++ b/zombienet_tests/functional/0002-parachains-disputes.zndsl @@ -48,8 +48,9 @@ eve: reports parachain_candidate_disputes_total is at least 10 within 15 seconds eve: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 10 within 15 seconds eve: reports parachain_candidate_dispute_concluded{validity="invalid"} is 0 within 15 seconds -# Check there is an offence report -alice: system event contains "There is an offence reported" within 60 seconds +# As of , we don't slash on disputes +# with `valid` outcome, so there is no offence reported. +# alice: system event contains "There is an offence reported" within 60 seconds # Check lag - approval alice: reports polkadot_parachain_approval_checking_finality_lag is 0 diff --git a/zombienet_tests/functional/0003-parachains-garbage-candidate.zndsl b/zombienet_tests/functional/0003-parachains-garbage-candidate.zndsl index 24be749f13e6..71e93fcf359e 100644 --- a/zombienet_tests/functional/0003-parachains-garbage-candidate.zndsl +++ b/zombienet_tests/functional/0003-parachains-garbage-candidate.zndsl @@ -18,6 +18,11 @@ honest-validator-0: parachain 2000 block height is at least 2 within 180 seconds honest-validator-1: parachain 2001 block height is at least 2 within 180 seconds honest-validator-2: parachain 2002 block height is at least 2 within 180 seconds +# Check there is an offence report after dispute conclusion +honest-validator-0: system event contains "There is an offence reported" within 180 seconds +honest-validator-1: system event contains "There is an offence reported" within 180 seconds +honest-validator-2: system event contains "There is an offence reported" within 180 seconds + # Check for chain reversion after dispute conclusion. honest-validator-0: log line contains "reverted due to a bad parachain block" within 180 seconds honest-validator-1: log line contains "reverted due to a bad parachain block" within 180 seconds