diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index cc2b4d49b8..65ff35111f 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -516,14 +516,19 @@ pub mod pallet { bitflags::bitflags! { #[derive(Default)] struct DisputeStateFlags: u8 { + /// The byzantine threshold of `f + 1` votes (and hence participating validators) was reached. const CONFIRMED = 0b0001; + /// Is the supermajority for validity of the dispute reached. const FOR_SUPERMAJORITY = 0b0010; + /// Is the supermajority against the validity of the block reached. const AGAINST_SUPERMAJORITY = 0b0100; } } impl DisputeStateFlags { fn from_state(state: &DisputeState) -> Self { + // Only correct since `DisputeState` is _always_ initialized + // with the validator set based on the session. let n = state.validators_for.len(); let byzantine_threshold = byzantine_threshold(n); @@ -549,18 +554,20 @@ impl DisputeStateFlags { #[derive(PartialEq, RuntimeDebug)] enum SpamSlotChange { + /// Add a `+1` to the spam slot for a particular validator index in this session. Inc, + /// Subtract `-1` ... Dec, } struct ImportSummary { - // The new state, with all votes imported. + /// The new state, with all votes imported. state: DisputeState, - // Changes to spam slots. Validator index paired with directional change. + /// Changes to spam slots. Validator index paired with directional change. spam_slot_changes: Vec<(ValidatorIndex, SpamSlotChange)>, - // Validators to slash for being (wrongly) on the AGAINST side. + /// Validators to slash for being (wrongly) on the AGAINST side. slash_against: Vec, - // Validators to slash for being (wrongly) on the FOR side. + /// Validators to slash for being (wrongly) on the FOR side. slash_for: Vec, // New participants in the dispute. new_participants: bitvec::vec::BitVec, @@ -570,7 +577,9 @@ struct ImportSummary { #[derive(RuntimeDebug, PartialEq, Eq)] enum VoteImportError { + /// Validator index was outside the range of valid validator indices in the given session. ValidatorIndexOutOfBounds, + /// Found a duplicate statement in the dispute statement set. DuplicateStatement, } @@ -583,10 +592,15 @@ impl From for Error { } } +/// A transport statement bit change for a single validator. #[derive(RuntimeDebug, PartialEq, Eq)] 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, + /// Has the validator participated before, i.e. in backing or + /// with an oposing vote. new_participant: bool, } @@ -632,9 +646,9 @@ impl DisputeStateImporter { bits.set(validator.0 as usize, true); - // New participants tracks those which didn't appear on either - // side of the dispute until now. So we check the other side - // and checked the first side before. + // 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). + // To verify this we need to assure they also were not part of the opposing side before. if other_bits.get(validator.0 as usize).map_or(false, |b| !*b) { undo.new_participant = true; self.new_participants.set(validator.0 as usize, true); @@ -643,6 +657,7 @@ impl DisputeStateImporter { Ok(undo) } + /// Revert a done transaction. fn undo(&mut self, undo: ImportUndo) { if undo.valid { self.state.validators_for.set(undo.validator_index.0 as usize, false); @@ -655,6 +670,7 @@ impl DisputeStateImporter { } } + /// Collect all dispute votes. fn finish(mut self) -> ImportSummary { let pre_flags = self.pre_flags; let post_flags = DisputeStateFlags::from_state(&self.state); @@ -681,8 +697,12 @@ impl DisputeStateImporter { .map(|i| (ValidatorIndex(i as _), SpamSlotChange::Dec)) .collect() }, - (true, true) | (true, false) => { - // nothing to do. (true, false) is also impossible. + (true, false) => { + log::error!("Dispute statements are never removed. This is a bug"); + Vec::new() + }, + (true, true) => { + // No change, nothing to do. Vec::new() }, }; @@ -925,27 +945,32 @@ impl Pallet { let n_validators = session_info.validators.len(); // Check for ancient. - let dispute_state = { + let (first_votes, dispute_state) = { if let Some(dispute_state) = >::get(&set.session, &set.candidate_hash) { if dispute_state.concluded_at.as_ref().map_or(false, |c| c < &oldest_accepted) { return StatementSetFilter::RemoveAll } - dispute_state + (false, dispute_state) } else { - DisputeState { - validators_for: bitvec![u8, BitOrderLsb0; 0; n_validators], - validators_against: bitvec![u8, BitOrderLsb0; 0; n_validators], - start: now, - concluded_at: None, - } + // No state in storage, this indicates it's the first dispute statement set as well. + ( + true, + DisputeState { + validators_for: bitvec![u8, BitOrderLsb0; 0; n_validators], + validators_against: bitvec![u8, BitOrderLsb0; 0; n_validators], + start: now, + concluded_at: None, + }, + ) } }; // Check and import all votes. - let summary = { + let mut summary = { let mut importer = DisputeStateImporter::new(dispute_state, now); for (i, (statement, validator_index, signature)) in set.statements.iter().enumerate() { + // assure the validator index and is present in the session info let validator_public = match session_info.validators.get(validator_index.0 as usize) { None => { @@ -1005,7 +1030,7 @@ impl Pallet { if !is_local { let mut spam_slots: Vec = SpamSlots::::get(&set.session).unwrap_or_else(|| vec![0; n_validators]); - + let mut spam_filter_struck = false; for (validator_index, spam_slot_change) in summary.spam_slot_changes { let spam_slot = spam_slots .get_mut(validator_index.0 as usize) @@ -1013,11 +1038,13 @@ impl Pallet { if let SpamSlotChange::Inc = spam_slot_change { if *spam_slot >= max_spam_slots { + spam_filter_struck = true; + // Find the vote by this validator and filter it out. let first_index_in_set = set .statements .iter() - .position(|(_, v_i, _)| &validator_index == v_i) + .position(|(_statement, v_i, _signature)| &validator_index == v_i) .expect( "spam slots are only incremented when a new statement \ from a validator is included; qed", @@ -1032,6 +1059,10 @@ impl Pallet { // by the duplicate checks above. It's only the first one which // may not already have been filtered out. filter.remove_index(first_index_in_set); + + // Removing individual statments can cause the dispute to become onesided. + // Checking that (again) is done after the loop. Remove the bit indices. + summary.new_participants.set(validator_index.0 as _, false); } // It's also worth noting that the `DisputeStateImporter` @@ -1057,6 +1088,36 @@ impl Pallet { // However, 3 sequential calls, where the first increments, // the second decrements, and the third increments would be allowed. SpamSlots::::insert(&set.session, spam_slots); + + // This is only relevant in cases where it's the first vote and the state + // would hence hold a onesided dispute. If a onesided dispute can never be + // started, by induction, we can never enter a state of a one sided dispute. + if spam_filter_struck && first_votes { + let mut vote_for_count = 0_u64; + let mut vote_against_count = 0_u64; + // Since this is the first set of statements for the dispute, + // it's sufficient to count the votes in the statement set after they + set.statements.iter().for_each(|(statement, v_i, _signature)| { + if Some(true) == + summary.new_participants.get(v_i.0 as usize).map(|b| b.as_ref().clone()) + { + match statement { + // `summary.new_flags` contains the spam free votes. + // Note that this does not distinguish between pro or con votes, + // since allowing both of them, even if the spam threshold would be reached + // is a good thing. + // Overflow of the counters is no concern, disputes are limited by weight. + DisputeStatement::Valid(_) => vote_for_count += 1, + DisputeStatement::Invalid(_) => vote_against_count += 1, + } + } + }); + if vote_for_count.is_zero() || vote_against_count.is_zero() { + // It wasn't one-sided before the spam filters, but now it is, + // so we need to be thorough and not import that dispute. + return StatementSetFilter::RemoveAll + } + } } filter diff --git a/polkadot/runtime/parachains/src/disputes/tests.rs b/polkadot/runtime/parachains/src/disputes/tests.rs index 28d22f24fc..2897ced22e 100644 --- a/polkadot/runtime/parachains/src/disputes/tests.rs +++ b/polkadot/runtime/parachains/src/disputes/tests.rs @@ -24,6 +24,7 @@ use crate::{ REWARD_VALIDATORS, }, }; +use assert_matches::assert_matches; use frame_support::{ assert_err, assert_noop, assert_ok, traits::{OnFinalize, OnInitialize}, @@ -288,6 +289,217 @@ fn test_import_slash_against() { assert_eq!(summary.new_flags, DisputeStateFlags::FOR_SUPERMAJORITY); } +fn generate_dispute_statement_set_entry( + session: u32, + candidate_hash: CandidateHash, + statement: DisputeStatement, + validator: &::Pair, +) -> (DisputeStatement, ValidatorSignature) { + let valid = match &statement { + DisputeStatement::Valid(_) => true, + _ => false, + }; + let signature_bytes = validator + .sign(&ExplicitDisputeStatement { valid, candidate_hash, session }.signing_payload()); + let signature = ValidatorSignature::try_from(signature_bytes).unwrap(); + (statement, signature) +} + +fn generate_dispute_statement_set( + session: SessionIndex, + candidate_hash: CandidateHash, + validators: &[::Pair], + vidxs: Vec<(usize, DisputeStatement)>, +) -> DisputeStatementSet { + let statements = vidxs + .into_iter() + .map(|(v_i, statement)| { + let validator_index = ValidatorIndex(v_i as u32); + let (statement, signature) = generate_dispute_statement_set_entry( + session, + candidate_hash.clone(), + statement, + &validators[v_i], + ); + (statement, validator_index, signature) + }) + .collect::>(); + DisputeStatementSet { candidate_hash: candidate_hash.clone(), session, statements } +} + +#[test] +fn dispute_statement_becoming_onesided_due_to_spamslots_is_accepted() { + let dispute_conclusion_by_time_out_period = 3; + let start = 10; + let session = start - 1; + let dispute_max_spam_slots = 2; + let post_conclusion_acceptance_period = 3; + + let mock_genesis_config = MockGenesisConfig { + configuration: crate::configuration::GenesisConfig { + config: HostConfiguration { + dispute_conclusion_by_time_out_period, + dispute_max_spam_slots, + ..Default::default() + }, + ..Default::default() + }, + ..Default::default() + }; + + new_test_ext(mock_genesis_config).execute_with(|| { + // We need 6 validators for the byzantine threshold to be 2 + static ACCOUNT_IDS: &[AccountId] = &[0, 1, 2, 3, 4, 5, 6, 7]; + let validators = std::iter::repeat(()) + .take(7) + .map(|_| ::Pair::generate().0) + .collect::>(); + let validators = &validators; + + // a new session at each block, but always the same validators + let session_change_callback = |block_number: u32| -> Option> { + let session_validators = + Vec::from_iter(ACCOUNT_IDS.iter().zip(validators.iter().map(|pair| pair.public()))); + Some((true, block_number, session_validators.clone(), Some(session_validators))) + }; + + run_to_block(start, session_change_callback); + + // Must be _foreign_ parachain candidate + // otherwise slots do not trigger. + let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(0xA)); + let candidate_hash_b = CandidateHash(sp_core::H256::repeat_byte(0xB)); + let candidate_hash_c = CandidateHash(sp_core::H256::repeat_byte(0xC)); + let candidate_hash_d = CandidateHash(sp_core::H256::repeat_byte(0xD)); + + let stmts = vec![ + // a + generate_dispute_statement_set( + session, + candidate_hash_a, + validators, + vec![ + (3, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)), + (6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)), + ], + ), + // b + generate_dispute_statement_set( + session, + candidate_hash_b, + validators, + vec![ + (1, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)), + (6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)), + ], + ), + // c + generate_dispute_statement_set( + session, + candidate_hash_c, + validators, + vec![ + (2, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)), + (6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)), + ], + ), + // d + generate_dispute_statement_set( + session, + candidate_hash_d, + validators, + vec![ + (4, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)), + (5, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)), + ], + ), + generate_dispute_statement_set( + session, + candidate_hash_d, + validators, + vec![(6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit))], + ), + ]; + + // no filtering happens, host config for `dispute_max_spam_slots: 2` is the default + // updates spam slots implicitly + let set = stmts[0].clone(); + let filter = Pallet::::filter_dispute_data( + &set, + post_conclusion_acceptance_period, + dispute_max_spam_slots, + VerifyDisputeSignatures::Skip, + ); + assert_matches!(&filter, StatementSetFilter::RemoveIndices(v) if v.is_empty()); + assert_matches!(filter.filter_statement_set(set.clone()), Some(modified) => { + assert_eq!(&set, modified.as_ref()); + }); + assert_eq!(SpamSlots::::get(session), Some(vec![0, 0, 0, 1, 0, 0, 1])); + + // <-----> + + // 2nd, still ok? Should be + let set = stmts[1].clone(); + let filter = Pallet::::filter_dispute_data( + &set, + post_conclusion_acceptance_period, + dispute_max_spam_slots, + VerifyDisputeSignatures::Skip, + ); + assert_matches!(&filter, StatementSetFilter::RemoveIndices(v) if v.is_empty()); + assert_matches!(filter.filter_statement_set(set.clone()), Some(modified) => { + assert_eq!(&set, modified.as_ref()); + }); + assert_eq!(SpamSlots::::get(session), Some(vec![0, 1, 0, 1, 0, 0, 2])); + + // <-----> + + // now this is the third spammy participation of validator 6 and hence + let set = stmts[2].clone(); + let filter = Pallet::::filter_dispute_data( + &set, + post_conclusion_acceptance_period, + dispute_max_spam_slots, + VerifyDisputeSignatures::Skip, + ); + assert_matches!(&filter, StatementSetFilter::RemoveAll); + // no need to apply the filter, + // we don't do anything with the result, and spam slots were updated already + + // <-----> + + // now there is no pariticipation in this dispute being initiated + // only validator 4 and 5 are part of it + // with 3 validators it's not a an unconfirmed dispute anymore + // so validator 6, while being considered spammy should work again + let set = stmts[3].clone(); + let filter = Pallet::::filter_dispute_data( + &set, + post_conclusion_acceptance_period, + dispute_max_spam_slots, + VerifyDisputeSignatures::Skip, + ); + assert_matches!(&filter, StatementSetFilter::RemoveIndices(v) if v.is_empty()); + // no need to apply the filter, + // we don't do anything with the result, and spam slots were updated already + + // <-----> + + // it's a spammy participant, so a new dispute will not be accepted being initiated by a spammer + let set = stmts[4].clone(); + let filter = Pallet::::filter_dispute_data( + &set, + post_conclusion_acceptance_period, + dispute_max_spam_slots, + VerifyDisputeSignatures::Skip, + ); + assert_matches!(&filter, StatementSetFilter::RemoveAll); + assert_matches!(filter.filter_statement_set(set.clone()), None); + + assert_eq!(SpamSlots::::get(session), Some(vec![0, 1, 1, 1, 1, 1, 3])); + }); +} + // Test that punish_inconclusive is correctly called. #[test] fn test_initializer_initialize() {