paras: fix upgrade restriction signal

Closes #3971

Read the linked issue.

Apart from that, this addresses the concern raised in this [comment] by
just adding a test. I couldn't find a clean way to reconcile a block
number delay with a PVF voting TTL, so I just resorted to rely on the
test. Should be fine for now.

[comment]: #4457 (comment)

runtime/parachains/src/configuration.rs

@@ -73,12 +73,26 @@ pub struct HostConfiguration<BlockNumber> {
 	pub hrmp_max_message_num_per_candidate: u32,
 	/// The minimum period, in blocks, between which parachains can update their validation code.
 	///
+	/// This number is used to prevent parachains from spamming the relay chain with validation code
+	/// upgrades. The only thing it controls is the number of blocks the `UpgradeRestrictionSignal`
+	/// is set for the parachain in question.
+	///
 	/// If PVF pre-checking is enabled this should be greater than the maximum number of blocks
 	/// PVF pre-checking can take. Intuitively, this number should be greater than the duration
 	/// specified by [`pvf_voting_ttl`]. Unlike, [`pvf_voting_ttl`], this parameter uses blocks
 	/// as a unit.
 	pub validation_upgrade_frequency: BlockNumber,
 	/// The delay, in blocks, before a validation upgrade is applied.
+	///
+	/// The delay is counted from the relay-parent of the candidate that signalled the upgrade. The
+	/// sum of that block and the this delay is dubbed `expected_at`. The first parachain block with
+	/// relay-parent >= `expected_at` will run the current code and will apply the upgrade. The next
+	/// parachain block will run the upgraded validation code.
+	///
+	/// At some point we are going to change this to be counted from the moment when the candidate
+	/// that signalled the upgrade is enacted. See the issue [#4601].
+	///
+	/// [#4601]: https://github.com/paritytech/polkadot/issues/4601
 	pub validation_upgrade_delay: BlockNumber,
 
 	/**

runtime/parachains/src/dmp.rs

@@ -238,7 +238,7 @@ mod tests {
 	pub(crate) fn run_to_block(to: BlockNumber, new_session: Option<Vec<BlockNumber>>) {
 		while System::block_number() < to {
 			let b = System::block_number();
-			Paras::initializer_finalize();
+			Paras::initializer_finalize(b);
 			Dmp::initializer_finalize();
 			if new_session.as_ref().map_or(false, |v| v.contains(&(b + 1))) {
 				Dmp::initializer_on_new_session(&Default::default(), &Vec::new());
@@ -248,7 +248,7 @@ mod tests {
 			System::on_initialize(b + 1);
 			System::set_block_number(b + 1);
 
-			Paras::initializer_finalize();
+			Paras::initializer_finalize(b + 1);
 			Dmp::initializer_initialize(b + 1);
 		}
 	}

runtime/parachains/src/hrmp.rs

@@ -1281,7 +1281,7 @@ mod tests {
 
 			// NOTE: this is in reverse initialization order.
 			Hrmp::initializer_finalize();
-			Paras::initializer_finalize();
+			Paras::initializer_finalize(b);
 			ParasShared::initializer_finalize();
 
 			if new_session.as_ref().map_or(false, |v| v.contains(&(b + 1))) {

runtime/parachains/src/inclusion/tests.rs

@@ -170,7 +170,7 @@ pub(crate) fn run_to_block(
 		let b = System::block_number();
 
 		ParaInclusion::initializer_finalize();
-		Paras::initializer_finalize();
+		Paras::initializer_finalize(b);
 		ParasShared::initializer_finalize();
 
 		if let Some(notification) = new_session(b + 1) {

runtime/parachains/src/initializer.rs

@@ -168,7 +168,7 @@ pub mod pallet {
 			total_weight
 		}
 
-		fn on_finalize(_: T::BlockNumber) {
+		fn on_finalize(now: T::BlockNumber) {
 			// reverse initialization order.
 			hrmp::Pallet::<T>::initializer_finalize();
 			ump::Pallet::<T>::initializer_finalize();
@@ -177,7 +177,7 @@ pub mod pallet {
 			session_info::Pallet::<T>::initializer_finalize();
 			inclusion::Pallet::<T>::initializer_finalize();
 			scheduler::Pallet::<T>::initializer_finalize();
-			paras::Pallet::<T>::initializer_finalize();
+			paras::Pallet::<T>::initializer_finalize(now);
 			shared::Pallet::<T>::initializer_finalize();
 			configuration::Pallet::<T>::initializer_finalize();
 

runtime/parachains/src/paras.rs

@@ -923,7 +923,9 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// Called by the initializer to finalize the configuration pallet.
-	pub(crate) fn initializer_finalize() {}
+	pub(crate) fn initializer_finalize(now: T::BlockNumber) {
+		Self::process_scheduled_upgrade_cooldowns(now);
+	}
 
 	/// Called by the initializer to note that a new session has started.
 	///
@@ -1151,10 +1153,13 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// Process the timers related to upgrades. Specifically, the upgrade go ahead signals toggle
-	/// and the upgrade cooldown restrictions.
-	///
-	/// Takes the current block number and returns the weight consumed.
+	/// and the upgrade cooldown restrictions. However, this function does not actually unset
+	/// the upgrade restriction, that will happen in the `initializer_finalize` function. However,
+	/// this function does count the number of cooldown timers expired so that we can reserve weight
+	/// for the `initializer_finalize` function.
 	fn process_scheduled_upgrade_changes(now: T::BlockNumber) -> Weight {
+		// account weight for `UpcomingUpgrades::mutate`.
+		let mut weight = T::DbWeight::get().reads_writes(1, 1);
 		let upgrades_signaled = <Self as Store>::UpcomingUpgrades::mutate(
 			|upcoming_upgrades: &mut Vec<(ParaId, T::BlockNumber)>| {
 				let num = upcoming_upgrades.iter().take_while(|&(_, at)| at <= &now).count();
@@ -1164,17 +1169,35 @@ impl<T: Config> Pallet<T> {
 				num
 			},
 		);
-		let cooldowns_expired = <Self as Store>::UpgradeCooldowns::mutate(
+		weight += T::DbWeight::get().writes(upgrades_signaled as u64);
+
+		// account weight for `UpgradeCooldowns::get`.
+		weight += T::DbWeight::get().reads(1);
+		let cooldowns_expired = <Self as Store>::UpgradeCooldowns::get()
+			.iter()
+			.take_while(|&(_, at)| at <= &now)
+			.count();
+
+		// reserve weight for `initializer_finalize`:
+		// - 1 read and 1 write for `UpgradeCooldowns::mutate`.
+		// - 1 write per expired cooldown.
+		weight += T::DbWeight::get().reads_writes(1, 1);
+		weight += T::DbWeight::get().reads(cooldowns_expired as u64);
+
+		weight
+	}
+
+	/// Actually perform unsetting the expired upgrade restrictions.
+	///
+	/// See `process_scheduled_upgrade_changes` for more details.
+	fn process_scheduled_upgrade_cooldowns(now: T::BlockNumber) {
+		<Self as Store>::UpgradeCooldowns::mutate(
 			|upgrade_cooldowns: &mut Vec<(ParaId, T::BlockNumber)>| {
-				let num = upgrade_cooldowns.iter().take_while(|&(_, at)| at <= &now).count();
-				for (para, _) in upgrade_cooldowns.drain(..num) {
+				for &(para, _) in upgrade_cooldowns.iter().take_while(|&(_, at)| at <= &now) {
 					<Self as Store>::UpgradeRestrictionSignal::remove(&para);
 				}
-				num
 			},
 		);
-
-		T::DbWeight::get().reads_writes(2, upgrades_signaled as u64 + cooldowns_expired as u64)
 	}
 
 	/// Goes over all PVF votes in progress, reinitializes ballots, increments ages and prunes the
@@ -1896,7 +1919,7 @@ mod tests {
 
 		while System::block_number() < to {
 			let b = System::block_number();
-			Paras::initializer_finalize();
+			Paras::initializer_finalize(b);
 			ParasShared::initializer_finalize();
 			if new_session.as_ref().map_or(false, |v| v.contains(&(b + 1))) {
 				let mut session_change_notification = SessionChangeNotification::default();
@@ -2421,6 +2444,7 @@ mod tests {
 			// We expect that if an upgrade is signalled while there is already one pending we just
 			// ignore it. Note that this is only true from perspective of this module.
 			run_to_block(2, None);
+			assert!(!Paras::can_upgrade_validation_code(para_id));
 			Paras::schedule_code_upgrade(para_id, newer_code.clone(), 2, &Configuration::config());
 			assert_eq!(
 				<Paras as Store>::FutureCodeUpgrades::get(&para_id),
@@ -2431,6 +2455,79 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn upgrade_restriction_elapsed_doesnt_mean_can_upgrade() {
+		// Situation: parachain scheduled upgrade but it doesn't produce any candidate after
+		// `expected_at`. When `validation_upgrade_frequency` elapsed the parachain produces a
+		// candidate that tries to upgrade the code.
+		//
+		// In the current code this is not allowed: the upgrade should be consumed first. This is
+		// 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.
+
+		let code_retention_period = 10;
+		let validation_upgrade_delay = 7;
+		let validation_upgrade_frequency = 30;
+
+		let paras = vec![(
+			0u32.into(),
+			ParaGenesisArgs {
+				parachain: true,
+				genesis_head: dummy_head_data(),
+				validation_code: vec![1, 2, 3].into(),
+			},
+		)];
+
+		let genesis_config = MockGenesisConfig {
+			paras: GenesisConfig { paras, ..Default::default() },
+			configuration: crate::configuration::GenesisConfig {
+				config: HostConfiguration {
+					code_retention_period,
+					validation_upgrade_delay,
+					validation_upgrade_frequency,
+					pvf_checking_enabled: false,
+					..Default::default()
+				},
+				..Default::default()
+			},
+			..Default::default()
+		};
+
+		new_test_ext(genesis_config).execute_with(|| {
+			let para_id = 0u32.into();
+			let new_code = ValidationCode(vec![4, 5, 6]);
+			let newer_code = ValidationCode(vec![4, 5, 6, 7]);
+
+			run_to_block(1, None);
+			Paras::schedule_code_upgrade(para_id, new_code.clone(), 0, &Configuration::config());
+			Paras::note_new_head(para_id, dummy_head_data(), 0);
+			assert_eq!(
+				<Paras as Store>::UpgradeRestrictionSignal::get(&para_id),
+				Some(UpgradeRestriction::Present),
+			);
+			assert_eq!(
+				<Paras as Store>::FutureCodeUpgrades::get(&para_id),
+				Some(0 + validation_upgrade_delay)
+			);
+			assert!(!Paras::can_upgrade_validation_code(para_id));
+
+			run_to_block(31, None);
+			assert!(<Paras as Store>::UpgradeRestrictionSignal::get(&para_id).is_none());
+
+			// Note the para still cannot upgrade the validation code.
+			assert!(!Paras::can_upgrade_validation_code(para_id));
+
+			// And scheduling another upgrade does not do anything. `expected_at` is still the same.
+			Paras::schedule_code_upgrade(para_id, newer_code.clone(), 30, &Configuration::config());
+			assert_eq!(
+				<Paras as Store>::FutureCodeUpgrades::get(&para_id),
+				Some(0 + validation_upgrade_delay)
+			);
+		});
+	}
+
 	#[test]
 	fn full_parachain_cleanup_storage() {
 		let code_retention_period = 20;

runtime/parachains/src/scheduler.rs

@@ -797,7 +797,7 @@ mod tests {
 			let b = System::block_number();
 
 			Scheduler::initializer_finalize();
-			Paras::initializer_finalize();
+			Paras::initializer_finalize(b);
 
 			if let Some(notification) = new_session(b + 1) {
 				let mut notification_with_session_index = notification;
@@ -831,7 +831,7 @@ mod tests {
 		run_to_block(to, &new_session);
 
 		Scheduler::initializer_finalize();
-		Paras::initializer_finalize();
+		Paras::initializer_finalize(to);
 
 		if let Some(notification) = new_session(to + 1) {
 			Paras::initializer_on_new_session(&notification);