From 78134e48f51f14791b29c5b9bd44a971692f74ec Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 21 Nov 2022 14:39:21 +0200 Subject: [PATCH 01/44] disputes pallet: Filter disputes with votes less than supermajority threshold --- runtime/parachains/src/disputes.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index a01827f33d7c..f51875dc746b 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -521,6 +521,8 @@ pub mod pallet { PotentialSpam, /// A dispute where there are only votes on one side. SingleSidedDispute, + /// Unconfirmed dispute statement sets provided + UnconfirmedDispute, } #[pallet::call] @@ -938,6 +940,7 @@ impl Pallet { // // Votes which are duplicate or already known by the chain are filtered out. // The entire set is removed if the dispute is both, ancient and concluded. + // Disputes without enough votes to get confirmed are also filtered out. fn filter_dispute_data( set: &DisputeStatementSet, post_conclusion_acceptance_period: ::BlockNumber, @@ -1039,6 +1042,13 @@ impl Pallet { return StatementSetFilter::RemoveAll } + // Reject disputes containing less votes than needed for confirmation. + if (summary.state.validators_for.clone() | &summary.state.validators_against).count_ones() <= + byzantine_threshold(summary.state.validators_for.len()) + { + return StatementSetFilter::RemoveAll + } + // Apply spam slot changes. Bail early if too many occupied. let is_local = >::contains_key(&set.session, &set.candidate_hash); if !is_local { @@ -1201,6 +1211,13 @@ impl Pallet { Error::::SingleSidedDispute, ); + // Reject disputes containing less votes than needed for confirmation. + ensure!( + (summary.state.validators_for.clone() | &summary.state.validators_against).count_ones() > + byzantine_threshold(summary.state.validators_for.len()), + Error::::UnconfirmedDispute, + ); + let DisputeStatementSet { ref session, ref candidate_hash, .. } = set; let session = *session; let candidate_hash = *candidate_hash; From ef1823e56a2a5527ec89808332ab7e671549a7ef Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 24 Nov 2022 10:20:08 +0200 Subject: [PATCH 02/44] Remove `max_spam_slots` usages --- runtime/parachains/src/configuration.rs | 14 +------------- runtime/parachains/src/disputes.rs | 5 ----- runtime/parachains/src/paras_inherent/mod.rs | 4 ---- 3 files changed, 1 insertion(+), 22 deletions(-) diff --git a/runtime/parachains/src/configuration.rs b/runtime/parachains/src/configuration.rs index 2f09deb34ded..7e3bb65ba7fd 100644 --- a/runtime/parachains/src/configuration.rs +++ b/runtime/parachains/src/configuration.rs @@ -193,6 +193,7 @@ pub struct HostConfiguration { /// How long after dispute conclusion to accept statements. pub dispute_post_conclusion_acceptance_period: BlockNumber, /// The maximum number of dispute spam slots + /// TODO: Should be removed in next version pub dispute_max_spam_slots: u32, /// How long it takes for a dispute to conclude by time-out, if no supermajority is reached. pub dispute_conclusion_by_time_out_period: BlockNumber, @@ -752,19 +753,6 @@ pub mod pallet { }) } - /// Set the maximum number of dispute spam slots. - #[pallet::call_index(16)] - #[pallet::weight(( - T::WeightInfo::set_config_with_u32(), - DispatchClass::Operational, - ))] - pub fn set_dispute_max_spam_slots(origin: OriginFor, new: u32) -> DispatchResult { - ensure_root(origin)?; - Self::schedule_config_update(|config| { - config.dispute_max_spam_slots = new; - }) - } - /// Set the dispute conclusion by time out period. #[pallet::call_index(17)] #[pallet::weight(( diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index f51875dc746b..e372597bb7d3 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -262,7 +262,6 @@ pub trait DisputesHandler { /// accounting for maximum block weight. fn filter_dispute_data( statement_set: DisputeStatementSet, - max_spam_slots: u32, post_conclusion_acceptance_period: BlockNumber, verify_sigs: VerifyDisputeSignatures, ) -> Option; @@ -311,7 +310,6 @@ impl DisputesHandler for () { fn filter_dispute_data( _set: DisputeStatementSet, - _max_spam_slots: u32, _post_conclusion_acceptance_period: BlockNumber, _verify_sigs: VerifyDisputeSignatures, ) -> Option { @@ -361,14 +359,12 @@ where fn filter_dispute_data( set: DisputeStatementSet, - max_spam_slots: u32, post_conclusion_acceptance_period: T::BlockNumber, verify_sigs: VerifyDisputeSignatures, ) -> Option { pallet::Pallet::::filter_dispute_data( &set, post_conclusion_acceptance_period, - max_spam_slots, verify_sigs, ) .filter_statement_set(set) @@ -944,7 +940,6 @@ impl Pallet { fn filter_dispute_data( set: &DisputeStatementSet, post_conclusion_acceptance_period: ::BlockNumber, - max_spam_slots: u32, verify_sigs: VerifyDisputeSignatures, ) -> StatementSetFilter { let mut filter = StatementSetFilter::RemoveIndices(Vec::new()); diff --git a/runtime/parachains/src/paras_inherent/mod.rs b/runtime/parachains/src/paras_inherent/mod.rs index 15669ec9c15d..9ee51abd9932 100644 --- a/runtime/parachains/src/paras_inherent/mod.rs +++ b/runtime/parachains/src/paras_inherent/mod.rs @@ -349,7 +349,6 @@ impl Pallet { let (checked_disputes, total_consumed_weight) = { // Obtain config params.. let config = >::config(); - let max_spam_slots = config.dispute_max_spam_slots; let post_conclusion_acceptance_period = config.dispute_post_conclusion_acceptance_period; @@ -363,7 +362,6 @@ impl Pallet { let dispute_set_validity_check = move |set| { T::DisputesHandler::filter_dispute_data( set, - max_spam_slots, post_conclusion_acceptance_period, verify_dispute_sigs, ) @@ -595,7 +593,6 @@ impl Pallet { } let config = >::config(); - let max_spam_slots = config.dispute_max_spam_slots; let post_conclusion_acceptance_period = config.dispute_post_conclusion_acceptance_period; // TODO: Better if we can convert this to `with_transactional` and handle an error if @@ -609,7 +606,6 @@ impl Pallet { let dispute_statement_set_valid = move |set: DisputeStatementSet| { T::DisputesHandler::filter_dispute_data( set, - max_spam_slots, post_conclusion_acceptance_period, // `DisputeCoordinator` on the node side only forwards // valid dispute statement sets and hence this does not From ec7e53c2270214248e7498a53b5bbb5fe5638b51 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 23 Nov 2022 17:41:04 +0200 Subject: [PATCH 03/44] Remove `SpamSlots` --- runtime/parachains/src/disputes.rs | 105 +---------------------------- 1 file changed, 2 insertions(+), 103 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index e372597bb7d3..4cb2a43832fa 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -467,14 +467,6 @@ pub mod pallet { T::BlockNumber, >; - /// Maps session indices to a vector indicating the number of potentially-spam disputes - /// each validator is participating in. Potentially-spam disputes are remote disputes which have - /// fewer than `byzantine_threshold + 1` validators. - /// - /// The i'th entry of the vector corresponds to the i'th validator in the session. - #[pallet::storage] - pub(super) type SpamSlots = StorageMap<_, Twox64Concat, SessionIndex, Vec>; - /// Whether the chain is frozen. Starts as `None`. When this is `Some`, /// the chain will not accept any new parachain blocks for backing or inclusion, /// and its value indicates the last valid block number in the chain. @@ -836,24 +828,6 @@ impl Pallet { continue } - // mildly punish all validators involved. they've failed to make - // data available to others, so this is most likely spam. - SpamSlots::::mutate(session_index, |spam_slots| { - let spam_slots = match spam_slots { - Some(ref mut s) => s, - None => return, - }; - - // also reduce spam slots for all validators involved, if the dispute was unconfirmed. - // this does open us up to more spam, but only for validators who are willing - // to be punished more. - // - // it would be unexpected for any change here to occur when the dispute has not concluded - // in time, as a dispute guaranteed to have at least one honest participant should - // conclude quickly. - let _participating = decrement_spam(spam_slots, &dispute); - }); - weight += T::DbWeight::get().reads_writes(2, 2); } } @@ -892,7 +866,6 @@ impl Pallet { // TODO: https://github.com/paritytech/polkadot/issues/3469 #[allow(deprecated)] >::remove_prefix(to_prune, None); - SpamSlots::::remove(to_prune); } *last_pruned = Some(pruning_target); @@ -980,7 +953,7 @@ impl Pallet { }; // Check and import all votes. - let mut summary = { + let 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 @@ -1047,71 +1020,10 @@ impl Pallet { // 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]); - 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) - .expect("index is in-bounds, as checked above; qed"); - - 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(|(_statement, v_i, _signature)| &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); - - // 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` - // 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); - // 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 { + if 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, @@ -1131,11 +1043,6 @@ impl Pallet { } } }); - 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 - } } } @@ -1295,15 +1202,7 @@ impl Pallet { >::insert(&session, &candidate_hash, revert_to); - // If we just included a block locally which has a live dispute, decrement spam slots - // for any involved validators, if the dispute is not already confirmed by f + 1. if let Some(state) = >::get(&session, candidate_hash) { - SpamSlots::::mutate(&session, |spam_slots| { - if let Some(ref mut spam_slots) = *spam_slots { - decrement_spam(spam_slots, &state); - } - }); - if has_supermajority_against(&state) { Self::revert_and_freeze(revert_to); } From 2a0a7ee259173beb91d2b208429c6424c7ebd2aa Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 23 Nov 2022 17:45:12 +0200 Subject: [PATCH 04/44] Remove `SpamSlotChange` --- runtime/parachains/src/disputes.rs | 68 +----------------------------- 1 file changed, 2 insertions(+), 66 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 4cb2a43832fa..6273848ec817 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -564,19 +564,9 @@ 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. state: DisputeState, - /// 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. slash_against: Vec, /// Validators to slash for being (wrongly) on the FOR side. @@ -689,37 +679,7 @@ impl DisputeStateImporter { let pre_post_contains = |flags| (pre_flags.contains(flags), post_flags.contains(flags)); - // 1. Act on confirmed flag state to inform spam slots changes. - let spam_slot_changes: Vec<_> = match pre_post_contains(DisputeStateFlags::CONFIRMED) { - (false, false) => { - // increment spam slots for all new participants. - self.new_participants - .iter_ones() - .map(|i| (ValidatorIndex(i as _), SpamSlotChange::Inc)) - .collect() - }, - (false, true) => { - // all participants, which are not new participants - let prev_participants = (self.state.validators_for.clone() | - self.state.validators_against.clone()) & - !self.new_participants.clone(); - - prev_participants - .iter_ones() - .map(|i| (ValidatorIndex(i as _), SpamSlotChange::Dec)) - .collect() - }, - (true, false) => { - log::error!("Dispute statements are never removed. This is a bug"); - Vec::new() - }, - (true, true) => { - // No change, nothing to do. - Vec::new() - }, - }; - - // 2. Check for fresh FOR supermajority. Only if not already concluded. + // 1. Check for fresh FOR supermajority. Only if not already concluded. let slash_against = if let (false, true) = pre_post_contains(DisputeStateFlags::FOR_SUPERMAJORITY) { if self.state.concluded_at.is_none() { @@ -736,7 +696,7 @@ impl DisputeStateImporter { Vec::new() }; - // 3. Check for fresh AGAINST supermajority. + // 2. Check for fresh AGAINST supermajority. let slash_for = if let (false, true) = pre_post_contains(DisputeStateFlags::AGAINST_SUPERMAJORITY) { if self.state.concluded_at.is_none() { @@ -751,7 +711,6 @@ impl DisputeStateImporter { ImportSummary { state: self.state, - spam_slot_changes, slash_against, slash_for, new_participants: self.new_participants, @@ -1248,29 +1207,6 @@ fn has_supermajority_against(dispute: &DisputeState) - dispute.validators_against.count_ones() >= supermajority_threshold } -// If the dispute had not enough validators to confirm, decrement spam slots for all the participating -// validators. -// -// Returns the set of participating validators as a bitvec. -fn decrement_spam( - spam_slots: &mut [u32], - dispute: &DisputeState, -) -> bitvec::vec::BitVec { - let byzantine_threshold = byzantine_threshold(spam_slots.len()); - - let participating = dispute.validators_for.clone() | dispute.validators_against.clone(); - let decrement_spam = participating.count_ones() <= byzantine_threshold; - for validator_index in participating.iter_ones() { - if decrement_spam { - if let Some(occupied) = spam_slots.get_mut(validator_index as usize) { - *occupied = occupied.saturating_sub(1); - } - } - } - - participating -} - fn check_signature( validator_public: &ValidatorId, candidate_hash: CandidateHash, From 23fc0cfabb5b8771ceaf84b9e79d91a2abb0ace5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 23 Nov 2022 17:50:19 +0200 Subject: [PATCH 05/44] Remove `Error::PotentialSpam` and stale comments --- runtime/parachains/src/disputes.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 6273848ec817..b7a02511d915 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -505,8 +505,6 @@ pub mod pallet { InvalidSignature, /// Validator vote submitted more than once to dispute. DuplicateStatement, - /// Too many spam slots used by some specific validator. - PotentialSpam, /// A dispute where there are only votes on one side. SingleSidedDispute, /// Unconfirmed dispute statement sets provided @@ -781,8 +779,6 @@ impl Pallet { >::insert(session_index, candidate_hash, &dispute); if >::contains_key(&session_index, &candidate_hash) { - // Local disputes don't count towards spam. - weight += T::DbWeight::get().reads_writes(1, 1); continue } @@ -976,7 +972,6 @@ impl Pallet { return StatementSetFilter::RemoveAll } - // Apply spam slot changes. Bail early if too many occupied. let is_local = >::contains_key(&set.session, &set.candidate_hash); if !is_local { // This is only relevant in cases where it's the first vote and the state @@ -994,8 +989,7 @@ impl Pallet { 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. + // since allowing both of them. // Overflow of the counters is no concern, disputes are limited by weight. DisputeStatement::Valid(_) => vote_for_count += 1, DisputeStatement::Invalid(_) => vote_against_count += 1, @@ -1083,9 +1077,6 @@ impl Pallet { let session = *session; let candidate_hash = *candidate_hash; - // we can omit spam slot checks, `fn filter_disputes_data` is - // always called before calling this `fn`. - if fresh { let is_local = >::contains_key(&session, &candidate_hash); From ccfa0b186e9745386f5b96b52248d7a635875580 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 23 Nov 2022 17:51:10 +0200 Subject: [PATCH 06/44] `create_disputes_with_no_spam` -> `create_disputes` --- runtime/parachains/src/builder.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/builder.rs b/runtime/parachains/src/builder.rs index 9a1c16e6aa1d..84651a06003e 100644 --- a/runtime/parachains/src/builder.rs +++ b/runtime/parachains/src/builder.rs @@ -575,7 +575,7 @@ impl BenchBuilder { /// Fill cores `start..last` with dispute statement sets. The statement sets will have 3/4th of /// votes be valid, and 1/4th of votes be invalid. - fn create_disputes_with_no_spam( + fn create_disputes( &self, start: u32, last: u32, @@ -664,7 +664,7 @@ impl BenchBuilder { let backed_candidates = builder .create_backed_candidates(&builder.backed_and_concluding_cores, builder.code_upgrade); - let disputes = builder.create_disputes_with_no_spam( + let disputes = builder.create_disputes( builder.backed_and_concluding_cores.len() as u32, used_cores, builder.dispute_sessions.as_slice(), From bdd193fc22ebf2d82bf2b2008572348e5ff6c1dc Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 24 Nov 2022 10:42:07 +0200 Subject: [PATCH 07/44] Make tests compile - wip commit --- runtime/parachains/src/configuration/tests.rs | 5 - runtime/parachains/src/disputes/tests.rs | 103 +++++++----------- 2 files changed, 41 insertions(+), 67 deletions(-) diff --git a/runtime/parachains/src/configuration/tests.rs b/runtime/parachains/src/configuration/tests.rs index 6f2faf6cb204..30f9d5e5ac5e 100644 --- a/runtime/parachains/src/configuration/tests.rs +++ b/runtime/parachains/src/configuration/tests.rs @@ -402,11 +402,6 @@ fn setting_pending_config_members() { new_config.dispute_post_conclusion_acceptance_period, ) .unwrap(); - Configuration::set_dispute_max_spam_slots( - RuntimeOrigin::root(), - new_config.dispute_max_spam_slots, - ) - .unwrap(); Configuration::set_dispute_conclusion_by_time_out_period( RuntimeOrigin::root(), new_config.dispute_conclusion_by_time_out_period, diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 4d8ac714cb7b..8ec9d7a18a7c 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -34,7 +34,6 @@ use sp_core::{crypto::CryptoType, Pair}; /// Filtering updates the spam slots, as such update them. fn update_spam_slots(stmts: MultiDisputeStatementSet) -> CheckedMultiDisputeStatementSet { let config = >::config(); - let max_spam_slots = config.dispute_max_spam_slots; let post_conclusion_acceptance_period = config.dispute_post_conclusion_acceptance_period; stmts @@ -44,7 +43,6 @@ fn update_spam_slots(stmts: MultiDisputeStatementSet) -> CheckedMultiDisputeStat let filter = Pallet::::filter_dispute_data( &set, post_conclusion_acceptance_period, - max_spam_slots, VerifyDisputeSignatures::Skip, ); filter.filter_statement_set(set) @@ -171,7 +169,7 @@ fn test_import_new_participant_spam_inc() { concluded_at: None, }, ); - assert_eq!(summary.spam_slot_changes, vec![(ValidatorIndex(2), SpamSlotChange::Inc)]); + // TODO: assert_eq!(summary.spam_slot_changes, vec![(ValidatorIndex(2), SpamSlotChange::Inc)]); assert!(summary.slash_for.is_empty()); assert!(summary.slash_against.is_empty()); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 1, 0, 0, 0, 0, 0]); @@ -201,10 +199,10 @@ fn test_import_prev_participant_spam_dec_confirmed() { concluded_at: None, }, ); - assert_eq!( - summary.spam_slot_changes, - vec![(ValidatorIndex(0), SpamSlotChange::Dec), (ValidatorIndex(1), SpamSlotChange::Dec),], - ); + // TODO assert_eq!( + // summary.spam_slot_changes, + // vec![(ValidatorIndex(0), SpamSlotChange::Dec), (ValidatorIndex(1), SpamSlotChange::Dec),], + // ); assert!(summary.slash_for.is_empty()); assert!(summary.slash_against.is_empty()); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 1, 0, 0, 0, 0, 0]); @@ -240,10 +238,10 @@ fn test_import_prev_participant_spam_dec_confirmed_slash_for() { concluded_at: Some(0), }, ); - assert_eq!( - summary.spam_slot_changes, - vec![(ValidatorIndex(0), SpamSlotChange::Dec), (ValidatorIndex(1), SpamSlotChange::Dec),], - ); + // TODO: assert_eq!( + // summary.spam_slot_changes, + // vec![(ValidatorIndex(0), SpamSlotChange::Dec), (ValidatorIndex(1), SpamSlotChange::Dec),], + // ); assert_eq!(summary.slash_for, vec![ValidatorIndex(0), ValidatorIndex(2)]); assert!(summary.slash_against.is_empty()); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 1, 1, 1, 1, 1, 0]); @@ -281,7 +279,7 @@ fn test_import_slash_against() { concluded_at: Some(0), }, ); - assert!(summary.spam_slot_changes.is_empty()); + // TODO: assert!(summary.spam_slot_changes.is_empty()); assert!(summary.slash_for.is_empty()); assert_eq!(summary.slash_against, vec![ValidatorIndex(1), ValidatorIndex(5)]); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 0, 1, 1, 1, 1, 1]); @@ -331,14 +329,12 @@ 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() @@ -426,14 +422,13 @@ fn dispute_statement_becoming_onesided_due_to_spamslots_is_accepted() { 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])); + // TODO: assert_eq!(SpamSlots::::get(session), Some(vec![0, 0, 0, 1, 0, 0, 1])); // <-----> @@ -442,14 +437,13 @@ fn dispute_statement_becoming_onesided_due_to_spamslots_is_accepted() { 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])); + // TODO: assert_eq!(SpamSlots::::get(session), Some(vec![0, 1, 0, 1, 0, 0, 2])); // <-----> @@ -458,7 +452,6 @@ fn dispute_statement_becoming_onesided_due_to_spamslots_is_accepted() { let filter = Pallet::::filter_dispute_data( &set, post_conclusion_acceptance_period, - dispute_max_spam_slots, VerifyDisputeSignatures::Skip, ); assert_matches!(&filter, StatementSetFilter::RemoveAll); @@ -475,7 +468,6 @@ fn dispute_statement_becoming_onesided_due_to_spamslots_is_accepted() { 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()); @@ -489,13 +481,12 @@ fn dispute_statement_becoming_onesided_due_to_spamslots_is_accepted() { 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])); + // TODO: assert_eq!(SpamSlots::::get(session), Some(vec![0, 1, 1, 1, 1, 1, 3])); }); } @@ -587,7 +578,7 @@ fn test_dispute_timeout() { }]; let stmts = update_spam_slots(stmts); - assert_eq!(SpamSlots::::get(start - 1), Some(vec![1, 0, 0, 0, 0, 0, 1])); + // TODO: assert_eq!(SpamSlots::::get(start - 1), Some(vec![1, 0, 0, 0, 0, 0, 1])); assert_ok!( Pallet::::process_checked_multi_dispute_data(stmts), @@ -596,11 +587,11 @@ fn test_dispute_timeout() { // Run to timeout period run_to_block(start + dispute_conclusion_by_time_out_period, |_| None); - assert_eq!(SpamSlots::::get(start - 1), Some(vec![1, 0, 0, 0, 0, 0, 1])); + // TODO: assert_eq!(SpamSlots::::get(start - 1), Some(vec![1, 0, 0, 0, 0, 0, 1])); // Run to timeout + 1 in order to executive on_finalize(timeout) run_to_block(start + dispute_conclusion_by_time_out_period + 1, |_| None); - assert_eq!(SpamSlots::::get(start - 1), Some(vec![0, 0, 0, 0, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(start - 1), Some(vec![0, 0, 0, 0, 0, 0, 0])); }); } @@ -976,7 +967,7 @@ fn test_provide_multi_dispute_success_and_other() { }]; let stmts = update_spam_slots(stmts); - assert_eq!(SpamSlots::::get(3), Some(vec![1, 0, 1, 0, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![1, 0, 1, 0, 0, 0, 0])); assert_ok!( Pallet::::process_checked_multi_dispute_data(stmts), @@ -1039,8 +1030,8 @@ fn test_provide_multi_dispute_success_and_other() { Pallet::::process_checked_multi_dispute_data(stmts), vec![(4, candidate_hash.clone())], ); - assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); // Confirmed as no longer spam - assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); // Confirmed as no longer spam + // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); // v3 votes against 3 and for 5, v6 votes against 5. let stmts = vec![ @@ -1097,9 +1088,9 @@ fn test_provide_multi_dispute_success_and_other() { Pallet::::process_checked_multi_dispute_data(stmts), vec![(5, candidate_hash.clone())], ); - assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); - assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); - assert_eq!(SpamSlots::::get(5), Some(vec![0, 0, 1, 0, 0, 1, 0])); + // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(5), Some(vec![0, 0, 1, 0, 0, 1, 0])); // v2 votes for 3 and against 5 let stmts = vec![ @@ -1138,9 +1129,9 @@ fn test_provide_multi_dispute_success_and_other() { ]; let stmts = update_spam_slots(stmts); assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![]); - assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); - assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); - assert_eq!(SpamSlots::::get(5), Some(vec![0, 0, 0, 0, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(5), Some(vec![0, 0, 0, 0, 0, 0, 0])); let stmts = vec![ // 0, 4, and 5 vote against 5 @@ -1262,9 +1253,9 @@ fn test_provide_multi_dispute_success_and_other() { assert!(Pallet::::concluded_invalid(5, candidate_hash.clone())); // Ensure inclusion removes spam slots - assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); Pallet::::note_included(4, candidate_hash.clone(), 4); - assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 0, 0, 0, 0, 0])); + // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 0, 0, 0, 0, 0])); // Ensure the `reward_validator` function was correctly called assert_eq!( @@ -1382,10 +1373,10 @@ fn test_has_supermajority_against() { #[test] fn test_decrement_spam() { - let original_spam_slots = vec![0, 1, 2, 3, 4, 5, 6, 7]; + // TODO: let original_spam_slots = vec![0, 1, 2, 3, 4, 5, 6, 7]; // Test confirm is no-op - let mut spam_slots = original_spam_slots.clone(); + // TODO: let mut spam_slots = original_spam_slots.clone(); let dispute_state_confirm = DisputeState { validators_for: bitvec![u8, BitOrderLsb0; 1, 1, 0, 0, 0, 0, 0, 0], validators_against: bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], @@ -1393,14 +1384,14 @@ fn test_decrement_spam() { concluded_at: None, }; assert_eq!(DisputeStateFlags::from_state(&dispute_state_confirm), DisputeStateFlags::CONFIRMED); - assert_eq!( - decrement_spam(spam_slots.as_mut(), &dispute_state_confirm), - bitvec![u8, BitOrderLsb0; 1, 1, 1, 0, 0, 0, 0, 0], - ); - assert_eq!(spam_slots, original_spam_slots); + // TODO: assert_eq!( + // decrement_spam(spam_slots.as_mut(), &dispute_state_confirm), + // bitvec![u8, BitOrderLsb0; 1, 1, 1, 0, 0, 0, 0, 0], + // ); + // assert_eq!(spam_slots, original_spam_slots); // Test not confirm is decreasing spam - let mut spam_slots = original_spam_slots.clone(); + // TODO: let mut spam_slots = original_spam_slots.clone(); let dispute_state_no_confirm = DisputeState { validators_for: bitvec![u8, BitOrderLsb0; 1, 0, 0, 0, 0, 0, 0, 0], validators_against: bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], @@ -1411,11 +1402,11 @@ fn test_decrement_spam() { DisputeStateFlags::from_state(&dispute_state_no_confirm), DisputeStateFlags::default() ); - assert_eq!( - decrement_spam(spam_slots.as_mut(), &dispute_state_no_confirm), - bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], - ); - assert_eq!(spam_slots, vec![0, 1, 1, 3, 4, 5, 6, 7]); + // TODO: assert_eq!( + // decrement_spam(spam_slots.as_mut(), &dispute_state_no_confirm), + // bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], + // ); + // TODO: assert_eq!(spam_slots, vec![0, 1, 1, 3, 4, 5, 6, 7]); } #[test] @@ -1912,14 +1903,12 @@ fn apply_filter_all>( sets: I, ) -> Vec { let config = >::config(); - let max_spam_slots = config.dispute_max_spam_slots; let post_conclusion_acceptance_period = config.dispute_post_conclusion_acceptance_period; let mut acc = Vec::::new(); for dispute_statement in sets { if let Some(checked) = as DisputesHandler<::BlockNumber>>::filter_dispute_data( dispute_statement, - max_spam_slots, post_conclusion_acceptance_period, VerifyDisputeSignatures::Yes, ) { @@ -1993,13 +1982,11 @@ fn filter_removes_duplicates_within_set() { ], }; - let max_spam_slots = 10; let post_conclusion_acceptance_period = 10; let statements = as DisputesHandler< ::BlockNumber, >>::filter_dispute_data( statements, - max_spam_slots, post_conclusion_acceptance_period, VerifyDisputeSignatures::Yes, ); @@ -2087,15 +2074,7 @@ fn filter_bad_signatures_correctly_detects_single_sided() { #[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() - }; + let mock_genesis_config = MockGenesisConfig::default(); new_test_ext(mock_genesis_config).execute_with(|| { // We need 7 validators for the byzantine threshold to be 2 From 5a1800add11294c7cc1f6c74969ecbe632f54d4d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 24 Nov 2022 17:20:05 +0200 Subject: [PATCH 08/44] Rework `test_dispute_timeout`. Rename `update_spam_slots` to `filter_dispute_set` --- runtime/parachains/src/disputes/tests.rs | 50 +++++++++++++++++------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 8ec9d7a18a7c..7c7deb0c36d7 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -31,15 +31,13 @@ use frame_support::{ use primitives::v2::BlockNumber; use sp_core::{crypto::CryptoType, Pair}; -/// Filtering updates the spam slots, as such update them. -fn update_spam_slots(stmts: MultiDisputeStatementSet) -> CheckedMultiDisputeStatementSet { +fn filter_dispute_set(stmts: MultiDisputeStatementSet) -> CheckedMultiDisputeStatementSet { let config = >::config(); let post_conclusion_acceptance_period = config.dispute_post_conclusion_acceptance_period; stmts .into_iter() .filter_map(|set| { - // updates spam slots implicitly let filter = Pallet::::filter_dispute_data( &set, post_conclusion_acceptance_period, @@ -508,7 +506,7 @@ fn test_dispute_timeout() { }; new_test_ext(mock_genesis_config).execute_with(|| { - // We need 6 validators for the byzantine threshold to be 2 + // We need 7 validators for the byzantine threshold to be 2 let v0 = ::Pair::generate().0; let v1 = ::Pair::generate().0; let v2 = ::Pair::generate().0; @@ -545,10 +543,12 @@ fn test_dispute_timeout() { let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); - // v0 votes for 3, v6 against. + // 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. + let session = start - 1; let stmts = vec![DisputeStatementSet { candidate_hash: candidate_hash.clone(), - session: start - 1, + session, statements: vec![ ( DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), @@ -562,6 +562,18 @@ fn test_dispute_timeout() { .signing_payload(), ), ), + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(1), + v1.sign( + &ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash.clone(), + session: start - 1, + } + .signing_payload(), + ), + ), ( DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), ValidatorIndex(6), @@ -577,8 +589,7 @@ fn test_dispute_timeout() { ], }]; - let stmts = update_spam_slots(stmts); - // TODO: assert_eq!(SpamSlots::::get(start - 1), Some(vec![1, 0, 0, 0, 0, 0, 1])); + let stmts = filter_dispute_set(stmts); assert_ok!( Pallet::::process_checked_multi_dispute_data(stmts), @@ -587,11 +598,20 @@ fn test_dispute_timeout() { // Run to timeout period run_to_block(start + dispute_conclusion_by_time_out_period, |_| None); - // TODO: assert_eq!(SpamSlots::::get(start - 1), Some(vec![1, 0, 0, 0, 0, 0, 1])); + assert!(>::get(&session, &candidate_hash) + .expect("dispute should exist") + .concluded_at + .is_none()); // Run to timeout + 1 in order to executive on_finalize(timeout) run_to_block(start + dispute_conclusion_by_time_out_period + 1, |_| None); - // TODO: assert_eq!(SpamSlots::::get(start - 1), Some(vec![0, 0, 0, 0, 0, 0, 0])); + assert_eq!( + >::get(&session, &candidate_hash) + .expect("dispute should exist") + .concluded_at + .expect("dispute should have concluded"), + start + dispute_conclusion_by_time_out_period + 1 + ); }); } @@ -966,7 +986,7 @@ fn test_provide_multi_dispute_success_and_other() { ], }]; - let stmts = update_spam_slots(stmts); + let stmts = filter_dispute_set(stmts); // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![1, 0, 1, 0, 0, 0, 0])); assert_ok!( @@ -1024,7 +1044,7 @@ fn test_provide_multi_dispute_success_and_other() { }, ]; - let stmts = update_spam_slots(stmts); + let stmts = filter_dispute_set(stmts); assert_ok!( Pallet::::process_checked_multi_dispute_data(stmts), @@ -1083,7 +1103,7 @@ fn test_provide_multi_dispute_success_and_other() { }, ]; - let stmts = update_spam_slots(stmts); + let stmts = filter_dispute_set(stmts); assert_ok!( Pallet::::process_checked_multi_dispute_data(stmts), vec![(5, candidate_hash.clone())], @@ -1127,7 +1147,7 @@ fn test_provide_multi_dispute_success_and_other() { )], }, ]; - let stmts = update_spam_slots(stmts); + let stmts = filter_dispute_set(stmts); assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![]); // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); @@ -1209,7 +1229,7 @@ fn test_provide_multi_dispute_success_and_other() { ], }, ]; - let stmts = update_spam_slots(stmts); + let stmts = filter_dispute_set(stmts); assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![]); assert_eq!( From 55185633c205e7f508c545a0efe816b3e2889483 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 24 Nov 2022 17:40:46 +0200 Subject: [PATCH 09/44] Remove `dispute_statement_becoming_onesided_due_to_spamslots_is_accepted` and `filter_correctly_accounts_spam_slots` -> they bring no value with removed spam slots --- runtime/parachains/src/disputes/tests.rs | 303 ----------------------- 1 file changed, 303 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 7c7deb0c36d7..9206b46288a8 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -322,172 +322,6 @@ fn generate_dispute_statement_set( 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 post_conclusion_acceptance_period = 3; - - let mock_genesis_config = MockGenesisConfig { - configuration: crate::configuration::GenesisConfig { - config: HostConfiguration { - dispute_conclusion_by_time_out_period, - ..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, - 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()); - }); - // TODO: 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, - 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()); - }); - // TODO: 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, - 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, - 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, - VerifyDisputeSignatures::Skip, - ); - assert_matches!(&filter, StatementSetFilter::RemoveAll); - assert_matches!(filter.filter_statement_set(set.clone()), None); - - // TODO: assert_eq!(SpamSlots::::get(session), Some(vec![0, 1, 1, 1, 1, 1, 3])); - }); -} - // Test that dispute timeout is handled correctly. #[test] fn test_dispute_timeout() { @@ -2092,143 +1926,6 @@ fn filter_bad_signatures_correctly_detects_single_sided() { }) } -#[test] -fn filter_correctly_accounts_spam_slots() { - let mock_genesis_config = MockGenesisConfig::default(); - - new_test_ext(mock_genesis_config).execute_with(|| { - // We need 7 validators for the byzantine threshold to be 2 - 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; - - 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()), - (&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_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 = |c_hash: &CandidateHash, valid| { - ExplicitDisputeStatement { valid, candidate_hash: c_hash.clone(), session: 1 } - .signing_payload() - }; - - let payload_a = payload(&candidate_hash_a, true); - let payload_b = payload(&candidate_hash_b, true); - let payload_c = payload(&candidate_hash_c, true); - - let payload_a_bad = payload(&candidate_hash_a, false); - let payload_b_bad = payload(&candidate_hash_b, false); - let payload_c_bad = payload(&candidate_hash_c, false); - - 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 sig_2a = v2.sign(&payload_a_bad); - let sig_2b = v2.sign(&payload_b_bad); - let sig_2c = v2.sign(&payload_c_bad); - - let statements = vec![ - // validators 0 and 2 get 1 spam slot from this. - DisputeStatementSet { - candidate_hash: candidate_hash_a.clone(), - session: 1, - statements: vec![ - ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), - ValidatorIndex(0), - sig_0a.clone(), - ), - ( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(6), - sig_2a.clone(), - ), - ], - }, - // Validators 0, 2, and 3 get no spam slots for this - 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(), - ), - ( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(6), - sig_2b.clone(), - ), - ], - }, - // Validators 0 and 2 get an extra spam slot for this. - DisputeStatementSet { - candidate_hash: candidate_hash_c.clone(), - session: 1, - statements: vec![ - ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), - ValidatorIndex(0), - sig_0c.clone(), - ), - ( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(6), - sig_2c.clone(), - ), - ], - }, - ]; - - let old_statements = statements - .clone() - .into_iter() - .map(CheckedDisputeStatementSet::unchecked_from_unchecked) - .collect::>(); - let statements = apply_filter_all::(statements); - - assert_eq!(statements, old_statements); - }) -} - #[test] fn filter_removes_session_out_of_bounds() { new_test_ext(Default::default()).execute_with(|| { From 82a62ee758f424f7e5cfcac727eb4ff339a44cf2 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 25 Nov 2022 10:50:55 +0200 Subject: [PATCH 10/44] Fix `test_provide_multi_dispute_success_and_other` --- runtime/parachains/src/disputes/tests.rs | 189 +++++++++-------------- 1 file changed, 71 insertions(+), 118 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 9206b46288a8..29999e5aa529 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -737,8 +737,7 @@ fn test_freeze_provided_against_supermajority_for_included() { // * provide_multi_dispute: with success scenario // * disputes: correctness of datas // * could_be_invalid: correctness of datas -// * note_included: decrement spam correctly -// * spam slots: correctly incremented and decremented +// * unconfirmed disputes are filtered out // * ensure rewards and punishment are correctly called. #[test] fn test_provide_multi_dispute_success_and_other() { @@ -752,6 +751,7 @@ fn test_provide_multi_dispute_success_and_other() { let v5 = ::Pair::generate().0; let v6 = ::Pair::generate().0; + // Mapping between key pair and `ValidatorIndex` // v0 -> 0 // v1 -> 3 // v2 -> 6 @@ -788,7 +788,7 @@ fn test_provide_multi_dispute_success_and_other() { let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); - // v0 votes for 3, v6 votes against + // v0 and v1 vote for 3, v6 votes against let stmts = vec![DisputeStatementSet { candidate_hash: candidate_hash.clone(), session: 3, @@ -817,77 +817,66 @@ fn test_provide_multi_dispute_success_and_other() { .signing_payload(), ), ), + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(3), + v1.sign( + &ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash.clone(), + session: 3, + } + .signing_payload(), + ), + ), ], }]; let stmts = filter_dispute_set(stmts); - // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![1, 0, 1, 0, 0, 0, 0])); assert_ok!( Pallet::::process_checked_multi_dispute_data(stmts), vec![(3, candidate_hash.clone())], ); - // v1 votes for 4 and for 3, v6 votes against 4. - let stmts = vec![ - DisputeStatementSet { - candidate_hash: candidate_hash.clone(), - session: 4, - statements: vec![ - ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), - ValidatorIndex(3), - v1.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 4, - } - .signing_payload(), - ), - ), - ( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(2), - v6.sign( - &ExplicitDisputeStatement { - valid: false, - candidate_hash: candidate_hash.clone(), - session: 4, - } - .signing_payload(), - ), - ), - ], - }, - DisputeStatementSet { - candidate_hash: candidate_hash.clone(), - session: 3, - statements: vec![( + // v1 votes for 4, v6 votes against 4. + let stmts = vec![DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session: 4, + statements: vec![ + ( DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), ValidatorIndex(3), v1.sign( &ExplicitDisputeStatement { valid: true, candidate_hash: candidate_hash.clone(), - session: 3, + session: 4, } .signing_payload(), ), - )], - }, - ]; + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(2), + v6.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session: 4, + } + .signing_payload(), + ), + ), + ], + }]; let stmts = filter_dispute_set(stmts); - assert_ok!( - Pallet::::process_checked_multi_dispute_data(stmts), - vec![(4, candidate_hash.clone())], - ); - // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); // Confirmed as no longer spam - // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); + // Not confirmed => should be filtered out + assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![],); - // v3 votes against 3 and for 5, v6 votes against 5. + // v3 votes against 3 and for 5, v2 and v6 vote against 5. let stmts = vec![ DisputeStatementSet { candidate_hash: candidate_hash.clone(), @@ -921,6 +910,18 @@ fn test_provide_multi_dispute_success_and_other() { .signing_payload(), ), ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(6), + v2.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session: 5, + } + .signing_payload(), + ), + ), ( DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), ValidatorIndex(2), @@ -942,50 +943,26 @@ fn test_provide_multi_dispute_success_and_other() { Pallet::::process_checked_multi_dispute_data(stmts), vec![(5, candidate_hash.clone())], ); - // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); - // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); - // TODO: assert_eq!(SpamSlots::::get(5), Some(vec![0, 0, 1, 0, 0, 1, 0])); - // v2 votes for 3 and against 5 - let stmts = vec![ - DisputeStatementSet { - candidate_hash: candidate_hash.clone(), - session: 3, - statements: vec![( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), - ValidatorIndex(6), - v2.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 3, - } - .signing_payload(), - ), - )], - }, - DisputeStatementSet { - candidate_hash: candidate_hash.clone(), - session: 5, - statements: vec![( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(6), - v2.sign( - &ExplicitDisputeStatement { - valid: false, - candidate_hash: candidate_hash.clone(), - session: 5, - } - .signing_payload(), - ), - )], - }, - ]; + // v2 votes for 3 + let stmts = vec![DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session: 3, + statements: vec![( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(6), + v2.sign( + &ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash.clone(), + session: 3, + } + .signing_payload(), + ), + )], + }]; let stmts = filter_dispute_set(stmts); assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![]); - // TODO: assert_eq!(SpamSlots::::get(3), Some(vec![0, 0, 0, 0, 0, 0, 0])); - // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); - // TODO: assert_eq!(SpamSlots::::get(5), Some(vec![0, 0, 0, 0, 0, 0, 0])); let stmts = vec![ // 0, 4, and 5 vote against 5 @@ -1089,16 +1066,6 @@ fn test_provide_multi_dispute_success_and_other() { concluded_at: Some(6), // 5 vote for } ), - ( - 4, - candidate_hash.clone(), - DisputeState { - validators_for: bitvec![u8, BitOrderLsb0; 0, 0, 0, 1, 0, 0, 0], - validators_against: bitvec![u8, BitOrderLsb0; 0, 0, 1, 0, 0, 0, 0], - start: 6, - concluded_at: None, - } - ), ] ); @@ -1106,22 +1073,14 @@ fn test_provide_multi_dispute_success_and_other() { assert!(!Pallet::::concluded_invalid(4, candidate_hash.clone())); assert!(Pallet::::concluded_invalid(5, candidate_hash.clone())); - // Ensure inclusion removes spam slots - // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0])); - Pallet::::note_included(4, candidate_hash.clone(), 4); - // TODO: assert_eq!(SpamSlots::::get(4), Some(vec![0, 0, 0, 0, 0, 0, 0])); - // Ensure the `reward_validator` function was correctly called assert_eq!( REWARD_VALIDATORS.with(|r| r.borrow().clone()), vec![ - (3, vec![ValidatorIndex(0), ValidatorIndex(2)]), - (4, vec![ValidatorIndex(2), ValidatorIndex(3)]), - (3, vec![ValidatorIndex(3)]), + (3, vec![ValidatorIndex(0), ValidatorIndex(2), ValidatorIndex(3)]), (3, vec![ValidatorIndex(5)]), - (5, vec![ValidatorIndex(2), ValidatorIndex(5)]), + (5, vec![ValidatorIndex(2), ValidatorIndex(5), ValidatorIndex(6)]), (3, vec![ValidatorIndex(6)]), - (5, vec![ValidatorIndex(6)]), (5, vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(4)]), (3, vec![ValidatorIndex(1), ValidatorIndex(4)]), ], @@ -1131,14 +1090,11 @@ fn test_provide_multi_dispute_success_and_other() { assert_eq!( PUNISH_VALIDATORS_AGAINST.with(|r| r.borrow().clone()), vec![ - (3, vec![]), - (4, vec![]), (3, vec![]), (3, vec![]), (5, vec![]), (3, vec![]), (5, vec![]), - (5, vec![]), (3, vec![ValidatorIndex(2), ValidatorIndex(5)]), ], ); @@ -1147,13 +1103,10 @@ fn test_provide_multi_dispute_success_and_other() { assert_eq!( PUNISH_VALIDATORS_FOR.with(|r| r.borrow().clone()), vec![ - (3, vec![]), - (4, vec![]), (3, vec![]), (3, vec![]), (5, vec![]), (3, vec![]), - (5, vec![]), (5, vec![ValidatorIndex(5)]), (3, vec![]), ], From 1d3eb05b9e8a84919c01b2ab346eb63d7cc47a98 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 25 Nov 2022 10:59:47 +0200 Subject: [PATCH 11/44] Remove an old comment --- runtime/parachains/src/disputes.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index b7a02511d915..fe921ed24f13 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -987,7 +987,6 @@ impl Pallet { summary.new_participants.get(v_i.0 as usize).map(|b| *b.as_ref()) { 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. // Overflow of the counters is no concern, disputes are limited by weight. From a0e60de8001edd792ece038ee757ac7fbe1d0071 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 25 Nov 2022 11:35:53 +0200 Subject: [PATCH 12/44] Remove spam slots from tests - clean todo comments --- runtime/parachains/src/disputes/tests.rs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 29999e5aa529..86d3d48c6a7c 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -131,7 +131,7 @@ fn test_dispute_state_flag_from_state() { } #[test] -fn test_import_new_participant_spam_inc() { +fn test_import_new_participant() { let mut importer = DisputeStateImporter::new( DisputeState { validators_for: bitvec![u8, BitOrderLsb0; 1, 0, 0, 0, 0, 0, 0, 0], @@ -167,14 +167,13 @@ fn test_import_new_participant_spam_inc() { concluded_at: None, }, ); - // TODO: assert_eq!(summary.spam_slot_changes, vec![(ValidatorIndex(2), SpamSlotChange::Inc)]); assert!(summary.slash_for.is_empty()); assert!(summary.slash_against.is_empty()); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 1, 0, 0, 0, 0, 0]); } #[test] -fn test_import_prev_participant_spam_dec_confirmed() { +fn test_import_prev_participant_confirmed() { let mut importer = DisputeStateImporter::new( DisputeState { validators_for: bitvec![u8, BitOrderLsb0; 1, 0, 0, 0, 0, 0, 0, 0], @@ -197,10 +196,7 @@ fn test_import_prev_participant_spam_dec_confirmed() { concluded_at: None, }, ); - // TODO assert_eq!( - // summary.spam_slot_changes, - // vec![(ValidatorIndex(0), SpamSlotChange::Dec), (ValidatorIndex(1), SpamSlotChange::Dec),], - // ); + assert!(summary.slash_for.is_empty()); assert!(summary.slash_against.is_empty()); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 1, 0, 0, 0, 0, 0]); @@ -208,7 +204,7 @@ fn test_import_prev_participant_spam_dec_confirmed() { } #[test] -fn test_import_prev_participant_spam_dec_confirmed_slash_for() { +fn test_import_prev_participant_confirmed_slash_for() { let mut importer = DisputeStateImporter::new( DisputeState { validators_for: bitvec![u8, BitOrderLsb0; 1, 0, 0, 0, 0, 0, 0, 0], @@ -236,10 +232,7 @@ fn test_import_prev_participant_spam_dec_confirmed_slash_for() { concluded_at: Some(0), }, ); - // TODO: assert_eq!( - // summary.spam_slot_changes, - // vec![(ValidatorIndex(0), SpamSlotChange::Dec), (ValidatorIndex(1), SpamSlotChange::Dec),], - // ); + assert_eq!(summary.slash_for, vec![ValidatorIndex(0), ValidatorIndex(2)]); assert!(summary.slash_against.is_empty()); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 1, 1, 1, 1, 1, 0]); @@ -277,7 +270,6 @@ fn test_import_slash_against() { concluded_at: Some(0), }, ); - // TODO: assert!(summary.spam_slot_changes.is_empty()); assert!(summary.slash_for.is_empty()); assert_eq!(summary.slash_against, vec![ValidatorIndex(1), ValidatorIndex(5)]); assert_eq!(summary.new_participants, bitvec![u8, BitOrderLsb0; 0, 0, 0, 1, 1, 1, 1, 1]); From e02089d68fe9765bff770eb2da8bb6fbb4c57be5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 25 Nov 2022 11:36:21 +0200 Subject: [PATCH 13/44] Remove test - `test_decrement_spam` --- runtime/parachains/src/disputes/tests.rs | 38 ------------------------ 1 file changed, 38 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 86d3d48c6a7c..f544bcd58799 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -1170,44 +1170,6 @@ fn test_has_supermajority_against() { ); } -#[test] -fn test_decrement_spam() { - // TODO: let original_spam_slots = vec![0, 1, 2, 3, 4, 5, 6, 7]; - - // Test confirm is no-op - // TODO: let mut spam_slots = original_spam_slots.clone(); - let dispute_state_confirm = DisputeState { - validators_for: bitvec![u8, BitOrderLsb0; 1, 1, 0, 0, 0, 0, 0, 0], - validators_against: bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], - start: 0, - concluded_at: None, - }; - assert_eq!(DisputeStateFlags::from_state(&dispute_state_confirm), DisputeStateFlags::CONFIRMED); - // TODO: assert_eq!( - // decrement_spam(spam_slots.as_mut(), &dispute_state_confirm), - // bitvec![u8, BitOrderLsb0; 1, 1, 1, 0, 0, 0, 0, 0], - // ); - // assert_eq!(spam_slots, original_spam_slots); - - // Test not confirm is decreasing spam - // TODO: let mut spam_slots = original_spam_slots.clone(); - let dispute_state_no_confirm = DisputeState { - validators_for: bitvec![u8, BitOrderLsb0; 1, 0, 0, 0, 0, 0, 0, 0], - validators_against: bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], - start: 0, - concluded_at: None, - }; - assert_eq!( - DisputeStateFlags::from_state(&dispute_state_no_confirm), - DisputeStateFlags::default() - ); - // TODO: assert_eq!( - // decrement_spam(spam_slots.as_mut(), &dispute_state_no_confirm), - // bitvec![u8, BitOrderLsb0; 1, 0, 1, 0, 0, 0, 0, 0], - // ); - // TODO: assert_eq!(spam_slots, vec![0, 1, 1, 3, 4, 5, 6, 7]); -} - #[test] fn test_check_signature() { let validator_id = ::Pair::generate().0; From d1a468f655bfda4379e68e17d9ade5f8038486b5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 25 Nov 2022 14:19:00 +0200 Subject: [PATCH 14/44] todo comments --- runtime/parachains/src/disputes/slashing.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/parachains/src/disputes/slashing.rs b/runtime/parachains/src/disputes/slashing.rs index 419db8654d89..eade18ee59ab 100644 --- a/runtime/parachains/src/disputes/slashing.rs +++ b/runtime/parachains/src/disputes/slashing.rs @@ -569,6 +569,7 @@ impl Pallet { fn initializer_on_new_session(session_index: SessionIndex) { // This should be small, as disputes are limited by spam slots, so no limit is // fine. + // TODO: without spam slots is this still valid? const REMOVE_LIMIT: u32 = u32::MAX; let config = >::config(); From 7519851edbdd8b4561ccdbde7b72744b931fd0dc Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 1 Dec 2022 16:13:16 +0200 Subject: [PATCH 15/44] Update TODO comments --- runtime/parachains/src/configuration.rs | 2 +- runtime/parachains/src/disputes/slashing.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/runtime/parachains/src/configuration.rs b/runtime/parachains/src/configuration.rs index 7e3bb65ba7fd..357c4385a917 100644 --- a/runtime/parachains/src/configuration.rs +++ b/runtime/parachains/src/configuration.rs @@ -193,7 +193,7 @@ pub struct HostConfiguration { /// How long after dispute conclusion to accept statements. pub dispute_post_conclusion_acceptance_period: BlockNumber, /// The maximum number of dispute spam slots - /// TODO: Should be removed in next version + /// TODO: This will be removed once https://github.com/paritytech/polkadot/pull/6271 is merged pub dispute_max_spam_slots: u32, /// How long it takes for a dispute to conclude by time-out, if no supermajority is reached. pub dispute_conclusion_by_time_out_period: BlockNumber, diff --git a/runtime/parachains/src/disputes/slashing.rs b/runtime/parachains/src/disputes/slashing.rs index eade18ee59ab..419db8654d89 100644 --- a/runtime/parachains/src/disputes/slashing.rs +++ b/runtime/parachains/src/disputes/slashing.rs @@ -569,7 +569,6 @@ impl Pallet { fn initializer_on_new_session(session_index: SessionIndex) { // This should be small, as disputes are limited by spam slots, so no limit is // fine. - // TODO: without spam slots is this still valid? const REMOVE_LIMIT: u32 = u32::MAX; let config = >::config(); From d4df3d0950e0b1437ea22a7ad489675b5e481fcf Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 1 Dec 2022 18:32:36 +0200 Subject: [PATCH 16/44] Extract `test_unconfirmed_are_ignored` as separate test case --- runtime/parachains/src/disputes/tests.rs | 100 ++++++++++++++--------- 1 file changed, 62 insertions(+), 38 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index f544bcd58799..ce5badfc398b 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -725,11 +725,72 @@ fn test_freeze_provided_against_supermajority_for_included() { }); } +#[test] +fn test_unconfirmed_are_ignored() { + new_test_ext(Default::default()).execute_with(|| { + // 7 validators needed for byzantine threshold of 2. + let v0 = ::Pair::generate().0; + let v1 = ::Pair::generate().0; + + // Mapping between key pair and `ValidatorIndex` + // v0 -> 0 + // v1 -> 3 + + run_to_block(6, |b| { + // a new session at each block + Some(( + true, + b, + vec![(&0, v0.public()), (&1, v1.public())], + Some(vec![(&0, v0.public()), (&1, v1.public())]), + )) + }); + + let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); + + // v0 votes for 4, v1 votes against 4. + let stmts = vec![DisputeStatementSet { + candidate_hash: candidate_hash.clone(), + session: 4, + statements: vec![ + ( + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), + ValidatorIndex(0), + v0.sign( + &ExplicitDisputeStatement { + valid: true, + candidate_hash: candidate_hash.clone(), + session: 4, + } + .signing_payload(), + ), + ), + ( + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), + ValidatorIndex(3), + v1.sign( + &ExplicitDisputeStatement { + valid: false, + candidate_hash: candidate_hash.clone(), + session: 4, + } + .signing_payload(), + ), + ), + ], + }]; + + let stmts = filter_dispute_set(stmts); + + // Not confirmed => should be filtered out + assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![],); + }); +} + // tests for: // * provide_multi_dispute: with success scenario // * disputes: correctness of datas // * could_be_invalid: correctness of datas -// * unconfirmed disputes are filtered out // * ensure rewards and punishment are correctly called. #[test] fn test_provide_multi_dispute_success_and_other() { @@ -831,43 +892,6 @@ fn test_provide_multi_dispute_success_and_other() { vec![(3, candidate_hash.clone())], ); - // v1 votes for 4, v6 votes against 4. - let stmts = vec![DisputeStatementSet { - candidate_hash: candidate_hash.clone(), - session: 4, - statements: vec![ - ( - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit), - ValidatorIndex(3), - v1.sign( - &ExplicitDisputeStatement { - valid: true, - candidate_hash: candidate_hash.clone(), - session: 4, - } - .signing_payload(), - ), - ), - ( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(2), - v6.sign( - &ExplicitDisputeStatement { - valid: false, - candidate_hash: candidate_hash.clone(), - session: 4, - } - .signing_payload(), - ), - ), - ], - }]; - - let stmts = filter_dispute_set(stmts); - - // Not confirmed => should be filtered out - assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![],); - // v3 votes against 3 and for 5, v2 and v6 vote against 5. let stmts = vec![ DisputeStatementSet { From 397a060ae3dd6f244fd0e6778f6d3e7e360825fc Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 1 Dec 2022 18:33:36 +0200 Subject: [PATCH 17/44] Remove dead code --- runtime/parachains/src/disputes/tests.rs | 39 ------------------------ 1 file changed, 39 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index ce5badfc398b..91e4843a90b7 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -23,7 +23,6 @@ use crate::{ Test, PUNISH_VALIDATORS_AGAINST, PUNISH_VALIDATORS_FOR, REWARD_VALIDATORS, }, }; -use assert_matches::assert_matches; use frame_support::{ assert_err, assert_noop, assert_ok, traits::{OnFinalize, OnInitialize}, @@ -276,44 +275,6 @@ 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 that dispute timeout is handled correctly. #[test] fn test_dispute_timeout() { From 84cae48555253bb201e4e152aa39064c7a922c8f Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 1 Dec 2022 22:58:58 +0200 Subject: [PATCH 18/44] Fix `test_unconfirmed_are_ignored` --- runtime/parachains/src/disputes/tests.rs | 30 ++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 91e4843a90b7..86443b0ca591 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -692,18 +692,44 @@ fn test_unconfirmed_are_ignored() { // 7 validators needed for byzantine threshold of 2. 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())], - Some(vec![(&0, v0.public()), (&1, v1.public())]), + 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()), + ]), )) }); From 306acd3ded697d777227304826f95bbf192a97e3 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 2 Dec 2022 17:47:23 +0200 Subject: [PATCH 19/44] Remove dead code in `filter_dispute_data` --- runtime/parachains/src/disputes.rs | 45 ++++++------------------------ 1 file changed, 8 insertions(+), 37 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index fe921ed24f13..b76006403f3b 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -886,24 +886,21 @@ impl Pallet { let n_validators = session_info.validators.len(); // Check for ancient. - let (first_votes, dispute_state) = { + 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 } - (false, dispute_state) + dispute_state } else { // 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, - }, - ) + DisputeState { + validators_for: bitvec![u8, BitOrderLsb0; 0; n_validators], + validators_against: bitvec![u8, BitOrderLsb0; 0; n_validators], + start: now, + concluded_at: None, + } } }; @@ -972,32 +969,6 @@ impl Pallet { return StatementSetFilter::RemoveAll } - let is_local = >::contains_key(&set.session, &set.candidate_hash); - if !is_local { - // 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 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()) - { - match statement { - // Note that this does not distinguish between pro or con votes, - // since allowing both of them. - // Overflow of the counters is no concern, disputes are limited by weight. - DisputeStatement::Valid(_) => vote_for_count += 1, - DisputeStatement::Invalid(_) => vote_against_count += 1, - } - } - }); - } - } - filter } From b6661ce23373e138411e9df72678a79b4fd6a534 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 15 Dec 2022 10:41:02 +0200 Subject: [PATCH 20/44] Fix weights (related to commit "Remove `SpamSlots`") --- runtime/parachains/src/disputes.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index b76006403f3b..f16bfde1a352 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -778,12 +778,7 @@ impl Pallet { dispute.concluded_at = Some(now); >::insert(session_index, candidate_hash, &dispute); - if >::contains_key(&session_index, &candidate_hash) { - weight += T::DbWeight::get().reads_writes(1, 1); - continue - } - - weight += T::DbWeight::get().reads_writes(2, 2); + weight += T::DbWeight::get().reads_writes(1, 1); } } From 8c5f10ade77a33c818353106f096f15f4bbb7619 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 15 Dec 2022 17:37:58 +0200 Subject: [PATCH 21/44] Disputes migration - first try --- runtime/parachains/src/disputes.rs | 10 ++++ runtime/parachains/src/disputes/migration.rs | 59 ++++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 runtime/parachains/src/disputes/migration.rs diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index f16bfde1a352..0f44fb9b75e9 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -46,6 +46,8 @@ mod tests; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; +mod migration; + /// Whether the dispute is local or remote. #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)] pub enum DisputeLocation { @@ -467,6 +469,14 @@ pub mod pallet { T::BlockNumber, >; + /// Maps session indices to a vector indicating the number of potentially-spam disputes + /// each validator is participating in. Potentially-spam disputes are remote disputes which have + /// fewer than `byzantine_threshold + 1` validators. + /// + /// The i'th entry of the vector corresponds to the i'th validator in the session. + #[pallet::storage] + pub(super) type SpamSlots = StorageMap<_, Twox64Concat, SessionIndex, Vec>; + /// Whether the chain is frozen. Starts as `None`. When this is `Some`, /// the chain will not accept any new parachain blocks for backing or inclusion, /// and its value indicates the last valid block number in the chain. diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs new file mode 100644 index 000000000000..e18ab0b4e5cb --- /dev/null +++ b/runtime/parachains/src/disputes/migration.rs @@ -0,0 +1,59 @@ +// Copyright 2017-2022 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +//! Storage migration(s) related to disputes pallet + +use crate::disputes::{pallet::SpamSlots, Config, Pallet}; +use frame_support::{ + pallet_prelude::*, + traits::{OnRuntimeUpgrade, StorageVersion}, + weights::Weight, +}; +/// The current storage version. +const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + +pub struct MigrateToV1(sp_std::marker::PhantomData); +impl OnRuntimeUpgrade for MigrateToV1 { + fn on_runtime_upgrade() -> Weight { + let mut weight: Weight = Weight::zero(); + + if StorageVersion::get::>() < STORAGE_VERSION { + weight += migrate_to_v1::(); + STORAGE_VERSION.put::>(); + } + weight + } +} + +/// Migrates the pallet storage to the most recent version, checking and setting the `StorageVersion`. +pub fn migrate_to_v1() -> Weight { + let weight: Weight = Weight::zero(); + + // SpamSlots should not contain too many keys so removing everything at once should be safe + loop { + let res = SpamSlots::::clear(0, None); + // `loops` is the number of iterations => used to calculate read weights + // `backend` is the number of keys removed from the backend => used to calculate write weights + weight + .saturating_add(T::DbWeight::get().reads_writes(res.loops as u64, res.backend as u64)); + + if res.maybe_cursor.is_none() { + break + } + } + + weight +} From 80fda2e30f1b71be40746e177fa331120434cfdc Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Dec 2022 13:51:07 +0200 Subject: [PATCH 22/44] Remove `dispute_max_spam_slots` + storage migration --- runtime/parachains/src/configuration.rs | 4 -- .../parachains/src/configuration/migration.rs | 45 +++++++++---------- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/runtime/parachains/src/configuration.rs b/runtime/parachains/src/configuration.rs index 357c4385a917..b360c108e428 100644 --- a/runtime/parachains/src/configuration.rs +++ b/runtime/parachains/src/configuration.rs @@ -192,9 +192,6 @@ pub struct HostConfiguration { pub dispute_period: SessionIndex, /// How long after dispute conclusion to accept statements. pub dispute_post_conclusion_acceptance_period: BlockNumber, - /// The maximum number of dispute spam slots - /// TODO: This will be removed once https://github.com/paritytech/polkadot/pull/6271 is merged - pub dispute_max_spam_slots: u32, /// How long it takes for a dispute to conclude by time-out, if no supermajority is reached. pub dispute_conclusion_by_time_out_period: BlockNumber, /// The amount of consensus slots that must pass between submitting an assignment and @@ -264,7 +261,6 @@ impl> Default for HostConfiguration /// v1-v2: /// v2-v3: -pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(3); +/// v3-v4: +pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(4); -pub mod v3 { +pub mod v4 { use super::*; - use frame_support::traits::OnRuntimeUpgrade; + use frame_support::{traits::OnRuntimeUpgrade, weights::constants::WEIGHT_REF_TIME_PER_MILLIS}; use primitives::v2::{Balance, SessionIndex}; // Copied over from configuration.rs @ de9e147695b9f1be8bd44e07861a31e483c8343a and removed @@ -51,7 +48,7 @@ pub mod v3 { pub validation_upgrade_delay: BlockNumber, pub max_pov_size: u32, pub max_downward_message_size: u32, - pub ump_service_total_weight: OldWeight, + pub ump_service_total_weight: Weight, pub hrmp_max_parachain_outbound_channels: u32, pub hrmp_max_parathread_outbound_channels: u32, pub hrmp_sender_deposit: Balance, @@ -79,7 +76,7 @@ pub mod v3 { pub zeroth_delay_tranche_width: u32, pub needed_approvals: u32, pub relay_vrf_modulo_samples: u32, - pub ump_max_individual_weight: OldWeight, + pub ump_max_individual_weight: Weight, pub pvf_checking_enabled: bool, pub pvf_voting_ttl: SessionIndex, pub minimum_validation_upgrade_delay: BlockNumber, @@ -114,7 +111,7 @@ pub mod v3 { max_upward_queue_count: Default::default(), max_upward_queue_size: Default::default(), max_downward_message_size: Default::default(), - ump_service_total_weight: OldWeight(Default::default()), + ump_service_total_weight: Default::default(), max_upward_message_size: Default::default(), max_upward_message_num_per_candidate: Default::default(), hrmp_sender_deposit: Default::default(), @@ -127,8 +124,9 @@ pub mod v3 { hrmp_max_parachain_outbound_channels: Default::default(), hrmp_max_parathread_outbound_channels: Default::default(), hrmp_max_message_num_per_candidate: Default::default(), - ump_max_individual_weight: OldWeight( - frame_support::weights::constants::WEIGHT_REF_TIME_PER_MILLIS * 20, + ump_max_individual_weight: Weight::from_parts( + 20u64 * WEIGHT_REF_TIME_PER_MILLIS, + MAX_POV_SIZE as u64, ), pvf_checking_enabled: false, pvf_voting_ttl: 2u32.into(), @@ -137,32 +135,32 @@ pub mod v3 { } } - pub struct MigrateToV3(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for MigrateToV3 { + pub struct MigrateToV4(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV4 { fn on_runtime_upgrade() -> Weight { - if StorageVersion::get::>() == 2 { - let weight_consumed = migrate_to_v3::(); + if StorageVersion::get::>() == 3 { + let weight_consumed = migrate_to_v4::(); - log::info!(target: configuration::LOG_TARGET, "MigrateToV3 executed successfully"); + log::info!(target: configuration::LOG_TARGET, "MigrateToV4 executed successfully"); STORAGE_VERSION.put::>(); weight_consumed } else { - log::warn!(target: configuration::LOG_TARGET, "MigrateToV3 should be removed."); + log::warn!(target: configuration::LOG_TARGET, "MigrateToV4 should be removed."); T::DbWeight::get().reads(1) } } } } -fn migrate_to_v3() -> Weight { +fn migrate_to_v4() -> Weight { // Unusual formatting is justified: // - make it easier to verify that fields assign what they supposed to assign. // - this code is transient and will be removed after all migrations are done. // - this code is important enough to optimize for legibility sacrificing consistency. #[rustfmt::skip] let translate = - |pre: v3::OldHostConfiguration>| -> + |pre: v4::OldHostConfiguration>| -> configuration::HostConfiguration> { super::HostConfiguration { @@ -177,6 +175,7 @@ validation_upgrade_cooldown : pre.validation_upgrade_cooldown, validation_upgrade_delay : pre.validation_upgrade_delay, max_pov_size : pre.max_pov_size, max_downward_message_size : pre.max_downward_message_size, +ump_service_total_weight : pre.ump_service_total_weight, hrmp_max_parachain_outbound_channels : pre.hrmp_max_parachain_outbound_channels, hrmp_max_parathread_outbound_channels : pre.hrmp_max_parathread_outbound_channels, hrmp_sender_deposit : pre.hrmp_sender_deposit, @@ -197,19 +196,17 @@ max_validators_per_core : pre.max_validators_per_core, max_validators : pre.max_validators, dispute_period : pre.dispute_period, dispute_post_conclusion_acceptance_period: pre.dispute_post_conclusion_acceptance_period, -dispute_max_spam_slots : pre.dispute_max_spam_slots, dispute_conclusion_by_time_out_period : pre.dispute_conclusion_by_time_out_period, no_show_slots : pre.no_show_slots, n_delay_tranches : pre.n_delay_tranches, zeroth_delay_tranche_width : pre.zeroth_delay_tranche_width, needed_approvals : pre.needed_approvals, relay_vrf_modulo_samples : pre.relay_vrf_modulo_samples, +ump_max_individual_weight : pre.ump_max_individual_weight, pvf_checking_enabled : pre.pvf_checking_enabled, pvf_voting_ttl : pre.pvf_voting_ttl, minimum_validation_upgrade_delay : pre.minimum_validation_upgrade_delay, -ump_service_total_weight: Weight::from_ref_time(pre.ump_service_total_weight.0).set_proof_size(MAX_POV_SIZE as u64), -ump_max_individual_weight: Weight::from_ref_time(pre.ump_max_individual_weight.0).set_proof_size(MAX_POV_SIZE as u64), } }; From 28993af793a5c69cced77bc3dd6dfe6cb3295b70 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Dec 2022 14:29:07 +0200 Subject: [PATCH 23/44] Fix `HostConfig` migration tests --- .../parachains/src/configuration/migration.rs | 133 ++++++++++-------- runtime/parachains/src/configuration/tests.rs | 1 - 2 files changed, 71 insertions(+), 63 deletions(-) diff --git a/runtime/parachains/src/configuration/migration.rs b/runtime/parachains/src/configuration/migration.rs index 93204a6d8dcb..19167f0533b7 100644 --- a/runtime/parachains/src/configuration/migration.rs +++ b/runtime/parachains/src/configuration/migration.rs @@ -232,26 +232,38 @@ mod tests { #[test] fn v2_deserialized_from_actual_data() { - // Fetched at Kusama 14,703,780 (0x3b2c305d01bd4adf1973d32a2d55ca1260a55eea8dfb3168e317c57f2841fdf1) + // Example how to get new `raw_config`: + // We'll obtain the raw_config hes for block + // 15,772,152 (0xf89d3ab5312c5f70d396dc59612f0aa65806c798346f9db4b35278baed2e0e53) on Kusama. + // Steps: + // 1. Go to Polkadot.js -> Developer -> Chain state -> Storage: https://polkadot.js.org/apps/#/chainstate + // 2. Set these parameters: + // 2.1. selected state query: configuration; activeConfig(): PolkadotRuntimeParachainsConfigurationHostConfiguration + // 2.2. blockhash to query at: 0xf89d3ab5312c5f70d396dc59612f0aa65806c798346f9db4b35278baed2e0e53 (the hash of the block) + // 2.3. Note the value of encoded storage key -> 0x06de3d8a54d27e44a9d5ce189618f22db4b49d95320d9021994c850f25b8e385 for the referenced block. + // 2.4. You'll also need the decoded values to update the test. + // 3. Go to Polkadot.js -> Developer -> Chain state -> Raw storage + // 3.1 Enter the encoded storage key and you get the raw config. + + // Fetched at Kusama 15,772,152 (0xf89d3ab5312c5f70d396dc59612f0aa65806c798346f9db4b35278baed2e0e53) // // This exceeds the maximal line width length, but that's fine, since this is not code and // doesn't need to be read and also leaving it as one line allows to easily copy it. - let raw_config = hex_literal::hex!["0000a000005000000a00000000c8000000c800000a0000000a000000100e0000580200000000500000c8000000e87648170000001e00000000000000005039278c0400000000000000000000005039278c0400000000000000000000e8030000009001001e00000000000000009001008070000000000000000000000a0000000a0000000a00000001000000010500000001c8000000060000005802000002000000580200000200000059000000000000001e0000002800000000c817a804000000000200000014000000"]; + let raw_config = hex_literal::hex!["0000a000005000000a00000000c8000000c800000a0000000a000000100e0000580200000000500000c800000700e8764817020040011e00000000000000005039278c0400000000000000000000005039278c0400000000000000000000e8030000009001001e00000000000000009001008070000000000000000000000a0000000a0000000a00000001000000010500000001c8000000060000005802000002000000580200000200000059000000000000001e000000280000000700c817a80402004001000200000014000000"]; - let v2 = - v3::OldHostConfiguration::::decode(&mut &raw_config[..]) + let v3 = + v4::OldHostConfiguration::::decode(&mut &raw_config[..]) .unwrap(); // We check only a sample of the values here. If we missed any fields or messed up data types // that would skew all the fields coming after. - assert_eq!(v2.max_code_size, 10_485_760); - assert_eq!(v2.validation_upgrade_cooldown, 3600); - assert_eq!(v2.max_pov_size, 5_242_880); - assert_eq!(v2.hrmp_channel_max_message_size, 102_400); - assert_eq!(v2.dispute_max_spam_slots, 2); - assert_eq!(v2.n_delay_tranches, 89); - assert_eq!(v2.ump_max_individual_weight, OldWeight(20_000_000_000)); - assert_eq!(v2.minimum_validation_upgrade_delay, 20); + assert_eq!(v3.max_code_size, 10_485_760); + assert_eq!(v3.validation_upgrade_cooldown, 3600); + assert_eq!(v3.max_pov_size, 5_242_880); + assert_eq!(v3.hrmp_channel_max_message_size, 102_400); + assert_eq!(v3.n_delay_tranches, 89); + assert_eq!(v3.ump_max_individual_weight, Weight::from_parts(20_000_000_000, 5_242_880)); + assert_eq!(v3.minimum_validation_upgrade_delay, 20); } #[test] @@ -264,8 +276,8 @@ mod tests { // We specify only the picked fields and the rest should be provided by the `Default` // implementation. That implementation is copied over between the two types and should work // fine. - let v2 = v3::OldHostConfiguration:: { - ump_max_individual_weight: OldWeight(0x71616e6f6e0au64), + let v3 = v4::OldHostConfiguration:: { + ump_max_individual_weight: Weight::from_parts(0x71616e6f6e0au64, 0x71616e6f6e0au64), needed_approvals: 69, thread_availability_period: 55, hrmp_recipient_deposit: 1337, @@ -279,61 +291,58 @@ mod tests { // Implant the v2 version in the state. frame_support::storage::unhashed::put_raw( &configuration::ActiveConfig::::hashed_key(), - &v2.encode(), + &v3.encode(), ); - migrate_to_v3::(); + migrate_to_v4::(); - let v3 = configuration::ActiveConfig::::get(); + let v4 = configuration::ActiveConfig::::get(); #[rustfmt::skip] { - assert_eq!(v2.max_code_size , v3.max_code_size); - assert_eq!(v2.max_head_data_size , v3.max_head_data_size); - assert_eq!(v2.max_upward_queue_count , v3.max_upward_queue_count); - assert_eq!(v2.max_upward_queue_size , v3.max_upward_queue_size); - assert_eq!(v2.max_upward_message_size , v3.max_upward_message_size); - assert_eq!(v2.max_upward_message_num_per_candidate , v3.max_upward_message_num_per_candidate); - assert_eq!(v2.hrmp_max_message_num_per_candidate , v3.hrmp_max_message_num_per_candidate); - assert_eq!(v2.validation_upgrade_cooldown , v3.validation_upgrade_cooldown); - assert_eq!(v2.validation_upgrade_delay , v3.validation_upgrade_delay); - assert_eq!(v2.max_pov_size , v3.max_pov_size); - assert_eq!(v2.max_downward_message_size , v3.max_downward_message_size); - assert_eq!(v2.hrmp_max_parachain_outbound_channels , v3.hrmp_max_parachain_outbound_channels); - assert_eq!(v2.hrmp_max_parathread_outbound_channels , v3.hrmp_max_parathread_outbound_channels); - assert_eq!(v2.hrmp_sender_deposit , v3.hrmp_sender_deposit); - assert_eq!(v2.hrmp_recipient_deposit , v3.hrmp_recipient_deposit); - assert_eq!(v2.hrmp_channel_max_capacity , v3.hrmp_channel_max_capacity); - assert_eq!(v2.hrmp_channel_max_total_size , v3.hrmp_channel_max_total_size); - assert_eq!(v2.hrmp_max_parachain_inbound_channels , v3.hrmp_max_parachain_inbound_channels); - assert_eq!(v2.hrmp_max_parathread_inbound_channels , v3.hrmp_max_parathread_inbound_channels); - assert_eq!(v2.hrmp_channel_max_message_size , v3.hrmp_channel_max_message_size); - assert_eq!(v2.code_retention_period , v3.code_retention_period); - assert_eq!(v2.parathread_cores , v3.parathread_cores); - assert_eq!(v2.parathread_retries , v3.parathread_retries); - assert_eq!(v2.group_rotation_frequency , v3.group_rotation_frequency); - assert_eq!(v2.chain_availability_period , v3.chain_availability_period); - assert_eq!(v2.thread_availability_period , v3.thread_availability_period); - assert_eq!(v2.scheduling_lookahead , v3.scheduling_lookahead); - assert_eq!(v2.max_validators_per_core , v3.max_validators_per_core); - assert_eq!(v2.max_validators , v3.max_validators); - assert_eq!(v2.dispute_period , v3.dispute_period); - assert_eq!(v2.dispute_post_conclusion_acceptance_period, v3.dispute_post_conclusion_acceptance_period); - assert_eq!(v2.dispute_max_spam_slots , v3.dispute_max_spam_slots); - assert_eq!(v2.dispute_conclusion_by_time_out_period , v3.dispute_conclusion_by_time_out_period); - assert_eq!(v2.no_show_slots , v3.no_show_slots); - assert_eq!(v2.n_delay_tranches , v3.n_delay_tranches); - assert_eq!(v2.zeroth_delay_tranche_width , v3.zeroth_delay_tranche_width); - assert_eq!(v2.needed_approvals , v3.needed_approvals); - assert_eq!(v2.relay_vrf_modulo_samples , v3.relay_vrf_modulo_samples); - assert_eq!(v2.pvf_checking_enabled , v3.pvf_checking_enabled); - assert_eq!(v2.pvf_voting_ttl , v3.pvf_voting_ttl); - assert_eq!(v2.minimum_validation_upgrade_delay , v3.minimum_validation_upgrade_delay); + assert_eq!(v3.max_code_size , v4.max_code_size); + assert_eq!(v3.max_head_data_size , v4.max_head_data_size); + assert_eq!(v3.max_upward_queue_count , v4.max_upward_queue_count); + assert_eq!(v3.max_upward_queue_size , v4.max_upward_queue_size); + assert_eq!(v3.max_upward_message_size , v4.max_upward_message_size); + assert_eq!(v3.max_upward_message_num_per_candidate , v4.max_upward_message_num_per_candidate); + assert_eq!(v3.hrmp_max_message_num_per_candidate , v4.hrmp_max_message_num_per_candidate); + assert_eq!(v3.validation_upgrade_cooldown , v4.validation_upgrade_cooldown); + assert_eq!(v3.validation_upgrade_delay , v4.validation_upgrade_delay); + assert_eq!(v3.max_pov_size , v4.max_pov_size); + assert_eq!(v3.max_downward_message_size , v4.max_downward_message_size); + assert_eq!(v3.ump_service_total_weight , v4.ump_service_total_weight); + assert_eq!(v3.hrmp_max_parachain_outbound_channels , v4.hrmp_max_parachain_outbound_channels); + assert_eq!(v3.hrmp_max_parathread_outbound_channels , v4.hrmp_max_parathread_outbound_channels); + assert_eq!(v3.hrmp_sender_deposit , v4.hrmp_sender_deposit); + assert_eq!(v3.hrmp_recipient_deposit , v4.hrmp_recipient_deposit); + assert_eq!(v3.hrmp_channel_max_capacity , v4.hrmp_channel_max_capacity); + assert_eq!(v3.hrmp_channel_max_total_size , v4.hrmp_channel_max_total_size); + assert_eq!(v3.hrmp_max_parachain_inbound_channels , v4.hrmp_max_parachain_inbound_channels); + assert_eq!(v3.hrmp_max_parathread_inbound_channels , v4.hrmp_max_parathread_inbound_channels); + assert_eq!(v3.hrmp_channel_max_message_size , v4.hrmp_channel_max_message_size); + assert_eq!(v3.code_retention_period , v4.code_retention_period); + assert_eq!(v3.parathread_cores , v4.parathread_cores); + assert_eq!(v3.parathread_retries , v4.parathread_retries); + assert_eq!(v3.group_rotation_frequency , v4.group_rotation_frequency); + assert_eq!(v3.chain_availability_period , v4.chain_availability_period); + assert_eq!(v3.thread_availability_period , v4.thread_availability_period); + assert_eq!(v3.scheduling_lookahead , v4.scheduling_lookahead); + assert_eq!(v3.max_validators_per_core , v4.max_validators_per_core); + assert_eq!(v3.max_validators , v4.max_validators); + assert_eq!(v3.dispute_period , v4.dispute_period); + assert_eq!(v3.dispute_post_conclusion_acceptance_period, v4.dispute_post_conclusion_acceptance_period); + assert_eq!(v3.dispute_conclusion_by_time_out_period , v4.dispute_conclusion_by_time_out_period); + assert_eq!(v3.no_show_slots , v4.no_show_slots); + assert_eq!(v3.n_delay_tranches , v4.n_delay_tranches); + assert_eq!(v3.zeroth_delay_tranche_width , v4.zeroth_delay_tranche_width); + assert_eq!(v3.needed_approvals , v4.needed_approvals); + assert_eq!(v3.relay_vrf_modulo_samples , v4.relay_vrf_modulo_samples); + assert_eq!(v3.ump_max_individual_weight , v4.ump_max_individual_weight); + assert_eq!(v3.pvf_checking_enabled , v4.pvf_checking_enabled); + assert_eq!(v3.pvf_voting_ttl , v4.pvf_voting_ttl); + assert_eq!(v3.minimum_validation_upgrade_delay , v4.minimum_validation_upgrade_delay); - assert_eq!(v2.ump_service_total_weight, OldWeight(v3.ump_service_total_weight.ref_time())); - assert_eq!(v2.ump_max_individual_weight, OldWeight(v3.ump_max_individual_weight.ref_time())); - assert_eq!(v3.ump_service_total_weight.proof_size(), MAX_POV_SIZE as u64); - assert_eq!(v3.ump_max_individual_weight.proof_size(), MAX_POV_SIZE as u64); }; // ; makes this a statement. `rustfmt::skip` cannot be put on an expression. }); } diff --git a/runtime/parachains/src/configuration/tests.rs b/runtime/parachains/src/configuration/tests.rs index 30f9d5e5ac5e..d9d0ae6ea267 100644 --- a/runtime/parachains/src/configuration/tests.rs +++ b/runtime/parachains/src/configuration/tests.rs @@ -309,7 +309,6 @@ fn setting_pending_config_members() { max_validators: None, dispute_period: 239, dispute_post_conclusion_acceptance_period: 10, - dispute_max_spam_slots: 2, dispute_conclusion_by_time_out_period: 512, no_show_slots: 240, n_delay_tranches: 241, From f04fc04fcfb78d54bddecbf62d8070209dde76e8 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Dec 2022 15:33:19 +0200 Subject: [PATCH 24/44] Deprecate `SpamSlots` --- runtime/parachains/src/disputes.rs | 3 +++ runtime/parachains/src/disputes/migration.rs | 2 ++ 2 files changed, 5 insertions(+) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 0f44fb9b75e9..466b42b522b8 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -474,6 +474,9 @@ pub mod pallet { /// fewer than `byzantine_threshold + 1` validators. /// /// The i'th entry of the vector corresponds to the i'th validator in the session. + #[deprecated( + note = "SpamSlots is no longer used and will be removed. Check https://github.com/paritytech/polkadot/pull/6345 for details" + )] #[pallet::storage] pub(super) type SpamSlots = StorageMap<_, Twox64Concat, SessionIndex, Vec>; diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index e18ab0b4e5cb..3f3638d11d40 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -16,6 +16,7 @@ //! Storage migration(s) related to disputes pallet +#[allow(deprecated)] use crate::disputes::{pallet::SpamSlots, Config, Pallet}; use frame_support::{ pallet_prelude::*, @@ -44,6 +45,7 @@ pub fn migrate_to_v1() -> Weight { // SpamSlots should not contain too many keys so removing everything at once should be safe loop { + #[allow(deprecated)] let res = SpamSlots::::clear(0, None); // `loops` is the number of iterations => used to calculate read weights // `backend` is the number of keys removed from the backend => used to calculate write weights From d4b8b5b52a472e9761f67eac08dbb35e01462163 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 19 Dec 2022 10:33:55 +0200 Subject: [PATCH 25/44] Code review feedback * add weight for storage version update * fix bound for clear() --- runtime/parachains/src/disputes/migration.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index 3f3638d11d40..7180edc7b252 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -34,6 +34,7 @@ impl OnRuntimeUpgrade for MigrateToV1 { if StorageVersion::get::>() < STORAGE_VERSION { weight += migrate_to_v1::(); STORAGE_VERSION.put::>(); + weight.saturating_add(T::DbWeight::get().reads_writes(1, 1)); } weight } @@ -46,7 +47,7 @@ pub fn migrate_to_v1() -> Weight { // SpamSlots should not contain too many keys so removing everything at once should be safe loop { #[allow(deprecated)] - let res = SpamSlots::::clear(0, None); + let res = SpamSlots::::clear(u32::MAX, None); // `loops` is the number of iterations => used to calculate read weights // `backend` is the number of keys removed from the backend => used to calculate write weights weight From ac7c5c81b500d233b7c94ef5c5638394d5c1f615 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 20 Dec 2022 13:53:46 +0200 Subject: [PATCH 26/44] Fix weights in disputes migration --- runtime/parachains/src/disputes/migration.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index 7180edc7b252..d4367462f9da 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -34,7 +34,7 @@ impl OnRuntimeUpgrade for MigrateToV1 { if StorageVersion::get::>() < STORAGE_VERSION { weight += migrate_to_v1::(); STORAGE_VERSION.put::>(); - weight.saturating_add(T::DbWeight::get().reads_writes(1, 1)); + weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1)); } weight } @@ -42,7 +42,7 @@ impl OnRuntimeUpgrade for MigrateToV1 { /// Migrates the pallet storage to the most recent version, checking and setting the `StorageVersion`. pub fn migrate_to_v1() -> Weight { - let weight: Weight = Weight::zero(); + let mut weight: Weight = Weight::zero(); // SpamSlots should not contain too many keys so removing everything at once should be safe loop { @@ -50,7 +50,7 @@ pub fn migrate_to_v1() -> Weight { let res = SpamSlots::::clear(u32::MAX, None); // `loops` is the number of iterations => used to calculate read weights // `backend` is the number of keys removed from the backend => used to calculate write weights - weight + weight = weight .saturating_add(T::DbWeight::get().reads_writes(res.loops as u64, res.backend as u64)); if res.maybe_cursor.is_none() { From 1905b6fe676f5d6a78f9d9d261d95f69253d1f91 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 21 Dec 2022 18:10:51 +0200 Subject: [PATCH 27/44] Revert "Deprecate `SpamSlots`" This reverts commit 8c4d967c7b061abd76ba8b551223918c0b9e6370. --- runtime/parachains/src/disputes.rs | 3 --- runtime/parachains/src/disputes/migration.rs | 2 -- 2 files changed, 5 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 466b42b522b8..0f44fb9b75e9 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -474,9 +474,6 @@ pub mod pallet { /// fewer than `byzantine_threshold + 1` validators. /// /// The i'th entry of the vector corresponds to the i'th validator in the session. - #[deprecated( - note = "SpamSlots is no longer used and will be removed. Check https://github.com/paritytech/polkadot/pull/6345 for details" - )] #[pallet::storage] pub(super) type SpamSlots = StorageMap<_, Twox64Concat, SessionIndex, Vec>; diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index d4367462f9da..6c9048f6f43f 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -16,7 +16,6 @@ //! Storage migration(s) related to disputes pallet -#[allow(deprecated)] use crate::disputes::{pallet::SpamSlots, Config, Pallet}; use frame_support::{ pallet_prelude::*, @@ -46,7 +45,6 @@ pub fn migrate_to_v1() -> Weight { // SpamSlots should not contain too many keys so removing everything at once should be safe loop { - #[allow(deprecated)] let res = SpamSlots::::clear(u32::MAX, None); // `loops` is the number of iterations => used to calculate read weights // `backend` is the number of keys removed from the backend => used to calculate write weights From 746a5893bc0b3513dc071dd73a6244bef8b487fe Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 21 Dec 2022 18:16:48 +0200 Subject: [PATCH 28/44] Make mod migration public --- runtime/parachains/src/disputes.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 0f44fb9b75e9..81ab9ab00317 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -46,7 +46,9 @@ mod tests; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; -mod migration; +pub mod migration; + +const LOG_TARGET: &str = "runtime::disputes"; /// Whether the dispute is local or remote. #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)] From 647d409cc932db9ffb86dab2e12e2b379e0707b5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 21 Dec 2022 22:18:06 +0200 Subject: [PATCH 29/44] Remove `SpamSlots` from disputes pallet and use `storage_alias` in the migration --- runtime/parachains/src/disputes.rs | 8 --- runtime/parachains/src/disputes/migration.rs | 75 ++++++++++++-------- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 81ab9ab00317..56815c63c83a 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -471,14 +471,6 @@ pub mod pallet { T::BlockNumber, >; - /// Maps session indices to a vector indicating the number of potentially-spam disputes - /// each validator is participating in. Potentially-spam disputes are remote disputes which have - /// fewer than `byzantine_threshold + 1` validators. - /// - /// The i'th entry of the vector corresponds to the i'th validator in the session. - #[pallet::storage] - pub(super) type SpamSlots = StorageMap<_, Twox64Concat, SessionIndex, Vec>; - /// Whether the chain is frozen. Starts as `None`. When this is `Some`, /// the chain will not accept any new parachain blocks for backing or inclusion, /// and its value indicates the last valid block number in the chain. diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index 6c9048f6f43f..9d8dcd867102 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -16,45 +16,60 @@ //! Storage migration(s) related to disputes pallet -use crate::disputes::{pallet::SpamSlots, Config, Pallet}; -use frame_support::{ - pallet_prelude::*, - traits::{OnRuntimeUpgrade, StorageVersion}, - weights::Weight, -}; +use frame_support::traits::StorageVersion; + /// The current storage version. const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); -pub struct MigrateToV1(sp_std::marker::PhantomData); -impl OnRuntimeUpgrade for MigrateToV1 { - fn on_runtime_upgrade() -> Weight { - let mut weight: Weight = Weight::zero(); +pub mod v1 { + use super::*; + use crate::disputes::{Config, Pallet}; + use frame_support::{ + pallet_prelude::*, storage_alias, traits::OnRuntimeUpgrade, weights::Weight, + }; + use primitives::v2::SessionIndex; + use sp_std::prelude::*; + + #[storage_alias] + type SpamSlots = StorageMap, Twox64Concat, SessionIndex, Vec>; - if StorageVersion::get::>() < STORAGE_VERSION { - weight += migrate_to_v1::(); - STORAGE_VERSION.put::>(); - weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1)); + pub struct MigrateToV1(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV1 { + fn on_runtime_upgrade() -> Weight { + log::info!(target: crate::disputes::LOG_TARGET, "Migrating disputes storage"); + let mut weight: Weight = Weight::zero(); + + if StorageVersion::get::>() < STORAGE_VERSION { + log::info!( + target: crate::disputes::LOG_TARGET, + "Will migrate disputes storage!!!!" + ); + weight += migrate_to_v1::(); + STORAGE_VERSION.put::>(); + weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1)); + } + weight } - weight } -} -/// Migrates the pallet storage to the most recent version, checking and setting the `StorageVersion`. -pub fn migrate_to_v1() -> Weight { - let mut weight: Weight = Weight::zero(); + /// Migrates the pallet storage to the most recent version, checking and setting the `StorageVersion`. + pub fn migrate_to_v1() -> Weight { + let mut weight: Weight = Weight::zero(); - // SpamSlots should not contain too many keys so removing everything at once should be safe - loop { - let res = SpamSlots::::clear(u32::MAX, None); - // `loops` is the number of iterations => used to calculate read weights - // `backend` is the number of keys removed from the backend => used to calculate write weights - weight = weight - .saturating_add(T::DbWeight::get().reads_writes(res.loops as u64, res.backend as u64)); + // SpamSlots should not contain too many keys so removing everything at once should be safe + loop { + let res = SpamSlots::::clear(u32::MAX, None); + // `loops` is the number of iterations => used to calculate read weights + // `backend` is the number of keys removed from the backend => used to calculate write weights + weight = weight.saturating_add( + T::DbWeight::get().reads_writes(res.loops as u64, res.backend as u64), + ); - if res.maybe_cursor.is_none() { - break + if res.maybe_cursor.is_none() { + break + } } - } - weight + weight + } } From 68f956cc848cd65736af4f4f0f8c12b1438e3d87 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 21 Dec 2022 22:24:48 +0200 Subject: [PATCH 30/44] Fix call to `clear()` for `SpamSlots` in migration --- runtime/parachains/src/disputes/migration.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index 9d8dcd867102..10b1665bfe67 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -57,18 +57,11 @@ pub mod v1 { let mut weight: Weight = Weight::zero(); // SpamSlots should not contain too many keys so removing everything at once should be safe - loop { - let res = SpamSlots::::clear(u32::MAX, None); - // `loops` is the number of iterations => used to calculate read weights - // `backend` is the number of keys removed from the backend => used to calculate write weights - weight = weight.saturating_add( - T::DbWeight::get().reads_writes(res.loops as u64, res.backend as u64), - ); - - if res.maybe_cursor.is_none() { - break - } - } + let res = SpamSlots::::clear(u32::MAX, None); + // `loops` is the number of iterations => used to calculate read weights + // `backend` is the number of keys removed from the backend => used to calculate write weights + weight = weight + .saturating_add(T::DbWeight::get().reads_writes(res.loops as u64, res.backend as u64)); weight } From df8204921804995f785dfddb07a843d7f152720f Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Sat, 24 Dec 2022 15:18:50 +0200 Subject: [PATCH 31/44] Update migration and add a `try-runtime` test --- runtime/parachains/src/disputes/migration.rs | 24 ++++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index 10b1665bfe67..c641c5fa4386 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -36,20 +36,34 @@ pub mod v1 { pub struct MigrateToV1(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV1 { fn on_runtime_upgrade() -> Weight { - log::info!(target: crate::disputes::LOG_TARGET, "Migrating disputes storage"); let mut weight: Weight = Weight::zero(); if StorageVersion::get::>() < STORAGE_VERSION { - log::info!( - target: crate::disputes::LOG_TARGET, - "Will migrate disputes storage!!!!" - ); + log::info!(target: crate::disputes::LOG_TARGET, "Migrating disputes storage to v1"); weight += migrate_to_v1::(); STORAGE_VERSION.put::>(); weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1)); + } else { + log::info!( + target: crate::disputes::LOG_TARGET, + "Disputes storage up to date - no need for migration" + ); } weight } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + ensure!( + StorageVersion::get::>() == STORAGE_VERSION, + format!("Storage version should be {} after the migration", STORAGE_VERSION) + ); + ensure!( + SpamSlots::::iter().count() == 0, + "SpamSlots should be empty after the migration" + ); + Ok(()) + } } /// Migrates the pallet storage to the most recent version, checking and setting the `StorageVersion`. From 535482fb8220aca52a7671bfe07efa81a63164ba Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 2 Jan 2023 10:09:10 +0200 Subject: [PATCH 32/44] Add `pre_upgrade` `try-runtime` test --- runtime/parachains/src/disputes/migration.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index c641c5fa4386..3e32da4d334b 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -49,14 +49,24 @@ pub mod v1 { "Disputes storage up to date - no need for migration" ); } + weight } + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, &'static str> { + ensure!( + StorageVersion::get::>() < STORAGE_VERSION, + "Storage version should be less than `1` before the migration", + ); + Ok(Vec::new()) + } + #[cfg(feature = "try-runtime")] fn post_upgrade(_state: Vec) -> Result<(), &'static str> { ensure!( StorageVersion::get::>() == STORAGE_VERSION, - format!("Storage version should be {} after the migration", STORAGE_VERSION) + "Storage version should be `1` after the migration" ); ensure!( SpamSlots::::iter().count() == 0, From 49068a413a113b21e116e4023cf20626d26a5448 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 2 Jan 2023 12:33:00 +0200 Subject: [PATCH 33/44] Fix some test names in `HostConfiguration` migration --- runtime/parachains/src/configuration/migration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/configuration/migration.rs b/runtime/parachains/src/configuration/migration.rs index 19167f0533b7..62151b471d9f 100644 --- a/runtime/parachains/src/configuration/migration.rs +++ b/runtime/parachains/src/configuration/migration.rs @@ -231,7 +231,7 @@ mod tests { use crate::mock::{new_test_ext, Test}; #[test] - fn v2_deserialized_from_actual_data() { + fn v3_deserialized_from_actual_data() { // Example how to get new `raw_config`: // We'll obtain the raw_config hes for block // 15,772,152 (0xf89d3ab5312c5f70d396dc59612f0aa65806c798346f9db4b35278baed2e0e53) on Kusama. @@ -267,7 +267,7 @@ mod tests { } #[test] - fn test_migrate_to_v3() { + fn test_migrate_to_v4() { // Host configuration has lots of fields. However, in this migration we add only a couple of // fields. The most important part to check are a couple of the last fields. We also pick // extra fields to check arbitrarily, e.g. depending on their position (i.e. the middle) and From 09ed433af9a27b06df8cc6f00dbeff97986ac070 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 2 Jan 2023 12:14:43 +0200 Subject: [PATCH 34/44] Link spamslots migration in all runtimes --- runtime/kusama/src/lib.rs | 2 ++ runtime/polkadot/src/lib.rs | 2 ++ runtime/rococo/src/lib.rs | 2 ++ runtime/westend/src/lib.rs | 2 ++ 4 files changed, 8 insertions(+) diff --git a/runtime/kusama/src/lib.rs b/runtime/kusama/src/lib.rs index 125ce049b195..ca8505132020 100644 --- a/runtime/kusama/src/lib.rs +++ b/runtime/kusama/src/lib.rs @@ -1489,6 +1489,8 @@ pub type Migrations = ( Runtime, governance::FellowshipReferendaInstance, >, + parachains_disputes::migration::v1::MigrateToV1, + parachains_configuration::migration::v4::MigrateToV4, ); /// Unchecked extrinsic type as expected by this runtime. diff --git a/runtime/polkadot/src/lib.rs b/runtime/polkadot/src/lib.rs index 385b7b815fb8..eb82aaf0ed4f 100644 --- a/runtime/polkadot/src/lib.rs +++ b/runtime/polkadot/src/lib.rs @@ -1601,6 +1601,8 @@ impl Get<&'static str> for StakingMigrationV11OldPallet { pub type Migrations = ( pallet_balances::migration::MigrateToTrackInactive, crowdloan::migration::MigrateToTrackInactive, + parachains_disputes::migration::v1::MigrateToV1, + parachains_configuration::migration::v4::MigrateToV4, ); /// Unchecked extrinsic type as expected by this runtime. diff --git a/runtime/rococo/src/lib.rs b/runtime/rococo/src/lib.rs index bcf0a03c9456..d203bc0caaac 100644 --- a/runtime/rococo/src/lib.rs +++ b/runtime/rococo/src/lib.rs @@ -1480,6 +1480,8 @@ pub type UncheckedExtrinsic = pub type Migrations = ( pallet_balances::migration::MigrateToTrackInactive, crowdloan::migration::MigrateToTrackInactive, + parachains_disputes::migration::v1::MigrateToV1, + parachains_configuration::migration::v4::MigrateToV4, ); /// Executive: handles dispatch to the various modules. diff --git a/runtime/westend/src/lib.rs b/runtime/westend/src/lib.rs index 097dcfdd5897..b9838b175fab 100644 --- a/runtime/westend/src/lib.rs +++ b/runtime/westend/src/lib.rs @@ -1215,6 +1215,8 @@ impl Get<&'static str> for StakingMigrationV11OldPallet { pub type Migrations = ( pallet_balances::migration::MigrateToTrackInactive, crowdloan::migration::MigrateToTrackInactive, + parachains_disputes::migration::v1::MigrateToV1, + parachains_configuration::migration::v4::MigrateToV4, ); /// Unchecked extrinsic type as expected by this runtime. From c33d7775431c88ffe3d31bc2d3bb37510102d4e1 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 2 Jan 2023 15:06:09 +0200 Subject: [PATCH 35/44] Add `test_unconfirmed_disputes_cause_block_import_error` --- runtime/parachains/src/disputes/tests.rs | 40 +++++++++++++++++++----- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/runtime/parachains/src/disputes/tests.rs b/runtime/parachains/src/disputes/tests.rs index 86443b0ca591..d1c785223ff6 100644 --- a/runtime/parachains/src/disputes/tests.rs +++ b/runtime/parachains/src/disputes/tests.rs @@ -686,9 +686,13 @@ fn test_freeze_provided_against_supermajority_for_included() { }); } -#[test] -fn test_unconfirmed_are_ignored() { - new_test_ext(Default::default()).execute_with(|| { +mod unconfirmed_disputes { + use super::*; + use assert_matches::assert_matches; + use sp_runtime::ModuleError; + + // Shared initialization code between `test_unconfirmed_are_ignored` and `test_unconfirmed_disputes_cause_block_import_error` + fn generate_dispute_statement_set_and_run_to_block() -> DisputeStatementSet { // 7 validators needed for byzantine threshold of 2. let v0 = ::Pair::generate().0; let v1 = ::Pair::generate().0; @@ -736,7 +740,7 @@ fn test_unconfirmed_are_ignored() { let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1)); // v0 votes for 4, v1 votes against 4. - let stmts = vec![DisputeStatementSet { + DisputeStatementSet { candidate_hash: candidate_hash.clone(), session: 4, statements: vec![ @@ -765,13 +769,33 @@ fn test_unconfirmed_are_ignored() { ), ), ], - }]; + } + } + #[test] + fn test_unconfirmed_are_ignored() { + new_test_ext(Default::default()).execute_with(|| { + let stmts = vec![generate_dispute_statement_set_and_run_to_block()]; + let stmts = filter_dispute_set(stmts); + + // Not confirmed => should be filtered out + assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![],); + }); + } - let stmts = filter_dispute_set(stmts); + #[test] + fn test_unconfirmed_disputes_cause_block_import_error() { + new_test_ext(Default::default()).execute_with(|| { + + let stmts = generate_dispute_statement_set_and_run_to_block(); + let stmts = vec![CheckedDisputeStatementSet::unchecked_from_unchecked(stmts)]; + + assert_matches!( + Pallet::::process_checked_multi_dispute_data(stmts), + Err(DispatchError::Module(ModuleError{index: _, error: _, message})) => assert_eq!(message, Some("UnconfirmedDispute")) + ); - // Not confirmed => should be filtered out - assert_ok!(Pallet::::process_checked_multi_dispute_data(stmts), vec![],); }); + } } // tests for: From 577d4b3391dcc8630e228f9e037dbb3d130fb3c0 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 2 Jan 2023 16:51:22 +0200 Subject: [PATCH 36/44] Update guide - Remove `SpamSlots` related information from roadmap/implementers-guide/src/runtime/disputes.md - Add 'Disputes filtering' to Runtime section of the Implementor's guide --- .../src/runtime/disputes.md | 61 +++++++++++-------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/roadmap/implementers-guide/src/runtime/disputes.md b/roadmap/implementers-guide/src/runtime/disputes.md index 83ca86b36b04..1f1debee9cd4 100644 --- a/roadmap/implementers-guide/src/runtime/disputes.md +++ b/roadmap/implementers-guide/src/runtime/disputes.md @@ -37,12 +37,6 @@ Disputes: double_map (SessionIndex, CandidateHash) -> Option, // 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. Included: double_map (SessionIndex, CandidateHash) -> Option, -// Maps session indices to a vector indicating the number of potentially-spam disputes -// each validator is participating in. Potentially-spam disputes are remote disputes which have -// fewer than `byzantine_threshold + 1` validators. -// -// The i'th entry of the vector corresponds to the i'th validator in the session. -SpamSlots: map SessionIndex -> Option>, // Whether the chain is frozen or not. Starts as `None`. When this is `Some`, // the chain will not accept any new parachain blocks for backing or inclusion, // and its value indicates the last valid block number in the chain. @@ -55,50 +49,45 @@ Frozen: Option, ## Session Change 1. If the current session is not greater than `config.dispute_period + 1`, nothing to do here. -1. Set `pruning_target = current_session - config.dispute_period - 1`. We add the extra `1` because we want to keep things for `config.dispute_period` _full_ sessions. +1. Set `pruning_target = current_session - config.dispute_period - 1`. We add the extra `1` because we want to keep things for `config.dispute_period` _full_ sessions. The stuff at the end of the most recent session has been around for a little over 0 sessions, not a little over 1. 1. If `LastPrunedSession` is `None`, then set `LastPrunedSession` to `Some(pruning_target)` and return. -1. Otherwise, clear out all disputes, included candidates, and `SpamSlots` entries in the range `last_pruned..=pruning_target` and set `LastPrunedSession` to `Some(pruning_target)`. +2. Otherwise, clear out all disputes and included candidates entries in the range `last_pruned..=pruning_target` and set `LastPrunedSession` to `Some(pruning_target)`. ## Block Initialization -1. Iterate through all disputes. If any have not concluded and started more than `config.dispute_conclusion_by_timeout_period` blocks ago, set them to `Concluded` and mildly punish all validators associated, as they have failed to distribute available data. If the `Included` map does not contain the candidate and there are fewer than `byzantine_threshold + 1` participating validators, reduce `SpamSlots` for all participating validators. +1. Iterate through all disputes. If any have not concluded and started more than `config.dispute_conclusion_by_timeout_period` blocks ago, set them to `Concluded` and mildly punish all validators associated, as they have failed to distribute available data. ## 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 + ancient votes, duplicates and unconfirmed disputes. + 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. + 2. Return a list of all candidates who just had disputes initiated. * `provide_dispute_data(DisputeStatementSet) -> bool`: Provide data to an ongoing dispute or initiate a dispute. - 1. All statements must be issued under the correct session for the correct candidate. + 1. All statements must be issued under the correct session for the correct candidate. 1. `SessionInfo` is used to check statement signatures and this function should fail if any signatures are invalid. 1. If there is no dispute under `Disputes`, create a new `DisputeState` with blank bitfields. 1. If `concluded_at` is `Some`, and is `concluded_at + config.post_conclusion_acceptance_period < now`, return false. - 1. If the overlap of the validators in the `DisputeStatementSet` and those already present in the `DisputeState` is fewer in number than `byzantine_threshold + 1` and the candidate is not present in the `Included` map - 1. increment `SpamSlots` for each validator in the `DisputeStatementSet` which is not already in the `DisputeState`. Initialize the `SpamSlots` to a zeroed vector first, if necessary. do not increment `SpamSlots` if the candidate is local. - 1. If the value for any spam slot exceeds `config.dispute_max_spam_slots`, return false. - 1. If the overlap of the validators in the `DisputeStatementSet` and those already present in the `DisputeState` is at least `byzantine_threshold + 1`, the `DisputeState` has fewer than `byzantine_threshold + 1` validators, and the candidate is not present in the `Included` map, then decrease `SpamSlots` by 1 for each validator in the `DisputeState`. - 1. Import all statements into the dispute. This should fail if any statements are duplicate or if the corresponding bit for the corresponding validator is set in the dispute already. - 1. If `concluded_at` is `None`, reward all statements. - 1. If `concluded_at` is `Some`, reward all statements slightly less. - 1. If either side now has supermajority and did not previously, slash the other side. This may be both sides, and we support this possibility in code, but note that this requires validators to participate on both sides which has negative expected value. Set `concluded_at` to `Some(now)` if it was `None`. - 1. If just concluded against the candidate and the `Included` map contains `(session, candidate)`: invoke `revert_and_freeze` with the stored block number. - 1. Return true if just initiated, false otherwise. + 2. Import all statements into the dispute. This should fail if any statements are duplicate or if the corresponding bit for the corresponding validator is set in the dispute already. + 3. If `concluded_at` is `None`, reward all statements. + 4. If `concluded_at` is `Some`, reward all statements slightly less. + 5. If either side now has supermajority and did not previously, slash the other side. This may be both sides, and we support this possibility in code, but note that this requires validators to participate on both sides which has negative expected value. Set `concluded_at` to `Some(now)` if it was `None`. + 6. If just concluded against the candidate and the `Included` map contains `(session, candidate)`: invoke `revert_and_freeze` with the stored block number. + 7. Return true if just initiated, false otherwise. * `disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)>`: Get a list of all disputes and info about dispute state. 1. Iterate over all disputes in `Disputes` and collect into a vector. * `note_included(SessionIndex, CandidateHash, included_in: BlockNumber)`: 1. Add `(SessionIndex, CandidateHash)` to the `Included` map with `included_in - 1` as the value. - 1. If there is a dispute under `(Sessionindex, CandidateHash)` with fewer than `byzantine_threshold + 1` participating validators, decrease `SpamSlots` by 1 for each validator in the `DisputeState`. 1. If there is a dispute under `(SessionIndex, CandidateHash)` that has concluded against the candidate, invoke `revert_and_freeze` with the stored block number. * `concluded_invalid(SessionIndex, CandidateHash) -> bool`: Returns whether a candidate has already concluded a dispute in the negative. @@ -111,3 +100,27 @@ Frozen: Option, 1. If `is_frozen()` return. 1. Set `Frozen` to `Some(BlockNumber)` to indicate a rollback to the block number. 1. Issue a `Revert(BlockNumber + 1)` log to indicate a rollback of the block's child in the header chain, which is the same as a rollback to the block number. + +# Disputes filtering + +All disputes delivered to the runtime by the client are filtered before the actual import. In this context actual import +means persisted in the runtime storage. The filtering has got two purposes: +- Limit the amount of data saved onchain. +- Prevent persisting malicious dispute data onchain. + +*Implementation note*: Filtering is performed in function `filter_dispute_data` from `Disputes` pallet. + +The filtering is performed on the whole statement set which is about to be imported onchain. The following filters are +applied: +1. Remove ancient disputes - if a dispute is concluded before the block number indicated in `OLDEST_ACCEPTED` parameter + it is removed from the set. `OLDEST_ACCEPTED` is a runtime configuration option. + *Implementation note*: `dispute_post_conclusion_acceptance_period` from + `HostConfiguration` is used in the current Polkadot/Kusama implementation. +2. Remove votes from unknown validators. If there is a vote from a validator which wasn't an authority in the session + where the dispute was raised - they are removed. Please note that this step removes only single votes instead of + removing the whole dispute. +3. Remove one sided disputes - if a dispute doesn't contain two opposing votes it is not imported onchain. This serves + as a measure not to import one sided disputes. A dispute is raised only if there are two opposing votes so if the + client is not sending them the dispute is a potential spam. +4. Remove unconfirmed disputes - if a dispute contains less votes than the byzantine threshold it is removed. This is + also a spam precaution. A legitimate client will send only confirmed disputes to the runtime. \ No newline at end of file From 50fdc937d1bee0d8f8331dca829b9708e639701d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 4 Jan 2023 09:39:54 +0200 Subject: [PATCH 37/44] Update runtime/parachains/src/configuration/migration.rs Co-authored-by: Marcin S. --- runtime/parachains/src/configuration/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/configuration/migration.rs b/runtime/parachains/src/configuration/migration.rs index 62151b471d9f..c4bfe552f942 100644 --- a/runtime/parachains/src/configuration/migration.rs +++ b/runtime/parachains/src/configuration/migration.rs @@ -288,7 +288,7 @@ mod tests { }; new_test_ext(Default::default()).execute_with(|| { - // Implant the v2 version in the state. + // Implant the v3 version in the state. frame_support::storage::unhashed::put_raw( &configuration::ActiveConfig::::hashed_key(), &v3.encode(), From 0a567eb06864b2c681ede9ffb0bb6a9dcd9dce4e Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 4 Jan 2023 09:42:02 +0200 Subject: [PATCH 38/44] Code review feedback - update logs --- runtime/parachains/src/configuration/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/configuration/migration.rs b/runtime/parachains/src/configuration/migration.rs index c4bfe552f942..ba17ad2f2938 100644 --- a/runtime/parachains/src/configuration/migration.rs +++ b/runtime/parachains/src/configuration/migration.rs @@ -218,7 +218,7 @@ minimum_validation_upgrade_delay : pre.minimum_validation_upgrade_delay, // to be unlikely to be caused by this. So we just log. Maybe it'll work out still? log::error!( target: configuration::LOG_TARGET, - "unexpected error when performing translation of the configuration type during storage upgrade to v2." + "unexpected error when performing translation of the configuration type during storage upgrade to v4." ); } From ab5933b1dbe133f4fac68f2d34d653c2f0f31d32 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 4 Jan 2023 09:58:05 +0200 Subject: [PATCH 39/44] Code review feedback: fix weights --- runtime/parachains/src/disputes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 56815c63c83a..d25e12240e59 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -782,7 +782,7 @@ impl Pallet { dispute.concluded_at = Some(now); >::insert(session_index, candidate_hash, &dispute); - weight += T::DbWeight::get().reads_writes(1, 1); + weight += T::DbWeight::get().reads_writes(0, 1); } } From 1989026441326aa62dc67be05b0ae3a0e296583d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 4 Jan 2023 11:29:21 +0200 Subject: [PATCH 40/44] Update runtime/parachains/src/disputes.rs Co-authored-by: s0me0ne-unkn0wn <48632512+s0me0ne-unkn0wn@users.noreply.github.com> --- runtime/parachains/src/disputes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index d25e12240e59..ca4cc2dda236 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -782,7 +782,7 @@ impl Pallet { dispute.concluded_at = Some(now); >::insert(session_index, candidate_hash, &dispute); - weight += T::DbWeight::get().reads_writes(0, 1); + weight += T::DbWeight::get().writes(1); } } From 06f53bb87edf375adcd58690468ed7253005e82d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 4 Jan 2023 18:15:06 +0200 Subject: [PATCH 41/44] Additional logs in disputes migration --- runtime/parachains/src/disputes/migration.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index 3e32da4d334b..a10e3251dbbf 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -55,6 +55,11 @@ pub mod v1 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, &'static str> { + log::info!( + target: crate::disputes::LOG_TARGET, + "SpamSlots before migration: {}", + SpamSlots::::iter().count() + ); ensure!( StorageVersion::get::>() < STORAGE_VERSION, "Storage version should be less than `1` before the migration", From 91075ddea7f4b3f4e00fcf4bb614f127836b680e Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 4 Jan 2023 18:21:06 +0200 Subject: [PATCH 42/44] Fix merge conflicts --- runtime/westend/src/lib.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/runtime/westend/src/lib.rs b/runtime/westend/src/lib.rs index 08163838fae8..b10f76a2700d 100644 --- a/runtime/westend/src/lib.rs +++ b/runtime/westend/src/lib.rs @@ -1217,13 +1217,10 @@ impl Get<&'static str> for StakingMigrationV11OldPallet { pub type Migrations = ( pallet_balances::migration::MigrateToTrackInactive, crowdloan::migration::MigrateToTrackInactive, -<<<<<<< HEAD - parachains_disputes::migration::v1::MigrateToV1, - parachains_configuration::migration::v4::MigrateToV4, -======= pallet_scheduler::migration::v4::CleanupAgendas, pallet_staking::migrations::v13::MigrateToV13, ->>>>>>> master + parachains_disputes::migration::v1::MigrateToV1, + parachains_configuration::migration::v4::MigrateToV4, ); /// Unchecked extrinsic type as expected by this runtime. From 8831582511e14e4653cac2a186d9be26c6f688d7 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 5 Jan 2023 15:42:05 +0200 Subject: [PATCH 43/44] Add version checks in try-runtime tests --- .../parachains/src/configuration/migration.rs | 20 +++++++++++++++++++ runtime/parachains/src/disputes/migration.rs | 5 +++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/configuration/migration.rs b/runtime/parachains/src/configuration/migration.rs index ba17ad2f2938..f811d461692b 100644 --- a/runtime/parachains/src/configuration/migration.rs +++ b/runtime/parachains/src/configuration/migration.rs @@ -32,6 +32,7 @@ pub mod v4 { use super::*; use frame_support::{traits::OnRuntimeUpgrade, weights::constants::WEIGHT_REF_TIME_PER_MILLIS}; use primitives::v2::{Balance, SessionIndex}; + use sp_std::prelude::*; // Copied over from configuration.rs @ de9e147695b9f1be8bd44e07861a31e483c8343a and removed // all the comments, and changed the Weight struct to OldWeight @@ -137,6 +138,14 @@ pub mod v4 { pub struct MigrateToV4(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV4 { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, &'static str> { + log::trace!(target: crate::configuration::LOG_TARGET, "Running pre_upgrade()"); + + ensure!(StorageVersion::get::>() == 3, "The migration requires version 3"); + Ok(Vec::new()) + } + fn on_runtime_upgrade() -> Weight { if StorageVersion::get::>() == 3 { let weight_consumed = migrate_to_v4::(); @@ -150,6 +159,17 @@ pub mod v4 { T::DbWeight::get().reads(1) } } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + log::trace!(target: crate::configuration::LOG_TARGET, "Running post_upgrade()"); + ensure!( + StorageVersion::get::>() == STORAGE_VERSION, + "Storage version should be 4 after the migration" + ); + + Ok(()) + } } } diff --git a/runtime/parachains/src/disputes/migration.rs b/runtime/parachains/src/disputes/migration.rs index a10e3251dbbf..584d4b33872e 100644 --- a/runtime/parachains/src/disputes/migration.rs +++ b/runtime/parachains/src/disputes/migration.rs @@ -55,13 +55,13 @@ pub mod v1 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, &'static str> { - log::info!( + log::trace!( target: crate::disputes::LOG_TARGET, "SpamSlots before migration: {}", SpamSlots::::iter().count() ); ensure!( - StorageVersion::get::>() < STORAGE_VERSION, + StorageVersion::get::>() == 0, "Storage version should be less than `1` before the migration", ); Ok(Vec::new()) @@ -69,6 +69,7 @@ pub mod v1 { #[cfg(feature = "try-runtime")] fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + log::trace!(target: crate::disputes::LOG_TARGET, "Running post_upgrade()"); ensure!( StorageVersion::get::>() == STORAGE_VERSION, "Storage version should be `1` after the migration" From b11ce8bd4a141eb16e661cb6f64a29c4c6e2efb9 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 5 Jan 2023 16:05:56 +0200 Subject: [PATCH 44/44] Fix a compilation warning` --- runtime/parachains/src/configuration/migration.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/parachains/src/configuration/migration.rs b/runtime/parachains/src/configuration/migration.rs index f811d461692b..14c38bf3550b 100644 --- a/runtime/parachains/src/configuration/migration.rs +++ b/runtime/parachains/src/configuration/migration.rs @@ -32,6 +32,7 @@ pub mod v4 { use super::*; use frame_support::{traits::OnRuntimeUpgrade, weights::constants::WEIGHT_REF_TIME_PER_MILLIS}; use primitives::v2::{Balance, SessionIndex}; + #[cfg(feature = "try-runtime")] use sp_std::prelude::*; // Copied over from configuration.rs @ de9e147695b9f1be8bd44e07861a31e483c8343a and removed