From 675f71a8821d38a25f22eedd47c6ad09040664dd Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 21 Jul 2021 14:48:07 -0500 Subject: [PATCH] Dispute vote filtering for block authors (#3498) * guide: filter_multi_dispute_data * guide: elaborate * Implementation of dispute data filtering * tests for filtering * don't use std, you fool! * use swap_remove * Update runtime/parachains/src/disputes.rs Co-authored-by: Andronik Ordian * use btreeste * address API nit Co-authored-by: Andronik Ordian --- polkadot/primitives/src/v1/mod.rs | 16 + .../src/runtime/disputes.md | 7 + polkadot/runtime/parachains/src/disputes.rs | 646 +++++++++++++++++- 3 files changed, 660 insertions(+), 9 deletions(-) diff --git a/polkadot/primitives/src/v1/mod.rs b/polkadot/primitives/src/v1/mod.rs index ed082707a8..b12d63e529 100644 --- a/polkadot/primitives/src/v1/mod.rs +++ b/polkadot/primitives/src/v1/mod.rs @@ -1124,6 +1124,22 @@ impl DisputeStatement { Err(()) } } + + /// Whether the statement indicates validity. + pub fn indicates_validity(&self) -> bool { + match *self { + DisputeStatement::Valid(_) => true, + DisputeStatement::Invalid(_) => false, + } + } + + /// Whether the statement indicates invalidity. + pub fn indicates_invalidity(&self) -> bool { + match *self { + DisputeStatement::Valid(_) => false, + DisputeStatement::Invalid(_) => true, + } + } } /// Different kinds of statements of validity on a candidate. diff --git a/polkadot/roadmap/implementers-guide/src/runtime/disputes.md b/polkadot/roadmap/implementers-guide/src/runtime/disputes.md index 79015e4a74..4fb70eb93d 100644 --- a/polkadot/roadmap/implementers-guide/src/runtime/disputes.md +++ b/polkadot/roadmap/implementers-guide/src/runtime/disputes.md @@ -66,6 +66,13 @@ Frozen: Option, ## Routines +* `filter_multi_dispute_data(MultiDisputeStatementSet) -> MultiDisputeStatementSet`: + 1. Takes a `MultiDisputeStatementSet` and filters it down to a `MultiDisputeStatementSet` + that satisfies all the criteria of `provide_multi_dispute_data`. That is, eliminating + ancient votes, votes which overwhelm the maximum amount of spam slots, and duplicates. + This can be used by block authors to create the final submission in a block which is + guaranteed to pass the `provide_multi_dispute_data` checks. + * `provide_multi_dispute_data(MultiDisputeStatementSet) -> Vec<(SessionIndex, Hash)>`: 1. Pass on each dispute statement set to `provide_dispute_data`, propagating failure. 1. Return a list of all candidates who just had disputes initiated. diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index f7327ad2bd..fbc6e58c9c 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -17,6 +17,7 @@ //! Runtime component for handling disputes of parachain candidates. use sp_std::prelude::*; +use sp_std::collections::btree_set::BTreeSet; use primitives::v1::{ byzantine_threshold, supermajority_threshold, ApprovalVote, CandidateHash, CompactStatement, ConsensusLog, DisputeState, DisputeStatement, DisputeStatementSet, ExplicitDisputeStatement, @@ -176,11 +177,7 @@ impl DisputesHandler for pallet::Pallet { } fn filter_multi_dispute_data(statement_sets: &mut MultiDisputeStatementSet) { - // TODO: filter duplicate and ancient dispute statements. For now, don't import anything - // because there will be redundancies. - // - // https://github.com/paritytech/polkadot/issues/3472 - statement_sets.clear(); + pallet::Pallet::::filter_multi_dispute_data(statement_sets) } fn provide_multi_dispute_data( @@ -516,6 +513,48 @@ impl DisputeStateImporter { } } +// A filter on a dispute statement set. +#[derive(PartialEq)] +#[cfg_attr(test, derive(Debug))] +enum StatementSetFilter { + // Remove the entire dispute statement set. + RemoveAll, + // Remove the votes with given index from the statement set. + RemoveIndices(Vec), +} + +impl StatementSetFilter { + fn filter_statement_set(self, mut statement_set: DisputeStatementSet) + -> Option + { + match self { + StatementSetFilter::RemoveAll => None, + StatementSetFilter::RemoveIndices(mut indices) => { + indices.sort(); + indices.dedup(); + + // reverse order ensures correctness + for index in indices.into_iter().rev() { + // swap_remove guarantees linear complexity. + statement_set.statements.swap_remove(index); + } + + if statement_set.statements.is_empty() { + None + } else { + Some(statement_set) + } + } + } + } + + fn remove_index(&mut self, i: usize) { + if let StatementSetFilter::RemoveIndices(ref mut indices) = *self { + indices.push(i) + } + } +} + impl Pallet { /// Called by the initializer to initialize the disputes module. pub(crate) fn initializer_initialize(now: T::BlockNumber) -> Weight { @@ -644,6 +683,173 @@ impl Pallet { Ok(fresh) } + fn filter_multi_dispute_data(statement_sets: &mut MultiDisputeStatementSet) { + let config = >::config(); + + let old_statement_sets = sp_std::mem::take(statement_sets); + + // Deduplicate. + let dedup_iter = { + let mut targets = BTreeSet::new(); + old_statement_sets.into_iter().filter(move |set| { + let target = (set.candidate_hash, set.session); + targets.insert(target) + }) + }; + + *statement_sets = dedup_iter.filter_map(|set| { + let filter = Self::filter_dispute_data(&config, &set); + + filter.filter_statement_set(set) + }).collect(); + } + + // Given a statement set, this produces a filter to be applied to the statement set. + // It either removes the entire dispute statement set or some specific votes from it. + // + // Votes which are duplicate or already known by the chain are filtered out. + // The entire set is removed if the dispute is ancient or concluded. + fn filter_dispute_data(config: &HostConfiguration, set: &DisputeStatementSet) + -> StatementSetFilter + { + let mut filter = StatementSetFilter::RemoveIndices(Vec::new()); + + // Dispute statement sets on any dispute which concluded + // before this point are to be rejected. + let now = >::block_number(); + let oldest_accepted = now.saturating_sub(config.dispute_post_conclusion_acceptance_period); + + // Load session info to access validators + let session_info = match >::session_info(set.session) { + Some(s) => s, + None => return StatementSetFilter::RemoveAll, + }; + + let n_validators = session_info.validators.len(); + + // Check for ancient. + let 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 + } else { + DisputeState { + validators_for: bitvec![BitOrderLsb0, u8; 0; n_validators], + validators_against: bitvec![BitOrderLsb0, u8; 0; n_validators], + start: now, + concluded_at: None, + } + } + }; + + // Check and import all votes. + let summary = { + let mut importer = DisputeStateImporter::new(dispute_state, now); + for (i, (statement, validator_index, signature)) + in set.statements.iter().enumerate() + { + let validator_public = match session_info + .validators.get(validator_index.0 as usize) + { + None => { + filter.remove_index(i); + continue + } + Some(v) => v, + }; + + let valid = statement.indicates_validity(); + + if let Err(_) = importer.import(*validator_index, valid) { + filter.remove_index(i); + continue + } + + // Check signature after attempting import. + // + // Since we expect that this filter will be applied to + // disputes long after they're concluded, 99% of the time, + // the duplicate filter above will catch them before needing + // to do a heavy signature check. + // + // This is only really important until the post-conclusion acceptance threshold + // is reached, and then no part of this loop will be hit. + if let Err(()) = check_signature( + &validator_public, + set.candidate_hash, + set.session, + statement, + signature, + ) { + filter.remove_index(i); + continue + } + } + + importer.finish() + }; + + // Apply spam slot changes. Bail early if too many occupied. + let is_local = >::contains_key(&set.session, &set.candidate_hash); + if !is_local { + let mut spam_slots: Vec = SpamSlots::::get(&set.session) + .unwrap_or_else(|| vec![0; n_validators]); + + for (validator_index, spam_slot_change) in summary.spam_slot_changes { + let spam_slot = spam_slots.get_mut(validator_index.0 as usize) + .expect("index is in-bounds, as checked above; qed"); + + if let SpamSlotChange::Inc = spam_slot_change { + if *spam_slot >= config.dispute_max_spam_slots { + // 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) + .expect("spam slots are only incremented when a new statement \ + from a validator is included; qed"); + + // Note that there may be many votes by the validator in the statement + // set. There are not supposed to be, but the purpose of this function + // is to filter out invalid submissions, after all. + // + // This is fine - we only need to handle the first one, because all + // subsequent votes' indices have been added to the filter already + // 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); + } + + // It's also worth noting that the `DisputeStateImporter` + // which produces these spam slot updates only produces + // one spam slot update per validator because it rejects + // duplicate votes. + // + // So we don't need to worry about spam slots being + // updated incorrectly after receiving duplicates. + *spam_slot += 1; + } else { + *spam_slot = spam_slot.saturating_sub(1); + } + } + + // We write the spam slots here because sequential calls to + // `filter_dispute_data` have a dependency on each other. + // + // For example, if a validator V occupies 1 spam slot and + // max is 2, then 2 sequential calls incrementing spam slot + // cannot be allowed. + // + // However, 3 sequential calls, where the first increments, + // the second decrements, and the third increments would be allowed. + SpamSlots::::insert(&set.session, spam_slots); + } + + filter + } + /// Handle a set of dispute statements corresponding to a single candidate. /// /// Fails if the dispute data is invalid. Returns a boolean indicating whether the @@ -702,10 +908,7 @@ impl Pallet { signature, ).map_err(|()| Error::::InvalidSignature)?; - let valid = match statement { - DisputeStatement::Valid(_) => true, - DisputeStatement::Invalid(_) => false, - }; + let valid = statement.indicates_validity(); importer.import(*validator_index, valid).map_err(Error::::from)?; } @@ -2078,4 +2281,429 @@ mod tests { assert!(check_signature(&validator_id.public(), candidate_hash, session, &statement_3, &signed_5).is_err()); assert!(check_signature(&validator_id.public(), candidate_hash, session, &statement_4, &signed_5).is_err()); } + + #[test] + fn filter_removes_duplicates_within_set() { + new_test_ext(Default::default()).execute_with(|| { + let v0 = ::Pair::generate().0; + + run_to_block( + 3, + |b| { + // a new session at each block + Some(( + true, + b, + vec![(&0, v0.public())], + Some(vec![(&0, v0.public())]), + )) + } + ); + + let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + + let payload = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash.clone(), + session: 1, + }.signing_payload(); + + let sig_a = v0.sign(&payload); + let sig_b = v0.sign(&payload); + let sig_c = v0.sign(&payload); + + let mut statements = vec![ + DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_a.clone(), + ), + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_b, + ), + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_c, + ), + ] + } + ]; + + Pallet::::filter_multi_dispute_data(&mut statements); + + assert_eq!( + statements, + vec![ + DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_a, + ), + ] + } + ] + ) + }) + } + + #[test] + fn filter_correctly_accounts_spam_slots() { + let dispute_max_spam_slots = 2; + + let mock_genesis_config = MockGenesisConfig { + configuration: crate::configuration::GenesisConfig { + config: HostConfiguration { + dispute_max_spam_slots, + .. Default::default() + }, + .. Default::default() + }, + .. Default::default() + }; + + new_test_ext(mock_genesis_config).execute_with(|| { + let v0 = ::Pair::generate().0; + let v1 = ::Pair::generate().0; + let v2 = ::Pair::generate().0; + let v3 = ::Pair::generate().0; + + run_to_block( + 3, + |b| { + // a new session at each block + Some(( + true, + b, + vec![(&0, v0.public()), (&1, v1.public()), (&2, v2.public()), (&3, v3.public())], + Some(vec![(&0, v0.public()), (&1, v1.public()), (&2, v2.public()), (&3, v3.public())]), + )) + } + ); + + let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(1)); + let candidate_hash_b = CandidateHash(sp_core::H256::repeat_byte(2)); + let candidate_hash_c = CandidateHash(sp_core::H256::repeat_byte(3)); + + let payload_a = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash_a.clone(), + session: 1, + }.signing_payload(); + + let payload_b = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash_b.clone(), + session: 1, + }.signing_payload(); + + let payload_c = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash_c.clone(), + session: 1, + }.signing_payload(); + + let sig_0a = v0.sign(&payload_a); + let sig_0b = v0.sign(&payload_b); + let sig_0c = v0.sign(&payload_c); + + let sig_1b = v1.sign(&payload_b); + + let mut statements = vec![ + DisputeStatementSet { + candidate_hash: candidate_hash_a.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_0a.clone(), + ), + ] + }, + DisputeStatementSet { + candidate_hash: candidate_hash_b.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_0b.clone(), + ), + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(3), + sig_1b.clone(), + ), + ] + }, + DisputeStatementSet { + candidate_hash: candidate_hash_c.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_0c.clone(), + ), + ] + }, + ]; + + let old_statements = statements.clone(); + Pallet::::filter_multi_dispute_data(&mut statements); + + assert_eq!(statements, old_statements); + }) + } + + #[test] + fn filter_removes_session_out_of_bounds() { + new_test_ext(Default::default()).execute_with(|| { + let v0 = ::Pair::generate().0; + + run_to_block( + 3, + |b| { + // a new session at each block + Some(( + true, + b, + vec![(&0, v0.public())], + Some(vec![(&0, v0.public())]), + )) + } + ); + + let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + + let payload = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash.clone(), + session: 1, + }.signing_payload(); + + let sig_a = v0.sign(&payload); + + let mut statements = vec![ + DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session: 100, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_a, + ), + ] + } + ]; + + Pallet::::filter_multi_dispute_data(&mut statements); + + assert!(statements.is_empty()); + }) + } + + #[test] + fn filter_removes_concluded_ancient() { + let dispute_post_conclusion_acceptance_period = 2; + + let mock_genesis_config = MockGenesisConfig { + configuration: crate::configuration::GenesisConfig { + config: HostConfiguration { + dispute_post_conclusion_acceptance_period, + .. Default::default() + }, + .. Default::default() + }, + .. Default::default() + }; + + new_test_ext(mock_genesis_config).execute_with(|| { + let v0 = ::Pair::generate().0; + + run_to_block( + 3, + |b| { + // a new session at each block + Some(( + true, + b, + vec![(&0, v0.public())], + Some(vec![(&0, v0.public())]), + )) + } + ); + + let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(1)); + let candidate_hash_b = CandidateHash(sp_core::H256::repeat_byte(2)); + + >::insert( + &1, + &candidate_hash_a, + DisputeState { + validators_for: bitvec![BitOrderLsb0, u8; 0; 4], + validators_against: bitvec![BitOrderLsb0, u8; 0; 4], + start: 0, + concluded_at: Some(0), + }, + ); + + >::insert( + &1, + &candidate_hash_b, + DisputeState { + validators_for: bitvec![BitOrderLsb0, u8; 0; 4], + validators_against: bitvec![BitOrderLsb0, u8; 0; 4], + start: 0, + concluded_at: Some(1), + }, + ); + + let payload_a = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash_a.clone(), + session: 1, + }.signing_payload(); + + let payload_b = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash_b.clone(), + session: 1, + }.signing_payload(); + + let sig_a = v0.sign(&payload_a); + let sig_b = v0.sign(&payload_b); + + let mut statements = vec![ + DisputeStatementSet { + candidate_hash: candidate_hash_a.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_a, + ), + ] + }, + DisputeStatementSet { + candidate_hash: candidate_hash_b.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_b.clone(), + ), + ] + }, + ]; + + Pallet::::filter_multi_dispute_data(&mut statements); + + assert_eq!( + statements, + vec![ + DisputeStatementSet { + candidate_hash: candidate_hash_b.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_b, + ), + ] + } + ] + ); + }) + } + + #[test] + fn filter_removes_duplicate_statements_sets() { + new_test_ext(Default::default()).execute_with(|| { + let v0 = ::Pair::generate().0; + + run_to_block( + 3, + |b| { + // a new session at each block + Some(( + true, + b, + vec![(&0, v0.public())], + Some(vec![(&0, v0.public())]), + )) + } + ); + + let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(1)); + + let payload = ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash_a.clone(), + session: 1, + }.signing_payload(); + + let sig_a = v0.sign(&payload); + let sig_b = v0.sign(&payload); + + let mut statements = vec![ + DisputeStatementSet { + candidate_hash: candidate_hash_a.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_a.clone(), + ), + ] + }, + DisputeStatementSet { + candidate_hash: candidate_hash_a.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_b.clone(), + ), + ] + }, + ]; + + Pallet::::filter_multi_dispute_data(&mut statements); + + assert_eq!( + statements, + vec![ + DisputeStatementSet { + candidate_hash: candidate_hash_a.clone(), + session: 1, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + sig_a, + ), + ] + } + ] + ); + }) + } }