From a5d3051a321208083c1be759dc90abf4b9d092ad Mon Sep 17 00:00:00 2001 From: ordian Date: Fri, 12 Apr 2024 10:28:29 +0200 Subject: [PATCH 01/26] cosmetics --- .../runtime/parachains/src/paras_inherent/tests.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 64fbc9c4a4e0..b1f663287469 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -121,10 +121,10 @@ mod enter { } } - #[test] // Validate that if we create 2 backed candidates which are assigned to 2 cores that will be // freed via becoming fully available, the backed candidates will not be filtered out in // `create_inherent` and will not cause `enter` to early. + #[test] fn include_backed_candidates() { let config = MockGenesisConfig::default(); assert!(config.configuration.config.scheduler_params.lookahead > 0); @@ -582,8 +582,8 @@ mod enter { }); } - #[test] // Ensure that disputes are filtered out if the session is in the future. + #[test] fn filter_multi_dispute_data() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { // Create the inherent data for this block @@ -653,9 +653,9 @@ mod enter { }); } - #[test] // Ensure that when dispute data establishes an over weight block that we adequately // filter out disputes according to our prioritization rule + #[test] fn limit_dispute_data() { sp_tracing::try_init_simple(); new_test_ext(MockGenesisConfig::default()).execute_with(|| { @@ -721,10 +721,10 @@ mod enter { }); } - #[test] // Ensure that when a block is over weight due to disputes, but there is still sufficient // block weight to include a number of signed bitfields, the inherent data is filtered // as expected + #[test] fn limit_dispute_data_ignore_backed_candidates() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { // Create the inherent data for this block @@ -804,9 +804,9 @@ mod enter { }); } - #[test] // Ensure an overweight block with an excess amount of disputes and bitfields, the bitfields are // filtered to accommodate the block size and no backed candidates are included. + #[test] fn limit_bitfields_some() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { // Create the inherent data for this block @@ -892,8 +892,8 @@ mod enter { }); } - #[test] // Ensure that when a block is over weight due to disputes and bitfields, we filter. + #[test] fn limit_bitfields_overweight() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { // Create the inherent data for this block From b1c0830537c0f4835e915cc5b5aaeb7c1007ed94 Mon Sep 17 00:00:00 2001 From: ordian Date: Fri, 12 Apr 2024 15:19:07 +0200 Subject: [PATCH 02/26] commit a crime --- polkadot/parachain/src/primitives.rs | 2 +- polkadot/primitives/src/v7/mod.rs | 2 +- polkadot/runtime/parachains/src/disputes.rs | 2 +- .../runtime/parachains/src/disputes/tests.rs | 22 +++ .../runtime/parachains/src/inclusion/mod.rs | 10 ++ .../parachains/src/paras_inherent/tests.rs | 147 ++++++++++++++++-- polkadot/runtime/parachains/src/scheduler.rs | 5 + 7 files changed, 173 insertions(+), 17 deletions(-) diff --git a/polkadot/parachain/src/primitives.rs b/polkadot/parachain/src/primitives.rs index 5a1efdf89821..a4e81043ddba 100644 --- a/polkadot/parachain/src/primitives.rs +++ b/polkadot/parachain/src/primitives.rs @@ -41,7 +41,7 @@ pub use polkadot_core_primitives::BlockNumber as RelayChainBlockNumber; Ord, Encode, Decode, - RuntimeDebug, + Debug, derive_more::From, TypeInfo, Serialize, diff --git a/polkadot/primitives/src/v7/mod.rs b/polkadot/primitives/src/v7/mod.rs index 8a059408496c..15f4fb2a2e8f 100644 --- a/polkadot/primitives/src/v7/mod.rs +++ b/polkadot/primitives/src/v7/mod.rs @@ -626,7 +626,7 @@ impl Ord for CommittedCandidateReceipt { /// The `PersistedValidationData` should be relatively lightweight primarily because it is /// constructed during inclusion for each candidate and therefore lies on the critical path of /// inclusion. -#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, RuntimeDebug)] +#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, Debug)] #[cfg_attr(feature = "std", derive(Default))] pub struct PersistedValidationData { /// The parent head-data. diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index 62e02e67157d..92fe0baaffeb 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -40,7 +40,7 @@ use sp_std::{cmp::Ordering, collections::btree_set::BTreeSet, prelude::*}; #[cfg(test)] #[allow(unused_imports)] -pub(crate) use self::tests::run_to_block; +pub(crate) use self::tests::{make_dispute_concluding_against, run_to_block}; pub mod slashing; #[cfg(test)] diff --git a/polkadot/runtime/parachains/src/disputes/tests.rs b/polkadot/runtime/parachains/src/disputes/tests.rs index 16b4fa3a9f1a..f7cb63f347e8 100644 --- a/polkadot/runtime/parachains/src/disputes/tests.rs +++ b/polkadot/runtime/parachains/src/disputes/tests.rs @@ -29,6 +29,7 @@ use frame_support::{ traits::{OnFinalize, OnInitialize}, }; use frame_system::pallet_prelude::BlockNumberFor; +use keyring::Sr25519Keyring; use primitives::BlockNumber; use sp_core::{crypto::CryptoType, Pair}; @@ -86,6 +87,27 @@ type NewSession<'a> = ( Option>, ); +pub(crate) fn make_dispute_concluding_against( + candidate_hash: CandidateHash, + session: SessionIndex, + validators: &[(ValidatorIndex, Sr25519Keyring)], +) -> DisputeStatementSet { + let mut statements = Vec::new(); + for (i, key) in validators.iter() { + // make the first statement backing + // and the rest against it + let statement = if i.0 == 0 { + DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded(candidate_hash.0)) + } else { + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) + }; + let payload_data = statement.payload_data(candidate_hash, session).unwrap(); + let signature = key.sign(&payload_data); + statements.push((statement, *i, signature.into())); + } + DisputeStatementSet { candidate_hash, session, statements } +} + // Run to specific block, while calling disputes pallet hooks manually, because disputes is not // integrated in initializer yet. pub(crate) fn run_to_block<'a>( diff --git a/polkadot/runtime/parachains/src/inclusion/mod.rs b/polkadot/runtime/parachains/src/inclusion/mod.rs index 31befefa3220..794a25af4dd4 100644 --- a/polkadot/runtime/parachains/src/inclusion/mod.rs +++ b/polkadot/runtime/parachains/src/inclusion/mod.rs @@ -1235,6 +1235,11 @@ impl CandidateCheckContext { let relay_parent = backed_candidate_receipt.descriptor().relay_parent; // Check that the relay-parent is one of the allowed relay-parents. + log::debug!( + target: LOG_TARGET, + "Checking allowed relay parent with the prev context {:?}", + self.prev_context, + ); let (relay_parent_storage_root, relay_parent_number) = { match allowed_relay_parents.acquire_info(relay_parent, self.prev_context) { None => return Err(Error::::DisallowedRelayParent), @@ -1249,6 +1254,11 @@ impl CandidateCheckContext { parent_head_data, ); + log::debug!( + target: LOG_TARGET, + "Persisted validation data {:?}", + persisted_validation_data, + ); let expected = persisted_validation_data.hash(); ensure!( diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index b1f663287469..1136f568de58 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -61,18 +61,18 @@ mod enter { use sp_runtime::Perbill; use sp_std::collections::btree_map::BTreeMap; - struct TestConfig { - dispute_statements: BTreeMap, - dispute_sessions: Vec, - backed_and_concluding: BTreeMap, - num_validators_per_core: u32, - code_upgrade: Option, - fill_claimqueue: bool, - elastic_paras: BTreeMap, - unavailable_cores: Vec, + pub struct TestConfig { + pub dispute_statements: BTreeMap, + pub dispute_sessions: Vec, + pub backed_and_concluding: BTreeMap, + pub num_validators_per_core: u32, + pub code_upgrade: Option, + pub fill_claimqueue: bool, + pub elastic_paras: BTreeMap, + pub unavailable_cores: Vec, } - fn make_inherent_data( + pub fn make_inherent_data( TestConfig { dispute_statements, dispute_sessions, @@ -1741,10 +1741,11 @@ mod sanitizers { mod candidates { use crate::{ mock::{set_disabled_validators, RuntimeOrigin}, - scheduler::{common::Assignment, ParasEntry}, + scheduler::{common::Assignment, CoreOccupiedType, ParasEntry}, util::{make_persisted_validation_data, make_persisted_validation_data_with_parent}, }; use primitives::ValidationCode; + use sp_runtime::Digest; use sp_std::collections::vec_deque::VecDeque; use super::*; @@ -1755,6 +1756,7 @@ mod sanitizers { expected_backed_candidates_with_core: BTreeMap>, scheduled_paras: BTreeMap>, + validators: Vec, } // Generate test data for the candidates and assert that the environment is set as expected @@ -1768,7 +1770,7 @@ mod sanitizers { default_header().hash(), Default::default(), RELAY_PARENT_NUM, - 1, + 10, ); let header = default_header(); @@ -1817,20 +1819,26 @@ mod sanitizers { vec![ValidatorIndex(2), ValidatorIndex(3)], ]); + // Set the availability of the cores + scheduler::Pallet::::set_availability_cores(vec![ + CoreOccupiedType::::Free, + CoreOccupiedType::::Free, + ]); + // Update scheduler's claimqueue with the parachains scheduler::Pallet::::set_claimqueue(BTreeMap::from([ ( CoreIndex::from(0), VecDeque::from([ParasEntry::new( Assignment::Pool { para_id: 1.into(), core_index: CoreIndex(0) }, - RELAY_PARENT_NUM, + RELAY_PARENT_NUM + 10, )]), ), ( CoreIndex::from(1), VecDeque::from([ParasEntry::new( Assignment::Pool { para_id: 2.into(), core_index: CoreIndex(1) }, - RELAY_PARENT_NUM, + RELAY_PARENT_NUM + 10, )]), ), ])); @@ -1931,6 +1939,7 @@ mod sanitizers { backed_candidates, scheduled_paras: scheduled, expected_backed_candidates_with_core, + validators, } } @@ -2474,6 +2483,7 @@ mod sanitizers { backed_candidates, scheduled_paras: scheduled, expected_backed_candidates_with_core, + validators, } } @@ -2977,6 +2987,7 @@ mod sanitizers { backed_candidates, scheduled_paras: scheduled, expected_backed_candidates_with_core, + validators, } } @@ -2989,6 +3000,7 @@ mod sanitizers { backed_candidates, expected_backed_candidates_with_core, scheduled_paras: scheduled, + .. } = get_test_data_one_core_per_para(core_index_enabled); assert_eq!( @@ -3013,6 +3025,7 @@ mod sanitizers { backed_candidates, expected_backed_candidates_with_core, scheduled_paras: scheduled, + .. } = get_test_data_multiple_cores_per_para(core_index_enabled); assert_eq!( @@ -3037,6 +3050,7 @@ mod sanitizers { backed_candidates, scheduled_paras: scheduled, expected_backed_candidates_with_core, + .. } = get_test_data_for_order_checks(core_index_enabled); assert_eq!( @@ -3198,6 +3212,111 @@ mod sanitizers { }); } + #[test] + fn concluded_invalid_in_the_past_are_filtered_out() { + use crate::disputes::{make_dispute_concluding_against, run_to_block}; + + new_test_ext(default_config()).execute_with(|| { + let mut hc = configuration::ActiveConfig::::get(); + hc.minimum_backing_votes = 1; + hc.scheduler_params.group_rotation_frequency = 20; + hc.async_backing_params.allowed_ancestry_len = 10; + // hc.max_pov_size = 5_000_000; + configuration::Pallet::::force_set_active_config(hc); + + // run_to_block(parent_block_number, |_| None); + let TestData { backed_candidates, validators, .. } = + get_test_data_one_core_per_para(false); + + let session = SessionIndex::from(0_u32); + let mut validators: Vec<_> = validators + .into_iter() + .enumerate() + .map(|(i, v)| (ValidatorIndex::from(i as u32), v)) + .collect(); + let groups = scheduler::ValidatorGroups::::get(); + let para_id_to_group: BTreeMap> = groups + .into_iter() + .enumerate() + .map(|(i, g)| (ParaId::from(i as u32 + 1), g)) + .collect(); + + let disputes: MultiDisputeStatementSet = backed_candidates + .iter() + .map(|c| { + // make sure the backer is the first in the list + let para_id = c.descriptor().para_id; + let group = para_id_to_group.get(¶_id).unwrap(); + let backer = validators.iter().position(|v| group.contains(&v.0)).unwrap(); + validators.swap(0, backer); + + make_dispute_concluding_against(c.hash(), session, &validators) + }) + .collect(); + + let parent_block_number = 4; + let parent_header = HeaderFor::::new( + parent_block_number, // `block_number`, + Default::default(), // `extrinsics_root`, + Default::default(), // `storage_root`, + default_header().hash(), // `parent_hash`, + Default::default(), // digest, + ); + + // run_to_block(parent_block_number + 1, |_| None); + frame_system::Pallet::::reset_events(); + frame_system::Pallet::::initialize( + &(parent_header.number() + 1), + &parent_header.hash(), + &Digest { logs: Vec::new() }, + ); + + let data = ParachainsInherentData { + disputes: Vec::new(), + parent_header: parent_header.clone(), + bitfields: Vec::new(), + backed_candidates, + }; + let mut inherent_data = InherentData::new(); + inherent_data.put_data(PARACHAINS_INHERENT_IDENTIFIER, &data).unwrap(); + + let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); + + let parent_block_number = 5; + run_to_block(parent_block_number, |_| None); + let parent_header = HeaderFor::::new( + parent_block_number, // `block_number`, + Default::default(), // `extrinsics_root`, + Default::default(), // `storage_root`, + parent_header.hash(), // `parent_hash`, + Default::default(), // digest, + ); + + frame_system::Pallet::::reset_events(); + frame_system::Pallet::::initialize( + &(parent_header.number() + 1), + &parent_header.hash(), + &Digest { logs: Vec::new() }, + ); + + // let data = ParachainsInherentData { + // disputes: Vec::new(), + // parent_header, + // bitfields: Vec::new(), + // backed_candidates, + // }; + + // let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), + // data).unwrap(); + assert_eq!( + // The length of this vec is equal to the number of candidates, so we know + // all of our candidates got filtered out + OnChainVotes::::get().unwrap().backing_validators_per_candidate.len(), + 0, + ); + }); + } + #[rstest] #[case(false)] #[case(true)] diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index baeec49839df..699272319fa7 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -679,4 +679,9 @@ impl Pallet { pub(crate) fn set_claimqueue(claimqueue: BTreeMap>>) { ClaimQueue::::set(claimqueue); } + + #[cfg(test)] + pub(crate) fn set_availability_cores(cores: Vec>) { + AvailabilityCores::::set(cores); + } } From edf6ebfa18a53ce927e6eeab02deec621956d610 Mon Sep 17 00:00:00 2001 From: ordian Date: Fri, 10 May 2024 13:49:50 +0200 Subject: [PATCH 03/26] make it fail in the right way --- .../parachains/src/paras_inherent/tests.rs | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 1136f568de58..5efa7019a79f 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -3221,10 +3221,8 @@ mod sanitizers { hc.minimum_backing_votes = 1; hc.scheduler_params.group_rotation_frequency = 20; hc.async_backing_params.allowed_ancestry_len = 10; - // hc.max_pov_size = 5_000_000; configuration::Pallet::::force_set_active_config(hc); - // run_to_block(parent_block_number, |_| None); let TestData { backed_candidates, validators, .. } = get_test_data_one_core_per_para(false); @@ -3254,7 +3252,7 @@ mod sanitizers { }) .collect(); - let parent_block_number = 4; + let parent_block_number = 3; let parent_header = HeaderFor::::new( parent_block_number, // `block_number`, Default::default(), // `extrinsics_root`, @@ -3263,7 +3261,7 @@ mod sanitizers { Default::default(), // digest, ); - // run_to_block(parent_block_number + 1, |_| None); + run_to_block(parent_block_number + 1, |_| None); frame_system::Pallet::::reset_events(); frame_system::Pallet::::initialize( &(parent_header.number() + 1), @@ -3272,10 +3270,10 @@ mod sanitizers { ); let data = ParachainsInherentData { - disputes: Vec::new(), + disputes, parent_header: parent_header.clone(), bitfields: Vec::new(), - backed_candidates, + backed_candidates: Vec::new(), }; let mut inherent_data = InherentData::new(); inherent_data.put_data(PARACHAINS_INHERENT_IDENTIFIER, &data).unwrap(); @@ -3299,15 +3297,14 @@ mod sanitizers { &Digest { logs: Vec::new() }, ); - // let data = ParachainsInherentData { - // disputes: Vec::new(), - // parent_header, - // bitfields: Vec::new(), - // backed_candidates, - // }; + let data = ParachainsInherentData { + disputes: Vec::new(), + parent_header, + bitfields: Vec::new(), + backed_candidates, + }; - // let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), - // data).unwrap(); + let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); assert_eq!( // The length of this vec is equal to the number of candidates, so we know // all of our candidates got filtered out From 5899108147099c08f434cf441b63bc6a6acf66da Mon Sep 17 00:00:00 2001 From: ordian Date: Fri, 10 May 2024 13:52:24 +0200 Subject: [PATCH 04/26] revert debug logs --- polkadot/parachain/src/primitives.rs | 2 +- polkadot/primitives/src/v7/mod.rs | 2 +- polkadot/runtime/parachains/src/inclusion/mod.rs | 10 ---------- 3 files changed, 2 insertions(+), 12 deletions(-) diff --git a/polkadot/parachain/src/primitives.rs b/polkadot/parachain/src/primitives.rs index a4e81043ddba..5a1efdf89821 100644 --- a/polkadot/parachain/src/primitives.rs +++ b/polkadot/parachain/src/primitives.rs @@ -41,7 +41,7 @@ pub use polkadot_core_primitives::BlockNumber as RelayChainBlockNumber; Ord, Encode, Decode, - Debug, + RuntimeDebug, derive_more::From, TypeInfo, Serialize, diff --git a/polkadot/primitives/src/v7/mod.rs b/polkadot/primitives/src/v7/mod.rs index 15f4fb2a2e8f..8a059408496c 100644 --- a/polkadot/primitives/src/v7/mod.rs +++ b/polkadot/primitives/src/v7/mod.rs @@ -626,7 +626,7 @@ impl Ord for CommittedCandidateReceipt { /// The `PersistedValidationData` should be relatively lightweight primarily because it is /// constructed during inclusion for each candidate and therefore lies on the critical path of /// inclusion. -#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, Debug)] +#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, RuntimeDebug)] #[cfg_attr(feature = "std", derive(Default))] pub struct PersistedValidationData { /// The parent head-data. diff --git a/polkadot/runtime/parachains/src/inclusion/mod.rs b/polkadot/runtime/parachains/src/inclusion/mod.rs index 794a25af4dd4..31befefa3220 100644 --- a/polkadot/runtime/parachains/src/inclusion/mod.rs +++ b/polkadot/runtime/parachains/src/inclusion/mod.rs @@ -1235,11 +1235,6 @@ impl CandidateCheckContext { let relay_parent = backed_candidate_receipt.descriptor().relay_parent; // Check that the relay-parent is one of the allowed relay-parents. - log::debug!( - target: LOG_TARGET, - "Checking allowed relay parent with the prev context {:?}", - self.prev_context, - ); let (relay_parent_storage_root, relay_parent_number) = { match allowed_relay_parents.acquire_info(relay_parent, self.prev_context) { None => return Err(Error::::DisallowedRelayParent), @@ -1254,11 +1249,6 @@ impl CandidateCheckContext { parent_head_data, ); - log::debug!( - target: LOG_TARGET, - "Persisted validation data {:?}", - persisted_validation_data, - ); let expected = persisted_validation_data.hash(); ensure!( From c66c4f47b8412c41f2ecd411ee03dd3c03593bfd Mon Sep 17 00:00:00 2001 From: ordian Date: Thu, 30 May 2024 18:01:20 +0200 Subject: [PATCH 05/26] fix the test again --- polkadot/runtime/parachains/src/builder.rs | 2 +- polkadot/runtime/parachains/src/disputes.rs | 39 ++++++-- .../runtime/parachains/src/disputes/tests.rs | 4 +- .../parachains/src/paras_inherent/mod.rs | 28 ++---- .../parachains/src/paras_inherent/tests.rs | 93 ++++++++++++++++--- polkadot/runtime/parachains/src/shared.rs | 7 +- 6 files changed, 132 insertions(+), 41 deletions(-) diff --git a/polkadot/runtime/parachains/src/builder.rs b/polkadot/runtime/parachains/src/builder.rs index d1e2bc392feb..25ad28168bd0 100644 --- a/polkadot/runtime/parachains/src/builder.rs +++ b/polkadot/runtime/parachains/src/builder.rs @@ -53,7 +53,7 @@ fn mock_validation_code() -> ValidationCode { /// /// This is directly from frame-benchmarking. Copy/pasted so we can use it when not compiling with /// "features = runtime-benchmarks". -fn account(name: &'static str, index: u32, seed: u32) -> AccountId { +pub fn account(name: &'static str, index: u32, seed: u32) -> AccountId { let entropy = (name, index, seed).using_encoded(sp_io::hashing::blake2_256); AccountId::decode(&mut TrailingZeroInput::new(&entropy[..])) .expect("infinite input; no invalid input; qed") diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index 92fe0baaffeb..7acfc0d9c088 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -229,6 +229,9 @@ pub trait DisputesHandler { included_in: BlockNumber, ); + /// Get a list of disputes in a session that concluded invalid. + fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet; + /// Retrieve the included state of a given candidate in a particular session. If it /// returns `Some`, then we have a local dispute for the given `candidate_hash`. fn included_state(session: SessionIndex, candidate_hash: CandidateHash) -> Option; @@ -271,6 +274,10 @@ impl DisputesHandler for () { Ok(Vec::new()) } + fn disputes_concluded_invalid(_session: SessionIndex) -> BTreeSet { + BTreeSet::new() + } + fn note_included( _session: SessionIndex, _candidate_hash: CandidateHash, @@ -320,6 +327,10 @@ where pallet::Pallet::::process_checked_multi_dispute_data(statement_sets) } + fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet { + pallet::Pallet::::disputes_concluded_invalid(session) + } + fn note_included( session: SessionIndex, candidate_hash: CandidateHash, @@ -555,7 +566,7 @@ enum VoteImportError { MaliciousBacker, } -#[derive(RuntimeDebug, Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] enum VoteKind { /// A backing vote that is counted as "for" vote in dispute resolution. Backing, @@ -691,6 +702,7 @@ impl DisputeStateImporter { let mut undo = ImportUndo { validator_index: validator, vote_kind: kind, new_participant: false }; + log::debug!(target: LOG_TARGET, "Setting dispute bit: {}", validator.0); bits.set(validator.0 as usize, true); if kind.is_backing() { let is_new = self.backers.insert(validator); @@ -948,7 +960,10 @@ impl Pallet { // Load session info to access validators let session_info = match session_info::Sessions::::get(set.session) { Some(s) => s, - None => return StatementSetFilter::RemoveAll, + None => { + log::debug!(target: LOG_TARGET, "SessionInfo not found: {}", set.session); + return StatementSetFilter::RemoveAll + }, }; let config = configuration::ActiveConfig::::get(); @@ -959,6 +974,7 @@ impl Pallet { let dispute_state = { if let Some(dispute_state) = Disputes::::get(&set.session, &set.candidate_hash) { if dispute_state.concluded_at.as_ref().map_or(false, |c| c < &oldest_accepted) { + log::debug!(target: LOG_TARGET, "Dispute is ancient: {:?}", set.candidate_hash); return StatementSetFilter::RemoveAll } @@ -976,6 +992,7 @@ impl Pallet { let backers = BackersOnDisputes::::get(&set.session, &set.candidate_hash).unwrap_or_default(); + log::debug!(target: LOG_TARGET, "Dispute import with {} backers", backers.len()); // Check and import all votes. let summary = { @@ -986,17 +1003,20 @@ impl Pallet { let validator_public = match session_info.validators.get(*validator_index) { None => { filter.remove_index(i); + log::debug!(target: LOG_TARGET, "Invalid validator index in a dispute: {}", validator_index.0); continue }, Some(v) => v, }; let kind = VoteKind::from(statement); + log::debug!(target: LOG_TARGET, "Importing {:?} vote from {}", kind, validator_index.0); let undo = match importer.import(*validator_index, kind) { Ok(u) => u, Err(_) => { filter.remove_index(i); + log::debug!(target: LOG_TARGET, "Import of a dispute failed for validator: {}", validator_index.0); continue }, }; @@ -1022,7 +1042,7 @@ impl Pallet { // config. config.approval_voting_params.max_approval_coalesce_count > 1, ) { - log::warn!("Failed to check dispute signature"); + log::warn!(target: LOG_TARGET, "Failed to check dispute signature"); importer.undo(undo); filter.remove_index(i); @@ -1037,6 +1057,7 @@ impl Pallet { if summary.state.validators_for.count_ones() == 0 || summary.state.validators_against.count_ones() == 0 { + log::debug!(target: LOG_TARGET, "Rejected one-sided dispute"); return StatementSetFilter::RemoveAll } @@ -1044,6 +1065,7 @@ impl Pallet { if (summary.state.validators_for.clone() | &summary.state.validators_against).count_ones() <= byzantine_threshold(summary.state.validators_for.len()) { + log::debug!(target: LOG_TARGET, "Rejected unconfirmed dispute"); return StatementSetFilter::RemoveAll } @@ -1215,12 +1237,13 @@ impl Pallet { let revert_to = included_in - One::one(); Included::::insert(&session, &candidate_hash, revert_to); + } - if let Some(state) = Disputes::::get(&session, candidate_hash) { - if has_supermajority_against(&state) { - Self::revert_and_freeze(revert_to); - } - } + pub(crate) fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet { + Disputes::::iter_prefix(session) + .filter(|(_hash, state)| has_supermajority_against(state)) + .map(|(hash, _state)| hash) + .collect() } pub(crate) fn included_state( diff --git a/polkadot/runtime/parachains/src/disputes/tests.rs b/polkadot/runtime/parachains/src/disputes/tests.rs index f7cb63f347e8..5e4e7036bfef 100644 --- a/polkadot/runtime/parachains/src/disputes/tests.rs +++ b/polkadot/runtime/parachains/src/disputes/tests.rs @@ -93,10 +93,10 @@ pub(crate) fn make_dispute_concluding_against( validators: &[(ValidatorIndex, Sr25519Keyring)], ) -> DisputeStatementSet { let mut statements = Vec::new(); - for (i, key) in validators.iter() { + for (j, (i, key)) in validators.iter().enumerate() { // make the first statement backing // and the rest against it - let statement = if i.0 == 0 { + let statement = if j == 0 { DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded(candidate_hash.0)) } else { DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index ac4cf5dc8d41..aa13fd6bf079 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -402,12 +402,14 @@ impl Pallet { // Limit the disputes first, since the following statements depend on the votes include // here. + log::debug!(target: LOG_TARGET, "Disputes before sanitization: {}", disputes.len()); let (checked_disputes_sets, checked_disputes_sets_consumed_weight) = limit_and_sanitize_disputes::( disputes, dispute_statement_set_valid, max_block_weight, ); + log::debug!(target: LOG_TARGET, "Disputes after sanitization: {}", checked_disputes_sets.len()); let all_weight_after = if context == ProcessInherentDataContext::ProvideInherent { // Assure the maximum block weight is adhered, by limiting bitfields and backed @@ -456,13 +458,11 @@ impl Pallet { // Note that `process_checked_multi_dispute_data` will iterate and import each // dispute; so the input here must be reasonably bounded, // which is guaranteed by the checks and weight limitation above. - // We don't care about fresh or not disputes - // this writes them to storage, so let's query it via those means - // if this fails for whatever reason, that's ok. if let Err(e) = T::DisputesHandler::process_checked_multi_dispute_data(&checked_disputes_sets) { log::warn!(target: LOG_TARGET, "MultiDisputesData failed to update: {:?}", e); + // return Err(e.into()) }; METRICS.on_disputes_imported(checked_disputes_sets.len() as u64); @@ -490,26 +490,15 @@ impl Pallet { // Contains the disputes that are concluded in the current session only, // since these are the only ones that are relevant for the occupied cores // and lightens the load on `free_disputed` significantly. - // Cores can't be occupied with candidates of the previous sessions, and only - // things with new votes can have just concluded. We only need to collect - // cores with disputes that conclude just now, because disputes that - // concluded longer ago have already had any corresponding cores cleaned up. - let current_concluded_invalid_disputes = checked_disputes_sets - .iter() - .map(AsRef::as_ref) - .filter(|dss| dss.session == current_session) - .map(|dss| (dss.session, dss.candidate_hash)) - .filter(|(session, candidate)| { - ::DisputesHandler::concluded_invalid(*session, *candidate) - }) - .map(|(_session, candidate)| candidate) - .collect::>(); + let mut current_concluded_invalid_disputes = + ::DisputesHandler::disputes_concluded_invalid(current_session); // Get the cores freed as a result of concluded invalid candidates. let (freed_disputed, concluded_invalid_hashes): (Vec, BTreeSet) = inclusion::Pallet::::free_disputed(¤t_concluded_invalid_disputes) .into_iter() .unzip(); + current_concluded_invalid_disputes.extend(concluded_invalid_hashes); // Create a bit index from the set of core indices where each index corresponds to // a core index that was freed due to a dispute. @@ -582,7 +571,7 @@ impl Pallet { let backed_candidates_with_core = sanitize_backed_candidates::( backed_candidates, &allowed_relay_parents, - concluded_invalid_hashes, + current_concluded_invalid_disputes, scheduled, core_index_enabled, ); @@ -1079,8 +1068,11 @@ fn limit_and_sanitize_disputes< if max_consumable_weight.all_gte(updated) { // Always apply the weight. Invalid data cost processing time too: weight_acc = updated; + let candidate_hash = dss.candidate_hash; if let Some(checked) = dispute_statement_set_valid(dss) { checked_acc.push(checked); + } else { + log::debug!(target: LOG_TARGET, "Filtered dispute: {:?}", candidate_hash); } } }); diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 5efa7019a79f..c71aa531c3bf 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1739,6 +1739,8 @@ mod sanitizers { } mod candidates { + use std::collections::HashMap; + use crate::{ mock::{set_disabled_validators, RuntimeOrigin}, scheduler::{common::Assignment, CoreOccupiedType, ParasEntry}, @@ -1766,15 +1768,15 @@ mod sanitizers { // Add the relay parent to `shared` pallet. Otherwise some code (e.g. filtering backing // votes) won't behave correctly + let header = default_header(); + let relay_parent = header.hash(); shared::Pallet::::add_allowed_relay_parent( - default_header().hash(), - Default::default(), + relay_parent, + *header.state_root(), RELAY_PARENT_NUM, 10, ); - let header = default_header(); - let relay_parent = header.hash(); let session_index = SessionIndex::from(0_u32); let keystore = LocalKeystore::in_memory(); @@ -3214,41 +3216,72 @@ mod sanitizers { #[test] fn concluded_invalid_in_the_past_are_filtered_out() { - use crate::disputes::{make_dispute_concluding_against, run_to_block}; + use crate::{ + builder::account, + disputes::{make_dispute_concluding_against, run_to_block}, + mock::AccountId, + }; + use assert_matches::assert_matches; new_test_ext(default_config()).execute_with(|| { let mut hc = configuration::ActiveConfig::::get(); hc.minimum_backing_votes = 1; hc.scheduler_params.group_rotation_frequency = 20; hc.async_backing_params.allowed_ancestry_len = 10; + hc.async_backing_params.max_candidate_depth = 10; configuration::Pallet::::force_set_active_config(hc); let TestData { backed_candidates, validators, .. } = get_test_data_one_core_per_para(false); let session = SessionIndex::from(0_u32); - let mut validators: Vec<_> = validators + let validators: Vec<_> = validators .into_iter() .enumerate() .map(|(i, v)| (ValidatorIndex::from(i as u32), v)) .collect(); + let validators_keyrings: HashMap = + validators.into_iter().map(|(_, v)| (v.public().into(), v)).collect(); + let validators_keys: Vec<_> = shared::ActiveValidatorKeys::::get() + .into_iter() + .enumerate() + .map(|(i, v)| { + let acc: AccountId = account("validator", i as u32, i as u32); + (acc, v) + }) + .collect(); + initializer::Pallet::::test_trigger_on_new_session( + true, + session, + validators_keys.iter().map(|(a, v)| (a, v.clone())), + None, + ); + let validators_keys = shared::ActiveValidatorKeys::::get(); + let mut validators: Vec<_> = validators_keys + .iter() + .enumerate() + .map(|(i, v)| (ValidatorIndex::from(i as u32), v)) + .collect(); let groups = scheduler::ValidatorGroups::::get(); let para_id_to_group: BTreeMap> = groups .into_iter() .enumerate() .map(|(i, g)| (ParaId::from(i as u32 + 1), g)) .collect(); - let disputes: MultiDisputeStatementSet = backed_candidates .iter() .map(|c| { // make sure the backer is the first in the list let para_id = c.descriptor().para_id; let group = para_id_to_group.get(¶_id).unwrap(); - let backer = validators.iter().position(|v| group.contains(&v.0)).unwrap(); - validators.swap(0, backer); - - make_dispute_concluding_against(c.hash(), session, &validators) + let backer = validators.iter().find(|v| group.contains(&v.0)).unwrap().0; + validators.swap(0, backer.0 as _); + let keyrings: Vec<_> = validators + .iter() + .map(|(i, v)| (i.clone(), validators_keyrings[v].clone())) + .collect(); + + make_dispute_concluding_against(c.hash(), session, &keyrings) }) .collect(); @@ -3269,6 +3302,7 @@ mod sanitizers { &Digest { logs: Vec::new() }, ); + let n_disputes = disputes.len(); let data = ParachainsInherentData { disputes, parent_header: parent_header.clone(), @@ -3278,7 +3312,25 @@ mod sanitizers { let mut inherent_data = InherentData::new(); inherent_data.put_data(PARACHAINS_INHERENT_IDENTIFIER, &data).unwrap(); + let header = default_header(); + shared::Pallet::::add_allowed_relay_parent( + header.hash(), + *header.state_root(), + 3, + 10, + ); + let allowed_parents = shared::AllowedRelayParents::::get(); + println!("before enter: {allowed_parents:?}"); let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); + let allowed_parents = shared::AllowedRelayParents::::get(); + println!("right after: {allowed_parents:?}"); + + assert_matches!(pallet::OnChainVotes::::get(), Some(ScrapedOnChainVotes { + disputes, + .. + } ) => { + assert_eq!(disputes.len(), n_disputes); + }); let parent_block_number = 5; run_to_block(parent_block_number, |_| None); @@ -3289,6 +3341,14 @@ mod sanitizers { parent_header.hash(), // `parent_hash`, Default::default(), // digest, ); + // let data = ParachainsInherentData { + // disputes: Vec::new(), + // parent_header: parent_header.clone(), + // bitfields: Vec::new(), + // backed_candidates: Vec::new(), + // }; + // let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), + // data).unwrap(); frame_system::Pallet::::reset_events(); frame_system::Pallet::::initialize( @@ -3304,6 +3364,17 @@ mod sanitizers { backed_candidates, }; + // shared::Pallet::::clear_allowed_relay_parents(); + // let header = default_header(); + // shared::Pallet::::add_allowed_relay_parent( + // header.hash(), + // *header.state_root(), + // 3, + // 10, + // ); + let allowed_parents = shared::AllowedRelayParents::::get(); + println!("{allowed_parents:?}"); + let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); assert_eq!( // The length of this vec is equal to the number of candidates, so we know diff --git a/polkadot/runtime/parachains/src/shared.rs b/polkadot/runtime/parachains/src/shared.rs index 319b22515889..d22c72253cac 100644 --- a/polkadot/runtime/parachains/src/shared.rs +++ b/polkadot/runtime/parachains/src/shared.rs @@ -44,7 +44,7 @@ pub(crate) const SESSION_DELAY: SessionIndex = 2; mod tests; /// Information about past relay-parents. -#[derive(Encode, Decode, Default, TypeInfo)] +#[derive(Encode, Decode, Default, TypeInfo, Debug)] pub struct AllowedRelayParentsTracker { // The past relay parents, paired with state roots, that are viable to build upon. // @@ -270,4 +270,9 @@ impl Pallet { tracker.update(relay_parent, state_root, number, max_ancestry_len) }) } + + #[cfg(test)] + pub(crate) fn clear_allowed_relay_parents() { + AllowedRelayParents::::kill(); + } } From 12961100873e83f9dcd951804b092883d2340ae6 Mon Sep 17 00:00:00 2001 From: ordian Date: Mon, 3 Jun 2024 15:34:10 +0200 Subject: [PATCH 06/26] cleanup --- polkadot/runtime/parachains/src/disputes.rs | 3 --- .../parachains/src/paras_inherent/mod.rs | 2 -- .../parachains/src/paras_inherent/tests.rs | 23 ------------------- polkadot/runtime/parachains/src/shared.rs | 7 +----- 4 files changed, 1 insertion(+), 34 deletions(-) diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index 7acfc0d9c088..353c2b0cf87f 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -702,7 +702,6 @@ impl DisputeStateImporter { let mut undo = ImportUndo { validator_index: validator, vote_kind: kind, new_participant: false }; - log::debug!(target: LOG_TARGET, "Setting dispute bit: {}", validator.0); bits.set(validator.0 as usize, true); if kind.is_backing() { let is_new = self.backers.insert(validator); @@ -992,7 +991,6 @@ impl Pallet { let backers = BackersOnDisputes::::get(&set.session, &set.candidate_hash).unwrap_or_default(); - log::debug!(target: LOG_TARGET, "Dispute import with {} backers", backers.len()); // Check and import all votes. let summary = { @@ -1010,7 +1008,6 @@ impl Pallet { }; let kind = VoteKind::from(statement); - log::debug!(target: LOG_TARGET, "Importing {:?} vote from {}", kind, validator_index.0); let undo = match importer.import(*validator_index, kind) { Ok(u) => u, diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index aa13fd6bf079..e5164f5b1f31 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -402,14 +402,12 @@ impl Pallet { // Limit the disputes first, since the following statements depend on the votes include // here. - log::debug!(target: LOG_TARGET, "Disputes before sanitization: {}", disputes.len()); let (checked_disputes_sets, checked_disputes_sets_consumed_weight) = limit_and_sanitize_disputes::( disputes, dispute_statement_set_valid, max_block_weight, ); - log::debug!(target: LOG_TARGET, "Disputes after sanitization: {}", checked_disputes_sets.len()); let all_weight_after = if context == ProcessInherentDataContext::ProvideInherent { // Assure the maximum block weight is adhered, by limiting bitfields and backed diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index c71aa531c3bf..078a6985f26c 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -3319,11 +3319,7 @@ mod sanitizers { 3, 10, ); - let allowed_parents = shared::AllowedRelayParents::::get(); - println!("before enter: {allowed_parents:?}"); let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); - let allowed_parents = shared::AllowedRelayParents::::get(); - println!("right after: {allowed_parents:?}"); assert_matches!(pallet::OnChainVotes::::get(), Some(ScrapedOnChainVotes { disputes, @@ -3341,14 +3337,6 @@ mod sanitizers { parent_header.hash(), // `parent_hash`, Default::default(), // digest, ); - // let data = ParachainsInherentData { - // disputes: Vec::new(), - // parent_header: parent_header.clone(), - // bitfields: Vec::new(), - // backed_candidates: Vec::new(), - // }; - // let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), - // data).unwrap(); frame_system::Pallet::::reset_events(); frame_system::Pallet::::initialize( @@ -3364,17 +3352,6 @@ mod sanitizers { backed_candidates, }; - // shared::Pallet::::clear_allowed_relay_parents(); - // let header = default_header(); - // shared::Pallet::::add_allowed_relay_parent( - // header.hash(), - // *header.state_root(), - // 3, - // 10, - // ); - let allowed_parents = shared::AllowedRelayParents::::get(); - println!("{allowed_parents:?}"); - let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); assert_eq!( // The length of this vec is equal to the number of candidates, so we know diff --git a/polkadot/runtime/parachains/src/shared.rs b/polkadot/runtime/parachains/src/shared.rs index d22c72253cac..319b22515889 100644 --- a/polkadot/runtime/parachains/src/shared.rs +++ b/polkadot/runtime/parachains/src/shared.rs @@ -44,7 +44,7 @@ pub(crate) const SESSION_DELAY: SessionIndex = 2; mod tests; /// Information about past relay-parents. -#[derive(Encode, Decode, Default, TypeInfo, Debug)] +#[derive(Encode, Decode, Default, TypeInfo)] pub struct AllowedRelayParentsTracker { // The past relay parents, paired with state roots, that are viable to build upon. // @@ -270,9 +270,4 @@ impl Pallet { tracker.update(relay_parent, state_root, number, max_ancestry_len) }) } - - #[cfg(test)] - pub(crate) fn clear_allowed_relay_parents() { - AllowedRelayParents::::kill(); - } } From f46c384e405ebc5833e7230f62cdf4597123fb2d Mon Sep 17 00:00:00 2001 From: ordian Date: Mon, 3 Jun 2024 18:21:56 +0200 Subject: [PATCH 07/26] finally make it fail the right way --- polkadot/runtime/parachains/src/disputes.rs | 6 + .../parachains/src/paras_inherent/mod.rs | 25 +++- .../parachains/src/paras_inherent/tests.rs | 131 +++++++++++++++--- 3 files changed, 141 insertions(+), 21 deletions(-) diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index 353c2b0cf87f..7c6dc1a158ef 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -1234,6 +1234,12 @@ impl Pallet { let revert_to = included_in - One::one(); Included::::insert(&session, &candidate_hash, revert_to); + + if let Some(state) = Disputes::::get(&session, candidate_hash) { + if has_supermajority_against(&state) { + Self::revert_and_freeze(revert_to); + } + } } pub(crate) fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet { diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index e5164f5b1f31..cfa37ddb3c44 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -487,16 +487,30 @@ impl Pallet { // Contains the disputes that are concluded in the current session only, // since these are the only ones that are relevant for the occupied cores - // and lightens the load on `free_disputed` significantly. - let mut current_concluded_invalid_disputes = - ::DisputesHandler::disputes_concluded_invalid(current_session); + // and lightens the load on + // Cores can't be occupied with candidates of the previous sessions, and only + // things with new votes can have just concluded. We only need to collect + // cores with disputes that conclude just now, because disputes that + // concluded longer ago have already had any corresponding cores cleaned up. + let current_concluded_invalid_disputes = checked_disputes_sets + .iter() + .map(AsRef::as_ref) + .filter(|dss| dss.session == current_session) + .map(|dss| (dss.session, dss.candidate_hash)) + .filter(|(session, candidate)| { + ::DisputesHandler::concluded_invalid(*session, *candidate) + }) + .map(|(_session, candidate)| candidate) + .collect::>(); + // let mut current_concluded_invalid_disputes = + // ::DisputesHandler::disputes_concluded_invalid(current_session); // Get the cores freed as a result of concluded invalid candidates. let (freed_disputed, concluded_invalid_hashes): (Vec, BTreeSet) = inclusion::Pallet::::free_disputed(¤t_concluded_invalid_disputes) .into_iter() .unzip(); - current_concluded_invalid_disputes.extend(concluded_invalid_hashes); + // current_concluded_invalid_disputes.extend(concluded_invalid_hashes); // Create a bit index from the set of core indices where each index corresponds to // a core index that was freed due to a dispute. @@ -569,7 +583,8 @@ impl Pallet { let backed_candidates_with_core = sanitize_backed_candidates::( backed_candidates, &allowed_relay_parents, - current_concluded_invalid_disputes, + concluded_invalid_hashes, + // current_concluded_invalid_disputes, scheduled, core_index_enabled, ); diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 078a6985f26c..51009e74c61a 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -3224,30 +3224,48 @@ mod sanitizers { use assert_matches::assert_matches; new_test_ext(default_config()).execute_with(|| { + const RELAY_PARENT_NUMBER: u32 = 1; + let mut hc = configuration::ActiveConfig::::get(); hc.minimum_backing_votes = 1; + // hc.max_validators = Some(5); hc.scheduler_params.group_rotation_frequency = 20; hc.async_backing_params.allowed_ancestry_len = 10; hc.async_backing_params.max_candidate_depth = 10; configuration::Pallet::::force_set_active_config(hc); - let TestData { backed_candidates, validators, .. } = - get_test_data_one_core_per_para(false); + let header = default_header(); + let relay_parent = header.hash(); + // let TestData { backed_candidates, validators, .. } = + // get_test_data_one_core_per_para(false); let session = SessionIndex::from(0_u32); - let validators: Vec<_> = validators - .into_iter() - .enumerate() - .map(|(i, v)| (ValidatorIndex::from(i as u32), v)) - .collect(); - let validators_keyrings: HashMap = - validators.into_iter().map(|(_, v)| (v.public().into(), v)).collect(); - let validators_keys: Vec<_> = shared::ActiveValidatorKeys::::get() - .into_iter() + let keystore = LocalKeystore::in_memory(); + let keystore = Arc::new(keystore) as KeystorePtr; + + let validators_keyrings = vec![ + keyring::Sr25519Keyring::Alice, + keyring::Sr25519Keyring::Bob, + keyring::Sr25519Keyring::Charlie, + keyring::Sr25519Keyring::Dave, + keyring::Sr25519Keyring::Eve, + ]; + for keyring in validators_keyrings.iter() { + Keystore::sr25519_generate_new( + &*keystore, + PARACHAIN_KEY_TYPE_ID, + Some(&keyring.to_seed()), + ) + .unwrap(); + } + let validators_to_keyrings: HashMap = + validators_keyrings.iter().map(|v| (v.public().into(), v.clone())).collect(); + let validators_keys: Vec<(_, ValidatorId)> = validators_keyrings + .iter() .enumerate() .map(|(i, v)| { let acc: AccountId = account("validator", i as u32, i as u32); - (acc, v) + (acc, v.public().into()) }) .collect(); initializer::Pallet::::test_trigger_on_new_session( @@ -3268,6 +3286,69 @@ mod sanitizers { .enumerate() .map(|(i, g)| (ParaId::from(i as u32 + 1), g)) .collect(); + let shuffled_indices = shared::ActiveValidatorIndices::::get(); + let validators_keyrings = + crate::util::take_active_subset(&shuffled_indices, &validators_keyrings); + + // Set the on-chain included head data for paras. + paras::Pallet::::set_current_head(ParaId::from(1), HeadData(vec![1])); + paras::Pallet::::set_current_head(ParaId::from(2), HeadData(vec![2])); + + // Set the current_code_hash + paras::Pallet::::force_set_current_code( + RuntimeOrigin::root(), + ParaId::from(1), + ValidationCode(vec![1]), + ) + .unwrap(); + paras::Pallet::::force_set_current_code( + RuntimeOrigin::root(), + ParaId::from(2), + ValidationCode(vec![2]), + ) + .unwrap(); + + let backed_candidates = (0_usize..2) + .into_iter() + .map(|idx0| { + let idx1 = idx0 + 1; + let para_id = ParaId::from(idx1); + let mut candidate = TestCandidateBuilder { + para_id, + relay_parent, + pov_hash: Hash::repeat_byte(idx1 as u8), + persisted_validation_data_hash: make_persisted_validation_data::( + para_id, + RELAY_PARENT_NUMBER, + Default::default(), + ) + .unwrap() + .hash(), + hrmp_watermark: RELAY_PARENT_NUMBER, + validation_code: ValidationCode(vec![idx1 as u8]), + ..Default::default() + } + .build(); + + collator_sign_candidate(Sr25519Keyring::One, &mut candidate); + + let signing_context = + SigningContext { parent_hash: relay_parent, session_index: session }; + let group = para_id_to_group[¶_id].as_ref(); + let backed = back_candidate( + candidate, + &validators_keyrings, // TODO: shuffle + group, + &keystore, + &signing_context, + BackingKind::Threshold, + None, + // Some(CoreIndex(idx0 as u32)), + ); + backed + }) + .collect::>(); + let disputes: MultiDisputeStatementSet = backed_candidates .iter() .map(|c| { @@ -3278,14 +3359,14 @@ mod sanitizers { validators.swap(0, backer.0 as _); let keyrings: Vec<_> = validators .iter() - .map(|(i, v)| (i.clone(), validators_keyrings[v].clone())) + .map(|(i, v)| (i.clone(), validators_to_keyrings[v].clone())) .collect(); make_dispute_concluding_against(c.hash(), session, &keyrings) }) .collect(); - let parent_block_number = 3; + let parent_block_number = RELAY_PARENT_NUMBER; let parent_header = HeaderFor::::new( parent_block_number, // `block_number`, Default::default(), // `extrinsics_root`, @@ -3316,9 +3397,27 @@ mod sanitizers { shared::Pallet::::add_allowed_relay_parent( header.hash(), *header.state_root(), - 3, + RELAY_PARENT_NUMBER, 10, ); + // Update scheduler's claimqueue with the parachains + let ttl = RELAY_PARENT_NUMBER + 10; + scheduler::Pallet::::set_claimqueue(BTreeMap::from([ + ( + CoreIndex::from(0), + VecDeque::from([ParasEntry::new( + Assignment::Pool { para_id: 1.into(), core_index: CoreIndex(0) }, + ttl, + )]), + ), + ( + CoreIndex::from(1), + VecDeque::from([ParasEntry::new( + Assignment::Pool { para_id: 2.into(), core_index: CoreIndex(1) }, + ttl, + )]), + ), + ])); let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); assert_matches!(pallet::OnChainVotes::::get(), Some(ScrapedOnChainVotes { @@ -3328,7 +3427,7 @@ mod sanitizers { assert_eq!(disputes.len(), n_disputes); }); - let parent_block_number = 5; + let parent_block_number = RELAY_PARENT_NUMBER + 2; run_to_block(parent_block_number, |_| None); let parent_header = HeaderFor::::new( parent_block_number, // `block_number`, From cc8e9ef9cf4e4a9bf85612e7daae6fed9e187ccd Mon Sep 17 00:00:00 2001 From: ordian Date: Mon, 3 Jun 2024 18:34:01 +0200 Subject: [PATCH 08/26] fix the issue to make the test pass --- .../parachains/src/paras_inherent/mod.rs | 24 ++++--------------- .../parachains/src/paras_inherent/tests.rs | 10 ++------ 2 files changed, 7 insertions(+), 27 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index cfa37ddb3c44..4a6cf726bb91 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -488,29 +488,15 @@ impl Pallet { // Contains the disputes that are concluded in the current session only, // since these are the only ones that are relevant for the occupied cores // and lightens the load on - // Cores can't be occupied with candidates of the previous sessions, and only - // things with new votes can have just concluded. We only need to collect - // cores with disputes that conclude just now, because disputes that - // concluded longer ago have already had any corresponding cores cleaned up. - let current_concluded_invalid_disputes = checked_disputes_sets - .iter() - .map(AsRef::as_ref) - .filter(|dss| dss.session == current_session) - .map(|dss| (dss.session, dss.candidate_hash)) - .filter(|(session, candidate)| { - ::DisputesHandler::concluded_invalid(*session, *candidate) - }) - .map(|(_session, candidate)| candidate) - .collect::>(); - // let mut current_concluded_invalid_disputes = - // ::DisputesHandler::disputes_concluded_invalid(current_session); + let mut current_concluded_invalid_disputes = + ::DisputesHandler::disputes_concluded_invalid(current_session); // Get the cores freed as a result of concluded invalid candidates. let (freed_disputed, concluded_invalid_hashes): (Vec, BTreeSet) = inclusion::Pallet::::free_disputed(¤t_concluded_invalid_disputes) .into_iter() .unzip(); - // current_concluded_invalid_disputes.extend(concluded_invalid_hashes); + current_concluded_invalid_disputes.extend(concluded_invalid_hashes); // Create a bit index from the set of core indices where each index corresponds to // a core index that was freed due to a dispute. @@ -583,8 +569,8 @@ impl Pallet { let backed_candidates_with_core = sanitize_backed_candidates::( backed_candidates, &allowed_relay_parents, - concluded_invalid_hashes, - // current_concluded_invalid_disputes, + // concluded_invalid_hashes, + current_concluded_invalid_disputes, scheduled, core_index_enabled, ); diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 51009e74c61a..2fe064c6b1f2 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -3228,7 +3228,6 @@ mod sanitizers { let mut hc = configuration::ActiveConfig::::get(); hc.minimum_backing_votes = 1; - // hc.max_validators = Some(5); hc.scheduler_params.group_rotation_frequency = 20; hc.async_backing_params.allowed_ancestry_len = 10; hc.async_backing_params.max_candidate_depth = 10; @@ -3451,13 +3450,8 @@ mod sanitizers { backed_candidates, }; - let _ = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap(); - assert_eq!( - // The length of this vec is equal to the number of candidates, so we know - // all of our candidates got filtered out - OnChainVotes::::get().unwrap().backing_validators_per_candidate.len(), - 0, - ); + let _ = + Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap_err(); }); } From cd583f7a4a11c758b1d2326da2b984cf41b3b09f Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 11:40:03 +0200 Subject: [PATCH 09/26] clean up a bit --- .../parachains/src/paras_inherent/mod.rs | 1 + .../parachains/src/paras_inherent/tests.rs | 21 +++---------------- polkadot/runtime/parachains/src/scheduler.rs | 5 ----- 3 files changed, 4 insertions(+), 23 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 4a6cf726bb91..60a9464c96c9 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -496,6 +496,7 @@ impl Pallet { inclusion::Pallet::::free_disputed(¤t_concluded_invalid_disputes) .into_iter() .unzip(); + // Also include descendants of the concluded invalid candidates. current_concluded_invalid_disputes.extend(concluded_invalid_hashes); // Create a bit index from the set of core indices where each index corresponds to diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 2fe064c6b1f2..60d94c2e15bd 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1743,7 +1743,7 @@ mod sanitizers { use crate::{ mock::{set_disabled_validators, RuntimeOrigin}, - scheduler::{common::Assignment, CoreOccupiedType, ParasEntry}, + scheduler::{common::Assignment, ParasEntry}, util::{make_persisted_validation_data, make_persisted_validation_data_with_parent}, }; use primitives::ValidationCode; @@ -1758,7 +1758,6 @@ mod sanitizers { expected_backed_candidates_with_core: BTreeMap>, scheduled_paras: BTreeMap>, - validators: Vec, } // Generate test data for the candidates and assert that the environment is set as expected @@ -1821,12 +1820,6 @@ mod sanitizers { vec![ValidatorIndex(2), ValidatorIndex(3)], ]); - // Set the availability of the cores - scheduler::Pallet::::set_availability_cores(vec![ - CoreOccupiedType::::Free, - CoreOccupiedType::::Free, - ]); - // Update scheduler's claimqueue with the parachains scheduler::Pallet::::set_claimqueue(BTreeMap::from([ ( @@ -1941,7 +1934,6 @@ mod sanitizers { backed_candidates, scheduled_paras: scheduled, expected_backed_candidates_with_core, - validators, } } @@ -2485,7 +2477,6 @@ mod sanitizers { backed_candidates, scheduled_paras: scheduled, expected_backed_candidates_with_core, - validators, } } @@ -2989,7 +2980,6 @@ mod sanitizers { backed_candidates, scheduled_paras: scheduled, expected_backed_candidates_with_core, - validators, } } @@ -3227,16 +3217,11 @@ mod sanitizers { const RELAY_PARENT_NUMBER: u32 = 1; let mut hc = configuration::ActiveConfig::::get(); - hc.minimum_backing_votes = 1; - hc.scheduler_params.group_rotation_frequency = 20; hc.async_backing_params.allowed_ancestry_len = 10; - hc.async_backing_params.max_candidate_depth = 10; configuration::Pallet::::force_set_active_config(hc); let header = default_header(); let relay_parent = header.hash(); - // let TestData { backed_candidates, validators, .. } = - // get_test_data_one_core_per_para(false); let session = SessionIndex::from(0_u32); let keystore = LocalKeystore::in_memory(); @@ -3286,6 +3271,7 @@ mod sanitizers { .map(|(i, g)| (ParaId::from(i as u32 + 1), g)) .collect(); let shuffled_indices = shared::ActiveValidatorIndices::::get(); + // shuffle the keyrings in the same way as the validators let validators_keyrings = crate::util::take_active_subset(&shuffled_indices, &validators_keyrings); @@ -3336,13 +3322,12 @@ mod sanitizers { let group = para_id_to_group[¶_id].as_ref(); let backed = back_candidate( candidate, - &validators_keyrings, // TODO: shuffle + &validators_keyrings, group, &keystore, &signing_context, BackingKind::Threshold, None, - // Some(CoreIndex(idx0 as u32)), ); backed }) diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index 699272319fa7..baeec49839df 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -679,9 +679,4 @@ impl Pallet { pub(crate) fn set_claimqueue(claimqueue: BTreeMap>>) { ClaimQueue::::set(claimqueue); } - - #[cfg(test)] - pub(crate) fn set_availability_cores(cores: Vec>) { - AvailabilityCores::::set(cores); - } } From cfbb524e3dd9b3e9a7c32162eac8a68fbca1b143 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 11:41:57 +0200 Subject: [PATCH 10/26] remove the commented out code for now --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 60a9464c96c9..45d7b6d8a545 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -460,7 +460,6 @@ impl Pallet { T::DisputesHandler::process_checked_multi_dispute_data(&checked_disputes_sets) { log::warn!(target: LOG_TARGET, "MultiDisputesData failed to update: {:?}", e); - // return Err(e.into()) }; METRICS.on_disputes_imported(checked_disputes_sets.len() as u64); From 1c818e1ca82251ea3567f7b4172a6b847ed9b5d5 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 11:44:45 +0200 Subject: [PATCH 11/26] restore previous comment --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 45d7b6d8a545..6ab940396f88 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -456,6 +456,9 @@ impl Pallet { // Note that `process_checked_multi_dispute_data` will iterate and import each // dispute; so the input here must be reasonably bounded, // which is guaranteed by the checks and weight limitation above. + // We don't care about fresh or not disputes + // this writes them to storage, so let's query it via those means + // if this fails for whatever reason, that's ok. if let Err(e) = T::DisputesHandler::process_checked_multi_dispute_data(&checked_disputes_sets) { From dd79e6fe1a3b0515f9033e17e91310a4eb36f612 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 11:45:55 +0200 Subject: [PATCH 12/26] remove disputes check on inclusion (not possible) --- polkadot/runtime/parachains/src/disputes.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index 7c6dc1a158ef..353c2b0cf87f 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -1234,12 +1234,6 @@ impl Pallet { let revert_to = included_in - One::one(); Included::::insert(&session, &candidate_hash, revert_to); - - if let Some(state) = Disputes::::get(&session, candidate_hash) { - if has_supermajority_against(&state) { - Self::revert_and_freeze(revert_to); - } - } } pub(crate) fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet { From eab058f77b40fa3ce2badff762d682b124dfc740 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 12:01:08 +0200 Subject: [PATCH 13/26] fixup the comment --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 6ab940396f88..5b5fc8dc04ca 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -489,7 +489,7 @@ impl Pallet { // Contains the disputes that are concluded in the current session only, // since these are the only ones that are relevant for the occupied cores - // and lightens the load on + // and lightens the load on `free_disputed` significantly. let mut current_concluded_invalid_disputes = ::DisputesHandler::disputes_concluded_invalid(current_session); From 56c65e7a0c45222333d32faf4be20bbdd4b1b00d Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 12:07:11 +0200 Subject: [PATCH 14/26] clean up some more --- .../parachains/src/paras_inherent/tests.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 60d94c2e15bd..2357a8052394 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -61,18 +61,18 @@ mod enter { use sp_runtime::Perbill; use sp_std::collections::btree_map::BTreeMap; - pub struct TestConfig { - pub dispute_statements: BTreeMap, - pub dispute_sessions: Vec, - pub backed_and_concluding: BTreeMap, - pub num_validators_per_core: u32, - pub code_upgrade: Option, - pub fill_claimqueue: bool, - pub elastic_paras: BTreeMap, - pub unavailable_cores: Vec, + struct TestConfig { + dispute_statements: BTreeMap, + dispute_sessions: Vec, + backed_and_concluding: BTreeMap, + num_validators_per_core: u32, + code_upgrade: Option, + fill_claimqueue: bool, + elastic_paras: BTreeMap, + unavailable_cores: Vec, } - pub fn make_inherent_data( + fn make_inherent_data( TestConfig { dispute_statements, dispute_sessions, From 67b09a14641011867b14e75d47556ca62c170969 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 12:15:47 +0200 Subject: [PATCH 15/26] prdoc --- prdoc/pr_4690.prdoc | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 prdoc/pr_4690.prdoc diff --git a/prdoc/pr_4690.prdoc b/prdoc/pr_4690.prdoc new file mode 100644 index 000000000000..6ebe60badca6 --- /dev/null +++ b/prdoc/pr_4690.prdoc @@ -0,0 +1,11 @@ +title: Relay chain runtime filtering changes for backing votes + +doc: + - audience: Node Dev + description: | + This change is only relevant for the relay chain runtime. It changes the way that backing votes are filtered in the runtime. + This change is necessary to ensure that the backing votes are correctly filtered in the runtime for an extra layer of protection again bugs in the code. + +crates: + - name: polkadot-runtime-parachains + bump: major From c82c4babc75e8b27e924e43021412afa5a5c4fef Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 12:22:42 +0200 Subject: [PATCH 16/26] more cleanup --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 5b5fc8dc04ca..63fedadd6b2a 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -572,7 +572,6 @@ impl Pallet { let backed_candidates_with_core = sanitize_backed_candidates::( backed_candidates, &allowed_relay_parents, - // concluded_invalid_hashes, current_concluded_invalid_disputes, scheduled, core_index_enabled, From dd04483585ffcb8a571cdc2f1dfec7ba53f242e6 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 12:28:19 +0200 Subject: [PATCH 17/26] fix clippy in tests --- polkadot/runtime/parachains/src/paras_inherent/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 2357a8052394..e6a18338645b 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -3243,7 +3243,7 @@ mod sanitizers { .unwrap(); } let validators_to_keyrings: HashMap = - validators_keyrings.iter().map(|v| (v.public().into(), v.clone())).collect(); + validators_keyrings.iter().map(|v| (v.public().into(), *v)).collect(); let validators_keys: Vec<(_, ValidatorId)> = validators_keyrings .iter() .enumerate() @@ -3343,7 +3343,7 @@ mod sanitizers { validators.swap(0, backer.0 as _); let keyrings: Vec<_> = validators .iter() - .map(|(i, v)| (i.clone(), validators_to_keyrings[v].clone())) + .map(|(i, v)| (*i, validators_to_keyrings[v])) .collect(); make_dispute_concluding_against(c.hash(), session, &keyrings) From f5d78ffd7535e02374e901ecf1bf5b3244dffa80 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 12:30:46 +0200 Subject: [PATCH 18/26] update prdoc --- prdoc/pr_4690.prdoc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/prdoc/pr_4690.prdoc b/prdoc/pr_4690.prdoc index 6ebe60badca6..8f9a75a41fc9 100644 --- a/prdoc/pr_4690.prdoc +++ b/prdoc/pr_4690.prdoc @@ -1,10 +1,12 @@ title: Relay chain runtime filtering changes for backing votes doc: - - audience: Node Dev + - audience: Runtime Dev description: | - This change is only relevant for the relay chain runtime. It changes the way that backing votes are filtered in the runtime. - This change is necessary to ensure that the backing votes are correctly filtered in the runtime for an extra layer of protection again bugs in the code. + This change is only relevant for the relay chain runtime. + This change is necessary to ensure that the backing votes are correctly + filtered in the runtime for an extra layer of protection again bugs in + the code. crates: - name: polkadot-runtime-parachains From a22beacc59e5b271a57e89d10fe12c9c3f424ce4 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 4 Jun 2024 16:18:50 +0200 Subject: [PATCH 19/26] no test - no problem --- .../runtime/parachains/src/disputes/tests.rs | 65 ------------------- .../parachains/src/paras_inherent/mod.rs | 10 +-- 2 files changed, 5 insertions(+), 70 deletions(-) diff --git a/polkadot/runtime/parachains/src/disputes/tests.rs b/polkadot/runtime/parachains/src/disputes/tests.rs index 5e4e7036bfef..4fbddf821a5d 100644 --- a/polkadot/runtime/parachains/src/disputes/tests.rs +++ b/polkadot/runtime/parachains/src/disputes/tests.rs @@ -577,71 +577,6 @@ fn test_disputes_with_missing_backing_votes_are_rejected() { }) } -#[test] -fn test_freeze_on_note_included() { - new_test_ext(Default::default()).execute_with(|| { - let v0 = ::Pair::generate().0; - let v1 = ::Pair::generate().0; - - 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)); - let inclusion_parent = sp_core::H256::repeat_byte(0xff); - let session = 3; - - // v0 votes for 3 - let stmts = vec![DisputeStatementSet { - candidate_hash, - session: 3, - statements: vec![ - ( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(0), - v0.sign( - &ExplicitDisputeStatement { valid: false, candidate_hash, session: 3 } - .signing_payload(), - ), - ), - ( - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit), - ValidatorIndex(1), - v1.sign( - &ExplicitDisputeStatement { valid: false, candidate_hash, session: 3 } - .signing_payload(), - ), - ), - ( - DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid( - inclusion_parent, - )), - ValidatorIndex(1), - v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload( - &SigningContext { session_index: session, parent_hash: inclusion_parent }, - )), - ), - ], - }]; - assert!(Pallet::::process_checked_multi_dispute_data( - &stmts - .into_iter() - .map(CheckedDisputeStatementSet::unchecked_from_unchecked) - .collect() - ) - .is_ok()); - - Pallet::::note_included(3, candidate_hash, 3); - assert_eq!(Frozen::::get(), Some(2)); - }); -} - #[test] fn test_freeze_provided_against_supermajority_for_included() { new_test_ext(Default::default()).execute_with(|| { diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 63fedadd6b2a..370d9bbe51e4 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -424,11 +424,11 @@ impl Pallet { METRICS.on_after_filter(all_weight_after.ref_time()); log::debug!( - target: LOG_TARGET, - "[process_inherent_data] after filter: bitfields.len(): {}, backed_candidates.len(): {}, checked_disputes_sets.len() {}", - bitfields.len(), - backed_candidates.len(), - checked_disputes_sets.len() + target: LOG_TARGET, + "[process_inherent_data] after filter: bitfields.len(): {}, backed_candidates.len(): {}, checked_disputes_sets.len() {}", + bitfields.len(), + backed_candidates.len(), + checked_disputes_sets.len() ); log::debug!(target: LOG_TARGET, "Size after filter: {}, candidates + bitfields: {}, disputes: {}", all_weight_after.proof_size(), non_disputes_weight.proof_size(), checked_disputes_sets_consumed_weight.proof_size()); log::debug!(target: LOG_TARGET, "Time weight after filter: {}, candidates + bitfields: {}, disputes: {}", all_weight_after.ref_time(), non_disputes_weight.ref_time(), checked_disputes_sets_consumed_weight.ref_time()); From 296d44a10135fb7d24039063763a3d342f677b37 Mon Sep 17 00:00:00 2001 From: ordian Date: Thu, 13 Jun 2024 23:44:12 +0200 Subject: [PATCH 20/26] fmt --- polkadot/runtime/parachains/src/disputes/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/disputes/tests.rs b/polkadot/runtime/parachains/src/disputes/tests.rs index e66323a5b4d7..a4810b1b5f8e 100644 --- a/polkadot/runtime/parachains/src/disputes/tests.rs +++ b/polkadot/runtime/parachains/src/disputes/tests.rs @@ -29,9 +29,9 @@ use frame_support::{ traits::{OnFinalize, OnInitialize}, }; use frame_system::pallet_prelude::BlockNumberFor; -use sp_keyring::Sr25519Keyring; use polkadot_primitives::BlockNumber; use sp_core::{crypto::CryptoType, Pair}; +use sp_keyring::Sr25519Keyring; const VOTE_FOR: VoteKind = VoteKind::ExplicitValid; const VOTE_AGAINST: VoteKind = VoteKind::Invalid; From 53d25cc4d234ce626b3fdfb54b96cd9c57361960 Mon Sep 17 00:00:00 2001 From: ordian Date: Thu, 18 Jul 2024 17:12:28 +0200 Subject: [PATCH 21/26] Apply suggestions from code review --- prdoc/pr_4690.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_4690.prdoc b/prdoc/pr_4690.prdoc index 8f9a75a41fc9..154f513847bf 100644 --- a/prdoc/pr_4690.prdoc +++ b/prdoc/pr_4690.prdoc @@ -5,7 +5,7 @@ doc: description: | This change is only relevant for the relay chain runtime. This change is necessary to ensure that the backing votes are correctly - filtered in the runtime for an extra layer of protection again bugs in + filtered in the runtime for an extra layer of protection against bugs in the code. crates: From 683d0ea23e559ff7875a4c84bbae9f98e0a50abd Mon Sep 17 00:00:00 2001 From: ordian Date: Wed, 14 Aug 2024 21:57:41 +0200 Subject: [PATCH 22/26] compute concluded invalid lazily --- polkadot/runtime/parachains/src/disputes.rs | 18 --------- .../runtime/parachains/src/inclusion/mod.rs | 11 ++--- .../parachains/src/paras_inherent/mod.rs | 40 ++++++++++--------- 3 files changed, 26 insertions(+), 43 deletions(-) diff --git a/polkadot/runtime/parachains/src/disputes.rs b/polkadot/runtime/parachains/src/disputes.rs index b5ff9802e446..00c9dba24f20 100644 --- a/polkadot/runtime/parachains/src/disputes.rs +++ b/polkadot/runtime/parachains/src/disputes.rs @@ -230,9 +230,6 @@ pub trait DisputesHandler { included_in: BlockNumber, ); - /// Get a list of disputes in a session that concluded invalid. - fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet; - /// Retrieve the included state of a given candidate in a particular session. If it /// returns `Some`, then we have a local dispute for the given `candidate_hash`. fn included_state(session: SessionIndex, candidate_hash: CandidateHash) -> Option; @@ -275,10 +272,6 @@ impl DisputesHandler for () { Ok(Vec::new()) } - fn disputes_concluded_invalid(_session: SessionIndex) -> BTreeSet { - BTreeSet::new() - } - fn note_included( _session: SessionIndex, _candidate_hash: CandidateHash, @@ -328,10 +321,6 @@ where pallet::Pallet::::process_checked_multi_dispute_data(statement_sets) } - fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet { - pallet::Pallet::::disputes_concluded_invalid(session) - } - fn note_included( session: SessionIndex, candidate_hash: CandidateHash, @@ -1237,13 +1226,6 @@ impl Pallet { Included::::insert(&session, &candidate_hash, revert_to); } - pub(crate) fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet { - Disputes::::iter_prefix(session) - .filter(|(_hash, state)| has_supermajority_against(state)) - .map(|(hash, _state)| hash) - .collect() - } - pub(crate) fn included_state( session: SessionIndex, candidate_hash: CandidateHash, diff --git a/polkadot/runtime/parachains/src/inclusion/mod.rs b/polkadot/runtime/parachains/src/inclusion/mod.rs index 281dc5d0c5f4..9bc7be654014 100644 --- a/polkadot/runtime/parachains/src/inclusion/mod.rs +++ b/polkadot/runtime/parachains/src/inclusion/mod.rs @@ -1032,14 +1032,11 @@ impl Pallet { /// /// Returns a vector of cleaned-up core IDs, along with the evicted candidate hashes. pub(crate) fn free_disputed( - disputed: &BTreeSet, + disputed: impl Fn(CandidateHash) -> bool, ) -> Vec<(CoreIndex, CandidateHash)> { - Self::free_failed_cores( - |candidate| disputed.contains(&candidate.hash), - Some(disputed.len()), - ) - .map(|candidate| (candidate.core, candidate.hash)) - .collect() + Self::free_failed_cores(|candidate| disputed(candidate.hash), None) + .map(|candidate| (candidate.core, candidate.hash)) + .collect() } // Clean up cores whose candidates are deemed as failed by the predicate. `pred` returns true if diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 2749fddc99a4..06b511072a3e 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -490,16 +490,19 @@ impl Pallet { // Contains the disputes that are concluded in the current session only, // since these are the only ones that are relevant for the occupied cores // and lightens the load on `free_disputed` significantly. - let mut current_concluded_invalid_disputes = - ::DisputesHandler::disputes_concluded_invalid(current_session); + let current_concluded_invalid_disputes = + |hash: CandidateHash| ::DisputesHandler::concluded_invalid(current_session, hash); // Get the cores freed as a result of concluded invalid candidates. let (freed_disputed, concluded_invalid_hashes): (Vec, BTreeSet) = - inclusion::Pallet::::free_disputed(¤t_concluded_invalid_disputes) + inclusion::Pallet::::free_disputed(current_concluded_invalid_disputes) .into_iter() .unzip(); // Also include descendants of the concluded invalid candidates. - current_concluded_invalid_disputes.extend(concluded_invalid_hashes); + let current_concluded_invalid_disputes = |hash: CandidateHash| { + concluded_invalid_hashes.contains(&hash) || + ::DisputesHandler::concluded_invalid(current_session, hash) + }; // Create a bit index from the set of core indices where each index corresponds to // a core index that was freed due to a dispute. @@ -946,7 +949,7 @@ pub(crate) fn sanitize_bitfields( fn sanitize_backed_candidates( backed_candidates: Vec>, allowed_relay_parents: &AllowedRelayParentsTracker>, - concluded_invalid_with_descendants: BTreeSet, + concluded_invalid_with_descendants: impl Fn(CandidateHash) -> bool, scheduled: BTreeMap>, core_index_enabled: bool, ) -> BTreeMap, CoreIndex)>> { @@ -967,7 +970,7 @@ fn sanitize_backed_candidates( // Remove any candidates that were concluded invalid or who are descendants of concluded invalid // candidates (along with their descendants). retain_candidates::(&mut candidates_per_para, |_, candidate| { - let keep = !concluded_invalid_with_descendants.contains(&candidate.candidate().hash()); + let keep = !concluded_invalid_with_descendants(candidate.candidate().hash()); if !keep { log::debug!( @@ -1157,18 +1160,19 @@ fn filter_backed_statements_from_disabled_validators< // Get relay parent block number of the candidate. We need this to get the group index // assigned to this core at this block number - let relay_parent_block_number = - match allowed_relay_parents.acquire_info(bc.descriptor().relay_parent, None) { - Some((_, block_num)) => block_num, - None => { - log::debug!( - target: LOG_TARGET, - "Relay parent {:?} for candidate is not in the allowed relay parents. Dropping the candidate.", - bc.descriptor().relay_parent - ); - return false - }, - }; + let relay_parent_block_number = match allowed_relay_parents + .acquire_info(bc.descriptor().relay_parent, None) + { + Some((_, block_num)) => block_num, + None => { + log::debug!( + target: LOG_TARGET, + "Relay parent {:?} for candidate is not in the allowed relay parents. Dropping the candidate.", + bc.descriptor().relay_parent + ); + return false + }, + }; // Get the group index for the core let group_idx = match scheduler::Pallet::::group_assigned_to_core( From 2b9eef6f255822f592b48760e4ef8a8ec03beee0 Mon Sep 17 00:00:00 2001 From: ordian Date: Thu, 15 Aug 2024 10:12:57 +0200 Subject: [PATCH 23/26] fix tests --- .../runtime/parachains/src/inclusion/tests.rs | 8 +++---- .../parachains/src/paras_inherent/tests.rs | 24 +++++++++++-------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/polkadot/runtime/parachains/src/inclusion/tests.rs b/polkadot/runtime/parachains/src/inclusion/tests.rs index 3ead456cde5a..e298c04899a5 100644 --- a/polkadot/runtime/parachains/src/inclusion/tests.rs +++ b/polkadot/runtime/parachains/src/inclusion/tests.rs @@ -525,12 +525,10 @@ fn free_disputed() { let mut config = genesis_config(paras); config.configuration.config.scheduler_params.group_rotation_frequency = 3; new_test_ext(config).execute_with(|| { - let disputed_cores = ParaInclusion::free_disputed(&BTreeSet::new()); + let disputed_cores = ParaInclusion::free_disputed(|_| false); assert!(disputed_cores.is_empty()); - let disputed_cores = ParaInclusion::free_disputed( - &[CandidateHash::default()].into_iter().collect::>(), - ); + let disputed_cores = ParaInclusion::free_disputed(|h| h == CandidateHash::default()); assert!(disputed_cores.is_empty()); let make_candidate = |core_index: u32| { @@ -609,7 +607,7 @@ fn free_disputed() { ] .into_iter() .collect::>(); - let disputed_cores = ParaInclusion::free_disputed(&disputed_candidates); + let disputed_cores = ParaInclusion::free_disputed(|h| disputed_candidates.contains(&h)); assert_eq!( disputed_cores.into_iter().map(|(core, _)| core).collect::>(), diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 27c2b2efcc9b..181779cbba0c 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -3413,6 +3413,10 @@ mod sanitizers { } } + fn no_disputes(_h: CandidateHash) -> bool { + false + } + #[rstest] #[case(false)] #[case(true)] @@ -3429,7 +3433,7 @@ mod sanitizers { sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - BTreeSet::new(), + no_disputes, scheduled, core_index_enabled ), @@ -3454,7 +3458,7 @@ mod sanitizers { sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - BTreeSet::new(), + no_disputes, scheduled, core_index_enabled ), @@ -3479,7 +3483,7 @@ mod sanitizers { sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - BTreeSet::new(), + no_disputes, scheduled, core_index_enabled, ), @@ -3511,7 +3515,7 @@ mod sanitizers { sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - BTreeSet::new(), + no_disputes, scheduled, core_index_enabled, ), @@ -3551,7 +3555,7 @@ mod sanitizers { let res = sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - BTreeSet::new(), + no_disputes, scheduled, core_index_enabled, ); @@ -3621,7 +3625,7 @@ mod sanitizers { let res = sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - BTreeSet::new(), + no_disputes, scheduled, core_index_enabled, ); @@ -3659,7 +3663,7 @@ mod sanitizers { let sanitized_backed_candidates = sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - BTreeSet::new(), + no_disputes, scheduled, core_index_enabled, ); @@ -3695,7 +3699,7 @@ mod sanitizers { > = sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - set, + |h| set.contains(&h), scheduled, core_index_enabled, ); @@ -3732,7 +3736,7 @@ mod sanitizers { > = sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - invalid_set, + |h| invalid_set.contains(&h), scheduled, true, ); @@ -3768,7 +3772,7 @@ mod sanitizers { > = sanitize_backed_candidates::( backed_candidates.clone(), &shared::AllowedRelayParents::::get(), - invalid_set, + |h| invalid_set.contains(&h), scheduled, true, ); From 8369fe9afd4c0816f0af5a6b8acdb827e8c0c88d Mon Sep 17 00:00:00 2001 From: ordian Date: Thu, 15 Aug 2024 10:13:55 +0200 Subject: [PATCH 24/26] fmt --- .../parachains/src/paras_inherent/mod.rs | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index add5b939aa61..a64feb60e65f 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1160,19 +1160,18 @@ fn filter_backed_statements_from_disabled_validators< // Get relay parent block number of the candidate. We need this to get the group index // assigned to this core at this block number - let relay_parent_block_number = match allowed_relay_parents - .acquire_info(bc.descriptor().relay_parent, None) - { - Some((_, block_num)) => block_num, - None => { - log::debug!( - target: LOG_TARGET, - "Relay parent {:?} for candidate is not in the allowed relay parents. Dropping the candidate.", - bc.descriptor().relay_parent - ); - return false - }, - }; + let relay_parent_block_number = + match allowed_relay_parents.acquire_info(bc.descriptor().relay_parent, None) { + Some((_, block_num)) => block_num, + None => { + log::debug!( + target: LOG_TARGET, + "Relay parent {:?} for candidate is not in the allowed relay parents. Dropping the candidate.", + bc.descriptor().relay_parent + ); + return false + }, + }; // Get the group index for the core let group_idx = match scheduler::Pallet::::group_assigned_to_core( From e946580d63ea6e46fd3ddc6f40cd3b0047e757dd Mon Sep 17 00:00:00 2001 From: ordian Date: Fri, 16 Aug 2024 14:23:06 +0200 Subject: [PATCH 25/26] fix the test --- .../parachains/src/paras_inherent/mod.rs | 24 +++++++++++++++---- .../parachains/src/paras_inherent/tests.rs | 13 ++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index a64feb60e65f..fecc77221941 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -490,14 +490,28 @@ impl Pallet { // Contains the disputes that are concluded in the current session only, // since these are the only ones that are relevant for the occupied cores // and lightens the load on `free_disputed` significantly. - let current_concluded_invalid_disputes = - |hash: CandidateHash| ::DisputesHandler::concluded_invalid(current_session, hash); + // Cores can't be occupied with candidates of the previous sessions, and only + // things with new votes can have just concluded. We only need to collect + // cores with disputes that conclude just now, because disputes that + // concluded longer ago have already had any corresponding cores cleaned up. + let current_concluded_invalid_disputes = checked_disputes_sets + .iter() + .map(AsRef::as_ref) + .filter(|dss| dss.session == current_session) + .map(|dss| (dss.session, dss.candidate_hash)) + .filter(|(session, candidate)| { + ::DisputesHandler::concluded_invalid(*session, *candidate) + }) + .map(|(_session, candidate)| candidate) + .collect::>(); // Get the cores freed as a result of concluded invalid candidates. let (freed_disputed, concluded_invalid_hashes): (Vec, BTreeSet) = - inclusion::Pallet::::free_disputed(current_concluded_invalid_disputes) - .into_iter() - .unzip(); + inclusion::Pallet::::free_disputed(|h| { + current_concluded_invalid_disputes.contains(&h) + }) + .into_iter() + .unzip(); // Also include descendants of the concluded invalid candidates. let current_concluded_invalid_disputes = |hash: CandidateHash| { concluded_invalid_hashes.contains(&hash) || diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 181779cbba0c..856ca6948043 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -3876,6 +3876,19 @@ mod sanitizers { ValidationCode(vec![2]), ) .unwrap(); + // Set the most recent relay parent. + paras::Pallet::::force_set_most_recent_context( + RuntimeOrigin::root(), + ParaId::from(1), + BlockNumberFor::::from(0u32), + ) + .unwrap(); + paras::Pallet::::force_set_most_recent_context( + RuntimeOrigin::root(), + ParaId::from(2), + BlockNumberFor::::from(0u32), + ) + .unwrap(); let backed_candidates = (0_usize..2) .into_iter() From b83f01bf584099f6d59abee904106c04d0ad09f0 Mon Sep 17 00:00:00 2001 From: ordian Date: Tue, 27 Aug 2024 13:08:42 +0200 Subject: [PATCH 26/26] address review comments --- .../runtime/parachains/src/paras_inherent/mod.rs | 15 ++++++++------- .../parachains/src/paras_inherent/tests.rs | 13 ++++++++++--- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index fecc77221941..42e0a00899a9 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -512,10 +512,10 @@ impl Pallet { }) .into_iter() .unzip(); - // Also include descendants of the concluded invalid candidates. - let current_concluded_invalid_disputes = |hash: CandidateHash| { - concluded_invalid_hashes.contains(&hash) || - ::DisputesHandler::concluded_invalid(current_session, hash) + // Also include cores freed as a result of concluded invalid candidates. + let current_concluded_invalid_disputes = |hash: &CandidateHash| { + concluded_invalid_hashes.contains(hash) || + ::DisputesHandler::concluded_invalid(current_session, *hash) }; // Create a bit index from the set of core indices where each index corresponds to @@ -963,7 +963,7 @@ pub(crate) fn sanitize_bitfields( fn sanitize_backed_candidates( backed_candidates: Vec>, allowed_relay_parents: &AllowedRelayParentsTracker>, - concluded_invalid_with_descendants: impl Fn(CandidateHash) -> bool, + concluded_invalid_with_descendants: impl Fn(&CandidateHash) -> bool, scheduled: BTreeMap>, core_index_enabled: bool, ) -> BTreeMap, CoreIndex)>> { @@ -984,13 +984,14 @@ fn sanitize_backed_candidates( // Remove any candidates that were concluded invalid or who are descendants of concluded invalid // candidates (along with their descendants). retain_candidates::(&mut candidates_per_para, |_, candidate| { - let keep = !concluded_invalid_with_descendants(candidate.candidate().hash()); + let hash = candidate.candidate().hash(); + let keep = !concluded_invalid_with_descendants(&hash); if !keep { log::debug!( target: LOG_TARGET, "Found backed candidate {:?} which was concluded invalid or is a descendant of a concluded invalid candidate, for paraid {:?}.", - candidate.candidate().hash(), + hash, candidate.descriptor().para_id ); } diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 856ca6948043..cb1a70fda93d 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1748,7 +1748,7 @@ mod sanitizers { }; use alloc::collections::vec_deque::VecDeque; use polkadot_primitives::ValidationCode; - use sp_runtime::Digest; + use sp_runtime::{Digest, ModuleError}; use super::*; @@ -3413,7 +3413,7 @@ mod sanitizers { } } - fn no_disputes(_h: CandidateHash) -> bool { + fn no_disputes(_h: &CandidateHash) -> bool { false } @@ -4032,8 +4032,15 @@ mod sanitizers { backed_candidates, }; - let _ = + let err = Pallet::::enter(frame_system::RawOrigin::None.into(), data).unwrap_err(); + assert_matches!( + err.error, + DispatchError::Module(ModuleError { + message: Some("CandidatesFilteredDuringExecution"), + .. + }) + ); }); }