paras: dismiss pvf_checking_enabled configuration (#7138)

* paras: unconditionally precheck pvfs

* Update integration tests

* paras_registrar tests

* runtime benchmark tests

* fix bench

* bypass prechecking in test node

* adjust bench

* ".git/.scripts/commands/bench/bench.sh" runtime polkadot runtime_parachains::paras

* ".git/.scripts/commands/bench/bench.sh" runtime kusama runtime_parachains::paras

* ".git/.scripts/commands/bench/bench.sh" runtime rococo runtime_parachains::paras

* ".git/.scripts/commands/bench/bench.sh" runtime westend runtime_parachains::paras

* use test helper

* fix new test

---------

Co-authored-by: command-bot <>

polkadot/runtime/parachains/src/builder.rs

@@ -340,16 +340,22 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
 		// make sure parachains exist prior to session change.
 		for i in 0..cores {
 			let para_id = ParaId::from(i as u32);
+			let validation_code = mock_validation_code();
 
 			paras::Pallet::<T>::schedule_para_initialize(
 				para_id,
 				paras::ParaGenesisArgs {
 					genesis_head: Self::mock_head_data(),
-					validation_code: mock_validation_code(),
+					validation_code: validation_code.clone(),
 					para_kind: ParaKind::Parachain,
 				},
 			)
 			.unwrap();
+			paras::Pallet::<T>::add_trusted_validation_code(
+				frame_system::Origin::<T>::Root.into(),
+				validation_code,
+			)
+			.unwrap();
 		}
 	}
 

polkadot/runtime/parachains/src/hrmp/tests.rs

@@ -23,7 +23,7 @@ use crate::{
 	paras::ParaKind,
 };
 use frame_support::{assert_noop, assert_ok, traits::Currency as _};
-use primitives::BlockNumber;
+use primitives::{BlockNumber, ValidationCode};
 use std::collections::BTreeMap;
 
 fn run_to_block(to: BlockNumber, new_session: Option<Vec<BlockNumber>>) {
@@ -130,14 +130,17 @@ fn default_genesis_config() -> MockGenesisConfig {
 }
 
 fn register_parachain_with_balance(id: ParaId, balance: Balance) {
+	let validation_code: ValidationCode = vec![1].into();
 	assert_ok!(Paras::schedule_para_initialize(
 		id,
 		crate::paras::ParaGenesisArgs {
 			para_kind: ParaKind::Parachain,
 			genesis_head: vec![1].into(),
-			validation_code: vec![1].into(),
+			validation_code: validation_code.clone(),
 		},
 	));
+
+	assert_ok!(Paras::add_trusted_validation_code(RuntimeOrigin::root(), validation_code));
 	<Test as Config>::Currency::make_free_balance_be(&id.into_account_truncating(), balance);
 }
 

polkadot/runtime/parachains/src/paras/benchmarking.rs

@@ -137,6 +137,8 @@ benchmarks! {
 	add_trusted_validation_code {
 		let c in 1 .. MAX_CODE_SIZE;
 		let new_code = ValidationCode(vec![0; c as usize]);
+
+		pvf_check::prepare_bypassing_bench::<T>(new_code.clone());
 	}: _(RawOrigin::Root, new_code)
 
 	poke_unused_validation_code {

polkadot/runtime/parachains/src/paras/benchmarking/pvf_check.rs

@@ -17,6 +17,7 @@
 //! This module focuses on the benchmarking of the `include_pvf_check_statement` dispatchable.
 
 use crate::{configuration, paras::*, shared::Pallet as ParasShared};
+use frame_support::assert_ok;
 use frame_system::RawOrigin;
 use primitives::{HeadData, Id as ParaId, ValidationCode, ValidatorId, ValidatorIndex};
 use sp_application_crypto::RuntimeAppPublic;
@@ -43,7 +44,7 @@ where
 {
 	initialize::<T>();
 	// we do not plan to trigger finalization, thus the cause is inconsequential.
-	initialize_pvf_active_vote::<T>(VoteCause::Onboarding);
+	initialize_pvf_active_vote::<T>(VoteCause::Onboarding, CAUSES_NUM);
 
 	// `unwrap` cannot panic here since the `initialize` function should initialize validators count
 	// to be more than 0.
@@ -68,7 +69,7 @@ where
 	T: Config + shared::Config,
 {
 	initialize::<T>();
-	initialize_pvf_active_vote::<T>(cause);
+	initialize_pvf_active_vote::<T>(cause, CAUSES_NUM);
 
 	let mut stmts = generate_statements::<T>(outcome).collect::<Vec<_>>();
 	// this should be ensured by the `initialize` function.
@@ -85,6 +86,29 @@ where
 	stmt_n_sig
 }
 
+/// Prepares storage for invoking `add_trusted_validation_code` with several paras initializing to
+/// the same code.
+pub fn prepare_bypassing_bench<T>(validation_code: ValidationCode)
+where
+	T: Config + shared::Config,
+{
+	// Suppose a sensible number of paras initialize to the same code.
+	const PARAS_NUM: usize = 10;
+
+	initialize::<T>();
+	for i in 0..PARAS_NUM {
+		let id = ParaId::from(i as u32);
+		assert_ok!(Pallet::<T>::schedule_para_initialize(
+			id,
+			ParaGenesisArgs {
+				para_kind: ParaKind::Parachain,
+				genesis_head: HeadData(vec![1, 2, 3, 4]),
+				validation_code: validation_code.clone(),
+			},
+		));
+	}
+}
+
 /// What caused the PVF pre-checking vote?
 #[derive(PartialEq, Eq, Copy, Clone, Debug)]
 pub enum VoteCause {
@@ -122,11 +146,11 @@ where
 ///
 /// The subject of the vote (i.e. validation code) and the cause (upgrade/onboarding) is specified
 /// by the test setup.
-fn initialize_pvf_active_vote<T>(vote_cause: VoteCause)
+fn initialize_pvf_active_vote<T>(vote_cause: VoteCause, causes_num: usize)
 where
 	T: Config + shared::Config,
 {
-	for i in 0..CAUSES_NUM {
+	for i in 0..causes_num {
 		let id = ParaId::from(i as u32);
 
 		if vote_cause == VoteCause::Upgrade {

polkadot/runtime/parachains/src/paras/mod.rs

@@ -501,7 +501,8 @@ impl WeightInfo for TestWeightInfo {
 		Weight::MAX
 	}
 	fn add_trusted_validation_code(_c: u32) -> Weight {
-		Weight::MAX
+		// Called during integration tests for para initialization.
+		Weight::zero()
 	}
 	fn poke_unused_validation_code() -> Weight {
 		Weight::MAX
@@ -602,9 +603,6 @@ pub mod pallet {
 		PvfCheckDoubleVote,
 		/// The given PVF does not exist at the moment of process a vote.
 		PvfCheckSubjectInvalid,
-		/// The PVF pre-checking statement cannot be included since the PVF pre-checking mechanism
-		/// is disabled.
-		PvfCheckDisabled,
 		/// Parachain cannot currently schedule a code upgrade.
 		CannotUpgradeCode,
 	}
@@ -970,12 +968,6 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			ensure_none(origin)?;
 
-			// Make sure that PVF pre-checking is enabled.
-			ensure!(
-				configuration::Pallet::<T>::config().pvf_checking_enabled,
-				Error::<T>::PvfCheckDisabled,
-			);
-
 			let validators = shared::Pallet::<T>::active_validator_keys();
 			let current_session = shared::Pallet::<T>::session_index();
 			if stmt.session_index < current_session {
@@ -1062,10 +1054,6 @@ pub mod pallet {
 				_ => return InvalidTransaction::Call.into(),
 			};
 
-			if !configuration::Pallet::<T>::config().pvf_checking_enabled {
-				return InvalidTransaction::Custom(INVALID_TX_PVF_CHECK_DISABLED).into()
-			}
-
 			let current_session = shared::Pallet::<T>::session_index();
 			if stmt.session_index < current_session {
 				return InvalidTransaction::Stale.into()
@@ -1126,7 +1114,6 @@ pub mod pallet {
 const INVALID_TX_BAD_VALIDATOR_IDX: u8 = 1;
 const INVALID_TX_BAD_SUBJECT: u8 = 2;
 const INVALID_TX_DOUBLE_VOTE: u8 = 3;
-const INVALID_TX_PVF_CHECK_DISABLED: u8 = 4;
 
 impl<T: Config> Pallet<T> {
 	/// This is a call to schedule code upgrades for parachains which is safe to be called
@@ -1828,9 +1815,7 @@ impl<T: Config> Pallet<T> {
 	/// Makes sure that the given code hash has passed pre-checking.
 	///
 	/// If the given code hash has already passed pre-checking, then the approval happens
-	/// immediately. Similarly, if the pre-checking is turned off, the update is scheduled immediately
-	/// as well. In this case, the behavior is similar to the previous, i.e. the upgrade sequence
-	/// is purely time-based.
+	/// immediately.
 	///
 	/// If the code is unknown, but the pre-checking for that PVF is already running then we perform
 	/// "coalescing". We save the cause for this PVF pre-check request and just add it to the
@@ -1859,12 +1844,9 @@ impl<T: Config> Pallet<T> {
 				let known_code = CodeByHash::<T>::contains_key(&code_hash);
 				weight += T::DbWeight::get().reads(1);
 
-				if !cfg.pvf_checking_enabled || known_code {
-					// Either:
-					// - the code is known and there is no active PVF vote for it meaning it is
-					//   already checked, or
-					// - the PVF checking is diabled
-					// In any case: fast track the PVF checking into the accepted state
+				if known_code {
+					// The code is known and there is no active PVF vote for it meaning it is
+					// already checked -- fast track the PVF checking into the accepted state.
 					weight += T::DbWeight::get().reads(1);
 					let now = <frame_system::Pallet<T>>::block_number();
 					weight += Self::enact_pvf_accepted(now, &code_hash, &[cause], 0, cfg);

polkadot/runtime/parachains/src/paras/tests.rs

@@ -55,6 +55,22 @@ fn sign_and_include_pvf_check_statement(stmt: PvfCheckStatement) {
 	Paras::include_pvf_check_statement(None.into(), stmt, signature.into()).unwrap();
 }
 
+fn submit_super_majority_pvf_votes(
+	validation_code: &ValidationCode,
+	session_index: SessionIndex,
+	accept: bool,
+) {
+	[0, 1, 2, 3]
+		.into_iter()
+		.map(|i| PvfCheckStatement {
+			accept,
+			subject: validation_code.hash(),
+			session_index,
+			validator_index: i.into(),
+		})
+		.for_each(sign_and_include_pvf_check_statement);
+}
+
 fn run_to_block(to: BlockNumber, new_session: Option<Vec<BlockNumber>>) {
 	let keystore: KeystorePtr = Arc::new(LocalKeystore::in_memory());
 	for validator in VALIDATORS.iter() {
@@ -420,7 +436,9 @@ fn code_upgrade_applied_after_delay() {
 		let para_id = ParaId::from(0);
 		let new_code = ValidationCode(vec![4, 5, 6]);
 
-		run_to_block(2, None);
+		// Wait for at least one session change to set active validators.
+		const EXPECTED_SESSION: SessionIndex = 1;
+		run_to_block(2, Some(vec![1]));
 		assert_eq!(Paras::current_code(&para_id), Some(original_code.clone()));
 
 		let expected_at = {
@@ -428,6 +446,9 @@ fn code_upgrade_applied_after_delay() {
 			let expected_at = 1 + validation_upgrade_delay;
 			let next_possible_upgrade_at = 1 + validation_upgrade_cooldown;
 			Paras::schedule_code_upgrade(para_id, new_code.clone(), 1, &Configuration::config());
+			// Include votes for super-majority.
+			submit_super_majority_pvf_votes(&new_code, EXPECTED_SESSION, true);
+
 			Paras::note_new_head(para_id, Default::default(), 1);
 
 			assert!(Paras::past_code_meta(&para_id).most_recent_change().is_none());
@@ -515,7 +536,9 @@ fn code_upgrade_applied_after_delay_even_when_late() {
 		let para_id = ParaId::from(0);
 		let new_code = ValidationCode(vec![4, 5, 6]);
 
-		run_to_block(2, None);
+		// Wait for at least one session change to set active validators.
+		const EXPECTED_SESSION: SessionIndex = 1;
+		run_to_block(2, Some(vec![1]));
 		assert_eq!(Paras::current_code(&para_id), Some(original_code.clone()));
 
 		let expected_at = {
@@ -523,6 +546,9 @@ fn code_upgrade_applied_after_delay_even_when_late() {
 			let expected_at = 1 + validation_upgrade_delay;
 			let next_possible_upgrade_at = 1 + validation_upgrade_cooldown;
 			Paras::schedule_code_upgrade(para_id, new_code.clone(), 1, &Configuration::config());
+			// Include votes for super-majority.
+			submit_super_majority_pvf_votes(&new_code, EXPECTED_SESSION, true);
+
 			Paras::note_new_head(para_id, Default::default(), 1);
 
 			assert!(Paras::past_code_meta(&para_id).most_recent_change().is_none());
@@ -595,8 +621,14 @@ fn submit_code_change_when_not_allowed_is_err() {
 		let new_code = ValidationCode(vec![4, 5, 6]);
 		let newer_code = ValidationCode(vec![4, 5, 6, 7]);
 
-		run_to_block(1, None);
+		// Wait for at least one session change to set active validators.
+		const EXPECTED_SESSION: SessionIndex = 1;
+		run_to_block(1, Some(vec![1]));
+
 		Paras::schedule_code_upgrade(para_id, new_code.clone(), 1, &Configuration::config());
+		// Include votes for super-majority.
+		submit_super_majority_pvf_votes(&new_code, EXPECTED_SESSION, true);
+
 		assert_eq!(FutureCodeUpgrades::<Test>::get(&para_id), Some(1 + validation_upgrade_delay));
 		assert_eq!(FutureCodeHash::<Test>::get(&para_id), Some(new_code.hash()));
 		check_code_is_stored(&new_code);
@@ -625,7 +657,7 @@ fn upgrade_restriction_elapsed_doesnt_mean_can_upgrade() {
 	// rather an artifact of the current implementation and not necessarily something we want
 	// to keep in the future.
 	//
-	// This test exists that this is not accidentially changed.
+	// This test exists that this is not accidentally changed.
 
 	let code_retention_period = 10;
 	let validation_upgrade_delay = 7;
@@ -660,8 +692,14 @@ fn upgrade_restriction_elapsed_doesnt_mean_can_upgrade() {
 		let new_code = ValidationCode(vec![4, 5, 6]);
 		let newer_code = ValidationCode(vec![4, 5, 6, 7]);
 
-		run_to_block(1, None);
+		// Wait for at least one session change to set active validators.
+		const EXPECTED_SESSION: SessionIndex = 1;
+		run_to_block(1, Some(vec![1]));
+
 		Paras::schedule_code_upgrade(para_id, new_code.clone(), 0, &Configuration::config());
+		// Include votes for super-majority.
+		submit_super_majority_pvf_votes(&new_code, EXPECTED_SESSION, true);
+
 		Paras::note_new_head(para_id, dummy_head_data(), 0);
 		assert_eq!(
 			UpgradeRestrictionSignal::<Test>::get(&para_id),
@@ -722,7 +760,10 @@ fn full_parachain_cleanup_storage() {
 		let para_id = ParaId::from(0);
 		let new_code = ValidationCode(vec![4, 5, 6]);
 
-		run_to_block(2, None);
+		// Wait for at least one session change to set active validators.
+		const EXPECTED_SESSION: SessionIndex = 1;
+		run_to_block(2, Some(vec![1]));
+
 		assert_eq!(Paras::current_code(&para_id), Some(original_code.clone()));
 		check_code_is_stored(&original_code);
 
@@ -730,6 +771,9 @@ fn full_parachain_cleanup_storage() {
 			// this parablock is in the context of block 1.
 			let expected_at = 1 + validation_upgrade_delay;
 			Paras::schedule_code_upgrade(para_id, new_code.clone(), 1, &Configuration::config());
+			// Include votes for super-majority.
+			submit_super_majority_pvf_votes(&new_code, EXPECTED_SESSION, true);
+
 			Paras::note_new_head(para_id, Default::default(), 1);
 
 			assert!(Paras::past_code_meta(&para_id).most_recent_change().is_none());
@@ -832,14 +876,7 @@ fn cannot_offboard_ongoing_pvf_check() {
 		assert_err!(Paras::schedule_para_cleanup(para_id), Error::<Test>::CannotOffboard);
 
 		// Include votes for super-majority.
-		IntoIterator::into_iter([0, 1, 2, 3])
-			.map(|i| PvfCheckStatement {
-				accept: true,
-				subject: new_code.hash(),
-				session_index: EXPECTED_SESSION,
-				validator_index: i.into(),
-			})
-			.for_each(sign_and_include_pvf_check_statement);
+		submit_super_majority_pvf_votes(&new_code, EXPECTED_SESSION, true);
 
 		// Voting concluded, can offboard even though an upgrade is in progress.
 		assert_ok!(Paras::schedule_para_cleanup(para_id));
@@ -985,10 +1022,16 @@ fn code_hash_at_returns_up_to_end_of_code_retention_period() {
 	};
 
 	new_test_ext(genesis_config).execute_with(|| {
+		// Wait for at least one session change to set active validators.
+		run_to_block(2, Some(vec![1]));
+		const EXPECTED_SESSION: SessionIndex = 1;
+
 		let para_id = ParaId::from(0);
 		let old_code: ValidationCode = vec![1, 2, 3].into();
 		let new_code: ValidationCode = vec![4, 5, 6].into();
 		Paras::schedule_code_upgrade(para_id, new_code.clone(), 0, &Configuration::config());
+		// Include votes for super-majority.
+		submit_super_majority_pvf_votes(&new_code, EXPECTED_SESSION, true);
 
 		// The new validation code can be applied but a new parablock hasn't gotten in yet,
 		// so the old code should still be current.
@@ -998,7 +1041,7 @@ fn code_hash_at_returns_up_to_end_of_code_retention_period() {
 		run_to_block(10, None);
 		Paras::note_new_head(para_id, Default::default(), 7);
 
-		assert_eq!(Paras::past_code_meta(&para_id).upgrade_times, vec![upgrade_at(2, 10)]);
+		assert_eq!(Paras::past_code_meta(&para_id).upgrade_times, vec![upgrade_at(4, 10)]);
 		assert_eq!(Paras::current_code(&para_id), Some(new_code.clone()));
 
 		// Make sure that the old code is available **before** the code retion period passes.
@@ -1103,14 +1146,7 @@ fn pvf_check_coalescing_onboarding_and_upgrade() {
 		assert!(!Paras::pvfs_require_precheck().is_empty());
 
 		// Supermajority of validators vote for `validation_code`. It should be approved.
-		IntoIterator::into_iter([0, 1, 2, 3])
-			.map(|i| PvfCheckStatement {
-				accept: true,
-				subject: validation_code.hash(),
-				session_index: EXPECTED_SESSION,
-				validator_index: i.into(),
-			})
-			.for_each(sign_and_include_pvf_check_statement);
+		submit_super_majority_pvf_votes(&validation_code, EXPECTED_SESSION, true);
 
 		// Check that `b` actually onboards.
 		assert_eq!(ActionsQueue::<Test>::get(EXPECTED_SESSION + 2), vec![b]);
@@ -1253,47 +1289,6 @@ fn pvf_check_upgrade_reject() {
 	});
 }
 
-#[test]
-fn pvf_check_submit_vote_while_disabled() {
-	let genesis_config = MockGenesisConfig {
-		configuration: crate::configuration::GenesisConfig {
-			config: HostConfiguration { pvf_checking_enabled: false, ..Default::default() },
-			..Default::default()
-		},
-		..Default::default()
-	};
-
-	new_test_ext(genesis_config).execute_with(|| {
-		// This will set the session index to 1 and seed the validators.
-		run_to_block(1, Some(vec![1]));
-
-		let stmt = PvfCheckStatement {
-			accept: false,
-			subject: ValidationCode(vec![1, 2, 3]).hash(),
-			session_index: 1,
-			validator_index: 1.into(),
-		};
-
-		let signature: ValidatorSignature =
-			Sr25519Keyring::Alice.sign(&stmt.signing_payload()).into();
-
-		let call =
-			Call::include_pvf_check_statement { stmt: stmt.clone(), signature: signature.clone() };
-
-		let validate_unsigned =
-			<Paras as ValidateUnsigned>::validate_unsigned(TransactionSource::InBlock, &call);
-		assert_eq!(
-			validate_unsigned,
-			InvalidTransaction::Custom(INVALID_TX_PVF_CHECK_DISABLED).into()
-		);
-
-		assert_err!(
-			Paras::include_pvf_check_statement(None.into(), stmt.clone(), signature.clone()),
-			Error::<Test>::PvfCheckDisabled
-		);
-	});
-}
-
 #[test]
 fn pvf_check_submit_vote() {
 	let code_a: ValidationCode = vec![3, 2, 1].into();

polkadot/runtime/parachains/src/scheduler/tests.rs

@@ -18,26 +18,30 @@ use super::*;
 
 use frame_support::assert_ok;
 use keyring::Sr25519Keyring;
-use primitives::{BlockNumber, CollatorId, SessionIndex, ValidatorId};
+use primitives::{BlockNumber, CollatorId, SessionIndex, ValidationCode, ValidatorId};
 
 use crate::{
 	configuration::HostConfiguration,
 	initializer::SessionChangeNotification,
 	mock::{
-		new_test_ext, Configuration, MockGenesisConfig, Paras, ParasShared, Scheduler, System, Test,
+		new_test_ext, Configuration, MockGenesisConfig, Paras, ParasShared, RuntimeOrigin,
+		Scheduler, System, Test,
 	},
 	paras::{ParaGenesisArgs, ParaKind},
 };
 
 fn schedule_blank_para(id: ParaId, parakind: ParaKind) {
+	let validation_code: ValidationCode = vec![1, 2, 3].into();
 	assert_ok!(Paras::schedule_para_initialize(
 		id,
 		ParaGenesisArgs {
 			genesis_head: Vec::new().into(),
-			validation_code: vec![1, 2, 3].into(),
+			validation_code: validation_code.clone(),
 			para_kind: parakind,
 		}
 	));
+
+	assert_ok!(Paras::add_trusted_validation_code(RuntimeOrigin::root(), validation_code));
 }
 
 fn run_to_block(

polkadot/runtime/parachains/src/shared.rs

@@ -123,8 +123,8 @@ impl<T: Config> Pallet<T> {
 		CurrentSessionIndex::<T>::set(index);
 	}
 
-	#[cfg(any(feature = "runtime-benchmarks", test))]
-	pub(crate) fn set_active_validators_ascending(active: Vec<ValidatorId>) {
+	#[cfg(any(feature = "std", feature = "runtime-benchmarks", test))]
+	pub fn set_active_validators_ascending(active: Vec<ValidatorId>) {
 		ActiveValidatorIndices::<T>::set(
 			(0..active.len()).map(|i| ValidatorIndex(i as _)).collect(),
 		);