From 913232ae00b4954baf538a67a6b022a947579bfb Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 20 Oct 2023 16:13:29 +0300 Subject: [PATCH 1/5] Handling of disabled validators in backing subsystem (#1259) Handles validator disabling in the backing subsystem. The PR introduces two changes: 1. Don't import statements from disabled validators. 2. Don't validate and second if the local validator is disabled. The purpose of this change is first to ignore statements from disabled validators on the node side. This is an optimisation as these statements are supposed to be cleared in the runtime too (still not implemented at the moment). Additionally if the local node is a validator which is disabled - don't process any new candidates. --------- Co-authored-by: ordian Co-authored-by: ordian --- polkadot/node/core/backing/src/lib.rs | 87 ++++- polkadot/node/core/backing/src/tests/mod.rs | 327 +++++++++++++++++- .../src/tests/prospective_parachains.rs | 20 ++ polkadot/node/core/provisioner/src/lib.rs | 59 +--- polkadot/node/subsystem-util/src/lib.rs | 80 ++++- polkadot/node/subsystem-util/src/vstaging.rs | 56 +++ 6 files changed, 556 insertions(+), 73 deletions(-) create mode 100644 polkadot/node/subsystem-util/src/vstaging.rs diff --git a/polkadot/node/core/backing/src/lib.rs b/polkadot/node/core/backing/src/lib.rs index 97efb3ba8089f..9fc03f2efdd5b 100644 --- a/polkadot/node/core/backing/src/lib.rs +++ b/polkadot/node/core/backing/src/lib.rs @@ -118,6 +118,7 @@ use statement_table::{ }, Config as TableConfig, Context as TableContextTrait, Table, }; +use util::vstaging::get_disabled_validators_with_fallback; mod error; @@ -383,6 +384,21 @@ struct TableContext { validator: Option, groups: HashMap>, validators: Vec, + disabled_validators: Vec, +} + +impl TableContext { + // Returns `true` if the provided `ValidatorIndex` is in the disabled validators list + pub fn validator_is_disabled(&self, validator_idx: &ValidatorIndex) -> bool { + self.disabled_validators + .iter() + .any(|disabled_val_idx| *disabled_val_idx == *validator_idx) + } + + // Returns `true` if the local validator is in the disabled validators list + pub fn local_validator_is_disabled(&self) -> Option { + self.validator.as_ref().map(|v| v.disabled()) + } } impl TableContextTrait for TableContext { @@ -1010,21 +1026,33 @@ async fn construct_per_relay_parent_state( let minimum_backing_votes = try_runtime_api!(request_min_backing_votes(parent, session_index, ctx.sender()).await); + // TODO: https://github.com/paritytech/polkadot-sdk/issues/1940 + // Once runtime ver `DISABLED_VALIDATORS_RUNTIME_REQUIREMENT` is released remove this call to + // `get_disabled_validators_with_fallback`, add `request_disabled_validators` call to the + // `try_join!` above and use `try_runtime_api!` to get `disabled_validators` + let disabled_validators = get_disabled_validators_with_fallback(ctx.sender(), parent) + .await + .map_err(Error::UtilError)?; + let signing_context = SigningContext { parent_hash: parent, session_index }; - let validator = - match Validator::construct(&validators, signing_context.clone(), keystore.clone()) { - Ok(v) => Some(v), - Err(util::Error::NotAValidator) => None, - Err(e) => { - gum::warn!( - target: LOG_TARGET, - err = ?e, - "Cannot participate in candidate backing", - ); + let validator = match Validator::construct( + &validators, + &disabled_validators, + signing_context.clone(), + keystore.clone(), + ) { + Ok(v) => Some(v), + Err(util::Error::NotAValidator) => None, + Err(e) => { + gum::warn!( + target: LOG_TARGET, + err = ?e, + "Cannot participate in candidate backing", + ); - return Ok(None) - }, - }; + return Ok(None) + }, + }; let mut groups = HashMap::new(); let n_cores = cores.len(); @@ -1054,7 +1082,7 @@ async fn construct_per_relay_parent_state( } } - let table_context = TableContext { groups, validators, validator }; + let table_context = TableContext { validator, groups, validators, disabled_validators }; let table_config = TableConfig { allow_multiple_seconded: match mode { ProspectiveParachainsMode::Enabled { .. } => true, @@ -1728,6 +1756,19 @@ async fn kick_off_validation_work( background_validation_tx: &mpsc::Sender<(Hash, ValidatedCandidateCommand)>, attesting: AttestingData, ) -> Result<(), Error> { + // Do nothing if the local validator is disabled or not a validator at all + match rp_state.table_context.local_validator_is_disabled() { + Some(true) => { + gum::info!(target: LOG_TARGET, "We are disabled - don't kick off validation"); + return Ok(()) + }, + Some(false) => {}, // we are not disabled - move on + None => { + gum::debug!(target: LOG_TARGET, "We are not a validator - don't kick off validation"); + return Ok(()) + }, + } + let candidate_hash = attesting.candidate.hash(); if rp_state.issued_statements.contains(&candidate_hash) { return Ok(()) @@ -1785,6 +1826,16 @@ async fn maybe_validate_and_import( }, }; + // Don't import statement if the sender is disabled + if rp_state.table_context.validator_is_disabled(&statement.validator_index()) { + gum::debug!( + target: LOG_TARGET, + sender_validator_idx = ?statement.validator_index(), + "Not importing statement because the sender is disabled" + ); + return Ok(()) + } + let res = import_statement(ctx, rp_state, &mut state.per_candidate, &statement).await; // if we get an Error::RejectedByProspectiveParachains, @@ -1946,6 +1997,13 @@ async fn handle_second_message( Some(r) => r, }; + // Just return if the local validator is disabled. If we are here the local node should be a + // validator but defensively use `unwrap_or(false)` to continue processing in this case. + if rp_state.table_context.local_validator_is_disabled().unwrap_or(false) { + gum::warn!(target: LOG_TARGET, "Local validator is disabled. Don't validate and second"); + return Ok(()) + } + // Sanity check that candidate is from our assignment. if Some(candidate.descriptor().para_id) != rp_state.assignment { gum::debug!( @@ -1992,6 +2050,7 @@ async fn handle_statement_message( ) -> Result<(), Error> { let _timer = metrics.time_process_statement(); + // Validator disabling is handled in `maybe_validate_and_import` match maybe_validate_and_import(ctx, state, relay_parent, statement).await { Err(Error::ValidationFailed(_)) => Ok(()), Err(e) => Err(e), diff --git a/polkadot/node/core/backing/src/tests/mod.rs b/polkadot/node/core/backing/src/tests/mod.rs index 35d17f3d905e4..26b0c679325bc 100644 --- a/polkadot/node/core/backing/src/tests/mod.rs +++ b/polkadot/node/core/backing/src/tests/mod.rs @@ -41,7 +41,7 @@ use sp_keyring::Sr25519Keyring; use sp_keystore::Keystore; use sp_tracing as _; use statement_table::v2::Misbehavior; -use std::collections::HashMap; +use std::{collections::HashMap, time::Duration}; mod prospective_parachains; @@ -77,6 +77,7 @@ struct TestState { signing_context: SigningContext, relay_parent: Hash, minimum_backing_votes: u32, + disabled_validators: Vec, } impl TestState { @@ -148,6 +149,7 @@ impl Default for TestState { signing_context, relay_parent, minimum_backing_votes: LEGACY_MIN_BACKING_VOTES, + disabled_validators: Vec::new(), } } } @@ -293,6 +295,26 @@ async fn test_startup(virtual_overseer: &mut VirtualOverseer, test_state: &TestS tx.send(Ok(test_state.minimum_backing_votes)).unwrap(); } ); + + // Check that subsystem job issues a request for the runtime version. + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(parent, RuntimeApiRequest::Version(tx)) + ) if parent == test_state.relay_parent => { + tx.send(Ok(RuntimeApiRequest::DISABLED_VALIDATORS_RUNTIME_REQUIREMENT)).unwrap(); + } + ); + + // Check that subsystem job issues a request for the disabled validators. + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(parent, RuntimeApiRequest::DisabledValidators(tx)) + ) if parent == test_state.relay_parent => { + tx.send(Ok(test_state.disabled_validators.clone())).unwrap(); + } + ); } async fn assert_validation_requests( @@ -1420,6 +1442,7 @@ fn candidate_backing_reorders_votes() { let table_context = TableContext { validator: None, + disabled_validators: Vec::new(), groups: validator_groups, validators: validator_public.clone(), }; @@ -1958,3 +1981,305 @@ fn new_leaf_view_doesnt_clobber_old() { virtual_overseer }); } + +// Test that a disabled local validator doesn't do any work on `CandidateBackingMessage::Second` +#[test] +fn disabled_validator_doesnt_distribute_statement_on_receiving_second() { + let mut test_state = TestState::default(); + test_state.disabled_validators.push(ValidatorIndex(0)); + + test_harness(test_state.keystore.clone(), |mut virtual_overseer| async move { + test_startup(&mut virtual_overseer, &test_state).await; + + let pov = PoV { block_data: BlockData(vec![42, 43, 44]) }; + let pvd = dummy_pvd(); + let validation_code = ValidationCode(vec![1, 2, 3]); + + let expected_head_data = test_state.head_data.get(&test_state.chain_ids[0]).unwrap(); + + let pov_hash = pov.hash(); + let candidate = TestCandidateBuilder { + para_id: test_state.chain_ids[0], + relay_parent: test_state.relay_parent, + pov_hash, + head_data: expected_head_data.clone(), + erasure_root: make_erasure_root(&test_state, pov.clone(), pvd.clone()), + persisted_validation_data_hash: pvd.hash(), + validation_code: validation_code.0.clone(), + } + .build(); + + let second = CandidateBackingMessage::Second( + test_state.relay_parent, + candidate.to_plain(), + pvd.clone(), + pov.clone(), + ); + + virtual_overseer.send(FromOrchestra::Communication { msg: second }).await; + + // Ensure backing subsystem is not doing any work + assert_matches!(virtual_overseer.recv().timeout(Duration::from_secs(1)).await, None); + + virtual_overseer + .send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves( + ActiveLeavesUpdate::stop_work(test_state.relay_parent), + ))) + .await; + virtual_overseer + }); +} + +// Test that a disabled local validator doesn't do any work on `CandidateBackingMessage::Statement` +#[test] +fn disabled_validator_doesnt_distribute_statement_on_receiving_statement() { + let mut test_state = TestState::default(); + test_state.disabled_validators.push(ValidatorIndex(0)); + + test_harness(test_state.keystore.clone(), |mut virtual_overseer| async move { + test_startup(&mut virtual_overseer, &test_state).await; + + let pov = PoV { block_data: BlockData(vec![42, 43, 44]) }; + let pvd = dummy_pvd(); + let validation_code = ValidationCode(vec![1, 2, 3]); + + let expected_head_data = test_state.head_data.get(&test_state.chain_ids[0]).unwrap(); + + let pov_hash = pov.hash(); + let candidate = TestCandidateBuilder { + para_id: test_state.chain_ids[0], + relay_parent: test_state.relay_parent, + pov_hash, + head_data: expected_head_data.clone(), + erasure_root: make_erasure_root(&test_state, pov.clone(), pvd.clone()), + persisted_validation_data_hash: pvd.hash(), + validation_code: validation_code.0.clone(), + } + .build(); + + let public2 = Keystore::sr25519_generate_new( + &*test_state.keystore, + ValidatorId::ID, + Some(&test_state.validators[2].to_seed()), + ) + .expect("Insert key into keystore"); + + let signed = SignedFullStatementWithPVD::sign( + &test_state.keystore, + StatementWithPVD::Seconded(candidate.clone(), pvd.clone()), + &test_state.signing_context, + ValidatorIndex(2), + &public2.into(), + ) + .ok() + .flatten() + .expect("should be signed"); + + let statement = CandidateBackingMessage::Statement(test_state.relay_parent, signed.clone()); + + virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; + + // Ensure backing subsystem is not doing any work + assert_matches!(virtual_overseer.recv().timeout(Duration::from_secs(1)).await, None); + + virtual_overseer + .send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves( + ActiveLeavesUpdate::stop_work(test_state.relay_parent), + ))) + .await; + virtual_overseer + }); +} + +// Test that a validator doesn't do any work on receiving a `CandidateBackingMessage::Statement` +// from a disabled validator +#[test] +fn validator_ignores_statements_from_disabled_validators() { + let mut test_state = TestState::default(); + test_state.disabled_validators.push(ValidatorIndex(2)); + + test_harness(test_state.keystore.clone(), |mut virtual_overseer| async move { + test_startup(&mut virtual_overseer, &test_state).await; + + let pov = PoV { block_data: BlockData(vec![42, 43, 44]) }; + let pvd = dummy_pvd(); + let validation_code = ValidationCode(vec![1, 2, 3]); + + let expected_head_data = test_state.head_data.get(&test_state.chain_ids[0]).unwrap(); + + let pov_hash = pov.hash(); + let candidate = TestCandidateBuilder { + para_id: test_state.chain_ids[0], + relay_parent: test_state.relay_parent, + pov_hash, + head_data: expected_head_data.clone(), + erasure_root: make_erasure_root(&test_state, pov.clone(), pvd.clone()), + persisted_validation_data_hash: pvd.hash(), + validation_code: validation_code.0.clone(), + } + .build(); + let candidate_commitments_hash = candidate.commitments.hash(); + + let public2 = Keystore::sr25519_generate_new( + &*test_state.keystore, + ValidatorId::ID, + Some(&test_state.validators[2].to_seed()), + ) + .expect("Insert key into keystore"); + + let signed_2 = SignedFullStatementWithPVD::sign( + &test_state.keystore, + StatementWithPVD::Seconded(candidate.clone(), pvd.clone()), + &test_state.signing_context, + ValidatorIndex(2), + &public2.into(), + ) + .ok() + .flatten() + .expect("should be signed"); + + let statement_2 = + CandidateBackingMessage::Statement(test_state.relay_parent, signed_2.clone()); + + virtual_overseer.send(FromOrchestra::Communication { msg: statement_2 }).await; + + // Ensure backing subsystem is not doing any work + assert_matches!(virtual_overseer.recv().timeout(Duration::from_secs(1)).await, None); + + // Now send a statement from a honest validator and make sure it gets processed + let public3 = Keystore::sr25519_generate_new( + &*test_state.keystore, + ValidatorId::ID, + Some(&test_state.validators[3].to_seed()), + ) + .expect("Insert key into keystore"); + + let signed_3 = SignedFullStatementWithPVD::sign( + &test_state.keystore, + StatementWithPVD::Seconded(candidate.clone(), pvd.clone()), + &test_state.signing_context, + ValidatorIndex(3), + &public3.into(), + ) + .ok() + .flatten() + .expect("should be signed"); + + let statement_3 = + CandidateBackingMessage::Statement(test_state.relay_parent, signed_3.clone()); + + virtual_overseer.send(FromOrchestra::Communication { msg: statement_3 }).await; + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(_, RuntimeApiRequest::ValidationCodeByHash(hash, tx)) + ) if hash == validation_code.hash() => { + tx.send(Ok(Some(validation_code.clone()))).unwrap(); + } + ); + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(_, RuntimeApiRequest::SessionIndexForChild(tx)) + ) => { + tx.send(Ok(1u32.into())).unwrap(); + } + ); + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(_, RuntimeApiRequest::SessionExecutorParams(sess_idx, tx)) + ) if sess_idx == 1 => { + tx.send(Ok(Some(ExecutorParams::default()))).unwrap(); + } + ); + + // Sending a `Statement::Seconded` for our assignment will start + // validation process. The first thing requested is the PoV. + assert_matches!( + virtual_overseer.recv().await, + AllMessages::AvailabilityDistribution( + AvailabilityDistributionMessage::FetchPoV { + relay_parent, + tx, + .. + } + ) if relay_parent == test_state.relay_parent => { + tx.send(pov.clone()).unwrap(); + } + ); + + // The next step is the actual request to Validation subsystem + // to validate the `Seconded` candidate. + assert_matches!( + virtual_overseer.recv().await, + AllMessages::CandidateValidation( + CandidateValidationMessage::ValidateFromExhaustive( + _pvd, + _validation_code, + candidate_receipt, + _pov, + _, + timeout, + tx, + ), + ) if _pvd == pvd && + _validation_code == validation_code && + *_pov == pov && &candidate_receipt.descriptor == candidate.descriptor() && + timeout == PvfExecTimeoutKind::Backing && + candidate_commitments_hash == candidate_receipt.commitments_hash => + { + tx.send(Ok( + ValidationResult::Valid(CandidateCommitments { + head_data: expected_head_data.clone(), + upward_messages: Default::default(), + horizontal_messages: Default::default(), + new_validation_code: None, + processed_downward_messages: 0, + hrmp_watermark: 0, + }, test_state.validation_data.clone()), + )).unwrap(); + } + ); + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::AvailabilityStore( + AvailabilityStoreMessage::StoreAvailableData { candidate_hash, tx, .. } + ) if candidate_hash == candidate.hash() => { + tx.send(Ok(())).unwrap(); + } + ); + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::StatementDistribution( + StatementDistributionMessage::Share(hash, _stmt) + ) => { + assert_eq!(test_state.relay_parent, hash); + } + ); + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::Provisioner( + ProvisionerMessage::ProvisionableData( + _, + ProvisionableData::BackedCandidate(candidate_receipt) + ) + ) => { + assert_eq!(candidate_receipt, candidate.to_plain()); + } + ); + + virtual_overseer + .send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves( + ActiveLeavesUpdate::stop_work(test_state.relay_parent), + ))) + .await; + virtual_overseer + }); +} diff --git a/polkadot/node/core/backing/src/tests/prospective_parachains.rs b/polkadot/node/core/backing/src/tests/prospective_parachains.rs index b79515ed37a6e..661b74c2f969a 100644 --- a/polkadot/node/core/backing/src/tests/prospective_parachains.rs +++ b/polkadot/node/core/backing/src/tests/prospective_parachains.rs @@ -195,6 +195,26 @@ async fn activate_leaf( tx.send(Ok(test_state.minimum_backing_votes)).unwrap(); } ); + + // Check that subsystem job issues a request for the runtime version. + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(parent, RuntimeApiRequest::Version(tx)) + ) if parent == hash => { + tx.send(Ok(RuntimeApiRequest::DISABLED_VALIDATORS_RUNTIME_REQUIREMENT)).unwrap(); + } + ); + + // Check that the subsystem job issues a request for the disabled validators. + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(parent, RuntimeApiRequest::DisabledValidators(tx)) + ) if parent == hash => { + tx.send(Ok(Vec::new())).unwrap(); + } + ); } } diff --git a/polkadot/node/core/provisioner/src/lib.rs b/polkadot/node/core/provisioner/src/lib.rs index f81e5550b15da..a8475cffa1990 100644 --- a/polkadot/node/core/provisioner/src/lib.rs +++ b/polkadot/node/core/provisioner/src/lib.rs @@ -29,13 +29,13 @@ use polkadot_node_subsystem::{ jaeger, messages::{ CandidateBackingMessage, ChainApiMessage, ProspectiveParachainsMessage, ProvisionableData, - ProvisionerInherentData, ProvisionerMessage, RuntimeApiMessage, RuntimeApiRequest, + ProvisionerInherentData, ProvisionerMessage, RuntimeApiRequest, }, overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, LeafStatus, OverseerSignal, - PerLeafSpan, RuntimeApiError, SpawnedSubsystem, SubsystemError, + PerLeafSpan, SpawnedSubsystem, SubsystemError, }; use polkadot_node_subsystem_util::{ - request_availability_cores, request_persisted_validation_data, + has_required_runtime, request_availability_cores, request_persisted_validation_data, runtime::{prospective_parachains_mode, ProspectiveParachainsMode}, TimeoutExt, }; @@ -862,56 +862,3 @@ fn bitfields_indicate_availability( 3 * availability.count_ones() >= 2 * availability.len() } - -// If we have to be absolutely precise here, this method gets the version of the `ParachainHost` -// api. For brevity we'll just call it 'runtime version'. -async fn has_required_runtime( - sender: &mut impl overseer::ProvisionerSenderTrait, - relay_parent: Hash, - required_runtime_version: u32, -) -> bool { - gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching ParachainHost runtime api version"); - - let (tx, rx) = oneshot::channel(); - sender - .send_message(RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::Version(tx))) - .await; - - match rx.await { - Result::Ok(Ok(runtime_version)) => { - gum::trace!( - target: LOG_TARGET, - ?relay_parent, - ?runtime_version, - ?required_runtime_version, - "Fetched ParachainHost runtime api version" - ); - runtime_version >= required_runtime_version - }, - Result::Ok(Err(RuntimeApiError::Execution { source: error, .. })) => { - gum::trace!( - target: LOG_TARGET, - ?relay_parent, - ?error, - "Execution error while fetching ParachainHost runtime api version" - ); - false - }, - Result::Ok(Err(RuntimeApiError::NotSupported { .. })) => { - gum::trace!( - target: LOG_TARGET, - ?relay_parent, - "NotSupported error while fetching ParachainHost runtime api version" - ); - false - }, - Result::Err(_) => { - gum::trace!( - target: LOG_TARGET, - ?relay_parent, - "Cancelled error while fetching ParachainHost runtime api version" - ); - false - }, - } -} diff --git a/polkadot/node/subsystem-util/src/lib.rs b/polkadot/node/subsystem-util/src/lib.rs index a5f3e9d4a0c0e..1fe52767df61c 100644 --- a/polkadot/node/subsystem-util/src/lib.rs +++ b/polkadot/node/subsystem-util/src/lib.rs @@ -55,6 +55,7 @@ use sp_core::ByteArray; use sp_keystore::{Error as KeystoreError, KeystorePtr}; use std::time::Duration; use thiserror::Error; +use vstaging::get_disabled_validators_with_fallback; pub use metered; pub use polkadot_node_network_protocol::MIN_GOSSIP_PEERS; @@ -79,6 +80,9 @@ pub mod inclusion_emulator; /// Convenient and efficient runtime info access. pub mod runtime; +/// Helpers for working with unreleased runtime calls +pub mod vstaging; + /// Nested message sending /// /// Useful for having mostly synchronous code, with submodules spawning short lived asynchronous @@ -92,6 +96,8 @@ mod determine_new_blocks; #[cfg(test)] mod tests; +const LOG_TARGET: &'static str = "parachain::subsystem-util"; + /// Duration a job will wait after sending a stop signal before hard-aborting. pub const JOB_GRACEFUL_STOP_DURATION: Duration = Duration::from_secs(1); /// Capacity of channels to and from individual jobs @@ -157,6 +163,62 @@ where rx } +/// Verifies if `ParachainHost` runtime api is at least at version `required_runtime_version`. This +/// method is used to determine if a given runtime call is supported by the runtime. +pub async fn has_required_runtime( + sender: &mut Sender, + relay_parent: Hash, + required_runtime_version: u32, +) -> bool +where + Sender: SubsystemSender, +{ + gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching ParachainHost runtime api version"); + + let (tx, rx) = oneshot::channel(); + sender + .send_message(RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::Version(tx))) + .await; + + match rx.await { + Result::Ok(Ok(runtime_version)) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + ?runtime_version, + ?required_runtime_version, + "Fetched ParachainHost runtime api version" + ); + runtime_version >= required_runtime_version + }, + Result::Ok(Err(RuntimeApiError::Execution { source: error, .. })) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + ?error, + "Execution error while fetching ParachainHost runtime api version" + ); + false + }, + Result::Ok(Err(RuntimeApiError::NotSupported { .. })) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + "NotSupported error while fetching ParachainHost runtime api version" + ); + false + }, + Result::Err(_) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + "Cancelled error while fetching ParachainHost runtime api version" + ); + false + }, + } +} + /// Construct specialized request functions for the runtime. /// /// These would otherwise get pretty repetitive. @@ -378,6 +440,7 @@ pub struct Validator { signing_context: SigningContext, key: ValidatorId, index: ValidatorIndex, + disabled: bool, } impl Validator { @@ -399,7 +462,12 @@ impl Validator { let validators = validators?; - Self::construct(&validators, signing_context, keystore) + // TODO: https://github.com/paritytech/polkadot-sdk/issues/1940 + // When `DisabledValidators` is released remove this and add a + // `request_disabled_validators` call here + let disabled_validators = get_disabled_validators_with_fallback(sender, parent).await?; + + Self::construct(&validators, &disabled_validators, signing_context, keystore) } /// Construct a validator instance without performing runtime fetches. @@ -407,13 +475,16 @@ impl Validator { /// This can be useful if external code also needs the same data. pub fn construct( validators: &[ValidatorId], + disabled_validators: &[ValidatorIndex], signing_context: SigningContext, keystore: KeystorePtr, ) -> Result { let (key, index) = signing_key_and_index(validators, &keystore).ok_or(Error::NotAValidator)?; - Ok(Validator { signing_context, key, index }) + let disabled = disabled_validators.iter().any(|d: &ValidatorIndex| *d == index); + + Ok(Validator { signing_context, key, index, disabled }) } /// Get this validator's id. @@ -426,6 +497,11 @@ impl Validator { self.index } + /// Get the enabled/disabled state of this validator + pub fn disabled(&self) -> bool { + self.disabled + } + /// Get the current signing context. pub fn signing_context(&self) -> &SigningContext { &self.signing_context diff --git a/polkadot/node/subsystem-util/src/vstaging.rs b/polkadot/node/subsystem-util/src/vstaging.rs new file mode 100644 index 0000000000000..3efd3b61f93cd --- /dev/null +++ b/polkadot/node/subsystem-util/src/vstaging.rs @@ -0,0 +1,56 @@ +// Copyright (C) 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 . + +//! Contains helpers for staging runtime calls. +//! +//! This module is intended to contain common boiler plate code handling unreleased runtime API +//! calls. + +use polkadot_node_subsystem_types::messages::{RuntimeApiMessage, RuntimeApiRequest}; +use polkadot_overseer::SubsystemSender; +use polkadot_primitives::{Hash, ValidatorIndex}; + +use crate::{has_required_runtime, request_disabled_validators, Error}; + +const LOG_TARGET: &'static str = "parachain::subsystem-util-vstaging"; + +// TODO: https://github.com/paritytech/polkadot-sdk/issues/1940 +/// Returns disabled validators list if the runtime supports it. Otherwise logs a debug messages and +/// returns an empty vec. +/// Once runtime ver `DISABLED_VALIDATORS_RUNTIME_REQUIREMENT` is released remove this function and +/// replace all usages with `request_disabled_validators` +pub async fn get_disabled_validators_with_fallback>( + sender: &mut Sender, + relay_parent: Hash, +) -> Result, Error> { + let disabled_validators = if has_required_runtime( + sender, + relay_parent, + RuntimeApiRequest::DISABLED_VALIDATORS_RUNTIME_REQUIREMENT, + ) + .await + { + request_disabled_validators(relay_parent, sender) + .await + .await + .map_err(Error::Oneshot)?? + } else { + gum::debug!(target: LOG_TARGET, "Runtime doesn't support `DisabledValidators` - continuing with an empty disabled validators set"); + vec![] + }; + + Ok(disabled_validators) +} From bff0240d28ac8e5f91e837d389546883108fe747 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 9 Nov 2023 19:09:14 +0200 Subject: [PATCH 2/5] Fix a compilation error after merge master: `CandidateValidationMessage::ValidateFromExhaustive` now has got named fields --- polkadot/node/core/backing/src/tests/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/polkadot/node/core/backing/src/tests/mod.rs b/polkadot/node/core/backing/src/tests/mod.rs index a1cc8dc73ad55..1703934a1ce04 100644 --- a/polkadot/node/core/backing/src/tests/mod.rs +++ b/polkadot/node/core/backing/src/tests/mod.rs @@ -2216,15 +2216,15 @@ fn validator_ignores_statements_from_disabled_validators() { assert_matches!( virtual_overseer.recv().await, AllMessages::CandidateValidation( - CandidateValidationMessage::ValidateFromExhaustive( - _pvd, - _validation_code, + CandidateValidationMessage::ValidateFromExhaustive{ + validation_data: _pvd, + validation_code: _validation_code, candidate_receipt, - _pov, - _, - timeout, - tx, - ), + pov: _pov, + executor_params: _, + exec_timeout_kind: timeout, + response_sender: tx, + }, ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate.descriptor() && From 5e8b367593822855d78632d973706331db5d9f69 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 29 Nov 2023 17:59:39 +0200 Subject: [PATCH 3/5] Fix compilation errors after merge --- polkadot/node/core/backing/src/tests/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/node/core/backing/src/tests/mod.rs b/polkadot/node/core/backing/src/tests/mod.rs index a748be11c758c..a5e878caba31f 100644 --- a/polkadot/node/core/backing/src/tests/mod.rs +++ b/polkadot/node/core/backing/src/tests/mod.rs @@ -2222,13 +2222,13 @@ fn validator_ignores_statements_from_disabled_validators() { candidate_receipt, pov: _pov, executor_params: _, - exec_timeout_kind: timeout, + exec_kind, response_sender: tx, }, ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + exec_kind == PvfExecKind::Backing && candidate_commitments_hash == candidate_receipt.commitments_hash => { tx.send(Ok( From f2d4c48a867ab20cdf7b7e9ae8c89de1adb0ff28 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 30 Nov 2023 13:16:48 +0200 Subject: [PATCH 4/5] Filter votes from disabled validators in `BackedCandidates` in process_inherent_data (#1863) Fixes https://github.com/paritytech/polkadot-sdk/issues/1592 Please refer to the issue for details. --------- Co-authored-by: ordian Co-authored-by: ordian Co-authored-by: Maciej --- .../src/runtime/parainherent.md | 31 +++ .../runtime/common/src/assigned_slots/mod.rs | 4 +- .../runtime/common/src/integration_tests.rs | 4 +- .../runtime/common/src/paras_registrar/mod.rs | 4 +- polkadot/runtime/parachains/src/mock.rs | 27 ++- .../parachains/src/paras_inherent/mod.rs | 181 ++++++++++++++-- .../parachains/src/paras_inherent/tests.rs | 193 ++++++++++++++++-- .../src/runtime_api_impl/vstaging.rs | 19 +- polkadot/runtime/parachains/src/scheduler.rs | 7 + polkadot/runtime/parachains/src/shared.rs | 42 +++- polkadot/runtime/rococo/src/lib.rs | 4 +- polkadot/runtime/test-runtime/src/lib.rs | 4 +- polkadot/runtime/westend/src/lib.rs | 4 +- polkadot/xcm/xcm-builder/tests/mock/mod.rs | 4 +- .../xcm-simulator/example/src/relay_chain.rs | 4 +- .../xcm-simulator/fuzzer/src/relay_chain.rs | 4 +- substrate/frame/aura/src/mock.rs | 4 + .../contracts/mock-network/src/relay_chain.rs | 4 +- substrate/frame/session/src/lib.rs | 4 + .../frame/support/src/traits/validation.rs | 7 + 20 files changed, 491 insertions(+), 64 deletions(-) diff --git a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md index 4a771f1df6441..9489a286a0cbf 100644 --- a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md +++ b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md @@ -60,3 +60,34 @@ processing it, so the processed inherent data is simply dropped. This also means that the `enter` function keeps data around for no good reason. This seems acceptable though as the size of a block is rather limited. Nevertheless if we ever wanted to optimize this we can easily implement an inherent collector that has two implementations, where one clones and stores the data and the other just passes it on. + +## Data sanitization +`ParasInherent` with the entry point of `create_inherent` sanitizes the input data, while the `enter` entry point +enforces already sanitized input data. If unsanitized data is provided the module generates an error. + +Disputes are included in the block with a priority for a security reasons. It's important to include as many dispute +votes onchain as possible so that disputes conclude faster and the offenders are punished. However if there are too many +disputes to include in a block the dispute set is trimmed so that it respects max block weight. + +Dispute data is first deduplicated and sorted by block number (older first) and dispute location (local then remote). +Concluded and ancient (disputes initiated before the post conclusion acceptance period) disputes are filtered out. +Votes with invalid signatures or from unknown validators (not found in the active set for the current session) are also +filtered out. + +All dispute statements are included in the order described in the previous paragraph until the available block weight is +exhausted. After the dispute data is included all remaining weight is filled in with candidates and availability +bitfields. Bitfields are included with priority, then candidates containing code updates and finally any backed +candidates. If there is not enough weight for all backed candidates they are trimmed by random selection. Disputes are +processed in three separate functions - `deduplicate_and_sort_dispute_data`, `filter_dispute_data` and +`limit_and_sanitize_disputes`. + +Availability bitfields are also sanitized by dropping malformed ones, containing disputed cores or bad signatures. Refer +to `sanitize_bitfields` function for implementation details. + +Backed candidates sanitization removes malformed ones, candidates which have got concluded invalid disputes against them +or candidates produced by unassigned cores. Furthermore any backing votes from disabled validators for a candidate are +dropped. This is part of the validator disabling strategy. After filtering the statements from disabled validators a +backed candidate may end up with votes count less than `minimum_backing_votes` (a parameter from `HostConfiguiration`). +In this case the whole candidate is dropped otherwise it will be rejected by `process_candidates` from pallet inclusion. +All checks related to backed candidates are implemented in `sanitize_backed_candidates` and +`filter_backed_statements_from_disabled_validators`. diff --git a/polkadot/runtime/common/src/assigned_slots/mod.rs b/polkadot/runtime/common/src/assigned_slots/mod.rs index f5e3aaef6324b..efd80a16744da 100644 --- a/polkadot/runtime/common/src/assigned_slots/mod.rs +++ b/polkadot/runtime/common/src/assigned_slots/mod.rs @@ -745,7 +745,9 @@ mod tests { type OnNewHead = (); } - impl parachains_shared::Config for Test {} + impl parachains_shared::Config for Test { + type DisabledValidators = (); + } parameter_types! { pub const LeasePeriod: BlockNumber = 3; diff --git a/polkadot/runtime/common/src/integration_tests.rs b/polkadot/runtime/common/src/integration_tests.rs index b0d277a702d64..825115d82905a 100644 --- a/polkadot/runtime/common/src/integration_tests.rs +++ b/polkadot/runtime/common/src/integration_tests.rs @@ -197,7 +197,9 @@ impl configuration::Config for Test { type WeightInfo = configuration::TestWeightInfo; } -impl shared::Config for Test {} +impl shared::Config for Test { + type DisabledValidators = (); +} impl origin::Config for Test {} diff --git a/polkadot/runtime/common/src/paras_registrar/mod.rs b/polkadot/runtime/common/src/paras_registrar/mod.rs index 12376ae6f1ff7..b966967f21fde 100644 --- a/polkadot/runtime/common/src/paras_registrar/mod.rs +++ b/polkadot/runtime/common/src/paras_registrar/mod.rs @@ -799,7 +799,9 @@ mod tests { type MaxFreezes = ConstU32<1>; } - impl shared::Config for Test {} + impl shared::Config for Test { + type DisabledValidators = (); + } impl origin::Config for Test {} diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index 222942922f911..75fb1ee1ad436 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -184,7 +184,22 @@ impl crate::configuration::Config for Test { type WeightInfo = crate::configuration::TestWeightInfo; } -impl crate::shared::Config for Test {} +pub struct MockDisabledValidators {} +impl frame_support::traits::DisabledValidators for MockDisabledValidators { + /// Returns true if the given validator is disabled. + fn is_disabled(index: u32) -> bool { + disabled_validators().iter().any(|v| *v == index) + } + + /// Returns a hardcoded list (`DISABLED_VALIDATORS`) of disabled validators + fn disabled_validators() -> Vec { + disabled_validators() + } +} + +impl crate::shared::Config for Test { + type DisabledValidators = MockDisabledValidators; +} impl origin::Config for Test {} @@ -432,6 +447,8 @@ thread_local! { pub static AVAILABILITY_REWARDS: RefCell> = RefCell::new(HashMap::new()); + + pub static DISABLED_VALIDATORS: RefCell> = RefCell::new(vec![]); } pub fn backing_rewards() -> HashMap { @@ -442,6 +459,10 @@ pub fn availability_rewards() -> HashMap { AVAILABILITY_REWARDS.with(|r| r.borrow().clone()) } +pub fn disabled_validators() -> Vec { + DISABLED_VALIDATORS.with(|r| r.borrow().clone()) +} + parameter_types! { pub static Processed: Vec<(ParaId, UpwardMessage)> = vec![]; } @@ -581,3 +602,7 @@ pub(crate) fn deregister_parachain(id: ParaId) { pub(crate) fn try_deregister_parachain(id: ParaId) -> crate::DispatchResult { frame_support::storage::transactional::with_storage_layer(|| Paras::schedule_para_cleanup(id)) } + +pub(crate) fn set_disabled_validators(disabled: Vec) { + DISABLED_VALIDATORS.with(|d| *d.borrow_mut() = disabled) +} diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 8e918d35d5ff0..7a4cb8ae31060 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -30,7 +30,8 @@ use crate::{ metrics::METRICS, paras, scheduler::{self, FreedReason}, - shared, ParaId, + shared::{self, AllowedRelayParentsTracker}, + ParaId, }; use bitvec::prelude::BitVec; use frame_support::{ @@ -42,8 +43,8 @@ use frame_support::{ use frame_system::pallet_prelude::*; use pallet_babe::{self, ParentBlockRandomness}; use primitives::{ - BackedCandidate, CandidateHash, CandidateReceipt, CheckedDisputeStatementSet, - CheckedMultiDisputeStatementSet, CoreIndex, DisputeStatementSet, + effective_minimum_backing_votes, BackedCandidate, CandidateHash, CandidateReceipt, + CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CoreIndex, DisputeStatementSet, InherentData as ParachainsInherentData, MultiDisputeStatementSet, ScrapedOnChainVotes, SessionIndex, SignedAvailabilityBitfields, SigningContext, UncheckedSignedAvailabilityBitfield, UncheckedSignedAvailabilityBitfields, ValidatorId, ValidatorIndex, ValidityAttestation, @@ -142,6 +143,8 @@ pub mod pallet { DisputeStatementsUnsortedOrDuplicates, /// A dispute statement was invalid. DisputeInvalid, + /// A candidate was backed by a disabled validator + BackedByDisabled, } /// Whether the paras inherent was included within this block. @@ -378,6 +381,7 @@ impl Pallet { let bitfields_weight = signed_bitfields_weight::(&bitfields); let disputes_weight = multi_dispute_statement_sets_weight::(&disputes); + // Weight before filtering/sanitization let all_weight_before = candidates_weight + bitfields_weight + disputes_weight; METRICS.on_before_filter(all_weight_before.ref_time()); @@ -587,17 +591,19 @@ impl Pallet { METRICS.on_candidates_processed_total(backed_candidates.len() as u64); - let backed_candidates = sanitize_backed_candidates::( - backed_candidates, - |candidate_idx: usize, - backed_candidate: &BackedCandidate<::Hash>| - -> bool { - let para_id = backed_candidate.descriptor().para_id; - let prev_context = >::para_most_recent_context(para_id); - let check_ctx = CandidateCheckContext::::new(prev_context); - - // never include a concluded-invalid candidate - current_concluded_invalid_disputes.contains(&backed_candidate.hash()) || + let SanitizedBackedCandidates { backed_candidates, votes_from_disabled_were_dropped } = + sanitize_backed_candidates::( + backed_candidates, + &allowed_relay_parents, + |candidate_idx: usize, + backed_candidate: &BackedCandidate<::Hash>| + -> bool { + let para_id = backed_candidate.descriptor().para_id; + let prev_context = >::para_most_recent_context(para_id); + let check_ctx = CandidateCheckContext::::new(prev_context); + + // never include a concluded-invalid candidate + current_concluded_invalid_disputes.contains(&backed_candidate.hash()) || // Instead of checking the candidates with code upgrades twice // move the checking up here and skip it in the training wheels fallback. // That way we avoid possible duplicate checks while assuring all @@ -607,12 +613,19 @@ impl Pallet { check_ctx .verify_backed_candidate(&allowed_relay_parents, candidate_idx, backed_candidate) .is_err() - }, - &scheduled, - ); + }, + &scheduled, + ); METRICS.on_candidates_sanitized(backed_candidates.len() as u64); + // In `Enter` context (invoked during execution) there should be no backing votes from + // disabled validators because they should have been filtered out during inherent data + // preparation (`ProvideInherent` context). Abort in such cases. + if context == ProcessInherentDataContext::Enter { + ensure!(!votes_from_disabled_were_dropped, Error::::BackedByDisabled); + } + // Process backed candidates according to scheduled cores. let inclusion::ProcessedCandidates::< as HeaderT>::Hash> { core_indices: occupied, @@ -900,7 +913,19 @@ pub(crate) fn sanitize_bitfields( bitfields } -/// Filter out any candidates that have a concluded invalid dispute. +// Result from `sanitize_backed_candidates` +#[derive(Debug, PartialEq)] +struct SanitizedBackedCandidates { + // Sanitized backed candidates. The `Vec` is sorted according to the occupied core index. + backed_candidates: Vec>, + // Set to true if any votes from disabled validators were dropped from the input. + votes_from_disabled_were_dropped: bool, +} + +/// Filter out: +/// 1. any candidates that have a concluded invalid dispute +/// 2. all backing votes from disabled validators +/// 3. any candidates that end up with less than `effective_minimum_backing_votes` backing votes /// /// `scheduled` follows the same naming scheme as provided in the /// guide: Currently `free` but might become `occupied`. @@ -910,15 +935,17 @@ pub(crate) fn sanitize_bitfields( /// `candidate_has_concluded_invalid_dispute` must return `true` if the candidate /// is disputed, false otherwise. The passed `usize` is the candidate index. /// -/// The returned `Vec` is sorted according to the occupied core index. +/// Returns struct `SanitizedBackedCandidates` where `backed_candidates` are sorted according to the +/// occupied core index. fn sanitize_backed_candidates< T: crate::inclusion::Config, F: FnMut(usize, &BackedCandidate) -> bool, >( mut backed_candidates: Vec>, + allowed_relay_parents: &AllowedRelayParentsTracker>, mut candidate_has_concluded_invalid_dispute_or_is_invalid: F, scheduled: &BTreeMap, -) -> Vec> { +) -> SanitizedBackedCandidates { // Remove any candidates that were concluded invalid. // This does not assume sorting. backed_candidates.indexed_retain(move |candidate_idx, backed_candidate| { @@ -936,6 +963,13 @@ fn sanitize_backed_candidates< scheduled.get(&desc.para_id).is_some() }); + // Filter out backing statements from disabled validators + let dropped_disabled = filter_backed_statements_from_disabled_validators::( + &mut backed_candidates, + &allowed_relay_parents, + scheduled, + ); + // Sort the `Vec` last, once there is a guarantee that these // `BackedCandidates` references the expected relay chain parent, // but more importantly are scheduled for a free core. @@ -946,7 +980,10 @@ fn sanitize_backed_candidates< scheduled[&x.descriptor().para_id].cmp(&scheduled[&y.descriptor().para_id]) }); - backed_candidates + SanitizedBackedCandidates { + backed_candidates, + votes_from_disabled_were_dropped: dropped_disabled, + } } /// Derive entropy from babe provided per block randomness. @@ -1029,3 +1066,105 @@ fn limit_and_sanitize_disputes< (checked, checked_disputes_weight) } } + +// Filters statements from disabled validators in `BackedCandidate`, non-scheduled candidates and +// few more sanity checks. Returns `true` if at least one statement is removed and `false` +// otherwise. +fn filter_backed_statements_from_disabled_validators( + backed_candidates: &mut Vec::Hash>>, + allowed_relay_parents: &AllowedRelayParentsTracker>, + scheduled: &BTreeMap, +) -> bool { + let disabled_validators = + BTreeSet::<_>::from_iter(shared::Pallet::::disabled_validators().into_iter()); + + if disabled_validators.is_empty() { + // No disabled validators - nothing to do + return false + } + + let backed_len_before = backed_candidates.len(); + + // Flag which will be returned. Set to `true` if at least one vote is filtered. + let mut filtered = false; + + let minimum_backing_votes = configuration::Pallet::::config().minimum_backing_votes; + + // Process all backed candidates. `validator_indices` in `BackedCandidates` are indices within + // the validator group assigned to the parachain. To obtain this group we need: + // 1. Core index assigned to the parachain which has produced the candidate + // 2. The relay chain block number of the candidate + backed_candidates.retain_mut(|bc| { + // Get `core_idx` assigned to the `para_id` of the candidate + let core_idx = match scheduled.get(&bc.descriptor().para_id) { + Some(core_idx) => *core_idx, + None => { + log::debug!(target: LOG_TARGET, "Can't get core idx of a backed candidate for para id {:?}. Dropping the candidate.", bc.descriptor().para_id); + return false + } + }; + + // 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 + } + }; + + // Get the group index for the core + let group_idx = match >::group_assigned_to_core( + core_idx, + relay_parent_block_number + One::one(), + ) { + Some(group_idx) => group_idx, + None => { + log::debug!(target: LOG_TARGET, "Can't get the group index for core idx {:?}. Dropping the candidate.", core_idx); + return false + }, + }; + + // And finally get the validator group for this group index + let validator_group = match >::group_validators(group_idx) { + Some(validator_group) => validator_group, + None => { + log::debug!(target: LOG_TARGET, "Can't get the validators from group {:?}. Dropping the candidate.", group_idx); + return false + } + }; + + // Bitmask with the disabled indices within the validator group + let disabled_indices = BitVec::::from_iter(validator_group.iter().map(|idx| disabled_validators.contains(idx))); + // The indices of statements from disabled validators in `BackedCandidate`. We have to drop these. + let indices_to_drop = disabled_indices.clone() & &bc.validator_indices; + // Apply the bitmask to drop the disabled validator from `validator_indices` + bc.validator_indices &= !disabled_indices; + // Remove the corresponding votes from `validity_votes` + for idx in indices_to_drop.iter_ones().rev() { + bc.validity_votes.remove(idx); + } + + // If at least one statement was dropped we need to return `true` + if indices_to_drop.count_ones() > 0 { + filtered = true; + } + + // By filtering votes we might render the candidate invalid and cause a failure in + // [`process_candidates`]. To avoid this we have to perform a sanity check here. If there + // are not enough backing votes after filtering we will remove the whole candidate. + if bc.validity_votes.len() < effective_minimum_backing_votes( + validator_group.len(), + minimum_backing_votes + + ) { + return false + } + + true + }); + + // Also return `true` if a whole candidate was dropped from the set + filtered || backed_len_before != backed_candidates.len() +} diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 4fc60792e3468..364c6192bebed 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1206,6 +1206,12 @@ mod sanitizers { } mod candidates { + use crate::{ + mock::set_disabled_validators, + scheduler::{common::Assignment, ParasEntry}, + }; + use sp_std::collections::vec_deque::VecDeque; + use super::*; // Backed candidates and scheduled parachains used for `sanitize_backed_candidates` testing @@ -1214,10 +1220,20 @@ mod sanitizers { scheduled_paras: BTreeMap, } - // Generate test data for the candidates test + // Generate test data for the candidates and assert that the evnironment is set as expected + // (check the comments for details) fn get_test_data() -> TestData { const RELAY_PARENT_NUM: u32 = 3; + // Add the relay parent to `shared` pallet. Otherwise some code (e.g. filtering backing + // votes) won't behave correctly + shared::Pallet::::add_allowed_relay_parent( + default_header().hash(), + Default::default(), + RELAY_PARENT_NUM, + 1, + ); + let header = default_header(); let relay_parent = header.hash(); let session_index = SessionIndex::from(0_u32); @@ -1231,6 +1247,7 @@ mod sanitizers { keyring::Sr25519Keyring::Bob, keyring::Sr25519Keyring::Charlie, keyring::Sr25519Keyring::Dave, + keyring::Sr25519Keyring::Eve, ]; for validator in validators.iter() { Keystore::sr25519_generate_new( @@ -1241,11 +1258,36 @@ mod sanitizers { .unwrap(); } + // Set active validators in `shared` pallet + let validator_ids = + validators.iter().map(|v| v.public().into()).collect::>(); + shared::Pallet::::set_active_validators_ascending(validator_ids); + + // Two scheduled parachains - ParaId(1) on CoreIndex(0) and ParaId(2) on CoreIndex(1) let scheduled = (0_usize..2) .into_iter() .map(|idx| (ParaId::from(1_u32 + idx as u32), CoreIndex::from(idx as u32))) .collect::>(); + // Set the validator groups in `scheduler` + scheduler::Pallet::::set_validator_groups(vec![ + vec![ValidatorIndex(0), ValidatorIndex(1)], + vec![ValidatorIndex(2), ValidatorIndex(3)], + ]); + + // Update scheduler's claimqueue with the parachains + scheduler::Pallet::::set_claimqueue(BTreeMap::from([ + ( + CoreIndex::from(0), + VecDeque::from([Some(ParasEntry::new(Assignment::new(1.into()), 1))]), + ), + ( + CoreIndex::from(1), + VecDeque::from([Some(ParasEntry::new(Assignment::new(2.into()), 1))]), + ), + ])); + + // Callback used for backing candidates let group_validators = |group_index: GroupIndex| { match group_index { group_index if group_index == GroupIndex::from(0) => Some(vec![0, 1]), @@ -1255,6 +1297,7 @@ mod sanitizers { .map(|m| m.into_iter().map(ValidatorIndex).collect::>()) }; + // Two backed candidates from each parachain let backed_candidates = (0_usize..2) .into_iter() .map(|idx0| { @@ -1283,6 +1326,22 @@ mod sanitizers { }) .collect::>(); + // State sanity checks + assert_eq!( + >::scheduled_paras().collect::>(), + vec![(CoreIndex(0), ParaId::from(1)), (CoreIndex(1), ParaId::from(2))] + ); + assert_eq!( + shared::Pallet::::active_validator_indices(), + vec![ + ValidatorIndex(0), + ValidatorIndex(1), + ValidatorIndex(2), + ValidatorIndex(3), + ValidatorIndex(4) + ] + ); + TestData { backed_candidates, scheduled_paras: scheduled } } @@ -1297,10 +1356,14 @@ mod sanitizers { assert_eq!( sanitize_backed_candidates::( backed_candidates.clone(), + &>::allowed_relay_parents(), has_concluded_invalid, &scheduled ), - backed_candidates + SanitizedBackedCandidates { + backed_candidates, + votes_from_disabled_were_dropped: false + } ); {} @@ -1316,12 +1379,18 @@ mod sanitizers { let has_concluded_invalid = |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; - assert!(sanitize_backed_candidates::( + let SanitizedBackedCandidates { + backed_candidates: sanitized_backed_candidates, + votes_from_disabled_were_dropped, + } = sanitize_backed_candidates::( backed_candidates.clone(), + &>::allowed_relay_parents(), has_concluded_invalid, - &scheduled - ) - .is_empty()); + &scheduled, + ); + + assert!(sanitized_backed_candidates.is_empty()); + assert!(!votes_from_disabled_were_dropped); }); } @@ -1343,15 +1412,113 @@ mod sanitizers { }; let has_concluded_invalid = |_idx: usize, candidate: &BackedCandidate| set.contains(&candidate.hash()); + let SanitizedBackedCandidates { + backed_candidates: sanitized_backed_candidates, + votes_from_disabled_were_dropped, + } = sanitize_backed_candidates::( + backed_candidates.clone(), + &>::allowed_relay_parents(), + has_concluded_invalid, + &scheduled, + ); + + assert_eq!(sanitized_backed_candidates.len(), backed_candidates.len() / 2); + assert!(!votes_from_disabled_were_dropped); + }); + } + + #[test] + fn disabled_non_signing_validator_doesnt_get_filtered() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let TestData { mut backed_candidates, scheduled_paras } = get_test_data(); + + // Disable Eve + set_disabled_validators(vec![4]); + + let before = backed_candidates.clone(); + + // Eve is disabled but no backing statement is signed by it so nothing should be + // filtered + assert!(!filter_backed_statements_from_disabled_validators::( + &mut backed_candidates, + &>::allowed_relay_parents(), + &scheduled_paras + )); + assert_eq!(backed_candidates, before); + }); + } + + #[test] + fn drop_statements_from_disabled_without_dropping_candidate() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let TestData { mut backed_candidates, scheduled_paras } = get_test_data(); + + // Disable Alice + set_disabled_validators(vec![0]); + + // Update `minimum_backing_votes` in HostConfig. We want `minimum_backing_votes` set + // to one so that the candidate will have enough backing votes even after dropping + // Alice's one. + let mut hc = configuration::Pallet::::config(); + hc.minimum_backing_votes = 1; + configuration::Pallet::::force_set_active_config(hc); + + // Verify the initial state is as expected + assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); assert_eq!( - sanitize_backed_candidates::( - backed_candidates.clone(), - has_concluded_invalid, - &scheduled - ) - .len(), - backed_candidates.len() / 2 + backed_candidates.get(0).unwrap().validator_indices.get(0).unwrap(), + true + ); + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(1).unwrap(), + true + ); + let untouched = backed_candidates.get(1).unwrap().clone(); + + assert!(filter_backed_statements_from_disabled_validators::( + &mut backed_candidates, + &>::allowed_relay_parents(), + &scheduled_paras + )); + + // there should still be two backed candidates + assert_eq!(backed_candidates.len(), 2); + // but the first one should have only one validity vote + assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 1); + // Validator 0 vote should be dropped, validator 1 - retained + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(0).unwrap(), + false ); + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(1).unwrap(), + true + ); + // the second candidate shouldn't be modified + assert_eq!(*backed_candidates.get(1).unwrap(), untouched); + }); + } + + #[test] + fn drop_candidate_if_all_statements_are_from_disabled() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let TestData { mut backed_candidates, scheduled_paras } = get_test_data(); + + // Disable Alice and Bob + set_disabled_validators(vec![0, 1]); + + // Verify the initial state is as expected + assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); + let untouched = backed_candidates.get(1).unwrap().clone(); + + assert!(filter_backed_statements_from_disabled_validators::( + &mut backed_candidates, + &>::allowed_relay_parents(), + &scheduled_paras + )); + + assert_eq!(backed_candidates.len(), 1); + assert_eq!(*backed_candidates.get(0).unwrap(), untouched); }); } } diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs b/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs index 200fd57915f9b..6aa455a7d0c70 100644 --- a/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs +++ b/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs @@ -18,29 +18,16 @@ use crate::{configuration, initializer, shared}; use primitives::{vstaging::NodeFeatures, ValidatorIndex}; -use sp_std::{collections::btree_map::BTreeMap, prelude::Vec}; +use sp_std::prelude::Vec; /// Implementation for `DisabledValidators` // CAVEAT: this should only be called on the node side // as it might produce incorrect results on session boundaries pub fn disabled_validators() -> Vec where - T: pallet_session::Config + shared::Config, + T: shared::Config, { - let shuffled_indices = >::active_validator_indices(); - // mapping from raw validator index to `ValidatorIndex` - // this computation is the same within a session, but should be cheap - let reverse_index = shuffled_indices - .iter() - .enumerate() - .map(|(i, v)| (v.0, ValidatorIndex(i as u32))) - .collect::>(); - - // we might have disabled validators who are not parachain validators - >::disabled_validators() - .iter() - .filter_map(|v| reverse_index.get(v).cloned()) - .collect() + >::disabled_validators() } /// Returns the current state of the node features. diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index b81b68b5745ee..e3534ff9c1d20 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -743,4 +743,11 @@ impl Pallet { pub(crate) fn set_validator_groups(validator_groups: Vec>) { ValidatorGroups::::set(validator_groups); } + + #[cfg(test)] + pub(crate) fn set_claimqueue( + claimqueue: BTreeMap>>>>, + ) { + ClaimQueue::::set(claimqueue); + } } diff --git a/polkadot/runtime/parachains/src/shared.rs b/polkadot/runtime/parachains/src/shared.rs index ad13c9e48448f..bdaffcd505f8e 100644 --- a/polkadot/runtime/parachains/src/shared.rs +++ b/polkadot/runtime/parachains/src/shared.rs @@ -19,11 +19,14 @@ //! To avoid cyclic dependencies, it is important that this pallet is not //! dependent on any of the other pallets. -use frame_support::pallet_prelude::*; +use frame_support::{pallet_prelude::*, traits::DisabledValidators}; use frame_system::pallet_prelude::BlockNumberFor; use primitives::{SessionIndex, ValidatorId, ValidatorIndex}; use sp_runtime::traits::AtLeast32BitUnsigned; -use sp_std::{collections::vec_deque::VecDeque, vec::Vec}; +use sp_std::{ + collections::{btree_map::BTreeMap, vec_deque::VecDeque}, + vec::Vec, +}; use rand::{seq::SliceRandom, SeedableRng}; use rand_chacha::ChaCha20Rng; @@ -129,7 +132,9 @@ pub mod pallet { pub struct Pallet(_); #[pallet::config] - pub trait Config: frame_system::Config {} + pub trait Config: frame_system::Config { + type DisabledValidators: frame_support::traits::DisabledValidators; + } /// The current session index. #[pallet::storage] @@ -216,6 +221,25 @@ impl Pallet { Self::session_index().saturating_add(SESSION_DELAY) } + /// Fetches disabled validators list from session pallet. + /// CAVEAT: this might produce incorrect results on session boundaries + pub fn disabled_validators() -> Vec { + let shuffled_indices = Pallet::::active_validator_indices(); + // mapping from raw validator index to `ValidatorIndex` + // this computation is the same within a session, but should be cheap + let reverse_index = shuffled_indices + .iter() + .enumerate() + .map(|(i, v)| (v.0, ValidatorIndex(i as u32))) + .collect::>(); + + // we might have disabled validators who are not parachain validators + T::DisabledValidators::disabled_validators() + .iter() + .filter_map(|v| reverse_index.get(v).cloned()) + .collect() + } + /// Test function for setting the current session index. #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] pub fn set_session_index(index: SessionIndex) { @@ -239,4 +263,16 @@ impl Pallet { ActiveValidatorIndices::::set(indices); ActiveValidatorKeys::::set(keys); } + + #[cfg(test)] + pub(crate) fn add_allowed_relay_parent( + relay_parent: T::Hash, + state_root: T::Hash, + number: BlockNumberFor, + max_ancestry_len: u32, + ) { + AllowedRelayParents::::mutate(|tracker| { + tracker.update(relay_parent, state_root, number, max_ancestry_len) + }) + } } diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 5ac92a737324d..2593cd9ca1069 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -911,7 +911,9 @@ impl parachains_configuration::Config for Runtime { type WeightInfo = weights::runtime_parachains_configuration::WeightInfo; } -impl parachains_shared::Config for Runtime {} +impl parachains_shared::Config for Runtime { + type DisabledValidators = Session; +} impl parachains_session_info::Config for Runtime { type ValidatorSet = Historical; diff --git a/polkadot/runtime/test-runtime/src/lib.rs b/polkadot/runtime/test-runtime/src/lib.rs index 596e65eca0680..0abc780dee861 100644 --- a/polkadot/runtime/test-runtime/src/lib.rs +++ b/polkadot/runtime/test-runtime/src/lib.rs @@ -486,7 +486,9 @@ impl parachains_configuration::Config for Runtime { type WeightInfo = parachains_configuration::TestWeightInfo; } -impl parachains_shared::Config for Runtime {} +impl parachains_shared::Config for Runtime { + type DisabledValidators = Session; +} impl parachains_inclusion::Config for Runtime { type RuntimeEvent = RuntimeEvent; diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index f7ff2d5e9e1bd..4211c073cbfda 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1130,7 +1130,9 @@ impl parachains_configuration::Config for Runtime { type WeightInfo = weights::runtime_parachains_configuration::WeightInfo; } -impl parachains_shared::Config for Runtime {} +impl parachains_shared::Config for Runtime { + type DisabledValidators = Session; +} impl parachains_session_info::Config for Runtime { type ValidatorSet = Historical; diff --git a/polkadot/xcm/xcm-builder/tests/mock/mod.rs b/polkadot/xcm/xcm-builder/tests/mock/mod.rs index 968b294c6a434..5427bffe32e99 100644 --- a/polkadot/xcm/xcm-builder/tests/mock/mod.rs +++ b/polkadot/xcm/xcm-builder/tests/mock/mod.rs @@ -126,7 +126,9 @@ impl pallet_balances::Config for Runtime { type MaxFreezes = ConstU32<0>; } -impl shared::Config for Runtime {} +impl shared::Config for Runtime { + type DisabledValidators = (); +} impl configuration::Config for Runtime { type WeightInfo = configuration::TestWeightInfo; diff --git a/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs b/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs index 20070d192b545..1d1ee385d31ea 100644 --- a/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs +++ b/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs @@ -120,7 +120,9 @@ impl pallet_uniques::Config for Runtime { type Helper = (); } -impl shared::Config for Runtime {} +impl shared::Config for Runtime { + type DisabledValidators = (); +} impl configuration::Config for Runtime { type WeightInfo = configuration::TestWeightInfo; diff --git a/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs b/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs index 085773f30737b..572cee3db536e 100644 --- a/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs +++ b/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs @@ -98,7 +98,9 @@ impl pallet_balances::Config for Runtime { type MaxFreezes = ConstU32<0>; } -impl shared::Config for Runtime {} +impl shared::Config for Runtime { + type DisabledValidators = (); +} impl configuration::Config for Runtime { type WeightInfo = configuration::TestWeightInfo; diff --git a/substrate/frame/aura/src/mock.rs b/substrate/frame/aura/src/mock.rs index 14b87089ce391..d38a8583819e3 100644 --- a/substrate/frame/aura/src/mock.rs +++ b/substrate/frame/aura/src/mock.rs @@ -96,6 +96,10 @@ impl DisabledValidators for MockDisabledValidators { fn is_disabled(index: AuthorityIndex) -> bool { DisabledValidatorTestValue::get().binary_search(&index).is_ok() } + + fn disabled_validators() -> Vec { + DisabledValidatorTestValue::get() + } } impl pallet_aura::Config for Test { diff --git a/substrate/frame/contracts/mock-network/src/relay_chain.rs b/substrate/frame/contracts/mock-network/src/relay_chain.rs index 17e36eada2598..cc6b2953a6660 100644 --- a/substrate/frame/contracts/mock-network/src/relay_chain.rs +++ b/substrate/frame/contracts/mock-network/src/relay_chain.rs @@ -97,7 +97,9 @@ impl pallet_balances::Config for Runtime { type RuntimeFreezeReason = RuntimeFreezeReason; } -impl shared::Config for Runtime {} +impl shared::Config for Runtime { + type DisabledValidators = (); +} impl configuration::Config for Runtime { type WeightInfo = configuration::TestWeightInfo; diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index bf4671a247f0d..178d43f596b27 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -918,6 +918,10 @@ impl frame_support::traits::DisabledValidators for Pallet { fn is_disabled(index: u32) -> bool { >::disabled_validators().binary_search(&index).is_ok() } + + fn disabled_validators() -> Vec { + >::disabled_validators() + } } /// Wraps the author-scraping logic for consensus engines that can recover diff --git a/substrate/frame/support/src/traits/validation.rs b/substrate/frame/support/src/traits/validation.rs index 617cdb2d3f461..4b099b2c766fe 100644 --- a/substrate/frame/support/src/traits/validation.rs +++ b/substrate/frame/support/src/traits/validation.rs @@ -251,10 +251,17 @@ pub trait ValidatorRegistration { pub trait DisabledValidators { /// Returns true if the given validator is disabled. fn is_disabled(index: u32) -> bool; + + /// Returns all disabled validators + fn disabled_validators() -> Vec; } impl DisabledValidators for () { fn is_disabled(_index: u32) -> bool { false } + + fn disabled_validators() -> Vec { + vec![] + } } From 93f5189a9e1592eb86e50ab844312b4f86d03839 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 5 Jan 2024 15:34:34 +0200 Subject: [PATCH 5/5] Fix a compilation error from merge --- .../runtime/parachains/src/paras_inherent/tests.rs | 10 ++++++++-- polkadot/runtime/parachains/src/scheduler.rs | 4 +--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 6ffa1092d7a1c..6f3eac35685a8 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1300,11 +1300,17 @@ mod sanitizers { scheduler::Pallet::::set_claimqueue(BTreeMap::from([ ( CoreIndex::from(0), - VecDeque::from([Some(ParasEntry::new(Assignment::new(1.into()), 1))]), + VecDeque::from([ParasEntry::new( + Assignment::Pool { para_id: 1.into(), core_index: CoreIndex(1) }, + RELAY_PARENT_NUM, + )]), ), ( CoreIndex::from(1), - VecDeque::from([Some(ParasEntry::new(Assignment::new(2.into()), 1))]), + VecDeque::from([ParasEntry::new( + Assignment::Pool { para_id: 2.into(), core_index: CoreIndex(1) }, + RELAY_PARENT_NUM, + )]), ), ])); diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index 5224dc3cd70d2..a666f5689089a 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -693,9 +693,7 @@ impl Pallet { } #[cfg(test)] - pub(crate) fn set_claimqueue( - claimqueue: BTreeMap>>>>, - ) { + pub(crate) fn set_claimqueue(claimqueue: BTreeMap>>) { ClaimQueue::::set(claimqueue); } }